Skip to contentSkip to conversation

nfsd: more delegation fixes to prepare for client_mutex removal

PATCH Superseded Jeff Layton 5 participants 21 messages Jul 21, 2014

Subsystems (2)
NFS client NFS server

v5 of 6 Superseded Latest is v6 →

lore
Share & tools
lore

Jeff Layton

author
#0 lore raw

v5 cover nfsd: more delegation fixes to prepare for client_mutex removal

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(-)
Signature · 1 line
1.9.3
author
#1 lore raw

PATCH v5 1/10 nfsd: Protect the nfs4_file delegation fields using the fi_lock

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 <[email protected]>
Signed-off-by Jeff Layton <[email protected]>
---
 fs/nfsd/nfs4state.c | 89 ++++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 65 insertions(+), 24 deletions(-)
fs/nfsd/nfs4state.c +65 -24
index fd4deb049ddf..b05ac229fff0 100644
@@ -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)
Signature · 1 line
1.9.3
author
#3 lore raw

PATCH v5 2/10 nfsd: Move the delegation reference counter into the struct nfs4_stid

In reply to Jeff Layton

From: Trond Myklebust <[email protected]>

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

Signed-off-by Trond Myklebust <[email protected]>
Reviewed-by Christoph Hellwig <[email protected]>
---
 fs/nfsd/nfs4state.c | 6 +++---
 fs/nfsd/state.h     | 2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)
fs/nfsd/nfs4state.c +3 -3
index b05ac229fff0..a47c97586c76 100644
@@ -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);
 }
fs/nfsd/state.h +1 -1
index 996d61eeb357..e68a9ae30fd7 100644
@@ -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;
Signature · 1 line
1.9.3
author
#4 lore raw

PATCH v5 3/10 nfsd: simplify stateid allocation and file handling

In reply to Jeff Layton

From: Trond Myklebust <[email protected]>

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 <[email protected]>
Signed-off-by Jeff Layton <[email protected]>
Reviewed-by Christoph Hellwig <[email protected]>
---
 fs/nfsd/nfs4state.c | 22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)
fs/nfsd/nfs4state.c +10 -12
index a47c97586c76..8ff5c97a2c5a 100644
@@ -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
Signature · 1 line
1.9.3
#5 lore raw

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

Quoted 9 lines · Jeff Layton

From: Trond Myklebust <[email protected]>

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.

Quoted 97 lines
Signed-off-by Trond Myklebust <[email protected]>
Signed-off-by Jeff Layton <[email protected]>
Reviewed-by Christoph Hellwig <[email protected]>
---
 fs/nfsd/nfs4state.c | 22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)
fs/nfsd/nfs4state.c +10 -12
index a47c97586c76..8ff5c97a2c5a 100644
@@ -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
Signature · 1 line
1.9.3
#6 lore raw

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

Quoted 8 lines · On Mon, 21 Jul 2014 17:30:56 -0400 "J. Bruce Fields" <[email protected]>…
Quoted 7 lines · Jeff Layton

From: Trond Myklebust <[email protected]>

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.

Quoted 7 lines
Quoted 2 lines

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.

Quoted 4 lines

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.

Quoted 97 lines
Quoted 97 lines
Signed-off-by Trond Myklebust <[email protected]>
Signed-off-by Jeff Layton <[email protected]>
Reviewed-by Christoph Hellwig <[email protected]>
---
 fs/nfsd/nfs4state.c | 22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)
fs/nfsd/nfs4state.c +10 -12
index a47c97586c76..8ff5c97a2c5a 100644
@@ -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
Signature · 1 line
1.9.3
Signature · 1 line
Jeff Layton <[email protected]>
#7 lore raw

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

Quoted 31 lines · Jeff Layton
Quoted 8 lines · On Mon, 21 Jul 2014 17:30:56 -0400 "J. Bruce Fields" <[email protected]>…
Quoted 7 lines · Jeff Layton

From: Trond Myklebust <[email protected]>

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.

Quoted 7 lines
Quoted 2 lines

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.

Quoted 7 lines
Quoted 4 lines

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.

Quoted 4 lines

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

Quoted 101 lines
Quoted 89 lines
Quoted 89 lines
Signed-off-by Trond Myklebust <[email protected]>
Signed-off-by Jeff Layton <[email protected]>
Reviewed-by Christoph Hellwig <[email protected]>
---
 fs/nfsd/nfs4state.c | 22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)
fs/nfsd/nfs4state.c +8 -12
index a47c97586c76..8ff5c97a2c5a 100644
@@ -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);
Quoted 8 lines
Quoted 8 lines
fs/nfsd/nfs4state.c +2 -1
+                   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 <[email protected]>

Signature · 5 lines
Trond Myklebust

Linux NFS client maintainer, PrimaryData

[email protected]
author
#8 lore raw

PATCH v5 4/10 nfsd: Fix delegation revocation

In reply to 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 <[email protected]>
Signed-off-by Jeff Layton <[email protected]>
Reviewed-by Christoph Hellwig <[email protected]>
---
 fs/nfsd/nfs4state.c | 37 +++++++++++++++++++++----------------
 1 file changed, 21 insertions(+), 16 deletions(-)
