setattr ATTR_SIZE vs the rest

setattr ATTR_SIZE vs the rest

[Christoph Hellwig]

Hi Bruce,

I've got a report that there NFS clients that send SETATTR requests that
mix size changes with uid/gid changes (see the recent pynfs patch for an
artifical reproducer).  At least XFS and GFS2 are very unhappy with this,
and other file systems also don't seem to handle the case correctly.

This patch splits the truncate processing in NFS out into a separate
->setattr call and uses the vfs_truncate helper for it, which also happens
to shrink the NFSD code size by reusing more VFS boiler plate code.

I suspect in the mid-term we really should add a ->truncate method (different
from the previous callback of the same name) to separate the two concepts
clearly at the VFS level.

[PATCH] nfsd: special case truncates some more

[Christoph Hellwig]

Both the NFS protocols and the Linux VFS use a setattr operation with a
bitmap of attributs to set to set various file attributes including the
file size and the uid/gid.

The Linux syscalls never mixe size updates with unrelated updates like
the uid/gid, and some file systems like XFS and GFS2 rely on the fact
that truncates might not update random other attributes, and many
other file systems handle the case but do not update the different
attributes in the same transaction.  NFSD on the other hand passes
the attributes it gets on the wire more or less directly through to
the VFS, leading to updates the file systems don't expect.  XFS at
least has an assert on the allowed attributes, which cought an NFS
client sets the size and group ІD at the same time.

To handles this issue properly this switches nfsd to call vfs_truncate
for size changes, and then handling all other attributes through
notify_change.  As a side effect this also means less boilerplace
code around the size change as we can now reuse the VFS code.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/nfsd/vfs.c | 77 +++++++++++++++++++----------------------------------------
 1 file changed, 24 insertions(+), 53 deletions(-)

diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index 26c6fdb..fafff37 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -332,37 +332,6 @@ nfsd_sanitize_attrs(struct inode *inode, struct iattr *iap)
 	}
 }
 
-static __be32
-nfsd_get_write_access(struct svc_rqst *rqstp, struct svc_fh *fhp,
-		struct iattr *iap)
-{
-	struct inode *inode = d_inode(fhp->fh_dentry);
-	int host_err;
-
-	if (iap->ia_size < inode->i_size) {
-		__be32 err;
-
-		err = nfsd_permission(rqstp, fhp->fh_export, fhp->fh_dentry,
-				NFSD_MAY_TRUNC | NFSD_MAY_OWNER_OVERRIDE);
-		if (err)
-			return err;
-	}
-
-	host_err = get_write_access(inode);
-	if (host_err)
-		goto out_nfserrno;
-
-	host_err = locks_verify_truncate(inode, NULL, iap->ia_size);
-	if (host_err)
-		goto out_put_write_access;
-	return 0;
-
-out_put_write_access:
-	put_write_access(inode);
-out_nfserrno:
-	return nfserrno(host_err);
-}
-
 /*
  * Set various file attributes.  After this call fhp needs an fh_put.
  */
@@ -377,7 +346,6 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp, struct iattr *iap,
 	__be32		err;
 	int		host_err;
 	bool		get_write_count;
-	int		size_change = 0;
 
 	if (iap->ia_valid & (ATTR_ATIME | ATTR_MTIME | ATTR_SIZE))
 		accmode |= NFSD_MAY_WRITE|NFSD_MAY_OWNER_OVERRIDE;
@@ -390,11 +358,11 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp, struct iattr *iap,
 	/* Get inode */
 	err = fh_verify(rqstp, fhp, ftype, accmode);
 	if (err)
-		goto out;
+		return err;
 	if (get_write_count) {
 		host_err = fh_want_write(fhp);
 		if (host_err)
-			return nfserrno(host_err);
+			goto out_host_err;
 	}
 
 	dentry = fhp->fh_dentry;
@@ -405,19 +373,25 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp, struct iattr *iap,
 		iap->ia_valid &= ~ATTR_MODE;
 
 	if (!iap->ia_valid)
-		goto out;
+		return 0;
 
 	nfsd_sanitize_attrs(inode, iap);
 
+	if (check_guard && guardtime != inode->i_ctime.tv_sec)
+		return nfserr_notsync;
+
 	/*
 	 * The size case is special, it changes the file in addition to the
-	 * attributes.
+	 * attributes, and file systems don't expect it to be mixed with
+	 * "random" attribute changes.  We thus split out the size change
+	 * into a separate calo for vfs_truncate, and do the rest as a
+	 * a separate setattr call.
 	 */
 	if (iap->ia_valid & ATTR_SIZE) {
-		err = nfsd_get_write_access(rqstp, fhp, iap);
-		if (err)
-			goto out;
-		size_change = 1;
+		struct path path = {
+			.mnt	= fhp->fh_export->ex_path.mnt,
+			.dentry	= dentry,
+		};
 
 		/*
 		 * RFC5661, Section 18.30.4:
@@ -428,27 +402,24 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp, struct iattr *iap,
 		 */
 		if (iap->ia_size != i_size_read(inode))
 			iap->ia_valid |= ATTR_MTIME;
-	}
 
-	iap->ia_valid |= ATTR_CTIME;
+		host_err = vfs_truncate(&path, iap->ia_size);
+		if (host_err)
+			goto out_host_err;
 
-	if (check_guard && guardtime != inode->i_ctime.tv_sec) {
-		err = nfserr_notsync;
-		goto out_put_write_access;
+		iap->ia_valid &= ~ATTR_SIZE;
 	}
 
+	iap->ia_valid |= ATTR_CTIME;
+
 	fh_lock(fhp);
 	host_err = notify_change(dentry, iap, NULL);
 	fh_unlock(fhp);
-	err = nfserrno(host_err);
 
-out_put_write_access:
-	if (size_change)
-		put_write_access(inode);
-	if (!err)
-		err = nfserrno(commit_metadata(fhp));
-out:
-	return err;
+	if (!host_err)
+		host_err = commit_metadata(fhp);
+out_host_err:
+	return nfserrno(host_err);
 }
 
 #if defined(CONFIG_NFSD_V4)
-- 
2.1.4

Re: [PATCH] nfsd: special case truncates some more

[Jeff Layton]

