nfs: fix locking in nfs/inode.c in nfs_free_open_context

PATCH In review Christian Krafft 3 participants 11 messages Jul 25, 2007 – Jul 27, 2007 NFS client

lore

Share & tools

lore

Christian Krafft

author

2007-07-25 15:09 UTC #0 lore raw

PATCH 1/1 nfs: fix locking in nfs/inode.c in nfs_free_open_context

From: Christian Krafft <[email protected]>

While running some tests on a server using NFS root I found some locking problems.

The scripts I am using can be found under at

http://localhorst.gotdns.org/parabelboi/nfs/

Please note, I am using rpm to generate read load. I found no better way to generate that much stress on the read path ;-)

It's a two way machine btw, with mount options tcp,nolock the problem doesn't occur (probably due to a different timing).
When running write_back.sh six times concurrent with one read.sh, the processes will be locked within a few minutes.

The problem occurs at least on 2.6.22 and 2.6.23-rc1. It shows up like this:

Badness at lib/kref.c:33
NIP: c00000000018b734 LR: c00000000014c37c CTR: c0000000001566f0
REGS: c00000007aebf160 TRAP: 0700 Not tainted (2.6.23-rc1)
MSR: 9000000000029032 <EE,ME,IR,DR> CR: 84000422 XER: 00000000 TASK = c00000007df7d530[30769] 'sync' THREAD: c00000007aebc000 CPU: 3 NIP [c00000000018b734] .kref_get+0xc/0x28
LR [c00000000014c37c] .get_nfs_open_context+0x1c/0x38

Call Trace:
[c00000007aebf3e0] [c00000000033d9f4] ._spin_lock+0x10/0x24 (unreliable)
[c00000007aebf460] [c00000000014cf40] .nfs_find_open_context+0x68/0xcc
[c00000007aebf500] [c000000000156654] .nfs_writepage_locked+0x164/0x200
[c00000007aebf600] [c00000000015670c] .nfs_writepage+0x1c/0x48
[c00000007aebf690] [c00000000008e53c] .__writepage+0x3c/0x84
[c00000007aebf710] [c00000000008ed84] .write_cache_pages+0x238/0x3f4
[c00000007aebf880] [c000000000155840] .nfs_writepages+0x7c/0xc0
[c00000007aebf970] [c00000000008efe0] .do_writepages+0x68/0xa4
[c00000007aebf9f0] [c0000000000e0290] .__writeback_single_inode+0x1bc/0x3b8
[c00000007aebfae0] [c0000000000e0984] .sync_sb_inodes+0x20c/0x308
[c00000007aebfb90] [c0000000000e0b50] .sync_inodes_sb+0xd0/0x100
[c00000007aebfc80] [c0000000000e0c14] .__sync_inodes+0x94/0x120
nstruction dump:
4e800421 e8410028 38000001 38210080 7c030378 e8010010 ebc1fff0 7c0803a6 
4e800020 80030000 7c000034 5400d97e <0b000000> 7c001828 30000001 7c00192d 
Unable to handle kernel paging request for data at address 0x00200200
Faulting instruction address: 0xc000000000193474
cpu 0x0: Vector: 300 (Data Access) at [c00000000ff4b5b0]
    pc: c000000000193474: .list_del+0x20/0xa4
    lr: c00000000014c2e8: .nfs_free_open_context+0x4c/0xc4
    sp: c00000000ff4b830
   msr: 9000000000009032
   dar: 200200
 dsisr: 40000000
  current = 0xc00000000ff0a230
  paca    = 0xc00000000047e900
    pid   = 506, comm = rpciod/0

enter ? for help
[c00000000ff4b8b0] c00000000014c2e8 .nfs_free_open_context+0x4c/0xc4 [c00000000ff4b940] c00000000018b708 .kref_put+0x74/0x94 [c00000000ff4b9c0] c00000000014c1dc .put_nfs_open_context+0x1c/0x34 [c00000000ff4ba40] c000000000150ec0 .nfs_free_request+0x2c/0x5c [c00000000ff4bad0] c00000000018b708 .kref_put+0x74/0x94 [c00000000ff4bb50] c000000000150e2c .nfs_release_request+0x20/0x38 [c00000000ff4bbd0] c00000000015750c .nfs_writeback_done_partial+0x270/0x2c8 [c00000000ff4bc80] c0000000003274fc .rpc_exit_task+0x5c/0xa0

