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

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

[Vasily Averin]

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.

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

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;
+
 	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;
 }
 
-- 
2.25.1

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

[Dominique Martinet]

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

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

[Vasily Averin]

On 24.12.2021 02:14, Dominique Martinet wrote:
> 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.

I apologize in advance for the long answer, but I tried to state all the details
of the detected problem.

Below is description copy-pasted from comment above vfs_lock_file()
"
 * To avoid blocking kernel daemons, such as lockd, that need to acquire POSIX
 * locks, the ->lock() interface may return asynchronously, before the lock has
 * been granted or denied by the underlying filesystem, if (and only if)
 * lm_grant is set. Callers expecting ->lock() to return asynchronously
 * will only use F_SETLK, not F_SETLKW; they will set FL_SLEEP if (and only if)
 * the request is for a blocking lock. When ->lock() does return asynchronously,
 * it must return FILE_LOCK_DEFERRED, and call ->lm_grant() when the lock
 * request completes.
 * If the request is for non-blocking lock the file system should return
 * FILE_LOCK_DEFERRED then try to get the lock and call the callback routine
 * with the result. If the request timed out the callback routine will return a
 * nonzero return code and the file system should release the lock. The file
 * system is also responsible to keep a corresponding posix lock when it
 * grants a lock so the VFS can find out which locks are locally held and do
 * the correct lock cleanup when required.
"

lockd used by nfs v2 and v3 was first kernel daemon processed locking requests.
For nfs v4 locking requests are handled by nfsd thread. Recently ksmbd was added
into kernel, it also handles locking requests.

They all are servers, and they can receive blocking lock requests from own clients.
They all cannot process such requests synchronously because it causes server deadlock.
In simplest form, if single threaded nfsd is blocked on processing such request,
whole nfs server cannot process any other commands.

To avoid this deadlock, kernel threads are forced to use F_SETLK with FL_SLEEP,
to ask callers to process such request asynchronously or if it is impossible to
do not block such requests.

Most part of file systems do not have own ->lock functions, in this case vfs_lock_file()
calls posix_lock_file(), which correctly handles such requests.

However some other file systems, i.e. ones defined own ->lock function,
incorrectly handles such requests and can block on its processing.
For example v9fs calls blocking locks_lock_file_wait() function.

It is not "pure theoretic" issue.
One of our customers tried to export fuse/glusters via nfsd and reported about
memory corruption in nfsd4_lock() function. We found few related bugs in nfsd,
however finally we noticed that fuse blocks on processing such requests. 
During investigation we have found that fuse just ignored F_SETLK command,
handled FL_SLEEP flag and submitted blocking FUSE_SETLKW command.
Similar troubles was found in some other file systems defines own ->lock() function.

To work around the problem nfsd maintainer decided to drop FL_SLEEP flag if
exported file system defines own ->lock() function.

Then I've found the problem affects recently added ksmbd and patched it by the same way.

To resolve this problem completely I've created https://bugzilla.kernel.org/show_bug.cgi?id=215383
where I'm trying to handle the problem in all affected file systems.
When this will be done kernel export threads can revert own work around with dropped FL_SLEEP flag.

Answering on you question: it's ok to ignore of FL_SLEEP flag for F_SETLK command,
It would be even better to use posix_lock_file() instead of locks_lock_file_wait(),
but I cannot do it without your assistance.

Thank you,
	Vasily Averin

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

[Dominique Martinet]

Vasily Averin wrote on Fri, Dec 24, 2021 at 10:08:57AM +0300:
> > 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.
> 
> I apologize in advance for the long answer, but I tried to state all the details
> of the detected problem.
> 
> Below is description copy-pasted from comment above vfs_lock_file()

Thanks, I hadn't noticed this comment this morning.

