nfsd: lease handling cleanups

PATCH In review Jeff Layton 3 participants 9 messages Aug 9, 2014 – Aug 11, 2014

Subsystems (2)

NFS client NFS server

lore

Share & tools

lore

Jeff Layton

author

2014-08-09 14:22 UTC #0 lore raw

cover nfsd: lease handling cleanups

This patchset represents some small-ish lease-handling cleanups. In particular, it moves the vfs_setlease F_UNLCK call out from under any spinlocks, which is necessary for eventually allowing ->setlease methods to block.

These aren't strictly bugfixes, so they can probably wait until v3.18 if needed.

Jeff Layton (3):
nfsd: protect lease-related nfs4_file fields with fi_lock
nfsd: call nfs4_put_deleg_lease outside of state_lock
nfsd: allow find_any_file to return a fi_deleg_file reference

 fs/nfsd/nfs4state.c | 45 +++++++++++++++++++++++++++++++--------------
 1 file changed, 31 insertions(+), 14 deletions(-)

Signature · 1 line

1.9.3

Jeff Layton

author

2014-08-09 14:22 UTC #1 lore raw

PATCH 1/3 nfsd: protect lease-related nfs4_file fields with fi_lock

Currently these fields are protected with the state_lock, but that doesn't really make a lot of sense. These fields are "private" to the nfs4_file, and can be protected with the more granular fi_lock.

The fi_lock is already held when setting these fields. Make the code hold the fp->fi_lock when clearing the lease-related fields in the nfs4_file, and no longer require that the state_lock be held when calling into this function.

To prevent lock inversion with the i_lock, we also move the vfs_setlease and fput calls outside of the fi_lock. This also sets us up for allowing vfs_setlease calls to block in the future.

Finally, remove a redundant NULL pointer check. unhash_delegation_locked locks the fp->fi_lock prior to that check, so fp in that function must never be NULL.

Signed-off-by Jeff Layton <[email protected]>

---
 fs/nfsd/nfs4state.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

fs/nfsd/nfs4state.c +13 -9

index 2e80a59e7e91..309ec3b1090a 100644
@@ -673,15 +673,20 @@ nfs4_put_stid(struct nfs4_stid *s)
 
 static void nfs4_put_deleg_lease(struct nfs4_file *fp)
 {
-	lockdep_assert_held(&state_lock);
+	struct file *filp = NULL;
+	struct file_lock *fl;
 
-	if (!fp->fi_lease)
-		return;
-	if (atomic_dec_and_test(&fp->fi_delegees)) {
-		vfs_setlease(fp->fi_deleg_file, F_UNLCK, &fp->fi_lease);
+	spin_lock(&fp->fi_lock);
+	if (fp->fi_lease && atomic_dec_and_test(&fp->fi_delegees)) {
+		swap(filp, fp->fi_deleg_file);
+		fl = fp->fi_lease;
 		fp->fi_lease = NULL;
-		fput(fp->fi_deleg_file);
-		fp->fi_deleg_file = NULL;
+	}
+	spin_unlock(&fp->fi_lock);
+
+	if (filp) {
+		vfs_setlease(filp, F_UNLCK, &fl);
+		fput(filp);
 	}
 }

@@ -717,8 +722,7 @@ unhash_delegation_locked(struct nfs4_delegation *dp)
 	list_del_init(&dp->dl_recall_lru);
 	list_del_init(&dp->dl_perfile);
 	spin_unlock(&fp->fi_lock);
-	if (fp)
-		nfs4_put_deleg_lease(fp);
+	nfs4_put_deleg_lease(fp);
 }
 
 static void destroy_delegation(struct nfs4_delegation *dp)

Signature · 1 line

1.9.3

Jeff Layton

author

2014-08-09 14:22 UTC #2 lore raw

PATCH 2/3 nfsd: call nfs4_put_deleg_lease outside of state_lock

In reply to Jeff Layton

