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@xxxxxxxxxxxxxxxxx>
> ---
> 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@xxxxxxxxxx>
