linux-nfs.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/2] nfsd: ensure we wake file lock waiters before deleting blocked lock
@ 2019-04-22 16:34 Jeff Layton
  2019-04-22 16:34 ` [PATCH v2 1/2] nfsd: wake waiters blocked on file_lock before deleting it Jeff Layton
  2019-04-22 16:34 ` [PATCH v2 2/2] nfsd: wake blocked file lock waiters before sending callback Jeff Layton
  0 siblings, 2 replies; 13+ messages in thread
From: Jeff Layton @ 2019-04-22 16:34 UTC (permalink / raw)
  To: bfields; +Cc: slawek1211, neilb, linux-nfs

This is a respin of the patch that I sent over the weekend. It's a bit
more obvious to just call locks_delete_block from free_blocked_lock,
and that allows us to remove that call from some other sites.

Still though, I think we probably do want to wake blocked lock requests
prior to sending a callback to ensure that they can make progress when
the callback isn't getting a reply. The second patch does this.

Jeff Layton (2):
  nfsd: wake waiters blocked on file_lock before deleting it
  nfsd: wake blocked file lock waiters before sending callback

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

-- 
2.20.1


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH v2 1/2] nfsd: wake waiters blocked on file_lock before deleting it
  2019-04-22 16:34 [PATCH v2 0/2] nfsd: ensure we wake file lock waiters before deleting blocked lock Jeff Layton
@ 2019-04-22 16:34 ` Jeff Layton
  2019-04-22 23:47   ` NeilBrown
  2019-04-22 16:34 ` [PATCH v2 2/2] nfsd: wake blocked file lock waiters before sending callback Jeff Layton
  1 sibling, 1 reply; 13+ messages in thread
From: Jeff Layton @ 2019-04-22 16:34 UTC (permalink / raw)
  To: bfields; +Cc: slawek1211, neilb, linux-nfs

After a blocked nfsd file_lock request is deleted, knfsd will send a
callback to the client and then free the request. Commit 16306a61d3b7
("fs/locks: always delete_block after waiting.") changed it such that
locks_delete_block is always called on a request after it is awoken,
but that patch missed fixing up blocked nfsd request handling.

Call locks_delete_block on the block to wake up any locks still blocked
on the nfsd lock request before freeing it. Some of its callers already
do this however, so just remove those calls.

URL: https://bugzilla.kernel.org/show_bug.cgi?id=203363
Fixes: 16306a61d3b7 ("fs/locks: always delete_block after waiting.")
Reported-by: Slawomir Pryczek <slawek1211@gmail.com>
Cc: Neil Brown <neilb@suse.com>
Cc: stable@vger.kernel.org
Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/nfsd/nfs4state.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 6a45fb00c5fc..e87e15df2044 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -265,6 +265,7 @@ find_or_allocate_block(struct nfs4_lockowner *lo, struct knfsd_fh *fh,
 static void
 free_blocked_lock(struct nfsd4_blocked_lock *nbl)
 {
+	locks_delete_block(&nbl->nbl_lock);
 	locks_release_private(&nbl->nbl_lock);
 	kfree(nbl);
 }
@@ -293,7 +294,6 @@ remove_blocked_locks(struct nfs4_lockowner *lo)
 		nbl = list_first_entry(&reaplist, struct nfsd4_blocked_lock,
 					nbl_lru);
 		list_del_init(&nbl->nbl_lru);
-		locks_delete_block(&nbl->nbl_lock);
 		free_blocked_lock(nbl);
 	}
 }
@@ -4863,7 +4863,6 @@ nfs4_laundromat(struct nfsd_net *nn)
 		nbl = list_first_entry(&reaplist,
 					struct nfsd4_blocked_lock, nbl_lru);
 		list_del_init(&nbl->nbl_lru);
-		locks_delete_block(&nbl->nbl_lock);
 		free_blocked_lock(nbl);
 	}
 out:
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v2 2/2] nfsd: wake blocked file lock waiters before sending callback
  2019-04-22 16:34 [PATCH v2 0/2] nfsd: ensure we wake file lock waiters before deleting blocked lock Jeff Layton
  2019-04-22 16:34 ` [PATCH v2 1/2] nfsd: wake waiters blocked on file_lock before deleting it Jeff Layton
@ 2019-04-22 16:34 ` Jeff Layton
  2019-04-22 19:46   ` J. Bruce Fields
  1 sibling, 1 reply; 13+ messages in thread
From: Jeff Layton @ 2019-04-22 16:34 UTC (permalink / raw)
  To: bfields; +Cc: slawek1211, neilb, linux-nfs

When a blocked NFS lock is "awoken" we send a callback to the server and
then wake any hosts waiting on it. If a client attempts to get a lock
and then drops off the net, we could end up waiting for a long time
until we end up waking locks blocked on that request.

Add a new "prepare" phase for CB_NOTIFY_LOCK callbacks, and have it
call locks_delete_block to wake any lock requests waiting on the
one for which we're sending the callback before it is sent.

URL: https://bugzilla.kernel.org/show_bug.cgi?id=203363
Fixes: 16306a61d3b7 ("fs/locks: always delete_block after waiting.")
Reported-by: Slawomir Pryczek <slawek1211@gmail.com>
Cc: Neil Brown <neilb@suse.com>
Cc: stable@vger.kernel.org
Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/nfsd/nfs4state.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index e87e15df2044..f056b1d3fecd 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -298,6 +298,14 @@ remove_blocked_locks(struct nfs4_lockowner *lo)
 	}
 }
 
