[PATCH v5 00/10] nfsd: more delegation fixes to prepare for client_mutex removal

[PATCH v5 00/10] nfsd: more delegation fixes to prepare for client_mutex removal

[Jeff Layton]

v5:
- fix spinlocking in block_delegations. Lock should be held around all
  of the set_bit calls so we don't race with the swap of the two
  fields.

- eliminate destroy_revoked_delegation (just use nfs4_put_delegation)

- eliminate unneeded NULL pointer check in nfs4_setlease

v4:
- close more potential races in setlease code, and fix some bugs in
  error handling in that code.

- clean up delegation setting functions, eliminating unused arguments
  and avoiding allocations when there has already been a delegation
  break

- add separate spinlock for block_delegations/delegation_blocked code

v3:
- use alternate method for checking for delegation break races after
  getting a lease (just check fi_had_conflict instead)

- drop file_has_lease patch -- no longer needed

- move cl_revoked handling patch into this set. It means altering a
  few of the later patches, but it keeps the set more topically
  coherent

v2:
- move remove_stid call from nfs4_free_stid and into callers

Ok, I'm hoping we're now on the final respin here. This just cleans
up some nits that Christoph noticed in the last set. It also fixes
a potential bug that Neil noticed around the block_delegations
spinlocking.

Bruce, I noticed that you had merged the last set into your nfsd-next
branch, so let me know if you'd prefer me to do incremental changes
on top of that instead.

Jeff Layton (7):
  nfsd: Protect the nfs4_file delegation fields using the fi_lock
  nfsd: Fix delegation revocation
  nfsd: ensure that clp->cl_revoked list is protected by clp->cl_lock
  nfsd: drop unused stp arg to alloc_init_deleg
  nfsd: clean up arguments to nfs4_open_delegation
  nfsd: clean up nfs4_set_delegation
  nfsd: give block_delegation and delegation_blocked its own spinlock

Trond Myklebust (3):
  nfsd: Move the delegation reference counter into the struct nfs4_stid
  nfsd: simplify stateid allocation and file handling
  nfsd: Convert delegation counter to an atomic_long_t type

 fs/nfsd/nfs4state.c | 253 ++++++++++++++++++++++++++++++++--------------------
 fs/nfsd/state.h     |   2 +-
 2 files changed, 159 insertions(+), 96 deletions(-)

-- 
1.9.3

[PATCH v5 01/10] nfsd: Protect the nfs4_file delegation fields using the fi_lock

[Jeff Layton]

Move more of the delegation fields to be protected by the fi_lock. It's
more granular than the state_lock and in later patches we'll want to
be able to rely on it in addition to the state_lock.

Also, the current code in nfs4_setlease calls vfs_setlease and uses the
client_mutex to ensure that it doesn't disappear before we can hash the
delegation. With the client_mutex gone, we'll have a potential race
condition.

It's possible that the delegation could be recalled after we acquire the
lease but before we ever get around to hashing it. If that happens, then
we'd have a nfs4_file that *thinks* it has a delegation, when it
actually has none.

Attempt to acquire a delegation. If that succeeds, take the spinlocks
and then check to see if the file has had a conflict show up since then.
If it has, then we assume that the lease is no longer valid and that
we shouldn't hand out a delegation.

There's also one more potential (but very unlikely) problem. If the
lease is broken before the delegation is hashed, then it could leak.
In the event that the fi_delegations list is empty, reset the
fl_break_time to jiffies so that it's cleaned up ASAP by
the normal lease handling code.

Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
Signed-off-by: Jeff Layton <jlayton@primarydata.com>
---
 fs/nfsd/nfs4state.c | 89 ++++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 65 insertions(+), 24 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index fd4deb049ddf..b05ac229fff0 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -624,6 +624,8 @@ nfs4_put_delegation(struct nfs4_delegation *dp)
 
 static void nfs4_put_deleg_lease(struct nfs4_file *fp)
 {
+	lockdep_assert_held(&state_lock);
+
 	if (!fp->fi_lease)
 		return;
 	if (atomic_dec_and_test(&fp->fi_delegees)) {
@@ -643,11 +645,10 @@ static void
 hash_delegation_locked(struct nfs4_delegation *dp, struct nfs4_file *fp)
 {
 	lockdep_assert_held(&state_lock);
+	lockdep_assert_held(&fp->fi_lock);
 
 	dp->dl_stid.sc_type = NFS4_DELEG_STID;
-	spin_lock(&fp->fi_lock);
 	list_add(&dp->dl_perfile, &fp->fi_delegations);
-	spin_unlock(&fp->fi_lock);
 	list_add(&dp->dl_perclnt, &dp->dl_stid.sc_client->cl_delegations);
 }
 
@@ -659,17 +660,18 @@ unhash_delegation(struct nfs4_delegation *dp)
 
 	spin_lock(&state_lock);
 	dp->dl_stid.sc_type = NFS4_CLOSED_DELEG_STID;
+	spin_lock(&fp->fi_lock);
 	list_del_init(&dp->dl_perclnt);
 	list_del_init(&dp->dl_recall_lru);
-	spin_lock(&fp->fi_lock);
 	list_del_init(&dp->dl_perfile);
 	spin_unlock(&fp->fi_lock);
-	spin_unlock(&state_lock);
 	if (fp) {
 		nfs4_put_deleg_lease(fp);
-		put_nfs4_file(fp);
 		dp->dl_file = NULL;
 	}
+	spin_unlock(&state_lock);
+	if (fp)
+		put_nfs4_file(fp);
 }
 
 static void destroy_revoked_delegation(struct nfs4_delegation *dp)
@@ -3143,10 +3145,19 @@ static void nfsd_break_deleg_cb(struct file_lock *fl)
 	 */
 	fl->fl_break_time = 0;
 
-	fp->fi_had_conflict = true;
 	spin_lock(&fp->fi_lock);
-	list_for_each_entry(dp, &fp->fi_delegations, dl_perfile)
-		nfsd_break_one_deleg(dp);
+	fp->fi_had_conflict = true;
+	/*
+	 * If there are no delegations on the list, then we can't count on this
+	 * lease ever being cleaned up. Set the fl_break_time to jiffies so that
+	 * time_out_leases will do it ASAP. The fact that fi_had_conflict is now
+	 * true should keep any new delegations from being hashed.
+	 */
+	if (list_empty(&fp->fi_delegations))
+		fl->fl_break_time = jiffies;
+	else
+		list_for_each_entry(dp, &fp->fi_delegations, dl_perfile)
+			nfsd_break_one_deleg(dp);
 	spin_unlock(&fp->fi_lock);
 }
 
@@ -3493,46 +3504,76 @@ static int nfs4_setlease(struct nfs4_delegation *dp)
 {
 	struct nfs4_file *fp = dp->dl_file;
 	struct file_lock *fl;
-	int status;
+	struct file *filp;
+	int status = 0;
 
 	fl = nfs4_alloc_init_lease(fp, NFS4_OPEN_DELEGATE_READ);
 	if (!fl)
 		return -ENOMEM;
-	fl->fl_file = find_readable_file(fp);
-	status = vfs_setlease(fl->fl_file, fl->fl_type, &fl);
-	if (status)
-		goto out_free;
+	filp = find_readable_file(fp);
+	if (!filp) {
+		/* We should always have a readable file here */
+		WARN_ON_ONCE(1);
+		return -EBADF;
+	}
+	status = vfs_setlease(filp, fl->fl_type, &fl);
+	if (status) {
+		locks_free_lock(fl);
+		goto out_fput;
+	}
+	spin_lock(&state_lock);
+	spin_lock(&fp->fi_lock);
+	/* Did the lease get broken before we took the lock? */
+	status = -EAGAIN;
+	if (fp->fi_had_conflict)
+		goto out_unlock;
+	/* Race breaker */
+	if (fp->fi_lease) {
+		status = 0;
+		atomic_inc(&fp->fi_delegees);
+		hash_delegation_locked(dp, fp);
+		goto out_unlock;
+	}
 	fp->fi_lease = fl;
-	fp->fi_deleg_file = fl->fl_file;
+	fp->fi_deleg_file = filp;
 	atomic_set(&fp->fi_delegees, 1);
-	spin_lock(&state_lock);
 	hash_delegation_locked(dp, fp);
+	spin_unlock(&fp->fi_lock);
 	spin_unlock(&state_lock);
 	return 0;
-out_free:
-	if (fl->fl_file)
-		fput(fl->fl_file);
-	locks_free_lock(fl);
+out_unlock:
+	spin_unlock(&fp->fi_lock);
+	spin_unlock(&state_lock);
+out_fput:
+	fput(filp);
 	return status;
 }
 
 static int nfs4_set_delegation(struct nfs4_delegation *dp, struct nfs4_file *fp)
 {
+	int status = 0;
+
 	if (fp->fi_had_conflict)
 		return -EAGAIN;
 	get_nfs4_file(fp);
+	spin_lock(&state_lock);
+	spin_lock(&fp->fi_lock);
 	dp->dl_file = fp;
-	if (!fp->fi_lease)
+	if (!fp->fi_lease) {
+		spin_unlock(&fp->fi_lock);
+		spin_unlock(&state_lock);
 		return nfs4_setlease(dp);
-	spin_lock(&state_lock);
+	}
 	atomic_inc(&fp->fi_delegees);
 	if (fp->fi_had_conflict) {
-		spin_unlock(&state_lock);
-		return -EAGAIN;
+		status = -EAGAIN;
+		goto out_unlock;
 	}
 	hash_delegation_locked(dp, fp);
+out_unlock:
+	spin_unlock(&fp->fi_lock);
 	spin_unlock(&state_lock);
-	return 0;
+	return status;
 }
 
 static void nfsd4_open_deleg_none_ext(struct nfsd4_open *open, int status)
