features/locks: Clean up logging of cleanup in DISCONNECT codepath

Now, gfid is printed as opposed to path in cleanup messages.

Also, refkeeper update is eliminated in inodelk and entrylk.
Instead, the patch ensures inode and pl_inode are kept alive as
long as there is atleast one lock (granted/blocked) on an inode.

Also, every inode is unref'd appropriately on a DISCONNECT from the
lock-owning client.

Change-Id: I531b1a02fe1b889fdd7f54b1fd522e78a18ed1df
BUG: 1104915
Signed-off-by: Krutika Dhananjay <kdhananj@redhat.com>
Reviewed-on: http://review.gluster.org/7981
Tested-by: Gluster Build System <jenkins@build.gluster.com>
Reviewed-by: Pranith Kumar Karampuri <pkarampu@redhat.com>
Tested-by: Pranith Kumar Karampuri <pkarampu@redhat.com>

1 files changed, 59 insertions, 26 deletions

diff --git a/xlators/features/locks/src/inodelk.c b/xlators/features/locks/src/inodelk.c
index c76cb7f9199..ef06531cfde 100644
--- a/xlators/features/locks/src/inodelk.c
+++ b/xlators/features/locks/src/inodelk.c
@@ -383,24 +383,13 @@ static void
 pl_inodelk_log_cleanup (pl_inode_lock_t *lock)
 {
 	pl_inode_t *pl_inode = NULL;
-        char *path = NULL;
-        char *file = NULL;
 
 	pl_inode = lock->pl_inode;
 
-	inode_path (pl_inode->refkeeper, NULL, &path);
-
-	if (path)
-		file = path;
-	else
-		file = uuid_utoa (pl_inode->refkeeper->gfid);
-
-	gf_log (THIS->name, GF_LOG_WARNING,
-		"releasing lock on %s held by "
-		"{client=%p, pid=%"PRId64" lk-owner=%s}",
-		file, lock->client, (uint64_t) lock->client_pid,
-		lkowner_utoa (&lock->owner));
-	GF_FREE (path);
+        gf_log (THIS->name, GF_LOG_WARNING, "releasing lock on %s held by "
+                "{client=%p, pid=%"PRId64" lk-owner=%s}",
+                uuid_utoa (pl_inode->gfid), lock->client,
+                (uint64_t) lock->client_pid, lkowner_utoa (&lock->owner));
 }
 
 
@@ -501,6 +490,7 @@ pl_inodelk_client_cleanup (xlator_t *this, pl_ctx_t *ctx)
 			__pl_inodelk_unref (l);
 		}
 		pthread_mutex_unlock (&pl_inode->mutex);
+                inode_unref (pl_inode->inode);
         }
 
         return 0;
