* [PATCH] locks: add locks_move_blocks in posix_lock_inode
@ 2020-06-01 9:16 yangerkun
2020-06-01 23:10 ` NeilBrown
0 siblings, 1 reply; 6+ messages in thread
From: yangerkun @ 2020-06-01 9:16 UTC (permalink / raw)
To: viro, jlayton, neilb; +Cc: linux-fsdevel
We forget to call locks_move_blocks in posix_lock_inode when try to
process same owner and different types.
Signed-off-by: yangerkun <yangerkun@huawei.com>
---
fs/locks.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/fs/locks.c b/fs/locks.c
index b8a31c1c4fff..36bd2c221786 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -1282,6 +1282,7 @@ static int posix_lock_inode(struct inode *inode, struct file_lock *request,
if (!new_fl)
goto out;
locks_copy_lock(new_fl, request);
+ locks_move_blocks(new_fl, request);
request = new_fl;
new_fl = NULL;
locks_insert_lock_ctx(request, &fl->fl_list);
--
2.21.3
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] locks: add locks_move_blocks in posix_lock_inode
2020-06-01 9:16 [PATCH] locks: add locks_move_blocks in posix_lock_inode yangerkun
@ 2020-06-01 23:10 ` NeilBrown
2020-06-02 13:49 ` yangerkun
0 siblings, 1 reply; 6+ messages in thread
From: NeilBrown @ 2020-06-01 23:10 UTC (permalink / raw)
To: yangerkun, viro, jlayton, neilb; +Cc: linux-fsdevel
[-- Attachment #1: Type: text/plain, Size: 1171 bytes --]
On Mon, Jun 01 2020, yangerkun wrote:
> We forget to call locks_move_blocks in posix_lock_inode when try to
> process same owner and different types.
>
This patch is not necessary.
The caller of posix_lock_inode() must calls locks_delete_block() on
'request', and that will remove all blocked request and retry them.
So calling locks_move_blocks() here is at most an optimization. Maybe
it is a useful one.
What led you to suggesting this patch? Were you just examining the
code, or was there some problem that you were trying to solve?
Thanks,
NeilBrown
> Signed-off-by: yangerkun <yangerkun@huawei.com>
> ---
> fs/locks.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/fs/locks.c b/fs/locks.c
> index b8a31c1c4fff..36bd2c221786 100644
> --- a/fs/locks.c
> +++ b/fs/locks.c
> @@ -1282,6 +1282,7 @@ static int posix_lock_inode(struct inode *inode, struct file_lock *request,
> if (!new_fl)
> goto out;
> locks_copy_lock(new_fl, request);
> + locks_move_blocks(new_fl, request);
> request = new_fl;
> new_fl = NULL;
> locks_insert_lock_ctx(request, &fl->fl_list);
> --
> 2.21.3
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] locks: add locks_move_blocks in posix_lock_inode
2020-06-01 23:10 ` NeilBrown
@ 2020-06-02 13:49 ` yangerkun
2020-06-02 15:56 ` Jeff Layton
0 siblings, 1 reply; 6+ messages in thread
From: yangerkun @ 2020-06-02 13:49 UTC (permalink / raw)
To: NeilBrown, viro, jlayton, neilb; +Cc: linux-fsdevel
在 2020/6/2 7:10, NeilBrown 写道:
> On Mon, Jun 01 2020, yangerkun wrote:
>
>> We forget to call locks_move_blocks in posix_lock_inode when try to
>> process same owner and different types.
>>
>
> This patch is not necessary.
> The caller of posix_lock_inode() must calls locks_delete_block() on
> 'request', and that will remove all blocked request and retry them.
>
> So calling locks_move_blocks() here is at most an optimization. Maybe
> it is a useful one.
>
> What led you to suggesting this patch? Were you just examining the
> code, or was there some problem that you were trying to solve?
Actually, case of this means just replace a exists file_lock. And once
we forget to call locks_move_blocks, the function call of
posix_lock_inode will also call locks_delete_block, and will wakeup all
blocked requests and retry them. But we should do this until we UNLOCK
the file_lock! So, it's really a bug here.
Thanks,
Kun.
>
> Thanks,
> NeilBrown
>
>
>> Signed-off-by: yangerkun <yangerkun@huawei.com>
>> ---
>> fs/locks.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/fs/locks.c b/fs/locks.c
>> index b8a31c1c4fff..36bd2c221786 100644
>> --- a/fs/locks.c
>> +++ b/fs/locks.c
>> @@ -1282,6 +1282,7 @@ static int posix_lock_inode(struct inode *inode, struct file_lock *request,
>> if (!new_fl)
>> goto out;
>> locks_copy_lock(new_fl, request);
>> + locks_move_blocks(new_fl, request);
>> request = new_fl;
>> new_fl = NULL;
>> locks_insert_lock_ctx(request, &fl->fl_list);
>> --
>> 2.21.3
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] locks: add locks_move_blocks in posix_lock_inode
2020-06-02 13:49 ` yangerkun
@ 2020-06-02 15:56 ` Jeff Layton
2020-06-03 1:22 ` yangerkun
0 siblings, 1 reply; 6+ messages in thread
From: Jeff Layton @ 2020-06-02 15:56 UTC (permalink / raw)
To: yangerkun, NeilBrown, viro, neilb; +Cc: linux-fsdevel, bfields
On Tue, 2020-06-02 at 21:49 +0800, yangerkun wrote:
>
> 在 2020/6/2 7:10, NeilBrown 写道:
> > On Mon, Jun 01 2020, yangerkun wrote:
> >
> > > We forget to call locks_move_blocks in posix_lock_inode when try to
> > > process same owner and different types.
> > >
> >
> > This patch is not necessary.
> > The caller of posix_lock_inode() must calls locks_delete_block() on
> > 'request', and that will remove all blocked request and retry them.
> >
> > So calling locks_move_blocks() here is at most an optimization. Maybe
> > it is a useful one.
> >
> > What led you to suggesting this patch? Were you just examining the
> > code, or was there some problem that you were trying to solve?
>
>
> Actually, case of this means just replace a exists file_lock. And once
> we forget to call locks_move_blocks, the function call of
> posix_lock_inode will also call locks_delete_block, and will wakeup all
> blocked requests and retry them. But we should do this until we UNLOCK
> the file_lock! So, it's really a bug here.
>
Waking up waiters to re-poll a lock that's still blocked seems wrong. I
agree with Neil that this is mainly an optimization, but it does look
useful.
Unfortunately this is the type of thing that's quite difficult to test
for in a userland testcase. Is this something you noticed due to the
extra wakeups or did you find it by inspection? It'd be great to have a
better way to test for this in xfstests or something.
I'll plan to add this to linux-next. It should make v5.9, but let me
know if this is causing real-world problems and maybe we can make a case
for v5.8.
Thanks,
Jeff
> >
> > > Signed-off-by: yangerkun <yangerkun@huawei.com>
> > > ---
> > > fs/locks.c | 1 +
> > > 1 file changed, 1 insertion(+)
> > >
> > > diff --git a/fs/locks.c b/fs/locks.c
> > > index b8a31c1c4fff..36bd2c221786 100644
> > > --- a/fs/locks.c
> > > +++ b/fs/locks.c
> > > @@ -1282,6 +1282,7 @@ static int posix_lock_inode(struct inode *inode, struct file_lock *request,
> > > if (!new_fl)
> > > goto out;
> > > locks_copy_lock(new_fl, request);
> > > + locks_move_blocks(new_fl, request);
> > > request = new_fl;
> > > new_fl = NULL;
> > > locks_insert_lock_ctx(request, &fl->fl_list);
> > > --
> > > 2.21.3
--
Jeff Layton <jlayton@kernel.org>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] locks: add locks_move_blocks in posix_lock_inode
2020-06-02 15:56 ` Jeff Layton
@ 2020-06-03 1:22 ` yangerkun
2020-06-03 10:34 ` Jeff Layton
0 siblings, 1 reply; 6+ messages in thread
From: yangerkun @ 2020-06-03 1:22 UTC (permalink / raw)
To: Jeff Layton, NeilBrown, viro, neilb; +Cc: linux-fsdevel, bfields
在 2020/6/2 23:56, Jeff Layton 写道:
> On Tue, 2020-06-02 at 21:49 +0800, yangerkun wrote:
>>
>> 在 2020/6/2 7:10, NeilBrown 写道:
>>> On Mon, Jun 01 2020, yangerkun wrote:
>>>
>>>> We forget to call locks_move_blocks in posix_lock_inode when try to
>>>> process same owner and different types.
>>>>
>>>
>>> This patch is not necessary.
>>> The caller of posix_lock_inode() must calls locks_delete_block() on
>>> 'request', and that will remove all blocked request and retry them.
>>>
>>> So calling locks_move_blocks() here is at most an optimization. Maybe
>>> it is a useful one.
>>>
>>> What led you to suggesting this patch? Were you just examining the
>>> code, or was there some problem that you were trying to solve?
>>
>>
>> Actually, case of this means just replace a exists file_lock. And once
>> we forget to call locks_move_blocks, the function call of
>> posix_lock_inode will also call locks_delete_block, and will wakeup all
>> blocked requests and retry them. But we should do this until we UNLOCK
>> the file_lock! So, it's really a bug here.
>>
>
> Waking up waiters to re-poll a lock that's still blocked seems wrong. I
> agree with Neil that this is mainly an optimization, but it does look
> useful.
Agree. Logic of this seems wrong, but it won't trigger any problem since
the waiters will conflict and try wait again.
>
> Unfortunately this is the type of thing that's quite difficult to test
> for in a userland testcase. Is this something you noticed due to the
> extra wakeups or did you find it by inspection? It'd be great to have a
> better way to test for this in xfstests or something.
Notice this after reading the patch 5946c4319ebb ("fs/locks: allow a
lock request to block other requests."), and find that we have do the
same thing exist in flock_lock_inode and another place exists in
posix_lock_inode.
>
> I'll plan to add this to linux-next. It should make v5.9, but let me
> know if this is causing real-world problems and maybe we can make a case
> for v5.8.
Actually, I have not try to find will this lead to some real-world
problems... Sorry for this.:(
Thanks,
Kun.
>
> Thanks,
> Jeff
>
>>>
>>>> Signed-off-by: yangerkun <yangerkun@huawei.com>
>>>> ---
>>>> fs/locks.c | 1 +
>>>> 1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/fs/locks.c b/fs/locks.c
>>>> index b8a31c1c4fff..36bd2c221786 100644
>>>> --- a/fs/locks.c
>>>> +++ b/fs/locks.c
>>>> @@ -1282,6 +1282,7 @@ static int posix_lock_inode(struct inode *inode, struct file_lock *request,
>>>> if (!new_fl)
>>>> goto out;
>>>> locks_copy_lock(new_fl, request);
>>>> + locks_move_blocks(new_fl, request);
>>>> request = new_fl;
>>>> new_fl = NULL;
>>>> locks_insert_lock_ctx(request, &fl->fl_list);
>>>> --
>>>> 2.21.3
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] locks: add locks_move_blocks in posix_lock_inode
2020-06-03 1:22 ` yangerkun
@ 2020-06-03 10:34 ` Jeff Layton
0 siblings, 0 replies; 6+ messages in thread
From: Jeff Layton @ 2020-06-03 10:34 UTC (permalink / raw)
To: yangerkun, NeilBrown, viro, neilb; +Cc: linux-fsdevel, bfields
On Wed, 2020-06-03 at 09:22 +0800, yangerkun wrote:
>
> 在 2020/6/2 23:56, Jeff Layton 写道:
> > On Tue, 2020-06-02 at 21:49 +0800, yangerkun wrote:
> > > 在 2020/6/2 7:10, NeilBrown 写道:
> > > > On Mon, Jun 01 2020, yangerkun wrote:
> > > >
> > > > > We forget to call locks_move_blocks in posix_lock_inode when try to
> > > > > process same owner and different types.
> > > > >
> > > >
> > > > This patch is not necessary.
> > > > The caller of posix_lock_inode() must calls locks_delete_block() on
> > > > 'request', and that will remove all blocked request and retry them.
> > > >
> > > > So calling locks_move_blocks() here is at most an optimization. Maybe
> > > > it is a useful one.
> > > >
> > > > What led you to suggesting this patch? Were you just examining the
> > > > code, or was there some problem that you were trying to solve?
> > >
> > > Actually, case of this means just replace a exists file_lock. And once
> > > we forget to call locks_move_blocks, the function call of
> > > posix_lock_inode will also call locks_delete_block, and will wakeup all
> > > blocked requests and retry them. But we should do this until we UNLOCK
> > > the file_lock! So, it's really a bug here.
> > >
> >
> > Waking up waiters to re-poll a lock that's still blocked seems wrong. I
> > agree with Neil that this is mainly an optimization, but it does look
> > useful.
>
> Agree. Logic of this seems wrong, but it won't trigger any problem since
> the waiters will conflict and try wait again.
>
> > Unfortunately this is the type of thing that's quite difficult to test
> > for in a userland testcase. Is this something you noticed due to the
> > extra wakeups or did you find it by inspection? It'd be great to have a
> > better way to test for this in xfstests or something.
>
> Notice this after reading the patch 5946c4319ebb ("fs/locks: allow a
> lock request to block other requests."), and find that we have do the
> same thing exist in flock_lock_inode and another place exists in
> posix_lock_inode.
>
> > I'll plan to add this to linux-next. It should make v5.9, but let me
> > know if this is causing real-world problems and maybe we can make a case
> > for v5.8.
>
> Actually, I have not try to find will this lead to some real-world
> problems... Sorry for this.:(
>
>
> Thanks,
> Kun.
>
No problem. I doubt this would be easily noticeable in testing. Given
that it's not causing immediate issues, we'll let it sit in linux-next
for a cycle and plan to merge this for v5.9.
Thanks!
--
Jeff Layton <jlayton@kernel.org>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-06-03 10:34 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-01 9:16 [PATCH] locks: add locks_move_blocks in posix_lock_inode yangerkun
2020-06-01 23:10 ` NeilBrown
2020-06-02 13:49 ` yangerkun
2020-06-02 15:56 ` Jeff Layton
2020-06-03 1:22 ` yangerkun
2020-06-03 10:34 ` Jeff Layton
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).