From: Vasily Averin <vvs@virtuozzo.com>
To: "J. Bruce Fields" <bfields@fieldses.org>
Cc: Trond Myklebust <trond.myklebust@hammerspace.com>,
Anna Schumaker <anna.schumaker@netapp.com>,
kernel@openvz.org, linux-nfs@vger.kernel.org,
linux-kernel@vger.kernel.org,
Chuck Lever <chuck.lever@oracle.com>
Subject: Re: [PATCH v3 2/3] nfs4: handle async processing of F_SETLK with FL_SLEEP
Date: Sun, 16 Jan 2022 15:44:21 +0300 [thread overview]
Message-ID: <7666958f-6215-a8eb-3412-b613158406db@virtuozzo.com> (raw)
In-Reply-To: <20220103195333.GG21514@fieldses.org>
On 03.01.2022 22:53, J. Bruce Fields wrote:
> On Wed, Dec 29, 2021 at 11:24:43AM +0300, Vasily Averin wrote:
>> nfsd and lockd use F_SETLK cmd with the FL_SLEEP flag set to request
>> asynchronous processing of blocking locks.
>>
>> Currently nfs4 use locks_lock_inode_wait() function which is blocked
>> for such requests. To handle them correctly FL_SLEEP flag should be
>> temporarily reset before executing the locks_lock_inode_wait() function.
>>
>> Additionally block flag is forced to set, to translate blocking lock to
>> remote nfs server, expecting it supports async processing of the blocking
>> locks too.
>
> But this on its own isn't enough for the client to support asynchronous
> blocking locks, right? Don't we also need the logic that calls knfsd's
> lm_notify when it gets a CB_NOTIFY_LOCK from the server?
No, I think this should be enough.
We are here a nfs client,
we can get F_SETLK with FL_SLEEP from nfsd only (i.e. in re-export case)
we need to avoid blocking if lock is already taken,
so we need to call locks_lock_inode_wait without FL_SLEEP,
then we submit _sleeping_ request to NFS server (i.e. set )data->arg.block = 1)
and waiting for reply from server.
Here we rely that server will NOT block on such request too, so our reply wel not be blocked too.
Under "block" I mean that handler can sleep or process request for a very long time
but it will NOT BE BLOCKED if lock is taken already, it WILL NOT WAIT when lock will be released,
it just return some error in this case.
I think it is correct.
Do you think I am wrong or maybe I missed something?
Thank you,
Vasily Averin
However I noticed now that past is incorrect,
temporally dropped FL_SLEEP should be restored back in _nfs4_proc_setlk before _nfs4_do_setlk() call.
I'll fix it in next version of this patch-set.
>> https://bugzilla.kernel.org/show_bug.cgi?id=215383
>> Signed-off-by: Vasily Averin <vvs@virtuozzo.com>
>> ---
>> fs/nfs/nfs4proc.c | 5 ++++-
>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
>> index ee3bc79f6ca3..9b1380c4223c 100644
>> --- a/fs/nfs/nfs4proc.c
>> +++ b/fs/nfs/nfs4proc.c
>> @@ -7094,7 +7094,7 @@ static int _nfs4_do_setlk(struct nfs4_state *state, int cmd, struct file_lock *f
>> recovery_type == NFS_LOCK_NEW ? GFP_KERNEL : GFP_NOFS);
>> if (data == NULL)
>> return -ENOMEM;
>> - if (IS_SETLKW(cmd))
>> + if (IS_SETLKW(cmd) || (fl->fl_flags & FL_SLEEP))
>> data->arg.block = 1;
>> nfs4_init_sequence(&data->arg.seq_args, &data->res.seq_res, 1,
>> recovery_type > NFS_LOCK_NEW);
>> @@ -7200,6 +7200,9 @@ static int _nfs4_proc_setlk(struct nfs4_state *state, int cmd, struct file_lock
>> int status;
>>
>> request->fl_flags |= FL_ACCESS;
>> + if (((fl_flags & FL_SLEEP_POSIX) == FL_SLEEP_POSIX) && IS_SETLK(cmd))
>> + request->fl_flags &= ~FL_SLEEP;
>> +
>> status = locks_lock_inode_wait(state->inode, request);
>> if (status < 0)
>> goto out;
>> --
>> 2.25.1
next prev parent reply other threads:[~2022-01-16 12:44 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-12-27 10:45 [PATCH] nfs4: handle async processing of F_SETLK with FL_SLEEP Vasily Averin
2021-12-27 15:51 ` [PATCH v2] " Vasily Averin
2021-12-28 1:04 ` Vasily Averin
2021-12-29 8:24 ` [PATCH v3 1/3] nfs: local_lock: " Vasily Averin
2021-12-29 8:24 ` [PATCH v3 2/3] nfs4: " Vasily Averin
2022-01-03 19:40 ` J. Bruce Fields
2022-01-16 12:25 ` Vasily Averin
2022-01-03 19:53 ` J. Bruce Fields
2022-01-16 12:44 ` Vasily Averin [this message]
2022-01-16 18:28 ` Vasily Averin
2022-01-18 22:35 ` J. Bruce Fields
2021-12-29 8:24 ` [PATCH v3 3/3] nfs v2/3: nlmclnt_lock: " Vasily Averin
2021-12-27 17:58 ` [PATCH] nfs4: " kernel test robot
2021-12-27 17:58 ` kernel test robot
-- strict thread matches above, loose matches on Subject: below --
2021-12-28 8:43 kernel test robot
2022-01-06 10:37 ` Dan Carpenter
2022-01-06 10:37 ` Dan Carpenter
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=7666958f-6215-a8eb-3412-b613158406db@virtuozzo.com \
--to=vvs@virtuozzo.com \
--cc=anna.schumaker@netapp.com \
--cc=bfields@fieldses.org \
--cc=chuck.lever@oracle.com \
--cc=kernel@openvz.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-nfs@vger.kernel.org \
--cc=trond.myklebust@hammerspace.com \
/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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.