Blob Blame History Raw
From adeec3d5d85baad8b50d203f34a47ad5360d7cd7 Mon Sep 17 00:00:00 2001
From: karthik-us <ksubrahm@redhat.com>
Date: Mon, 7 Jun 2021 18:36:11 +0530
Subject: [PATCH 582/584] protocol/client: Fix lock memory leak

Problem-1:
When an overlapping lock is issued the merged lock is not assigned the
owner. When flush is issued on the fd, this particular lock is not freed
leading to memory leak

Fix-1:
Assign the owner while merging the locks.

Problem-2:
On fd-destroy lock structs could be present in fdctx. For some reason
with flock -x command and closing of the bash fd, it leads to this code
path. Which leaks the lock structs.

Fix-2:
When fdctx is being destroyed in client, make sure to cleanup any lock
structs.

> Upstream patch: https://github.com/gluster/glusterfs/pull/2338/commits/926402f639471d2664bf00c6692221ba297c525f
> fixes: gluster#2337
> Change-Id: I298124213ce5a1cf2b1f1756d5e8a9745d9c0a1c
> Signed-off-by: Pranith Kumar K <pranith.karampuri@phonepe.com>

BUG: 1689375
Change-Id: I298124213ce5a1cf2b1f1756d5e8a9745d9c0a1c
Signed-off-by: karthik-us <ksubrahm@redhat.com>
Reviewed-on: https://code.engineering.redhat.com/gerrit/c/rhs-glusterfs/+/245603
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/client/issue-2337-lock-mem-leak.c | 52 ++++++++++++++++++
 tests/bugs/client/issue-2337-lock-mem-leak.t | 42 ++++++++++++++
 tests/bugs/replicate/do-not-reopen-fd.t      | 65 ++++++++++++++--------
 tests/volume.rc                              |  8 +++
 xlators/protocol/client/src/client-helpers.c | 10 ++++
 xlators/protocol/client/src/client-lk.c      | 82 ++++++++++++++++++----------
 xlators/protocol/client/src/client.h         |  8 ++-
 7 files changed, 213 insertions(+), 54 deletions(-)
 create mode 100644 tests/bugs/client/issue-2337-lock-mem-leak.c
 create mode 100644 tests/bugs/client/issue-2337-lock-mem-leak.t

diff --git a/tests/bugs/client/issue-2337-lock-mem-leak.c b/tests/bugs/client/issue-2337-lock-mem-leak.c
new file mode 100644
index 0000000..d4e02a7
--- /dev/null
+++ b/tests/bugs/client/issue-2337-lock-mem-leak.c
@@ -0,0 +1,52 @@
+#include <sys/file.h>
+#include <stdio.h>
+#include <string.h>
+#include <errno.h>
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <fcntl.h>
+
+int
+main(int argc, char *argv[])
+{
+    int fd = -1;
+    char *filename = NULL;
+    struct flock lock = {
+        0,
+    };
+    int i = 0;
+    int ret = -1;
+
+    if (argc != 2) {
+        fprintf(stderr, "Usage: %s <filename> ", argv[0]);
+        goto out;
+    }
+
+    filename = argv[1];
+
+    fd = open(filename, O_RDWR | O_CREAT, 0);
+    if (fd < 0) {
+        fprintf(stderr, "open (%s) failed (%s)\n", filename, strerror(errno));
+        goto out;
+    }
+
+    lock.l_type = F_WRLCK;
+    lock.l_whence = SEEK_SET;
+    lock.l_len = 2;
+
+    while (i < 100) {
+        lock.l_start = i;
+        ret = fcntl(fd, F_SETLK, &lock);
+        if (ret < 0) {
+            fprintf(stderr, "fcntl setlk failed (%s)\n", strerror(errno));
+            goto out;
+        }
+
+        i++;
+    }
+
+    ret = 0;
+
+out:
+    return ret;
+}
diff --git a/tests/bugs/client/issue-2337-lock-mem-leak.t b/tests/bugs/client/issue-2337-lock-mem-leak.t
new file mode 100644
index 0000000..64132a2
--- /dev/null
+++ b/tests/bugs/client/issue-2337-lock-mem-leak.t
@@ -0,0 +1,42 @@
+#!/bin/bash
+
+#Test that lock fop is not leaking any memory for overlapping regions
+. $(dirname $0)/../../include.rc
+. $(dirname $0)/../../volume.rc
+. $(dirname $0)/../../fileio.rc
+
+cleanup;
+
+LOCK_TEST=$(dirname $0)/issue-2337-lock-mem-leak
+build_tester $(dirname $0)/issue-2337-lock-mem-leak.c -o ${LOCK_TEST}
+
+TEST glusterd
+TEST pidof glusterd
+TEST $CLI volume create $V0 $H0:$B0/${V0}1
+#Guard against flush-behind
+TEST $CLI volume set $V0 performance.write-behind off
+TEST $CLI volume start $V0
+TEST $GFS --volfile-id=/$V0 --volfile-server=$H0 $M0
+
+TEST touch $M0/a
+TEST fd1=`fd_available`
+TEST fd_open $fd1 'w' $M0/a
+TEST flock -x $fd1
+statedump=$(generate_mount_statedump $V0 $M0)
+EXPECT_NOT "^nostatedump$" echo $statedump
+#Making sure no one changes this mem-tracker name
+TEST grep gf_client_mt_clnt_lock_t $statedump
+TEST fd_close $fd1
+
+statedump=$(generate_mount_statedump $V0 $M0)
+EXPECT_NOT "^nostatedump$" echo $statedump
+TEST ! grep gf_client_mt_clnt_lock_t $statedump
+
+TEST ${LOCK_TEST} $M0/a
+
+statedump=$(generate_mount_statedump $V0 $M0)
+EXPECT_NOT "^nostatedump$" echo $statedump
+TEST ! grep gf_client_mt_clnt_lock_t $statedump
+TEST cleanup_mount_statedump $V0
+TEST rm ${LOCK_TEST}
+cleanup
diff --git a/tests/bugs/replicate/do-not-reopen-fd.t b/tests/bugs/replicate/do-not-reopen-fd.t
index 76d8e70..13b5218 100644
--- a/tests/bugs/replicate/do-not-reopen-fd.t
+++ b/tests/bugs/replicate/do-not-reopen-fd.t
@@ -45,13 +45,17 @@ EXPECT "data-2" cat $B0/${V0}2/a
 gfid_a=$(gf_get_gfid_xattr $B0/${V0}0/a)
 gfid_str_a=$(gf_gfid_xattr_to_str $gfid_a)
 
