From e431321f1348b5d51733a6b6c5e046fd8c6e28cc Mon Sep 17 00:00:00 2001

From: karthik-us <ksubrahm@redhat.com>

Date: Mon, 5 Jul 2021 10:52:10 +0530

Subject: [PATCH 586/586] protocol/client: Do not reopen fd post handshake if

 posix lock is held

Problem:

With client.strict-locks enabled, in some cases where the posix lock is

taken after a brick gets disconnected, the fd is getting reopened when

the brick gets reconnected to the client as part of client_post_handshake.

In such cases the saved fdctx's lock_list may not have the latest

information.

Fix:

Check the lock information in the fdctx->lk_ctx as well post handshake

which will have the latest information on the locks.

Also check for this field in other places as well to prevent writes

happening with anonymous fd even without re-opening the fd on the

restarted brick.

> Upstream patch: https://github.com/gluster/glusterfs/pull/2582

> Fixes: #2581

> Change-Id: I7a0799e242ce188c6597dec0a65b4dae7dcd815b

> Signed-off-by: karthik-us ksubrahm@redhat.com

BUG: 1689375

Change-Id: I7a0799e242ce188c6597dec0a65b4dae7dcd815b

Signed-off-by: karthik-us <ksubrahm@redhat.com>

Reviewed-on: https://code.engineering.redhat.com/gerrit/c/rhs-glusterfs/+/252588

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

Reviewed-by: Ravishankar Narayanankutty <ravishankar@redhat.com>

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

---

 tests/bugs/replicate/do-not-reopen-fd.t        | 76 ++++++++++++++++++--------

 xlators/protocol/client/src/client-handshake.c |  2 +-

 xlators/protocol/client/src/client-helpers.c   | 11 +++-

 xlators/protocol/client/src/client.c           |  2 +-

 xlators/protocol/client/src/client.h           |  3 +

 5 files changed, 67 insertions(+), 27 deletions(-)

diff --git a/tests/bugs/replicate/do-not-reopen-fd.t b/tests/bugs/replicate/do-not-reopen-fd.t

index 13b5218..f346709 100644

--- a/tests/bugs/replicate/do-not-reopen-fd.t

+++ b/tests/bugs/replicate/do-not-reopen-fd.t

@@ -20,10 +20,41 @@ TEST $GFS --volfile-id=/$V0 --volfile-server=$H0 $M0

 TEST $GFS --volfile-id=/$V0 --volfile-server=$H0 $M1

 TEST touch $M0/a

+gfid_a=$(gf_get_gfid_xattr $B0/${V0}0/a)

+gfid_str_a=$(gf_gfid_xattr_to_str $gfid_a)

+

+

+# Open fd from a client, check for open fd on all the bricks.

+TEST fd1=`fd_available`

+TEST fd_open $fd1 'rw' $M0/a

+EXPECT "^1$" gf_open_file_count_in_brick $V0 $H0 $B0/${V0}0 $gfid_str_a

+EXPECT "^1$" gf_open_file_count_in_brick $V0 $H0 $B0/${V0}1 $gfid_str_a

+EXPECT "^1$" gf_open_file_count_in_brick $V0 $H0 $B0/${V0}2 $gfid_str_a

+

+# Kill a brick and take lock on the fd

+TEST kill_brick $V0 $H0 $B0/${V0}0

+EXPECT_WITHIN $PROCESS_DOWN_TIMEOUT "^0$" afr_child_up_status_meta $M0 $V0-replicate-0 0

+TEST flock -x $fd1

+

+# Restart the brick and check for no open fd on the restarted brick.

+TEST $CLI volume start $V0 force

+EXPECT_WITHIN $PROCESS_UP_TIMEOUT "^1$" brick_up_status $V0 $H0 $B0/${V0}0

+EXPECT_WITHIN $PROCESS_UP_TIMEOUT "^1$" afr_child_up_status_meta $M0 $V0-replicate-0 0

+EXPECT "^0$" gf_open_file_count_in_brick $V0 $H0 $B0/${V0}0 $gfid_str_a

+EXPECT "^1$" gf_open_file_count_in_brick $V0 $H0 $B0/${V0}1 $gfid_str_a

+EXPECT "^1$" gf_open_file_count_in_brick $V0 $H0 $B0/${V0}2 $gfid_str_a

+