+static void
+nfsd4_cb_notify_lock_prepare(struct nfsd4_callback *cb)
+{
+	struct nfsd4_blocked_lock	*nbl = container_of(cb,
+						struct nfsd4_blocked_lock, nbl_cb);
+	locks_delete_block(&nbl->nbl_lock);
+}
+
 static int
 nfsd4_cb_notify_lock_done(struct nfsd4_callback *cb, struct rpc_task *task)
 {
@@ -325,6 +333,7 @@ nfsd4_cb_notify_lock_release(struct nfsd4_callback *cb)
 }
 
 static const struct nfsd4_callback_ops nfsd4_cb_notify_lock_ops = {
+	.prepare	= nfsd4_cb_notify_lock_prepare,
 	.done		= nfsd4_cb_notify_lock_done,
 	.release	= nfsd4_cb_notify_lock_release,
 };
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 2/2] nfsd: wake blocked file lock waiters before sending callback
  2019-04-22 16:34 ` [PATCH v2 2/2] nfsd: wake blocked file lock waiters before sending callback Jeff Layton
@ 2019-04-22 19:46   ` J. Bruce Fields
  0 siblings, 0 replies; 13+ messages in thread
From: J. Bruce Fields @ 2019-04-22 19:46 UTC (permalink / raw)
  To: Jeff Layton; +Cc: slawek1211, neilb, linux-nfs

On Mon, Apr 22, 2019 at 12:34:24PM -0400, Jeff Layton wrote:
> When a blocked NFS lock is "awoken" we send a callback to the server and
> then wake any hosts waiting on it. If a client attempts to get a lock
> and then drops off the net, we could end up waiting for a long time
> until we end up waking locks blocked on that request.
> 
> Add a new "prepare" phase for CB_NOTIFY_LOCK callbacks, and have it
> call locks_delete_block to wake any lock requests waiting on the
> one for which we're sending the callback before it is sent.

How about this for the wording of that last sentence:

	So, wake any other waiting lock requests before sending the
	callback.  Do this by calling locks_delete_block in a new
	"prepare" phase for CB_NOTIFY_LOCK callbacks.

Come to think of that, the second sentence is redundant with the code; I
think I'll just go with the first.

Committing with that.

Thanks for working this out, and thanks to Slawomir Pryczek for the
reproducer.

--b.

> 
> URL: https://bugzilla.kernel.org/show_bug.cgi?id=203363
> Fixes: 16306a61d3b7 ("fs/locks: always delete_block after waiting.")
> Reported-by: Slawomir Pryczek <slawek1211@gmail.com>
> Cc: Neil Brown <neilb@suse.com>
> Cc: stable@vger.kernel.org
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
>  fs/nfsd/nfs4state.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index e87e15df2044..f056b1d3fecd 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -298,6 +298,14 @@ remove_blocked_locks(struct nfs4_lockowner *lo)
>  	}
>  }
>  
> +static void
> +nfsd4_cb_notify_lock_prepare(struct nfsd4_callback *cb)
> +{
> +	struct nfsd4_blocked_lock	*nbl = container_of(cb,
> +						struct nfsd4_blocked_lock, nbl_cb);
> +	locks_delete_block(&nbl->nbl_lock);
> +}
> +
>  static int
>  nfsd4_cb_notify_lock_done(struct nfsd4_callback *cb, struct rpc_task *task)
>  {
> @@ -325,6 +333,7 @@ nfsd4_cb_notify_lock_release(struct nfsd4_callback *cb)
>  }
>  
>  static const struct nfsd4_callback_ops nfsd4_cb_notify_lock_ops = {
> +	.prepare	= nfsd4_cb_notify_lock_prepare,
>  	.done		= nfsd4_cb_notify_lock_done,
>  	.release	= nfsd4_cb_notify_lock_release,
>  };
> -- 
> 2.20.1

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 1/2] nfsd: wake waiters blocked on file_lock before deleting it
  2019-04-22 16:34 ` [PATCH v2 1/2] nfsd: wake waiters blocked on file_lock before deleting it Jeff Layton
@ 2019-04-22 23:47   ` NeilBrown
  2019-04-23 10:57     ` Jeff Layton
  2019-04-24 13:58     ` [PATCH v2 1/2] nfsd: wake waiters blocked on file_lock before deleting it J. Bruce Fields
  0 siblings, 2 replies; 13+ messages in thread
From: NeilBrown @ 2019-04-22 23:47 UTC (permalink / raw)
  To: Jeff Layton, bfields; +Cc: slawek1211, linux-nfs

