dht: Inconsistent permission for directories after brick stop/start

Problem: Inconsistent access permissions on directories after
         bringing back the down sub-volumes, in case of directories dht_setattr
         first wind a call on MDS once call is finished on MDS then wind a call
         on NON-MDS.At the time of revalidating dht just compare the uid/gid with
         stbuf uid/gid and if anyone differs set a flag to heal the same.

Solution: Add a condition to compare permission also in dht_revalidate_cbk
          to set a flag to call dht_dir_attr_heal.

BUG: 1584517
Change-Id: I3e039607148005015b5d93364536158380d4c5aa
fixes: bz#1584517
Signed-off-by: Mohit Agrawal <moagrawa@redhat.com>

4 files changed, 147 insertions, 9 deletions

diff --git a/tests/bugs/bug-1584517.t b/tests/bugs/bug-1584517.t
new file mode 100644
index 00000000000..7f48015a034
--- /dev/null
+++ b/tests/bugs/bug-1584517.t
@@ -0,0 +1,70 @@
+#!/bin/bash
+. $(dirname $0)/../include.rc
+. $(dirname $0)/../volume.rc
+. $(dirname $0)/../dht.rc
+cleanup;
+#This test case verifies attributes (uid/gid/perm) for the
+#directory are healed after stop/start brick. To verify the same
+#test case change attributes of the directory after down a DHT subvolume
+#and one AFR children. After start the volume with force and run lookup
+#operation attributes should be healed on started bricks at the backend.
+
+
+TEST glusterd
+TEST pidof glusterd
+TEST $CLI volume create $V0 replica 3 $H0:$B0/${V0}{0,1,2,3,4,5}
+TEST $CLI volume start $V0
+TEST useradd dev -M
+TEST groupadd QA
+
+TEST glusterfs --volfile-id=$V0 --volfile-server=$H0 $M0;
+
+TEST mkdir $M0/dironedown
+
+TEST kill_brick $V0 $H0 $B0/${V0}2
+EXPECT_WITHIN ${PROCESS_UP_TIMEOUT} "5" online_brick_count
+
+TEST kill_brick $V0 $H0 $B0/${V0}3
+EXPECT_WITHIN ${PROCESS_UP_TIMEOUT} "4" online_brick_count
+
+TEST kill_brick $V0 $H0 $B0/${V0}4
+EXPECT_WITHIN ${PROCESS_UP_TIMEOUT} "3" online_brick_count
+
+TEST kill_brick $V0 $H0 $B0/${V0}5
+EXPECT_WITHIN ${PROCESS_UP_TIMEOUT} "2" online_brick_count
+
+TEST chown dev $M0/dironedown
+TEST chgrp QA $M0/dironedown
+TEST chmod 777 $M0/dironedown
+
+#store the permissions for comparision
+permission_onedown=`ls -l $M0 | grep dironedown | awk '{print $1}'`
+
+TEST $CLI volume start $V0 force
+EXPECT_WITHIN ${PROCESS_UP_TIMEOUT} "6" online_brick_count
+
+TEST glusterfs --volfile-id=$V0 --volfile-server=$H0 $M0;
+
+#Run lookup two times to hit revalidate code path in dht
+# to heal user attr
+
+TEST ls $M0/dironedown
+
+#check attributes those were created post brick going down
+TEST brick_perm=`ls -l $B0/${V0}3 | grep dironedown | awk '{print $1}'`
+TEST echo $brick_perm
+TEST [ ${brick_perm} = ${permission_onedown} ]
+uid=`ls -l $B0/${V0}3 | grep dironedown | awk '{print $3}'`
+TEST echo $uid
+TEST [ $uid = dev ]
+gid=`ls -l $B0/${V0}3 | grep dironedown | awk '{print $4}'`
+TEST echo $gid
+TEST [ $gid = QA ]
+
+TEST umount $M0
+userdel --force dev
+groupdel QA
+
+cleanup
+exit
+
diff --git a/xlators/cluster/dht/src/dht-common.c b/xlators/cluster/dht/src/dht-common.c
index 07f00b4a92d..900789ca8aa 100644
--- a/xlators/cluster/dht/src/dht-common.c
+++ b/xlators/cluster/dht/src/dht-common.c
@@ -1340,6 +1340,8 @@ dht_lookup_dir_cbk (call_frame_t *frame, void *cookie, xlator_t *this,
         char          gfid_local[GF_UUID_BUF_SIZE] = {0};
         char          gfid_node[GF_UUID_BUF_SIZE]  = {0};
         int32_t       mds_xattr_val[1]                 = {0};
+        call_frame_t *copy                         = NULL;
+        dht_local_t  *copy_local                   = NULL;
 
         GF_VALIDATE_OR_GOTO ("dht", frame, out);
         GF_VALIDATE_OR_GOTO ("dht", this, out);
@@ -1412,6 +1414,23 @@ dht_lookup_dir_cbk (call_frame_t *frame, void *cookie, xlator_t *this,
                         dht_aggregate_xattr (local->xattr, xattr);
                 }
 
+                if (dict_get (xattr, conf->mds_xattr_key)) {
+                        local->mds_subvol  = prev;
+                        local->mds_stbuf.ia_gid = stbuf->ia_gid;
+                        local->mds_stbuf.ia_uid = stbuf->ia_uid;
+                        local->mds_stbuf.ia_prot = stbuf->ia_prot;
+                }
+
+                if (local->stbuf.ia_type != IA_INVAL) {
+                        if (!__is_root_gfid (stbuf->ia_gfid) &&
+                            ((local->stbuf.ia_gid != stbuf->ia_gid) ||
+                            (local->stbuf.ia_uid != stbuf->ia_uid) ||
+                            (is_permission_different (&local->stbuf.ia_prot,
+                                                      &stbuf->ia_prot)))) {
+                                local->need_attrheal = 1;
+                        }
+                }
+
                 if (local->inode == NULL)
                         local->inode = inode_ref (inode);
 
@@ -1507,6 +1526,43 @@ unlock:
                                                    &local->postparent, 1);
                 }
 
