nfsd: bump dp->dl_time when unhashing delegation
Subsystems (2)
PATCH 1/1 nfsd: bump dp->dl_time when unhashing delegation
There's a potential race between a lease break and DELEGRETURN call.
Suppose a lease break comes in and queues the workqueue job for a delegation, but it doesn't run just yet. Then, a DELEGRETURN comes in finds the delegation and calls destroy_delegation on it to unhash it and put its primary reference.
Next, the workqueue job runs and queues the delegation back onto the del_recall_lru list, issues the CB_RECALL and puts the final reference. With that, the final reference to the delegation is put, but it's still on the LRU list.
When we go to unhash a delegation, it's because we intend to get rid of it soon afterward, so we don't want lease breaks to mess with it once that occurs. Fix this by bumping the dl_time whenever we unhash a delegation, to ensure that lease breaks don't monkey with it.
--- fs/nfsd/nfs4state.c | 2 ++ 1 file changed, 2 insertions(+)
fs/nfsd/nfs4state.c +2 -0
@@ -660,6 +660,8 @@ unhash_delegation(struct nfs4_delegation *dp)spin_lock (&state_lock); dp->dl_stid.sc_type =NFS4_CLOSED_DELEG_STID ; +/* Ensure that deleg break won't try to requeue it */ + ++dp->dl_time;spin_lock (&fp->fi_lock);list_del_init (&dp->dl_perclnt);list_del_init (&dp->dl_recall_lru);
Signature · 1 line
1.9.3
Quoted 12 lines · Jeff Layton
There's a potential race between a lease break and DELEGRETURN call.
Suppose a lease break comes in and queues the workqueue job for a delegation, but it doesn't run just yet. Then, a DELEGRETURN comes in finds the delegation and calls
destroy_delegationon it to unhash it and put its primary reference.Next, the workqueue job runs and queues the delegation back onto the
del_recall_lrulist, issues theCB_RECALLand puts the final reference. With that, the final reference to the delegation is put, but it's still on the LRU list.
Quoted 4 lines
When we go to unhash a delegation, it's because we intend to get rid of it soon afterward, so we don't want lease breaks to mess with it once that occurs. Fix this by bumping the
dl_timewhenever we unhash a delegation, to ensure that lease breaks don't monkey with it.
Makes sense, thanks. Repeating from IRC: this fixes a regression from 02e1215f9f7 "nfsd: Avoid taking state_lock while holding inode lock in nfsd_break_one_deleg". (In my tree only.)
--b.
Quoted 22 lines
Signed-off-by Jeff Layton <[email protected]>--- fs/nfsd/nfs4state.c | 2 ++ 1 file changed, 2 insertions(+)fs/nfsd/nfs4state.c +2
-0@@ -660,6 +660,8 @@ unhash_delegation(struct nfs4_delegation *dp)spin_lock (&state_lock); dp->dl_stid.sc_type =NFS4_CLOSED_DELEG_STID ; +/* Ensure that deleg break won't try to requeue it */ + ++dp->dl_time;spin_lock (&fp->fi_lock);list_del_init (&dp->dl_perclnt);list_del_init (&dp->dl_recall_lru);Signature · 1 line
1.9.3
Quoted 36 lines · On Tue, 22 Jul 2014 13:45:52 -0400 "J. Bruce Fields" <[email protected]>…
Quoted 12 lines · Jeff Layton
There's a potential race between a lease break and DELEGRETURN call.
Suppose a lease break comes in and queues the workqueue job for a delegation, but it doesn't run just yet. Then, a DELEGRETURN comes in finds the delegation and calls
destroy_delegationon it to unhash it and put its primary reference.Next, the workqueue job runs and queues the delegation back onto the
del_recall_lrulist, issues theCB_RECALLand puts the final reference. With that, the final reference to the delegation is put, but it's still on the LRU list.Quoted 4 lines
When we go to unhash a delegation, it's because we intend to get rid of it soon afterward, so we don't want lease breaks to mess with it once that occurs. Fix this by bumping the
dl_timewhenever we unhash a delegation, to ensure that lease breaks don't monkey with it.Makes sense, thanks. Repeating from IRC: this fixes a regression from
02e1215f9f7"nfsd: Avoid takingstate_lockwhile holding inode lock innfsd_break_one_deleg". (In my tree only.)--b.
Quoted 12 lines
Signed-off-by Jeff Layton <[email protected]>--- fs/nfsd/nfs4state.c | 2 ++ 1 file changed, 2 insertions(+)fs/nfsd/nfs4state.c +0
-0@@ -660,6 +660,8 @@ unhash_delegation(struct nfs4_delegation *dp)
Quoted 10 lines
Quoted 10 lines
fs/nfsd/nfs4state.c +2
-0spin_lock (&state_lock); dp->dl_stid.sc_type =NFS4_CLOSED_DELEG_STID ; +/* Ensure that deleg break won't try to requeue it */ + ++dp->dl_time;spin_lock (&fp->fi_lock);list_del_init (&dp->dl_perclnt);list_del_init (&dp->dl_recall_lru);Signature · 1 line
1.9.3
Sorry, I think I sent you a version with an earlier description. Here's the one I meant to send:
--------------------[snip]---------------------
[PATCH] nfsd: bump dl_time when unhashing delegation
There's a potential race between a lease break and DELEGRETURN call.
Suppose a lease break comes in and queues the workqueue job for a delegation, but it doesn't run just yet. Then, a DELEGRETURN comes in finds the delegation and calls destroy_delegation on it to unhash it and put its primary reference.
Next, the workqueue job runs and queues the delegation back onto the del_recall_lru list, issues the CB_RECALL and puts the final reference. With that, the final reference to the delegation is put, but it's still on the LRU list.
When we go to unhash a delegation, it's because we intend to get rid of it soon afterward, so we don't want lease breaks to mess with it once that occurs. Fix this by bumping the dl_time whenever we unhash a delegation, to ensure that lease breaks don't monkey with it.
I believe this is a regression due to commit 02e1215f9f7 (nfsd: Avoid taking state_lock while holding inode lock in nfsd_break_one_deleg). Prior to that, the state_lock was held in the lm_break callback itself, and that would have prevented this race.
--- fs/nfsd/nfs4state.c | 2 ++ 1 file changed, 2 insertions(+)
fs/nfsd/nfs4state.c +2 -0
@@ -660,6 +660,8 @@ unhash_delegation(struct nfs4_delegation *dp)spin_lock (&state_lock); dp->dl_stid.sc_type =NFS4_CLOSED_DELEG_STID ; +/* Ensure that deleg break won't try to requeue it */ + ++dp->dl_time;spin_lock (&fp->fi_lock);list_del_init (&dp->dl_perclnt);list_del_init (&dp->dl_recall_lru);
Signature · 1 line
1.9.3
Quoted 39 lines · Jeff Layton
Quoted 36 lines · On Tue, 22 Jul 2014 13:45:52 -0400 "J. Bruce Fields" <[email protected]>…
Quoted 12 lines · Jeff Layton
There's a potential race between a lease break and DELEGRETURN call.
Suppose a lease break comes in and queues the workqueue job for a delegation, but it doesn't run just yet. Then, a DELEGRETURN comes in finds the delegation and calls
destroy_delegationon it to unhash it and put its primary reference.Next, the workqueue job runs and queues the delegation back onto the
del_recall_lrulist, issues theCB_RECALLand puts the final reference. With that, the final reference to the delegation is put, but it's still on the LRU list.Quoted 4 lines
When we go to unhash a delegation, it's because we intend to get rid of it soon afterward, so we don't want lease breaks to mess with it once that occurs. Fix this by bumping the
dl_timewhenever we unhash a delegation, to ensure that lease breaks don't monkey with it.Makes sense, thanks. Repeating from IRC: this fixes a regression from
02e1215f9f7"nfsd: Avoid takingstate_lockwhile holding inode lock innfsd_break_one_deleg". (In my tree only.)--b.
Quoted 12 lines
Signed-off-by Jeff Layton <[email protected]>--- fs/nfsd/nfs4state.c | 2 ++ 1 file changed, 2 insertions(+)fs/nfsd/nfs4state.c +0
-0@@ -660,6 +660,8 @@ unhash_delegation(struct nfs4_delegation *dp)
Quoted 13 lines
Quoted 10 lines
Quoted 10 lines
fs/nfsd/nfs4state.c +2
-0spin_lock (&state_lock); dp->dl_stid.sc_type =NFS4_CLOSED_DELEG_STID ; +/* Ensure that deleg break won't try to requeue it */ + ++dp->dl_time;spin_lock (&fp->fi_lock);list_del_init (&dp->dl_perclnt);list_del_init (&dp->dl_recall_lru);Signature · 1 line
1.9.3Sorry, I think I sent you a version with an earlier description. Here's the one I meant to send:
Thanks, applying this version.--b.
Quoted 53 lines
--------------------[snip]---------------------
[PATCH] nfsd: bump
dl_timewhen unhashing delegationThere's a potential race between a lease break and DELEGRETURN call.
Suppose a lease break comes in and queues the workqueue job for a delegation, but it doesn't run just yet. Then, a DELEGRETURN comes in finds the delegation and calls
destroy_delegationon it to unhash it and put its primary reference.Next, the workqueue job runs and queues the delegation back onto the
del_recall_lrulist, issues theCB_RECALLand puts the final reference. With that, the final reference to the delegation is put, but it's still on the LRU list.When we go to unhash a delegation, it's because we intend to get rid of it soon afterward, so we don't want lease breaks to mess with it once that occurs. Fix this by bumping the
dl_timewhenever we unhash a delegation, to ensure that lease breaks don't monkey with it.I believe this is a regression due to commit
02e1215f9f7(nfsd: Avoid takingstate_lockwhile holding inode lock innfsd_break_one_deleg). Prior to that, thestate_lockwas held in thelm_breakcallback itself, and that would have prevented this race.Cc Trond Myklebust <[email protected]>Signed-off-by Jeff Layton <[email protected]>--- fs/nfsd/nfs4state.c | 2 ++ 1 file changed, 2 insertions(+)fs/nfsd/nfs4state.c +2
-0@@ -660,6 +660,8 @@ unhash_delegation(struct nfs4_delegation *dp)spin_lock (&state_lock); dp->dl_stid.sc_type =NFS4_CLOSED_DELEG_STID ; +/* Ensure that deleg break won't try to requeue it */ + ++dp->dl_time;spin_lock (&fp->fi_lock);list_del_init (&dp->dl_perclnt);list_del_init (&dp->dl_recall_lru);Signature · 6 lines
1.9.3 -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html