-EXPECT "N" gf_check_file_opened_in_brick $V0 $H0 $B0/${V0}0 $gfid_str_a
-EXPECT "Y" gf_check_file_opened_in_brick $V0 $H0 $B0/${V0}1 $gfid_str_a
-EXPECT "Y" gf_check_file_opened_in_brick $V0 $H0 $B0/${V0}2 $gfid_str_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
 
 TEST fd2=`fd_available`
 TEST fd_open $fd2 'rw' $M1/a
 
+EXPECT "^1$" gf_open_file_count_in_brick $V0 $H0 $B0/${V0}0 $gfid_str_a
+EXPECT "^2$" gf_open_file_count_in_brick $V0 $H0 $B0/${V0}1 $gfid_str_a
+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
@@ -66,6 +70,9 @@ 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
 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
+EXPECT "^1$" gf_open_file_count_in_brick $V0 $H0 $B0/${V0}1 $gfid_str_a
+EXPECT "^2$" gf_open_file_count_in_brick $V0 $H0 $B0/${V0}2 $gfid_str_a
 
 # Enable heal and check the files will have same content on all the bricks after
 # the heal is completed.
@@ -89,7 +96,9 @@ TEST ! fd_write $fd1 "data-5"
 
 # At this point only one brick will have the lock. Try taking the lock again on
 # the bad fd, which should also fail with EBADFD.
-TEST ! flock -x $fd1
+# TODO: At the moment quorum failure in lk leads to unlock on the bricks where
+# lock succeeds. This will change lock state on 3rd brick, commenting for now
+#TEST ! flock -x $fd1
 
 # Kill the only brick that is having lock and try taking lock on another client
 # which should succeed.
@@ -97,15 +106,25 @@ TEST kill_brick $V0 $H0 $B0/${V0}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
+EXPECT "^1$" gf_open_file_count_in_brick $V0 $H0 $B0/${V0}1 $gfid_str_a
+
 
 # Bring the brick up and try writing & reading on the old fd, which should still
 # 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 "^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
 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
 TEST fd_cat $fd2
 
 # Close both the fds which will release the locks and then re-open and take lock
@@ -113,17 +132,15 @@ TEST fd_cat $fd2
 TEST fd_close $fd1
 TEST fd_close $fd2
 
