Re: [RFCv2 2/7] lockd: FILE_LOCK_DEFERRED only on FL_SLEEP

[Jeff Layton <jlayton@kernel.org>]

On Mon, 2023-08-14 at 17:11 -0400, Alexander Aring wrote:
> This patch removes to handle non-blocking lock requests as asynchronous
> lock request returning FILE_LOCK_DEFERRED. When fl_lmops and lm_grant()
> is set and a non-blocking lock request returns FILE_LOCK_DEFERRED will
> end in an WARNING to signal the user the misusage of the API.
> 

Probably need to rephrase the word salad in the first sentence of the
commit log. I had to go over it a few times to understand what was going
on here.

In any case, I'm guessing that the idea here is that GFS2/DLM shouldn't
ever return FILE_LOCK_DEFERRED if this is a non-wait request (i.e.
someone called F_SETLK instead of F_SETLKW)?

That may be ok, but again, lockd goes to great lengths to avoid blocking
and I think it's generally a good idea. If an upcall to DLM can take a
long time, it might be a good idea to continue to allow a !wait request
to return FILE_LOCK_DEFERRED.

I guess this really depends on the current behavior today though. Does
DLM ever return FILE_LOCK_DEFERRED on a non-blocking lock request?


> The reason why we moving to make non-blocking lock request as
> synchronized call is that we already doing this behaviour for unlock or
> cancellation as well. Those are POSIX lock operations which are handled
> in an synchronized way and waiting for an answer. For non-blocking lock
> requests the answer will probably arrive in the same time as unlock or
> cancellation operations as those are trylock operations only.
> 
> In case of a blocking lock request we need to have it asynchronously
> because the time when the lock request getting granted is unknown.
> 
> Signed-off-by: Alexander Aring <aahringo@redhat.com>
> ---
>  fs/lockd/svclock.c | 39 +++++++--------------------------------
>  1 file changed, 7 insertions(+), 32 deletions(-)
> 
> diff --git a/fs/lockd/svclock.c b/fs/lockd/svclock.c
> index 7d63524bdb81..1e74a578d7de 100644
> --- a/fs/lockd/svclock.c
> +++ b/fs/lockd/svclock.c
> @@ -440,31 +440,6 @@ static void nlmsvc_freegrantargs(struct nlm_rqst *call)
>  	locks_release_private(&call->a_args.lock.fl);
>  }
>  
> -/*
> - * Deferred lock request handling for non-blocking lock
> - */
> -static __be32
> -nlmsvc_defer_lock_rqst(struct svc_rqst *rqstp, struct nlm_block *block)
> -{
> -	__be32 status = nlm_lck_denied_nolocks;
> -
> -	block->b_flags |= B_QUEUED;
> -
> -	nlmsvc_insert_block(block, NLM_TIMEOUT);
> -
> -	block->b_cache_req = &rqstp->rq_chandle;
> -	if (rqstp->rq_chandle.defer) {
> -		block->b_deferred_req =
> -			rqstp->rq_chandle.defer(block->b_cache_req);
> -		if (block->b_deferred_req != NULL)
> -			status = nlm_drop_reply;
> -	}
> -	dprintk("lockd: nlmsvc_defer_lock_rqst block %p flags %d status %d\n",
> -		block, block->b_flags, ntohl(status));
> -
> -	return status;
> -}
> -
>  /*
>   * Attempt to establish a lock, and if it can't be granted, block it
>   * if required.
> @@ -569,14 +544,14 @@ nlmsvc_lock(struct svc_rqst *rqstp, struct nlm_file *file,
>  			ret = async_block ? nlm_lck_blocked : nlm_lck_denied;
>  			goto out_cb_mutex;
>  		case FILE_LOCK_DEFERRED:
> -			block->b_flags |= B_PENDING_CALLBACK;
> +			/* lock requests without waiters are handled in
> +			 * a non async way. Let assert this to inform
> +			 * the user about a API violation.
> +			 */
> +			WARN_ON_ONCE(!wait);
>  
> -			if (wait)
> -				break;
> -			/* Filesystem lock operation is in progress
> -			   Add it to the queue waiting for callback */
> -			ret = nlmsvc_defer_lock_rqst(rqstp, block);
> -			goto out_cb_mutex;
> +			block->b_flags |= B_PENDING_CALLBACK;
> +			break;
>  		case -EDEADLK:
>  			nlmsvc_remove_block(block);
>  			ret = nlm_deadlock;

-- 
Jeff Layton <jlayton@kernel.org>