> "
>  * To avoid blocking kernel daemons, such as lockd, that need to acquire POSIX
>  * locks, the ->lock() interface may return asynchronously, before the lock has
>  * been granted or denied by the underlying filesystem, if (and only if)
>  * lm_grant is set. Callers expecting ->lock() to return asynchronously
>  * will only use F_SETLK, not F_SETLKW; they will set FL_SLEEP if (and only if)
>  * the request is for a blocking lock. When ->lock() does return asynchronously,
>  * it must return FILE_LOCK_DEFERRED, and call ->lm_grant() when the lock
>  * request completes.

Ok so that's the part I was missing.
The file_lock struct will have fl_lmops->lm_grant set in this case and
we just need to remember that and call lm_grant when the lock has been set...

> They all are servers, and they can receive blocking lock requests from own clients.
> They all cannot process such requests synchronously because it causes server deadlock.
> In simplest form, if single threaded nfsd is blocked on processing such request,
> whole nfs server cannot process any other commands.

Note 9p does not have an fh_to_dentry op (no open by handle type of
calls, the protocol just has no way of dealing with it), so I believe
knfsd cannot re-export 9p filesystems and wouldn't be surprised if
others can't either -- as thus this all might not be an issue for you if
F_SETLK|FL_SLEEP users all are such servers


> One of our customers tried to export fuse/glusters via nfsd and reported about
> memory corruption in nfsd4_lock() function. We found few related bugs in nfsd,
> however finally we noticed that fuse blocks on processing such requests. 
> During investigation we have found that fuse just ignored F_SETLK command,
> handled FL_SLEEP flag and submitted blocking FUSE_SETLKW command.

I'm not sure I understand how upgrading to SETLKW is worse than dropping
the FL_SLEEP flag (I mean, I see it's bad as it wasn't what the server
expects, but while it will block a thread for an undefined period of
time and may cause deadlocks I don't see how it would cause memory
corruptions?)

> Answering on you question: it's ok to ignore of FL_SLEEP flag for F_SETLK command,

On the other hand, just clearing the FL_SLEEP flag like you've done for
9p will make the server think the lock has been queued when it hasn't
really been.
That means the client lock request will hang forever and never be
granted even when the lock becomes available later on, so unless I
misunderstood something here I don't think that's a reasonable fallback.
So...

> It would be even better to use posix_lock_file() instead of locks_lock_file_wait(),
> but I cannot do it without your assistance.

let's try to fix this properly instead, I'm happy to help.

Basically 9p does things in two steps:
 - first it tries to get the lock locally at the vfs level.
I'm not familiar with all the locking helpers we have at disposal, but
as long as the distinction between flock and posix locks is kept I'm
happy with anything here.

If that process is made asynchronous, we need a way to run more
9p-specific code in that one's lm_grant callback, so we can proceed onto
the second step which is...

 - send the lock request to the 9p server and wait for its reply
(note that the current code is always synchronous here: even if you
request SETLK without the SLEEP flag you can be made to wait here.
I have work in the closest to make some requests asynchronous, so
locking could be made asynchronous when that lands, but my code
introduced a race somewhere I haven't had the time to fix so this
improvement will come later)



What would you suggest with that?


-- 
Dominique

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

[Vasily Averin]

On 24.12.2021 10:31, Dominique Martinet wrote:
> Vasily Averin wrote on Fri, Dec 24, 2021 at 10:08:57AM +0300:
>>> 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.
>>
>> I apologize in advance for the long answer, but I tried to state all the details
>> of the detected problem.
>>
>> Below is description copy-pasted from comment above vfs_lock_file()
> 
> Thanks, I hadn't noticed this comment this morning.
> 
>> "
>>  * To avoid blocking kernel daemons, such as lockd, that need to acquire POSIX
>>  * locks, the ->lock() interface may return asynchronously, before the lock has
>>  * been granted or denied by the underlying filesystem, if (and only if)
>>  * lm_grant is set. Callers expecting ->lock() to return asynchronously
>>  * will only use F_SETLK, not F_SETLKW; they will set FL_SLEEP if (and only if)
>>  * the request is for a blocking lock. When ->lock() does return asynchronously,
>>  * it must return FILE_LOCK_DEFERRED, and call ->lm_grant() when the lock
>>  * request completes.
> 
> Ok so that's the part I was missing.
> The file_lock struct will have fl_lmops->lm_grant set in this case and
> we just need to remember that and call lm_grant when the lock has been set...