Currently, we hold the state_lock when releasing the lease. That's potentially problematic in the future if we allow for setlease methods that can sleep. Move the nfs4_put_deleg_lease call out of the delegation unhashing routine (which was always a bit goofy anyway), and into the unlocked sections of the callers of unhash_delegation_locked.

Signed-off-by Jeff Layton <[email protected]>

---
 fs/nfsd/nfs4state.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

fs/nfsd/nfs4state.c +5 -1

index 309ec3b1090a..4356d32479b2 100644
@@ -722,7 +722,6 @@ unhash_delegation_locked(struct nfs4_delegation *dp)
 	list_del_init(&dp->dl_recall_lru);
 	list_del_init(&dp->dl_perfile);
 	spin_unlock(&fp->fi_lock);
-	nfs4_put_deleg_lease(fp);
 }
 
 static void destroy_delegation(struct nfs4_delegation *dp)

@@ -730,6 +729,7 @@ static void destroy_delegation(struct nfs4_delegation *dp)
 	spin_lock(&state_lock);
 	unhash_delegation_locked(dp);
 	spin_unlock(&state_lock);
+	nfs4_put_deleg_lease(dp->dl_stid.sc_file);
 	nfs4_put_stid(&dp->dl_stid);
 }

@@ -739,6 +739,8 @@ static void revoke_delegation(struct nfs4_delegation *dp)
 
 	WARN_ON(!list_empty(&dp->dl_recall_lru));
 
+	nfs4_put_deleg_lease(dp->dl_stid.sc_file);
+
 	if (clp->cl_minorversion == 0)
 		nfs4_put_stid(&dp->dl_stid);
 	else {

@@ -1639,6 +1641,7 @@ __destroy_client(struct nfs4_client *clp)
 	while (!list_empty(&reaplist)) {
 		dp = list_entry(reaplist.next, struct nfs4_delegation, dl_recall_lru);
 		list_del_init(&dp->dl_recall_lru);
+		nfs4_put_deleg_lease(dp->dl_stid.sc_file);
 		nfs4_put_stid(&dp->dl_stid);
 	}
 	while (!list_empty(&clp->cl_revoked)) {

@@ -6406,6 +6409,7 @@ nfs4_state_shutdown_net(struct net *net)
 	list_for_each_safe(pos, next, &reaplist) {
 		dp = list_entry (pos, struct nfs4_delegation, dl_recall_lru);
 		list_del_init(&dp->dl_recall_lru);
+		nfs4_put_deleg_lease(dp->dl_stid.sc_file);
 		nfs4_put_stid(&dp->dl_stid);
 	}

Signature · 1 line

1.9.3

Jeff Layton

author

2014-08-09 14:22 UTC #3 lore raw

PATCH 3/3 nfsd: allow find_any_file to return a fi_deleg_file reference

In reply to Jeff Layton

It's possible that we'll have a nfs4_file that has nothing in its fds array, but that has a populated fi_deleg_file field. Since that function is generally happy with any file reference, allow it to find and take a reference to it in that situation. Also, clean up find_any_file for better readability.

Signed-off-by Jeff Layton <[email protected]>

---
 fs/nfsd/nfs4state.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

fs/nfsd/nfs4state.c +14 -5

index 4356d32479b2..5c5873811bd9 100644
@@ -355,11 +355,20 @@ find_any_file(struct nfs4_file *f)
 
 	spin_lock(&f->fi_lock);
 	ret = __nfs4_get_fd(f, O_RDWR);
-	if (!ret) {
-		ret = __nfs4_get_fd(f, O_WRONLY);
-		if (!ret)
-			ret = __nfs4_get_fd(f, O_RDONLY);
-	}
+	if (ret)
+		goto out;
+
+	ret = __nfs4_get_fd(f, O_WRONLY);
+	if (ret)
+		goto out;
+
+	ret = __nfs4_get_fd(f, O_RDONLY);
+	if (ret)
+		goto out;
+
+	if (f->fi_deleg_file)
+		ret = get_file(f->fi_deleg_file);
+out:
 	spin_unlock(&f->fi_lock);
 	return ret;
 }

Signature · 1 line

1.9.3

J. Bruce Fields

2014-08-11 16:08 UTC #4 lore raw

Re: [PATCH 3/3] nfsd: allow find_any_file to return a fi_deleg_file reference

Quoted 2 lines · Jeff Layton

It's possible that we'll have a nfs4_file that has nothing in its fds array, but that has a populated fi_deleg_file field.

Is it really possible? On a quick skim it looks like this is only used in the presence of lock stateid's, when we should have an open.

OK with the cleanup I'm just not seeing a reason either one way or the other for the fi_deleg_file change.

--b.

Quoted 43 lines

Since that function
is generally happy with any file reference, allow it to find and take a reference to it in that situation. Also, clean up find_any_file for better readability.

Signed-off-by Jeff Layton <[email protected]>

---
 fs/nfsd/nfs4state.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

fs/nfsd/nfs4state.c +14 -5

index 4356d32479b2..5c5873811bd9 100644
@@ -355,11 +355,20 @@ find_any_file(struct nfs4_file *f)
 
 	spin_lock(&f->fi_lock);
 	ret = __nfs4_get_fd(f, O_RDWR);
-	if (!ret) {
-		ret = __nfs4_get_fd(f, O_WRONLY);
-		if (!ret)
-			ret = __nfs4_get_fd(f, O_RDONLY);
-	}
+	if (ret)
+		goto out;
+
+	ret = __nfs4_get_fd(f, O_WRONLY);
+	if (ret)
+		goto out;
+
+	ret = __nfs4_get_fd(f, O_RDONLY);
+	if (ret)
+		goto out;
+
+	if (f->fi_deleg_file)
+		ret = get_file(f->fi_deleg_file);
+out:
 	spin_unlock(&f->fi_lock);
 	return ret;
 }

