Re: [patch 1/9] lockd: dont return EAGAIN for a permanent error

["J. Bruce Fields" <bfields@fieldses.org>]

On Thu, May 15, 2008 at 09:50:20PM +0200, Miklos Szeredi wrote:
> From: Miklos Szeredi <mszeredi@suse.cz>
> 
> Fix nlm_fopen() to return NLM_FAILED (or NLM_LCK_DENIED_NOLOCKS)
> instead of NLM_LCK_DENIED.  The latter means the lock request failed
> because of a conflicting lock (i.e. a temporary error), which is wrong
> in this case.
> 
> Also fix the client to return ENOLCK instead of EAGAIN if a blocking
> lock request returns with NLM_LOCK_DENIED.
> 
> Signed-off-by: Miklos Szeredi <mszeredi@suse.cz>
> CC: Trond Myklebust <trond.myklebust@fys.uio.no>
> CC: "J. Bruce Fields" <bfields@fieldses.org>
> ---
>  fs/lockd/clntproc.c |   10 +++++++++-
>  fs/nfsd/lockd.c     |   13 +++++++++----
>  2 files changed, 18 insertions(+), 5 deletions(-)
> 
> Index: linux-2.6/fs/lockd/clntproc.c
> ===================================================================
> --- linux-2.6.orig/fs/lockd/clntproc.c	2008-05-15 17:54:30.000000000 +0200
> +++ linux-2.6/fs/lockd/clntproc.c	2008-05-15 17:59:41.000000000 +0200
> @@ -580,7 +580,15 @@ again:
>  	}
>  	if (status < 0)
>  		goto out_unlock;
> -	status = nlm_stat_to_errno(resp->status);
> +	/*
> +	 * EAGAIN doesn't make sense for sleeping locks, and in some
> +	 * cases NLM_LCK_DENIED is returned for a permanent error.  So

Just for the sake of future readers, I might go ahead and give
specifics: "and older versions of the linux server sometimes returned
NLM_LCK_DENIED for permanent errors", or something.

> +	 * turn it into an ENOLCK.
> +	 */
> +	if (resp->status == nlm_lck_denied && (fl_flags & FL_SLEEP))
> +		status = -ENOLCK;
> +	else
> +		status = nlm_stat_to_errno(resp->status);

Might be a bit clearer (or at least make the comment and code more
obviously agree) to do:

	status = nlm_stat_to_errno(resp->status);
	if (status == -EAGAIN && (fl_flags & FL_SLEEP))
		status = -ENOLCK;

This might make more sense as separate client and server-side patches.

Seems reasonable to me otherwise.

--b.

>  out_unblock:
>  	nlmclnt_finish_block(block);
>  out:
> Index: linux-2.6/fs/nfsd/lockd.c
> ===================================================================
> --- linux-2.6.orig/fs/nfsd/lockd.c	2008-05-15 17:54:30.000000000 +0200
> +++ linux-2.6/fs/nfsd/lockd.c	2008-05-15 17:57:45.000000000 +0200
> @@ -19,6 +19,13 @@
>  
>  #define NFSDDBG_FACILITY		NFSDDBG_LOCKD
>  
> +#ifdef CONFIG_LOCKD_V4
> +#define nlm_stale_fh	nlm4_stale_fh
> +#define nlm_failed	nlm4_failed
> +#else
> +#define nlm_stale_fh	nlm_lck_denied_nolocks
> +#define nlm_failed	nlm_lck_denied_nolocks
> +#endif
>  /*
>   * Note: we hold the dentry use count while the file is open.
>   */
> @@ -47,12 +54,10 @@ nlm_fopen(struct svc_rqst *rqstp, struct
>  		return 0;
>  	case nfserr_dropit:
>  		return nlm_drop_reply;
> -#ifdef CONFIG_LOCKD_V4
>  	case nfserr_stale:
> -		return nlm4_stale_fh;
> -#endif
> +		return nlm_stale_fh;
>  	default:
> -		return nlm_lck_denied;
> +		return nlm_failed;
>  	}
>  }
>  
> 
> --