From: yangerkun <yangerkun@huawei.com>
To: Jeff Layton <jlayton@kernel.org>, NeilBrown <neilb@suse.de>,
<viro@zeniv.linux.org.uk>, <neilb@suse.com>
Cc: <linux-fsdevel@vger.kernel.org>,
"bfields@vger.kernel.org" <bfields@vger.kernel.org>
Subject: Re: [PATCH] locks: add locks_move_blocks in posix_lock_inode
Date: Wed, 3 Jun 2020 09:22:49 +0800 [thread overview]
Message-ID: <e4a8cdbc-dfe6-4630-ce5e-49958f5f0813@huawei.com> (raw)
In-Reply-To: <63020790a240cfcd1d798147edebbc231b1ff32b.camel@kernel.org>
在 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
>
next prev parent reply other threads:[~2020-06-03 1:22 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-06-01 9:16 [PATCH] locks: add locks_move_blocks in posix_lock_inode yangerkun
2020-06-01 23:10 ` NeilBrown
2020-06-02 13:49 ` yangerkun
2020-06-02 15:56 ` Jeff Layton
2020-06-03 1:22 ` yangerkun [this message]
2020-06-03 10:34 ` Jeff Layton
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=e4a8cdbc-dfe6-4630-ce5e-49958f5f0813@huawei.com \
--to=yangerkun@huawei.com \
--cc=bfields@vger.kernel.org \
--cc=jlayton@kernel.org \
--cc=linux-fsdevel@vger.kernel.org \
--cc=neilb@suse.com \
--cc=neilb@suse.de \
--cc=viro@zeniv.linux.org.uk \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).