[-- Attachment #1: Type: text/plain, Size: 3105 bytes --]

On Mon, Apr 22 2019, Jeff Layton wrote:

> After a blocked nfsd file_lock request is deleted, knfsd will send a
> callback to the client and then free the request. Commit 16306a61d3b7
> ("fs/locks: always delete_block after waiting.") changed it such that
> locks_delete_block is always called on a request after it is awoken,
> but that patch missed fixing up blocked nfsd request handling.
>
> Call locks_delete_block on the block to wake up any locks still blocked
> on the nfsd lock request before freeing it. Some of its callers already
> do this however, so just remove those calls.
>
> URL: https://bugzilla.kernel.org/show_bug.cgi?id=203363
> Fixes: 16306a61d3b7 ("fs/locks: always delete_block after waiting.")
> Reported-by: Slawomir Pryczek <slawek1211@gmail.com>
> Cc: Neil Brown <neilb@suse.com>
> Cc: stable@vger.kernel.org
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
>  fs/nfsd/nfs4state.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 6a45fb00c5fc..e87e15df2044 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -265,6 +265,7 @@ find_or_allocate_block(struct nfs4_lockowner *lo, struct knfsd_fh *fh,
>  static void
>  free_blocked_lock(struct nfsd4_blocked_lock *nbl)
>  {
> +	locks_delete_block(&nbl->nbl_lock);
>  	locks_release_private(&nbl->nbl_lock);

Thanks for tracking this down.

An implication of this bug and fix is that we need to be particularly
careful to make sure locks_delete_block() is called on all relevant
paths.
Can we make that easier?  My first thought was to include the call in
locks_release_private, but lockd calls the two quite separately and it
certainly seems appropriate that locks_delete_block should be called
asap, but locks_release_private() can be delayed.

Also cifs calls locks_delete_block, but never calls
locks_release_private, so it wouldn't help there.

Looking at cifs, I think there is a call missing there too.
cifs_posix_lock_set() *doesn't* always call locks_delete_block() after
waiting.  In particular, if ->can_cache_brlcks becomes true while
waiting then I don't think the behaviour is right.... though I'm not
sure it is right for other reasons.  It looks like the return value
should be 1 in that case, but it'll be zero.

But back to my question about making it easier, move the BUG_ON()
calls from locks_free_lock() into locks_release_private().

??

Thanks,
NeilBrown


>  	kfree(nbl);
>  }
> @@ -293,7 +294,6 @@ remove_blocked_locks(struct nfs4_lockowner *lo)
>  		nbl = list_first_entry(&reaplist, struct nfsd4_blocked_lock,
>  					nbl_lru);
>  		list_del_init(&nbl->nbl_lru);
> -		locks_delete_block(&nbl->nbl_lock);
>  		free_blocked_lock(nbl);
>  	}
>  }
> @@ -4863,7 +4863,6 @@ nfs4_laundromat(struct nfsd_net *nn)
>  		nbl = list_first_entry(&reaplist,
>  					struct nfsd4_blocked_lock, nbl_lru);
>  		list_del_init(&nbl->nbl_lru);
> -		locks_delete_block(&nbl->nbl_lock);
>  		free_blocked_lock(nbl);
>  	}
>  out:
> -- 
> 2.20.1

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 1/2] nfsd: wake waiters blocked on file_lock before deleting it
  2019-04-22 23:47   ` NeilBrown
@ 2019-04-23 10:57     ` Jeff Layton
  2019-04-24  2:00       ` [PATCH] locks: move checks from locks_free_lock() to locks_release_private() NeilBrown
  2019-04-24 13:58     ` [PATCH v2 1/2] nfsd: wake waiters blocked on file_lock before deleting it J. Bruce Fields
  1 sibling, 1 reply; 13+ messages in thread
From: Jeff Layton @ 2019-04-23 10:57 UTC (permalink / raw)
  To: NeilBrown; +Cc: Bruce Fields, slawek1211, open list:NFS, SUNRPC, AND...