Unfortunately it isn't enough.
lm_grant is defined by lockd only, but nfsd and ksmbd does not use it.

Most of file systems does not use lm_grant, and it's OK.
2 file systems --  gfs2 and ocfs -- use lm_grant via dlm_posix_lock(),
which works correctly even if lm_grant is not defined.

>> They all are servers, and they can receive blocking lock requests from own clients.
>> They all cannot process such requests synchronously because it causes server deadlock.
>> In simplest form, if single threaded nfsd is blocked on processing such request,
>> whole nfs server cannot process any other commands.
> 
> Note 9p does not have an fh_to_dentry op (no open by handle type of
> calls, the protocol just has no way of dealing with it), so I believe
> knfsd cannot re-export 9p filesystems and wouldn't be surprised if
> others can't either -- as thus this all might not be an issue for you if
> F_SETLK|FL_SLEEP users all are such servers

Perhaps you are right.
I'm not qualified enough to confirm it, but I tend to agree with you: without defined export_op
nfsd cannot export your file system.
However:
1) perhaps this issue can be triggered via ksmbd
2) you can add export_op in the future and forget about this problem.
3) proposed changes are minimal and does not affect any other use cases.

>> One of our customers tried to export fuse/glusters via nfsd and reported about
>> memory corruption in nfsd4_lock() function. We found few related bugs in nfsd,
>> however finally we noticed that fuse blocks on processing such requests. 
>> During investigation we have found that fuse just ignored F_SETLK command,
>> handled FL_SLEEP flag and submitted blocking FUSE_SETLKW command.
> 
> I'm not sure I understand how upgrading to SETLKW is worse than dropping
> the FL_SLEEP flag (I mean, I see it's bad as it wasn't what the server
> expects, but while it will block a thread for an undefined period of
> time and may cause deadlocks I don't see how it would cause memory
> corruptions?)

kernel threads cannot use blocking SETLKW. 
SETLKW can be safely used by usual processes, if they want to wait until lock
will be captured. However it is not an option for server with limited number
of working threads.

Memory corruption was internal nfsd issues triggered by very long processing
of blocking lock request in fuse.  

I'm sorry, I'll answer rest of  questions later.

Thank you,	Vasily Averin

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

[Vasily Averin]

On 24.12.2021 10:31, Dominique Martinet wrote:
> Vasily Averin wrote on Fri, Dec 24, 2021 at 10:08:57AM +0300:
>> Answering on you question: it's ok to ignore of FL_SLEEP flag for F_SETLK command,
> 
> On the other hand, just clearing the FL_SLEEP flag like you've done for
> 9p will make the server think the lock has been queued when it hasn't
> really been.
> That means the client lock request will hang forever and never be
> granted even when the lock becomes available later on, so unless I
> misunderstood something here I don't think that's a reasonable fallback.

I did not get your this statement. Could you please elaborate it in more details?

Right now nfsd/lockd/ksmbd drop FL_SLEEP on own side, and this looks acceptable for them:
instead of blocking lock they submit non-blocking SETLK and it's enough to avoid server deadlock. 

If the lock is already taken: SETLK just return an error and will not wait.
I'm agree it isn't ideal, and perhaps  can cause server will return some unexpected errno,
but I do not see how it can make the server think the lock has been queued.