+# Write on the fd. It should fail on the restarted brick.

+TEST fd_write $fd1 "data-0"

+EXPECT "" cat $B0/${V0}0/a

+EXPECT "data-0" cat $B0/${V0}1/a

+EXPECT "data-0" cat $B0/${V0}2/a

+

+TEST fd_close $fd1

 # Kill one brick and take lock on the fd and do a write.

 TEST kill_brick $V0 $H0 $B0/${V0}0

-EXPECT_WITHIN $PROCESS_DOWN_TIMEOUT "0" afr_child_up_status_meta $M0 $V0-replicate-0 0

+EXPECT_WITHIN $PROCESS_DOWN_TIMEOUT "^0$" afr_child_up_status_meta $M0 $V0-replicate-0 0

 TEST fd1=`fd_available`

 TEST fd_open $fd1 'rw' $M0/a

@@ -34,7 +65,7 @@ TEST fd_write $fd1 "data-1"

 # should still succeed as there were no quorum disconnects.

 TEST $CLI volume start $V0 force

 EXPECT_WITHIN $PROCESS_UP_TIMEOUT "^1$" brick_up_status $V0 $H0 $B0/${V0}0

-EXPECT_WITHIN $PROCESS_UP_TIMEOUT "1" afr_child_up_status_meta $M0 $V0-replicate-0 0

+EXPECT_WITHIN $PROCESS_UP_TIMEOUT "^1$" afr_child_up_status_meta $M0 $V0-replicate-0 0

 TEST fd_write $fd1 "data-2"

 EXPECT "" cat $B0/${V0}0/a

 EXPECT "data-2" cat $B0/${V0}1/a

@@ -42,9 +73,6 @@ EXPECT "data-2" cat $B0/${V0}2/a

 # Check there is no fd opened on the 1st brick by checking for the gfid inside

 # /proc/pid-of-brick/fd/ directory

-gfid_a=$(gf_get_gfid_xattr $B0/${V0}0/a)

-gfid_str_a=$(gf_gfid_xattr_to_str $gfid_a)

-

 EXPECT "^0$" gf_open_file_count_in_brick $V0 $H0 $B0/${V0}0 $gfid_str_a

 EXPECT "^1$" gf_open_file_count_in_brick $V0 $H0 $B0/${V0}1 $gfid_str_a

 EXPECT "^1$" gf_open_file_count_in_brick $V0 $H0 $B0/${V0}2 $gfid_str_a

@@ -59,7 +87,7 @@ EXPECT "^2$" gf_open_file_count_in_brick $V0 $H0 $B0/${V0}2 $gfid_str_a

 # Kill 2nd brick and try writing to the file. The write should fail due to

 # quorum failure.

 TEST kill_brick $V0 $H0 $B0/${V0}1

-EXPECT_WITHIN $PROCESS_DOWN_TIMEOUT "0" afr_child_up_status_meta $M0 $V0-replicate-0 1

+EXPECT_WITHIN $PROCESS_DOWN_TIMEOUT "^0$" afr_child_up_status_meta $M0 $V0-replicate-0 1

 TEST ! fd_write $fd1 "data-3"

 TEST ! fd_cat $fd1

@@ -67,7 +95,7 @@ TEST ! fd_cat $fd1

 # which were down previously, will return EBADFD now.

 TEST $CLI volume start $V0 force

 EXPECT_WITHIN $PROCESS_UP_TIMEOUT "^1$" brick_up_status $V0 $H0 $B0/${V0}1

-EXPECT_WITHIN $PROCESS_UP_TIMEOUT "1" afr_child_up_status_meta $M0 $V0-replicate-0 1

+EXPECT_WITHIN $PROCESS_UP_TIMEOUT "^1$" afr_child_up_status_meta $M0 $V0-replicate-0 1

 TEST ! fd_write $fd1 "data-4"

 TEST ! fd_cat $fd1

 EXPECT "^1$" gf_open_file_count_in_brick $V0 $H0 $B0/${V0}0 $gfid_str_a

@@ -79,9 +107,9 @@ EXPECT "^2$" gf_open_file_count_in_brick $V0 $H0 $B0/${V0}2 $gfid_str_a

 EXPECT_WITHIN $HEAL_TIMEOUT "^2$" get_pending_heal_count $V0

 TEST $CLI volume heal $V0 enable

 EXPECT_WITHIN $PROCESS_UP_TIMEOUT "Y" glustershd_up_status

