nfsd: more delegation fixes to prepare for client_mutex removal

static int nfs4_set_delegation(struct nfs4_delegation *dp, struct nfs4_file *fp) {

fs/nfsd/nfs4state.c +8 -0

+	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) {
+		spin_unlock(&fp->fi_lock);
+		spin_unlock(&state_lock);
 		return nfs4_setlease(dp);
+	}

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

Quoted 6 lines

fs/nfsd/nfs4state.c +6 -0

+out_unlock:
+	spin_unlock(&fp->fi_lock);
+	spin_unlock(&state_lock);
+out_fput:
+	if (filp)
+		fput(filp);

I don't think fput can be NULL here.

Quoted 22 lines

static int nfs4_set_delegation(struct nfs4_delegation *dp, struct nfs4_file *fp) {

fs/nfsd/nfs4state.c +11 -0

+	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) {
+		spin_unlock(&fp->fi_lock);
+		spin_unlock(&state_lock);
 		return nfs4_setlease(dp);
+	}
 	atomic_inc(&fp->fi_delegees);
 	if (fp->fi_had_conflict) {
+		status = -EAGAIN;
+		goto out_unlock;
 	}
 	hash_delegation_locked(dp, fp);
+out_unlock:

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

Quoted 3 lines

fs/nfsd/nfs4state.c +2 -0

+	spin_unlock(&fp->fi_lock);
 	spin_unlock(&state_lock);
+	return status;

I have to admit that I didn't have time to go through all the surrounding code yet, but is there error handling correct here, i.e. no need to rop the file reference and cleanup dp->dl_file for any error or race case?

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 | 90 +++++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 66 insertions(+), 24 deletions(-)

fs/nfsd/nfs4state.c +66 -24

index fd4deb049ddf..9ab067e85b51 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,77 @@ 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:
+	if (filp)
+		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

Quoted 9 lines · 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.

Quoted 4 lines

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.

I understand now, thanks: so the lease break code walks the list of delegations associated with the file, finds none, and issues no recall, but the open code continues merrily on and returns a delegation, with the result that we return the client a delegation that will never be recalled.

That could be worded more carefully, and would be worth a separate patch (since the bug predates the new locking).

Quoted 10 lines

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.

Is there actually any guarantee time_out_leases() will get called on this inode again?

--b.

Quoted 180 lines

Signed-off-by Trond Myklebust <[email protected]>

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

---
 fs/nfsd/nfs4state.c | 90

+++++++++++++++++++++++++++++++++++++++-------------- 1 file changed, 66 insertions(+), 24 deletions(-)

fs/nfsd/nfs4state.c +7 -5

index fd4deb049ddf..9ab067e85b51 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) */

fs/nfsd/nfs4state.c +3 -3

 	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

fs/nfsd/nfs4state.c +1 -0

+	 * lease ever being cleaned up. Set the fl_break_time to

jiffies so that

fs/nfsd/nfs4state.c +1 -0

+	 * time_out_leases will do it ASAP. The fact that

fi_had_conflict is now

fs/nfsd/nfs4state.c +6 -0

+	 * 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)

fs/nfsd/nfs4state.c +48 -16

+			nfsd_break_one_deleg(dp);
 	spin_unlock(&fp->fi_lock);
 }

@@ -3493,46 +3504,77 @@ 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:
+	if (filp)
+		fput(filp);
 	return status;
 }
 
 static int nfs4_set_delegation(struct nfs4_delegation *dp, struct
 nfs4_file *fp) {
+	int status = 0;
+
 	if (fp->fi_had_conflict)
 		return -EAGAIN;
 	get_nfs4_file(fp);
+	spin_lock(&state_lock);
+	spin_lock(&fp->fi_lock);
 	dp->dl_file = fp;
-	if (!fp->fi_lease)
+	if (!fp->fi_lease) {
+		spin_unlock(&fp->fi_lock);
+		spin_unlock(&state_lock);
 		return nfs4_setlease(dp);
-	spin_lock(&state_lock);
+	}
 	atomic_inc(&fp->fi_delegees);
 	if (fp->fi_had_conflict) {
-		spin_unlock(&state_lock);
-		return -EAGAIN;
+		status = -EAGAIN;
+		goto out_unlock;
 	}
 	hash_delegation_locked(dp, fp);
+out_unlock:
+	spin_unlock(&fp->fi_lock);
 	spin_unlock(&state_lock);
-	return 0;
+	return status;
 }
 
 static void nfsd4_open_deleg_none_ext(struct nfsd4_open *open, int

status) --
1.9.3

Quoted 21 lines · On Fri, 18 Jul 2014 12:28:25 -0400 "J. Bruce Fields" <[email protected]>…

Quoted 9 lines · 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.

Quoted 4 lines

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.

I understand now, thanks: so the lease break code walks the list of delegations associated with the file, finds none, and issues no recall, but the open code continues merrily on and returns a delegation, with the result that we return the client a delegation that will never be recalled.

Quoted 3 lines

That could be worded more carefully, and would be worth a separate patch (since the bug predates the new locking).