On Mon, Apr 22, 2019 at 7:47 PM NeilBrown <neilb@suse.com> wrote:
>
> On Mon, Apr 22 2019, Jeff Layton wrote:
>
> > After a blocked nfsd file_lock request is deleted, knfsd will send a
> > callback to the client and then free the request. Commit 16306a61d3b7
> > ("fs/locks: always delete_block after waiting.") changed it such that
> > locks_delete_block is always called on a request after it is awoken,
> > but that patch missed fixing up blocked nfsd request handling.
> >
> > Call locks_delete_block on the block to wake up any locks still blocked
> > on the nfsd lock request before freeing it. Some of its callers already
> > do this however, so just remove those calls.
> >
> > URL: https://bugzilla.kernel.org/show_bug.cgi?id=203363
> > Fixes: 16306a61d3b7 ("fs/locks: always delete_block after waiting.")
> > Reported-by: Slawomir Pryczek <slawek1211@gmail.com>
> > Cc: Neil Brown <neilb@suse.com>
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > ---
> >  fs/nfsd/nfs4state.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> >
> > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > index 6a45fb00c5fc..e87e15df2044 100644
> > --- a/fs/nfsd/nfs4state.c
> > +++ b/fs/nfsd/nfs4state.c
> > @@ -265,6 +265,7 @@ find_or_allocate_block(struct nfs4_lockowner *lo, struct knfsd_fh *fh,
> >  static void
> >  free_blocked_lock(struct nfsd4_blocked_lock *nbl)
> >  {
> > +     locks_delete_block(&nbl->nbl_lock);
> >       locks_release_private(&nbl->nbl_lock);
>
> Thanks for tracking this down.
>
> An implication of this bug and fix is that we need to be particularly
> careful to make sure locks_delete_block() is called on all relevant
> paths.
> Can we make that easier?  My first thought was to include the call in
> locks_release_private, but lockd calls the two quite separately and it
> certainly seems appropriate that locks_delete_block should be called
> asap, but locks_release_private() can be delayed.
>
> Also cifs calls locks_delete_block, but never calls
> locks_release_private, so it wouldn't help there.
>
> Looking at cifs, I think there is a call missing there too.
> cifs_posix_lock_set() *doesn't* always call locks_delete_block() after
> waiting.  In particular, if ->can_cache_brlcks becomes true while
> waiting then I don't think the behaviour is right.... though I'm not
> sure it is right for other reasons.  It looks like the return value
> should be 1 in that case, but it'll be zero.
>
> But back to my question about making it easier, move the BUG_ON()
> calls from locks_free_lock() into locks_release_private().
>
> ??
>

That sounds like a fine idea. I was thinking about putting all of the
BUG_ONs there in a separate function that both spots could call, but
moving them into locks_release_private should work just as well. Care
to propose a patch?
--
Jeff Layton <jlayton@kernel.org>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH] locks: move checks from locks_free_lock() to locks_release_private()
  2019-04-23 10:57     ` Jeff Layton
@ 2019-04-24  2:00       ` NeilBrown
  2019-04-24 13:47         ` Jeff Layton
  0 siblings, 1 reply; 13+ messages in thread
From: NeilBrown @ 2019-04-24  2:00 UTC (permalink / raw)
  To: Jeff Layton; +Cc: Bruce Fields, slawek1211, open list:NFS, SUNRPC, AND...

[-- Attachment #1: Type: text/plain, Size: 1784 bytes --]


Code that allocates locks using locks_alloc_lock() will free it
using locks_free_lock(), and will benefit from the BUG_ON()
consistency checks therein.

However some code (nfsd and lockd) allocate a lock embedded in
some other data structure, and so free the lock themselves after
calling locks_release_private().  This path does not benefit from
the consistency checks.

To help catch future errors, move the BUG_ON() checks to
locks_release_private() - which locks_free_lock() already calls.
This ensures that all users for locks will find out if the lock
isn't detached properly before being free.

Signed-off-by: NeilBrown <neilb@suse.com>
---
 fs/locks.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/fs/locks.c b/fs/locks.c
index 71d0c6c2aac5..456a3782c6ca 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -352,6 +352,12 @@ EXPORT_SYMBOL_GPL(locks_alloc_lock);
 
 void locks_release_private(struct file_lock *fl)
 {
+	BUG_ON(waitqueue_active(&fl->fl_wait));
+	BUG_ON(!list_empty(&fl->fl_list));
+	BUG_ON(!list_empty(&fl->fl_blocked_requests));
+	BUG_ON(!list_empty(&fl->fl_blocked_member));
+	BUG_ON(!hlist_unhashed(&fl->fl_link));
+
 	if (fl->fl_ops) {
 		if (fl->fl_ops->fl_release_private)
 			fl->fl_ops->fl_release_private(fl);
@@ -371,12 +377,6 @@ EXPORT_SYMBOL_GPL(locks_release_private);
 /* Free a lock which is not in use. */
 void locks_free_lock(struct file_lock *fl)
 {
-	BUG_ON(waitqueue_active(&fl->fl_wait));
-	BUG_ON(!list_empty(&fl->fl_list));
-	BUG_ON(!list_empty(&fl->fl_blocked_requests));
-	BUG_ON(!list_empty(&fl->fl_blocked_member));
-	BUG_ON(!hlist_unhashed(&fl->fl_link));
-
 	locks_release_private(fl);
 	kmem_cache_free(filelock_cache, fl);
 }
-- 
2.14.0.rc0.dirty


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH] locks: move checks from locks_free_lock() to locks_release_private()
  2019-04-24  2:00       ` [PATCH] locks: move checks from locks_free_lock() to locks_release_private() NeilBrown
@ 2019-04-24 13:47         ` Jeff Layton
  2019-04-24 13:55           ` Bruce Fields
  0 siblings, 1 reply; 13+ messages in thread
From: Jeff Layton @ 2019-04-24 13:47 UTC (permalink / raw)
  To: NeilBrown; +Cc: Bruce Fields, slawek1211, open list:NFS, SUNRPC, AND...

On Tue, Apr 23, 2019 at 10:00 PM NeilBrown <neilb@suse.com> wrote:
>
>
> Code that allocates locks using locks_alloc_lock() will free it
> using locks_free_lock(), and will benefit from the BUG_ON()
> consistency checks therein.
>
> However some code (nfsd and lockd) allocate a lock embedded in
> some other data structure, and so free the lock themselves after
> calling locks_release_private().  This path does not benefit from
> the consistency checks.
>
> To help catch future errors, move the BUG_ON() checks to
> locks_release_private() - which locks_free_lock() already calls.
> This ensures that all users for locks will find out if the lock
> isn't detached properly before being free.
>
> Signed-off-by: NeilBrown <neilb@suse.com>
> ---
>  fs/locks.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/fs/locks.c b/fs/locks.c
> index 71d0c6c2aac5..456a3782c6ca 100644
> --- a/fs/locks.c
> +++ b/fs/locks.c
> @@ -352,6 +352,12 @@ EXPORT_SYMBOL_GPL(locks_alloc_lock);
>
>  void locks_release_private(struct file_lock *fl)
>  {
> +       BUG_ON(waitqueue_active(&fl->fl_wait));
> +       BUG_ON(!list_empty(&fl->fl_list));
> +       BUG_ON(!list_empty(&fl->fl_blocked_requests));
> +       BUG_ON(!list_empty(&fl->fl_blocked_member));
> +       BUG_ON(!hlist_unhashed(&fl->fl_link));
> +
>         if (fl->fl_ops) {
>                 if (fl->fl_ops->fl_release_private)
>                         fl->fl_ops->fl_release_private(fl);
> @@ -371,12 +377,6 @@ EXPORT_SYMBOL_GPL(locks_release_private);
>  /* Free a lock which is not in use. */
>  void locks_free_lock(struct file_lock *fl)
>  {
> -       BUG_ON(waitqueue_active(&fl->fl_wait));
> -       BUG_ON(!list_empty(&fl->fl_list));
> -       BUG_ON(!list_empty(&fl->fl_blocked_requests));
> -       BUG_ON(!list_empty(&fl->fl_blocked_member));
> -       BUG_ON(!hlist_unhashed(&fl->fl_link));
> -
>         locks_release_private(fl);
>         kmem_cache_free(filelock_cache, fl);
>  }
> --
> 2.14.0.rc0.dirty
>

Looks good to me. Bruce, would you mind picking this up for the next
merge window? I don't have any other file locking patches queued up
for v5.2, and I hate to send Linus a PR for just this.

Either way:

Reviewed-by: Jeff Layton <jlayton@kernel.org>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] locks: move checks from locks_free_lock() to locks_release_private()
  2019-04-24 13:47         ` Jeff Layton
@ 2019-04-24 13:55           ` Bruce Fields
  0 siblings, 0 replies; 13+ messages in thread
From: Bruce Fields @ 2019-04-24 13:55 UTC (permalink / raw)
  To: Jeff Layton; +Cc: NeilBrown, slawek1211, open list:NFS, SUNRPC, AND...

On Wed, Apr 24, 2019 at 09:47:59AM -0400, Jeff Layton wrote:
> Looks good to me. Bruce, would you mind picking this up for the next
> merge window? I don't have any other file locking patches queued up
> for v5.2, and I hate to send Linus a PR for just this.
> 
> Either way:
> 
> Reviewed-by: Jeff Layton <jlayton@kernel.org>

Got it, thanks.--b.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 1/2] nfsd: wake waiters blocked on file_lock before deleting it
  2019-04-22 23:47   ` NeilBrown
  2019-04-23 10:57     ` Jeff Layton
@ 2019-04-24 13:58     ` J. Bruce Fields
  2019-04-24 15:29       ` Steve Dickson
  1 sibling, 1 reply; 13+ messages in thread
From: J. Bruce Fields @ 2019-04-24 13:58 UTC (permalink / raw)
  To: NeilBrown; +Cc: Jeff Layton, slawek1211, linux-nfs, linux-cifs, Steve French

Steve, see Neil's comment, is there a cifs bug here?

--b.

On Tue, Apr 23, 2019 at 09:47:06AM +1000, NeilBrown wrote:
> On Mon, Apr 22 2019, Jeff Layton wrote:
> 
> > After a blocked nfsd file_lock request is deleted, knfsd will send a
> > callback to the client and then free the request. Commit 16306a61d3b7
> > ("fs/locks: always delete_block after waiting.") changed it such that
> > locks_delete_block is always called on a request after it is awoken,
> > but that patch missed fixing up blocked nfsd request handling.
> >
> > Call locks_delete_block on the block to wake up any locks still blocked
> > on the nfsd lock request before freeing it. Some of its callers already
> > do this however, so just remove those calls.
> >
> > URL: https://bugzilla.kernel.org/show_bug.cgi?id=203363
> > Fixes: 16306a61d3b7 ("fs/locks: always delete_block after waiting.")
> > Reported-by: Slawomir Pryczek <slawek1211@gmail.com>
> > Cc: Neil Brown <neilb@suse.com>
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > ---
> >  fs/nfsd/nfs4state.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> >
> > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > index 6a45fb00c5fc..e87e15df2044 100644
> > --- a/fs/nfsd/nfs4state.c
> > +++ b/fs/nfsd/nfs4state.c
> > @@ -265,6 +265,7 @@ find_or_allocate_block(struct nfs4_lockowner *lo, struct knfsd_fh *fh,
> >  static void
> >  free_blocked_lock(struct nfsd4_blocked_lock *nbl)
> >  {
> > +	locks_delete_block(&nbl->nbl_lock);
> >  	locks_release_private(&nbl->nbl_lock);
> 
> Thanks for tracking this down.
> 
> An implication of this bug and fix is that we need to be particularly
> careful to make sure locks_delete_block() is called on all relevant
> paths.
> Can we make that easier?  My first thought was to include the call in
> locks_release_private, but lockd calls the two quite separately and it
> certainly seems appropriate that locks_delete_block should be called
> asap, but locks_release_private() can be delayed.
> 
> Also cifs calls locks_delete_block, but never calls
> locks_release_private, so it wouldn't help there.
> 
> Looking at cifs, I think there is a call missing there too.
> cifs_posix_lock_set() *doesn't* always call locks_delete_block() after
> waiting.  In particular, if ->can_cache_brlcks becomes true while
> waiting then I don't think the behaviour is right.... though I'm not
> sure it is right for other reasons.  It looks like the return value
> should be 1 in that case, but it'll be zero.
> 
> But back to my question about making it easier, move the BUG_ON()
> calls from locks_free_lock() into locks_release_private().
> 
> ??
> 
> Thanks,
> NeilBrown
> 
> 
> >  	kfree(nbl);
> >  }
> > @@ -293,7 +294,6 @@ remove_blocked_locks(struct nfs4_lockowner *lo)
> >  		nbl = list_first_entry(&reaplist, struct nfsd4_blocked_lock,
> >  					nbl_lru);
> >  		list_del_init(&nbl->nbl_lru);
> > -		locks_delete_block(&nbl->nbl_lock);
> >  		free_blocked_lock(nbl);
> >  	}
> >  }
> > @@ -4863,7 +4863,6 @@ nfs4_laundromat(struct nfsd_net *nn)
> >  		nbl = list_first_entry(&reaplist,
> >  					struct nfsd4_blocked_lock, nbl_lru);
> >  		list_del_init(&nbl->nbl_lru);
> > -		locks_delete_block(&nbl->nbl_lock);
> >  		free_blocked_lock(nbl);
> >  	}
> >  out:
> > -- 
> > 2.20.1