@@ -509,13 +499,36 @@ pl_inodelk_client_cleanup (xlator_t *this, pl_ctx_t *ctx)
 
 static int
 pl_inode_setlk (xlator_t *this, pl_ctx_t *ctx, pl_inode_t *pl_inode,
-		pl_inode_lock_t *lock, int can_block, pl_dom_list_t *dom)
+		pl_inode_lock_t *lock, int can_block, pl_dom_list_t *dom,
+                inode_t *inode)
 {
-        int ret = -EINVAL;
-        pl_inode_lock_t *retlock = NULL;
-        gf_boolean_t    unref = _gf_true;
+        int               ret              = -EINVAL;
+        pl_inode_lock_t  *retlock          =  NULL;
+        gf_boolean_t      unref            =  _gf_true;
+        gf_boolean_t      need_inode_unref =  _gf_false;
+        short             fl_type;
 
 	lock->pl_inode = pl_inode;
+        fl_type = lock->fl_type;
+
+        /* Ideally, AFTER a successful lock (both blocking and non-blocking) or
+         * an unsuccessful blocking lock operation, the inode needs to be ref'd.
+         *
+         * But doing so might give room to a race where the lock-requesting
+         * client could send a DISCONNECT just before this thread refs the inode
+         * after the locking is done, and the epoll thread could unref the inode
+         * in cleanup which means the inode's refcount would come down to 0, and
+         * the call to pl_forget() at this point destroys @pl_inode. Now when
+         * the io-thread executing this function tries to access pl_inode,
+         * it could crash on account of illegal memory access.
+         *
+         * To get around this problem, the inode is ref'd once even before
+         * adding the lock into client_list as a precautionary measure.
+         * This way even if there are DISCONNECTs, there will always be 1 extra
+         * ref on the inode, so @pl_inode is still alive until after the
+         * current stack unwinds.
+         */
+        pl_inode->inode = inode_ref (inode);
 
 	if (ctx)
 		pthread_mutex_lock (&ctx->lock);
@@ -542,12 +555,24 @@ pl_inode_setlk (xlator_t *this, pl_ctx_t *ctx, pl_inode_t *pl_inode,
                                         lock->user_flock.l_len);
                                 if (can_block)
                                         unref = _gf_false;
+                                /* For all but the case where a non-blocking
+                                 * lock attempt fails, the extra ref taken at
+                                 * the start of this function must be negated.
+                                 */
+                                else
+                                        need_inode_unref = _gf_true;
                         }
 
 			if (ctx && (!ret || can_block))
 				list_add_tail (&lock->client_list,
 					       &ctx->inodelk_lockers);
                 } else {
+                        /* Irrespective of whether unlock succeeds or not,
+                         * the extra inode ref that was done at the start of
+                         * this function must be negated. Towards this,
+                         * @need_inode_unref flag is set unconditionally here.
+                         */
+                        need_inode_unref = _gf_true;
                         retlock = __inode_unlock_lock (this, lock, dom);
                         if (!retlock) {
                                 gf_log (this->name, GF_LOG_DEBUG,
@@ -568,7 +593,16 @@ out:
 	if (ctx)
 		pthread_mutex_unlock (&ctx->lock);
 
-        grant_blocked_inode_locks (this, pl_inode, dom);
+        if (need_inode_unref)
+                inode_unref (pl_inode->inode);
+
+        /* The following (extra) unref corresponds to the ref that
+         * was done at the time the lock was granted.
+         */
+        if ((fl_type == F_UNLCK) && (ret == 0)) {
+                inode_unref (pl_inode->inode);
+                grant_blocked_inode_locks (this, pl_inode, dom);
+        }
 
         return ret;
 }
@@ -707,7 +741,7 @@ pl_common_inodelk (call_frame_t *frame, xlator_t *this,
         }
 
         reqlock = new_inode_lock (flock, frame->root->client, frame->root->pid,
-                                  frame, this, volume, conn_id);
+                                  frame, this, dom->domain, conn_id);
 
         if (!reqlock) {
                 op_ret = -1;
@@ -725,7 +759,7 @@ pl_common_inodelk (call_frame_t *frame, xlator_t *this,
         case F_SETLK:
                 memcpy (&reqlock->user_flock, flock, sizeof (struct gf_flock));
                 ret = pl_inode_setlk (this, ctx, pinode, reqlock, can_block,
-				      dom);
+				      dom, inode);
 
                 if (ret < 0) {
                         if ((can_block) && (F_UNLCK != flock->l_type)) {
@@ -751,10 +785,9 @@ pl_common_inodelk (call_frame_t *frame, xlator_t *this,
         op_ret = 0;
 
 unwind:
-        if ((inode != NULL) && (flock !=NULL)) {
-                pl_update_refkeeper (this, inode);
-                pl_trace_out (this, frame, fd, loc, cmd, flock, op_ret, op_errno, volume);
-        }
+        if (flock != NULL)
+                pl_trace_out (this, frame, fd, loc, cmd, flock, op_ret,
+                              op_errno, volume);
 
         STACK_UNWIND_STRICT (inodelk, frame, op_ret, op_errno, NULL);
 out: