Re: [PATCH] locks: add locks_move_blocks in posix_lock_inode

[Jeff Layton <jlayton@kernel.org>]

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>