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
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 <[email protected]>
Cc: Neil Brown <[email protected]>
Cc: [email protected]
Signed-off-by: Jeff Layton <[email protected]>
---
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
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 <[email protected]>
Cc: Neil Brown <[email protected]>
Cc: [email protected]
Signed-off-by: Jeff Layton <[email protected]>
---
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
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 <[email protected]>
> Cc: Neil Brown <[email protected]>
> Cc: [email protected]
> Signed-off-by: Jeff Layton <[email protected]>
> ---
> 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
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 <[email protected]>
> Cc: Neil Brown <[email protected]>
> Cc: [email protected]
> Signed-off-by: Jeff Layton <[email protected]>
> ---
> 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
On Mon, Apr 22, 2019 at 7:47 PM NeilBrown <[email protected]> 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 <[email protected]>
> > Cc: Neil Brown <[email protected]>
> > Cc: [email protected]
> > Signed-off-by: Jeff Layton <[email protected]>
> > ---
> > 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 <[email protected]>
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 <[email protected]>
---
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
On Tue, Apr 23, 2019 at 10:00 PM NeilBrown <[email protected]> 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 <[email protected]>
> ---
> 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 <[email protected]>
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 <[email protected]>
Got it, thanks.--b.
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 <[email protected]>
> > Cc: Neil Brown <[email protected]>
> > Cc: [email protected]
> > Signed-off-by: Jeff Layton <[email protected]>
> > ---
> > 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
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 <[email protected]>
>>> Cc: Neil Brown <[email protected]>
>>> Cc: [email protected]
>>> Signed-off-by: Jeff Layton <[email protected]>
>>> ---
>>> 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
>
>
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 <[email protected]>
> >>> Cc: Neil Brown <[email protected]>
> >>> Cc: [email protected]
> >>> Signed-off-by: Jeff Layton <[email protected]>
> >>> ---
> >>> 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
> >
> >
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 <[email protected]>:
>
> 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 <[email protected]>
> > >>> Cc: Neil Brown <[email protected]>
> > >>> Cc: [email protected]
> > >>> Signed-off-by: Jeff Layton <[email protected]>
> > >>> ---
> > >>> 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
> > >
> > >