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 blocked on such requests.
To handle such requests properly, non-blocking posix_file_lock()
function should be used instead.
https://bugzilla.kernel.org/show_bug.cgi?id=215383
Signed-off-by: Vasily Averin <[email protected]>
---
VvS: I'm not sure that request->fl_file points to the same state->inode
used in locks_lock_inode_wait(). If it is not, posix_lock_inode() can be
used here, but this function is static currently and should be exported first.
---
fs/nfs/nfs4proc.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index ee3bc79f6ca3..54431b296c85 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -7200,8 +7200,11 @@ static int _nfs4_proc_setlk(struct nfs4_state *state, int cmd, struct file_lock
int status;
request->fl_flags |= FL_ACCESS;
- status = locks_lock_inode_wait(state->inode, request);
- if (status < 0)
+ if ((request->fl_flags && FL_SLEEP) && IS_SETLK(cmd))
+ status = posix_lock_file(request->fl_file, request, NULL);
+ else
+ status = locks_lock_inode_wait(state->inode, request);
+ if (status)
goto out;
mutex_lock(&sp->so_delegreturn_mutex);
down_read(&nfsi->rwsem);
--
2.25.1
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 blocked on such requests.
To handle such requests properly, non-blocking posix_file_lock()
function should be used instead.
https://bugzilla.kernel.org/show_bug.cgi?id=215383
Signed-off-by: Vasily Averin <[email protected]>
---
VvS: I'm not sure that request->fl_file points to the same state->inode
used in locks_lock_inode_wait(). If it is not, posix_lock_inode() can be
used here, but this function is static currently and should be exported first.
v2: fixed 'fl_flags && FL_SLEEP' => 'fl_flags & FL_SLEEP'
---
fs/nfs/nfs4proc.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index ee3bc79f6ca3..f899f4bcdae5 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -7200,8 +7200,11 @@ static int _nfs4_proc_setlk(struct nfs4_state *state, int cmd, struct file_lock
int status;
request->fl_flags |= FL_ACCESS;
- status = locks_lock_inode_wait(state->inode, request);
- if (status < 0)
+ if ((request->fl_flags & FL_SLEEP) && IS_SETLK(cmd))
+ status = posix_lock_file(request->fl_file, request, NULL);
+ else
+ status = locks_lock_inode_wait(state->inode, request);
+ if (status)
goto out;
mutex_lock(&sp->so_delegreturn_mutex);
down_read(&nfsi->rwsem);
--
2.25.1
Hi Vasily,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on trondmy-nfs/linux-next]
[also build test WARNING on v5.16-rc7 next-20211224]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Vasily-Averin/nfs4-handle-async-processing-of-F_SETLK-with-FL_SLEEP/20211227-184632
base: git://git.linux-nfs.org/projects/trondmy/linux-nfs.git linux-next
config: i386-randconfig-r005-20211227 (https://download.01.org/0day-ci/archive/20211228/[email protected]/config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project 511726c64d3b6cca66f7c54d457d586aa3129f67)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/7ae55d384b2f337e6630509e451c35dda45ae185
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Vasily-Averin/nfs4-handle-async-processing-of-F_SETLK-with-FL_SLEEP/20211227-184632
git checkout 7ae55d384b2f337e6630509e451c35dda45ae185
# save the config file to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash fs/nfs/
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
All warnings (new ones prefixed by >>):
>> fs/nfs/nfs4proc.c:7202:25: warning: use of logical '&&' with constant operand [-Wconstant-logical-operand]
if ((request->fl_flags && FL_SLEEP) && IS_SETLK(cmd))
^ ~~~~~~~~
fs/nfs/nfs4proc.c:7202:25: note: use '&' for a bitwise operation
if ((request->fl_flags && FL_SLEEP) && IS_SETLK(cmd))
^~
&
fs/nfs/nfs4proc.c:7202:25: note: remove constant to silence this warning
if ((request->fl_flags && FL_SLEEP) && IS_SETLK(cmd))
~^~~~~~~~~~~
1 warning generated.
vim +7202 fs/nfs/nfs4proc.c
7193
7194 static int _nfs4_proc_setlk(struct nfs4_state *state, int cmd, struct file_lock *request)
7195 {
7196 struct nfs_inode *nfsi = NFS_I(state->inode);
7197 struct nfs4_state_owner *sp = state->owner;
7198 unsigned char fl_flags = request->fl_flags;
7199 int status;
7200
7201 request->fl_flags |= FL_ACCESS;
> 7202 if ((request->fl_flags && FL_SLEEP) && IS_SETLK(cmd))
7203 status = posix_lock_file(request->fl_file, request, NULL);
7204 else
7205 status = locks_lock_inode_wait(state->inode, request);
7206 if (status)
7207 goto out;
7208 mutex_lock(&sp->so_delegreturn_mutex);
7209 down_read(&nfsi->rwsem);
7210 if (test_bit(NFS_DELEGATED_STATE, &state->flags)) {
7211 /* Yes: cache locks! */
7212 /* ...but avoid races with delegation recall... */
7213 request->fl_flags = fl_flags & ~FL_SLEEP;
7214 status = locks_lock_inode_wait(state->inode, request);
7215 up_read(&nfsi->rwsem);
7216 mutex_unlock(&sp->so_delegreturn_mutex);
7217 goto out;
7218 }
7219 up_read(&nfsi->rwsem);
7220 mutex_unlock(&sp->so_delegreturn_mutex);
7221 status = _nfs4_do_setlk(state, cmd, request, NFS_LOCK_NEW);
7222 out:
7223 request->fl_flags = fl_flags;
7224 return status;
7225 }
7226
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]
Patch is wrong,
I missed that FL_FLOCK is handled here too,
moreover locks_lock_inode_wait() is used in nfs_proc_lock and nfs3_proc_locks,
and FL_POSIX request for nfs v2 and v3 can be blocked too.
On 27.12.2021 18:51, 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 blocked on such requests.
> To handle such requests properly, non-blocking posix_file_lock()
> function should be used instead.
>
> https://bugzilla.kernel.org/show_bug.cgi?id=215383
> Signed-off-by: Vasily Averin <[email protected]>
> ---
> VvS: I'm not sure that request->fl_file points to the same state->inode
> used in locks_lock_inode_wait(). If it is not, posix_lock_inode() can be
> used here, but this function is static currently and should be exported first.
>
> v2: fixed 'fl_flags && FL_SLEEP' => 'fl_flags & FL_SLEEP'
> ---
> fs/nfs/nfs4proc.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index ee3bc79f6ca3..f899f4bcdae5 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -7200,8 +7200,11 @@ static int _nfs4_proc_setlk(struct nfs4_state *state, int cmd, struct file_lock
> int status;
>
> request->fl_flags |= FL_ACCESS;
> - status = locks_lock_inode_wait(state->inode, request);
> - if (status < 0)
> + if ((request->fl_flags & FL_SLEEP) && IS_SETLK(cmd))
> + status = posix_lock_file(request->fl_file, request, NULL);
> + else
> + status = locks_lock_inode_wait(state->inode, request);
> + if (status)
> goto out;
> mutex_lock(&sp->so_delegreturn_mutex);
> down_read(&nfsi->rwsem);
>
nfsd and lockd use F_SETLK cmd with FL_SLEEP and FL_POSIX flags set
to request asynchronous processing of blocking locks.
Currently nfs mounted with 'local_lock' option use locks_lock_file_wait()
function which is blocked for such requests. To handle them correctly
non-blocking posix_file_lock() function should be used instead.
https://bugzilla.kernel.org/show_bug.cgi?id=215383
Signed-off-by: Vasily Averin <[email protected]>
---
fs/nfs/file.c | 5 ++++-
include/linux/fs.h | 1 +
2 files changed, 5 insertions(+), 1 deletion(-)
diff --git a/fs/nfs/file.c b/fs/nfs/file.c
index 24e7dccce355..83cf42cabe41 100644
--- a/fs/nfs/file.c
+++ b/fs/nfs/file.c
@@ -753,6 +753,7 @@ static int
do_setlk(struct file *filp, int cmd, struct file_lock *fl, int is_local)
{
struct inode *inode = filp->f_mapping->host;
+ unsigned int flags = fl->fl_flags;
int status;
/*
@@ -769,9 +770,11 @@ do_setlk(struct file *filp, int cmd, struct file_lock *fl, int is_local)
*/
if (!is_local)
status = NFS_PROTO(inode)->lock(filp, cmd, fl);
+ else if (((flags & FL_SLEEP_POSIX) == FL_SLEEP_POSIX) && IS_SETLK(cmd))
+ status = posix_lock_file(filp, fl, NULL);
else
status = locks_lock_file_wait(filp, fl);
- if (status < 0)
+ if (status)
goto out;
/*
diff --git a/include/linux/fs.h b/include/linux/fs.h
index bbf812ce89a8..8b6e9332b39f 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1042,6 +1042,7 @@ static inline struct file *get_file(struct file *f)
#define FL_RECLAIM 4096 /* reclaiming from a reboot server */
#define FL_CLOSE_POSIX (FL_POSIX | FL_CLOSE)
+#define FL_SLEEP_POSIX (FL_POSIX | FL_SLEEP)
/*
* Special return value from posix_lock_file() and vfs_lock_file() for
--
2.25.1
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 <[email protected]>
---
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
nfsd and lockd use F_SETLK cmd with the FL_SLEEP flag set to request
asynchronous processing of blocking locks.
Currently nfs v2/3 handles such requests by using nlmclnt_lock() ->
do_vfs_lock() -> locks_lock_inode_wait() function which is blocked
if request have FL_SLEEP flag set.
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 <[email protected]>
---
fs/lockd/clntproc.c | 14 ++++++++++++--
1 file changed, 12 insertions(+), 2 deletions(-)
diff --git a/fs/lockd/clntproc.c b/fs/lockd/clntproc.c
index 99fffc9cb958..5941aa7c9cc9 100644
--- a/fs/lockd/clntproc.c
+++ b/fs/lockd/clntproc.c
@@ -519,11 +519,18 @@ nlmclnt_lock(struct nlm_rqst *req, struct file_lock *fl)
unsigned char fl_flags = fl->fl_flags;
unsigned char fl_type;
int status = -ENOLCK;
+ bool async = false;
if (nsm_monitor(host) < 0)
goto out;
req->a_args.state = nsm_local_state;
+ async = !req->a_args.block &&
+ ((fl_flags & FL_SLEEP_POSIX) == FL_SLEEP_POSIX);
+ if (async) {
+ fl->fl_flags &= ~FL_SLEEP;
+ req->a_args.block = 1;
+ }
fl->fl_flags |= FL_ACCESS;
status = do_vfs_lock(fl);
fl->fl_flags = fl_flags;
@@ -573,8 +580,11 @@ nlmclnt_lock(struct nlm_rqst *req, struct file_lock *fl)
up_read(&host->h_rwsem);
goto again;
}
- /* Ensure the resulting lock will get added to granted list */
- fl->fl_flags |= FL_SLEEP;
+ if (async)
+ fl->fl_flags &= ~FL_SLEEP;
+ else
+ /* Ensure the resulting lock will get added to granted list */
+ fl->fl_flags |= FL_SLEEP;
if (do_vfs_lock(fl) < 0)
printk(KERN_WARNING "%s: VFS is out of sync with lock manager!\n", __func__);
up_read(&host->h_rwsem);
--
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.
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 <[email protected]>
> ---
> 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 <[email protected]>
> ---
> 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
Hi Vasily,
url: https://github.com/0day-ci/linux/commits/Vasily-Averin/nfs4-handle-async-processing-of-F_SETLK-with-FL_SLEEP/20211227-184632
base: git://git.linux-nfs.org/projects/trondmy/linux-nfs.git linux-next
config: i386-randconfig-m021-20211227 (https://download.01.org/0day-ci/archive/20211228/[email protected]/config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
Reported-by: Dan Carpenter <[email protected]>
New smatch warnings:
fs/nfs/nfs4proc.c:7202 _nfs4_proc_setlk() warn: should this be a bitwise op?
vim +7202 fs/nfs/nfs4proc.c
^1da177e4c3f41 Linus Torvalds 2005-04-16 7194 static int _nfs4_proc_setlk(struct nfs4_state *state, int cmd, struct file_lock *request)
^1da177e4c3f41 Linus Torvalds 2005-04-16 7195 {
19e03c570e6099 Trond Myklebust 2008-12-23 7196 struct nfs_inode *nfsi = NFS_I(state->inode);
11476e9dec39d9 Chuck Lever 2016-04-11 7197 struct nfs4_state_owner *sp = state->owner;
01c3b861cd77b2 Trond Myklebust 2006-06-29 7198 unsigned char fl_flags = request->fl_flags;
1ea67dbd982827 Jeff Layton 2016-09-17 7199 int status;
^1da177e4c3f41 Linus Torvalds 2005-04-16 7200
01c3b861cd77b2 Trond Myklebust 2006-06-29 7201 request->fl_flags |= FL_ACCESS;
7ae55d384b2f33 Vasily Averin 2021-12-27 @7202 if ((request->fl_flags && FL_SLEEP) && IS_SETLK(cmd))
^^
Same thing but for nfsv4.
7ae55d384b2f33 Vasily Averin 2021-12-27 7203 status = posix_lock_file(request->fl_file, request, NULL);
7ae55d384b2f33 Vasily Averin 2021-12-27 7204 else
75575ddf29cbbf Jeff Layton 2016-09-17 7205 status = locks_lock_inode_wait(state->inode, request);
7ae55d384b2f33 Vasily Averin 2021-12-27 7206 if (status)
01c3b861cd77b2 Trond Myklebust 2006-06-29 7207 goto out;
11476e9dec39d9 Chuck Lever 2016-04-11 7208 mutex_lock(&sp->so_delegreturn_mutex);
19e03c570e6099 Trond Myklebust 2008-12-23 7209 down_read(&nfsi->rwsem);
01c3b861cd77b2 Trond Myklebust 2006-06-29 7210 if (test_bit(NFS_DELEGATED_STATE, &state->flags)) {
01c3b861cd77b2 Trond Myklebust 2006-06-29 7211 /* Yes: cache locks! */
01c3b861cd77b2 Trond Myklebust 2006-06-29 7212 /* ...but avoid races with delegation recall... */
01c3b861cd77b2 Trond Myklebust 2006-06-29 7213 request->fl_flags = fl_flags & ~FL_SLEEP;
75575ddf29cbbf Jeff Layton 2016-09-17 7214 status = locks_lock_inode_wait(state->inode, request);
9a99af494bd714 Trond Myklebust 2013-02-04 7215 up_read(&nfsi->rwsem);
11476e9dec39d9 Chuck Lever 2016-04-11 7216 mutex_unlock(&sp->so_delegreturn_mutex);
9a99af494bd714 Trond Myklebust 2013-02-04 7217 goto out;
9a99af494bd714 Trond Myklebust 2013-02-04 7218 }
19e03c570e6099 Trond Myklebust 2008-12-23 7219 up_read(&nfsi->rwsem);
11476e9dec39d9 Chuck Lever 2016-04-11 7220 mutex_unlock(&sp->so_delegreturn_mutex);
c69899a17ca483 Trond Myklebust 2015-01-24 7221 status = _nfs4_do_setlk(state, cmd, request, NFS_LOCK_NEW);
01c3b861cd77b2 Trond Myklebust 2006-06-29 7222 out:
01c3b861cd77b2 Trond Myklebust 2006-06-29 7223 request->fl_flags = fl_flags;
^1da177e4c3f41 Linus Torvalds 2005-04-16 7224 return status;
^1da177e4c3f41 Linus Torvalds 2005-04-16 7225 }
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]
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 <[email protected]>
>> ---
>> 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 <[email protected]>
> >> ---
> >> 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