On Sun, 2017-01-22 at 17:54 +0100, Christoph Hellwig wrote:
> Both the NFS protocols and the Linux VFS use a setattr operation with a
> bitmap of attributs to set to set various file attributes including the
> file size and the uid/gid.
> 
> The Linux syscalls never mixe size updates with unrelated updates like
> the uid/gid, and some file systems like XFS and GFS2 rely on the fact
> that truncates might not update random other attributes, and many
> other file systems handle the case but do not update the different
> attributes in the same transaction.  NFSD on the other hand passes
> the attributes it gets on the wire more or less directly through to
> the VFS, leading to updates the file systems don't expect.  XFS at
> least has an assert on the allowed attributes, which cought an NFS
> client sets the size and group ІD at the same time.
> 
> To handles this issue properly this switches nfsd to call vfs_truncate
> for size changes, and then handling all other attributes through
> notify_change.  As a side effect this also means less boilerplace
> code around the size change as we can now reuse the VFS code.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/nfsd/vfs.c | 77 +++++++++++++++++++----------------------------------------
>  1 file changed, 24 insertions(+), 53 deletions(-)
> 
> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> index 26c6fdb..fafff37 100644
> --- a/fs/nfsd/vfs.c
> +++ b/fs/nfsd/vfs.c
> @@ -332,37 +332,6 @@ nfsd_sanitize_attrs(struct inode *inode, struct iattr *iap)
>  	}
>  }
>  
> -static __be32
> -nfsd_get_write_access(struct svc_rqst *rqstp, struct svc_fh *fhp,
> -		struct iattr *iap)
> -{
> -	struct inode *inode = d_inode(fhp->fh_dentry);
> -	int host_err;
> -
> -	if (iap->ia_size < inode->i_size) {
> -		__be32 err;
> -
> -		err = nfsd_permission(rqstp, fhp->fh_export, fhp->fh_dentry,
> -				NFSD_MAY_TRUNC | NFSD_MAY_OWNER_OVERRIDE);
> -		if (err)
> -			return err;
> -	}
> -
> -	host_err = get_write_access(inode);
> -	if (host_err)
> -		goto out_nfserrno;
> -
> -	host_err = locks_verify_truncate(inode, NULL, iap->ia_size);
> -	if (host_err)
> -		goto out_put_write_access;
> -	return 0;
> -
> -out_put_write_access:
> -	put_write_access(inode);
> -out_nfserrno:
> -	return nfserrno(host_err);
> -}
> -
>  /*
>   * Set various file attributes.  After this call fhp needs an fh_put.
>   */
> @@ -377,7 +346,6 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp, struct iattr *iap,
>  	__be32		err;
>  	int		host_err;
>  	bool		get_write_count;
> -	int		size_change = 0;
>  
>  	if (iap->ia_valid & (ATTR_ATIME | ATTR_MTIME | ATTR_SIZE))
>  		accmode |= NFSD_MAY_WRITE|NFSD_MAY_OWNER_OVERRIDE;
> @@ -390,11 +358,11 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp, struct iattr *iap,
>  	/* Get inode */
>  	err = fh_verify(rqstp, fhp, ftype, accmode);
>  	if (err)
> -		goto out;
> +		return err;
>  	if (get_write_count) {
>  		host_err = fh_want_write(fhp);
>  		if (host_err)
> -			return nfserrno(host_err);
> +			goto out_host_err;
>  	}
>  
>  	dentry = fhp->fh_dentry;
> @@ -405,19 +373,25 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp, struct iattr *iap,
>  		iap->ia_valid &= ~ATTR_MODE;
>  
>  	if (!iap->ia_valid)
> -		goto out;
> +		return 0;
>  
>  	nfsd_sanitize_attrs(inode, iap);
>  
> +	if (check_guard && guardtime != inode->i_ctime.tv_sec)
> +		return nfserr_notsync;
> +
>  	/*
>  	 * The size case is special, it changes the file in addition to the
> -	 * attributes.
> +	 * attributes, and file systems don't expect it to be mixed with
> +	 * "random" attribute changes.  We thus split out the size change
> +	 * into a separate calo for vfs_truncate, and do the rest as a
> +	 * a separate setattr call.
>  	 */
>  	if (iap->ia_valid & ATTR_SIZE) {
> -		err = nfsd_get_write_access(rqstp, fhp, iap);
> -		if (err)
> -			goto out;
> -		size_change = 1;
> +		struct path path = {
> +			.mnt	= fhp->fh_export->ex_path.mnt,
> +			.dentry	= dentry,
> +		};
>  
>  		/*
>  		 * RFC5661, Section 18.30.4:
> @@ -428,27 +402,24 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp, struct iattr *iap,
>  		 */
>  		if (iap->ia_size != i_size_read(inode))
>  			iap->ia_valid |= ATTR_MTIME;
> -	}
>  
> -	iap->ia_valid |= ATTR_CTIME;
> +		host_err = vfs_truncate(&path, iap->ia_size);
> +		if (host_err)
> +			goto out_host_err;
>  
> -	if (check_guard && guardtime != inode->i_ctime.tv_sec) {
> -		err = nfserr_notsync;
> -		goto out_put_write_access;
> +		iap->ia_valid &= ~ATTR_SIZE;
>  	}
>  
> +	iap->ia_valid |= ATTR_CTIME;
> +

So if you only have ATTR_SIZE then you're going to end up having to do
another notify_change to update the ctime? Can we get away with just
calling vfs_truncate when only ATTR_SIZE is set and skipping the
notify_change to update the ctime?

>  	fh_lock(fhp);
>  	host_err = notify_change(dentry, iap, NULL);
>  	fh_unlock(fhp);
> -	err = nfserrno(host_err);
>  
> -out_put_write_access:
> -	if (size_change)
> -		put_write_access(inode);
> -	if (!err)
> -		err = nfserrno(commit_metadata(fhp));
> -out:
> -	return err;
> +	if (!host_err)
> +		host_err = commit_metadata(fhp);
> +out_host_err:
> +	return nfserrno(host_err);
>  }
>  
>  #if defined(CONFIG_NFSD_V4)

Overall though, this is a reasonable change I think. Size changes are
more of a special case. I also like the idea of breaking out truncates
to a separate operation, but that's a much bigger project.