Obviously the locking code in nfs_free_open_context is wrong.
Checking the list for entries and removing the entry should be an atomic operation. Also list_del_init should be used, because list_del will leave the empty list in an undefined condition.

After applying the patch the scripts run a bit longer, but another error occurs :-/

Signed-off-by Christian Krafft <[email protected]>

fs/nfs/inode.c +6 -6

Index: linux-2.6.23-rc1/fs/nfs/inode.c
===================================================================
@@ -484,13 +484,13 @@ static void nfs_free_open_context(struct
 {
 	struct nfs_open_context *ctx = container_of(kref,
 			struct nfs_open_context, kref);
+	struct inode *inode = ctx->path.dentry->d_inode;
+
+	spin_lock(&inode->i_lock);
+	if (!list_empty(&ctx->list))
+		list_del_init(&ctx->list);
+	spin_unlock(&inode->i_lock);
 
-	if (!list_empty(&ctx->list)) {
-		struct inode *inode = ctx->path.dentry->d_inode;
-		spin_lock(&inode->i_lock);
-		list_del(&ctx->list);
-		spin_unlock(&inode->i_lock);
-	}
 	if (ctx->state != NULL)
 		nfs4_close_state(&ctx->path, ctx->state, ctx->mode);
 	if (ctx->cred != NULL)

Signature · 13 lines

Mit freundlichen Gruessen,
kind regards,

Christian Krafft
IBM Systems & Technology Group,
Linux Kernel Development
IT Specialist


Vorsitzender des Aufsichtsrats:	Martin Jetter
Geschaeftsfuehrung:		Herbert Kircher
Sitz der Gesellschaft:		Boeblingen
Registriergericht:		Amtsgericht Stuttgart, HRB 243294

Trond Myklebust

2007-07-25 17:28 UTC #1 lore raw

Quoted 2 lines · Christian Krafft

Obviously the locking code in nfs_free_open_context is wrong.
Checking the list for entries and removing the entry should be an atomic operation.

Wrong. It is quite safe to test the structure member ctx->list for emptiness outside the spinlock because we have an explicit guarantee that nobody else has a reference to this structure, plus the atomic_dec_and_test() in kref_put() has acted as a memory barrier for us.

Quoted 2 lines

Also list_del_init should be used, because list_del will leave the empty list in an undefined condition.

Huh? We're freeing the context. It will _never_ _ever_ _ever_ be used again. If anything tries to use ctx->list after this, then that is a major bug.

Patch rejected...

Trond

Arnd Bergmann

2007-07-26 11:24 UTC #2 lore raw

Quoted 11 lines · Trond Myklebust

Quoted 2 lines · Christian Krafft
Obviously the locking code in nfs_free_open_context is wrong.
Checking the list for entries and removing the entry should be an atomic operation.
Wrong. It is quite safe to test the structure member ctx->list for emptiness outside the spinlock because we have an explicit guarantee that nobody else has a reference to this structure, plus the atomic_dec_and_test() in kref_put() has acted as a memory barrier for us.

Well, the real question then is how the ctx can still be present in the nfsi->open_files list. Since we are in nfs_free_open_context(), there must not be any pointer to the ctx anywhere, but still we have this other thread calling get_nfs_open_context() on it.

	Arnd <><

Trond Myklebust

2007-07-26 12:37 UTC #3 lore raw

Quoted 13 lines · Arnd Bergmann

Quoted 11 lines · Trond Myklebust
Quoted 2 lines · Christian Krafft
Obviously the locking code in nfs_free_open_context is wrong.
Checking the list for entries and removing the entry should be an atomic operation.
Wrong. It is quite safe to test the structure member ctx->list for emptiness outside the spinlock because we have an explicit guarantee that nobody else has a reference to this structure, plus the atomic_dec_and_test() in kref_put() has acted as a memory barrier for us.

Quoted 4 lines

Well, the real question then is how the ctx can still be present in the nfsi->open_files list. Since we are in nfs_free_open_context(), there must not be any pointer to the ctx anywhere, but still we have this other thread calling get_nfs_open_context() on it.

Yup. That is definitely a bug. I wish we had a 'kref_put_and_lock' to deal with these situations where you want to grab a lock atomically with the last put. It would make krefs a lot more useful...

Trond

Christian Krafft

author

2007-07-26 12:45 UTC #4 lore raw

In reply to Arnd Bergmann

Quoted 13 lines · On Thu, 26 Jul 2007 13:23:37 +0200 Arnd Bergmann <[email protected]> wrote:

Quoted 11 lines · Trond Myklebust
Quoted 2 lines · Christian Krafft
Obviously the locking code in nfs_free_open_context is wrong.
Checking the list for entries and removing the entry should be an atomic operation.
Wrong. It is quite safe to test the structure member ctx->list for emptiness outside the spinlock because we have an explicit guarantee that nobody else has a reference to this structure, plus the atomic_dec_and_test() in kref_put() has acted as a memory barrier for us.

Quoted 6 lines

Well, the real question then is how the ctx can still be present in the nfsi->open_files list. Since we are in nfs_free_open_context(), there must not be any pointer to the ctx anywhere, but still we have this other thread calling get_nfs_open_context() on it.
	Arnd <><

Thanks for the pointer Arnd,

Trond, does the patch below look better to you ?

Note:
For now the list_del_init is needed, so that the BUG_ON(!list_empty) works correctly. (I have no clue why list_empty should not be working, when the list has been poisoned by a list_del.) I am running the stress tests now. It seems to work so far.
---

Subject: nfs: fix locking in nfs/inode.c in nfs_free_open_context

From: Christian Krafft <[email protected]>

While running some tests on a server using NFS root I found some locking problems.

The scripts I am using can be found under at

http://localhorst.gotdns.org/parabelboi/nfs/

Please note, I am using rpm to generate read load. I found no better way to generate that much stress on the read path ;-)

The problem occurs at least on 2.6.22 and 2.6.23-rc1. It shows up like this:

Call Trace:
[c00000007aebf3e0] [c00000000033d9f4] ._spin_lock+0x10/0x24 (unreliable)
[c00000007aebf460] [c00000000014cf40] .nfs_find_open_context+0x68/0xcc
[c00000007aebf500] [c000000000156654] .nfs_writepage_locked+0x164/0x200
[c00000007aebf600] [c00000000015670c] .nfs_writepage+0x1c/0x48
[c00000007aebf690] [c00000000008e53c] .__writepage+0x3c/0x84
[c00000007aebf710] [c00000000008ed84] .write_cache_pages+0x238/0x3f4
[c00000007aebf880] [c000000000155840] .nfs_writepages+0x7c/0xc0
[c00000007aebf970] [c00000000008efe0] .do_writepages+0x68/0xa4
[c00000007aebf9f0] [c0000000000e0290] .__writeback_single_inode+0x1bc/0x3b8
[c00000007aebfae0] [c0000000000e0984] .sync_sb_inodes+0x20c/0x308
[c00000007aebfb90] [c0000000000e0b50] .sync_inodes_sb+0xd0/0x100
[c00000007aebfc80] [c0000000000e0c14] .__sync_inodes+0x94/0x120
nstruction dump:
4e800421 e8410028 38000001 38210080 7c030378 e8010010 ebc1fff0 7c0803a6 
4e800020 80030000 7c000034 5400d97e <0b000000> 7c001828 30000001 7c00192d 
Unable to handle kernel paging request for data at address 0x00200200
Faulting instruction address: 0xc000000000193474
cpu 0x0: Vector: 300 (Data Access) at [c00000000ff4b5b0]
    pc: c000000000193474: .list_del+0x20/0xa4
    lr: c00000000014c2e8: .nfs_free_open_context+0x4c/0xc4
    sp: c00000000ff4b830
   msr: 9000000000009032
   dar: 200200
 dsisr: 40000000
  current = 0xc00000000ff0a230
  paca    = 0xc00000000047e900
    pid   = 506, comm = rpciod/0

After applying the patch the scripts run a bit longer, but another error occurs :-/

Signed-off-by Christian Krafft <[email protected]>

fs/nfs/inode.c +3 -7

Index: linux-2.6.23-rc1/fs/nfs/inode.c
===================================================================
@@ -485,12 +485,8 @@ static void nfs_free_open_context(struct
 	struct nfs_open_context *ctx = container_of(kref,
 			struct nfs_open_context, kref);
 
-	if (!list_empty(&ctx->list)) {
-		struct inode *inode = ctx->path.dentry->d_inode;
-		spin_lock(&inode->i_lock);
-		list_del(&ctx->list);
-		spin_unlock(&inode->i_lock);
-	}
+	BUG_ON(!list_empty(&ctx->list));
+
 	if (ctx->state != NULL)
 		nfs4_close_state(&ctx->path, ctx->state, ctx->mode);
 	if (ctx->cred != NULL)

@@ -549,7 +545,7 @@ static void nfs_file_clear_open_context(
 	if (ctx) {
 		filp->private_data = NULL;
 		spin_lock(&inode->i_lock);
-		list_move_tail(&ctx->list, &NFS_I(inode)->open_files);
+		list_del_init(&ctx->list);
 		spin_unlock(&inode->i_lock);
 		put_nfs_open_context(ctx);
 	}

Signature · 13 lines

Mit freundlichen Gruessen,
kind regards,

Christian Krafft
IBM Systems & Technology Group,
Linux Kernel Development
IT Specialist


Vorsitzender des Aufsichtsrats:	Martin Jetter
Geschaeftsfuehrung:		Herbert Kircher
Sitz der Gesellschaft:		Boeblingen
Registriergericht:		Amtsgericht Stuttgart, HRB 243294

Trond Myklebust

2007-07-26 13:23 UTC #5 lore raw

Quoted 21 lines · Christian Krafft

Quoted 18 lines · On Thu, 26 Jul 2007 13:23:37 +0200 Arnd Bergmann <[email protected]> wrote:
Quoted 11 lines · Trond Myklebust
Quoted 2 lines · Christian Krafft
Obviously the locking code in nfs_free_open_context is wrong.
Checking the list for entries and removing the entry should be an atomic operation.
Wrong. It is quite safe to test the structure member ctx->list for emptiness outside the spinlock because we have an explicit guarantee that nobody else has a reference to this structure, plus the atomic_dec_and_test() in kref_put() has acted as a memory barrier for us.
Well, the real question then is how the ctx can still be present in the nfsi->open_files list. Since we are in nfs_free_open_context(), there must not be any pointer to the ctx anywhere, but still we have this other thread calling get_nfs_open_context() on it.

Quoted 5 lines

Quoted 1 line
	Arnd <><
Thanks for the pointer Arnd,
Trond, does the patch below look better to you ?

No. That is still incorrect. The list of open contexts is used for things like NFSv4 state recovery (when we're doing background writes, and the server happens to reboot). The lifetime of the open context may exceed that of the struct file that created it.

Trond

Arnd Bergmann

2007-07-26 15:23 UTC #6 lore raw

Quoted 12 lines · Trond Myklebust

Quoted 7 lines
Quoted 7 lines
Quoted 6 lines
Wrong. It is quite safe to test the structure member ctx->list for emptiness outside the spinlock because we have an explicit guarantee that nobody else has a reference to this structure, plus the atomic_dec_and_test() in kref_put() has acted as a memory barrier for us
Quoted 5 lines
Quoted 4 lines
Well, the real question then is how the ctx can still be present in the nfsi->open_files list. Since we are in nfs_free_open_context(), there must not be any pointer to the ctx anywhere, but still we have this other thread calling get_nfs_open_context() on it

Quoted 4 lines

No. That is still incorrect. The list of open contexts is used for things like NFSv4 state recovery (when we're doing background writes, and the server happens to reboot). The lifetime of the open context may exceed that of the struct file that created it.

Unfortunately, you didn't answer my question. The observed problem is that the final kref_put gets called at a time where there are still references to the context in nfsi->open_files, and other threads therefore do get at them.

The list_empty()/list_del() in nfs_free_open_context is bogus, because as you say, there is a guarantee that there is no other reference on the structure, and I take it that ought to include the ->open_files list.

I would guess that a quick fix for this looks something like the patch below. The race is in put_nfs_open_context() between the point where we check the reference count and the point where the context gets removed from the open_files list. If another thread searches the list between these points and grabs a new reference, we die.

The patch holds the i_lock around the kref_put to prevent others from searching the list. Ugly, I know, but it seems that's the price you pay for using a kref in such unconventional ways, i.e. not counting every reference.

	Arnd <><

fs/nfs/inode.c +6 -2

@@ -487,10 +487,10 @@ static void nfs_free_open_context(struct kref *kref)
 
 	if (!list_empty(&ctx->list)) {
 		struct inode *inode = ctx->path.dentry->d_inode;
-		spin_lock(&inode->i_lock);
 		list_del(&ctx->list);
-		spin_unlock(&inode->i_lock);
 	}
+	spin_unlock(&inode->i_lock);
+
 	if (ctx->state != NULL)
 		nfs4_close_state(&ctx->path, ctx->state, ctx->mode);
 	if (ctx->cred != NULL)

@@ -498,11 +498,15 @@ static void nfs_free_open_context(struct kref *kref)
 	dput(ctx->path.dentry);
 	mntput(ctx->path.mnt);
 	kfree(ctx);
+
+	spin_lock(&inode->i_lock);
 }
 
 void put_nfs_open_context(struct nfs_open_context *ctx)
 {
+	spin_lock(&inode->i_lock);
 	kref_put(&ctx->kref, nfs_free_open_context);
+	spin_unlock(&inode->i_lock);
 }
 
 /*

Trond Myklebust

2007-07-26 16:08 UTC #7 lore raw

Quoted 4 lines · Arnd Bergmann

Unfortunately, you didn't answer my question. The observed problem is that the final kref_put gets called at a time where there are still references to the context in nfsi->open_files, and other threads therefore do get at them.

Actually, I thought I did: I said we need to grab the lock atomically with the last put. See below.

Quoted 4 lines

The patch holds the i_lock around the kref_put to prevent others from searching the list. Ugly, I know, but it seems that's the price you pay for using a kref in such unconventional ways, i.e. not counting every reference.

Really ugly. Here's an alternative that is a lot more palatable.

Trond

---------------------
From: Trond Myklebust <[email protected]>
Date: Thu, 26 Jul 2007 12:06:17 -0400
NFS: Fix put_nfs_open_context

We need to grab the inode->i_lock atomically with the last reference put in order to remove the open context that is being freed from the nfsi->open_files list.

Fix by converting the kref to a standard atomic counter and then using atomic_dec_and_lock()...

Signed-off-by Trond Myklebust <[email protected]>

---

 fs/nfs/inode.c         |   24 ++++++++----------------
 include/linux/nfs_fs.h |    2 +-
 2 files changed, 9 insertions(+), 17 deletions(-)

fs/nfs/inode.c +8 ~~-16~~

index bca6cdc..71a49c3 100644
@@ -468,7 +468,7 @@ static struct nfs_open_context *alloc_nfs_open_context(struct vfsmount *mnt, str
 		ctx->lockowner = current->files;
 		ctx->error = 0;
 		ctx->dir_cookie = 0;
-		kref_init(&ctx->kref);
+		atomic_set(&ctx->count, 1);
 	}
 	return ctx;
 }

@@ -476,21 +476,18 @@ static struct nfs_open_context *alloc_nfs_open_context(struct vfsmount *mnt, str
 struct nfs_open_context *get_nfs_open_context(struct nfs_open_context *ctx)
 {
 	if (ctx != NULL)
-		kref_get(&ctx->kref);
+		atomic_inc(&ctx->count);
 	return ctx;
 }
 
-static void nfs_free_open_context(struct kref *kref)
+void put_nfs_open_context(struct nfs_open_context *ctx)
 {
-	struct nfs_open_context *ctx = container_of(kref,
-			struct nfs_open_context, kref);
+	struct inode *inode = ctx->path.dentry->d_inode;
 
-	if (!list_empty(&ctx->list)) {
-		struct inode *inode = ctx->path.dentry->d_inode;
-		spin_lock(&inode->i_lock);
-		list_del(&ctx->list);
-		spin_unlock(&inode->i_lock);
-	}
+	if (!atomic_dec_and_lock(&ctx->count, &inode->i_lock))
+		return;
+	list_del(&ctx->list);
+	spin_unlock(&inode->i_lock);
 	if (ctx->state != NULL)
 		nfs4_close_state(&ctx->path, ctx->state, ctx->mode);
 	if (ctx->cred != NULL)

@@ -500,11 +497,6 @@ static void nfs_free_open_context(struct kref *kref)
 	kfree(ctx);
 }
 
-void put_nfs_open_context(struct nfs_open_context *ctx)
-{
-	kref_put(&ctx->kref, nfs_free_open_context);
-}
-
 /*
  * Ensure that mmap has a recent RPC credential for use when writing out
  * shared pages

include/linux/nfs_fs.h +1 -1

index 9ba4aec..157dcb0 100644
@@ -71,7 +71,7 @@ struct nfs_access_entry {
 
 struct nfs4_state;
 struct nfs_open_context {
-	struct kref kref;
+	atomic_t count;
 	struct path path;
 	struct rpc_cred *cred;
 	struct nfs4_state *state;

Christian Krafft

author

2007-07-26 18:01 UTC #8 lore raw

Quoted 9 lines · On Thu, 26 Jul 2007 12:08:17 -0400 Trond Myklebust…

Quoted 4 lines · Arnd Bergmann
Unfortunately, you didn't answer my question. The observed problem is that the final kref_put gets called at a time where there are still references to the context in nfsi->open_files, and other threads therefore do get at them.
Actually, I thought I did: I said we need to grab the lock atomically with the last put. See below.

Quoted 6 lines

Quoted 4 lines
The patch holds the i_lock around the kref_put to prevent others from searching the list. Ugly, I know, but it seems that's the price you pay for using a kref in such unconventional ways, i.e. not counting every reference.
Really ugly. Here's an alternative that is a lot more palatable.

Indeed, and it also compiles ;-)
I started a test run using this patch, we'll see in a few hours ...

Signature · 13 lines

Mit freundlichen Gruessen,
kind regards,

Christian Krafft
IBM Systems & Technology Group,
Linux Kernel Development
IT Specialist


Vorsitzender des Aufsichtsrats:	Martin Jetter
Geschaeftsfuehrung:		Herbert Kircher
Sitz der Gesellschaft:		Boeblingen
Registriergericht:		Amtsgericht Stuttgart, HRB 243294

Christian Krafft

author

2007-07-27 10:03 UTC #9 lore raw

Hi Trond,

Quoted 4 lines · On Thu, 26 Jul 2007 20:00:47 +0200 Christian Krafft <[email protected]> wrote:

Indeed, and it also compiles ;-)
I started a test run using this patch, we'll see in a few hours ...

Scripts were running over night, no hang occurred. Would be great to see this patch integrated.

Signature · 13 lines

Mit freundlichen Gruessen,
kind regards,

Christian Krafft
IBM Systems & Technology Group,
Linux Kernel Development
IT Specialist


Vorsitzender des Aufsichtsrats:	Martin Jetter
Geschaeftsfuehrung:		Herbert Kircher
Sitz der Gesellschaft:		Boeblingen
Registriergericht:		Amtsgericht Stuttgart, HRB 243294

Arnd Bergmann

Acked-by

2007-07-27 10:41 UTC #10 lore raw

In reply to Trond Myklebust

Quoted 1 line · Trond Myklebust

Really ugly. Here's an alternative that is a lot more palatable.

Yes, much better than mine.

Quoted 5 lines

---------------------
From: Trond Myklebust <[email protected]>
Date: Thu, 26 Jul 2007 12:06:17 -0400
NFS: Fix put_nfs_open_context

Quoted 8 lines

We need to grab the inode->i_lock atomically with the last reference put in order to remove the open context that is being freed from the nfsi->open_files list.
Fix by converting the kref to a standard atomic counter and then using atomic_dec_and_lock()...
Signed-off-by Trond Myklebust <[email protected]>

Acked-by Arnd Bergmann <[email protected]>

Quoted 1 line

From the bit of research I did on the bug yesterday, it seems

that the race has been in there for ages, but may have become easier to hit with the change to kref after 2.6.22. I don't really understand why we didn't hit it in RHEL5/2.6.18 with the same test case, but in Fedora 7/2.6.21.

Should a patch like the one below (I said like, you know you can't trust my patches for your code ;-) be put into 2.6.22.x and updated distro kernels?

	Arnd <><

fs/nfs/inode.c +13 ~~-15~~

@@ -482,21 +482,19 @@ struct nfs_open_context *get_nfs_open_context(struct nfs_open_context *ctx)
 
 void put_nfs_open_context(struct nfs_open_context *ctx)
 {
-	if (atomic_dec_and_test(&ctx->count)) {
-		if (!list_empty(&ctx->list)) {
-			struct inode *inode = ctx->dentry->d_inode;
-			spin_lock(&inode->i_lock);
-			list_del(&ctx->list);
-			spin_unlock(&inode->i_lock);
-		}
-		if (ctx->state != NULL)
-			nfs4_close_state(ctx->state, ctx->mode);
-		if (ctx->cred != NULL)
-			put_rpccred(ctx->cred);
-		dput(ctx->dentry);
-		mntput(ctx->vfsmnt);
-		kfree(ctx);
-	}
+	struct inode *inode = ctx->dentry->d_inode;
+
+	if (!atomic_dec_and_lock(&ctx->count, &inode->i_lock))
+		return;
+	list_del(&ctx->list);
+	spin_unlock(&inode->i_lock);
+	if (ctx->state != NULL)
+		nfs4_close_state(ctx->state, ctx->mode);
+	if (ctx->cred != NULL)
+		put_rpccred(ctx->cred);
+	dput(ctx->dentry);
+	mntput(ctx->vfsmnt);
+	kfree(ctx);
 }
 
 /*