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: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 <[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 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 <[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
>