-- 
Jeff Layton <jlayton@poochiereds.net>

Re: [PATCH] nfsd: special case truncates some more

[Christoph Hellwig]

On Mon, Jan 23, 2017 at 07:21:56AM -0500, Jeff Layton wrote:
> So if you only have ATTR_SIZE then you're going to end up having to do
> another notify_change to update the ctime? Can we get away with just
> calling vfs_truncate when only ATTR_SIZE is set and skipping the
> notify_change to update the ctime?

We probably could, but there are some fine details there:

For truncate(2) Posix require us to not updated the time stamps when
truncating and already zero length file to 0, but for ftruncate(2) and
O_TRUNC we do have to update the mtime and ctime.  The Linux VFS
communicates that difference by not setting ATTR_CTIME and ATTR_MTIME in
ia_valid for truncate(2), but expecting the fs to update them anyway.
(another reason for a proper truncate method to make this explicit).

I'll need to look at the exact NFS semantics in that area, but after
a bit of research I can probably come up with something that will work.

Re: [PATCH] nfsd: special case truncates some more

[Christoph Hellwig]

On Mon, Jan 23, 2017 at 01:33:48PM +0100, Christoph Hellwig wrote:
> I'll need to look at the exact NFS semantics in that area, but after
> a bit of research I can probably come up with something that will work.

Here is my first attempt.  As vfs_truncate will add the ctime and mtime
updates when needed it just leaves handling that quirk to vfs_truncate
and then exits early if no other attributes are set.

Unfortunately at least the Linux client always seems to also request
a mtime update with a size update.  We could keep the

	if (iap->ia_size != i_size_read(inode))

check from the old code and remove ATTR_MTIME, but these racy checks
outside i_rwsem make me feel a bit uneasy.  Jeff, Bruce - any opinion
if we should add something like this:

	/* vfs_truncate will update ctime and mtime if the size changes */
	if (iap->ia_size != i_size_read(inode))
		iap->ia_valid &= ATTR_MTIME;

back to nfsd_setattr?  This would avoid the additional setattr call,
but make me feel dirty :)

---
>From 0e06e2fc6157bb97692ed47c21e36120efb9f15c Mon Sep 17 00:00:00 2001
From: Christoph Hellwig <hch@lst.de>
Date: Sun, 22 Jan 2017 17:17:48 +0100
Subject: nfsd: special case truncates some more
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Both the NFS protocols and the Linux VFS use a setattr operation with a
bitmap of attributs to set to set various file attributes including the
file size and the uid/gid.

The Linux syscalls never mixe size updates with unrelated updates like
the uid/gid, and some file systems like XFS and GFS2 rely on the fact
that truncates might not update random other attributes, and many
other file systems handle the case but do not update the different
attributes in the same transaction.  NFSD on the other hand passes
the attributes it gets on the wire more or less directly through to
the VFS, leading to updates the file systems don't expect.  XFS at
least has an assert on the allowed attributes, which cought an NFS
client sets the size and group ІD at the same time.

To handles this issue properly this switches nfsd to call vfs_truncate
for size changes, and then handling all other attributes through
notify_change.  As a side effect this also means less boilerplace
code around the size change as we can now reuse the VFS code.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/nfsd/vfs.c | 92 +++++++++++++++++++----------------------------------------
 1 file changed, 30 insertions(+), 62 deletions(-)

diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index 26c6fdb..4ca5b92 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -332,37 +332,6 @@ nfsd_sanitize_attrs(struct inode *inode, struct iattr *iap)
 	}
 }
 
-static __be32
-nfsd_get_write_access(struct svc_rqst *rqstp, struct svc_fh *fhp,
-		struct iattr *iap)
-{
-	struct inode *inode = d_inode(fhp->fh_dentry);
-	int host_err;
-
-	if (iap->ia_size < inode->i_size) {
-		__be32 err;
-
-		err = nfsd_permission(rqstp, fhp->fh_export, fhp->fh_dentry,
-				NFSD_MAY_TRUNC | NFSD_MAY_OWNER_OVERRIDE);
-		if (err)
-			return err;
-	}
-
-	host_err = get_write_access(inode);
-	if (host_err)
-		goto out_nfserrno;
-
-	host_err = locks_verify_truncate(inode, NULL, iap->ia_size);
-	if (host_err)
-		goto out_put_write_access;
-	return 0;
-
-out_put_write_access:
-	put_write_access(inode);
-out_nfserrno:
-	return nfserrno(host_err);
-}
-
 /*
  * Set various file attributes.  After this call fhp needs an fh_put.
  */
@@ -377,7 +346,6 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp, struct iattr *iap,
 	__be32		err;
 	int		host_err;
 	bool		get_write_count;
-	int		size_change = 0;
 
 	if (iap->ia_valid & (ATTR_ATIME | ATTR_MTIME | ATTR_SIZE))
 		accmode |= NFSD_MAY_WRITE|NFSD_MAY_OWNER_OVERRIDE;
@@ -390,11 +358,11 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp, struct iattr *iap,
 	/* Get inode */
 	err = fh_verify(rqstp, fhp, ftype, accmode);
 	if (err)
-		goto out;
+		return err;
 	if (get_write_count) {
 		host_err = fh_want_write(fhp);
 		if (host_err)
-			return nfserrno(host_err);
+			goto out_host_err;
 	}
 
 	dentry = fhp->fh_dentry;
@@ -405,50 +373,50 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp, struct iattr *iap,
 		iap->ia_valid &= ~ATTR_MODE;
 
 	if (!iap->ia_valid)
-		goto out;
+		return 0;
 
 	nfsd_sanitize_attrs(inode, iap);
 
