[PATCH] nfs4: handle async processing of F_SETLK with FL_SLEEP

[PATCH] nfs4: handle async processing of F_SETLK with FL_SLEEP

[Vasily Averin]

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 <vvs@virtuozzo.com>
---
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

[PATCH v2] nfs4: handle async processing of F_SETLK with FL_SLEEP

[Vasily Averin]

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 <vvs@virtuozzo.com>
---
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

Re: [PATCH v2] nfs4: handle async processing of F_SETLK with FL_SLEEP

[Vasily Averin]

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 <vvs@virtuozzo.com>
> ---
> 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);
> 

[PATCH v3 1/3] nfs: local_lock: handle async processing of F_SETLK with FL_SLEEP

[Vasily Averin]

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 <vvs@virtuozzo.com>
---
 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

[PATCH v3 2/3] nfs4: handle async processing of F_SETLK with FL_SLEEP

[Vasily Averin]

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(-)

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

Re: [PATCH v3 2/3] nfs4: handle async processing of F_SETLK with FL_SLEEP

[J. Bruce Fields]

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

Re: [PATCH v3 2/3] nfs4: handle async processing of F_SETLK with FL_SLEEP

[Vasily Averin]

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

Re: [PATCH v3 2/3] nfs4: handle async processing of F_SETLK with FL_SLEEP

[J. Bruce Fields]

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

Re: [PATCH v3 2/3] nfs4: handle async processing of F_SETLK with FL_SLEEP

[Vasily Averin]

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

Re: [PATCH v3 2/3] nfs4: handle async processing of F_SETLK with FL_SLEEP

[Vasily Averin]

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
> 

Re: [PATCH v3 2/3] nfs4: handle async processing of F_SETLK with FL_SLEEP

[J. Bruce Fields]

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

[PATCH v3 3/3] nfs v2/3: nlmclnt_lock: handle async processing of F_SETLK with FL_SLEEP

[Vasily Averin]

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 <vvs@virtuozzo.com>
---
 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

Re: [PATCH] nfs4: handle async processing of F_SETLK with FL_SLEEP

[kernel test robot]

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/202112280146.yo82FThq-lkp@intel.com/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 <lkp@intel.com>

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/kbuild-all@lists.01.org

Re: [PATCH] nfs4: handle async processing of F_SETLK with FL_SLEEP

[Dan Carpenter]

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/202112281638.RNX7e40X-lkp@intel.com/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 <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>

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/kbuild-all@lists.01.org