| Message ID | 00d1e0fa-55dd-82a5-2607-70d4552cc7f4@virtuozzo.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3,1/3] nfs: local_lock: handle async processing of F_SETLK with FL_SLEEP | expand |
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. Seems like an improvement, but is there some way to make this more straightforward by just calling a function that doesn't sleep in the first place? (posix_lock_inode(), maybe?) --b. > > 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
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? --b. > > 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
On 03.01.2022 22:40, 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. > > Seems like an improvement, but is there some way to make this more > straightforward by just calling a function that doesn't sleep in the > first place? (posix_lock_inode(), maybe?) There are few problems: 1) posix_lock_inode() is static in fs/locks.c 2) exported posix_lock_file() used posix_lock_inode() inside requires file pointer, and I do not understand how to get it. 3) _nfs4_do_setlk() is called from do_setlk and handles flocks too, therefore any posix-only calls requires additional checks or branches. On the other hand all that is required to handle F_SETLK with FL_SLEEP correctly : to avoid blocking on exiting lock. We can reach this goal here by drop of FL_SLEEP flag before locks_lock_inode_wait() execution. Thank you, Vasily Averin PS I'm worry for a long delay with answer, in Russia we have long holidays after New Year, then I dealt with urgent tasks accumulated over the holidays then I forgot the context of this patch and I was need to spend some time to re-member the details. >> 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
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
On 16.01.2022 15:44, Vasily Averin wrote: > 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. Now I think this assumption is wrong. We cannot guarantee that NFS server will process our sleeping request asynchronously. yes, new version of knfsd will do it. however there are a lot of other NFS servers, that can process this request synchronously and wait till locked fail will be unlocked. All we can do here is just drop FL_SLEEP and handle incoming async request (F_SETLK with FL_SLEEP) like a regular non-blocking F_SETLK. Thank you, Vasily Averin > 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 >
On Sun, Jan 16, 2022 at 03:44:21PM +0300, Vasily Averin wrote: > 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. Just on that one point: if there's a lock conflict, an NFSv4 server will return NFS4ERR_DENIED immediately and leave it to the client to poll. Or if you're using NFS version >= 4.1, the server has the option of calling back to the client with a CB_NOTIFY_LOCK to let the client know when the lock might be available. (See https://datatracker.ietf.org/doc/html/rfc8881#section-20.11 for details.) But if a server that blocked and didn't reply to the original LOCK request until the lock became available, that would be a bug. (Apologies for responding just to that one point, I'm also trying to get caught back up again here....). --b. > 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
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;
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. 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(-)