^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 1/2] nfsd: wake waiters blocked on file_lock before deleting it
  2019-04-24 13:58     ` [PATCH v2 1/2] nfsd: wake waiters blocked on file_lock before deleting it J. Bruce Fields
@ 2019-04-24 15:29       ` Steve Dickson
  2019-04-24 15:47         ` J. Bruce Fields
  0 siblings, 1 reply; 13+ messages in thread
From: Steve Dickson @ 2019-04-24 15:29 UTC (permalink / raw)
  To: J. Bruce Fields, NeilBrown
  Cc: Jeff Layton, slawek1211, linux-nfs, linux-cifs, Steve French



On 4/24/19 9:58 AM, J. Bruce Fields wrote:
> Steve, see Neil's comment, is there a cifs bug here?
Looking into it... 

steved.
> 
> --b.
> 
> On Tue, Apr 23, 2019 at 09:47:06AM +1000, NeilBrown wrote:
>> On Mon, Apr 22 2019, Jeff Layton wrote:
>>
>>> After a blocked nfsd file_lock request is deleted, knfsd will send a
>>> callback to the client and then free the request. Commit 16306a61d3b7
>>> ("fs/locks: always delete_block after waiting.") changed it such that
>>> locks_delete_block is always called on a request after it is awoken,
>>> but that patch missed fixing up blocked nfsd request handling.
>>>
>>> Call locks_delete_block on the block to wake up any locks still blocked
>>> on the nfsd lock request before freeing it. Some of its callers already
>>> do this however, so just remove those calls.
>>>
>>> URL: https://bugzilla.kernel.org/show_bug.cgi?id=203363
>>> Fixes: 16306a61d3b7 ("fs/locks: always delete_block after waiting.")
>>> Reported-by: Slawomir Pryczek <slawek1211@gmail.com>
>>> Cc: Neil Brown <neilb@suse.com>
>>> Cc: stable@vger.kernel.org
>>> Signed-off-by: Jeff Layton <jlayton@kernel.org>
>>> ---
>>>  fs/nfsd/nfs4state.c | 3 +--
>>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>>
>>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>>> index 6a45fb00c5fc..e87e15df2044 100644
>>> --- a/fs/nfsd/nfs4state.c
>>> +++ b/fs/nfsd/nfs4state.c
>>> @@ -265,6 +265,7 @@ find_or_allocate_block(struct nfs4_lockowner *lo, struct knfsd_fh *fh,
>>>  static void
>>>  free_blocked_lock(struct nfsd4_blocked_lock *nbl)
>>>  {
>>> +	locks_delete_block(&nbl->nbl_lock);
>>>  	locks_release_private(&nbl->nbl_lock);
>>
>> Thanks for tracking this down.
>>
>> An implication of this bug and fix is that we need to be particularly
>> careful to make sure locks_delete_block() is called on all relevant
>> paths.
>> Can we make that easier?  My first thought was to include the call in
>> locks_release_private, but lockd calls the two quite separately and it
>> certainly seems appropriate that locks_delete_block should be called
>> asap, but locks_release_private() can be delayed.
>>
>> Also cifs calls locks_delete_block, but never calls
>> locks_release_private, so it wouldn't help there.
>>
>> Looking at cifs, I think there is a call missing there too.
>> cifs_posix_lock_set() *doesn't* always call locks_delete_block() after
>> waiting.  In particular, if ->can_cache_brlcks becomes true while
>> waiting then I don't think the behaviour is right.... though I'm not
>> sure it is right for other reasons.  It looks like the return value
>> should be 1 in that case, but it'll be zero.
>>
>> But back to my question about making it easier, move the BUG_ON()
>> calls from locks_free_lock() into locks_release_private().
>>
>> ??
>>
>> Thanks,
>> NeilBrown
>>
>>
>>>  	kfree(nbl);
>>>  }
>>> @@ -293,7 +294,6 @@ remove_blocked_locks(struct nfs4_lockowner *lo)
>>>  		nbl = list_first_entry(&reaplist, struct nfsd4_blocked_lock,
>>>  					nbl_lru);
>>>  		list_del_init(&nbl->nbl_lru);
>>> -		locks_delete_block(&nbl->nbl_lock);
>>>  		free_blocked_lock(nbl);
>>>  	}
>>>  }
>>> @@ -4863,7 +4863,6 @@ nfs4_laundromat(struct nfsd_net *nn)
>>>  		nbl = list_first_entry(&reaplist,
>>>  					struct nfsd4_blocked_lock, nbl_lru);
>>>  		list_del_init(&nbl->nbl_lru);
>>> -		locks_delete_block(&nbl->nbl_lock);
>>>  		free_blocked_lock(nbl);
>>>  	}
>>>  out:
>>> -- 
>>> 2.20.1
> 
> 

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 1/2] nfsd: wake waiters blocked on file_lock before deleting it
  2019-04-24 15:29       ` Steve Dickson