+	if (check_guard && guardtime != inode->i_ctime.tv_sec)
+		return nfserr_notsync;
+
 	/*
 	 * The size case is special, it changes the file in addition to the
-	 * attributes.
+	 * attributes, and file systems don't expect it to be mixed with
+	 * "random" attribute changes.  We thus split out the size change
+	 * into a separate calo for vfs_truncate, and do the rest as a
+	 * a separate setattr call.
+	 *
+	 * Note that vfs_truncate will also update ctime and mtime if
+	 * the file size changes.
 	 */
 	if (iap->ia_valid & ATTR_SIZE) {
-		err = nfsd_get_write_access(rqstp, fhp, iap);
-		if (err)
-			goto out;
-		size_change = 1;
+		struct path path = {
+			.mnt	= fhp->fh_export->ex_path.mnt,
+			.dentry	= dentry,
+		};
 
-		/*
-		 * RFC5661, Section 18.30.4:
-		 *   Changing the size of a file with SETATTR indirectly
-		 *   changes the time_modify and change attributes.
-		 *
-		 * (and similar for the older RFCs)
-		 */
-		if (iap->ia_size != i_size_read(inode))
-			iap->ia_valid |= ATTR_MTIME;
+		host_err = vfs_truncate(&path, iap->ia_size);
+		if (host_err)
+			goto out_host_err;
+
+		iap->ia_valid &= ~ATTR_SIZE;
+		if (!iap->ia_valid)
+			goto done;
 	}
 
 	iap->ia_valid |= ATTR_CTIME;
 
-	if (check_guard && guardtime != inode->i_ctime.tv_sec) {
-		err = nfserr_notsync;
-		goto out_put_write_access;
-	}
-
 	fh_lock(fhp);
 	host_err = notify_change(dentry, iap, NULL);
 	fh_unlock(fhp);
-	err = nfserrno(host_err);
+	if (host_err)
+		goto out_host_err;
 
-out_put_write_access:
-	if (size_change)
-		put_write_access(inode);
-	if (!err)
-		err = nfserrno(commit_metadata(fhp));
-out:
-	return err;
+done:
+	host_err = commit_metadata(fhp);
+out_host_err:
+	return nfserrno(host_err);
 }
 
 #if defined(CONFIG_NFSD_V4)
-- 
2.1.4

Re: [PATCH] nfsd: special case truncates some more

[Jeff Layton]

On Mon, 2017-01-23 at 16:36 +0100, Christoph Hellwig wrote:
> On Mon, Jan 23, 2017 at 01:33:48PM +0100, Christoph Hellwig wrote:
> > I'll need to look at the exact NFS semantics in that area, but after
> > a bit of research I can probably come up with something that will work.
> 
> Here is my first attempt.  As vfs_truncate will add the ctime and mtime
> updates when needed it just leaves handling that quirk to vfs_truncate
> and then exits early if no other attributes are set.
> 
> Unfortunately at least the Linux client always seems to also request
> a mtime update with a size update.  We could keep the
> 
> 	if (iap->ia_size != i_size_read(inode))
> 
> check from the old code and remove ATTR_MTIME, but these racy checks
> outside i_rwsem make me feel a bit uneasy.  Jeff, Bruce - any opinion
> if we should add something like this:
> 

Ok, that's more complicated than it looked at first blush. :)

To be clear, the client is requesting to set the mtime to current server
time and not to a specific mtime, right?

> 	/* vfs_truncate will update ctime and mtime if the size changes */
> 	if (iap->ia_size != i_size_read(inode))
> 		iap->ia_valid &= ATTR_MTIME;
> 
> back to nfsd_setattr?  This would avoid the additional setattr call,
> but make me feel dirty :)
> 

I agree that I wouldn't want to go with a potentially racy check.

I don't see where vfs_truncate will handle the times though. do_truncate
will, but you have to pass in a non-zero time_attrs and vfs_truncate
always sets that to 0.

If we did want to do this, it seems like it might be better to just add
a new time_attrs arg to vfs_truncate that gets passed to do_truncate.
Most callers would set it to zero, but nfsd could set it to:

    iap->ia_valid & (ATTR_MTIME|ATTR_CTIME)

Would that work?

