locks: add locks_move_blocks in posix_lock_inode
diff mbox series

Message ID 20200601091616.34137-1-yangerkun@huawei.com
State New
Headers show
Series
  • locks: add locks_move_blocks in posix_lock_inode
Related show

Commit Message

yangerkun June 1, 2020, 9:16 a.m. UTC
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(+)

Comments

NeilBrown June 1, 2020, 11:10 p.m. UTC | #1
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
yangerkun June 2, 2020, 1:49 p.m. UTC | #2
在 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
Jeff Layton June 2, 2020, 3:56 p.m. UTC | #3
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
yangerkun June 3, 2020, 1:22 a.m. UTC | #4
在 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
>
Jeff Layton June 3, 2020, 10:34 a.m. UTC | #5
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!

Patch
diff mbox series

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