-TEST ! ls /proc/$$/fd/$fd1
-TEST ! ls /proc/$$/fd/$fd2
-EXPECT_WITHIN $REOPEN_TIMEOUT "N" gf_check_file_opened_in_brick $V0 $H0 $B0/${V0}0 $gfid_str_a
-EXPECT_WITHIN $REOPEN_TIMEOUT "N" gf_check_file_opened_in_brick $V0 $H0 $B0/${V0}1 $gfid_str_a
-EXPECT_WITHIN $REOPEN_TIMEOUT "N" gf_check_file_opened_in_brick $V0 $H0 $B0/${V0}2 $gfid_str_a
+EXPECT_WITHIN $REOPEN_TIMEOUT "^0$" gf_open_file_count_in_brick $V0 $H0 $B0/${V0}0 $gfid_str_a
+EXPECT_WITHIN $REOPEN_TIMEOUT "^0$" gf_open_file_count_in_brick $V0 $H0 $B0/${V0}1 $gfid_str_a
+EXPECT_WITHIN $REOPEN_TIMEOUT "^0$" gf_open_file_count_in_brick $V0 $H0 $B0/${V0}2 $gfid_str_a
 
 TEST fd1=`fd_available`
 TEST fd_open $fd1 'rw' $M0/a
-EXPECT_WITHIN $REOPEN_TIMEOUT "Y" gf_check_file_opened_in_brick $V0 $H0 $B0/${V0}0 $gfid_str_a
-EXPECT_WITHIN $REOPEN_TIMEOUT "Y" gf_check_file_opened_in_brick $V0 $H0 $B0/${V0}1 $gfid_str_a
-EXPECT_WITHIN $REOPEN_TIMEOUT "Y" gf_check_file_opened_in_brick $V0 $H0 $B0/${V0}2 $gfid_str_a
+EXPECT_WITHIN $REOPEN_TIMEOUT "^1$" gf_open_file_count_in_brick $V0 $H0 $B0/${V0}0 $gfid_str_a
+EXPECT_WITHIN $REOPEN_TIMEOUT "^1$" gf_open_file_count_in_brick $V0 $H0 $B0/${V0}1 $gfid_str_a
+EXPECT_WITHIN $REOPEN_TIMEOUT "^1$" gf_open_file_count_in_brick $V0 $H0 $B0/${V0}2 $gfid_str_a
 
 TEST flock -x $fd1
 TEST fd_write $fd1 "data-8"
@@ -134,6 +151,10 @@ EXPECT "data-8" head -n 1 $B0/${V0}1/a
 EXPECT "data-8" head -n 1 $B0/${V0}2/a
 
 TEST fd_close $fd1
+EXPECT_WITHIN $REOPEN_TIMEOUT "^0$" gf_open_file_count_in_brick $V0 $H0 $B0/${V0}0 $gfid_str_a
+EXPECT_WITHIN $REOPEN_TIMEOUT "^0$" gf_open_file_count_in_brick $V0 $H0 $B0/${V0}1 $gfid_str_a
+EXPECT_WITHIN $REOPEN_TIMEOUT "^0$" gf_open_file_count_in_brick $V0 $H0 $B0/${V0}2 $gfid_str_a
+
 
 # Heal the volume
 TEST $CLI volume heal $V0 enable
@@ -152,9 +173,9 @@ EXPECT_WITHIN $PROCESS_DOWN_TIMEOUT "0" afr_child_up_status_meta $M0 $V0-replica
 TEST fd1=`fd_available`
 TEST fd_open $fd1 'rw' $M0/a
 
-EXPECT "N" gf_check_file_opened_in_brick $V0 $H0 $B0/${V0}0 $gfid_str_a
-EXPECT "Y" gf_check_file_opened_in_brick $V0 $H0 $B0/${V0}1 $gfid_str_a
-EXPECT "Y" gf_check_file_opened_in_brick $V0 $H0 $B0/${V0}2 $gfid_str_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
 
 # Restart the brick and then write. Now fd should get re-opened and write should
 # succeed on the previously down brick as well since there are no locks held on
@@ -163,7 +184,7 @@ 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
 TEST fd_write $fd1 "data-10"
-EXPECT "Y" gf_check_file_opened_in_brick $V0 $H0 $B0/${V0}0 $gfid_str_a
+EXPECT "^1$" gf_open_file_count_in_brick $V0 $H0 $B0/${V0}0 $gfid_str_a
 
 EXPECT "data-10" head -n 1 $B0/${V0}0/a
 EXPECT "data-10" head -n 1 $B0/${V0}1/a
@@ -177,9 +198,9 @@ TEST fd1=`fd_available`
 TEST fd_open $fd1 'rw' $M0/a
 TEST flock -x $fd1
 