-EXPECT_WITHIN $CHILD_UP_TIMEOUT "1" afr_child_up_status_in_shd $V0 0

-EXPECT_WITHIN $CHILD_UP_TIMEOUT "1" afr_child_up_status_in_shd $V0 1

-EXPECT_WITHIN $CHILD_UP_TIMEOUT "1" afr_child_up_status_in_shd $V0 2

+EXPECT_WITHIN $CHILD_UP_TIMEOUT "^1$" afr_child_up_status_in_shd $V0 0

+EXPECT_WITHIN $CHILD_UP_TIMEOUT "^1$" afr_child_up_status_in_shd $V0 1

+EXPECT_WITHIN $CHILD_UP_TIMEOUT "^1$" afr_child_up_status_in_shd $V0 2

 TEST $CLI volume heal $V0

 EXPECT_WITHIN $HEAL_TIMEOUT "^0$" get_pending_heal_count $V0

@@ -103,7 +131,7 @@ TEST ! fd_write $fd1 "data-5"

 # Kill the only brick that is having lock and try taking lock on another client

 # which should succeed.

 TEST kill_brick $V0 $H0 $B0/${V0}2

-EXPECT_WITHIN $PROCESS_DOWN_TIMEOUT "0" afr_child_up_status_meta $M0 $V0-replicate-0 2

+EXPECT_WITHIN $PROCESS_DOWN_TIMEOUT "^0$" afr_child_up_status_meta $M0 $V0-replicate-0 2

 TEST flock -x $fd2

 TEST fd_write $fd2 "data-6"

 EXPECT "^1$" gf_open_file_count_in_brick $V0 $H0 $B0/${V0}0 $gfid_str_a

@@ -114,17 +142,17 @@ EXPECT "^1$" gf_open_file_count_in_brick $V0 $H0 $B0/${V0}1 $gfid_str_a

 # fail and operations on the 2nd fd should succeed.

 TEST $CLI volume start $V0 force

 EXPECT_WITHIN $PROCESS_UP_TIMEOUT "^1$" brick_up_status $V0 $H0 $B0/${V0}2

-EXPECT_WITHIN $PROCESS_UP_TIMEOUT "1" afr_child_up_status_meta $M0 $V0-replicate-0 2

-EXPECT_WITHIN $PROCESS_UP_TIMEOUT "1" afr_child_up_status_meta $M1 $V0-replicate-0 2

+EXPECT_WITHIN $PROCESS_UP_TIMEOUT "^1$" afr_child_up_status_meta $M0 $V0-replicate-0 2

+EXPECT_WITHIN $PROCESS_UP_TIMEOUT "^1$" afr_child_up_status_meta $M1 $V0-replicate-0 2

 EXPECT "^1$" gf_open_file_count_in_brick $V0 $H0 $B0/${V0}0 $gfid_str_a

 EXPECT "^1$" gf_open_file_count_in_brick $V0 $H0 $B0/${V0}1 $gfid_str_a

-EXPECT "^1$" gf_open_file_count_in_brick $V0 $H0 $B0/${V0}2 $gfid_str_a

+EXPECT "^0$" gf_open_file_count_in_brick $V0 $H0 $B0/${V0}2 $gfid_str_a

 TEST ! fd_write $fd1 "data-7"

 TEST ! fd_cat $fd1

 EXPECT "^1$" gf_open_file_count_in_brick $V0 $H0 $B0/${V0}0 $gfid_str_a

 EXPECT "^1$" gf_open_file_count_in_brick $V0 $H0 $B0/${V0}1 $gfid_str_a

-EXPECT "^1$" gf_open_file_count_in_brick $V0 $H0 $B0/${V0}2 $gfid_str_a

+EXPECT "^0" gf_open_file_count_in_brick $V0 $H0 $B0/${V0}2 $gfid_str_a

 TEST fd_cat $fd2

 # Close both the fds which will release the locks and then re-open and take lock

