[PATCH 0/3] nfsd: lease handling cleanups

[PATCH 0/3] nfsd: lease handling cleanups

[Jeff Layton]

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(-)

-- 
1.9.3

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

[Jeff Layton]

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 <jlayton@primarydata.com>
---
 fs/nfsd/nfs4state.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 2e80a59e7e91..309ec3b1090a 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -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)
-- 
1.9.3

[PATCH 2/3] nfsd: call nfs4_put_deleg_lease outside of state_lock

[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 <jlayton@primarydata.com>
---
 fs/nfsd/nfs4state.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 309ec3b1090a..4356d32479b2 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -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);
 	}
 
-- 
1.9.3

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

[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 <jlayton@primarydata.com>
---
 fs/nfsd/nfs4state.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 4356d32479b2..5c5873811bd9 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -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;
 }
-- 
1.9.3

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

[J. Bruce Fields]

On Sat, Aug 09, 2014 at 10:22:42AM -0400, Jeff Layton wrote:
> 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.

> 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 <jlayton@primarydata.com>
> ---
>  fs/nfsd/nfs4state.c | 19 ++++++++++++++-----
>  1 file changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 4356d32479b2..5c5873811bd9 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -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;
>  }
> -- 
> 1.9.3
> 

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

[Jeff Layton]

On Mon, 11 Aug 2014 12:08:40 -0400
"J. Bruce Fields" <bfields@fieldses.org> wrote:

> On Sat, Aug 09, 2014 at 10:22:42AM -0400, Jeff Layton wrote:
> > 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.
> 

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.


> > 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 <jlayton@primarydata.com>
> > ---
> >  fs/nfsd/nfs4state.c | 19 ++++++++++++++-----
> >  1 file changed, 14 insertions(+), 5 deletions(-)
> > 
> > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > index 4356d32479b2..5c5873811bd9 100644
> > --- a/fs/nfsd/nfs4state.c
> > +++ b/fs/nfsd/nfs4state.c
> > @@ -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;
> >  }
> > -- 
> > 1.9.3
> > 


-- 
Jeff Layton <jlayton@primarydata.com>

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

[J. Bruce Fields]

On Mon, Aug 11, 2014 at 12:40:39PM -0400, Jeff Layton wrote:
> On Mon, 11 Aug 2014 12:08:40 -0400
> "J. Bruce Fields" <bfields@fieldses.org> wrote:
> 
> > On Sat, Aug 09, 2014 at 10:22:42AM -0400, Jeff Layton wrote:
> > > 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.
> > 
> 
> 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.

> 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.

> 
> 
> > > 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 <jlayton@primarydata.com>
> > > ---
> > >  fs/nfsd/nfs4state.c | 19 ++++++++++++++-----
> > >  1 file changed, 14 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > > index 4356d32479b2..5c5873811bd9 100644
> > > --- a/fs/nfsd/nfs4state.c
> > > +++ b/fs/nfsd/nfs4state.c
> > > @@ -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;
> > >  }
> > > -- 
> > > 1.9.3
> > > 
> 
> 
> -- 
> Jeff Layton <jlayton@primarydata.com>

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

[Jeff Layton]

On Mon, 11 Aug 2014 13:09:37 -0400
"J. Bruce Fields" <bfields@fieldses.org> wrote:

> On Mon, Aug 11, 2014 at 12:40:39PM -0400, Jeff Layton wrote:
> > On Mon, 11 Aug 2014 12:08:40 -0400
> > "J. Bruce Fields" <bfields@fieldses.org> wrote:
> > 
> > > On Sat, Aug 09, 2014 at 10:22:42AM -0400, Jeff Layton wrote:
> > > > 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.
> > > 
> > 
> > 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.
> 

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...

> > 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!
-- 
Jeff Layton <jlayton@primarydata.com>

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

[J. Bruce Fields]

On Mon, Aug 11, 2014 at 01:20:53PM -0400, Jeff Layton wrote:
> On Mon, 11 Aug 2014 13:09:37 -0400
> "J. Bruce Fields" <bfields@fieldses.org> wrote:
> 
> > On Mon, Aug 11, 2014 at 12:40:39PM -0400, Jeff Layton wrote:
> > > On Mon, 11 Aug 2014 12:08:40 -0400
> > > "J. Bruce Fields" <bfields@fieldses.org> wrote:
> > > 
> > > > On Sat, Aug 09, 2014 at 10:22:42AM -0400, Jeff Layton wrote:
> > > > > 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.
> > > > 
> > > 
> > > 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.
> > 
> 
> 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...

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

--b.

> 
> > > 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!
> -- 
> Jeff Layton <jlayton@primarydata.com>