Yes, that's basically correct. I'd have to think about how to fix that with the current code. It's probably doable if you think it's worthwhile, but I'll need to rebase this set on top of it.

Quoted 14 lines

Quoted 10 lines

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.

Is there actually any guarantee time_out_leases() will get called on this inode again?

Quoted 2 lines

--b.

Yes. Lease breaks are handled in two phases. We walk the i_flock list and issue a ->lm_break on each lease, and then later we walk the list again after putting the task to sleep, and try to time out the leases. So by doing this, we should ensure that the task will wake up after sleeping and delete it.

Quoted 181 lines

Quoted 181 lines

Signed-off-by Trond Myklebust <[email protected]>

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

---
 fs/nfsd/nfs4state.c | 90

+++++++++++++++++++++++++++++++++++++++-------------- 1 file changed, 66 insertions(+), 24 deletions(-)

fs/nfsd/nfs4state.c +7 -5

index fd4deb049ddf..9ab067e85b51 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) */

fs/nfsd/nfs4state.c +3 -3

 	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

fs/nfsd/nfs4state.c +1 -0

+	 * lease ever being cleaned up. Set the fl_break_time to

jiffies so that

fs/nfsd/nfs4state.c +1 -0

+	 * time_out_leases will do it ASAP. The fact that

fi_had_conflict is now

fs/nfsd/nfs4state.c +6 -0

+	 * 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)

fs/nfsd/nfs4state.c +48 -16

+			nfsd_break_one_deleg(dp);
 	spin_unlock(&fp->fi_lock);
 }

@@ -3493,46 +3504,77 @@ 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:
+	if (filp)
+		fput(filp);
 	return status;
 }
 
 static int nfs4_set_delegation(struct nfs4_delegation *dp, struct
 nfs4_file *fp) {
+	int status = 0;
+
 	if (fp->fi_had_conflict)
 		return -EAGAIN;
 	get_nfs4_file(fp);
+	spin_lock(&state_lock);
+	spin_lock(&fp->fi_lock);
 	dp->dl_file = fp;
-	if (!fp->fi_lease)
+	if (!fp->fi_lease) {
+		spin_unlock(&fp->fi_lock);
+		spin_unlock(&state_lock);
 		return nfs4_setlease(dp);
-	spin_lock(&state_lock);
+	}
 	atomic_inc(&fp->fi_delegees);
 	if (fp->fi_had_conflict) {
-		spin_unlock(&state_lock);
-		return -EAGAIN;
+		status = -EAGAIN;
+		goto out_unlock;
 	}
 	hash_delegation_locked(dp, fp);
+out_unlock:
+	spin_unlock(&fp->fi_lock);
 	spin_unlock(&state_lock);
-	return 0;
+	return status;
 }
 
 static void nfsd4_open_deleg_none_ext(struct nfsd4_open *open, int

status) --
1.9.3

Signature · 1 line

Jeff Layton <[email protected]>

Quoted 28 lines · Jeff Layton

Quoted 24 lines · On Fri, 18 Jul 2014 12:28:25 -0400 "J. Bruce Fields" <[email protected]>…

Quoted 9 lines · 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.

Quoted 4 lines

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.

I understand now, thanks: so the lease break code walks the list of delegations associated with the file, finds none, and issues no recall, but the open code continues merrily on and returns a delegation, with the result that we return the client a delegation that will never be recalled.

That could be worded more carefully, and would be worth a separate patch (since the bug predates the new locking).

Quoted 3 lines

Yes, that's basically correct. I'd have to think about how to fix that with the current code. It's probably doable if you think it's worthwhile, but I'll need to rebase this set on top of it.

Well, I was wondering if this patch could just be split in two, no need to backport further than that.

Quoted 14 lines

Quoted 14 lines

Quoted 10 lines

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.

Is there actually any guarantee time_out_leases() will get called on this inode again?

Quoted 8 lines

Quoted 2 lines

--b.

Yes. Lease breaks are handled in two phases. We walk the i_flock list and issue a ->lm_break on each lease, and then later we walk the list again after putting the task to sleep, and try to time out the leases. So by doing this, we should ensure that the task will wake up after sleeping and delete it.

In the case of an interrupt or a nonblocking break (which is what nfsd will do), then time_out_leases isn't called again from what I could tell.

--b.

Quoted 185 lines

Quoted 181 lines

Quoted 181 lines

Signed-off-by Trond Myklebust <[email protected]>

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

---
 fs/nfsd/nfs4state.c | 90

+++++++++++++++++++++++++++++++++++++++-------------- 1 file changed, 66 insertions(+), 24 deletions(-)

fs/nfsd/nfs4state.c +7 -5

index fd4deb049ddf..9ab067e85b51 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) */

fs/nfsd/nfs4state.c +3 -3

 	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

fs/nfsd/nfs4state.c +1 -0

+	 * lease ever being cleaned up. Set the fl_break_time to

jiffies so that

fs/nfsd/nfs4state.c +1 -0

+	 * time_out_leases will do it ASAP. The fact that

fi_had_conflict is now

fs/nfsd/nfs4state.c +6 -0

