Re: [Cluster-devel] [PATCH 2/7] lockd: don't call vfs_lock_file() for pending requests

[Jeff Layton <jlayton@kernel.org>]

On Wed, 2023-08-23 at 17:33 -0400, Alexander Aring wrote:
> This patch returns nlm_lck_blocked in nlmsvc_lock() when an asynchronous
> lock request is pending. During testing I ran into the case with the
> side-effects that lockd is waiting for only one lm_grant() callback
> because it's already part of the nlm_blocked list. If another
> asynchronous for the same nlm_block is triggered two lm_grant()
> callbacks will occur but lockd was only waiting for one.
> 
> To avoid any change of existing users this handling will only being made
> when export_op_support_safe_async_lock() returns true.
> 
> Signed-off-by: Alexander Aring <aahringo@redhat.com>
> ---
>  fs/lockd/svclock.c | 24 +++++++++++++++++-------
>  1 file changed, 17 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/lockd/svclock.c b/fs/lockd/svclock.c
> index 6e3b230e8317..aa4174fbaf5b 100644
> --- a/fs/lockd/svclock.c
> +++ b/fs/lockd/svclock.c
> @@ -531,6 +531,23 @@ nlmsvc_lock(struct svc_rqst *rqstp, struct nlm_file *file,
>  		goto out;
>  	}
>  
> +	spin_lock(&nlm_blocked_lock);
> +	/*
> +	 * If this is a lock request for an already pending
> +	 * lock request we return nlm_lck_blocked without calling
> +	 * vfs_lock_file() again. Otherwise we have two pending
> +	 * requests on the underlaying ->lock() implementation but
> +	 * only one nlm_block to being granted by lm_grant().
> +	 */
> +	if (export_op_support_safe_async_lock(inode->i_sb->s_export_op,
> +					      nlmsvc_file_file(file)->f_op) &&
> +	    !list_empty(&block->b_list)) {
> +		spin_unlock(&nlm_blocked_lock);
> +		ret = nlm_lck_blocked;
> +		goto out;
> +	}

Looks reasonable. The block->b_list check is subtle, but the comment
helps.

> +	spin_unlock(&nlm_blocked_lock);
> +
>  	if (!wait)
>  		lock->fl.fl_flags &= ~FL_SLEEP;
>  	mode = lock_to_openmode(&lock->fl);
> @@ -543,13 +560,6 @@ nlmsvc_lock(struct svc_rqst *rqstp, struct nlm_file *file,
>  			ret = nlm_granted;
>  			goto out;
>  		case -EAGAIN:
> -			/*
> -			 * If this is a blocking request for an
> -			 * already pending lock request then we need
> -			 * to put it back on lockd's block list
> -			 */
> -			if (wait)
> -				break;
>  			ret = async_block ? nlm_lck_blocked : nlm_lck_denied;
>  			goto out;
>  		case FILE_LOCK_DEFERRED:


Reviewed-by: Jeff Layton <jlayton@kernel.org>