Signature · 1 line

1.9.3

Jeff Layton

2014-08-11 16:40 UTC #5 lore raw

Re: [PATCH 3/3] nfsd: allow find_any_file to return a fi_deleg_file reference

Quoted 4 lines · On Mon, 11 Aug 2014 12:08:40 -0400 "J. Bruce Fields" <[email protected]>…

Quoted 2 lines · Jeff Layton
It's possible that we'll have a nfs4_file that has nothing in its fds array, but that has a populated fi_deleg_file field.

Quoted 8 lines

Is it really possible? On a quick skim it looks like this is only used in the presence of lock stateid's, when we should have an open.
OK with the cleanup I'm just not seeing a reason either one way or the other for the fi_deleg_file change.
--b.

You're correct. The existing code doesn't specifically require this patch since find_any_file is only used with lock stateids. It should be harmless but it won't hurt anything to drop it.

I did however need this when I rebased some pnfsd patches on top of the state overhaul, and it seemed like a reasonable change from a "future-proofing" standpoint.

Do you intend to the take the first two in the series? I would like to see those go in since they move the lease removal outside of spinlocks.

Quoted 43 lines

Since that function
is generally happy with any file reference, allow it to find and take a reference to it in that situation. Also, clean up find_any_file for better readability.

Signed-off-by Jeff Layton <[email protected]>

---
 fs/nfsd/nfs4state.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

fs/nfsd/nfs4state.c +14 -5

index 4356d32479b2..5c5873811bd9 100644
@@ -355,11 +355,20 @@ find_any_file(struct nfs4_file *f)
 
 	spin_lock(&f->fi_lock);
 	ret = __nfs4_get_fd(f, O_RDWR);
