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@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxx>