-- 
1.9.3

Re: [PATCH v5 01/10] nfsd: Protect the nfs4_file delegation fields using the fi_lock

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH v5 02/10] nfsd: Move the delegation reference counter into the struct nfs4_stid

[Jeff Layton]

From: Trond Myklebust <trond.myklebust@primarydata.com>

We will want to add reference counting to the lock stateid and open
stateids too in later patches.

Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 fs/nfsd/nfs4state.c | 6 +++---
 fs/nfsd/state.h     | 2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index b05ac229fff0..a47c97586c76 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -472,6 +472,7 @@ kmem_cache *slab)
 	stid->sc_stateid.si_opaque.so_clid = cl->cl_clientid;
 	/* Will be incremented before return to client: */
 	stid->sc_stateid.si_generation = 0;
+	atomic_set(&stid->sc_count, 1);
 
 	/*
 	 * It shouldn't be a problem to reuse an opaque stateid value.
@@ -595,7 +596,6 @@ alloc_init_deleg(struct nfs4_client *clp, struct nfs4_ol_stateid *stp, struct sv
 	dp->dl_type = NFS4_OPEN_DELEGATE_READ;
 	fh_copy_shallow(&dp->dl_fh, &current_fh->fh_handle);
 	dp->dl_time = 0;
-	atomic_set(&dp->dl_count, 1);
 	INIT_WORK(&dp->dl_recall.cb_work, nfsd4_run_cb_recall);
 	return dp;
 }
@@ -615,7 +615,7 @@ static void nfs4_free_stid(struct kmem_cache *slab, struct nfs4_stid *s)
 void
 nfs4_put_delegation(struct nfs4_delegation *dp)
 {
-	if (atomic_dec_and_test(&dp->dl_count)) {
+	if (atomic_dec_and_test(&dp->dl_stid.sc_count)) {
 		remove_stid(&dp->dl_stid);
 		nfs4_free_stid(deleg_slab, &dp->dl_stid);
 		num_delegations--;
@@ -3120,7 +3120,7 @@ static void nfsd_break_one_deleg(struct nfs4_delegation *dp)
 	 * lock) we know the server hasn't removed the lease yet, we know
 	 * it's safe to take a reference.
 	 */
-	atomic_inc(&dp->dl_count);
+	atomic_inc(&dp->dl_stid.sc_count);
 	nfsd4_cb_recall(dp);
 }
 
diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
index 996d61eeb357..e68a9ae30fd7 100644
--- a/fs/nfsd/state.h
+++ b/fs/nfsd/state.h
@@ -73,6 +73,7 @@ struct nfsd4_callback {
 };
 
 struct nfs4_stid {
+	atomic_t sc_count;
 #define NFS4_OPEN_STID 1
 #define NFS4_LOCK_STID 2
 #define NFS4_DELEG_STID 4
@@ -91,7 +92,6 @@ struct nfs4_delegation {
 	struct list_head	dl_perfile;
 	struct list_head	dl_perclnt;
 	struct list_head	dl_recall_lru;  /* delegation recalled */
-	atomic_t		dl_count;       /* ref count */
 	struct nfs4_file	*dl_file;
 	u32			dl_type;
 	time_t			dl_time;
-- 
1.9.3

[PATCH v5 03/10] nfsd: simplify stateid allocation and file handling

[Jeff Layton]

From: Trond Myklebust <trond.myklebust@primarydata.com>

Don't allow stateids to clear the open file pointer until they are
being destroyed. In a later patch we'll want to move the putting of
the nfs4_file into generic stateid handling code and this will help
facilitate that change.

Also, move to allocating stateids with kzalloc and get rid of the
explicit zeroing of fields.

Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
Signed-off-by: Jeff Layton <jlayton@primarydata.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 fs/nfsd/nfs4state.c | 22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index a47c97586c76..8ff5c97a2c5a 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -459,7 +459,7 @@ kmem_cache *slab)
 	struct nfs4_stid *stid;
 	int new_id;
 
-	stid = kmem_cache_alloc(slab, GFP_KERNEL);
+	stid = kmem_cache_zalloc(slab, GFP_KERNEL);
 	if (!stid)
 		return NULL;
 
@@ -467,11 +467,9 @@ kmem_cache *slab)
 	if (new_id < 0)
 		goto out_free;
 	stid->sc_client = cl;
-	stid->sc_type = 0;
 	stid->sc_stateid.si_opaque.so_id = new_id;
 	stid->sc_stateid.si_opaque.so_clid = cl->cl_clientid;
 	/* Will be incremented before return to client: */
