[PATCH] nfsd: Ensure that nfsd_create_setattr commits files to stable storage

[PATCH] nfsd: Ensure that nfsd_create_setattr commits files to stable storage

[Trond Myklebust]

Since nfsd_create_setattr strips the mode from the struct iattr, it
is quite possible that it will optimise away the call to nfsd_setattr
altogether.
If this is the case, then we never call commit_metadata() on the
newly created file.

Also ensure that both nfsd_setattr() and nfsd_create_setattr() fail
when the call to commit_metadata fails.

Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
---
 fs/nfsd/vfs.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index 140c496f612c..5fd9095268cb 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -464,7 +464,7 @@ out_put_write_access:
 	if (size_change)
 		put_write_access(inode);
 	if (!err)
-		commit_metadata(fhp);
+		err = commit_metadata(fhp);
 out:
 	return err;
 }
@@ -1121,7 +1121,8 @@ nfsd_create_setattr(struct svc_rqst *rqstp, struct svc_fh *resfhp,
 		iap->ia_valid &= ~(ATTR_UID|ATTR_GID);
 	if (iap->ia_valid)
 		return nfsd_setattr(rqstp, resfhp, iap, 0, (time_t)0);
-	return 0;
+	/* Callers expect file metadata to be committed here */
+	return commit_metadata(resfhp);
 }
 
 /* HPUX client sometimes creates a file in mode 000, and sets size to 0.
@@ -1253,9 +1254,10 @@ nfsd_create(struct svc_rqst *rqstp, struct svc_fh *fhp,
 	err = nfsd_create_setattr(rqstp, resfhp, iap);
 
 	/*
-	 * nfsd_setattr already committed the child.  Transactional filesystems
-	 * had a chance to commit changes for both parent and child
-	 * simultaneously making the following commit_metadata a noop.
+	 * nfsd_create_setattr already committed the child.  Transactional
+	 * filesystems had a chance to commit changes for both parent and
+	 * child * simultaneously making the following commit_metadata a
+	 * noop.
 	 */
 	err2 = nfserrno(commit_metadata(fhp));
 	if (err2)
@@ -1426,7 +1428,8 @@ do_nfsd_create(struct svc_rqst *rqstp, struct svc_fh *fhp,
 	err = nfsd_create_setattr(rqstp, resfhp, iap);
 
 	/*
-	 * nfsd_setattr already committed the child (and possibly also the parent).
+	 * nfsd_create_setattr already committed the child
+	 * (and possibly also the parent).
 	 */
 	if (!err)
 		err = nfserrno(commit_metadata(fhp));
-- 
1.9.3

Re: [PATCH] nfsd: Ensure that nfsd_create_setattr commits files to stable storage

[Bruce Fields]

On Tue, Jul 01, 2014 at 06:27:53PM -0400, Trond Myklebust wrote:
> Since nfsd_create_setattr strips the mode from the struct iattr, it
> is quite possible that it will optimise away the call to nfsd_setattr
> altogether.
> If this is the case, then we never call commit_metadata() on the
> newly created file.

Thanks.  Looks like a relatively rare problem that we've lived with for
a while, so I'm inclined to just wait for 3.17, but it could also go to
3.16 and stable if you saw it as more urgent....

--b.

> 
> Also ensure that both nfsd_setattr() and nfsd_create_setattr() fail
> when the call to commit_metadata fails.
> 
> Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
> ---
>  fs/nfsd/vfs.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> index 140c496f612c..5fd9095268cb 100644
> --- a/fs/nfsd/vfs.c
> +++ b/fs/nfsd/vfs.c
> @@ -464,7 +464,7 @@ out_put_write_access:
>  	if (size_change)
>  		put_write_access(inode);
>  	if (!err)
> -		commit_metadata(fhp);
> +		err = commit_metadata(fhp);
>  out:
>  	return err;
>  }
> @@ -1121,7 +1121,8 @@ nfsd_create_setattr(struct svc_rqst *rqstp, struct svc_fh *resfhp,
>  		iap->ia_valid &= ~(ATTR_UID|ATTR_GID);
>  	if (iap->ia_valid)
>  		return nfsd_setattr(rqstp, resfhp, iap, 0, (time_t)0);
> -	return 0;
> +	/* Callers expect file metadata to be committed here */
> +	return commit_metadata(resfhp);
>  }
>  
>  /* HPUX client sometimes creates a file in mode 000, and sets size to 0.
> @@ -1253,9 +1254,10 @@ nfsd_create(struct svc_rqst *rqstp, struct svc_fh *fhp,
>  	err = nfsd_create_setattr(rqstp, resfhp, iap);
>  
>  	/*
> -	 * nfsd_setattr already committed the child.  Transactional filesystems
> -	 * had a chance to commit changes for both parent and child
> -	 * simultaneously making the following commit_metadata a noop.
> +	 * nfsd_create_setattr already committed the child.  Transactional
> +	 * filesystems had a chance to commit changes for both parent and
> +	 * child * simultaneously making the following commit_metadata a
> +	 * noop.
>  	 */
>  	err2 = nfserrno(commit_metadata(fhp));
>  	if (err2)
> @@ -1426,7 +1428,8 @@ do_nfsd_create(struct svc_rqst *rqstp, struct svc_fh *fhp,
>  	err = nfsd_create_setattr(rqstp, resfhp, iap);
>  
>  	/*
> -	 * nfsd_setattr already committed the child (and possibly also the parent).
> +	 * nfsd_create_setattr already committed the child
> +	 * (and possibly also the parent).
>  	 */
>  	if (!err)
>  		err = nfserrno(commit_metadata(fhp));
> -- 
> 1.9.3
>