+                if (local->need_attrheal) {
+                        local->need_attrheal = 0;
+                        if (!__is_root_gfid (inode->gfid)) {
+                                gf_uuid_copy (local->gfid, local->mds_stbuf.ia_gfid);
+                                local->stbuf.ia_gid = local->mds_stbuf.ia_gid;
+                                local->stbuf.ia_uid = local->mds_stbuf.ia_uid;
+                                local->stbuf.ia_prot = local->mds_stbuf.ia_prot;
+                        }
+                        copy = create_frame (this, this->ctx->pool);
+                        if (copy) {
+                                copy_local = dht_local_init (copy, &local->loc,
+                                                             NULL, 0);
+                                if (!copy_local) {
+                                        DHT_STACK_DESTROY (copy);
+                                        goto skip_attr_heal;
+                                }
+                                copy_local->stbuf = local->stbuf;
+                                copy_local->mds_stbuf = local->mds_stbuf;
+                                copy_local->mds_subvol = local->mds_subvol;
+                                copy->local = copy_local;
+                                FRAME_SU_DO (copy, dht_local_t);
+                                ret = synctask_new (this->ctx->env,
+                                                    dht_dir_attr_heal,
+                                                    dht_dir_attr_heal_done,
+                                                    copy, copy);
+                                if (ret) {
+                                        gf_msg (this->name, GF_LOG_ERROR, ENOMEM,
+                                                DHT_MSG_DIR_ATTR_HEAL_FAILED,
+                                                "Synctask creation failed to heal attr "
+                                                "for path %s gfid %s ",
+                                                local->loc.path, local->gfid);
+                                        DHT_STACK_DESTROY (copy);
+                                }
+                        }
+                }
+
+skip_attr_heal:
                 DHT_STRIP_PHASE1_FLAGS (&local->stbuf);
                 dht_set_fixed_dir_stat (&local->postparent);
                 /* Delete mds xattr at the time of STACK UNWIND */
@@ -1527,7 +1583,7 @@ out:
         return ret;
 }
 