+	 * 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)

fs/nfsd/nfs4state.c +48 -16

+			nfsd_break_one_deleg(dp);
 	spin_unlock(&fp->fi_lock);
 }

@@ -3493,46 +3504,77 @@ 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:
+	if (filp)
+		fput(filp);
 	return status;
 }
 
 static int nfs4_set_delegation(struct nfs4_delegation *dp, struct
 nfs4_file *fp) {
+	int status = 0;
+
 	if (fp->fi_had_conflict)
 		return -EAGAIN;
 	get_nfs4_file(fp);
+	spin_lock(&state_lock);
+	spin_lock(&fp->fi_lock);
 	dp->dl_file = fp;
-	if (!fp->fi_lease)
+	if (!fp->fi_lease) {
+		spin_unlock(&fp->fi_lock);
+		spin_unlock(&state_lock);
 		return nfs4_setlease(dp);
-	spin_lock(&state_lock);
+	}
 	atomic_inc(&fp->fi_delegees);
 	if (fp->fi_had_conflict) {
-		spin_unlock(&state_lock);
-		return -EAGAIN;
+		status = -EAGAIN;
+		goto out_unlock;
 	}
 	hash_delegation_locked(dp, fp);
+out_unlock:
+	spin_unlock(&fp->fi_lock);
 	spin_unlock(&state_lock);
-	return 0;
+	return status;
 }
 
 static void nfsd4_open_deleg_none_ext(struct nfsd4_open *open, int

status) --
1.9.3

Signature · 1 line

Jeff Layton <[email protected]>

Quoted 29 lines · On Fri, 18 Jul 2014 13:49:57 -0400 "J. Bruce Fields" <[email protected]>…

Quoted 28 lines · Jeff Layton

Quoted 24 lines · On Fri, 18 Jul 2014 12:28:25 -0400 "J. Bruce Fields" <[email protected]>…

Quoted 9 lines · 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.

Quoted 4 lines

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.

I understand now, thanks: so the lease break code walks the list of delegations associated with the file, finds none, and issues no recall, but the open code continues merrily on and returns a delegation, with the result that we return the client a delegation that will never be recalled.

That could be worded more carefully, and would be worth a separate patch (since the bug predates the new locking).

Quoted 7 lines

Quoted 3 lines

Yes, that's basically correct. I'd have to think about how to fix that with the current code. It's probably doable if you think it's worthwhile, but I'll need to rebase this set on top of it.

Well, I was wondering if this patch could just be split in two, no need to backport further than that.

Erm, now that I've looked, I don't think it'll be that easy. The key here is to ensure that fi_had_conflict is set while holding the fi_lock. The trick here is that we need to take it in nfs4_setlease as well, and check the flag before hashing the delegation without dropping the fi_lock.

Quoted 27 lines

Quoted 14 lines

Quoted 14 lines

Quoted 10 lines

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.

Is there actually any guarantee time_out_leases() will get called on this inode again?

Quoted 8 lines

Quoted 2 lines

--b.

Yes. Lease breaks are handled in two phases. We walk the i_flock list and issue a ->lm_break on each lease, and then later we walk the list again after putting the task to sleep, and try to time out the leases. So by doing this, we should ensure that the task will wake up after sleeping and delete it.

In the case of an interrupt or a nonblocking break (which is what nfsd will do), then time_out_leases isn't called again from what I could tell.

Quoted 2 lines

--b.

In both cases, time_out_leases is still called at the beginning of __break_lease. So the next time that function is called it'll get cleaned up, or when the filp is closed (in locks_remove_file).

Quoted 40 lines · Jeff Layton

Quoted 29 lines · On Fri, 18 Jul 2014 13:49:57 -0400 "J. Bruce Fields" <[email protected]>…

Quoted 28 lines · Jeff Layton

Quoted 24 lines · On Fri, 18 Jul 2014 12:28:25 -0400 "J. Bruce Fields" <[email protected]>…

Quoted 9 lines · 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.

Quoted 4 lines

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.

I understand now, thanks: so the lease break code walks the list of delegations associated with the file, finds none, and issues no recall, but the open code continues merrily on and returns a delegation, with the result that we return the client a delegation that will never be recalled.

That could be worded more carefully, and would be worth a separate patch (since the bug predates the new locking).

Quoted 7 lines

Quoted 3 lines

Yes, that's basically correct. I'd have to think about how to fix that with the current code. It's probably doable if you think it's worthwhile, but I'll need to rebase this set on top of it.

Well, I was wondering if this patch could just be split in two, no need to backport further than that.

Quoted 5 lines

Erm, now that I've looked, I don't think it'll be that easy. The key here is to ensure that fi_had_conflict is set while holding the fi_lock. The trick here is that we need to take it in nfs4_setlease as well, and check the flag before hashing the delegation without dropping the fi_lock.

OK, I'll live. For the sake of anyone that actually runs across that bug I'll update the summary and changelog to emphasize the bugfix over the locking change.

Quoted 30 lines

Quoted 29 lines

Quoted 14 lines

Quoted 14 lines

Quoted 10 lines

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.