>> It would be even better to use posix_lock_file() instead of locks_lock_file_wait(),
>> but I cannot do it without your assistance.
> 
> let's try to fix this properly instead, I'm happy to help.
> 
> Basically 9p does things in two steps:
>  - first it tries to get the lock locally at the vfs level.
> I'm not familiar with all the locking helpers we have at disposal, but
> as long as the distinction between flock and posix locks is kept I'm
> happy with anything here.
> 
> If that process is made asynchronous, we need a way to run more
> 9p-specific code in that one's lm_grant callback, so we can proceed onto
> the second step which is...
> 
>  - send the lock request to the 9p server and wait for its reply
> (note that the current code is always synchronous here: even if you
> request SETLK without the SLEEP flag you can be made to wait here.
> I have work in the closest to make some requests asynchronous, so
> locking could be made asynchronous when that lands, but my code
> introduced a race somewhere I haven't had the time to fix so this
> improvement will come later)
> 
> What would you suggest with that?

It seems we can just replace locks_lock_file_wait() call by posix_lock_file()
in described scenario. I'll sent v2 patch version soon.

Thank you,
	Vasily Averin

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

[Vasily Averin]

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.

To work around the problem let's detect such request and call
non-blocking posix_file_lock() instead of locks_lock_file_wait().

https://bugzilla.kernel.org/show_bug.cgi?id=215383
Signed-off-by: Vasily Averin <vvs@virtuozzo.com>
---
 fs/9p/vfs_file.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/fs/9p/vfs_file.c b/fs/9p/vfs_file.c
index 612e297f3763..27ede4a4a6f4 100644
--- a/fs/9p/vfs_file.c
+++ b/fs/9p/vfs_file.c
@@ -142,10 +142,15 @@ static int v9fs_file_do_lock(struct file *filp, int cmd, struct file_lock *fl)
 	if ((fl->fl_flags & FL_POSIX) != FL_POSIX)
 		BUG();
 
-	res = locks_lock_file_wait(filp, fl);
-	if (res < 0)
-		goto out;
-
+	if ((fl->fl_flags & FL_SLEEP) && IS_SETLK(cmd)) {
+		res = posix_lock_file(filp, fl, NULL);
+		if (res)
+			goto out;
+	} else {
+		res = locks_lock_file_wait(filp, fl);
+		if (res < 0)
+			goto out;
+	}
 	/* convert posix lock to p9 tlock args */
 	memset(&flock, 0, sizeof(flock));
 	/* map the lock type */
-- 
2.25.1

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

[Dominique Martinet]

Vasily Averin wrote on Fri, Dec 24, 2021 at 01:18:54PM +0300:
> > On the other hand, just clearing the FL_SLEEP flag like you've done for
> > 9p will make the server think the lock has been queued when it hasn't
> > really been.
> > That means the client lock request will hang forever and never be
> > granted even when the lock becomes available later on, so unless I
> > misunderstood something here I don't think that's a reasonable fallback.
> 
> I did not get your this statement. Could you please elaborate it in more details?
> 
> Right now nfsd/lockd/ksmbd drop FL_SLEEP on own side, and this looks acceptable for them:
> instead of blocking lock they submit non-blocking SETLK and it's enough to avoid server deadlock. 
> 
> If the lock is already taken: SETLK just return an error and will not wait.
> I'm agree it isn't ideal, and perhaps  can cause server will return some unexpected errno,
> but I do not see how it can make the server think the lock has been queued.

Right, sorry I was still under the assumption that SETLK+sleep would
return error + enqueue something.
Reading again it must return FILE_LOCK_DEFERRED if enqueued, so the
server can make the difference, and we're "just" not respecting the
client's request to enqueue the lock as you say -- I guess that's
acceptable.

