| Message ID | 20200601091616.34137-1-yangerkun@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | locks: add locks_move_blocks in posix_lock_inode | expand |
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
在 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
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
在 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 >
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!
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);
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(+)