replicate: don't stop checking xattrs because one was absent

The functional issue is described by the subject line.  This patch also
addresses several efficiency/structure issues, such as...

* Calling dict_set_ptr once for each txn type, instead of once overall.

* Calling afr_index_for_transaction_type once per iteration instead of
  once per call (or better yet zero since the conversion is unnecessary).

* Implementation of inner functions in a different file than their one
  caller, creating a spurious header-file dependency.

Change-Id: I29e0df906a820533b66b9ced73e015dfe77267d2
BUG: 865825
Signed-off-by: Jeff Darcy <jdarcy@redhat.com>
Reviewed-on: http://review.gluster.org/4070
Tested-by: Gluster Build System <jenkins@build.gluster.com>
Reviewed-by: Pranith Kumar Karampuri <pkarampu@redhat.com>
Reviewed-by: Anand Avati <avati@redhat.com>

4 files changed, 115 insertions, 114 deletions

diff --git a/tests/bugs/bug-865825.t b/tests/bugs/bug-865825.t
new file mode 100755
index 00000000000..82e1476f7b0
--- /dev/null
+++ b/tests/bugs/bug-865825.t
@@ -0,0 +1,74 @@
+#!/bin/bash
+
+. $(dirname $0)/../include.rc
+
+TEST glusterd
+TEST pidof glusterd
+TEST $CLI volume info;
+
+## Start and create a volume
+mkdir -p ${B0}/${V0}-0
+mkdir -p ${B0}/${V0}-1
+mkdir -p ${B0}/${V0}-2
+TEST $CLI volume create $V0 replica 3 $H0:$B0/${V0}-{0,1,2}
+
+function volinfo_field()
+{
+    local vol=$1;
+    local field=$2;
+
+    $CLI volume info $vol | grep "^$field: " | sed 's/.*: //';
+}
+
+
+## Verify volume is created
+EXPECT "$V0" volinfo_field $V0 'Volume Name';
+EXPECT 'Created' volinfo_field $V0 'Status';
+
+## Make sure io-cache and write-behind don't interfere.
+TEST $CLI volume set $V0 performance.io-cache off;
+TEST $CLI volume set $V0 performance.write-behind off;
+
+## Make sure automatic self-heal doesn't perturb our results.
+TEST $CLI volume set $V0 cluster.self-heal-daemon off
+
+## Start volume and verify
+TEST $CLI volume start $V0;
+EXPECT 'Started' volinfo_field $V0 'Status';
+
+## Mount native
+TEST glusterfs --volfile-server=$H0 --volfile-id=$V0 $M0
+
+## Create a file with some recognizable contents.
+echo "test_data" > $M0/a_file;
+
+## Unmount.
+TEST umount $M0;
+
+## Mess with the flags as though brick-0 accuses brick-2 while brick-1 is
+## missing its brick-2 changelog altogether.
+value=0x000000010000000000000000
+setfattr -n trusted.afr.${V0}-client-2 -v $value $B0/${V0}-0/a_file
+setfattr -x trusted.afr.${V0}-client-2 $B0/${V0}-1/a_file
+echo "wrong_data" > $B0/${V0}-2/a_file
+
+## Remount and force a self-heal.
+TEST glusterfs --volfile-server=$H0 --volfile-id=$V0 $M0
+stat ${M0}/a_file > /dev/null
+
+## Make sure brick 2 now has the correct contents.
+EXPECT "test_data" cat $B0/${V0}-2/a_file
+
+if [ "$EXIT_EARLY" = "1" ]; then
+	exit 0;
+fi
+
+## Finish up
+TEST umount $M0;
+TEST $CLI volume stop $V0;
+EXPECT 'Stopped' volinfo_field $V0 'Status';
+
+TEST $CLI volume delete $V0;
+TEST ! $CLI volume info $V0;
+
+cleanup;
diff --git a/xlators/cluster/afr/src/afr-common.c b/xlators/cluster/afr/src/afr-common.c
index 693c3a070a2..a1dd4a5ce57 100644
--- a/xlators/cluster/afr/src/afr-common.c
+++ b/xlators/cluster/afr/src/afr-common.c
@@ -1157,31 +1157,57 @@ afr_lookup_update_lk_counts (afr_local_t *local, xlator_t *this,
                 local->cont.lookup.parent_entrylk += parent_entrylk;
 }
 
