[PATCH v3 0/6] nfsd: more delegation fixes to prepare for client_mutex removal

[PATCH v3 0/6] nfsd: more delegation fixes to prepare for client_mutex removal

[Jeff Layton]

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

Here's a respin of the delegation fixes that Bruce hasn't yet merged
into his nfsd-next branch. I think this should address most of the
review comments so far.

I'm still not 100% thrilled with the delegation handling as a whole, but
I think it's getting a bit tighter now. I tend to think that we'll need
to rework this at some point in the future (and maybe the generic lease
code as well). I'd prefer to wait until after the rest of the
scalability work is complete before doing that if possible though.

Jeff Layton (3):
  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

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 | 148 +++++++++++++++++++++++++++++++++-------------------
 fs/nfsd/state.h     |   2 +-
 2 files changed, 96 insertions(+), 54 deletions(-)

-- 
1.9.3

[PATCH 1/6] 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.

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

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index fd4deb049ddf..9bc896720db3 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,8 +3145,8 @@ 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);
+	fp->fi_had_conflict = true;
 	list_for_each_entry(dp, &fp->fi_delegations, dl_perfile)
 		nfsd_break_one_deleg(dp);
 	spin_unlock(&fp->fi_lock);
@@ -3493,7 +3495,7 @@ static int nfs4_setlease(struct nfs4_delegation *dp)
 {
 	struct nfs4_file *fp = dp->dl_file;
 	struct file_lock *fl;
-	int status;
+	int status = 0;
 
 	fl = nfs4_alloc_init_lease(fp, NFS4_OPEN_DELEGATE_READ);
 	if (!fl)
@@ -3501,15 +3503,31 @@ static int nfs4_setlease(struct nfs4_delegation *dp)
 	fl->fl_file = find_readable_file(fp);
 	status = vfs_setlease(fl->fl_file, fl->fl_type, &fl);
 	if (status)
-		goto out_free;
+		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;
 	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:
+out_unlock:
+	spin_unlock(&fp->fi_lock);
+	spin_unlock(&state_lock);
+out_fput:
 	if (fl->fl_file)
 		fput(fl->fl_file);
 	locks_free_lock(fl);
@@ -3518,19 +3536,27 @@ out_free:
 
 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;
 }
-- 
1.9.3

[PATCH 2/6] 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 9bc896720db3..d88a33fa8e8f 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 3/6] 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.

Also, move to kzalloc and get rid of explicit zeroing of fields.

Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
---
 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 d88a33fa8e8f..c06a08711e7f 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);
 }
@@ -4447,6 +4441,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

[PATCH 4/6] 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 c06a08711e7f..4e42c3823a8a 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)) {
@@ -3639,7 +3640,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 &&
@@ -3878,7 +3879,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) {
@@ -5353,7 +5355,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;
@@ -5608,12 +5611,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 5/6] 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 | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 4e42c3823a8a..ca8b4d0e712b 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -669,7 +669,7 @@ unhash_delegation_locked(struct nfs4_delegation *dp)
 
 static void destroy_revoked_delegation(struct nfs4_delegation *dp)
 {
-	list_del_init(&dp->dl_recall_lru);
+	WARN_ON(!list_empty(&dp->dl_recall_lru));
 	nfs4_put_delegation(dp);
 }
 
@@ -685,11 +685,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);
 	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,9 +1462,9 @@ 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);
