* [PATCH 0/2] Fix two bugs CB_NOTIFY_LOCK failing to wake a water @ 2019-05-08 9:11 Yihao Wu 2019-05-08 9:13 ` [PATCH 1/2] NFSv4.1: Again fix a race where CB_NOTIFY_LOCK fails to wake a waiter Yihao Wu 0 siblings, 1 reply; 8+ messages in thread From: Yihao Wu @ 2019-05-08 9:11 UTC (permalink / raw) To: linux-nfs, Jeff Layton, J. Bruce Fields; +Cc: stable, Joseph Qi, caspar This patch set fix bugs related to CB_NOTIFY_LOCK. And it should also fix the failure when running xfstests generic/089 on a NFSv4.1 filesystem. Yihao Wu (2): NFSv4.1: Again fix a race where CB_NOTIFY_LOCK fails to wake a waiter NFSv4.1: Fix bug only the first CB_NOTIFY_LOCK is handled fs/nfs/nfs4proc.c | 23 ++++++++--------------- 1 file changed, 8 insertions(+), 15 deletions(-) -- 1.8.3.1 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 1/2] NFSv4.1: Again fix a race where CB_NOTIFY_LOCK fails to wake a waiter 2019-05-08 9:11 [PATCH 0/2] Fix two bugs CB_NOTIFY_LOCK failing to wake a water Yihao Wu @ 2019-05-08 9:13 ` Yihao Wu 2019-05-08 9:15 ` [PATCH 2/2] NFSv4.1: Fix bug only the first CB_NOTIFY_LOCK is handled Yihao Wu ` (2 more replies) 0 siblings, 3 replies; 8+ messages in thread From: Yihao Wu @ 2019-05-08 9:13 UTC (permalink / raw) To: linux-nfs, Jeff Layton, J. Bruce Fields; +Cc: stable, Joseph Qi, caspar Commit b7dbcc0e433f ""NFSv4.1: Fix a race where CB_NOTIFY_LOCK fails to wake a waiter" found this bug. However it didn't fix it. This can be fixed by adding memory barrier pair. Specifically, if any CB_NOTIFY_LOCK should be handled between unlocking the wait queue and freezable_schedule_timeout, only two cases are possible. So CB_NOTIFY_LOCK will not be dropped unexpectly. 1. The callback thread marks the NFS client as waked. Then NFS client noticed that itself is waked, so it don't goes to sleep. And it cleans its wake mark. 2. The NFS client noticed that itself is not waked yet, so it goes to sleep. No modification will ever happen to the wake mark in between. Fixes: a1d617d ("nfs: allow blocking locks to be awoken by lock callbacks") Signed-off-by: Yihao Wu <wuyihao@linux.alibaba.com> --- fs/nfs/nfs4proc.c | 21 +++++---------------- 1 file changed, 5 insertions(+), 16 deletions(-) diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c index 741ff8c..f13ea09 100644 --- a/fs/nfs/nfs4proc.c +++ b/fs/nfs/nfs4proc.c @@ -6867,7 +6867,6 @@ struct nfs4_lock_waiter { struct task_struct *task; struct inode *inode; struct nfs_lowner *owner; - bool notified; }; static int @@ -6889,13 +6888,13 @@ struct nfs4_lock_waiter { /* Make sure it's for the right inode */ if (nfs_compare_fh(NFS_FH(waiter->inode), &cbnl->cbnl_fh)) return 0; - - waiter->notified = true; } /* override "private" so we can use default_wake_function */ wait->private = waiter->task; - ret = autoremove_wake_function(wait, mode, flags, key); + ret = woken_wake_function(wait, mode, flags, key); + if (ret) + list_del_init(&wait->entry); wait->private = waiter; return ret; } @@ -6914,8 +6913,7 @@ struct nfs4_lock_waiter { .s_dev = server->s_dev }; struct nfs4_lock_waiter waiter = { .task = current, .inode = state->inode, - .owner = &owner, - .notified = false }; + .owner = &owner}; wait_queue_entry_t wait; /* Don't bother with waitqueue if we don't expect a callback */ @@ -6928,21 +6926,12 @@ struct nfs4_lock_waiter { add_wait_queue(q, &wait); while(!signalled()) { - waiter.notified = false; status = nfs4_proc_setlk(state, cmd, request); if ((status != -EAGAIN) || IS_SETLK(cmd)) break; status = -ERESTARTSYS; - spin_lock_irqsave(&q->lock, flags); - if (waiter.notified) { - spin_unlock_irqrestore(&q->lock, flags); - continue; - } - set_current_state(TASK_INTERRUPTIBLE); - spin_unlock_irqrestore(&q->lock, flags); - - freezable_schedule_timeout(NFS4_LOCK_MAXTIMEOUT); + wait_woken(&wait, TASK_INTERRUPTIBLE, NFS4_LOCK_MAXTIMEOUT); } finish_wait(q, &wait); -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 2/2] NFSv4.1: Fix bug only the first CB_NOTIFY_LOCK is handled 2019-05-08 9:13 ` [PATCH 1/2] NFSv4.1: Again fix a race where CB_NOTIFY_LOCK fails to wake a waiter Yihao Wu @ 2019-05-08 9:15 ` Yihao Wu 2019-05-08 9:23 ` Greg KH 2019-05-08 9:19 ` [PATCH 1/2] NFSv4.1: Again fix a race where CB_NOTIFY_LOCK fails to wake a waiter Greg KH 2019-05-08 12:24 ` Jeff Layton 2 siblings, 1 reply; 8+ messages in thread From: Yihao Wu @ 2019-05-08 9:15 UTC (permalink / raw) To: linux-nfs, Jeff Layton, J. Bruce Fields; +Cc: stable, Joseph Qi, caspar When the first CB_NOTIFY_LOCK wakes a waiter, it can still fail to acquire the lock. Then it might goes to sleep again. However it's removed from the wait queue and not put back. So when the CB_NOTIFY_LOCK comes again, it fails to wake this waiter. Signed-off-by: Yihao Wu <wuyihao@linux.alibaba.com> --- fs/nfs/nfs4proc.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c index f13ea09..9de2c2d 100644 --- a/fs/nfs/nfs4proc.c +++ b/fs/nfs/nfs4proc.c @@ -6932,6 +6932,10 @@ struct nfs4_lock_waiter { status = -ERESTARTSYS; wait_woken(&wait, TASK_INTERRUPTIBLE, NFS4_LOCK_MAXTIMEOUT); + if (!signalled()) { + finish_wait(q, &wait); + add_wait_queue(q, &wait); + } } finish_wait(q, &wait); -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] NFSv4.1: Fix bug only the first CB_NOTIFY_LOCK is handled 2019-05-08 9:15 ` [PATCH 2/2] NFSv4.1: Fix bug only the first CB_NOTIFY_LOCK is handled Yihao Wu @ 2019-05-08 9:23 ` Greg KH 0 siblings, 0 replies; 8+ messages in thread From: Greg KH @ 2019-05-08 9:23 UTC (permalink / raw) To: Yihao Wu Cc: linux-nfs, Jeff Layton, J. Bruce Fields, stable, Joseph Qi, caspar On Wed, May 08, 2019 at 05:15:03PM +0800, Yihao Wu wrote: > When the first CB_NOTIFY_LOCK wakes a waiter, it can still fail to > acquire the lock. Then it might goes to sleep again. However it's removed > from the wait queue and not put back. So when the CB_NOTIFY_LOCK comes > again, it fails to wake this waiter. > > Signed-off-by: Yihao Wu <wuyihao@linux.alibaba.com> > --- > fs/nfs/nfs4proc.c | 4 ++++ > 1 file changed, 4 insertions(+) <formletter> This is not the correct way to submit patches for inclusion in the stable kernel tree. Please read: https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html for how to do this properly. </formletter> ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] NFSv4.1: Again fix a race where CB_NOTIFY_LOCK fails to wake a waiter 2019-05-08 9:13 ` [PATCH 1/2] NFSv4.1: Again fix a race where CB_NOTIFY_LOCK fails to wake a waiter Yihao Wu 2019-05-08 9:15 ` [PATCH 2/2] NFSv4.1: Fix bug only the first CB_NOTIFY_LOCK is handled Yihao Wu @ 2019-05-08 9:19 ` Greg KH 2019-05-08 9:39 ` Yihao Wu 2019-05-08 12:24 ` Jeff Layton 2 siblings, 1 reply; 8+ messages in thread From: Greg KH @ 2019-05-08 9:19 UTC (permalink / raw) To: Yihao Wu Cc: linux-nfs, Jeff Layton, J. Bruce Fields, stable, Joseph Qi, caspar On Wed, May 08, 2019 at 05:13:25PM +0800, Yihao Wu wrote: > Commit b7dbcc0e433f ""NFSv4.1: Fix a race where CB_NOTIFY_LOCK fails > to wake a waiter" found this bug. However it didn't fix it. This can > be fixed by adding memory barrier pair. > > Specifically, if any CB_NOTIFY_LOCK should be handled between unlocking > the wait queue and freezable_schedule_timeout, only two cases are > possible. So CB_NOTIFY_LOCK will not be dropped unexpectly. > > 1. The callback thread marks the NFS client as waked. Then NFS client > noticed that itself is waked, so it don't goes to sleep. And it cleans > its wake mark. > > 2. The NFS client noticed that itself is not waked yet, so it goes to > sleep. No modification will ever happen to the wake mark in between. > > Fixes: a1d617d ("nfs: allow blocking locks to be awoken by lock callbacks") > Signed-off-by: Yihao Wu <wuyihao@linux.alibaba.com> > --- > fs/nfs/nfs4proc.c | 21 +++++---------------- > 1 file changed, 5 insertions(+), 16 deletions(-) <formletter> This is not the correct way to submit patches for inclusion in the stable kernel tree. Please read: https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html for how to do this properly. </formletter> ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] NFSv4.1: Again fix a race where CB_NOTIFY_LOCK fails to wake a waiter 2019-05-08 9:19 ` [PATCH 1/2] NFSv4.1: Again fix a race where CB_NOTIFY_LOCK fails to wake a waiter Greg KH @ 2019-05-08 9:39 ` Yihao Wu 0 siblings, 0 replies; 8+ messages in thread From: Yihao Wu @ 2019-05-08 9:39 UTC (permalink / raw) To: Greg KH; +Cc: linux-nfs, Jeff Layton, J. Bruce Fields, Joseph Qi, caspar On 2019/5/8 5:19 PM, Greg KH wrote: > On Wed, May 08, 2019 at 05:13:25PM +0800, Yihao Wu wrote: >> Commit b7dbcc0e433f ""NFSv4.1: Fix a race where CB_NOTIFY_LOCK fails >> to wake a waiter" found this bug. However it didn't fix it. This can >> be fixed by adding memory barrier pair. >> >> Specifically, if any CB_NOTIFY_LOCK should be handled between unlocking >> the wait queue and freezable_schedule_timeout, only two cases are >> possible. So CB_NOTIFY_LOCK will not be dropped unexpectly. >> >> 1. The callback thread marks the NFS client as waked. Then NFS client >> noticed that itself is waked, so it don't goes to sleep. And it cleans >> its wake mark. >> >> 2. The NFS client noticed that itself is not waked yet, so it goes to >> sleep. No modification will ever happen to the wake mark in between. >> >> Fixes: a1d617d ("nfs: allow blocking locks to be awoken by lock callbacks") >> Signed-off-by: Yihao Wu <wuyihao@linux.alibaba.com> >> --- >> fs/nfs/nfs4proc.c | 21 +++++---------------- >> 1 file changed, 5 insertions(+), 16 deletions(-) > > <formletter> > > This is not the correct way to submit patches for inclusion in the > stable kernel tree. Please read: > https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html > for how to do this properly. > > </formletter> > Thanks for your reply! And I'm sorry about that. I will correct this in patch v2 and read the rules before sending patches. Thanks, Yihao Wu ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] NFSv4.1: Again fix a race where CB_NOTIFY_LOCK fails to wake a waiter 2019-05-08 9:13 ` [PATCH 1/2] NFSv4.1: Again fix a race where CB_NOTIFY_LOCK fails to wake a waiter Yihao Wu 2019-05-08 9:15 ` [PATCH 2/2] NFSv4.1: Fix bug only the first CB_NOTIFY_LOCK is handled Yihao Wu 2019-05-08 9:19 ` [PATCH 1/2] NFSv4.1: Again fix a race where CB_NOTIFY_LOCK fails to wake a waiter Greg KH @ 2019-05-08 12:24 ` Jeff Layton 2019-05-08 18:18 ` Yihao Wu 2 siblings, 1 reply; 8+ messages in thread From: Jeff Layton @ 2019-05-08 12:24 UTC (permalink / raw) To: Yihao Wu, linux-nfs, J. Bruce Fields; +Cc: stable, Joseph Qi, caspar On Wed, 2019-05-08 at 17:13 +0800, Yihao Wu wrote: > Commit b7dbcc0e433f ""NFSv4.1: Fix a race where CB_NOTIFY_LOCK fails > to wake a waiter" found this bug. However it didn't fix it. This can > be fixed by adding memory barrier pair. > > Specifically, if any CB_NOTIFY_LOCK should be handled between unlocking > the wait queue and freezable_schedule_timeout, only two cases are > possible. So CB_NOTIFY_LOCK will not be dropped unexpectly. > > 1. The callback thread marks the NFS client as waked. Then NFS client > noticed that itself is waked, so it don't goes to sleep. And it cleans > its wake mark. > > 2. The NFS client noticed that itself is not waked yet, so it goes to > sleep. No modification will ever happen to the wake mark in between. > It's not clear to me what you mean by "wake mark" here. Do you mean the "notified" flag? This could use a better description. > Fixes: a1d617d ("nfs: allow blocking locks to be awoken by lock callbacks") > Signed-off-by: Yihao Wu <wuyihao@linux.alibaba.com> > --- > fs/nfs/nfs4proc.c | 21 +++++---------------- > 1 file changed, 5 insertions(+), 16 deletions(-) > > diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c > index 741ff8c..f13ea09 100644 > --- a/fs/nfs/nfs4proc.c > +++ b/fs/nfs/nfs4proc.c > @@ -6867,7 +6867,6 @@ struct nfs4_lock_waiter { > struct task_struct *task; > struct inode *inode; > struct nfs_lowner *owner; > - bool notified; > }; > > static int > @@ -6889,13 +6888,13 @@ struct nfs4_lock_waiter { > /* Make sure it's for the right inode */ > if (nfs_compare_fh(NFS_FH(waiter->inode), &cbnl->cbnl_fh)) > return 0; > - > - waiter->notified = true; > } > > /* override "private" so we can use default_wake_function */ > wait->private = waiter->task; > - ret = autoremove_wake_function(wait, mode, flags, key); > + ret = woken_wake_function(wait, mode, flags, key); > + if (ret) > + list_del_init(&wait->entry); > wait->private = waiter; > return ret; > } > @@ -6914,8 +6913,7 @@ struct nfs4_lock_waiter { > .s_dev = server->s_dev }; > struct nfs4_lock_waiter waiter = { .task = current, > .inode = state->inode, > - .owner = &owner, > - .notified = false }; > + .owner = &owner}; > wait_queue_entry_t wait; > > /* Don't bother with waitqueue if we don't expect a callback */ > @@ -6928,21 +6926,12 @@ struct nfs4_lock_waiter { > add_wait_queue(q, &wait); > > while(!signalled()) { > - waiter.notified = false; > status = nfs4_proc_setlk(state, cmd, request); > if ((status != -EAGAIN) || IS_SETLK(cmd)) > break; > > status = -ERESTARTSYS; > - spin_lock_irqsave(&q->lock, flags); > - if (waiter.notified) { > - spin_unlock_irqrestore(&q->lock, flags); > - continue; > - } > - set_current_state(TASK_INTERRUPTIBLE); > - spin_unlock_irqrestore(&q->lock, flags); > - > - freezable_schedule_timeout(NFS4_LOCK_MAXTIMEOUT); > + wait_woken(&wait, TASK_INTERRUPTIBLE, NFS4_LOCK_MAXTIMEOUT); This seems to have dropped the "freezable" part above, such that waiting on a file lock will prevent (e.g.) a laptop from suspending. I think that needs to be in here as those waits can be quite long. > } > > finish_wait(q, &wait); -- Jeff Layton <jlayton@kernel.org> ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] NFSv4.1: Again fix a race where CB_NOTIFY_LOCK fails to wake a waiter 2019-05-08 12:24 ` Jeff Layton @ 2019-05-08 18:18 ` Yihao Wu 0 siblings, 0 replies; 8+ messages in thread From: Yihao Wu @ 2019-05-08 18:18 UTC (permalink / raw) To: Jeff Layton, linux-nfs, J. Bruce Fields; +Cc: stable, Joseph Qi, caspar On 2019/5/8 8:24 PM, Jeff Layton wrote: > On Wed, 2019-05-08 at 17:13 +0800, Yihao Wu wrote: >> Commit b7dbcc0e433f ""NFSv4.1: Fix a race where CB_NOTIFY_LOCK fails >> to wake a waiter" found this bug. However it didn't fix it. This can >> be fixed by adding memory barrier pair. >> >> Specifically, if any CB_NOTIFY_LOCK should be handled between unlocking >> the wait queue and freezable_schedule_timeout, only two cases are >> possible. So CB_NOTIFY_LOCK will not be dropped unexpectly. >> >> 1. The callback thread marks the NFS client as waked. Then NFS client >> noticed that itself is waked, so it don't goes to sleep. And it cleans >> its wake mark. >> >> 2. The NFS client noticed that itself is not waked yet, so it goes to >> sleep. No modification will ever happen to the wake mark in between. >> > > It's not clear to me what you mean by "wake mark" here. Do you mean the > "notified" flag? This could use a better description. Yes. I mean "notified flag" by "wake mark". I will clear these ambiguities. Thanks > >> Fixes: a1d617d ("nfs: allow blocking locks to be awoken by lock callbacks") >> Signed-off-by: Yihao Wu <wuyihao@linux.alibaba.com> >> --- >> fs/nfs/nfs4proc.c | 21 +++++---------------- >> 1 file changed, 5 insertions(+), 16 deletions(-) >> >> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c >> index 741ff8c..f13ea09 100644 >> --- a/fs/nfs/nfs4proc.c >> +++ b/fs/nfs/nfs4proc.c >> @@ -6867,7 +6867,6 @@ struct nfs4_lock_waiter { >> struct task_struct *task; >> struct inode *inode; >> struct nfs_lowner *owner; >> - bool notified; >> }; >> >> static int >> @@ -6889,13 +6888,13 @@ struct nfs4_lock_waiter { >> /* Make sure it's for the right inode */ >> if (nfs_compare_fh(NFS_FH(waiter->inode), &cbnl->cbnl_fh)) >> return 0; >> - >> - waiter->notified = true; >> } >> >> /* override "private" so we can use default_wake_function */ >> wait->private = waiter->task; >> - ret = autoremove_wake_function(wait, mode, flags, key); >> + ret = woken_wake_function(wait, mode, flags, key); >> + if (ret) >> + list_del_init(&wait->entry); >> wait->private = waiter; >> return ret; >> } >> @@ -6914,8 +6913,7 @@ struct nfs4_lock_waiter { >> .s_dev = server->s_dev }; >> struct nfs4_lock_waiter waiter = { .task = current, >> .inode = state->inode, >> - .owner = &owner, >> - .notified = false }; >> + .owner = &owner}; >> wait_queue_entry_t wait; >> >> /* Don't bother with waitqueue if we don't expect a callback */ >> @@ -6928,21 +6926,12 @@ struct nfs4_lock_waiter { >> add_wait_queue(q, &wait); >> >> while(!signalled()) { >> - waiter.notified = false; >> status = nfs4_proc_setlk(state, cmd, request); >> if ((status != -EAGAIN) || IS_SETLK(cmd)) >> break; >> >> status = -ERESTARTSYS; >> - spin_lock_irqsave(&q->lock, flags); >> - if (waiter.notified) { >> - spin_unlock_irqrestore(&q->lock, flags); >> - continue; >> - } >> - set_current_state(TASK_INTERRUPTIBLE); >> - spin_unlock_irqrestore(&q->lock, flags); >> - >> - freezable_schedule_timeout(NFS4_LOCK_MAXTIMEOUT); >> + wait_woken(&wait, TASK_INTERRUPTIBLE, NFS4_LOCK_MAXTIMEOUT); > > This seems to have dropped the "freezable" part above, such that waiting > on a file lock will prevent (e.g.) a laptop from suspending. I think > that needs to be in here as those waits can be quite long. > You're right. I overlooked this. This will be fixed. Thanks >> } >> >> finish_wait(q, &wait); > ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2019-05-08 18:18 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-05-08 9:11 [PATCH 0/2] Fix two bugs CB_NOTIFY_LOCK failing to wake a water Yihao Wu 2019-05-08 9:13 ` [PATCH 1/2] NFSv4.1: Again fix a race where CB_NOTIFY_LOCK fails to wake a waiter Yihao Wu 2019-05-08 9:15 ` [PATCH 2/2] NFSv4.1: Fix bug only the first CB_NOTIFY_LOCK is handled Yihao Wu 2019-05-08 9:23 ` Greg KH 2019-05-08 9:19 ` [PATCH 1/2] NFSv4.1: Again fix a race where CB_NOTIFY_LOCK fails to wake a waiter Greg KH 2019-05-08 9:39 ` Yihao Wu 2019-05-08 12:24 ` Jeff Layton 2019-05-08 18:18 ` Yihao Wu
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).