-EXPECT "N" gf_check_file_opened_in_brick $V0 $H0 $B0/${V0}0 $gfid_str_a
-EXPECT "Y" gf_check_file_opened_in_brick $V0 $H0 $B0/${V0}1 $gfid_str_a
-EXPECT "Y" gf_check_file_opened_in_brick $V0 $H0 $B0/${V0}2 $gfid_str_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
 
 # Kill & restart another brick so that it will return EBADFD
 TEST kill_brick $V0 $H0 $B0/${V0}1
@@ -194,9 +215,9 @@ 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
 TEST ! fd_write $fd1 "data-11"
-EXPECT "N" gf_check_file_opened_in_brick $V0 $H0 $B0/${V0}0 $gfid_str_a
-EXPECT "N" gf_check_file_opened_in_brick $V0 $H0 $B0/${V0}1 $gfid_str_a
-EXPECT "Y" gf_check_file_opened_in_brick $V0 $H0 $B0/${V0}2 $gfid_str_a
+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
+EXPECT "^1$" gf_open_file_count_in_brick $V0 $H0 $B0/${V0}2 $gfid_str_a
 
 EXPECT "data-10" head -n 1 $B0/${V0}0/a
 EXPECT "data-10" head -n 1 $B0/${V0}1/a
diff --git a/tests/volume.rc b/tests/volume.rc
index f5dd0b1..17c3835 100644
--- a/tests/volume.rc
+++ b/tests/volume.rc
@@ -407,6 +407,14 @@ function gf_check_file_opened_in_brick {
         fi
 }
 