> diff --git a/fs/9p/vfs_file.c b/fs/9p/vfs_file.c
> index 612e297f3763..27ede4a4a6f4 100644
> --- a/fs/9p/vfs_file.c
> +++ b/fs/9p/vfs_file.c
> @@ -142,10 +142,15 @@ static int v9fs_file_do_lock(struct file *filp, int cmd, struct file_lock *fl)
>  	if ((fl->fl_flags & FL_POSIX) != FL_POSIX)
>  		BUG();
>  
> -	res = locks_lock_file_wait(filp, fl);
> -	if (res < 0)
> -		goto out;
> -
> +	if ((fl->fl_flags & FL_SLEEP) && IS_SETLK(cmd)) {
> +		res = posix_lock_file(filp, fl, NULL);

Should we also check fl->fl_flags & (FL_POSIX|FL_FLOCK) like
locks_lock_file_wait does, to call either posix_lock_file or ... there
doesn't seem to be an exported flock_lock_file equivalent in the other
case, so back to wait variant there?
(or rephrasing the question, what happens if the lock is a FL_FLOCK lock
and we call posix_lock_file on it? Or are we guaranted that if FL_SLEEP
is set we're about posix locks?)

> +		if (res)
> +			goto out;
> +	} else {
> +		res = locks_lock_file_wait(filp, fl);
> +		if (res < 0)
> +			goto out;
> +	}
>  	/* convert posix lock to p9 tlock args */
>  	memset(&flock, 0, sizeof(flock));
>  	/* map the lock type */

Vasily Averin wrote on Fri, Dec 24, 2021 at 03:07:38PM +0300:
 > On 24.12.2021 10:31, Dominique Martinet wrote:
> > If that process is made asynchronous, we need a way to run more
> > 9p-specific code in that one's lm_grant callback, so we can proceed onto
> > the second step which is...
> > 
> >  - send the lock request to the 9p server and wait for its reply
> > (note that the current code is always synchronous here: even if you
> > request SETLK without the SLEEP flag you can be made to wait here.
> > I have work in the closest to make some requests asynchronous, so
> > locking could be made asynchronous when that lands, but my code
> > introduced a race somewhere I haven't had the time to fix so this
> > improvement will come later)
> > 
> > What would you suggest with that?
> 
> It isn't necessary to make request asynchronous,
> it's enough to avoid blocking locks.

Well, it depends on what you have in mind with blocking.
A synchronous RPC to some server which might never reply if it doesn't
feel like it (bug or whatever) is very much blocking in my opinion.

> As far as  I understand blocking does not happen for SETLK command,
> so it should be enough to chenge first part and call non-blocking 
> posix_file_lock() instead of blocking locks_lock_file_wait().
> 
> It would be great to make processing of 2nd part asynchronous too,
> but I think it looks like over-engineering, because we even are not
> 100% sure that someone really uses this functionality.

Yes, it will have to wait anyway, that asynchronous code has been
waiting for me to debug it for at least two years so it won't be ready
tomorrow... Let's start with what we can do; I'm happy with your
approach if it doesn't bring obvious problems.

-- 
Dominique

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

[Vasily Averin]

On 24.12.2021 17:56, Dominique Martinet wrote:
> Vasily Averin wrote on Fri, Dec 24, 2021 at 01:18:54PM +0300:

>> diff --git a/fs/9p/vfs_file.c b/fs/9p/vfs_file.c
>> index 612e297f3763..27ede4a4a6f4 100644
>> --- a/fs/9p/vfs_file.c
>> +++ b/fs/9p/vfs_file.c
>> @@ -142,10 +142,15 @@ static int v9fs_file_do_lock(struct file *filp, int cmd, struct file_lock *fl)
>>  	if ((fl->fl_flags & FL_POSIX) != FL_POSIX)
>>  		BUG();
>>  
>> -	res = locks_lock_file_wait(filp, fl);
>> -	if (res < 0)
>> -		goto out;
>> -
>> +	if ((fl->fl_flags & FL_SLEEP) && IS_SETLK(cmd)) {
>> +		res = posix_lock_file(filp, fl, NULL);
> 
> Should we also check fl->fl_flags & (FL_POSIX|FL_FLOCK) like
> locks_lock_file_wait does, to call either posix_lock_file or ... there
> doesn't seem to be an exported flock_lock_file equivalent in the other
> case, so back to wait variant there?
> (or rephrasing the question, what happens if the lock is a FL_FLOCK lock
> and we call posix_lock_file on it? Or are we guaranted that if FL_SLEEP
> is set we're about posix locks?)

SETLK with FL_SLEEP flag can be set by kernel export threads for posix locks only.

> Vasily Averin wrote on Fri, Dec 24, 2021 at 03:07:38PM +0300:
>  > On 24.12.2021 10:31, Dominique Martinet wrote:
>> It isn't necessary to make request asynchronous,
>> it's enough to avoid blocking locks.
> 
> Well, it depends on what you have in mind with blocking.
> A synchronous RPC to some server which might never reply if it doesn't
> feel like it (bug or whatever) is very much blocking in my opinion.

The main goal is to avoid deadlock of server threads.
It is acceptable to sleep or process such a request for a very long time,
however it is unacceptable to wait for another command to remove our lock,
because there can be no available working threads to process this command:
all of them can be busy by processing of blocking locks. 
Thank you,
	Vasily Averin

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

[Dominique Martinet]

Vasily Averin wrote on Mon, Dec 27, 2021 at 10:42:48AM +0300:
> > Should we also check fl->fl_flags & (FL_POSIX|FL_FLOCK) like
> > locks_lock_file_wait does, to call either posix_lock_file or ... there
> > doesn't seem to be an exported flock_lock_file equivalent in the other
> > case, so back to wait variant there?
> > (or rephrasing the question, what happens if the lock is a FL_FLOCK lock
> > and we call posix_lock_file on it? Or are we guaranted that if FL_SLEEP
> > is set we're about posix locks?)
> 
> SETLK with FL_SLEEP flag can be set by kernel export threads for posix locks only.

I see.
Would it make sense to add a WARN_ON or similar in case that ever
changes in the future?

I'd be more comfortable with one given it'd be quite a sneaky bug when
it does (locks will still appear to work, just wrong kind of locks...).
I assume that if it ever does all filesystems will be reviewed for
it... But sometimes it's best to make sure.

> > Well, it depends on what you have in mind with blocking.
> > A synchronous RPC to some server which might never reply if it doesn't
> > feel like it (bug or whatever) is very much blocking in my opinion.
> 
> The main goal is to avoid deadlock of server threads.
> It is acceptable to sleep or process such a request for a very long time,
> however it is unacceptable to wait for another command to remove our lock,
> because there can be no available working threads to process this command:
> all of them can be busy by processing of blocking locks. 

Ok, that makes sense to me.

I'm happy with the current patch except for my paranoia on that fcntl
lock check, let me know what you think about it and I'll apply either
this or a new version.

Thanks!
-- 
Dominique

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

[Vasily Averin]

On 24.12.2021 10:31, Dominique Martinet wrote:
> If that process is made asynchronous, we need a way to run more
> 9p-specific code in that one's lm_grant callback, so we can proceed onto
> the second step which is...
> 
>  - send the lock request to the 9p server and wait for its reply
> (note that the current code is always synchronous here: even if you
> request SETLK without the SLEEP flag you can be made to wait here.
> I have work in the closest to make some requests asynchronous, so
> locking could be made asynchronous when that lands, but my code
> introduced a race somewhere I haven't had the time to fix so this
> improvement will come later)
> 
> What would you suggest with that?

It isn't necessary to make  request asynchronous,
it's enough to avoid blocking locks.
As far as  I understand blocking does not happen for SETLK command,
so it should be enough to chenge first part and call non-blocking 
posix_file_lock() instead of blocking locks_lock_file_wait().

It would be great to make processing of 2nd part asynchronous too,
but I think it looks like over-engineering, because we even are not
100% sure that someone really uses this functionality.

Thank you,
	Vasily Averin