@@ -159,9 +187,9 @@ EXPECT_WITHIN $REOPEN_TIMEOUT "^0$" gf_open_file_count_in_brick $V0 $H0 $B0/${V0

 # Heal the volume

 TEST $CLI volume heal $V0 enable

 EXPECT_WITHIN $PROCESS_UP_TIMEOUT "Y" glustershd_up_status

-EXPECT_WITHIN $CHILD_UP_TIMEOUT "1" afr_child_up_status_in_shd $V0 0

-EXPECT_WITHIN $CHILD_UP_TIMEOUT "1" afr_child_up_status_in_shd $V0 1

-EXPECT_WITHIN $CHILD_UP_TIMEOUT "1" afr_child_up_status_in_shd $V0 2

+EXPECT_WITHIN $CHILD_UP_TIMEOUT "^1$" afr_child_up_status_in_shd $V0 0

+EXPECT_WITHIN $CHILD_UP_TIMEOUT "^1$" afr_child_up_status_in_shd $V0 1

+EXPECT_WITHIN $CHILD_UP_TIMEOUT "^1$" afr_child_up_status_in_shd $V0 2

 TEST $CLI volume heal $V0

 EXPECT_WITHIN $HEAL_TIMEOUT "^0$" get_pending_heal_count $V0

@@ -169,7 +197,7 @@ TEST $CLI volume heal $V0 disable

 # Kill one brick and open a fd.

 TEST kill_brick $V0 $H0 $B0/${V0}0

-EXPECT_WITHIN $PROCESS_DOWN_TIMEOUT "0" afr_child_up_status_meta $M0 $V0-replicate-0 0

+EXPECT_WITHIN $PROCESS_DOWN_TIMEOUT "^0$" afr_child_up_status_meta $M0 $V0-replicate-0 0

 TEST fd1=`fd_available`

 TEST fd_open $fd1 'rw' $M0/a

@@ -182,7 +210,7 @@ EXPECT "^1$" gf_open_file_count_in_brick $V0 $H0 $B0/${V0}2 $gfid_str_a

 # any of the bricks.

 TEST $CLI volume start $V0 force

 EXPECT_WITHIN $PROCESS_UP_TIMEOUT "^1$" brick_up_status $V0 $H0 $B0/${V0}0

-EXPECT_WITHIN $PROCESS_UP_TIMEOUT "1" afr_child_up_status_meta $M0 $V0-replicate-0 0

+EXPECT_WITHIN $PROCESS_UP_TIMEOUT "^1$" afr_child_up_status_meta $M0 $V0-replicate-0 0

 TEST fd_write $fd1 "data-10"

 EXPECT "^1$" gf_open_file_count_in_brick $V0 $H0 $B0/${V0}0 $gfid_str_a

@@ -193,7 +221,7 @@ TEST fd_close $fd1

 # Kill one brick, open and take lock on a fd.

 TEST kill_brick $V0 $H0 $B0/${V0}0

-EXPECT_WITHIN $PROCESS_DOWN_TIMEOUT "0" afr_child_up_status_meta $M0 $V0-replicate-0 0

+EXPECT_WITHIN $PROCESS_DOWN_TIMEOUT "^0$" afr_child_up_status_meta $M0 $V0-replicate-0 0

 TEST fd1=`fd_available`

 TEST fd_open $fd1 'rw' $M0/a

 TEST flock -x $fd1

@@ -204,7 +232,7 @@ EXPECT "^1$" gf_open_file_count_in_brick $V0 $H0 $B0/${V0}2 $gfid_str_a

 # Kill & restart another brick so that it will return EBADFD

 TEST kill_brick $V0 $H0 $B0/${V0}1

-EXPECT_WITHIN $PROCESS_DOWN_TIMEOUT "0" brick_up_status $V0 $H0 $B0/${V0}1

+EXPECT_WITHIN $PROCESS_DOWN_TIMEOUT "^0$" brick_up_status $V0 $H0 $B0/${V0}1

 # Restart the bricks and then write. Now fd should not get re-opened since lock

 # is still held on one brick and write should also fail as there is no quorum.

@@ -212,8 +240,8 @@ EXPECT_WITHIN $PROCESS_DOWN_TIMEOUT "0" brick_up_status $V0 $H0 $B0/${V0}1

 TEST $CLI volume start $V0 force

 EXPECT_WITHIN $PROCESS_UP_TIMEOUT "^1$" brick_up_status $V0 $H0 $B0/${V0}0

 EXPECT_WITHIN $PROCESS_UP_TIMEOUT "^1$" brick_up_status $V0 $H0 $B0/${V0}1

-EXPECT_WITHIN $PROCESS_UP_TIMEOUT "1" afr_child_up_status_meta $M0 $V0-replicate-0 0

-EXPECT_WITHIN $PROCESS_UP_TIMEOUT "1" afr_child_up_status_meta $M0 $V0-replicate-0 1

+EXPECT_WITHIN $PROCESS_UP_TIMEOUT "^1$" afr_child_up_status_meta $M0 $V0-replicate-0 0

+EXPECT_WITHIN $PROCESS_UP_TIMEOUT "^1$" afr_child_up_status_meta $M0 $V0-replicate-0 1

 TEST ! fd_write $fd1 "data-11"

 EXPECT "^0$" gf_open_file_count_in_brick $V0 $H0 $B0/${V0}0 $gfid_str_a

 EXPECT "^0$" gf_open_file_count_in_brick $V0 $H0 $B0/${V0}1 $gfid_str_a

diff --git a/xlators/protocol/client/src/client-handshake.c b/xlators/protocol/client/src/client-handshake.c

index a12472b..20e03d8 100644

--- a/xlators/protocol/client/src/client-handshake.c

+++ b/xlators/protocol/client/src/client-handshake.c

@@ -911,7 +911,7 @@ client_post_handshake(call_frame_t *frame, xlator_t *this)

         list_for_each_entry_safe(fdctx, tmp, &conf->saved_fds, sfd_pos)

         {

             if (fdctx->remote_fd != -1 ||

-                (!list_empty(&fdctx->lock_list) && conf->strict_locks))

+                (!fdctx_lock_lists_empty(fdctx) && conf->strict_locks))

                 continue;

             fdctx->reopen_done = client_child_up_reopen_done;

diff --git a/xlators/protocol/client/src/client-helpers.c b/xlators/protocol/client/src/client-helpers.c

index a80f303..b4a7294 100644

--- a/xlators/protocol/client/src/client-helpers.c

+++ b/xlators/protocol/client/src/client-helpers.c

@@ -15,6 +15,15 @@

 #include <glusterfs/compat-errno.h>

 #include <glusterfs/common-utils.h>

+gf_boolean_t

+fdctx_lock_lists_empty(clnt_fd_ctx_t *fdctx)

+{

+    if (list_empty(&fdctx->lock_list) && fd_lk_ctx_empty(fdctx->lk_ctx))

+        return _gf_true;

+

+    return _gf_false;

+}

+

 int

 client_fd_lk_list_empty(fd_lk_ctx_t *lk_ctx, gf_boolean_t try_lock)

 {

@@ -441,7 +450,7 @@ client_get_remote_fd(xlator_t *this, fd_t *fd, int flags, int64_t *remote_fd,

                 *remote_fd = fdctx->remote_fd;

             }

-            locks_involved = !list_empty(&fdctx->lock_list);

+            locks_involved = !fdctx_lock_lists_empty(fdctx);

         }

     }

     pthread_spin_unlock(&conf->fd_lock);

