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 <aahri...@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 <jlay...@kernel.org>

Reply via email to