Is there actually any guarantee time_out_leases() will get called on this inode again?

Quoted 8 lines

Quoted 2 lines

--b.

Yes. Lease breaks are handled in two phases. We walk the i_flock list and issue a ->lm_break on each lease, and then later we walk the list again after putting the task to sleep, and try to time out the leases. So by doing this, we should ensure that the task will wake up after sleeping and delete it.

In the case of an interrupt or a nonblocking break (which is what nfsd will do), then time_out_leases isn't called again from what I could tell.

--b.

Quoted 3 lines

In both cases, time_out_leases is still called at the beginning of __break_lease. So the next time that function is called it'll get cleaned up, or when the filp is closed (in locks_remove_file).

Right, but there's no guarantee another break_lease comes. E.g. the process waiting on the lease break could get killed.

--b.

Quoted 47 lines · On Fri, 18 Jul 2014 15:21:49 -0400 "J. Bruce Fields" <[email protected]>…

Quoted 40 lines · Jeff Layton

Quoted 29 lines · On Fri, 18 Jul 2014 13:49:57 -0400 "J. Bruce Fields" <[email protected]>…

Quoted 28 lines · Jeff Layton

Quoted 24 lines · On Fri, 18 Jul 2014 12:28:25 -0400 "J. Bruce Fields" <[email protected]>…

Quoted 9 lines · 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.

Quoted 4 lines

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.

I understand now, thanks: so the lease break code walks the list of delegations associated with the file, finds none, and issues no recall, but the open code continues merrily on and returns a delegation, with the result that we return the client a delegation that will never be recalled.

That could be worded more carefully, and would be worth a separate patch (since the bug predates the new locking).

Quoted 7 lines

Quoted 3 lines

Yes, that's basically correct. I'd have to think about how to fix that with the current code. It's probably doable if you think it's worthwhile, but I'll need to rebase this set on top of it.

Well, I was wondering if this patch could just be split in two, no need to backport further than that.

Quoted 5 lines

Erm, now that I've looked, I don't think it'll be that easy. The key here is to ensure that fi_had_conflict is set while holding the fi_lock. The trick here is that we need to take it in nfs4_setlease as well, and check the flag before hashing the delegation without dropping the fi_lock.

Quoted 4 lines

OK, I'll live. For the sake of anyone that actually runs across that bug I'll update the summary and changelog to emphasize the bugfix over the locking change.

Ok, thanks.

Quoted 34 lines

Quoted 33 lines

Quoted 23 lines

Quoted 14 lines

Quoted 14 lines

Quoted 10 lines

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.

Is there actually any guarantee time_out_leases() will get called on this inode again?

Quoted 8 lines

Quoted 2 lines

--b.

Yes. Lease breaks are handled in two phases. We walk the i_flock list and issue a ->lm_break on each lease, and then later we walk the list again after putting the task to sleep, and try to time out the leases. So by doing this, we should ensure that the task will wake up after sleeping and delete it.

Quoted 6 lines

In the case of an interrupt or a nonblocking break (which is what nfsd will do), then time_out_leases isn't called again from what I could tell.

--b.

In both cases, time_out_leases is still called at the beginning of __break_lease. So the next time that function is called it'll get cleaned up, or when the filp is closed (in locks_remove_file).

Quoted 4 lines

Right, but there's no guarantee another break_lease comes. E.g. the process waiting on the lease break could get killed.

--b.

In that case, there's no harm in leaving the lease on the list until the filp closes.

FWIW, I looked at trying to just remove the lease from the list, but that's not safe from the lm_break callback. So, I think this is the best we can reasonably do here.

Quoted 40 lines · Jeff Layton

Quoted 29 lines · On Fri, 18 Jul 2014 13:49:57 -0400 "J. Bruce Fields" <[email protected]>…

Quoted 28 lines · Jeff Layton

Quoted 24 lines · On Fri, 18 Jul 2014 12:28:25 -0400 "J. Bruce Fields" <[email protected]>…

Quoted 9 lines · 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.

Quoted 4 lines

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.

I understand now, thanks: so the lease break code walks the list of delegations associated with the file, finds none, and issues no recall, but the open code continues merrily on and returns a delegation, with the result that we return the client a delegation that will never be recalled.

That could be worded more carefully, and would be worth a separate patch (since the bug predates the new locking).

Quoted 7 lines

Quoted 3 lines

Yes, that's basically correct. I'd have to think about how to fix that with the current code. It's probably doable if you think it's worthwhile, but I'll need to rebase this set on top of it.

Well, I was wondering if this patch could just be split in two, no need to backport further than that.

Quoted 5 lines

Erm, now that I've looked, I don't think it'll be that easy. The key here is to ensure that fi_had_conflict is set while holding the fi_lock. The trick here is that we need to take it in nfs4_setlease as well, and check the flag before hashing the delegation without dropping the fi_lock.

OK, I'll live. For the sake of anyone that actually runs across that bug I'll update the summary and changelog to emphasize the bugfix over the locking change.

Quoted 47 lines · J. Bruce Fields

Quoted 40 lines · Jeff Layton