diff --git a/xlators/protocol/client/src/client.c b/xlators/protocol/client/src/client.c

index 35a5340..6df2ed1 100644

--- a/xlators/protocol/client/src/client.c

+++ b/xlators/protocol/client/src/client.c

@@ -881,7 +881,7 @@ client_open(call_frame_t *frame, xlator_t *this, loc_t *loc, int32_t flags,

         pthread_spin_lock(&conf->fd_lock);

         {

             fdctx = this_fd_get_ctx(fd, this);

-            if (fdctx && !list_empty(&fdctx->lock_list)) {

+            if (fdctx && !fdctx_lock_lists_empty(fdctx)) {

                 ret = -1;

                 op_errno = EBADFD;

             }

diff --git a/xlators/protocol/client/src/client.h b/xlators/protocol/client/src/client.h

index f952aea..799fe6e 100644

--- a/xlators/protocol/client/src/client.h

+++ b/xlators/protocol/client/src/client.h

@@ -535,4 +535,7 @@ client_add_lock_for_recovery(fd_t *fd, struct gf_flock *flock,

 int

 client_is_setlk(int32_t cmd);

+gf_boolean_t

+fdctx_lock_lists_empty(clnt_fd_ctx_t *fdctx);

+

 #endif /* !_CLIENT_H */

-- 

1.8.3.1