-	if (!ret) {
-		ret = __nfs4_get_fd(f, O_WRONLY);
-		if (!ret)
-			ret = __nfs4_get_fd(f, O_RDONLY);
-	}
+	if (ret)
+		goto out;
+
+	ret = __nfs4_get_fd(f, O_WRONLY);
+	if (ret)
+		goto out;
+
+	ret = __nfs4_get_fd(f, O_RDONLY);
+	if (ret)
+		goto out;
+
+	if (f->fi_deleg_file)
+		ret = get_file(f->fi_deleg_file);
+out:
 	spin_unlock(&f->fi_lock);
 	return ret;
 }

Signature · 1 line

1.9.3

Signature · 1 line

Jeff Layton <[email protected]>

J. Bruce Fields

2014-08-11 17:09 UTC #6 lore raw

Re: [PATCH 3/3] nfsd: allow find_any_file to return a fi_deleg_file reference

Quoted 16 lines · Jeff Layton

Quoted 4 lines · On Mon, 11 Aug 2014 12:08:40 -0400 "J. Bruce Fields" <[email protected]>…
Quoted 2 lines · Jeff Layton
It's possible that we'll have a nfs4_file that has nothing in its fds array, but that has a populated fi_deleg_file field.
Quoted 8 lines
Is it really possible? On a quick skim it looks like this is only used in the presence of lock stateid's, when we should have an open.
OK with the cleanup I'm just not seeing a reason either one way or the other for the fi_deleg_file change.
--b.

Quoted 7 lines

You're correct. The existing code doesn't specifically require this patch since find_any_file is only used with lock stateids. It should be harmless but it won't hurt anything to drop it.
I did however need this when I rebased some pnfsd patches on top of the state overhaul, and it seemed like a reasonable change from a "future-proofing" standpoint.

So layout operations depend on this somehow? (But layouts can outlast delegations, so that must not be it.)

I'm not opposed to future-proofing as long as we have some evidence about the future.

Quoted 2 lines

Do you intend to the take the first two in the series? I would like to see those go in since they move the lease removal outside of spinlocks.

Yes, just waiting for -rc1 comes out to push out a for-3.18 tree.

--b.

Quoted 49 lines

Quoted 45 lines

Since that function
is generally happy with any file reference, allow it to find and take a reference to it in that situation. Also, clean up find_any_file for better readability.

Signed-off-by Jeff Layton <[email protected]>

---
 fs/nfsd/nfs4state.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

fs/nfsd/nfs4state.c +14 -5

index 4356d32479b2..5c5873811bd9 100644
@@ -355,11 +355,20 @@ find_any_file(struct nfs4_file *f)
 
 	spin_lock(&f->fi_lock);
 	ret = __nfs4_get_fd(f, O_RDWR);
-	if (!ret) {
-		ret = __nfs4_get_fd(f, O_WRONLY);
-		if (!ret)
-			ret = __nfs4_get_fd(f, O_RDONLY);
-	}
+	if (ret)
+		goto out;
+
+	ret = __nfs4_get_fd(f, O_WRONLY);
+	if (ret)
+		goto out;
+
+	ret = __nfs4_get_fd(f, O_RDONLY);
+	if (ret)
+		goto out;
+
+	if (f->fi_deleg_file)
+		ret = get_file(f->fi_deleg_file);
+out:
 	spin_unlock(&f->fi_lock);
 	return ret;
 }

Signature · 1 line

1.9.3

Signature · 1 line

Jeff Layton <[email protected]>

Jeff Layton

2014-08-11 17:20 UTC #7 lore raw

Re: [PATCH 3/3] nfsd: allow find_any_file to return a fi_deleg_file reference

Quoted 25 lines · On Mon, 11 Aug 2014 13:09:37 -0400 "J. Bruce Fields" <[email protected]>…