+		list_del_init(&dp->dl_recall_lru);
 		destroy_revoked_delegation(dp);
 	}
 	while (!list_empty(&clp->cl_openowners)) {
@@ -3883,8 +3887,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) {
@@ -4228,6 +4234,9 @@ nfsd4_free_stateid(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 		break;
 	case NFS4_REVOKED_DELEG_STID:
 		dp = delegstateid(s);
+		spin_lock(&cl->cl_lock);
+		list_del_init(&dp->dl_recall_lru);
+		spin_unlock(&cl->cl_lock);
 		destroy_revoked_delegation(dp);
 		ret = nfs_ok;
 		break;
-- 
1.9.3

[PATCH 6/6] 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 ca8b4d0e712b..ba742efbb251 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

Re: [PATCH 1/6] nfsd: Protect the nfs4_file delegation fields using the fi_lock

[Jeff Layton]

On Thu, 17 Jul 2014 15:12:29 -0400
Jeff Layton <jlayton@primarydata.com> wrote:

> 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.
> 
> Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
> Signed-off-by: Jeff Layton <jlayton@primarydata.com>
> ---
>  fs/nfsd/nfs4state.c | 54 +++++++++++++++++++++++++++++++++++++++--------------
>  1 file changed, 40 insertions(+), 14 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index fd4deb049ddf..9bc896720db3 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,8 +3145,8 @@ 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);
> +	fp->fi_had_conflict = true;
>  	list_for_each_entry(dp, &fp->fi_delegations, dl_perfile)
>  		nfsd_break_one_deleg(dp);
>  	spin_unlock(&fp->fi_lock);
> @@ -3493,7 +3495,7 @@ static int nfs4_setlease(struct nfs4_delegation *dp)
>  {
>  	struct nfs4_file *fp = dp->dl_file;
>  	struct file_lock *fl;
> -	int status;
> +	int status = 0;
>  
>  	fl = nfs4_alloc_init_lease(fp, NFS4_OPEN_DELEGATE_READ);
>  	if (!fl)
> @@ -3501,15 +3503,31 @@ static int nfs4_setlease(struct nfs4_delegation *dp)
>  	fl->fl_file = find_readable_file(fp);
>  	status = vfs_setlease(fl->fl_file, fl->fl_type, &fl);
>  	if (status)
> -		goto out_free;
> +		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;

So, fwiw -- I think this is a little closer to what HCH was suggesting.
We're not using an atomic flag, per-se but rather a bog standard bool
inside the fi_lock spinlock.

I do have a minor concern that you could have fp->fi_had_conflict flip
to true just before you call vfs_setlease. If that happens you could
end up getting a lease but not hashing the delegation for it.

It seems like an unlikely race though -- if that happens, vfs_setlease
would likely return error, I'd think.

Thoughts?

> +	/* 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;
>  	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:
> +out_unlock:
> +	spin_unlock(&fp->fi_lock);
> +	spin_unlock(&state_lock);
> +out_fput:
>  	if (fl->fl_file)
>  		fput(fl->fl_file);
>  	locks_free_lock(fl);
> @@ -3518,19 +3536,27 @@ out_free:
>  
>  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;
>  }


-- 
Jeff Layton <jlayton@primarydata.com>

Re: [PATCH 1/6] nfsd: Protect the nfs4_file delegation fields using the fi_lock

[J. Bruce Fields]

On Thu, Jul 17, 2014 at 03:12:29PM -0400, Jeff Layton wrote:
> 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.

Do you mean "recalled" or returned?  The recall code doesn't currently
take the state lock anyway.

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

What's the worst that could happen here?  Maybe we could very rarely
send the recall before we send the open reply granting the delegation?
A client can probably actually handle that (it's possible those could
get reordered for reasons out of our control).

--b.

> 
> Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
> Signed-off-by: Jeff Layton <jlayton@primarydata.com>
> ---
>  fs/nfsd/nfs4state.c | 54 +++++++++++++++++++++++++++++++++++++++--------------
>  1 file changed, 40 insertions(+), 14 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index fd4deb049ddf..9bc896720db3 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,8 +3145,8 @@ 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);
> +	fp->fi_had_conflict = true;
>  	list_for_each_entry(dp, &fp->fi_delegations, dl_perfile)
>  		nfsd_break_one_deleg(dp);
>  	spin_unlock(&fp->fi_lock);
> @@ -3493,7 +3495,7 @@ static int nfs4_setlease(struct nfs4_delegation *dp)
>  {
>  	struct nfs4_file *fp = dp->dl_file;
>  	struct file_lock *fl;
> -	int status;
> +	int status = 0;
>  
>  	fl = nfs4_alloc_init_lease(fp, NFS4_OPEN_DELEGATE_READ);
>  	if (!fl)
> @@ -3501,15 +3503,31 @@ static int nfs4_setlease(struct nfs4_delegation *dp)
>  	fl->fl_file = find_readable_file(fp);
>  	status = vfs_setlease(fl->fl_file, fl->fl_type, &fl);
>  	if (status)
> -		goto out_free;
> +		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;
>  	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:
> +out_unlock:
> +	spin_unlock(&fp->fi_lock);
> +	spin_unlock(&state_lock);
> +out_fput:
>  	if (fl->fl_file)
>  		fput(fl->fl_file);
>  	locks_free_lock(fl);
> @@ -3518,19 +3536,27 @@ out_free:
>  
>  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;
>  }
> -- 
> 1.9.3
> 

Re: [PATCH 1/6] nfsd: Protect the nfs4_file delegation fields using the fi_lock

[Jeff Layton]

On Thu, 17 Jul 2014 15:12:29 -0400
Jeff Layton <jlayton@primarydata.com> wrote:

> 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.
> 
> Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
> Signed-off-by: Jeff Layton <jlayton@primarydata.com>
> ---
>  fs/nfsd/nfs4state.c | 54 +++++++++++++++++++++++++++++++++++++++--------------
>  1 file changed, 40 insertions(+), 14 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index fd4deb049ddf..9bc896720db3 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,8 +3145,8 @@ 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);
> +	fp->fi_had_conflict = true;
>  	list_for_each_entry(dp, &fp->fi_delegations, dl_perfile)
>  		nfsd_break_one_deleg(dp);
>  	spin_unlock(&fp->fi_lock);
> @@ -3493,7 +3495,7 @@ static int nfs4_setlease(struct nfs4_delegation *dp)
>  {
>  	struct nfs4_file *fp = dp->dl_file;
>  	struct file_lock *fl;
> -	int status;
> +	int status = 0;
>  
>  	fl = nfs4_alloc_init_lease(fp, NFS4_OPEN_DELEGATE_READ);
>  	if (!fl)
> @@ -3501,15 +3503,31 @@ static int nfs4_setlease(struct nfs4_delegation *dp)
>  	fl->fl_file = find_readable_file(fp);
>  	status = vfs_setlease(fl->fl_file, fl->fl_type, &fl);
>  	if (status)
> -		goto out_free;
> +		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;
>  	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:
> +out_unlock:
> +	spin_unlock(&fp->fi_lock);
> +	spin_unlock(&state_lock);
> +out_fput:
>  	if (fl->fl_file)
>  		fput(fl->fl_file);
>  	locks_free_lock(fl);
> @@ -3518,19 +3536,27 @@ out_free:
>  
>  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;
>  }

Oops. Stupid mistake that I just noticed. That return 0 above should be
"return status". Will fix...

-- 
Jeff Layton <jlayton@primarydata.com>

Re: [PATCH 1/6] nfsd: Protect the nfs4_file delegation fields using the fi_lock

[Jeff Layton]

On Thu, 17 Jul 2014 16:02:44 -0400
"J. Bruce Fields" <bfields@fieldses.org> wrote:

> On Thu, Jul 17, 2014 at 03:12:29PM -0400, Jeff Layton wrote:
> > 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.
> 
> Do you mean "recalled" or returned?  The recall code doesn't currently
> take the state lock anyway.
> 

Sorry, I meant "broken".

> > 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.
> 
> What's the worst that could happen here?  Maybe we could very rarely
> send the recall before we send the open reply granting the delegation?
> A client can probably actually handle that (it's possible those could
> get reordered for reasons out of our control).
> 
> --b.
> 

We could leak the lease we've just acquired. It's an unlikely race, but
the potential problem is this:

Suppose we have two callers into nfs4_setlease, nearly simultaneously.
One calls vfs_setlease and gets a lease and hashes its delegation. That
lease is broken and is eventually cleaned up. Now the fi_had_confict
flag is true.

The second now calls vfs_setlease and also gets a lease. It then checks
fi_had_conflict, finds it to be true and doesn't hash its delegation.
Eventually, that lease gets broken. The lm_break callback will set the
dl_time to 0 to ensure that time_out_leases won't clean it up, and
it'll just hang out there forever since nothing else would.

One possible fix would be to have the lm_break op set the dl_time to a
near-immediate value if there's nothing on the fi_delegations list. Then
we could just not hash the delegation if fl_had_conflict was true and
assume that time_out_leases will take care of it.

Bruce, can you hold off on merging these just yet? I'd like to do one
more respin to see if we can address that problem too. I also rolled up
a few other cleanups last night in this area.

Thanks,
Jeff

> > 
> > Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
> > Signed-off-by: Jeff Layton <jlayton@primarydata.com>
> > ---
> >  fs/nfsd/nfs4state.c | 54 +++++++++++++++++++++++++++++++++++++++--------------
> >  1 file changed, 40 insertions(+), 14 deletions(-)
> > 
> > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > index fd4deb049ddf..9bc896720db3 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,8 +3145,8 @@ 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);
> > +	fp->fi_had_conflict = true;
> >  	list_for_each_entry(dp, &fp->fi_delegations, dl_perfile)
> >  		nfsd_break_one_deleg(dp);
> >  	spin_unlock(&fp->fi_lock);
> > @@ -3493,7 +3495,7 @@ static int nfs4_setlease(struct nfs4_delegation *dp)
> >  {
> >  	struct nfs4_file *fp = dp->dl_file;
> >  	struct file_lock *fl;
> > -	int status;
> > +	int status = 0;
> >  
> >  	fl = nfs4_alloc_init_lease(fp, NFS4_OPEN_DELEGATE_READ);
> >  	if (!fl)
> > @@ -3501,15 +3503,31 @@ static int nfs4_setlease(struct nfs4_delegation *dp)
> >  	fl->fl_file = find_readable_file(fp);
> >  	status = vfs_setlease(fl->fl_file, fl->fl_type, &fl);
> >  	if (status)
> > -		goto out_free;
> > +		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;
> >  	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:
> > +out_unlock:
> > +	spin_unlock(&fp->fi_lock);
> > +	spin_unlock(&state_lock);
> > +out_fput:
> >  	if (fl->fl_file)
> >  		fput(fl->fl_file);
> >  	locks_free_lock(fl);
> > @@ -3518,19 +3536,27 @@ out_free:
> >  
> >  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;
> >  }
> > -- 
> > 1.9.3
> > 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


-- 
Jeff Layton <jlayton@primarydata.com>