-	stid->sc_stateid.si_generation = 0;
 	atomic_set(&stid->sc_count, 1);
 
 	/*
@@ -592,10 +590,8 @@ alloc_init_deleg(struct nfs4_client *clp, struct nfs4_ol_stateid *stp, struct sv
 	INIT_LIST_HEAD(&dp->dl_perfile);
 	INIT_LIST_HEAD(&dp->dl_perclnt);
 	INIT_LIST_HEAD(&dp->dl_recall_lru);
-	dp->dl_file = NULL;
 	dp->dl_type = NFS4_OPEN_DELEGATE_READ;
 	fh_copy_shallow(&dp->dl_fh, &current_fh->fh_handle);
-	dp->dl_time = 0;
 	INIT_WORK(&dp->dl_recall.cb_work, nfsd4_run_cb_recall);
 	return dp;
 }
@@ -616,6 +612,8 @@ void
 nfs4_put_delegation(struct nfs4_delegation *dp)
 {
 	if (atomic_dec_and_test(&dp->dl_stid.sc_count)) {
+		if (dp->dl_file)
+			put_nfs4_file(dp->dl_file);
 		remove_stid(&dp->dl_stid);
 		nfs4_free_stid(deleg_slab, &dp->dl_stid);
 		num_delegations--;
@@ -665,13 +663,9 @@ unhash_delegation(struct nfs4_delegation *dp)
 	list_del_init(&dp->dl_recall_lru);
 	list_del_init(&dp->dl_perfile);
 	spin_unlock(&fp->fi_lock);
-	if (fp) {
+	if (fp)
 		nfs4_put_deleg_lease(fp);
-		dp->dl_file = NULL;
-	}
 	spin_unlock(&state_lock);
-	if (fp)
-		put_nfs4_file(fp);
 }
 
 static void destroy_revoked_delegation(struct nfs4_delegation *dp)
@@ -879,12 +873,12 @@ static void unhash_generic_stateid(struct nfs4_ol_stateid *stp)
 static void close_generic_stateid(struct nfs4_ol_stateid *stp)
 {
 	release_all_access(stp);
-	put_nfs4_file(stp->st_file);
-	stp->st_file = NULL;
 }
 
 static void free_generic_stateid(struct nfs4_ol_stateid *stp)
 {
+	if (stp->st_file)
+		put_nfs4_file(stp->st_file);
 	remove_stid(&stp->st_stid);
 	nfs4_free_stid(stateid_slab, &stp->st_stid);
 }
@@ -4462,6 +4456,10 @@ static void nfsd4_close_open_stateid(struct nfs4_ol_stateid *s)
 		if (list_empty(&oo->oo_owner.so_stateids))
 			release_openowner(oo);
 	} else {
+		if (s->st_file) {
+			put_nfs4_file(s->st_file);
+			s->st_file = NULL;
+		}
 		oo->oo_last_closed_stid = s;
 		/*
 		 * In the 4.0 case we need to keep the owners around a
-- 
1.9.3

Re: [PATCH v5 03/10] nfsd: simplify stateid allocation and file handling

[J. Bruce Fields]

On Mon, Jul 21, 2014 at 09:34:59AM -0400, Jeff Layton wrote:
> From: Trond Myklebust <trond.myklebust@primarydata.com>
> 
> Don't allow stateids to clear the open file pointer until they are
> being destroyed. In a later patch we'll want to move the putting of
> the nfs4_file into generic stateid handling code and this will help
> facilitate that change.
> 
> Also, move to allocating stateids with kzalloc and get rid of the
> explicit zeroing of fields.

There's a regression here: we end up holding an unnecessary reference to
the inode a long time just because some client isn't responding to a
recall.

When I complained about this before Trond said something about replacing
fi_inode by a filehandle?  But I can't remember if we've seen code to do
that.

--b.

> 
> Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
> Signed-off-by: Jeff Layton <jlayton@primarydata.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/nfsd/nfs4state.c | 22 ++++++++++------------
>  1 file changed, 10 insertions(+), 12 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index a47c97586c76..8ff5c97a2c5a 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -459,7 +459,7 @@ kmem_cache *slab)
>  	struct nfs4_stid *stid;
>  	int new_id;
>  
> -	stid = kmem_cache_alloc(slab, GFP_KERNEL);
> +	stid = kmem_cache_zalloc(slab, GFP_KERNEL);
>  	if (!stid)
>  		return NULL;
>  
> @@ -467,11 +467,9 @@ kmem_cache *slab)
>  	if (new_id < 0)
>  		goto out_free;
>  	stid->sc_client = cl;
> -	stid->sc_type = 0;
>  	stid->sc_stateid.si_opaque.so_id = new_id;
>  	stid->sc_stateid.si_opaque.so_clid = cl->cl_clientid;
>  	/* Will be incremented before return to client: */
> -	stid->sc_stateid.si_generation = 0;
>  	atomic_set(&stid->sc_count, 1);
>  
>  	/*
> @@ -592,10 +590,8 @@ alloc_init_deleg(struct nfs4_client *clp, struct nfs4_ol_stateid *stp, struct sv
>  	INIT_LIST_HEAD(&dp->dl_perfile);
>  	INIT_LIST_HEAD(&dp->dl_perclnt);
>  	INIT_LIST_HEAD(&dp->dl_recall_lru);
> -	dp->dl_file = NULL;
>  	dp->dl_type = NFS4_OPEN_DELEGATE_READ;
>  	fh_copy_shallow(&dp->dl_fh, &current_fh->fh_handle);
> -	dp->dl_time = 0;
>  	INIT_WORK(&dp->dl_recall.cb_work, nfsd4_run_cb_recall);
>  	return dp;
>  }
> @@ -616,6 +612,8 @@ void
>  nfs4_put_delegation(struct nfs4_delegation *dp)
>  {
>  	if (atomic_dec_and_test(&dp->dl_stid.sc_count)) {
> +		if (dp->dl_file)
> +			put_nfs4_file(dp->dl_file);
>  		remove_stid(&dp->dl_stid);
>  		nfs4_free_stid(deleg_slab, &dp->dl_stid);
>  		num_delegations--;
> @@ -665,13 +663,9 @@ unhash_delegation(struct nfs4_delegation *dp)
>  	list_del_init(&dp->dl_recall_lru);
>  	list_del_init(&dp->dl_perfile);
>  	spin_unlock(&fp->fi_lock);
> -	if (fp) {
> +	if (fp)
>  		nfs4_put_deleg_lease(fp);
> -		dp->dl_file = NULL;
> -	}
>  	spin_unlock(&state_lock);
> -	if (fp)
> -		put_nfs4_file(fp);
>  }
>  
>  static void destroy_revoked_delegation(struct nfs4_delegation *dp)
> @@ -879,12 +873,12 @@ static void unhash_generic_stateid(struct nfs4_ol_stateid *stp)
>  static void close_generic_stateid(struct nfs4_ol_stateid *stp)
>  {
>  	release_all_access(stp);
> -	put_nfs4_file(stp->st_file);
> -	stp->st_file = NULL;
>  }
>  
>  static void free_generic_stateid(struct nfs4_ol_stateid *stp)
>  {
> +	if (stp->st_file)
> +		put_nfs4_file(stp->st_file);
>  	remove_stid(&stp->st_stid);
>  	nfs4_free_stid(stateid_slab, &stp->st_stid);
>  }
> @@ -4462,6 +4456,10 @@ static void nfsd4_close_open_stateid(struct nfs4_ol_stateid *s)
>  		if (list_empty(&oo->oo_owner.so_stateids))
>  			release_openowner(oo);
>  	} else {
> +		if (s->st_file) {
> +			put_nfs4_file(s->st_file);
> +			s->st_file = NULL;
> +		}
>  		oo->oo_last_closed_stid = s;
>  		/*
>  		 * In the 4.0 case we need to keep the owners around a
> -- 
> 1.9.3
> 

Re: [PATCH v5 03/10] nfsd: simplify stateid allocation and file handling

[Jeff Layton]

On Mon, 21 Jul 2014 17:30:56 -0400
"J. Bruce Fields" <bfields@fieldses.org> wrote:

> On Mon, Jul 21, 2014 at 09:34:59AM -0400, Jeff Layton wrote:
> > From: Trond Myklebust <trond.myklebust@primarydata.com>
> > 
> > Don't allow stateids to clear the open file pointer until they are
> > being destroyed. In a later patch we'll want to move the putting of
> > the nfs4_file into generic stateid handling code and this will help
> > facilitate that change.
> > 
> > Also, move to allocating stateids with kzalloc and get rid of the
> > explicit zeroing of fields.
> 
> There's a regression here: we end up holding an unnecessary reference to
> the inode a long time just because some client isn't responding to a
> recall.
> 

I must have missed that exchange...

So that's because we're putting the file reference when we put the
last delegation reference instead of when we unhash the
delegation...ok, good point.

I don't see any real harm right offhand of putting the file reference
early when we unhash, so I think that'll work. We do that already for
v4.0 open stateids when they are closed, so we might as well do it for
delegations too. I'll double check that we never use the file after
unhashing and send up a patch to change that unless I see a problem.

> When I complained about this before Trond said something about replacing
> fi_inode by a filehandle?  But I can't remember if we've seen code to do
> that.
> 

No I don't think we have. That sounds like a bit of an overhaul of the
nfs4_file code.

Personally, I think that hashing on the filehandle makes a lot of
sense, and I don't see a reason to pin down the inode except by virtue
of the fi_fds file references. That said, I'd prefer to defer that sort
of change until after this series is merged.

> > 
> > Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
> > Signed-off-by: Jeff Layton <jlayton@primarydata.com>
> > Reviewed-by: Christoph Hellwig <hch@lst.de>
> > ---
> >  fs/nfsd/nfs4state.c | 22 ++++++++++------------
> >  1 file changed, 10 insertions(+), 12 deletions(-)
> > 
> > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > index a47c97586c76..8ff5c97a2c5a 100644
> > --- a/fs/nfsd/nfs4state.c
> > +++ b/fs/nfsd/nfs4state.c
> > @@ -459,7 +459,7 @@ kmem_cache *slab)
> >  	struct nfs4_stid *stid;
> >  	int new_id;
> >  
> > -	stid = kmem_cache_alloc(slab, GFP_KERNEL);
> > +	stid = kmem_cache_zalloc(slab, GFP_KERNEL);
> >  	if (!stid)
> >  		return NULL;
> >  
> > @@ -467,11 +467,9 @@ kmem_cache *slab)
> >  	if (new_id < 0)
> >  		goto out_free;
> >  	stid->sc_client = cl;
> > -	stid->sc_type = 0;
> >  	stid->sc_stateid.si_opaque.so_id = new_id;
> >  	stid->sc_stateid.si_opaque.so_clid = cl->cl_clientid;
> >  	/* Will be incremented before return to client: */
> > -	stid->sc_stateid.si_generation = 0;
> >  	atomic_set(&stid->sc_count, 1);
> >  
> >  	/*
> > @@ -592,10 +590,8 @@ alloc_init_deleg(struct nfs4_client *clp, struct nfs4_ol_stateid *stp, struct sv
> >  	INIT_LIST_HEAD(&dp->dl_perfile);
> >  	INIT_LIST_HEAD(&dp->dl_perclnt);
> >  	INIT_LIST_HEAD(&dp->dl_recall_lru);
> > -	dp->dl_file = NULL;
> >  	dp->dl_type = NFS4_OPEN_DELEGATE_READ;
> >  	fh_copy_shallow(&dp->dl_fh, &current_fh->fh_handle);
> > -	dp->dl_time = 0;
> >  	INIT_WORK(&dp->dl_recall.cb_work, nfsd4_run_cb_recall);
> >  	return dp;
> >  }
> > @@ -616,6 +612,8 @@ void
> >  nfs4_put_delegation(struct nfs4_delegation *dp)
> >  {
> >  	if (atomic_dec_and_test(&dp->dl_stid.sc_count)) {
> > +		if (dp->dl_file)
> > +			put_nfs4_file(dp->dl_file);
> >  		remove_stid(&dp->dl_stid);
> >  		nfs4_free_stid(deleg_slab, &dp->dl_stid);
> >  		num_delegations--;
> > @@ -665,13 +663,9 @@ unhash_delegation(struct nfs4_delegation *dp)
> >  	list_del_init(&dp->dl_recall_lru);
> >  	list_del_init(&dp->dl_perfile);
> >  	spin_unlock(&fp->fi_lock);
> > -	if (fp) {
> > +	if (fp)
> >  		nfs4_put_deleg_lease(fp);
> > -		dp->dl_file = NULL;
> > -	}
> >  	spin_unlock(&state_lock);
> > -	if (fp)
> > -		put_nfs4_file(fp);
> >  }
> >  
> >  static void destroy_revoked_delegation(struct nfs4_delegation *dp)
> > @@ -879,12 +873,12 @@ static void unhash_generic_stateid(struct nfs4_ol_stateid *stp)
> >  static void close_generic_stateid(struct nfs4_ol_stateid *stp)
> >  {
> >  	release_all_access(stp);
> > -	put_nfs4_file(stp->st_file);
> > -	stp->st_file = NULL;
> >  }
> >  
> >  static void free_generic_stateid(struct nfs4_ol_stateid *stp)
> >  {
> > +	if (stp->st_file)
> > +		put_nfs4_file(stp->st_file);
> >  	remove_stid(&stp->st_stid);
> >  	nfs4_free_stid(stateid_slab, &stp->st_stid);
> >  }
> > @@ -4462,6 +4456,10 @@ static void nfsd4_close_open_stateid(struct nfs4_ol_stateid *s)
> >  		if (list_empty(&oo->oo_owner.so_stateids))
> >  			release_openowner(oo);
> >  	} else {
> > +		if (s->st_file) {
> > +			put_nfs4_file(s->st_file);
> > +			s->st_file = NULL;
> > +		}
> >  		oo->oo_last_closed_stid = s;
> >  		/*
> >  		 * In the 4.0 case we need to keep the owners around a
> > -- 
> > 1.9.3
> > 


-- 
Jeff Layton <jlayton@primarydata.com>

Re: [PATCH v5 03/10] nfsd: simplify stateid allocation and file handling

[Trond Myklebust]

On Mon, Jul 21, 2014 at 6:42 PM, Jeff Layton
<jeff.layton@primarydata.com> wrote:
> On Mon, 21 Jul 2014 17:30:56 -0400
> "J. Bruce Fields" <bfields@fieldses.org> wrote:
>
>> On Mon, Jul 21, 2014 at 09:34:59AM -0400, Jeff Layton wrote:
>> > From: Trond Myklebust <trond.myklebust@primarydata.com>
>> >
>> > Don't allow stateids to clear the open file pointer until they are
>> > being destroyed. In a later patch we'll want to move the putting of
>> > the nfs4_file into generic stateid handling code and this will help
>> > facilitate that change.
>> >
>> > Also, move to allocating stateids with kzalloc and get rid of the
>> > explicit zeroing of fields.
>>
>> There's a regression here: we end up holding an unnecessary reference to
>> the inode a long time just because some client isn't responding to a
>> recall.
>>
>
> I must have missed that exchange...
>
> So that's because we're putting the file reference when we put the
> last delegation reference instead of when we unhash the
> delegation...ok, good point.
>
> I don't see any real harm right offhand of putting the file reference
> early when we unhash, so I think that'll work. We do that already for
> v4.0 open stateids when they are closed, so we might as well do it for
> delegations too. I'll double check that we never use the file after
> unhashing and send up a patch to change that unless I see a problem.
>
>> When I complained about this before Trond said something about replacing
>> fi_inode by a filehandle?  But I can't remember if we've seen code to do
>> that.
>>
>
> No I don't think we have. That sounds like a bit of an overhaul of the
> nfs4_file code.

Nah... Let me send out those patches. They're still a little rough
around the edges, but really not that large.

> Personally, I think that hashing on the filehandle makes a lot of
> sense, and I don't see a reason to pin down the inode except by virtue
> of the fi_fds file references. That said, I'd prefer to defer that sort
> of change until after this series is merged.

Sure. I was planning on doing the same. Just a little disorganised right now.

Cheers
  Trond

>> >
>> > Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
>> > Signed-off-by: Jeff Layton <jlayton@primarydata.com>
>> > Reviewed-by: Christoph Hellwig <hch@lst.de>
>> > ---
>> >  fs/nfsd/nfs4state.c | 22 ++++++++++------------
>> >  1 file changed, 10 insertions(+), 12 deletions(-)
>> >
>> > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>> > index a47c97586c76..8ff5c97a2c5a 100644
>> > --- a/fs/nfsd/nfs4state.c
>> > +++ b/fs/nfsd/nfs4state.c
>> > @@ -459,7 +459,7 @@ kmem_cache *slab)
>> >     struct nfs4_stid *stid;
>> >     int new_id;
>> >
>> > -   stid = kmem_cache_alloc(slab, GFP_KERNEL);
>> > +   stid = kmem_cache_zalloc(slab, GFP_KERNEL);
>> >     if (!stid)
>> >             return NULL;
>> >
>> > @@ -467,11 +467,9 @@ kmem_cache *slab)
>> >     if (new_id < 0)
>> >             goto out_free;
>> >     stid->sc_client = cl;
>> > -   stid->sc_type = 0;
>> >     stid->sc_stateid.si_opaque.so_id = new_id;
>> >     stid->sc_stateid.si_opaque.so_clid = cl->cl_clientid;
>> >     /* Will be incremented before return to client: */
>> > -   stid->sc_stateid.si_generation = 0;
>> >     atomic_set(&stid->sc_count, 1);
>> >
>> >     /*
>> > @@ -592,10 +590,8 @@ alloc_init_deleg(struct nfs4_client *clp, struct nfs4_ol_stateid *stp, struct sv
>> >     INIT_LIST_HEAD(&dp->dl_perfile);
>> >     INIT_LIST_HEAD(&dp->dl_perclnt);
>> >     INIT_LIST_HEAD(&dp->dl_recall_lru);
>> > -   dp->dl_file = NULL;
>> >     dp->dl_type = NFS4_OPEN_DELEGATE_READ;
>> >     fh_copy_shallow(&dp->dl_fh, &current_fh->fh_handle);
>> > -   dp->dl_time = 0;
>> >     INIT_WORK(&dp->dl_recall.cb_work, nfsd4_run_cb_recall);
>> >     return dp;
>> >  }
>> > @@ -616,6 +612,8 @@ void
>> >  nfs4_put_delegation(struct nfs4_delegation *dp)
>> >  {
>> >     if (atomic_dec_and_test(&dp->dl_stid.sc_count)) {
>> > +           if (dp->dl_file)
>> > +                   put_nfs4_file(dp->dl_file);
>> >             remove_stid(&dp->dl_stid);
>> >             nfs4_free_stid(deleg_slab, &dp->dl_stid);
>> >             num_delegations--;
>> > @@ -665,13 +663,9 @@ unhash_delegation(struct nfs4_delegation *dp)
>> >     list_del_init(&dp->dl_recall_lru);
>> >     list_del_init(&dp->dl_perfile);
>> >     spin_unlock(&fp->fi_lock);
>> > -   if (fp) {
>> > +   if (fp)
>> >             nfs4_put_deleg_lease(fp);
>> > -           dp->dl_file = NULL;
>> > -   }
>> >     spin_unlock(&state_lock);
>> > -   if (fp)
>> > -           put_nfs4_file(fp);
>> >  }
>> >
>> >  static void destroy_revoked_delegation(struct nfs4_delegation *dp)
>> > @@ -879,12 +873,12 @@ static void unhash_generic_stateid(struct nfs4_ol_stateid *stp)
>> >  static void close_generic_stateid(struct nfs4_ol_stateid *stp)
>> >  {
>> >     release_all_access(stp);
>> > -   put_nfs4_file(stp->st_file);
>> > -   stp->st_file = NULL;
>> >  }
>> >
>> >  static void free_generic_stateid(struct nfs4_ol_stateid *stp)
>> >  {
>> > +   if (stp->st_file)
>> > +           put_nfs4_file(stp->st_file);
>> >     remove_stid(&stp->st_stid);
>> >     nfs4_free_stid(stateid_slab, &stp->st_stid);
>> >  }
>> > @@ -4462,6 +4456,10 @@ static void nfsd4_close_open_stateid(struct nfs4_ol_stateid *s)
>> >             if (list_empty(&oo->oo_owner.so_stateids))
>> >                     release_openowner(oo);
>> >     } else {
>> > +           if (s->st_file) {
>> > +                   put_nfs4_file(s->st_file);
>> > +                   s->st_file = NULL;
>> > +           }
>> >             oo->oo_last_closed_stid = s;
>> >             /*
>> >              * In the 4.0 case we need to keep the owners around a
>> > --
>> > 1.9.3
>> >
>
>
> --
> Jeff Layton <jlayton@primarydata.com>



-- 
Trond Myklebust

Linux NFS client maintainer, PrimaryData

trond.myklebust@primarydata.com

[PATCH v5 04/10] nfsd: Fix delegation revocation

[Jeff Layton]

Ensure that the delegations cannot be found by the laundromat etc once
we add them to the various 'revoke' lists.

Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
Signed-off-by: Jeff Layton <jlayton@primarydata.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 fs/nfsd/nfs4state.c | 37 +++++++++++++++++++++----------------
 1 file changed, 21 insertions(+), 16 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 8ff5c97a2c5a..d8c28d5c600f 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -650,13 +650,13 @@ hash_delegation_locked(struct nfs4_delegation *dp, struct nfs4_file *fp)
 	list_add(&dp->dl_perclnt, &dp->dl_stid.sc_client->cl_delegations);
 }
 
-/* Called under the state lock. */
 static void
