From 620158475f462251c996901a8e24306ef6cb4c42 Mon Sep 17 00:00:00 2001
From: Barak Sason Rofman <bsasonro@redhat.com>
Date: Sun, 21 Jun 2020 14:38:56 +0300
Subject: dht - fixing xattr inconsistency

The scenario of setting an xattr to a dir, killing one of the bricks,
removing the xattr, bringing back the brick results in xattr
inconsistency - The downed brick will still have the xattr, but the rest
won't.
This patch add a mechanism that will remove the extra xattrs during
lookup.

fixes: #1324
Change-Id: Ibcc449bad6c7cb46bcae380e42e4496d733b453d
Signed-off-by: Barak Sason Rofman <bsasonro@redhat.com>
---
 tests/bugs/distribute/bug-1600379.t    | 54 +++++++++++++++++++++++++++++
 xlators/cluster/dht/src/dht-selfheal.c | 63 ++++++++++++++++++++++++++++++++++
 2 files changed, 117 insertions(+)
 create mode 100644 tests/bugs/distribute/bug-1600379.t

diff --git a/tests/bugs/distribute/bug-1600379.t b/tests/bugs/distribute/bug-1600379.t
new file mode 100644
index 00000000000..3c98535fe27
--- /dev/null
+++ b/tests/bugs/distribute/bug-1600379.t
@@ -0,0 +1,54 @@
+#!/bin/bash
+
+. $(dirname $0)/../../include.rc
+. $(dirname $0)/../../volume.rc
+
+# Initialize
+#------------------------------------------------------------
+cleanup;
+
+# Start glusterd
+TEST glusterd;
+TEST pidof glusterd;
+TEST $CLI volume info;
+
+# Create a volume
+TEST $CLI volume create $V0 $H0:$B0/${V0}{1,2}
+
+# Verify volume creation
+EXPECT "$V0" volinfo_field $V0 'Volume Name';
+EXPECT 'Created' volinfo_field $V0 'Status';
+
+# Start volume and verify successful start
+TEST $CLI volume start $V0;
+EXPECT 'Started' volinfo_field $V0 'Status';
+TEST glusterfs --volfile-id=$V0 --volfile-server=$H0 --entry-timeout=0 $M0;
+#------------------------------------------------------------
+
+# Test case - Subvolume down + Healing
+#------------------------------------------------------------
+# Create a dir and set custom xattr
+TEST mkdir $M0/testdir
+TEST setfattr -n user.attr -v val $M0/testdir
+xattr_val=`getfattr -d $B0/${V0}2/testdir | awk '{print $1}'`;
+TEST ${xattr_val}='user.attr="val"';
+
+# Kill 2nd brick process
+TEST kill_brick $V0 $H0 $B0/${V0}2
+EXPECT_WITHIN ${PROCESS_UP_TIMEOUT} "1" online_brick_count
+
+# Remove custom xattr
+TEST setfattr -x user.attr $M0/testdir
+
+# Bring up the killed brick process
+TEST $CLI volume start $V0 force
+
+# Perform lookup
+sleep 5
+TEST ls $M0/testdir
+
+# Check brick xattrs
+xattr_val_2=`getfattr -d $B0/${V0}2/testdir`;
+TEST [ ${xattr_val_2} = ''] ;
+
+cleanup;
\ No newline at end of file
diff --git a/xlators/cluster/dht/src/dht-selfheal.c b/xlators/cluster/dht/src/dht-selfheal.c
index 6004ab09ff6..dfc44dbbb47 100644
--- a/xlators/cluster/dht/src/dht-selfheal.c
+++ b/xlators/cluster/dht/src/dht-selfheal.c
@@ -2053,6 +2053,61 @@ dht_selfheal_restore(call_frame_t *frame, dht_selfheal_dir_cbk_t dir_cbk,
     return ret;
 }
 
+/* List xattrs of a subvols and compare with mds xattrs.
+ * If there are differences, remove extra xattrs from the subvol */
+int
+dht_dir_remove_extra_xattrs(xlator_t *this, xlator_t *subvol,
+                            dht_local_t *local, dict_t *mds_xattr)
+{
+    dict_t *subvol_xattr = NULL;
+    data_pair_t *subvol_xattrs = NULL;
+    data_pair_t *mds_xattrs = NULL;
+    int ret = 0;
+
+    /* List the xattrs of the subvol */
+    ret = syncop_listxattr(subvol, &local->loc, &subvol_xattr, NULL, NULL);
+    if (ret < 0) {
+        gf_smsg(this->name, GF_LOG_ERROR, -ret, DHT_MSG_LIST_XATTRS_FAILED,
+                "path=%s", local->loc.path, "name=%s", local->mds_subvol->name,
+                NULL);
+        goto out;
+    }
+
+    subvol_xattrs = subvol_xattr->members_list;
+
+    /* While the subvol still has xattrs, try to find a matching xattrs in mds
+     */
+    while (subvol_xattrs != NULL) {
+        mds_xattrs = mds_xattr->members_list;
+
+        /* While mds still has xattrs, check if the current xattrs is a match */
+        while (mds_xattrs != NULL &&
+               strcmp(subvol_xattrs->key, mds_xattrs->key) != 0) {
+            mds_xattrs = mds_xattrs->next;
+        }
+
+        if (mds_xattrs == NULL) {
+            /* No xattrs matched, remove the extra xattrs from the subvol and
+             * it's dict */
+            ret = syncop_removexattr(subvol, &local->loc, subvol_xattrs->key,
+                                     NULL, NULL);
+            if (ret < 0) {
+                gf_msg(this->name, GF_LOG_ERROR, -ret, 0,
+                       "%s: removexattr failed key %s", local->loc.path,
+                       subvol_xattrs->key);
+                goto out;
+            }
+
+            dict_del(subvol_xattr, subvol_xattrs->key);
+        }
+
+        subvol_xattrs = subvol_xattrs->next;
+    }
+
+out:
+    return ret;
+}
+
 int
 dht_dir_heal_xattrs(void *data)
 {
@@ -2155,6 +2210,14 @@ dht_dir_heal_xattrs(void *data)
         subvol = conf->subvolumes[i];
         if (subvol == mds_subvol)
             continue;
+
+        ret = dht_dir_remove_extra_xattrs(this, subvol, local, mds_xattr);
+        if (ret < 0) {
+            gf_msg(this->name, GF_LOG_ERROR, -ret, 0,
+                   "removing extra xattrs from subvol %s failed on path %s",
+                   subvol->name, local->loc.path);
+        }
+
         if (uret || uflag) {
             ret = syncop_setxattr(subvol, &local->loc, user_xattr, 0, xdata,
                                   NULL);
-- 
cgit