Quoted 29 lines · On Fri, 18 Jul 2014 13:49:57 -0400 "J. Bruce Fields" <[email protected]>…

Quoted 28 lines · Jeff Layton

Quoted 24 lines · On Fri, 18 Jul 2014 12:28:25 -0400 "J. Bruce Fields" <[email protected]>…

Quoted 9 lines · 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.

Quoted 4 lines

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.

I understand now, thanks: so the lease break code walks the list of delegations associated with the file, finds none, and issues no recall, but the open code continues merrily on and returns a delegation, with the result that we return the client a delegation that will never be recalled.

That could be worded more carefully, and would be worth a separate patch (since the bug predates the new locking).

Quoted 7 lines

Quoted 3 lines

Yes, that's basically correct. I'd have to think about how to fix that with the current code. It's probably doable if you think it's worthwhile, but I'll need to rebase this set on top of it.

Well, I was wondering if this patch could just be split in two, no need to backport further than that.

Quoted 5 lines

Erm, now that I've looked, I don't think it'll be that easy. The key here is to ensure that fi_had_conflict is set while holding the fi_lock. The trick here is that we need to take it in nfs4_setlease as well, and check the flag before hashing the delegation without dropping the fi_lock.

Quoted 3 lines

OK, I'll live. For the sake of anyone that actually runs across that bug I'll update the summary and changelog to emphasize the bugfix over the locking change.

So, intending to apply as follows.

--b.

commit 417c6629b2d81d5a18d29c4bbb6a9a4c64282a36

Author: Jeff Layton <[email protected]>
Date:   Mon Jul 21 09:34:57 2014 -0400

nfsd: fix race that grants unrecallable delegation

If nfs4_setlease succesfully acquires a new delegation, then another task breaks the delegation before we reach hash_delegation_locked, then the breaking task will see an empty fi_delegations list and do nothing. The client will receive an open reply incorrectly granting a delegation and will never receive a recall.

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.

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

Reviewed-by Christoph Hellwig <[email protected]>

Signed-off-by J. Bruce Fields <[email protected]>

fs/nfsd/nfs4state.c +63 -23

index 10cdb67..cc477dd 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)

@@ -3141,10 +3143,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);
 }
 

@@ -3491,46 +3502,77 @@ 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;
+	}
+	fl->fl_file = filp;
+	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;

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 60ae21abce00..7c5233427c9b 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)) {

@@ -3655,7 +3656,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 &&

@@ -3894,7 +3895,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) {

@@ -5369,7 +5371,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;

@@ -5624,12 +5627,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

Quoted 2 lines · Jeff Layton

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

So if I understand right, the big mutex protects all of this right now, so this is all just moving the unhashing under a finer-grained lock to prevent that in-between state being exposed after the big lock's dropped. Looks reasonable.

--b.

Quoted 131 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 | 37 +++++++++++++++++++++----------------
 1 file changed, 21 insertions(+), 16 deletions(-)

fs/nfsd/nfs4state.c +21 -16

index 60ae21abce00..7c5233427c9b 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)) {

@@ -3655,7 +3656,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 &&

@@ -3894,7 +3895,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) {

@@ -5369,7 +5371,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;

@@ -5624,12 +5627,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

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

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

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

fs/nfsd/nfs4state.c +4 -1

+nfs4_set_delegation(struct nfs4_client *clp, struct svc_fh *fh,
+		    struct nfs4_file *fp)
 {
-	int status = 0;
+	int status;
+	struct nfs4_delegation *dp;

Quoted 6 lines

fs/nfsd/nfs4state.c +4 -1

+nfs4_set_delegation(struct nfs4_client *clp, struct svc_fh *fh,
+		    struct nfs4_file *fp)
 {
-	int status = 0;
+	int status;
+	struct nfs4_delegation *dp;

fs/nfsd/nfs4state.c out_fput: · line 3570

Quoted 10 lines

fs/nfsd/nfs4state.c +6 -1

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

Seems like we should pass the file pointer to alloc_init_deleg as well so it can set that one one up and grab the reference without any lock?

Otherwise looks good,

Reviewed-by Christoph Hellwig <[email protected]>

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

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

fs/nfsd/nfs4state.c +0 -0

index a2c6c85adfc7..952def00363b 100644

fs/nfsd/nfs4state.c +3 -2

  */
+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;

fs/nfsd/nfs4state.c +6 -5

 	if (bd->entries == 0)
 		bd->swap_time = seconds_since_boot();
 	bd->entries += 1;
+	spin_unlock(&blocked_delegations_lock);
 }
 
 static struct nfs4_delegation *

@@ -3097,16 +3098,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);

Quoted 8 lines · On Fri, 18 Jul 2014 11:13:36 -0400 Jeff Layton <[email protected]> wrote:

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

Makes sense, thanks.
However.....

Quoted 14 lines

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

fs/nfsd/nfs4state.c +0 -2

index a2c6c85adfc7..952def00363b 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,
- * is used to manage concurrent access.  Testing does not need the lock

fs/nfsd/nfs4state.c static struct nfs4_ol_stateid * nfs4_alloc_stateid(struct nfs4_client *clp) · line 511

Quoted 4 lines

fs/nfsd/nfs4state.c +3 -1

- * except when swapping the two filters.
+ * 'blocked_delegations_lock', which is always held when block_delegations()
+ * is called, is used to manage concurrent access.  Testing does not need the
+ * lock except when swapping the two filters.

...this comment is wrong. blocked_delegations_lock is *not* held when block_delegations() is call, it is taken when needed (almost) by block_delegations.

Quoted 25 lines

fs/nfsd/nfs4state.c +3 -2

  */
+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;

fs/nfsd/nfs4state.c static void block_delegations(struct knfsd_fh *fh) · line 564

Quoted 10 lines

fs/nfsd/nfs4state.c +1 -2

 	struct bloom_pair *bd = &blocked_delegations;
 
-	lockdep_assert_held(&state_lock);
-
 	hash = arch_fast_hash(&fh->fh_base, fh->fh_size, 0);
 
 	__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]);