-int static
+int
 is_permission_different (ia_prot_t *prot1, ia_prot_t *prot2)
 {
         if ((prot1->owner.read != prot2->owner.read) ||
@@ -1688,12 +1744,12 @@ dht_revalidate_cbk (call_frame_t *frame, void *cookie, xlator_t *this,
                         {
                                 if ((local->stbuf.ia_gid != stbuf->ia_gid) ||
                                     (local->stbuf.ia_uid != stbuf->ia_uid) ||
-                                    (__is_root_gfid (stbuf->ia_gfid) &&
                                      is_permission_different (&local->stbuf.ia_prot,
-                                                              &stbuf->ia_prot))) {
+                                                              &stbuf->ia_prot)) {
                                         local->need_selfheal = 1;
                                 }
                         }
+
                         if (!dict_get (xattr, conf->mds_xattr_key)) {
                                 gf_msg_debug (this->name, 0,
                                               "internal xattr %s is not present"
@@ -1839,10 +1895,9 @@ out:
                         local->need_selfheal = 0;
                         if (!__is_root_gfid (inode->gfid)) {
                                 gf_uuid_copy (local->gfid, local->mds_stbuf.ia_gfid);
-                                if (local->mds_stbuf.ia_gid || local->mds_stbuf.ia_uid) {
-                                        local->stbuf.ia_gid = local->mds_stbuf.ia_gid;
-                                        local->stbuf.ia_uid = local->mds_stbuf.ia_uid;
-                                }
+                                local->stbuf.ia_gid = local->mds_stbuf.ia_gid;
+                                local->stbuf.ia_uid = local->mds_stbuf.ia_uid;
+                                local->stbuf.ia_prot = local->mds_stbuf.ia_prot;
                         } else {
                                 gf_uuid_copy (local->gfid, local->stbuf.ia_gfid);
                                 local->stbuf.ia_gid = local->prebuf.ia_gid;
@@ -1854,8 +1909,10 @@ out:
                         if (copy) {
                                 copy_local = dht_local_init (copy, &local->loc,
                                                              NULL, 0);
-                                if (!copy_local)
+                                if (!copy_local) {
+                                        DHT_STACK_DESTROY (copy);
                                         goto cont;
+                                }
                                 copy_local->stbuf = local->stbuf;
                                 copy_local->mds_stbuf = local->mds_stbuf;
                                 copy_local->mds_subvol = local->mds_subvol;
@@ -1865,6 +1922,14 @@ out:
                                                     dht_dir_attr_heal,
                                                     dht_dir_attr_heal_done,
                                                     copy, copy);
+                                if (ret) {
+                                        gf_msg (this->name, GF_LOG_ERROR, ENOMEM,
+                                                DHT_MSG_DIR_ATTR_HEAL_FAILED,
+                                                "Synctask creation failed to heal attr "
+                                                "for path %s gfid %s ",
+                                                local->loc.path, local->gfid);
+                                        DHT_STACK_DESTROY (copy);
+                                }
                         }
                 }
 cont:
diff --git a/xlators/cluster/dht/src/dht-common.h b/xlators/cluster/dht/src/dht-common.h
index 43bceb9fd2d..02560ef94ab 100644
--- a/xlators/cluster/dht/src/dht-common.h
+++ b/xlators/cluster/dht/src/dht-common.h
@@ -298,6 +298,7 @@ struct dht_local {
         xlator_t                *mds_subvol; /* This is use for dir only */
         char                     need_selfheal;
         char                     need_xattr_heal;
+        char                     need_attrheal;
         int                      file_count;
         int                      dir_count;
         call_frame_t            *main_frame;
@@ -1488,4 +1489,6 @@ int dht_request_iatt_in_xdata (xlator_t *this, dict_t *xattr_req);
 int dht_read_iatt_from_xdata (xlator_t *this, dict_t *xdata,
                               struct iatt *stbuf);
 
+int
+is_permission_different (ia_prot_t *prot1, ia_prot_t *prot2);
 #endif/* _DHT_H */
diff --git a/xlators/cluster/dht/src/dht-selfheal.c b/xlators/cluster/dht/src/dht-selfheal.c
index 6425bdc506e..2aa75bcc4cd 100644
--- a/xlators/cluster/dht/src/dht-selfheal.c
+++ b/xlators/cluster/dht/src/dht-selfheal.c
@@ -2500,7 +2500,7 @@ dht_dir_attr_heal (void *data)
                                               NULL, NULL, NULL, NULL);
                 } else {
                         ret = syncop_setattr (subvol, &local->loc, &local->mds_stbuf,
-                                              (GF_SET_ATTR_UID | GF_SET_ATTR_GID),
+                                              (GF_SET_ATTR_UID | GF_SET_ATTR_GID | GF_SET_ATTR_MODE),
                                               NULL, NULL, NULL, NULL);
                 }