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

[Dominique Martinet <asmadeus@codewreck.org>]

Vasily Averin wrote on Thu, Dec 23, 2021 at 09:21:23PM +0300:
> kernel export thread (nfsd/lockd/ksmbd) uses F_SETLK cmd with the FL_SLEEP
> flag set to request asynchronous processing of blocking locks.
> 
> Currently v9fs does not support such requests and calls blocking
> locks_lock_file_wait() function.

There's two stages to 9p locks: the client first tries to get the lock
locally on the client, then if it was obtained locally also tries to get
it on the server.
I believe the servers should just ignores flags like FL_SLEEP they don't
know about, so we need to translate it as well if required.

> To work around the problem let's detect such request, drop FL_SLEEP
> before execution of potentially blocking functions.

I'm not up to date with lock mechanisms, could you confirm I understand
the flags right?
- F_SETLK: tries to lock, on conflict return immediately with error
- F_SETLKW|FL_SLEEP: tries to lock, on conflict wait for lock to become available
- F_SETLKW: not possible through flock/fcntl setlk, can happen otherwise?
but for 9p purpose same as above.
- F_SETLK|FL_SLEEP: tries to lock, on conflict ????? you'd want it to
return immediately but setup some callback to be woken up? how could
that work without passing some wake up struct? or just behave as plain
F_SETLK? but then FL_SLEEP has no purpose, I don't get it.


> 
> Dropped FL_SLEEP flag should be restored back because some calling
> function (nfsd4_lock) require it.
> 
> https://bugzilla.kernel.org/show_bug.cgi?id=215383
> Signed-off-by: Vasily Averin <vvs@virtuozzo.com>
> ---
>  fs/9p/vfs_file.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/fs/9p/vfs_file.c b/fs/9p/vfs_file.c
> index 612e297f3763..81c98afdbb32 100644
> --- a/fs/9p/vfs_file.c
> +++ b/fs/9p/vfs_file.c
> @@ -135,6 +135,7 @@ static int v9fs_file_do_lock(struct file *filp, int cmd, struct file_lock *fl)
>  	int res = 0;
>  	unsigned char fl_type;
>  	struct v9fs_session_info *v9ses;
> +	bool async = false;
>  
>  	fid = filp->private_data;
>  	BUG_ON(fid == NULL);
> @@ -142,6 +143,10 @@ static int v9fs_file_do_lock(struct file *filp, int cmd, struct file_lock *fl)
>  	if ((fl->fl_flags & FL_POSIX) != FL_POSIX)
>  		BUG();
>  
> +	async = (fl->fl_flags & FL_SLEEP) && IS_SETLK(cmd);
> +	if (async)
> +		fl->fl_flags &= ~FL_SLEEP;
> +

So clearing the flag makes the local lock not wait at all in case of
SETLK|FL_SLEEP, and this errors instead.

I can't comment on this without understanding what's expected better

>  	res = locks_lock_file_wait(filp, fl);
>  	if (res < 0)
>  		goto out;
> @@ -230,6 +235,8 @@ static int v9fs_file_do_lock(struct file *filp, int cmd, struct file_lock *fl)
>  	if (flock.client_id != fid->clnt->name)
>  		kfree(flock.client_id);
>  out:
> +	if (async)
> +		fl->fl_flags |= FL_SLEEP;
>  	return res;
>  }
>  

-- 
Dominique