+	spin_lock(&blocked_delegations_lock);

__set_bit isn't atomic. The spin_lock should be taken *before* these __set_bit() calls.

Otherwise, looks fine.

Thanks,
NeilBrown

Quoted 29 lines

fs/nfsd/nfs4state.c +6 -5

 	if (bd->entries == 0)
 		bd->swap_time = seconds_since_boot();
 	bd->entries += 1;
+	spin_unlock(&blocked_delegations_lock);
 }
 
 static struct nfs4_delegation *

@@ -3097,16 +3098,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);

Quoted 30 lines · On Mon, 21 Jul 2014 17:02:54 +1000 NeilBrown <[email protected]> wrote:

Quoted 8 lines · On Fri, 18 Jul 2014 11:13:36 -0400 Jeff Layton <[email protected]> wrote:

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

Makes sense, thanks.
However.....

Quoted 14 lines

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

fs/nfsd/nfs4state.c +0 -2

index a2c6c85adfc7..952def00363b 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,
- * is used to manage concurrent access.  Testing does not need the lock

Quoted 9 lines

fs/nfsd/nfs4state.c static struct nfs4_ol_stateid * nfs4_alloc_stateid(struct nfs4_client *clp) · line 511

Quoted 4 lines

fs/nfsd/nfs4state.c +3 -1

- * except when swapping the two filters.
+ * 'blocked_delegations_lock', which is always held when block_delegations()
+ * is called, is used to manage concurrent access.  Testing does not need the
+ * lock except when swapping the two filters.

...this comment is wrong. blocked_delegations_lock is *not* held when block_delegations() is call, it is taken when needed (almost) by block_delegations.

Thanks, fixed.

Quoted 39 lines

Quoted 25 lines

fs/nfsd/nfs4state.c +3 -2

  */
+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;

fs/nfsd/nfs4state.c static void block_delegations(struct knfsd_fh *fh) · line 564

Quoted 10 lines

fs/nfsd/nfs4state.c +1 -2

 	struct bloom_pair *bd = &blocked_delegations;
 
-	lockdep_assert_held(&state_lock);
-
 	hash = arch_fast_hash(&fh->fh_base, fh->fh_size, 0);
 
 	__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]);
+	spin_lock(&blocked_delegations_lock);

__set_bit isn't atomic. The spin_lock should be taken *before* these __set_bit() calls.

Quoted 6 lines

Otherwise, looks fine.

Thanks,
NeilBrown

Ok. I guess the worry is that we could end up setting bits in the middle of swapping the two fields? Makes sense -- fixed in my repo. I'll send out the updated set later today (it also includes a few nits that HCH pointed out last week).

As a side note...I wonder how much we'll get in the way of false positives with this scheme?

Given that we'll always have (or will have had) a nfs4_file corresponding to this inode, perhaps we'd be better off doing something like storing (and maybe hashing on) the filehandle in the nfs4_file, and just ensuring that we hold on to it for 30s or so after the last put?

Not something I'm looking at doing today, but it might be worth considering for a later delegations rework.

Quoted 30 lines

Quoted 29 lines

fs/nfsd/nfs4state.c +6 -5

 	if (bd->entries == 0)
 		bd->swap_time = seconds_since_boot();
 	bd->entries += 1;
+	spin_unlock(&blocked_delegations_lock);
 }
 
 static struct nfs4_delegation *

@@ -3097,16 +3098,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

Jeff Layton <[email protected]>

Quoted 96 lines · Jeff Layton

Quoted 30 lines · On Mon, 21 Jul 2014 17:02:54 +1000 NeilBrown <[email protected]> wrote:

Quoted 8 lines · On Fri, 18 Jul 2014 11:13:36 -0400 Jeff Layton <[email protected]> wrote:

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

Makes sense, thanks.
However.....

Quoted 14 lines

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

fs/nfsd/nfs4state.c +0 -2

index a2c6c85adfc7..952def00363b 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,
- * is used to manage concurrent access.  Testing does not need the lock

Quoted 9 lines

fs/nfsd/nfs4state.c static struct nfs4_ol_stateid * nfs4_alloc_stateid(struct nfs4_client *clp) · line 511

