From 9a95de08eb49f10c0a342099826d5e4c445749aa Mon Sep 17 00:00:00 2001

From: Mohit Agrawal <moagrawa@redhat.com>

Date: Thu, 31 May 2018 12:29:35 +0530

Subject: [PATCH 313/325] 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

> (cherry picked from commit e57cbae0bcc3d8649b869eda5ec20f3c6a6d34f0)

> (Reviewed on upstream link https://review.gluster.org/#/c/20108/)

BUG: 1582066

Change-Id: I985445521aeeddce52c0a56c20287e523aa3398b

Signed-off-by: Mohit Agrawal <moagrawa@redhat.com>

Reviewed-on: https://code.engineering.redhat.com/gerrit/143721

Tested-by: RHGS Build Bot <nigelb@redhat.com>

Reviewed-by: Sunil Kumar Heggodu Gopala Acharya <sheggodu@redhat.com>

---

 tests/bugs/bug-1584517.t               | 70 +++++++++++++++++++++++++++++

 xlators/cluster/dht/src/dht-common.c   | 81 ++++++++++++++++++++++++++++++----

 xlators/cluster/dht/src/dht-common.h   |  3 ++

 xlators/cluster/dht/src/dht-selfheal.c |  2 +-

 4 files changed, 147 insertions(+), 9 deletions(-)

 create mode 100644 tests/bugs/bug-1584517.t

diff --git a/tests/bugs/bug-1584517.t b/tests/bugs/bug-1584517.t

new file mode 100644

index 0000000..7f48015

--- /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 c6adce4..23049b6 100644

--- a/xlators/cluster/dht/src/dht-common.c

+++ b/xlators/cluster/dht/src/dht-common.c

@@ -1329,6 +1329,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);

@@ -1401,6 +1403,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);

@@ -1496,6 +1515,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 */

@@ -1516,7 +1572,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) ||

@@ -1677,12 +1733,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"

@@ -1828,10 +1884,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;

@@ -1843,8 +1898,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;

@@ -1854,6 +1911,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 a70342f..b40815c 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;

@@ -1491,4 +1492,6 @@ int

 dht_selfheal_dir_setattr (call_frame_t *frame, loc_t *loc, struct iatt *stbuf,

                           int32_t valid, dht_layout_t *layout);

+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 e9b1db9..035a709 100644

--- a/xlators/cluster/dht/src/dht-selfheal.c

+++ b/xlators/cluster/dht/src/dht-selfheal.c

@@ -2495,7 +2495,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);

                 }

-- 

1.8.3.1