@ 2019-04-24 15:47         ` J. Bruce Fields
  2019-04-24 19:09           ` Pavel Shilovsky
  0 siblings, 1 reply; 13+ messages in thread
From: J. Bruce Fields @ 2019-04-24 15:47 UTC (permalink / raw)
  To: Steve Dickson
  Cc: NeilBrown, Jeff Layton, slawek1211, linux-nfs, linux-cifs, Steve French

On Wed, Apr 24, 2019 at 11:29:59AM -0400, Steve Dickson wrote:
> 
> 
> On 4/24/19 9:58 AM, J. Bruce Fields wrote:
> > Steve, see Neil's comment, is there a cifs bug here?
> Looking into it... 

I was thinking Steve French, though I'm sure he wouldn't mind if you
fixed cifs bugs.  Too many Steves!

--b.

> 
> steved.
> > 
> > --b.
> > 
> > On Tue, Apr 23, 2019 at 09:47:06AM +1000, NeilBrown wrote:
> >> On Mon, Apr 22 2019, Jeff Layton wrote:
> >>
> >>> After a blocked nfsd file_lock request is deleted, knfsd will send a
> >>> callback to the client and then free the request. Commit 16306a61d3b7
> >>> ("fs/locks: always delete_block after waiting.") changed it such that
> >>> locks_delete_block is always called on a request after it is awoken,
> >>> but that patch missed fixing up blocked nfsd request handling.
> >>>
> >>> Call locks_delete_block on the block to wake up any locks still blocked
> >>> on the nfsd lock request before freeing it. Some of its callers already
> >>> do this however, so just remove those calls.
> >>>
> >>> URL: https://bugzilla.kernel.org/show_bug.cgi?id=203363
> >>> Fixes: 16306a61d3b7 ("fs/locks: always delete_block after waiting.")
> >>> Reported-by: Slawomir Pryczek <slawek1211@gmail.com>
> >>> Cc: Neil Brown <neilb@suse.com>
> >>> Cc: stable@vger.kernel.org
> >>> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> >>> ---
> >>>  fs/nfsd/nfs4state.c | 3 +--
> >>>  1 file changed, 1 insertion(+), 2 deletions(-)
> >>>
> >>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> >>> index 6a45fb00c5fc..e87e15df2044 100644
> >>> --- a/fs/nfsd/nfs4state.c
> >>> +++ b/fs/nfsd/nfs4state.c
> >>> @@ -265,6 +265,7 @@ find_or_allocate_block(struct nfs4_lockowner *lo, struct knfsd_fh *fh,
> >>>  static void
> >>>  free_blocked_lock(struct nfsd4_blocked_lock *nbl)
> >>>  {
> >>> +	locks_delete_block(&nbl->nbl_lock);
> >>>  	locks_release_private(&nbl->nbl_lock);
> >>
> >> Thanks for tracking this down.
> >>
> >> An implication of this bug and fix is that we need to be particularly
> >> careful to make sure locks_delete_block() is called on all relevant
> >> paths.
> >> Can we make that easier?  My first thought was to include the call in
> >> locks_release_private, but lockd calls the two quite separately and it
> >> certainly seems appropriate that locks_delete_block should be called
> >> asap, but locks_release_private() can be delayed.
> >>
> >> Also cifs calls locks_delete_block, but never calls
> >> locks_release_private, so it wouldn't help there.
> >>
> >> Looking at cifs, I think there is a call missing there too.
> >> cifs_posix_lock_set() *doesn't* always call locks_delete_block() after
> >> waiting.  In particular, if ->can_cache_brlcks becomes true while
> >> waiting then I don't think the behaviour is right.... though I'm not
> >> sure it is right for other reasons.  It looks like the return value
> >> should be 1 in that case, but it'll be zero.
> >>
> >> But back to my question about making it easier, move the BUG_ON()
> >> calls from locks_free_lock() into locks_release_private().
> >>
> >> ??
> >>
> >> Thanks,
> >> NeilBrown
> >>
> >>
> >>>  	kfree(nbl);
> >>>  }
> >>> @@ -293,7 +294,6 @@ remove_blocked_locks(struct nfs4_lockowner *lo)
> >>>  		nbl = list_first_entry(&reaplist, struct nfsd4_blocked_lock,
> >>>  					nbl_lru);
> >>>  		list_del_init(&nbl->nbl_lru);
> >>> -		locks_delete_block(&nbl->nbl_lock);
> >>>  		free_blocked_lock(nbl);
> >>>  	}
> >>>  }
> >>> @@ -4863,7 +4863,6 @@ nfs4_laundromat(struct nfsd_net *nn)
> >>>  		nbl = list_first_entry(&reaplist,
> >>>  					struct nfsd4_blocked_lock, nbl_lru);
> >>>  		list_del_init(&nbl->nbl_lru);
> >>> -		locks_delete_block(&nbl->nbl_lock);
> >>>  		free_blocked_lock(nbl);
> >>>  	}
> >>>  out:
> >>> -- 
> >>> 2.20.1
> > 
> > 

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 1/2] nfsd: wake waiters blocked on file_lock before deleting it
  2019-04-24 15:47         ` J. Bruce Fields
@ 2019-04-24 19:09           ` Pavel Shilovsky
  0 siblings, 0 replies; 13+ messages in thread