> ---
> From 0e06e2fc6157bb97692ed47c21e36120efb9f15c Mon Sep 17 00:00:00 2001
> From: Christoph Hellwig <hch@lst.de>
> Date: Sun, 22 Jan 2017 17:17:48 +0100
> Subject: nfsd: special case truncates some more
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
> 
> Both the NFS protocols and the Linux VFS use a setattr operation with a
> bitmap of attributs to set to set various file attributes including the
> file size and the uid/gid.
> 
> The Linux syscalls never mixe size updates with unrelated updates like
> the uid/gid, and some file systems like XFS and GFS2 rely on the fact
> that truncates might not update random other attributes, and many
> other file systems handle the case but do not update the different
> attributes in the same transaction.  NFSD on the other hand passes
> the attributes it gets on the wire more or less directly through to
> the VFS, leading to updates the file systems don't expect.  XFS at
> least has an assert on the allowed attributes, which cought an NFS
> client sets the size and group ІD at the same time.
> 
> To handles this issue properly this switches nfsd to call vfs_truncate
> for size changes, and then handling all other attributes through
> notify_change.  As a side effect this also means less boilerplace
> code around the size change as we can now reuse the VFS code.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/nfsd/vfs.c | 92 +++++++++++++++++++----------------------------------------
>  1 file changed, 30 insertions(+), 62 deletions(-)
> 
> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> index 26c6fdb..4ca5b92 100644
> --- a/fs/nfsd/vfs.c
> +++ b/fs/nfsd/vfs.c
> @@ -332,37 +332,6 @@ nfsd_sanitize_attrs(struct inode *inode, struct iattr *iap)
>  	}
>  }
>  
> -static __be32
> -nfsd_get_write_access(struct svc_rqst *rqstp, struct svc_fh *fhp,
> -		struct iattr *iap)
> -{
> -	struct inode *inode = d_inode(fhp->fh_dentry);
> -	int host_err;
> -
> -	if (iap->ia_size < inode->i_size) {
> -		__be32 err;
> -
> -		err = nfsd_permission(rqstp, fhp->fh_export, fhp->fh_dentry,
> -				NFSD_MAY_TRUNC | NFSD_MAY_OWNER_OVERRIDE);
> -		if (err)
> -			return err;
> -	}
> -
> -	host_err = get_write_access(inode);
> -	if (host_err)
> -		goto out_nfserrno;
> -
> -	host_err = locks_verify_truncate(inode, NULL, iap->ia_size);
> -	if (host_err)
> -		goto out_put_write_access;
> -	return 0;
> -
> -out_put_write_access:
> -	put_write_access(inode);
> -out_nfserrno:
> -	return nfserrno(host_err);
> -}
> -
>  /*
>   * Set various file attributes.  After this call fhp needs an fh_put.
>   */
> @@ -377,7 +346,6 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp, struct iattr *iap,
>  	__be32		err;
>  	int		host_err;
>  	bool		get_write_count;
> -	int		size_change = 0;
>  
>  	if (iap->ia_valid & (ATTR_ATIME | ATTR_MTIME | ATTR_SIZE))
>  		accmode |= NFSD_MAY_WRITE|NFSD_MAY_OWNER_OVERRIDE;
> @@ -390,11 +358,11 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp, struct iattr *iap,
>  	/* Get inode */
>  	err = fh_verify(rqstp, fhp, ftype, accmode);
>  	if (err)
> -		goto out;
> +		return err;
>  	if (get_write_count) {
>  		host_err = fh_want_write(fhp);
>  		if (host_err)
> -			return nfserrno(host_err);
> +			goto out_host_err;
>  	}
>  
>  	dentry = fhp->fh_dentry;
> @@ -405,50 +373,50 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp, struct iattr *iap,
>  		iap->ia_valid &= ~ATTR_MODE;
>  
>  	if (!iap->ia_valid)
> -		goto out;
> +		return 0;
>  
>  	nfsd_sanitize_attrs(inode, iap);
>  
> +	if (check_guard && guardtime != inode->i_ctime.tv_sec)
> +		return nfserr_notsync;
> +
>  	/*
>  	 * The size case is special, it changes the file in addition to the
> -	 * attributes.
> +	 * attributes, and file systems don't expect it to be mixed with
> +	 * "random" attribute changes.  We thus split out the size change
> +	 * into a separate calo for vfs_truncate, and do the rest as a
> +	 * a separate setattr call.
> +	 *
> +	 * Note that vfs_truncate will also update ctime and mtime if
> +	 * the file size changes.
>  	 */
>  	if (iap->ia_valid & ATTR_SIZE) {
> -		err = nfsd_get_write_access(rqstp, fhp, iap);
> -		if (err)
> -			goto out;
> -		size_change = 1;
> +		struct path path = {
> +			.mnt	= fhp->fh_export->ex_path.mnt,
> +			.dentry	= dentry,
> +		};
>  
> -		/*
> -		 * RFC5661, Section 18.30.4:
> -		 *   Changing the size of a file with SETATTR indirectly
> -		 *   changes the time_modify and change attributes.
> -		 *
> -		 * (and similar for the older RFCs)
> -		 */
> -		if (iap->ia_size != i_size_read(inode))
> -			iap->ia_valid |= ATTR_MTIME;
> +		host_err = vfs_truncate(&path, iap->ia_size);
> +		if (host_err)
> +			goto out_host_err;
> +
> +		iap->ia_valid &= ~ATTR_SIZE;
> +		if (!iap->ia_valid)
> +			goto done;
>  	}
>  
>  	iap->ia_valid |= ATTR_CTIME;
>  
> -	if (check_guard && guardtime != inode->i_ctime.tv_sec) {
> -		err = nfserr_notsync;
> -		goto out_put_write_access;
> -	}
> -
>  	fh_lock(fhp);
>  	host_err = notify_change(dentry, iap, NULL);
>  	fh_unlock(fhp);
> -	err = nfserrno(host_err);
> +	if (host_err)
> +		goto out_host_err;
>  
> -out_put_write_access:
> -	if (size_change)
> -		put_write_access(inode);
> -	if (!err)
> -		err = nfserrno(commit_metadata(fhp));
> -out:
> -	return err;
> +done:
> +	host_err = commit_metadata(fhp);
> +out_host_err:
> +	return nfserrno(host_err);
>  }
>  
>  #if defined(CONFIG_NFSD_V4)

-- 
Jeff Layton <jlayton@poochiereds.net>

Re: [PATCH] nfsd: special case truncates some more

[Christoph Hellwig]

On Mon, Jan 23, 2017 at 10:52:09AM -0500, Jeff Layton wrote:
> To be clear, the client is requesting to set the mtime to current server
> time and not to a specific mtime, right?

Yes.  And I think it's mostly the Linux client being lazy - ATTR_MTIME
is what it gets from the VFS for a truncate operation (but not ftrunate,
so we probably won't see it on the wire in that case, but I need to verify
that first).  Yet another reason for ->truncate :)

> I don't see where vfs_truncate will handle the times though. do_truncate
> will, but you have to pass in a non-zero time_attrs and vfs_truncate
> always sets that to 0.

This is the magic of the Linux VFS interface.  For a ATTR_SIZE operation
the file system is expected to update mtime and ctime if the size changes
even if ATTR_MTIME and ATTR_CTIME are not set.  See the comments
in xfs_vn_setattr_size, which I wrote many years ago when I tripped
over this interesting calling convention.

> If we did want to do this, it seems like it might be better to just add
> a new time_attrs arg to vfs_truncate that gets passed to do_truncate.
> Most callers would set it to zero, but nfsd could set it to:
> 
>     iap->ia_valid & (ATTR_MTIME|ATTR_CTIME)
> 
> Would that work?

I'd hate it.  I'd rather spent my time on a real truncate operation
which makes all the above magic explicit, and as a side effect would
fix the Linux client sending spurious mtime update requests that
the procotol already requires to be done implicitly.

Re: [PATCH] nfsd: special case truncates some more

[Jeff Layton]

On Mon, 2017-01-23 at 17:05 +0100, Christoph Hellwig wrote:
> On Mon, Jan 23, 2017 at 10:52:09AM -0500, Jeff Layton wrote:
> > To be clear, the client is requesting to set the mtime to current server
> > time and not to a specific mtime, right?
> 
> Yes.  And I think it's mostly the Linux client being lazy - ATTR_MTIME
> is what it gets from the VFS for a truncate operation (but not ftrunate,
> so we probably won't see it on the wire in that case, but I need to verify
> that first).  Yet another reason for ->truncate :)
> 

Heh, ok. Makes sense.