-unhash_delegation(struct nfs4_delegation *dp)
+unhash_delegation_locked(struct nfs4_delegation *dp)
 {
 	struct nfs4_file *fp = dp->dl_file;
 
-	spin_lock(&state_lock);
+	lockdep_assert_held(&state_lock);
+
 	dp->dl_stid.sc_type = NFS4_CLOSED_DELEG_STID;
 	spin_lock(&fp->fi_lock);
 	list_del_init(&dp->dl_perclnt);
@@ -665,7 +665,6 @@ unhash_delegation(struct nfs4_delegation *dp)
 	spin_unlock(&fp->fi_lock);
 	if (fp)
 		nfs4_put_deleg_lease(fp);
-	spin_unlock(&state_lock);
 }
 
 static void destroy_revoked_delegation(struct nfs4_delegation *dp)
@@ -676,7 +675,9 @@ static void destroy_revoked_delegation(struct nfs4_delegation *dp)
 
 static void destroy_delegation(struct nfs4_delegation *dp)
 {
-	unhash_delegation(dp);
+	spin_lock(&state_lock);
+	unhash_delegation_locked(dp);
+	spin_unlock(&state_lock);
 	nfs4_put_delegation(dp);
 }
 
@@ -685,11 +686,10 @@ static void revoke_delegation(struct nfs4_delegation *dp)
 	struct nfs4_client *clp = dp->dl_stid.sc_client;
 
 	if (clp->cl_minorversion == 0)