Quoted 4 lines

fs/nfsd/nfs4state.c +3 -1

- * except when swapping the two filters.
+ * 'blocked_delegations_lock', which is always held when block_delegations()
+ * is called, is used to manage concurrent access.  Testing does not need the
+ * lock except when swapping the two filters.

...this comment is wrong. blocked_delegations_lock is *not* held when block_delegations() is call, it is taken when needed (almost) by block_delegations.

Thanks, fixed.

Quoted 39 lines

Quoted 25 lines

fs/nfsd/nfs4state.c +3 -2

  */
+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;

fs/nfsd/nfs4state.c static void block_delegations(struct knfsd_fh *fh) · line 564

Quoted 10 lines

fs/nfsd/nfs4state.c +1 -2

 	struct bloom_pair *bd = &blocked_delegations;
 
-	lockdep_assert_held(&state_lock);
-
 	hash = arch_fast_hash(&fh->fh_base, fh->fh_size, 0);
 
 	__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]);
+	spin_lock(&blocked_delegations_lock);

__set_bit isn't atomic. The spin_lock should be taken *before* these __set_bit() calls.

Quoted 6 lines

Otherwise, looks fine.

Thanks,
NeilBrown

Ok. I guess the worry is that we could end up setting bits in the middle of swapping the two fields? Makes sense -- fixed in my repo. I'll send out the updated set later today (it also includes a few nits that HCH pointed out last week).

Quoted 8 lines

As a side note...I wonder how much we'll get in the way of false positives with this scheme?

Given that we'll always have (or will have had) a nfs4_file corresponding to this inode, perhaps we'd be better off doing something like storing (and maybe hashing on) the filehandle in the nfs4_file, and just ensuring that we hold on to it for 30s or so after the last put?

You don't want to hold a reference to the inode unnecessarily. (Consider for example the case of a deleted-but-still-opened file, in which case people can notice if a large file hangs around eating up space for an extra 30 seconds.) So I suppose you'd put fi_inode on last close and just make sure the rest of the code is prepared to deal with nfs4_file's with struct inodes. That might make sense to do.

Occasional false positives aren't necessarily a big deal, so the current approach seems a reasonable compromise for now.

Quoted 38 lines

Not something I'm looking at doing today, but it might be worth considering for a later delegations rework.

Quoted 30 lines

Quoted 29 lines

fs/nfsd/nfs4state.c +6 -5

 	if (bd->entries == 0)
 		bd->swap_time = seconds_since_boot();
 	bd->entries += 1;
+	spin_unlock(&blocked_delegations_lock);
 }
 
 static struct nfs4_delegation *

@@ -3097,16 +3098,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

Jeff Layton <[email protected]>

Quoted 12 lines · On Mon, 21 Jul 2014 17:02:54 +1000 NeilBrown <[email protected]> wrote:

fs/nfsd/nfs4state.c static void block_delegations(struct knfsd_fh *fh) · line 564

Quoted 6 lines

fs/nfsd/nfs4state.c +1 -0

 	hash = arch_fast_hash(&fh->fh_base, fh->fh_size, 0);
 
 	__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]);
+	spin_lock(&blocked_delegations_lock);

__set_bit isn't atomic. The spin_lock should be taken *before* these __set_bit() calls.

Otherwise, looks fine.

Quoted 4 lines

Thanks,
NeilBrown

Ok. I guess the worry is that we could end up setting bits in the middle of swapping the two fields? Makes sense -- fixed in my repo.

I'll send out the updated set later today (it also includes a few nits that HCH pointed out last week).

As a side note...I wonder how much we'll get in the way of false positives with this scheme?

Quoted 15 lines · On Mon, 21 Jul 2014 07:44:12 -0400 Jeff Layton <[email protected]>…

Quoted 12 lines · On Mon, 21 Jul 2014 17:02:54 +1000 NeilBrown <[email protected]> wrote:

fs/nfsd/nfs4state.c static void block_delegations(struct knfsd_fh *fh) · line 564

Quoted 6 lines

fs/nfsd/nfs4state.c +1 -0

 	hash = arch_fast_hash(&fh->fh_base, fh->fh_size, 0);
 
 	__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]);
+	spin_lock(&blocked_delegations_lock);

__set_bit isn't atomic. The spin_lock should be taken *before* these __set_bit() calls.

Otherwise, looks fine.

Quoted 7 lines

Quoted 4 lines

Thanks,
NeilBrown

Ok. I guess the worry is that we could end up setting bits in the middle of swapping the two fields? Makes sense -- fixed in my repo.

It is more subtle than that.
__set_bit() will:
read a value from memory to a register
set a bit in the register
write the register back out to memory

If two threads both run __set_bit on the same word of memory at the same time, one of the updates can get lost.
set_bit() (no underscore) performs an atomic RMW to avoid this, but is more expensive.
spin_lock() obviously ensures the required exclusion and as we are going to take the lock anyway we may as well take it before setting bits so we can use the non-atomic (cheaper) __set_bit function.

Quoted 3 lines