fs/nfsd/nfs4state.c +21 -16
index 8ff5c97a2c5a..d8c28d5c600f 100644
@@ -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);
Signature · 1 line
1.9.3
author
#9 lore raw

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

In reply to 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 <[email protected]>
---
 fs/nfsd/nfs4state.c | 31 +++++++++++++++++--------------
 1 file changed, 17 insertions(+), 14 deletions(-)
fs/nfsd/nfs4state.c +17 -14
index d8c28d5c600f..58746f0f34a1 100644
@@ -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:
Signature · 1 line
1.9.3
author
#11 lore raw

PATCH v5 6/10 nfsd: Convert delegation counter to an atomic_long_t type

In reply to Jeff Layton

From: Trond Myklebust <[email protected]>

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 <[email protected]>
Reviewed-by Christoph Hellwig <[email protected]>
---
 fs/nfsd/nfs4state.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)
fs/nfsd/nfs4state.c +11 -7
index 58746f0f34a1..390515889c91 100644
@@ -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);
 	}
 }
Signature · 1 line
1.9.3
author
#12 lore raw

PATCH v5 7/10 nfsd: drop unused stp arg to alloc_init_deleg

In reply to Jeff Layton
Signed-off-by Jeff Layton <[email protected]>
Reviewed-by Christoph Hellwig <[email protected]>
---
 fs/nfsd/nfs4state.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
fs/nfsd/nfs4state.c +2 -2
index 390515889c91..acddedb7250e 100644
@@ -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);
Signature · 1 line
1.9.3
author
#13 lore raw

PATCH v5 8/10 nfsd: clean up arguments to nfs4_open_delegation

In reply to Jeff Layton

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

Signed-off-by Jeff Layton <[email protected]>
Reviewed-by Christoph Hellwig <[email protected]>
---
 fs/nfsd/nfs4state.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)
fs/nfsd/nfs4state.c +7 -6
index acddedb7250e..171ab5a2c60c 100644
@@ -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;
Signature · 1 line
1.9.3
author
#14 lore raw

PATCH v5 9/10 nfsd: clean up nfs4_set_delegation

In reply to 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 <[email protected]>
Reviewed-by Christoph Hellwig <[email protected]>
---
 fs/nfsd/nfs4state.c | 34 ++++++++++++++++++++++------------
 1 file changed, 22 insertions(+), 12 deletions(-)
fs/nfsd/nfs4state.c +22 -12
index 171ab5a2c60c..18d1b60f506f 100644
@@ -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 &&
Signature · 1 line
1.9.3
author
#15 lore raw

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

In reply to 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 <[email protected]>
Signed-off-by Jeff Layton <[email protected]>
Reviewed-by Christoph Hellwig <[email protected]>
---
 fs/nfsd/nfs4state.c | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)
fs/nfsd/nfs4state.c +11 -10
index 18d1b60f506f..3946a5a9459c 100644
@@ -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);
Signature · 1 line
1.9.3
#16 lore raw
In reply to Jeff Layton
Quoted 32 lines · 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

Quoted 8 lines

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.

Quoted 22 lines

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(-)
Signature · 1 line
1.9.3
#17 lore raw
Quoted 38 lines · J. Bruce Fields
Quoted 37 lines · 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.

Quoted 8 lines
Quoted 3 lines

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.

author
#18 lore raw

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

In reply to 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 <[email protected]>
---
 fs/nfsd/nfs4state.c | 1 +
 1 file changed, 1 insertion(+)
fs/nfsd/nfs4state.c +1 -0
index 3946a5a9459c..b21319816b89 100644
@@ -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);
Signature · 1 line
1.9.3
#19 lore raw

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

Quoted 17 lines · On Mon, 21 Jul 2014 15:46:13 -0400 Jeff Layton <[email protected]> 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 <[email protected]>
---
 fs/nfsd/nfs4state.c | 1 +
 1 file changed, 1 insertion(+)
fs/nfsd/nfs4state.c +0 -0
index 3946a5a9459c..b21319816b89 100644

fs/nfsd/nfs4state.c static int nfs4_setlease(struct nfs4_delegation *dp) · line 3520

Quoted 7 lines
fs/nfsd/nfs4state.c +1 -0
@@ -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,

Signature · 1 line
Jeff Layton <[email protected]>
#20 lore raw

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

Quoted 15 lines · Jeff Layton
Quoted 12 lines · On Mon, 21 Jul 2014 15:46:13 -0400 Jeff Layton <[email protected]> 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 <[email protected]>
---
 fs/nfsd/nfs4state.c | 1 +
 1 file changed, 1 insertion(+)
Quoted 16 lines

fs/nfsd/nfs4state.c static int nfs4_setlease(struct nfs4_delegation *dp) · line 3520

Quoted 12 lines
fs/nfsd/nfs4state.c +1 -0
index 3946a5a9459c..b21319816b89 100644
@@ -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.