-		destroy_delegation(dp);
+		destroy_revoked_delegation(dp);
 	else {
-		unhash_delegation(dp);
 		dp->dl_stid.sc_type = NFS4_REVOKED_DELEG_STID;
-		list_add(&dp->dl_recall_lru, &clp->cl_revoked);
+		list_move(&dp->dl_recall_lru, &clp->cl_revoked);
 	}
 }
 
@@ -1447,15 +1447,16 @@ destroy_client(struct nfs4_client *clp)
 	spin_lock(&state_lock);
 	while (!list_empty(&clp->cl_delegations)) {
 		dp = list_entry(clp->cl_delegations.next, struct nfs4_delegation, dl_perclnt);
-		list_del_init(&dp->dl_perclnt);
+		unhash_delegation_locked(dp);
 		/* Ensure that deleg break won't try to requeue it */
 		++dp->dl_time;
-		list_move(&dp->dl_recall_lru, &reaplist);
+		list_add(&dp->dl_recall_lru, &reaplist);
 	}
 	spin_unlock(&state_lock);
 	while (!list_empty(&reaplist)) {
 		dp = list_entry(reaplist.next, struct nfs4_delegation, dl_recall_lru);
-		destroy_delegation(dp);
+		list_del_init(&dp->dl_recall_lru);
+		nfs4_put_delegation(dp);
 	}
 	list_splice_init(&clp->cl_revoked, &reaplist);
 	while (!list_empty(&reaplist)) {
@@ -3654,7 +3655,7 @@ nfs4_open_delegation(struct net *net, struct svc_fh *fh,
 	open->op_delegate_type = NFS4_OPEN_DELEGATE_READ;
 	return;
 out_free:
-	destroy_delegation(dp);
+	nfs4_put_delegation(dp);
 out_no_deleg:
 	open->op_delegate_type = NFS4_OPEN_DELEGATE_NONE;
 	if (open->op_claim_type == NFS4_OPEN_CLAIM_PREVIOUS &&
@@ -3893,7 +3894,8 @@ nfs4_laundromat(struct nfsd_net *nn)
 			new_timeo = min(new_timeo, t);
 			break;
 		}
-		list_move(&dp->dl_recall_lru, &reaplist);
+		unhash_delegation_locked(dp);
+		list_add(&dp->dl_recall_lru, &reaplist);
 	}
 	spin_unlock(&state_lock);
 	list_for_each_safe(pos, next, &reaplist) {
@@ -5368,7 +5370,8 @@ static u64 nfsd_find_all_delegations(struct nfs4_client *clp, u64 max,
 			 * don't monkey with it now that we are.
 			 */
 			++dp->dl_time;
-			list_move(&dp->dl_recall_lru, victims);
+			unhash_delegation_locked(dp);
+			list_add(&dp->dl_recall_lru, victims);
 		}
 		if (++count == max)
 			break;
@@ -5623,12 +5626,14 @@ nfs4_state_shutdown_net(struct net *net)
 	spin_lock(&state_lock);
 	list_for_each_safe(pos, next, &nn->del_recall_lru) {
 		dp = list_entry (pos, struct nfs4_delegation, dl_recall_lru);
-		list_move(&dp->dl_recall_lru, &reaplist);
+		unhash_delegation_locked(dp);
+		list_add(&dp->dl_recall_lru, &reaplist);
 	}
 	spin_unlock(&state_lock);
 	list_for_each_safe(pos, next, &reaplist) {
 		dp = list_entry (pos, struct nfs4_delegation, dl_recall_lru);
-		destroy_delegation(dp);
+		list_del_init(&dp->dl_recall_lru);
+		nfs4_put_delegation(dp);
 	}
 
 	nfsd4_client_tracking_exit(net);
-- 
1.9.3

[PATCH v5 05/10] nfsd: ensure that clp->cl_revoked list is protected by clp->cl_lock

[Jeff Layton]

Currently, both destroy_revoked_delegation and revoke_delegation
manipulate the cl_revoked list without any locking aside from the
client_mutex. Ensure that the clp->cl_lock is held when manipulating it,
except for the list walking in destroy_client. At that point, the client
should no longer be in use, and so it should be safe to walk the list
without any locking. That also means that we don't need to do the
list_splice_init there either.

Also, the fact that destroy_revoked_delegation and revoke_delegation
delete dl_recall_lru without any locking makes it difficult to know
whether they're doing so safely in all cases. Move the list_del_init
calls into the callers, and add WARN_ONs in the event that these calls
are passed a delegation that has a non-empty list_head.

Signed-off-by: Jeff Layton <jlayton@primarydata.com>
---
 fs/nfsd/nfs4state.c | 31 +++++++++++++++++--------------
 1 file changed, 17 insertions(+), 14 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index d8c28d5c600f..58746f0f34a1 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -667,12 +667,6 @@ unhash_delegation_locked(struct nfs4_delegation *dp)
 		nfs4_put_deleg_lease(fp);
 }
 
-static void destroy_revoked_delegation(struct nfs4_delegation *dp)
-{
-	list_del_init(&dp->dl_recall_lru);
-	nfs4_put_delegation(dp);
-}
-
 static void destroy_delegation(struct nfs4_delegation *dp)
 {
 	spin_lock(&state_lock);
@@ -685,11 +679,15 @@ static void revoke_delegation(struct nfs4_delegation *dp)
 {
 	struct nfs4_client *clp = dp->dl_stid.sc_client;
 
+	WARN_ON(!list_empty(&dp->dl_recall_lru));
+
 	if (clp->cl_minorversion == 0)
-		destroy_revoked_delegation(dp);
+		nfs4_put_delegation(dp);
 	else {
 		dp->dl_stid.sc_type = NFS4_REVOKED_DELEG_STID;
-		list_move(&dp->dl_recall_lru, &clp->cl_revoked);
+		spin_lock(&clp->cl_lock);
+		list_add(&dp->dl_recall_lru, &clp->cl_revoked);
+		spin_unlock(&clp->cl_lock);
 	}
 }
 
@@ -1458,10 +1456,10 @@ destroy_client(struct nfs4_client *clp)
 		list_del_init(&dp->dl_recall_lru);
 		nfs4_put_delegation(dp);
 	}
-	list_splice_init(&clp->cl_revoked, &reaplist);
-	while (!list_empty(&reaplist)) {
+	while (!list_empty(&clp->cl_revoked)) {
 		dp = list_entry(reaplist.next, struct nfs4_delegation, dl_recall_lru);
-		destroy_revoked_delegation(dp);
+		list_del_init(&dp->dl_recall_lru);
+		nfs4_put_delegation(dp);
 	}
 	while (!list_empty(&clp->cl_openowners)) {
 		oo = list_entry(clp->cl_openowners.next, struct nfs4_openowner, oo_perclient);
@@ -3898,8 +3896,10 @@ nfs4_laundromat(struct nfsd_net *nn)
 		list_add(&dp->dl_recall_lru, &reaplist);
 	}
 	spin_unlock(&state_lock);
-	list_for_each_safe(pos, next, &reaplist) {
-		dp = list_entry (pos, struct nfs4_delegation, dl_recall_lru);
+	while (!list_empty(&reaplist)) {
+		dp = list_first_entry(&reaplist, struct nfs4_delegation,
+					dl_recall_lru);
+		list_del_init(&dp->dl_recall_lru);
 		revoke_delegation(dp);
 	}
 	list_for_each_safe(pos, next, &nn->close_lru) {
@@ -4243,7 +4243,10 @@ nfsd4_free_stateid(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 		break;
 	case NFS4_REVOKED_DELEG_STID:
 		dp = delegstateid(s);
-		destroy_revoked_delegation(dp);
+		spin_lock(&cl->cl_lock);
+		list_del_init(&dp->dl_recall_lru);
+		spin_unlock(&cl->cl_lock);
+		nfs4_put_delegation(dp);
 		ret = nfs_ok;
 		break;
 	default:
-- 
1.9.3

Re: [PATCH v5 05/10] nfsd: ensure that clp->cl_revoked list is protected by clp->cl_lock

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH v5 06/10] nfsd: Convert delegation counter to an atomic_long_t type

[Jeff Layton]

From: Trond Myklebust <trond.myklebust@primarydata.com>

We want to convert to an atomic type so that we don't need to lock
across the call to alloc_init_deleg(). Then convert to a long type so
that we match the size of 'max_delegations'.

None of this is a problem today, but it will be once we remove
client_mutex protection.

Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 fs/nfsd/nfs4state.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 58746f0f34a1..390515889c91 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -343,7 +343,7 @@ find_any_file(struct nfs4_file *f)
 	return ret;
 }
 
-static int num_delegations;
+static atomic_long_t num_delegations;
 unsigned long max_delegations;
 
 /*
@@ -571,22 +571,23 @@ static struct nfs4_delegation *
 alloc_init_deleg(struct nfs4_client *clp, struct nfs4_ol_stateid *stp, struct svc_fh *current_fh)
 {
 	struct nfs4_delegation *dp;
+	long n;
 
 	dprintk("NFSD alloc_init_deleg\n");
-	if (num_delegations > max_delegations)
-		return NULL;
+	n = atomic_long_inc_return(&num_delegations);
+	if (n < 0 || n > max_delegations)
+		goto out_dec;
 	if (delegation_blocked(&current_fh->fh_handle))
-		return NULL;
+		goto out_dec;
 	dp = delegstateid(nfs4_alloc_stid(clp, deleg_slab));
 	if (dp == NULL)
-		return dp;
+		goto out_dec;
 	/*
 	 * delegation seqid's are never incremented.  The 4.1 special
 	 * meaning of seqid 0 isn't meaningful, really, but let's avoid
 	 * 0 anyway just for consistency and use 1:
 	 */
 	dp->dl_stid.sc_stateid.si_generation = 1;
-	num_delegations++;
 	INIT_LIST_HEAD(&dp->dl_perfile);
 	INIT_LIST_HEAD(&dp->dl_perclnt);
 	INIT_LIST_HEAD(&dp->dl_recall_lru);
@@ -594,6 +595,9 @@ alloc_init_deleg(struct nfs4_client *clp, struct nfs4_ol_stateid *stp, struct sv
 	fh_copy_shallow(&dp->dl_fh, &current_fh->fh_handle);
 	INIT_WORK(&dp->dl_recall.cb_work, nfsd4_run_cb_recall);
 	return dp;
+out_dec:
+	atomic_long_dec(&num_delegations);
+	return NULL;
 }
 
 static void remove_stid(struct nfs4_stid *s)
@@ -616,7 +620,7 @@ nfs4_put_delegation(struct nfs4_delegation *dp)
 			put_nfs4_file(dp->dl_file);
 		remove_stid(&dp->dl_stid);
 		nfs4_free_stid(deleg_slab, &dp->dl_stid);
-		num_delegations--;
+		atomic_long_dec(&num_delegations);
 	}
 }
 
-- 
1.9.3

[PATCH v5 07/10] nfsd: drop unused stp arg to alloc_init_deleg

[Jeff Layton]

Signed-off-by: Jeff Layton <jlayton@primarydata.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 fs/nfsd/nfs4state.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 390515889c91..acddedb7250e 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -568,7 +568,7 @@ static void block_delegations(struct knfsd_fh *fh)
 }
 
 static struct nfs4_delegation *
