Re: [PATCH] nfsd: don't break lease while servicing a COMMIT call (try #2)

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

On Fri, Mar 19, 2010 at 08:06:28AM -0400, Jeff Layton wrote:
> This is the second attempt to fix the problem whereby a COMMIT call
> causes a lease break and triggers a possible deadlock.
> 
> The problem is that nfsd attempts to break a lease on a COMMIT call.
> This triggers a delegation recall if the lease is held for a delegation.
> If the client is the one holding the delegation and it's the same one on
> which it's issuing the COMMIT, then it can't return that delegation
> until the COMMIT is complete. But, nfsd won't complete the COMMIT until
> the delegation is returned. The client and server are essentially
> deadlocked until the state is marked bad (due to the client not
> responding on the callback channel).
> 
> The first patch attempted to deal with this by eliminating the open of
> the file altogether and simply had nfsd_commit pass a NULL file pointer
> to the vfs_fsync_range. That would conflict with some work in progress
> by Christoph Hellwig to clean up the fsync interface, so this patch
> takes a different approach.
> 
> This declares a new NFSD_MAY_NOT_BREAK_LEASE access flag that indicates
> to nfsd_open that it should not break any leases when opening the file,
> and has nfsd_commit set that flag on the nfsd_open call.

Thanks; applying (after I run a few basic tests).

But: why would a client be waiting on a commit to return a read
delegation?  (We don't give out write delegations.)

--b.

> 
> For now, this patch leaves nfsd_commit opening the file with write
> access since I'm not clear on what sort of access would be more
> appropriate.
> 
> Signed-off-by: Jeff Layton <jlayton@redhat.com>
> ---
>  fs/nfsd/vfs.c |    8 +++++---
>  fs/nfsd/vfs.h |    1 +
>  2 files changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> index a11b0e8..c2dcb4c 100644
> --- a/fs/nfsd/vfs.c
> +++ b/fs/nfsd/vfs.c
> @@ -723,7 +723,7 @@ nfsd_open(struct svc_rqst *rqstp, struct svc_fh *fhp, int type,
>  	struct inode	*inode;
>  	int		flags = O_RDONLY|O_LARGEFILE;
>  	__be32		err;
> -	int		host_err;
> +	int		host_err = 0;
>  
>  	validate_process_creds();
>  
> @@ -760,7 +760,8 @@ nfsd_open(struct svc_rqst *rqstp, struct svc_fh *fhp, int type,
>  	 * Check to see if there are any leases on this file.
>  	 * This may block while leases are broken.
>  	 */
> -	host_err = break_lease(inode, O_NONBLOCK | ((access & NFSD_MAY_WRITE) ? O_WRONLY : 0));
> +	if (!(access & NFSD_MAY_NOT_BREAK_LEASE))
> +		host_err = break_lease(inode, O_NONBLOCK | ((access & NFSD_MAY_WRITE) ? O_WRONLY : 0));
>  	if (host_err == -EWOULDBLOCK)
>  		host_err = -ETIMEDOUT;
>  	if (host_err) /* NOMEM or WOULDBLOCK */
> @@ -1168,7 +1169,8 @@ nfsd_commit(struct svc_rqst *rqstp, struct svc_fh *fhp,
>  			goto out;
>  	}
>  
> -	err = nfsd_open(rqstp, fhp, S_IFREG, NFSD_MAY_WRITE, &file);
> +	err = nfsd_open(rqstp, fhp, S_IFREG,
> +			NFSD_MAY_WRITE|NFSD_MAY_NOT_BREAK_LEASE, &file);
>  	if (err)
>  		goto out;
>  	if (EX_ISSYNC(fhp->fh_export)) {
> diff --git a/fs/nfsd/vfs.h b/fs/nfsd/vfs.h
> index 4b1de0a..217a62c 100644
> --- a/fs/nfsd/vfs.h
> +++ b/fs/nfsd/vfs.h
> @@ -20,6 +20,7 @@
>  #define NFSD_MAY_OWNER_OVERRIDE	64
>  #define NFSD_MAY_LOCAL_ACCESS	128 /* IRIX doing local access check on device special file*/
>  #define NFSD_MAY_BYPASS_GSS_ON_ROOT 256
> +#define NFSD_MAY_NOT_BREAK_LEASE 512
>  
>  #define NFSD_MAY_CREATE		(NFSD_MAY_EXEC|NFSD_MAY_WRITE)
>  #define NFSD_MAY_REMOVE		(NFSD_MAY_EXEC|NFSD_MAY_WRITE|NFSD_MAY_TRUNC)
> -- 
> 1.6.6.1
>