> > I don't see where vfs_truncate will handle the times though. do_truncate
> > will, but you have to pass in a non-zero time_attrs and vfs_truncate
> > always sets that to 0.
> 
> This is the magic of the Linux VFS interface.  For a ATTR_SIZE operation
> the file system is expected to update mtime and ctime if the size changes
> even if ATTR_MTIME and ATTR_CTIME are not set.  See the comments
> in xfs_vn_setattr_size, which I wrote many years ago when I tripped
> over this interesting calling convention.
> 

Ick.

> > If we did want to do this, it seems like it might be better to just add
> > a new time_attrs arg to vfs_truncate that gets passed to do_truncate.
> > Most callers would set it to zero, but nfsd could set it to:
> > 
> >     iap->ia_valid & (ATTR_MTIME|ATTR_CTIME)
> > 
> > Would that work?
> 
> I'd hate it.  I'd rather spent my time on a real truncate operation
> which makes all the above magic explicit, and as a side effect would
> fix the Linux client sending spurious mtime update requests that
> the procotol already requires to be done implicitly.

Fair enough. In that case, I wouldn't try to optimize away the second
notify_change if the client sets ATTR_MTIME/ATTR_CTIME for now

 We might have to dip down into the fs twice for a truncate, but so be
it. If it becomes a problem then we can consider that more reason to add
a real ->truncate operation.

-- 
Jeff Layton <jlayton@poochiereds.net>

Re: [PATCH] nfsd: special case truncates some more

[Trond Myklebust]

On Mon, 2017-01-23 at 17:05 +0100, Christoph Hellwig wrote:
> On Mon, Jan 23, 2017 at 10:52:09AM -0500, Jeff Layton wrote:
> > To be clear, the client is requesting to set the mtime to current
> > server
> > time and not to a specific mtime, right?
> 
> Yes.  And I think it's mostly the Linux client being lazy -
> ATTR_MTIME
> is what it gets from the VFS for a truncate operation (but not
> ftrunate,
> so we probably won't see it on the wire in that case, but I need to
> verify
> that first).  Yet another reason for ->truncate :)
> 

Note that the POSIX spec seems to have changed recently. The current
spec appears to state that we should set the mtime and ctime (and
change attribute) on success in open(O_TRUNC), truncate() and
ftruncate(). In previous incarnations of the spec, truncate() would
only set the time if the size was changed:

See:
http://pubs.opengroup.org/onlinepubs/9699919799/functions/ftruncate.htm
l
http://pubs.opengroup.org/onlinepubs/9699919799/functions/truncate.html
http://pubs.opengroup.org/onlinepubs/9699919799/functions/open.html

-- 
Trond Myklebust
Linux NFS client maintainer, PrimaryData
trond.myklebust@primarydata.com

Re: [PATCH] nfsd: special case truncates some more

[hch]

On Mon, Jan 23, 2017 at 04:20:45PM +0000, Trond Myklebust wrote:
> Note that the POSIX spec seems to have changed recently. The current
> spec appears to state that we should set the mtime and ctime (and
> change attribute) on success in open(O_TRUNC), truncate() and
> ftruncate(). In previous incarnations of the spec, truncate() would
> only set the time if the size was changed:

Interesting.  But in this case historical Posix and thus Linux behavior
still takes precedence and we're not suddently going to change behavior.

Re: setattr ATTR_SIZE vs the rest

[J. Bruce Fields]

On Sun, Jan 22, 2017 at 05:54:19PM +0100, Christoph Hellwig wrote:
> I've got a report that there NFS clients that send SETATTR requests that
> mix size changes with uid/gid changes

That sounds a little weird.  Do you know anything about which clients,
or how common the behavior is?

> (see the recent pynfs patch for an
> artifical reproducer).  At least XFS and GFS2 are very unhappy with this,
> and other file systems also don't seem to handle the case correctly.

Does this cause a crash or corruption, or "just" fail to set the
attributes correctly?  (Just wondering how urgent the fix is....).

--b.

> 
> This patch splits the truncate processing in NFS out into a separate
> ->setattr call and uses the vfs_truncate helper for it, which also happens
> to shrink the NFSD code size by reusing more VFS boiler plate code.
> 
> I suspect in the mid-term we really should add a ->truncate method (different
> from the previous callback of the same name) to separate the two concepts
> clearly at the VFS level.

Re: [PATCH] nfsd: special case truncates some more

[Trond Myklebust]

On Mon, 2017-01-23 at 17:26 +0100, hch wrote:
> On Mon, Jan 23, 2017 at 04:20:45PM +0000, Trond Myklebust wrote:
> > Note that the POSIX spec seems to have changed recently. The
> > current
> > spec appears to state that we should set the mtime and ctime (and
> > change attribute) on success in open(O_TRUNC), truncate() and
> > ftruncate(). In previous incarnations of the spec, truncate() would
> > only set the time if the size was changed:
> 
> Interesting.  But in this case historical Posix and thus Linux
> behavior
> still takes precedence and we're not suddently going to change
> behavior.
> 


In that case the client will be required to continue to need to send
mtime/ctime in order to ensure that we get the same historical
semantics w.r.t. ftruncate() vs truncate().

IOW: It's not a question of the client being lazy about clearing the
flags. It's a question of enforcing the correct semantics.

-- 
Trond Myklebust
Linux NFS client maintainer, PrimaryData
trond.myklebust@primarydata.com

Re: [PATCH] nfsd: special case truncates some more

[hch]

On Mon, Jan 23, 2017 at 05:25:34PM +0000, Trond Myklebust wrote:
> In that case the client will be required to continue to need to send
> mtime/ctime in order to ensure that we get the same historical
> semantics w.r.t. ftruncate() vs truncate().
> 
> IOW: It's not a question of the client being lazy about clearing the
> flags. It's a question of enforcing the correct semantics.

No, the NFS spec requires the server to add an implicit mtime
when the size actually changes.  In fact the current code has a comment
pointing to the section:

 * RFC5661, Section 18.30.4:
 *   Changing the size of a file with SETATTR indirectly
 *   changes the time_modify and change attributes.
 *
 * (and similar for the older RFCs)

And yes, I've double checked that in the RFC.

Re: [PATCH] nfsd: special case truncates some more

[Trond Myklebust]