-alloc_init_deleg(struct nfs4_client *clp, struct nfs4_ol_stateid *stp, struct svc_fh *current_fh)
+alloc_init_deleg(struct nfs4_client *clp, struct svc_fh *current_fh)
 {
 	struct nfs4_delegation *dp;
 	long n;
@@ -3643,7 +3643,7 @@ nfs4_open_delegation(struct net *net, struct svc_fh *fh,
 		default:
 			goto out_no_deleg;
 	}
-	dp = alloc_init_deleg(oo->oo_owner.so_client, stp, fh);
+	dp = alloc_init_deleg(oo->oo_owner.so_client, fh);
 	if (dp == NULL)
 		goto out_no_deleg;
 	status = nfs4_set_delegation(dp, stp->st_file);
-- 
1.9.3

[PATCH v5 08/10] nfsd: clean up arguments to nfs4_open_delegation

[Jeff Layton]

No need to pass in a net pointer since we can derive that.

Signed-off-by: Jeff Layton <jlayton@primarydata.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 fs/nfsd/nfs4state.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index acddedb7250e..171ab5a2c60c 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -3601,11 +3601,12 @@ static void nfsd4_open_deleg_none_ext(struct nfsd4_open *open, int status)
  * proper support for them.
  */
 static void
-nfs4_open_delegation(struct net *net, struct svc_fh *fh,
-		     struct nfsd4_open *open, struct nfs4_ol_stateid *stp)
+nfs4_open_delegation(struct svc_fh *fh, struct nfsd4_open *open,
+			struct nfs4_ol_stateid *stp)
 {
 	struct nfs4_delegation *dp;
-	struct nfs4_openowner *oo = container_of(stp->st_stateowner, struct nfs4_openowner, oo_owner);
+	struct nfs4_openowner *oo = openowner(stp->st_stateowner);
+	struct nfs4_client *clp = stp->st_stid.sc_client;
 	int cb_up;
 	int status = 0;
 
@@ -3624,7 +3625,7 @@ nfs4_open_delegation(struct net *net, struct svc_fh *fh,
 			 * Let's not give out any delegations till everyone's
 			 * had the chance to reclaim theirs....
 			 */
-			if (locks_in_grace(net))
+			if (locks_in_grace(clp->net))
 				goto out_no_deleg;
 			if (!cb_up || !(oo->oo_flags & NFS4_OO_CONFIRMED))
 				goto out_no_deleg;
@@ -3643,7 +3644,7 @@ nfs4_open_delegation(struct net *net, struct svc_fh *fh,
 		default:
 			goto out_no_deleg;
 	}
-	dp = alloc_init_deleg(oo->oo_owner.so_client, fh);
+	dp = alloc_init_deleg(clp, fh);
 	if (dp == NULL)
 		goto out_no_deleg;
 	status = nfs4_set_delegation(dp, stp->st_file);
@@ -3757,7 +3758,7 @@ nfsd4_process_open2(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nf
 	* Attempt to hand out a delegation. No error return, because the
 	* OPEN succeeds even if we fail.
 	*/
-	nfs4_open_delegation(SVC_NET(rqstp), current_fh, open, stp);
+	nfs4_open_delegation(current_fh, open, stp);
 nodeleg:
 	status = nfs_ok;
 
-- 
1.9.3

[PATCH v5 09/10] nfsd: clean up nfs4_set_delegation

[Jeff Layton]

Move the alloc_init_deleg call into nfs4_set_delegation and change the
function to return a pointer to the delegation or an IS_ERR return. This
allows us to skip allocating a delegation if the file has already
experienced a lease conflict.

Signed-off-by: Jeff Layton <jlayton@primarydata.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 fs/nfsd/nfs4state.c | 34 ++++++++++++++++++++++------------
 1 file changed, 22 insertions(+), 12 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 171ab5a2c60c..18d1b60f506f 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -3546,12 +3546,20 @@ out_fput:
 	return status;
 }
 
-static int nfs4_set_delegation(struct nfs4_delegation *dp, struct nfs4_file *fp)
+static struct nfs4_delegation *
+nfs4_set_delegation(struct nfs4_client *clp, struct svc_fh *fh,
+		    struct nfs4_file *fp)
 {
-	int status = 0;
+	int status;
+	struct nfs4_delegation *dp;
 
 	if (fp->fi_had_conflict)
-		return -EAGAIN;
+		return ERR_PTR(-EAGAIN);
+
+	dp = alloc_init_deleg(clp, fh);
+	if (!dp)
+		return ERR_PTR(-ENOMEM);
+
 	get_nfs4_file(fp);
 	spin_lock(&state_lock);
 	spin_lock(&fp->fi_lock);
@@ -3559,7 +3567,8 @@ static int nfs4_set_delegation(struct nfs4_delegation *dp, struct nfs4_file *fp)
 	if (!fp->fi_lease) {
 		spin_unlock(&fp->fi_lock);
 		spin_unlock(&state_lock);
-		return nfs4_setlease(dp);
+		status = nfs4_setlease(dp);
+		goto out;
 	}
 	atomic_inc(&fp->fi_delegees);
 	if (fp->fi_had_conflict) {
@@ -3567,10 +3576,16 @@ static int nfs4_set_delegation(struct nfs4_delegation *dp, struct nfs4_file *fp)
 		goto out_unlock;
 	}
 	hash_delegation_locked(dp, fp);
+	status = 0;
 out_unlock:
 	spin_unlock(&fp->fi_lock);
 	spin_unlock(&state_lock);
-	return status;
+out:
+	if (status) {
+		nfs4_put_delegation(dp);
+		return ERR_PTR(status);
+	}
+	return dp;
 }
 
 static void nfsd4_open_deleg_none_ext(struct nfsd4_open *open, int status)
@@ -3644,12 +3659,9 @@ nfs4_open_delegation(struct svc_fh *fh, struct nfsd4_open *open,
 		default:
 			goto out_no_deleg;
 	}
-	dp = alloc_init_deleg(clp, fh);
-	if (dp == NULL)
+	dp = nfs4_set_delegation(clp, fh, stp->st_file);
+	if (IS_ERR(dp))
 		goto out_no_deleg;
-	status = nfs4_set_delegation(dp, stp->st_file);
-	if (status)
-		goto out_free;
 
 	memcpy(&open->op_delegate_stateid, &dp->dl_stid.sc_stateid, sizeof(dp->dl_stid.sc_stateid));
 
@@ -3657,8 +3669,6 @@ nfs4_open_delegation(struct svc_fh *fh, struct nfsd4_open *open,
 		STATEID_VAL(&dp->dl_stid.sc_stateid));
 	open->op_delegate_type = NFS4_OPEN_DELEGATE_READ;
 	return;
-out_free:
-	nfs4_put_delegation(dp);
 out_no_deleg:
 	open->op_delegate_type = NFS4_OPEN_DELEGATE_NONE;
 	if (open->op_claim_type == NFS4_OPEN_CLAIM_PREVIOUS &&
-- 
1.9.3

[PATCH v5 10/10] nfsd: give block_delegation and delegation_blocked its own spinlock

[Jeff Layton]

The state lock can be fairly heavily contended, and there's no reason
that nfs4_file lookups and delegation_blocked should be mutually
exclusive.  Let's give the new block_delegation code its own spinlock.
It does mean that we'll need to take a different lock in the delegation
break code, but that's not generally as critical to performance.

Cc: Neil Brown <neilb@suse.de>
Signed-off-by: Jeff Layton <jlayton@primarydata.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 fs/nfsd/nfs4state.c | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 18d1b60f506f..3946a5a9459c 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -506,10 +506,11 @@ static struct nfs4_ol_stateid * nfs4_alloc_stateid(struct nfs4_client *clp)
  * Each filter is 256 bits.  We hash the filehandle to 32bit and use the
  * low 3 bytes as hash-table indices.
  *
- * 'state_lock', which is always held when block_delegations() is called,
+ * 'blocked_delegations_lock', which is always taken in block_delegations(),
  * is used to manage concurrent access.  Testing does not need the lock
  * except when swapping the two filters.
  */
+static DEFINE_SPINLOCK(blocked_delegations_lock);
 static struct bloom_pair {
 	int	entries, old_entries;
 	time_t	swap_time;
@@ -525,7 +526,7 @@ static int delegation_blocked(struct knfsd_fh *fh)
 	if (bd->entries == 0)
 		return 0;
 	if (seconds_since_boot() - bd->swap_time > 30) {
-		spin_lock(&state_lock);
+		spin_lock(&blocked_delegations_lock);
 		if (seconds_since_boot() - bd->swap_time > 30) {
 			bd->entries -= bd->old_entries;
 			bd->old_entries = bd->entries;
@@ -534,7 +535,7 @@ static int delegation_blocked(struct knfsd_fh *fh)
 			bd->new = 1-bd->new;
 			bd->swap_time = seconds_since_boot();
 		}
-		spin_unlock(&state_lock);
+		spin_unlock(&blocked_delegations_lock);
 	}
 	hash = arch_fast_hash(&fh->fh_base, fh->fh_size, 0);
 	if (test_bit(hash&255, bd->set[0]) &&
@@ -555,16 +556,16 @@ static void block_delegations(struct knfsd_fh *fh)
 	u32 hash;
 	struct bloom_pair *bd = &blocked_delegations;
 
-	lockdep_assert_held(&state_lock);
-
 	hash = arch_fast_hash(&fh->fh_base, fh->fh_size, 0);
 
+	spin_lock(&blocked_delegations_lock);
 	__set_bit(hash&255, bd->set[bd->new]);
 	__set_bit((hash>>8)&255, bd->set[bd->new]);
 	__set_bit((hash>>16)&255, bd->set[bd->new]);
 	if (bd->entries == 0)
 		bd->swap_time = seconds_since_boot();
 	bd->entries += 1;
+	spin_unlock(&blocked_delegations_lock);
 }
 
 static struct nfs4_delegation *
@@ -3091,16 +3092,16 @@ void nfsd4_prepare_cb_recall(struct nfs4_delegation *dp)
 	struct nfs4_client *clp = dp->dl_stid.sc_client;
 	struct nfsd_net *nn = net_generic(clp->net, nfsd_net_id);
 
-	/*
-	 * We can't do this in nfsd_break_deleg_cb because it is
-	 * already holding inode->i_lock
-	 */
-	spin_lock(&state_lock);
 	block_delegations(&dp->dl_fh);
+
 	/*
+	 * We can't do this in nfsd_break_deleg_cb because it is
+	 * already holding inode->i_lock.
+	 *
 	 * If the dl_time != 0, then we know that it has already been
 	 * queued for a lease break. Don't queue it again.
 	 */
+	spin_lock(&state_lock);
 	if (dp->dl_time == 0) {
 		dp->dl_time = get_seconds();
 		list_add_tail(&dp->dl_recall_lru, &nn->del_recall_lru);
-- 
1.9.3

Re: [PATCH v5 00/10] nfsd: more delegation fixes to prepare for client_mutex removal

[J. Bruce Fields]

On Mon, Jul 21, 2014 at 09:34:56AM -0400, Jeff Layton wrote:
> v5:
> - fix spinlocking in block_delegations. Lock should be held around all
>   of the set_bit calls so we don't race with the swap of the two
>   fields.
> 
> - eliminate destroy_revoked_delegation (just use nfs4_put_delegation)
> 
> - eliminate unneeded NULL pointer check in nfs4_setlease
> 
> v4:
> - close more potential races in setlease code, and fix some bugs in
>   error handling in that code.
> 
> - clean up delegation setting functions, eliminating unused arguments
>   and avoiding allocations when there has already been a delegation
>   break
> 
> - add separate spinlock for block_delegations/delegation_blocked code
> 
> v3:
> - use alternate method for checking for delegation break races after
>   getting a lease (just check fi_had_conflict instead)
> 
> - drop file_has_lease patch -- no longer needed
> 
> - move cl_revoked handling patch into this set. It means altering a
>   few of the later patches, but it keeps the set more topically
>   coherent
> 
> v2:
> - move remove_stid call from nfs4_free_stid and into callers
> 
> Ok, I'm hoping we're now on the final respin here. This just cleans
> up some nits that Christoph noticed in the last set. It also fixes
> a potential bug that Neil noticed around the block_delegations
> spinlocking.
> 
> Bruce, I noticed that you had merged the last set into your nfsd-next
> branch, so let me know if you'd prefer me to do incremental changes
> on top of that instead.

As long as it's just in nfsd-next I'll take replacements, if it's in
master (== for-3.17) then I'm considering it really and truly committed
and would rather take incremental patches.  (Though I haven't been
really great about sticking to that.)

--b.

> 
> Jeff Layton (7):
>   nfsd: Protect the nfs4_file delegation fields using the fi_lock
>   nfsd: Fix delegation revocation
>   nfsd: ensure that clp->cl_revoked list is protected by clp->cl_lock
>   nfsd: drop unused stp arg to alloc_init_deleg
>   nfsd: clean up arguments to nfs4_open_delegation
>   nfsd: clean up nfs4_set_delegation
>   nfsd: give block_delegation and delegation_blocked its own spinlock
> 
> Trond Myklebust (3):
>   nfsd: Move the delegation reference counter into the struct nfs4_stid
>   nfsd: simplify stateid allocation and file handling
>   nfsd: Convert delegation counter to an atomic_long_t type
> 
>  fs/nfsd/nfs4state.c | 253 ++++++++++++++++++++++++++++++++--------------------
>  fs/nfsd/state.h     |   2 +-
>  2 files changed, 159 insertions(+), 96 deletions(-)
> 
> -- 
> 1.9.3
> 

Re: [PATCH v5 00/10] nfsd: more delegation fixes to prepare for client_mutex removal

[J. Bruce Fields]

On Mon, Jul 21, 2014 at 12:01:20PM -0400, J. Bruce Fields wrote:
> On Mon, Jul 21, 2014 at 09:34:56AM -0400, Jeff Layton wrote:
> > v5:
> > - fix spinlocking in block_delegations. Lock should be held around all
> >   of the set_bit calls so we don't race with the swap of the two
> >   fields.
> > 
> > - eliminate destroy_revoked_delegation (just use nfs4_put_delegation)
> > 
> > - eliminate unneeded NULL pointer check in nfs4_setlease
> > 
> > v4:
> > - close more potential races in setlease code, and fix some bugs in
> >   error handling in that code.
> > 
> > - clean up delegation setting functions, eliminating unused arguments
> >   and avoiding allocations when there has already been a delegation
> >   break
> > 
> > - add separate spinlock for block_delegations/delegation_blocked code
> > 
> > v3:
> > - use alternate method for checking for delegation break races after
> >   getting a lease (just check fi_had_conflict instead)
> > 
> > - drop file_has_lease patch -- no longer needed
> > 
> > - move cl_revoked handling patch into this set. It means altering a
> >   few of the later patches, but it keeps the set more topically
> >   coherent
> > 
> > v2:
> > - move remove_stid call from nfs4_free_stid and into callers
> > 
> > Ok, I'm hoping we're now on the final respin here. This just cleans
> > up some nits that Christoph noticed in the last set. It also fixes
> > a potential bug that Neil noticed around the block_delegations
> > spinlocking.
> > 
> > Bruce, I noticed that you had merged the last set into your nfsd-next
> > branch, so let me know if you'd prefer me to do incremental changes
> > on top of that instead.
> 
> As long as it's just in nfsd-next I'll take replacements, if it's in
> master (== for-3.17) then I'm considering it really and truly committed
> and would rather take incremental patches.  (Though I haven't been
> really great about sticking to that.)

I'm reliably getting a freeze running connectathon tests over NFSv4
after applying this set (specifically I saw it on ae4b884fc631, the
current tip of my nfsd-next).  I'll try to get some more information.

--b.

[PATCH v5 11/10] nfsd: set fl->fl_file properly in nfs4_setlease

[Jeff Layton]

We must set the fl->fl_file properly for the lease, or there will be
trouble when the lease gets removed.

Longer term, it would probably make sense to set fl_file during the
setlease call, since we pass it in, but this should work around the
immediate oops that I've been seeing.

Signed-off-by: Jeff Layton <jlayton@primarydata.com>
---
 fs/nfsd/nfs4state.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 3946a5a9459c..b21319816b89 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -3514,6 +3514,7 @@ static int nfs4_setlease(struct nfs4_delegation *dp)
 		WARN_ON_ONCE(1);
 		return -EBADF;
 	}
+	fl->fl_file = filp;
 	status = vfs_setlease(filp, fl->fl_type, &fl);
 	if (status) {
 		locks_free_lock(fl);
-- 
1.9.3

Re: [PATCH v5 11/10] nfsd: set fl->fl_file properly in nfs4_setlease

[Jeff Layton]

On Mon, 21 Jul 2014 15:46:13 -0400
Jeff Layton <jlayton@primarydata.com> wrote:

> We must set the fl->fl_file properly for the lease, or there will be
> trouble when the lease gets removed.
> 
> Longer term, it would probably make sense to set fl_file during the
> setlease call, since we pass it in, but this should work around the
> immediate oops that I've been seeing.
> 
> Signed-off-by: Jeff Layton <jlayton@primarydata.com>
> ---
>  fs/nfsd/nfs4state.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 3946a5a9459c..b21319816b89 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -3514,6 +3514,7 @@ static int nfs4_setlease(struct nfs4_delegation *dp)
>  		WARN_ON_ONCE(1);
>  		return -EBADF;
>  	}
> +	fl->fl_file = filp;
>  	status = vfs_setlease(filp, fl->fl_type, &fl);
>  	if (status) {
>  		locks_free_lock(fl);

Obviously, this should ideally be squashed into patch #1 in the series
I sent this morning to prevent regressions. Bruce, can you fix that up
or do I need to resend?

Thanks,
-- 
Jeff Layton <jlayton@primarydata.com>

Re: [PATCH v5 11/10] nfsd: set fl->fl_file properly in nfs4_setlease

[J. Bruce Fields]

On Mon, Jul 21, 2014 at 03:54:42PM -0400, Jeff Layton wrote:
> On Mon, 21 Jul 2014 15:46:13 -0400
> Jeff Layton <jlayton@primarydata.com> wrote:
> 
> > We must set the fl->fl_file properly for the lease, or there will be
> > trouble when the lease gets removed.
> > 
> > Longer term, it would probably make sense to set fl_file during the
> > setlease call, since we pass it in, but this should work around the
> > immediate oops that I've been seeing.
> > 
> > Signed-off-by: Jeff Layton <jlayton@primarydata.com>
> > ---
> >  fs/nfsd/nfs4state.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > index 3946a5a9459c..b21319816b89 100644
> > --- a/fs/nfsd/nfs4state.c
> > +++ b/fs/nfsd/nfs4state.c
> > @@ -3514,6 +3514,7 @@ static int nfs4_setlease(struct nfs4_delegation *dp)
> >  		WARN_ON_ONCE(1);
> >  		return -EBADF;
> >  	}
> > +	fl->fl_file = filp;
> >  	status = vfs_setlease(filp, fl->fl_type, &fl);
> >  	if (status) {
> >  		locks_free_lock(fl);
> 
> Obviously, this should ideally be squashed into patch #1 in the series
> I sent this morning to prevent regressions. Bruce, can you fix that up
> or do I need to resend?

I'll take care of it, thanks!

--b.