+/*
+ * It's important to maintain a commutative property on do_*_self_heal and
+ * found*; once set, they must not be cleared by a subsequent iteration or
+ * call, so that they represent a logical OR of all iterations and calls
+ * regardless of child/key order.  That allows the caller to call us multiple
+ * times without having to use a separate variable as a "reduce" accumulator.
+ */
 static void
 afr_lookup_set_self_heal_params_by_xattr (afr_local_t *local, xlator_t *this,
                                           dict_t *xattr)
 {
+        afr_private_t *priv        = NULL;
+        int            i           = 0;
+        int            ret         = -1;
+        void          *pending_raw = NULL;
+        int32_t       *pending     = NULL;
+
         GF_ASSERT (local);
         GF_ASSERT (this);
         GF_ASSERT (xattr);
 
-        if (afr_sh_has_metadata_pending (xattr, this)) {
-                local->self_heal.do_metadata_self_heal = _gf_true;
-                gf_log(this->name, GF_LOG_DEBUG,
-                       "metadata self-heal is pending for %s.",
-                       local->loc.path);
-        }
+        priv = this->private;
 
-        if (afr_sh_has_entry_pending (xattr, this)) {
-                local->self_heal.do_entry_self_heal = _gf_true;
-                gf_log(this->name, GF_LOG_DEBUG,
-                       "entry self-heal is pending for %s.", local->loc.path);
-        }
+        for (i = 0; i < priv->child_count; i++) {
+                ret = dict_get_ptr (xattr, priv->pending_key[i],
+                                    &pending_raw);
+                if (ret != 0) {
+                        continue;
+                }
+                pending = pending_raw;
 
-        if (afr_sh_has_data_pending (xattr, this)) {
-                local->self_heal.do_data_self_heal = _gf_true;
-                gf_log(this->name, GF_LOG_DEBUG,
-                       "data self-heal is pending for %s.", local->loc.path);
+                if (pending[AFR_METADATA_TRANSACTION]) {
+                        gf_log(this->name, GF_LOG_DEBUG,
+                               "metadata self-heal is pending for %s.",
+                               local->loc.path);
+                        local->self_heal.do_metadata_self_heal = _gf_true;
+                }
+
+                if (pending[AFR_ENTRY_TRANSACTION]) {
+                        gf_log(this->name, GF_LOG_DEBUG,
+                               "entry self-heal is pending for %s.",
+                               local->loc.path);
+                        local->self_heal.do_entry_self_heal = _gf_true;
+                }
+
+                if (pending[AFR_DATA_TRANSACTION]) {
+                        gf_log(this->name, GF_LOG_DEBUG,
+                               "data self-heal is pending for %s.",
+                               local->loc.path);
+                        local->self_heal.do_data_self_heal = _gf_true;
+                }
         }
 }
 
diff --git a/xlators/cluster/afr/src/afr-self-heal-common.c b/xlators/cluster/afr/src/afr-self-heal-common.c
index 49484bf7b7d..07603c5b2a2 100644
--- a/xlators/cluster/afr/src/afr-self-heal-common.c
+++ b/xlators/cluster/afr/src/afr-self-heal-common.c
@@ -904,98 +904,6 @@ afr_sh_delta_to_xattr (xlator_t *this,
 
 
 int
-afr_sh_has_metadata_pending (dict_t *xattr, xlator_t *this)
-{
-        /* Indexable by result of afr_index_for_transaction_type(): 0 -- 2. */
-        int32_t        pending[3]  = {0,};
-        void          *pending_raw = NULL;
-        afr_private_t *priv        = NULL;
-        int            ret         = -1;
-        int            i           = 0;
-        int            j           = 0;
-
-        priv = this->private;
-
-        for (i = 0; i < priv->child_count; i++) {
-                ret = dict_get_ptr (xattr, priv->pending_key[i],
-                                    &pending_raw);
-
-                if (ret != 0)
-                        return 0;
-
-                memcpy (pending, pending_raw, sizeof(pending));
-                j = afr_index_for_transaction_type (AFR_METADATA_TRANSACTION);
-
-                if (pending[j])
-                        return 1;
-        }
-
-        return 0;
-}
-
-
-int
-afr_sh_has_data_pending (dict_t *xattr, xlator_t *this)
-{
-        /* Indexable by result of afr_index_for_transaction_type(): 0 -- 2. */
-        int32_t        pending[3]  = {0,};
-        void          *pending_raw = NULL;
-        afr_private_t *priv        = NULL;
-        int            ret         = -1;
-        int            i           = 0;
-        int            j           = 0;
-
-        priv = this->private;
-
-        for (i = 0; i < priv->child_count; i++) {
-                ret = dict_get_ptr (xattr, priv->pending_key[i],
-                                    &pending_raw);
-
-                if (ret != 0)
-                        return 0;
-
-                memcpy (pending, pending_raw, sizeof(pending));
-                j = afr_index_for_transaction_type (AFR_DATA_TRANSACTION);
-
-                if (pending[j])
-                        return 1;
-        }
-
-        return 0;
-}
-
-
-int
-afr_sh_has_entry_pending (dict_t *xattr, xlator_t *this)
-{
-        /* Indexable by result of afr_index_for_transaction_type(): 0 -- 2. */
-        int32_t        pending[3]  = {0,};
-        void          *pending_raw = NULL;
-        afr_private_t *priv        = NULL;
-        int            ret         = -1;
-        int            i           = 0;
-        int            j           = 0;
-
-        priv = this->private;
-
-        for (i = 0; i < priv->child_count; i++) {
-                ret = dict_get_ptr (xattr, priv->pending_key[i],
-                                    &pending_raw);
-
-                if (ret != 0)
-                        return 0;
-
-                memcpy (pending, pending_raw, sizeof(pending));
-                j = afr_index_for_transaction_type (AFR_ENTRY_TRANSACTION);
-
-                if (pending[j])
-                        return 1;
-        }
-
-        return 0;
-}
-
-int
 afr_sh_missing_entries_done (call_frame_t *frame, xlator_t *this)
 {
         afr_local_t     *local = NULL;
diff --git a/xlators/cluster/afr/src/afr-self-heal.h b/xlators/cluster/afr/src/afr-self-heal.h
index 2efc1116d33..7c9bc81119c 100644
--- a/xlators/cluster/afr/src/afr-self-heal.h
+++ b/xlators/cluster/afr/src/afr-self-heal.h
@@ -21,13 +21,6 @@
 #define SIZE_GREATER(buf1,buf2) ((buf1)->ia_size > (buf2)->ia_size)
 
 int
-afr_sh_has_metadata_pending (dict_t *xattr, xlator_t *this);
-int
-afr_sh_has_entry_pending (dict_t *xattr, xlator_t *this);
-int
-afr_sh_has_data_pending (dict_t *xattr, xlator_t *this);
-
-int
 afr_self_heal_entry (call_frame_t *frame, xlator_t *this);
 
 int