On Mon, 2017-01-23 at 18:38 +0100, hch wrote:
> On Mon, Jan 23, 2017 at 05:25:34PM +0000, Trond Myklebust wrote:
> > In that case the client will be required to continue to need to
> > send
> > mtime/ctime in order to ensure that we get the same historical
> > semantics w.r.t. ftruncate() vs truncate().
> > 
> > IOW: It's not a question of the client being lazy about clearing
> > the
> > flags. It's a question of enforcing the correct semantics.
> 
> No, the NFS spec requires the server to add an implicit mtime
> when the size actually changes.  In fact the current code has a
> comment
> pointing to the section:
> 
>  * RFC5661, Section 18.30.4:
>  *   Changing the size of a file with SETATTR indirectly
>  *   changes the time_modify and change attributes.
>  *
>  * (and similar for the older RFCs)
> 
> And yes, I've double checked that in the RFC.

Sure, but truncate() on POSIX adds the requirement that the mtime/ctime
should change even when the file size is not changed.

-- 
Trond Myklebust
Linux NFS client maintainer, PrimaryData
trond.myklebust@primarydata.com

Re: setattr ATTR_SIZE vs the rest

[Christoph Hellwig]

On Mon, Jan 23, 2017 at 11:58:33AM -0500, J. Bruce Fields wrote:
> On Sun, Jan 22, 2017 at 05:54:19PM +0100, Christoph Hellwig wrote:
> > I've got a report that there NFS clients that send SETATTR requests that
> > mix size changes with uid/gid changes
> 
> That sounds a little weird.  Do you know anything about which clients,
> or how common the behavior is?

I've only gotten the bug report and don't know the details.  But from
my cursory knowledge of NFS spec I suspects it's a loosely coupled
flexfiles server that uses uid/gids for access control on the data
file.

> > (see the recent pynfs patch for an
> > artifical reproducer).  At least XFS and GFS2 are very unhappy with this,
> > and other file systems also don't seem to handle the case correctly.
> 
> Does this cause a crash or corruption, or "just" fail to set the
> attributes correctly?  (Just wondering how urgent the fix is....).

With a XFS debug build it crashes, otherwise it "only" fails to set the
attributes.  But given that the attributes that fail to be set are
owner/group (uid,gid) it would totally break the security model, so
I think it's pretty serious.

Re: [PATCH] nfsd: special case truncates some more

[J. Bruce Fields]

On Mon, Jan 23, 2017 at 05:26:07PM +0100, hch wrote:
> On Mon, Jan 23, 2017 at 04:20:45PM +0000, Trond Myklebust wrote:
> > Note that the POSIX spec seems to have changed recently. The current
> > spec appears to state that we should set the mtime and ctime (and
> > change attribute) on success in open(O_TRUNC), truncate() and
> > ftruncate(). In previous incarnations of the spec, truncate() would
> > only set the time if the size was changed:
> 
> Interesting.  But in this case historical Posix and thus Linux behavior
> still takes precedence and we're not suddently going to change behavior.

Makes sense as a general rule, but is it really likely that anyone
depends on ctime/mtime not changing on a non-size-changing truncate()?

--b.

Re: [PATCH] nfsd: special case truncates some more

[J. Bruce Fields]

On Mon, Jan 23, 2017 at 10:52:09AM -0500, Jeff Layton wrote:
> On Mon, 2017-01-23 at 16:36 +0100, Christoph Hellwig wrote:
> > On Mon, Jan 23, 2017 at 01:33:48PM +0100, Christoph Hellwig wrote:
> > > I'll need to look at the exact NFS semantics in that area, but after
> > > a bit of research I can probably come up with something that will work.
> > 
> > Here is my first attempt.  As vfs_truncate will add the ctime and mtime
> > updates when needed it just leaves handling that quirk to vfs_truncate
> > and then exits early if no other attributes are set.
> > 
> > Unfortunately at least the Linux client always seems to also request
> > a mtime update with a size update.  We could keep the
> > 
> > 	if (iap->ia_size != i_size_read(inode))
> > 
> > check from the old code and remove ATTR_MTIME, but these racy checks
> > outside i_rwsem make me feel a bit uneasy.  Jeff, Bruce - any opinion
> > if we should add something like this:
> > 
> 
> Ok, that's more complicated than it looked at first blush. :)
> 
> To be clear, the client is requesting to set the mtime to current server
> time and not to a specific mtime, right?
> 
> > 	/* vfs_truncate will update ctime and mtime if the size changes */
> > 	if (iap->ia_size != i_size_read(inode))
> > 		iap->ia_valid &= ATTR_MTIME;
> > 
> > back to nfsd_setattr?  This would avoid the additional setattr call,
> > but make me feel dirty :)
> > 
> 
> I agree that I wouldn't want to go with a potentially racy check.

Unless I'm misunderstanding: we've always had the race, and the
consequence is just an unnecessary update in the case the truncate
didn't actually change the size (although it looked like it would at the
time of the check).

I don't like that, but it's not going to keep me up at night.

--b.

