Fix nasty rebuild bug where partiy would be reconstructed incorrectly

when both data and parity was missing.  The fix is to just call decode
when reconstructing parity, since it will have to do extra work anyway
when data is missing.  We did a little extra work in ISA-L to do better,
but can save that for later, since 99% of the time decode will perform just
fine.

3 files changed, 96 insertions, 48 deletions

diff --git a/src/backends/jerasure/jerasure_rs_cauchy.c b/src/backends/jerasure/jerasure_rs_cauchy.c
index 4728a14..307dbe9 100644
--- a/src/backends/jerasure/jerasure_rs_cauchy.c
+++ b/src/backends/jerasure/jerasure_rs_cauchy.c
@@ -137,7 +137,7 @@ static int jerasure_rs_cauchy_reconstruct(void *desc, char **data, char **parity
         int *missing_idxs, int destination_idx, int blocksize)
 {
     int k, m, w;                  /* erasure code paramters */
-    int ret = 1;                  /* return code */
+    int ret = 0;                  /* return code */
     int *decoding_row = NULL;     /* decoding matrix row for decode */
     int *erased = NULL;           /* k+m length list of erased frag ids */
     int *dm_ids = NULL;           /* k length list of fragment ids */
@@ -149,29 +149,46 @@ static int jerasure_rs_cauchy_reconstruct(void *desc, char **data, char **parity
     m = jerasure_desc->m;
     w = jerasure_desc->w;
 
-    dm_ids = (int *) alloc_zeroed_buffer(sizeof(int) * k);
-    decoding_matrix = (int *) alloc_zeroed_buffer(sizeof(int *) * k * k * w * w);
-    erased = jerasure_desc->jerasure_erasures_to_erased(k, m, missing_idxs);
-    if (NULL == decoding_matrix || NULL == dm_ids || NULL == erased) {
-        goto out;
-    }
+    if (destination_idx < k) {
+        dm_ids = (int *) alloc_zeroed_buffer(sizeof(int) * k);
+        decoding_matrix = (int *) alloc_zeroed_buffer(sizeof(int *) * k * k * w * w);
+        erased = jerasure_desc->jerasure_erasures_to_erased(k, m, missing_idxs);
+        if (NULL == decoding_matrix || NULL == dm_ids || NULL == erased) {
+            goto out;
+        }
 
-    ret = jerasure_desc->jerasure_make_decoding_bitmatrix(k, m, w, 
+        ret = jerasure_desc->jerasure_make_decoding_bitmatrix(k, m, w, 
                                                jerasure_desc->bitmatrix,
                                                erased, decoding_matrix, dm_ids);
-    if (destination_idx < k) {
-        decoding_row = decoding_matrix + (destination_idx * k * w * w);
-    } else {
-        decoding_row = jerasure_desc->bitmatrix + ((destination_idx - k) * k * w * w);
-    }
+        if (ret == 0) {
+            decoding_row = decoding_matrix + (destination_idx * k * w * w);
    
-    if (ret == 0) {
-        jerasure_desc->jerasure_bitmatrix_dotprod(jerasure_desc->k, jerasure_desc->w, 
+            jerasure_desc->jerasure_bitmatrix_dotprod(jerasure_desc->k, jerasure_desc->w, 
                                    decoding_row, dm_ids, destination_idx,
-                                   data, parity, blocksize,
-                                   PYECC_CAUCHY_PACKETSIZE);
+                                   data, parity, blocksize, PYECC_CAUCHY_PACKETSIZE);
+        } else {
+           /*
+            * ToDo (KMG) I know this is not needed, but keeping to prevent future 
+            * memory leaks, as this function will be better optimized for decoding 
+            * missing parity 
+            */
+            goto out;
+        }
     } else {
-      goto out;
+        /*
+         * If it is parity we are reconstructing, then just call decode.
+         * ToDo (KMG): We can do better than this, but this should perform just
+         * fine for most cases.  We can adjust the decoding matrix like we
+         * did with ISA-L.
+         */
+        jerasure_desc->jerasure_bitmatrix_decode(k, m, w,
+                                             jerasure_desc->bitmatrix, 
+                                             0, 
+                                             missing_idxs,
+                                             data, 
+                                             parity, 
+                                             blocksize, 
+                                             PYECC_CAUCHY_PACKETSIZE); 
     }
 
 out:
diff --git a/src/backends/jerasure/jerasure_rs_vand.c b/src/backends/jerasure/jerasure_rs_vand.c
index 803bd95..ea2a846 100644
--- a/src/backends/jerasure/jerasure_rs_vand.c
+++ b/src/backends/jerasure/jerasure_rs_vand.c
@@ -108,7 +108,7 @@ static int jerasure_rs_vand_decode(void *desc, char **data, char **parity,
 static int jerasure_rs_vand_reconstruct(void *desc, char **data, char **parity,
         int *missing_idxs, int destination_idx, int blocksize)
 {
-    int ret = 1;                  /* return code */
+    int ret = 0;                  /* return code */
     int *decoding_row;            /* decoding matrix row for decode */
     int *erased = NULL;           /* k+m length list of erased frag ids */
     int *dm_ids = NULL;           /* k length list of frag ids */
@@ -117,35 +117,45 @@ static int jerasure_rs_vand_reconstruct(void *desc, char **data, char **parity,
     struct jerasure_rs_vand_descriptor *jerasure_desc = 
         (struct jerasure_rs_vand_descriptor*) desc;
     
-    dm_ids = (int *) alloc_zeroed_buffer(sizeof(int) * jerasure_desc->k);
-    decoding_matrix = (int *)
-        alloc_zeroed_buffer(sizeof(int*) * jerasure_desc->k * jerasure_desc->k);
-    if (NULL == decoding_matrix || NULL == dm_ids) {
-        goto out;
-    }
-    
-    erased = jerasure_desc->jerasure_erasures_to_erased(jerasure_desc->k,
-            jerasure_desc->m, missing_idxs);
-    if (NULL == erased) {
-        goto out;
-    }
-
-    ret = jerasure_desc->jerasure_make_decoding_matrix(jerasure_desc->k,
-            jerasure_desc->m, jerasure_desc->w, jerasure_desc->matrix,
-            erased, decoding_matrix, dm_ids);
     if (destination_idx < jerasure_desc->k) {
+        dm_ids = (int *) alloc_zeroed_buffer(sizeof(int) * jerasure_desc->k);
+        decoding_matrix = (int *)
+            alloc_zeroed_buffer(sizeof(int*) * jerasure_desc->k * jerasure_desc->k);
+        erased = jerasure_desc->jerasure_erasures_to_erased(jerasure_desc->k,
+                jerasure_desc->m, missing_idxs);
+        if (NULL == decoding_matrix || NULL == dm_ids || NULL == erased) {
+            goto out;
+        }
+
+        ret = jerasure_desc->jerasure_make_decoding_matrix(jerasure_desc->k,
+                jerasure_desc->m, jerasure_desc->w, jerasure_desc->matrix,
+                erased, decoding_matrix, dm_ids);
+
         decoding_row = decoding_matrix + (destination_idx * jerasure_desc->k);
-    } else {
-        decoding_row = jerasure_desc->matrix +
-            ((destination_idx - jerasure_desc->k) * jerasure_desc->k);
-    }
     
-    if (ret == 0) {
-        jerasure_desc->jerasure_matrix_dotprod(jerasure_desc->k,
-                jerasure_desc->w, decoding_row, dm_ids, destination_idx,
-                data, parity, blocksize);
+        if (ret == 0) {
+            jerasure_desc->jerasure_matrix_dotprod(jerasure_desc->k,
+                    jerasure_desc->w, decoding_row, dm_ids, destination_idx,
+                    data, parity, blocksize);
+        } else {
+            /*
+             * ToDo (KMG) I know this is not needed, but keeping to prevent future 
+             *  memory leaks, as this function will be better optimized for decoding 
+             * missing parity 
+             */
+            goto out;
+        }
     } else {
-      goto out;
+        /*
+         * If it is parity we are reconstructing, then just call decode.
+         * ToDo (KMG): We can do better than this, but this should perform just
+         * fine for most cases.  We can adjust the decoding matrix like we
+         * did with ISA-L.
+         */
+        jerasure_desc->jerasure_matrix_decode(jerasure_desc->k,
+                        jerasure_desc->m, jerasure_desc->w,
+                        jerasure_desc->matrix, 1, missing_idxs, data, parity, blocksize);
+        goto parity_reconstr_out;
     }
 
 out:
@@ -153,6 +163,7 @@ out:
     free(decoding_matrix);
     free(dm_ids);
 
+parity_reconstr_out:
     return ret;
 }
 
diff --git a/test/liberasurecode_test.c b/test/liberasurecode_test.c
index d1ad516..dc112f9 100644
--- a/test/liberasurecode_test.c
+++ b/test/liberasurecode_test.c
@@ -768,6 +768,17 @@ static void encode_decode_test_impl(const ec_backend_id_t be_id,
     free(avail_frags);
 }
 
+/**
+ * Note: this test will attempt to reconstruct a single fragment when
+ * one or more other fragments are missing (specified by skip).  
+ *
+ * For example, if skip is [0, 0, 0, 1, 0, 0] and we are reconstructing
+ * fragment 5, then it will test the reconstruction of fragment 5 when 3
+ * and 5 are assumed unavailable.
+ *
+ * We only mark at most 2 as unavailable, as we cannot guarantee every situation
+ * will be able to habndle 3 failures.
+ */
 static void reconstruct_test_impl(const ec_backend_id_t be_id,
                                  struct ec_args *args,
                                  int *skip)
@@ -796,15 +807,24 @@ static void reconstruct_test_impl(const ec_backend_id_t be_id,
     rc = liberasurecode_encode(desc, orig_data, orig_data_size,
             &encoded_data, &encoded_parity, &encoded_fragment_len);
     assert(rc == 0);
-    num_avail_frags = create_frags_array(&avail_frags, encoded_data,
-                                             encoded_parity, args, skip);
     out = malloc(encoded_fragment_len);
     assert(out != NULL);
     for (i = 0; i < num_fragments; i++) {
+        char *cmp = NULL;
+        // If the current fragment was not chosen as fragments to skip,
+        // remove it and the chosen fragments to skip from the available list
+        // and reset its state
         if (skip[i] == 0) {
-            continue;
+            skip[i] = 1;
+            num_avail_frags = create_frags_array(&avail_frags, encoded_data,
+                                             encoded_parity, args, skip);
+            skip[i] = 0;
+        // Do not reset the skip state if the fragment was chosen as a fragment
+        // to skip for this invocation of the test
+        } else {
+            num_avail_frags = create_frags_array(&avail_frags, encoded_data,
+                                             encoded_parity, args, skip);
         }
-        char *cmp = NULL;
         if (i < args->k) {
             cmp = encoded_data[i];
         }