Quoted 16 lines · Jeff Layton
Quoted 4 lines · On Mon, 11 Aug 2014 12:08:40 -0400 "J. Bruce Fields" <[email protected]>…
Quoted 2 lines · Jeff Layton
It's possible that we'll have a nfs4_file that has nothing in its fds array, but that has a populated fi_deleg_file field.
Quoted 8 lines
Is it really possible? On a quick skim it looks like this is only used in the presence of lock stateid's, when we should have an open.
OK with the cleanup I'm just not seeing a reason either one way or the other for the fi_deleg_file change.
--b.
Quoted 7 lines
You're correct. The existing code doesn't specifically require this patch since find_any_file is only used with lock stateids. It should be harmless but it won't hurt anything to drop it.
I did however need this when I rebased some pnfsd patches on top of the state overhaul, and it seemed like a reasonable change from a "future-proofing" standpoint.

Quoted 6 lines

So layout operations depend on this somehow? (But layouts can outlast delegations, so that must not be it.)
I'm not opposed to future-proofing as long as we have some evidence about the future.

The code I'm working on does depend on this, but it's may not be correct for any arbitrary filesystem.

This filesystem sets return_on_close, and we automatically return the layout to the fs when all of the open and deleg stateids go away. If the last one to drop its reference happens to be the delegation stateid, then you may have nothing in the fi_fds slots, and no way to get to the inode in order to return the layout.

I've been toying with the idea of keeping an inode reference in the layout state, but I'm not sure if it's the right thing to do...

Quoted 5 lines

Quoted 2 lines
Do you intend to the take the first two in the series? I would like to see those go in since they move the lease removal outside of spinlocks.
Yes, just waiting for -rc1 comes out to push out a for-3.18 tree.

Sounds good -- no rush on them.

Thanks!

Signature · 1 line

Jeff Layton <[email protected]>

J. Bruce Fields

2014-08-11 17:41 UTC #8 lore raw

Re: [PATCH 3/3] nfsd: allow find_any_file to return a fi_deleg_file reference

Quoted 38 lines · Jeff Layton

Quoted 25 lines · On Mon, 11 Aug 2014 13:09:37 -0400 "J. Bruce Fields" <[email protected]>…
Quoted 16 lines · Jeff Layton
Quoted 4 lines · On Mon, 11 Aug 2014 12:08:40 -0400 "J. Bruce Fields" <[email protected]>…
Quoted 2 lines · Jeff Layton
It's possible that we'll have a nfs4_file that has nothing in its fds array, but that has a populated fi_deleg_file field.
Quoted 8 lines
Is it really possible? On a quick skim it looks like this is only used in the presence of lock stateid's, when we should have an open.
OK with the cleanup I'm just not seeing a reason either one way or the other for the fi_deleg_file change.
--b.
Quoted 7 lines
You're correct. The existing code doesn't specifically require this patch since find_any_file is only used with lock stateids. It should be harmless but it won't hurt anything to drop it.
I did however need this when I rebased some pnfsd patches on top of the state overhaul, and it seemed like a reasonable change from a "future-proofing" standpoint.
Quoted 6 lines
So layout operations depend on this somehow? (But layouts can outlast delegations, so that must not be it.)
I'm not opposed to future-proofing as long as we have some evidence about the future.
The code I'm working on does depend on this, but it's may not be correct for any arbitrary filesystem.

Quoted 8 lines

This filesystem sets return_on_close, and we automatically return the layout to the fs when all of the open and deleg stateids go away. If the last one to drop its reference happens to be the delegation stateid, then you may have nothing in the fi_fds slots, and no way to get to the inode in order to return the layout.
I've been toying with the idea of keeping an inode reference in the layout state, but I'm not sure if it's the right thing to do...

OK, makes sense, thanks for the explanation. But I think I'll drop this third patch for now.

--b.

Quoted 12 lines

Quoted 6 lines
Quoted 3 lines
Do you intend to the take the first two in the series? I would like to see those go in since they move the lease removal outside of spinlocks.
Yes, just waiting for -rc1 comes out to push out a for-3.18 tree.
Sounds good -- no rush on them.
Thanks!
Signature · 1 line
Jeff Layton <[email protected]>