+function gf_open_file_count_in_brick {
+        vol=$1
+        host=$2
+        brick=$3
+        realpath=$4
+        ls -l /proc/$(get_brick_pid $vol $host $brick)/fd | grep "${realpath}$" | wc -l
+}
+
 function gf_get_gfid_backend_file_path {
         brickpath=$1
         filepath_in_brick=$2
diff --git a/xlators/protocol/client/src/client-helpers.c b/xlators/protocol/client/src/client-helpers.c
index 48b6448..a80f303 100644
--- a/xlators/protocol/client/src/client-helpers.c
+++ b/xlators/protocol/client/src/client-helpers.c
@@ -3156,11 +3156,14 @@ client_fdctx_destroy(xlator_t *this, clnt_fd_ctx_t *fdctx)
     int32_t ret = -1;
     char parent_down = 0;
     fd_lk_ctx_t *lk_ctx = NULL;
+    gf_lkowner_t null_owner = {0};
+    struct list_head deleted_list;
 
     GF_VALIDATE_OR_GOTO("client", this, out);
     GF_VALIDATE_OR_GOTO(this->name, fdctx, out);
 
     conf = (clnt_conf_t *)this->private;
+    INIT_LIST_HEAD(&deleted_list);
 
     if (fdctx->remote_fd == -1) {
         gf_msg_debug(this->name, 0, "not a valid fd");
@@ -3174,6 +3177,13 @@ client_fdctx_destroy(xlator_t *this, clnt_fd_ctx_t *fdctx)
     pthread_mutex_unlock(&conf->lock);
     lk_ctx = fdctx->lk_ctx;
     fdctx->lk_ctx = NULL;
+    pthread_spin_lock(&conf->fd_lock);
+    {
+        __delete_granted_locks_owner_from_fdctx(fdctx, &null_owner,
+                                                &deleted_list);
+    }
+    pthread_spin_unlock(&conf->fd_lock);
+    destroy_client_locks_from_list(&deleted_list);
 
     if (lk_ctx)
         fd_lk_ctx_unref(lk_ctx);
diff --git a/xlators/protocol/client/src/client-lk.c b/xlators/protocol/client/src/client-lk.c
index c1fb055..cb4e894 100644
--- a/xlators/protocol/client/src/client-lk.c
+++ b/xlators/protocol/client/src/client-lk.c
@@ -253,6 +253,7 @@ __insert_and_merge(clnt_fd_ctx_t *fdctx, client_posix_lock_t *lock)
                 sum = add_locks(lock, conf);
 
                 sum->fd = lock->fd;
+                sum->owner = conf->owner;
 
                 __delete_client_lock(conf);
                 __destroy_client_lock(conf);
@@ -320,56 +321,77 @@ destroy_client_lock(client_posix_lock_t *lock)
     GF_FREE(lock);
 }
 
-int32_t
-delete_granted_locks_owner(fd_t *fd, gf_lkowner_t *owner)
+void
+destroy_client_locks_from_list(struct list_head *deleted)
 {
-    clnt_fd_ctx_t *fdctx = NULL;
     client_posix_lock_t *lock = NULL;
     client_posix_lock_t *tmp = NULL;
-    xlator_t *this = NULL;
-    clnt_conf_t *conf = NULL;
-
-    struct list_head delete_list;
-    int ret = 0;
+    xlator_t *this = THIS;
     int count = 0;
 
-    INIT_LIST_HEAD(&delete_list);
-    this = THIS;
-    conf = this->private;
+    list_for_each_entry_safe(lock, tmp, deleted, list)
+    {
+        list_del_init(&lock->list);
+        destroy_client_lock(lock);
+        count++;
+    }
 
-    pthread_spin_lock(&conf->fd_lock);
+    /* FIXME: Need to actually print the locks instead of count */
+    gf_msg_trace(this->name, 0, "Number of locks cleared=%d", count);
+}
 
-    fdctx = this_fd_get_ctx(fd, this);
-    if (!fdctx) {
-        pthread_spin_unlock(&conf->fd_lock);
+void
+__delete_granted_locks_owner_from_fdctx(clnt_fd_ctx_t *fdctx,
+                                        gf_lkowner_t *owner,
+                                        struct list_head *deleted)
+{
+    client_posix_lock_t *lock = NULL;
+    client_posix_lock_t *tmp = NULL;
 
-        gf_msg(this->name, GF_LOG_WARNING, EINVAL, PC_MSG_FD_CTX_INVALID,
-               "fdctx not valid");
-        ret = -1;
-        goto out;
+    gf_boolean_t is_null_lkowner = _gf_false;
+
+    if (is_lk_owner_null(owner)) {
+        is_null_lkowner = _gf_true;
     }
 
     list_for_each_entry_safe(lock, tmp, &fdctx->lock_list, list)
     {
-        if (is_same_lkowner(&lock->owner, owner)) {
+        if (is_null_lkowner || is_same_lkowner(&lock->owner, owner)) {
             list_del_init(&lock->list);
-            list_add_tail(&lock->list, &delete_list);
-            count++;
+            list_add_tail(&lock->list, deleted);
         }
     }
+}
 
-    pthread_spin_unlock(&conf->fd_lock);
+int32_t
+delete_granted_locks_owner(fd_t *fd, gf_lkowner_t *owner)
+{
+    clnt_fd_ctx_t *fdctx = NULL;
+    xlator_t *this = NULL;
+    clnt_conf_t *conf = NULL;
+    int ret = 0;
+    struct list_head deleted_locks;
 
-    if (!list_empty(&delete_list)) {
-        list_for_each_entry_safe(lock, tmp, &delete_list, list)
-        {
-            list_del_init(&lock->list);
-            destroy_client_lock(lock);
+    this = THIS;
+    conf = this->private;
+    INIT_LIST_HEAD(&deleted_locks);
+
+    pthread_spin_lock(&conf->fd_lock);
+    {
+        fdctx = this_fd_get_ctx(fd, this);
+        if (!fdctx) {
+            pthread_spin_unlock(&conf->fd_lock);
+
+            gf_smsg(this->name, GF_LOG_WARNING, EINVAL, PC_MSG_FD_CTX_INVALID,
+                    NULL);
+            ret = -1;
+            goto out;
         }
+        __delete_granted_locks_owner_from_fdctx(fdctx, owner, &deleted_locks);
     }
+    pthread_spin_unlock(&conf->fd_lock);
 
-    /* FIXME: Need to actually print the locks instead of count */
-    gf_msg_trace(this->name, 0, "Number of locks cleared=%d", count);
+    destroy_client_locks_from_list(&deleted_locks);
 
 out:
     return ret;
diff --git a/xlators/protocol/client/src/client.h b/xlators/protocol/client/src/client.h
index 2a50625..f952aea 100644
--- a/xlators/protocol/client/src/client.h
+++ b/xlators/protocol/client/src/client.h
@@ -406,8 +406,12 @@ int
 client_attempt_lock_recovery(xlator_t *this, clnt_fd_ctx_t *fdctx);
 int32_t
 delete_granted_locks_owner(fd_t *fd, gf_lkowner_t *owner);
-int32_t
-delete_granted_locks_fd(clnt_fd_ctx_t *fdctx);
+void
+__delete_granted_locks_owner_from_fdctx(clnt_fd_ctx_t *fdctx,
+                                        gf_lkowner_t *owner,
+                                        struct list_head *deleted);
+void
+destroy_client_locks_from_list(struct list_head *deleted);
 int32_t
 client_cmd_to_gf_cmd(int32_t cmd, int32_t *gf_cmd);
 void
-- 
1.8.3.1