Re: [PATCH] locks: add locks_move_blocks in posix_lock_inode

[Jeff Layton <jlayton@kernel.org>]

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>