From: Pavel Shilovsky @ 2019-04-24 19:09 UTC (permalink / raw)
  To: J. Bruce Fields
  Cc: Steve Dickson, NeilBrown, Jeff Layton, slawek1211,
	Linux NFS Mailing list, linux-cifs, Steve French,
	Ronnie Sahlberg

Yes, I think there is a bug here, thanks!

If cinode->can_cache_brlcks is false we should return 1 but the code
will return 0 if coming from the "try_again" label. We also need to
call locks_delete_block unconditionally at the end of the function.

--
Best regards,
Pavel Shilovsky

ср, 24 апр. 2019 г. в 08:48, J. Bruce Fields <bfields@fieldses.org>:
>
> On Wed, Apr 24, 2019 at 11:29:59AM -0400, Steve Dickson wrote:
> >
> >
> > On 4/24/19 9:58 AM, J. Bruce Fields wrote:
> > > Steve, see Neil's comment, is there a cifs bug here?
> > Looking into it...
>
> I was thinking Steve French, though I'm sure he wouldn't mind if you
> fixed cifs bugs.  Too many Steves!
>
> --b.
>
> >
> > steved.
> > >
> > > --b.
> > >
> > > On Tue, Apr 23, 2019 at 09:47:06AM +1000, NeilBrown wrote:
> > >> On Mon, Apr 22 2019, Jeff Layton wrote:
> > >>
> > >>> After a blocked nfsd file_lock request is deleted, knfsd will send a
> > >>> callback to the client and then free the request. Commit 16306a61d3b7
> > >>> ("fs/locks: always delete_block after waiting.") changed it such that
> > >>> locks_delete_block is always called on a request after it is awoken,
> > >>> but that patch missed fixing up blocked nfsd request handling.
> > >>>
> > >>> Call locks_delete_block on the block to wake up any locks still blocked
> > >>> on the nfsd lock request before freeing it. Some of its callers already
> > >>> do this however, so just remove those calls.
> > >>>
> > >>> URL: https://bugzilla.kernel.org/show_bug.cgi?id=203363
> > >>> Fixes: 16306a61d3b7 ("fs/locks: always delete_block after waiting.")
> > >>> Reported-by: Slawomir Pryczek <slawek1211@gmail.com>
> > >>> Cc: Neil Brown <neilb@suse.com>
> > >>> Cc: stable@vger.kernel.org
> > >>> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > >>> ---
> > >>>  fs/nfsd/nfs4state.c | 3 +--
> > >>>  1 file changed, 1 insertion(+), 2 deletions(-)
> > >>>
> > >>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > >>> index 6a45fb00c5fc..e87e15df2044 100644
> > >>> --- a/fs/nfsd/nfs4state.c
> > >>> +++ b/fs/nfsd/nfs4state.c
> > >>> @@ -265,6 +265,7 @@ find_or_allocate_block(struct nfs4_lockowner *lo, struct knfsd_fh *fh,
> > >>>  static void
> > >>>  free_blocked_lock(struct nfsd4_blocked_lock *nbl)
> > >>>  {
> > >>> + locks_delete_block(&nbl->nbl_lock);
> > >>>   locks_release_private(&nbl->nbl_lock);
> > >>
> > >> Thanks for tracking this down.
> > >>
> > >> An implication of this bug and fix is that we need to be particularly
> > >> careful to make sure locks_delete_block() is called on all relevant
> > >> paths.
> > >> Can we make that easier?  My first thought was to include the call in
> > >> locks_release_private, but lockd calls the two quite separately and it
> > >> certainly seems appropriate that locks_delete_block should be called
> > >> asap, but locks_release_private() can be delayed.
> > >>
> > >> Also cifs calls locks_delete_block, but never calls
> > >> locks_release_private, so it wouldn't help there.
> > >>
> > >> Looking at cifs, I think there is a call missing there too.
> > >> cifs_posix_lock_set() *doesn't* always call locks_delete_block() after
> > >> waiting.  In particular, if ->can_cache_brlcks becomes true while
> > >> waiting then I don't think the behaviour is right.... though I'm not
> > >> sure it is right for other reasons.  It looks like the return value
> > >> should be 1 in that case, but it'll be zero.
> > >>
> > >> But back to my question about making it easier, move the BUG_ON()
> > >> calls from locks_free_lock() into locks_release_private().
> > >>
> > >> ??
> > >>
> > >> Thanks,
> > >> NeilBrown
> > >>
> > >>
> > >>>   kfree(nbl);
> > >>>  }
> > >>> @@ -293,7 +294,6 @@ remove_blocked_locks(struct nfs4_lockowner *lo)
> > >>>           nbl = list_first_entry(&reaplist, struct nfsd4_blocked_lock,
> > >>>                                   nbl_lru);
> > >>>           list_del_init(&nbl->nbl_lru);
> > >>> -         locks_delete_block(&nbl->nbl_lock);
> > >>>           free_blocked_lock(nbl);
> > >>>   }
> > >>>  }
> > >>> @@ -4863,7 +4863,6 @@ nfs4_laundromat(struct nfsd_net *nn)
> > >>>           nbl = list_first_entry(&reaplist,
> > >>>                                   struct nfsd4_blocked_lock, nbl_lru);
> > >>>           list_del_init(&nbl->nbl_lru);
> > >>> -         locks_delete_block(&nbl->nbl_lock);
> > >>>           free_blocked_lock(nbl);
> > >>>   }
> > >>>  out:
> > >>> --
> > >>> 2.20.1
> > >
> > >

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2019-04-24 19:09 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-04-22 16:34 [PATCH v2 0/2] nfsd: ensure we wake file lock waiters before deleting blocked lock Jeff Layton
2019-04-22 16:34 ` [PATCH v2 1/2] nfsd: wake waiters blocked on file_lock before deleting it Jeff Layton
2019-04-22 23:47   ` NeilBrown
2019-04-23 10:57     ` Jeff Layton
2019-04-24  2:00       ` [PATCH] locks: move checks from locks_free_lock() to locks_release_private() NeilBrown
2019-04-24 13:47         ` Jeff Layton
2019-04-24 13:55           ` Bruce Fields
2019-04-24 13:58     ` [PATCH v2 1/2] nfsd: wake waiters blocked on file_lock before deleting it J. Bruce Fields
2019-04-24 15:29       ` Steve Dickson
2019-04-24 15:47         ` J. Bruce Fields
2019-04-24 19:09           ` Pavel Shilovsky
2019-04-22 16:34 ` [PATCH v2 2/2] nfsd: wake blocked file lock waiters before sending callback Jeff Layton
2019-04-22 19:46   ` J. Bruce Fields

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