> 
> I don't see where vfs_truncate will handle the times though. do_truncate
> will, but you have to pass in a non-zero time_attrs and vfs_truncate
> always sets that to 0.
> 
> If we did want to do this, it seems like it might be better to just add
> a new time_attrs arg to vfs_truncate that gets passed to do_truncate.
> Most callers would set it to zero, but nfsd could set it to:
> 
>     iap->ia_valid & (ATTR_MTIME|ATTR_CTIME)
> 
> Would that work?
> 
> > ---
> > From 0e06e2fc6157bb97692ed47c21e36120efb9f15c Mon Sep 17 00:00:00 2001
> > From: Christoph Hellwig <hch@lst.de>
> > Date: Sun, 22 Jan 2017 17:17:48 +0100
> > Subject: nfsd: special case truncates some more
> > MIME-Version: 1.0
> > Content-Type: text/plain; charset=UTF-8
> > Content-Transfer-Encoding: 8bit
> > 
> > Both the NFS protocols and the Linux VFS use a setattr operation with a
> > bitmap of attributs to set to set various file attributes including the
> > file size and the uid/gid.
> > 
> > The Linux syscalls never mixe size updates with unrelated updates like
> > the uid/gid, and some file systems like XFS and GFS2 rely on the fact
> > that truncates might not update random other attributes, and many
> > other file systems handle the case but do not update the different
> > attributes in the same transaction.  NFSD on the other hand passes
> > the attributes it gets on the wire more or less directly through to
> > the VFS, leading to updates the file systems don't expect.  XFS at
> > least has an assert on the allowed attributes, which cought an NFS
> > client sets the size and group ІD at the same time.
> > 
> > To handles this issue properly this switches nfsd to call vfs_truncate
> > for size changes, and then handling all other attributes through
> > notify_change.  As a side effect this also means less boilerplace
> > code around the size change as we can now reuse the VFS code.
> > 
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > ---
> >  fs/nfsd/vfs.c | 92 +++++++++++++++++++----------------------------------------
> >  1 file changed, 30 insertions(+), 62 deletions(-)
> > 
> > diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> > index 26c6fdb..4ca5b92 100644
> > --- a/fs/nfsd/vfs.c
> > +++ b/fs/nfsd/vfs.c
> > @@ -332,37 +332,6 @@ nfsd_sanitize_attrs(struct inode *inode, struct iattr *iap)
> >  	}
> >  }
> >  
> > -static __be32
> > -nfsd_get_write_access(struct svc_rqst *rqstp, struct svc_fh *fhp,
> > -		struct iattr *iap)
> > -{
> > -	struct inode *inode = d_inode(fhp->fh_dentry);
> > -	int host_err;
> > -
> > -	if (iap->ia_size < inode->i_size) {
> > -		__be32 err;
> > -
> > -		err = nfsd_permission(rqstp, fhp->fh_export, fhp->fh_dentry,
> > -				NFSD_MAY_TRUNC | NFSD_MAY_OWNER_OVERRIDE);
> > -		if (err)
> > -			return err;
> > -	}
> > -
> > -	host_err = get_write_access(inode);
> > -	if (host_err)
> > -		goto out_nfserrno;
> > -
> > -	host_err = locks_verify_truncate(inode, NULL, iap->ia_size);
> > -	if (host_err)
> > -		goto out_put_write_access;
> > -	return 0;
> > -
> > -out_put_write_access:
> > -	put_write_access(inode);
> > -out_nfserrno:
> > -	return nfserrno(host_err);
> > -}
> > -
> >  /*
> >   * Set various file attributes.  After this call fhp needs an fh_put.
> >   */
> > @@ -377,7 +346,6 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp, struct iattr *iap,
> >  	__be32		err;
> >  	int		host_err;
> >  	bool		get_write_count;
> > -	int		size_change = 0;
> >  
> >  	if (iap->ia_valid & (ATTR_ATIME | ATTR_MTIME | ATTR_SIZE))
> >  		accmode |= NFSD_MAY_WRITE|NFSD_MAY_OWNER_OVERRIDE;
> > @@ -390,11 +358,11 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp, struct iattr *iap,
> >  	/* Get inode */
> >  	err = fh_verify(rqstp, fhp, ftype, accmode);
> >  	if (err)
> > -		goto out;
> > +		return err;
> >  	if (get_write_count) {
> >  		host_err = fh_want_write(fhp);
> >  		if (host_err)
> > -			return nfserrno(host_err);
> > +			goto out_host_err;
> >  	}
> >  
> >  	dentry = fhp->fh_dentry;
> > @@ -405,50 +373,50 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp, struct iattr *iap,
> >  		iap->ia_valid &= ~ATTR_MODE;
> >  
> >  	if (!iap->ia_valid)
> > -		goto out;
> > +		return 0;
> >  
> >  	nfsd_sanitize_attrs(inode, iap);
> >  
> > +	if (check_guard && guardtime != inode->i_ctime.tv_sec)
> > +		return nfserr_notsync;
> > +
> >  	/*
> >  	 * The size case is special, it changes the file in addition to the
> > -	 * attributes.
> > +	 * attributes, and file systems don't expect it to be mixed with
> > +	 * "random" attribute changes.  We thus split out the size change
> > +	 * into a separate calo for vfs_truncate, and do the rest as a
> > +	 * a separate setattr call.
> > +	 *
> > +	 * Note that vfs_truncate will also update ctime and mtime if
> > +	 * the file size changes.
> >  	 */
> >  	if (iap->ia_valid & ATTR_SIZE) {
> > -		err = nfsd_get_write_access(rqstp, fhp, iap);
> > -		if (err)
> > -			goto out;
> > -		size_change = 1;
> > +		struct path path = {
> > +			.mnt	= fhp->fh_export->ex_path.mnt,
> > +			.dentry	= dentry,
> > +		};
> >  
> > -		/*
> > -		 * RFC5661, Section 18.30.4:
> > -		 *   Changing the size of a file with SETATTR indirectly
> > -		 *   changes the time_modify and change attributes.
> > -		 *
> > -		 * (and similar for the older RFCs)
> > -		 */
> > -		if (iap->ia_size != i_size_read(inode))
> > -			iap->ia_valid |= ATTR_MTIME;
> > +		host_err = vfs_truncate(&path, iap->ia_size);
> > +		if (host_err)
> > +			goto out_host_err;
> > +
> > +		iap->ia_valid &= ~ATTR_SIZE;
> > +		if (!iap->ia_valid)
> > +			goto done;
> >  	}
> >  
> >  	iap->ia_valid |= ATTR_CTIME;
> >  
> > -	if (check_guard && guardtime != inode->i_ctime.tv_sec) {
> > -		err = nfserr_notsync;
> > -		goto out_put_write_access;
> > -	}
> > -
> >  	fh_lock(fhp);
> >  	host_err = notify_change(dentry, iap, NULL);
> >  	fh_unlock(fhp);
> > -	err = nfserrno(host_err);
> > +	if (host_err)
> > +		goto out_host_err;
> >  
> > -out_put_write_access:
> > -	if (size_change)
> > -		put_write_access(inode);
> > -	if (!err)
> > -		err = nfserrno(commit_metadata(fhp));
> > -out:
> > -	return err;
> > +done:
> > +	host_err = commit_metadata(fhp);
> > +out_host_err:
> > +	return nfserrno(host_err);
> >  }
> >  
> >  #if defined(CONFIG_NFSD_V4)
> 
> -- 
> Jeff Layton <jlayton@poochiereds.net>