I'll send out the updated set later today (it also includes a few nits that HCH pointed out last week).

Quoted 2 lines

As a side note...I wonder how much we'll get in the way of false positives with this scheme?

If a future version of NFSv4 could allow delegations to be granted while a file is open (oh, it seems you are the only client using this file at the moment, you can treat this "open" as a delegation if you like) a few false positives would be a complete non-issue.

Quoted 43 lines · NeilBrown

Quoted 15 lines · On Mon, 21 Jul 2014 07:44:12 -0400 Jeff Layton <[email protected]>…

Quoted 12 lines · On Mon, 21 Jul 2014 17:02:54 +1000 NeilBrown <[email protected]> wrote:

fs/nfsd/nfs4state.c static void block_delegations(struct knfsd_fh *fh) · line 564

Quoted 6 lines

fs/nfsd/nfs4state.c +1 -0

 	hash = arch_fast_hash(&fh->fh_base, fh->fh_size, 0);
 
 	__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]);
+	spin_lock(&blocked_delegations_lock);

__set_bit isn't atomic. The spin_lock should be taken *before* these __set_bit() calls.

Otherwise, looks fine.

Quoted 7 lines

Quoted 4 lines

Thanks,
NeilBrown

Ok. I guess the worry is that we could end up setting bits in the middle of swapping the two fields? Makes sense -- fixed in my repo.

It is more subtle than that.
__set_bit() will:
read a value from memory to a register
set a bit in the register
write the register back out to memory

If two threads both run __set_bit on the same word of memory at the same time, one of the updates can get lost.
set_bit() (no underscore) performs an atomic RMW to avoid this, but is more expensive.
spin_lock() obviously ensures the required exclusion and as we are going to take the lock anyway we may as well take it before setting bits so we can use the non-atomic (cheaper) __set_bit function.

Quoted 3 lines

I'll send out the updated set later today (it also includes a few nits that HCH pointed out last week).

Quoted 7 lines

Quoted 2 lines

As a side note...I wonder how much we'll get in the way of false positives with this scheme?

If a future version of NFSv4 could allow delegations to be granted while a file is open (oh, it seems you are the only client using this file at the moment, you can treat this "open" as a delegation if you like) a few false positives would be a complete non-issue.

For what it's worth, I think 4.1 provides what you're asking for here; see

	http://tools.ietf.org/html/rfc5661#section-20.7

and the discussion of the various WANT_ flags in

	http://tools.ietf.org/html/rfc5661#section-18.16.3

As far as I know none of that is implemented yet.

--b.

Quoted 63 lines · On Mon, 21 Jul 2014 17:17:57 -0400 "J. Bruce Fields" <[email protected]>…

Quoted 43 lines · NeilBrown

Quoted 15 lines · On Mon, 21 Jul 2014 07:44:12 -0400 Jeff Layton <[email protected]>…

Quoted 12 lines · On Mon, 21 Jul 2014 17:02:54 +1000 NeilBrown <[email protected]> wrote:

fs/nfsd/nfs4state.c static void block_delegations(struct knfsd_fh *fh) · line 564

Quoted 6 lines

fs/nfsd/nfs4state.c +1 -0

 	hash = arch_fast_hash(&fh->fh_base, fh->fh_size, 0);
 
 	__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]);
+	spin_lock(&blocked_delegations_lock);

__set_bit isn't atomic. The spin_lock should be taken *before* these __set_bit() calls.

Otherwise, looks fine.

Quoted 7 lines

Quoted 4 lines

Thanks,
NeilBrown

Ok. I guess the worry is that we could end up setting bits in the middle of swapping the two fields? Makes sense -- fixed in my repo.

It is more subtle than that.
__set_bit() will:
read a value from memory to a register
set a bit in the register
write the register back out to memory

If two threads both run __set_bit on the same word of memory at the same time, one of the updates can get lost.
set_bit() (no underscore) performs an atomic RMW to avoid this, but is more expensive.
spin_lock() obviously ensures the required exclusion and as we are going to take the lock anyway we may as well take it before setting bits so we can use the non-atomic (cheaper) __set_bit function.

Quoted 3 lines

I'll send out the updated set later today (it also includes a few nits that HCH pointed out last week).

Quoted 7 lines

Quoted 2 lines

As a side note...I wonder how much we'll get in the way of false positives with this scheme?

If a future version of NFSv4 could allow delegations to be granted while a file is open (oh, it seems you are the only client using this file at the moment, you can treat this "open" as a delegation if you like) a few false positives would be a complete non-issue.

For what it's worth, I think 4.1 provides what you're asking for here; see

	http://tools.ietf.org/html/rfc5661#section-20.7

and the discussion of the various WANT_ flags in

	http://tools.ietf.org/html/rfc5661#section-18.16.3

As far as I know none of that is implemented yet.

Quoted 1 line

--b.

I guess I should really read the 4.1 (and 4.2) spec some day.... Though the 20.7 section seems to be about saying "resources in general are available" rather than "this specific file that you wanted a delegation for but didn't get one is how up for delegation"....
But I only had a quick read so I might have missed something.