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@xxxxxxxxxx> > > > --- > > > 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@xxxxxxxxxx>
