cluster/afr: Fix deadlock due to compound fops

When an afr data transaction is eligible for using
eager-lock, this information is represented in
local->transaction.eager_lock_on. However, if non-blocking
inodelk attempt (which is a full lock) fails, AFR falls back
to blocking locks which are range locks. At this point,
local->transaction.eager_lock[] per brick is reset but
local->transaction.eager_lock_on is still true.
When AFR decides to compound post-op and unlock, it is after
confirming that the transaction did not use eager lock (well,
except for a small bug where local->transaction.locks_acquired[]
is not considered).

But within afr_post_op_unlock_do(), afr again incorrectly sets
the lock range to full-lock based on local->transaction.eager_lock_on
value. This is a bug and can lead to deadlock since the locks acquired
were range locks and a full unlock is being sent leading to unlock failure
and thereby every other lock request (be it from SHD or other clients or
glfsheal) getting blocked forever and the user perceives a hang.

FIX:
Unconditionally rely on the range locks in inodelk object for unlocking
when using compounded post-op + unlock.

Big thanks to Pranith for helping with the debugging.

Change-Id: Idb4938f90397fb4bd90921f9ae6ea582042e5c67
BUG: 1398566
Signed-off-by: Krutika Dhananjay <kdhananj@redhat.com>
Reviewed-on: http://review.gluster.org/15929
Smoke: Gluster Build System <jenkins@build.gluster.org>
NetBSD-regression: NetBSD Build System <jenkins@build.gluster.org>
CentOS-regression: Gluster Build System <jenkins@build.gluster.org>
Reviewed-by: Pranith Kumar Karampuri <pkarampu@redhat.com>

1 files changed, 8 insertions, 14 deletions

diff --git a/xlators/cluster/afr/src/afr-transaction.c b/xlators/cluster/afr/src/afr-transaction.c
index a456c827f4b..f81932b0c8c 100644
--- a/xlators/cluster/afr/src/afr-transaction.c
+++ b/xlators/cluster/afr/src/afr-transaction.c
@@ -839,7 +839,7 @@ afr_changelog_post_op_now (call_frame_t *frame, xlator_t *this)
         int                     ret             = 0;
         int                     idx             = 0;
         int                     nothing_failed  = 1;
-        int                     piggyback       = 0;
+        gf_boolean_t            compounded_unlock = _gf_true;
         gf_boolean_t            need_undirty    = _gf_false;
 
         afr_handle_quorum (frame);
@@ -913,9 +913,11 @@ afr_changelog_post_op_now (call_frame_t *frame, xlator_t *this)
                                 if (local->transaction.pre_op[i] &&
                                     local->transaction.eager_lock[i]) {
                                         if (fd_ctx->lock_piggyback[i])
-                                                piggyback = 1;
+                                                compounded_unlock = _gf_false;
+                                        else if (fd_ctx->lock_acquired[i])
+                                                compounded_unlock = _gf_false;
                                 }
-                                if (piggyback == 1)
+                                if (compounded_unlock == _gf_false)
                                         break;
                         }
                 }
@@ -924,7 +926,7 @@ afr_changelog_post_op_now (call_frame_t *frame, xlator_t *this)
 
         /* Do not compound if any brick got piggybacked lock as
          * unlock should not be done for that. */
-        if (local->compound && !piggyback) {
+        if (local->compound && compounded_unlock) {
                 afr_post_op_unlock_do (frame, this, xattr,
                                        afr_changelog_post_op_done,
                                        AFR_TRANSACTION_POST_OP);
@@ -1439,11 +1441,9 @@ afr_post_op_unlock_do (call_frame_t *frame, xlator_t *this, dict_t *xattr,
         compound_args_t         *args           = NULL;
         afr_internal_lock_t     *int_lock       = NULL;
         afr_inodelk_t           *inodelk        = NULL;
-        struct gf_flock         *flock_use      = NULL;
 	int                     i               = 0;
 	int                     call_count      = 0;
         struct gf_flock         flock           = {0,};
-        struct gf_flock         full_flock      = {0,};
         int                     ret             = 0;
 
 	local = frame->local;
@@ -1456,8 +1456,6 @@ afr_post_op_unlock_do (call_frame_t *frame, xlator_t *this, dict_t *xattr,
                 flock.l_start = inodelk->flock.l_start;
                 flock.l_len   = inodelk->flock.l_len;
                 flock.l_type  = F_UNLCK;
-                full_flock.l_type = F_UNLCK;
-
         }
 
         ret = afr_changelog_prepare (this, frame, &call_count, changelog_resume,
@@ -1485,22 +1483,18 @@ afr_post_op_unlock_do (call_frame_t *frame, xlator_t *this, dict_t *xattr,
                             local->fd, GF_XATTROP_ADD_ARRAY,
                             xattr, xdata);
         i++;
-        if (!local->transaction.eager_lock_on)
-                flock_use = &flock;
-        else
-                flock_use = &full_flock;
 
         if (afr_is_inodelk_transaction(local)) {
                 if (local->fd) {
                         COMPOUND_PACK_ARGS (finodelk, GF_FOP_FINODELK,
                                             args, i,
                                             int_lock->domain, local->fd,
-                                            F_SETLK, flock_use, NULL);
+                                            F_SETLK, &flock, NULL);
                 } else {
                         COMPOUND_PACK_ARGS (inodelk, GF_FOP_INODELK,
                                             args, i,
                                             int_lock->domain, &local->loc,
-                                            F_SETLK, flock_use, NULL);
+                                            F_SETLK, &flock, NULL);
                 }
         }