[PATCH 1/2] nfs: setattr can only change regular file sizes

[PATCH 1/2] nfs: setattr can only change regular file sizes

[Christoph Hellwig]

The VFS never calls setattr with ATTR_SIZE on anything but regular
files.  Remove the if check and turn it into an assert similar to
what some other file systems do.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/nfs/inode.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
index 577a36f..141c9f4 100644
--- a/fs/nfs/inode.c
+++ b/fs/nfs/inode.c
@@ -505,7 +505,9 @@ nfs_setattr(struct dentry *dentry, struct iattr *attr)
 		attr->ia_valid &= ~ATTR_MODE;
 
 	if (attr->ia_valid & ATTR_SIZE) {
-		if (!S_ISREG(inode->i_mode) || attr->ia_size == i_size_read(inode))
+		BUG_ON(!S_ISREG(inode->i_mode));
+
+		if (attr->ia_size == i_size_read(inode))
 			attr->ia_valid &= ~ATTR_SIZE;
 	}
 
-- 
1.9.1

[PATCH 2/2] nfs: update time staps on truncate

[Christoph Hellwig]

The VFS handles attributes on truncate in a strange way, fix NFS to
properly update timestamps on truncate().

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/nfs/inode.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
index 141c9f4..97bc485 100644
--- a/fs/nfs/inode.c
+++ b/fs/nfs/inode.c
@@ -507,8 +507,20 @@ nfs_setattr(struct dentry *dentry, struct iattr *attr)
 	if (attr->ia_valid & ATTR_SIZE) {
 		BUG_ON(!S_ISREG(inode->i_mode));
 
+		/*
+		 * Calling conventions are a little "unconventional" for
+		 * truncate:
+		 *  - for truncate() the VFS does not set ATTR_CTIME and
+		 *    ATTR_MTIME, but we still need need to update the time
+		 *    stamps if we change the file size.
+		 *  - for ftruncate() the VFS sets ATTR_CTIME and ATTR_MTIME,
+		 *    and we always need to change the time stamp, even if
+		 *    the size does not change.
+		 */
 		if (attr->ia_size == i_size_read(inode))
 			attr->ia_valid &= ~ATTR_SIZE;
+		else if (!(attr->ia_valid & (ATTR_CTIME | ATTR_MTIME)))
+			attr->ia_valid |= ATTR_CTIME | ATTR_MTIME;
 	}
 
 	/* Optimization: if the end result is no change, don't RPC */
-- 
1.9.1

Re: [PATCH 2/2] nfs: update time staps on truncate

[Trond Myklebust]

On Sun, Sep 7, 2014 at 11:36 AM, Christoph Hellwig <hch@lst.de> wrote:
> The VFS handles attributes on truncate in a strange way, fix NFS to
> properly update timestamps on truncate().
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/nfs/inode.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
> index 141c9f4..97bc485 100644
> --- a/fs/nfs/inode.c
> +++ b/fs/nfs/inode.c
> @@ -507,8 +507,20 @@ nfs_setattr(struct dentry *dentry, struct iattr *attr)
>         if (attr->ia_valid & ATTR_SIZE) {
>                 BUG_ON(!S_ISREG(inode->i_mode));
>
> +               /*
> +                * Calling conventions are a little "unconventional" for
> +                * truncate:
> +                *  - for truncate() the VFS does not set ATTR_CTIME and
> +                *    ATTR_MTIME, but we still need need to update the time
> +                *    stamps if we change the file size.
> +                *  - for ftruncate() the VFS sets ATTR_CTIME and ATTR_MTIME,
> +                *    and we always need to change the time stamp, even if
> +                *    the size does not change.
> +                */
>                 if (attr->ia_size == i_size_read(inode))
>                         attr->ia_valid &= ~ATTR_SIZE;
> +               else if (!(attr->ia_valid & (ATTR_CTIME | ATTR_MTIME)))
> +                       attr->ia_valid |= ATTR_CTIME | ATTR_MTIME;
>         }
>
>         /* Optimization: if the end result is no change, don't RPC */
> --
>

I'm still confused as to why we need this change to the client. In
RFC5661, Section 18.30.4 there is wording to the effect that:

   Changing the size of a file with SETATTR indirectly changes the
   time_modify and change attributes.  A client must account for this as
   size changes can result in data deletion.

There is similar wording in RFC3530 and even in RFC1813 (see section
3.3.2). Without this sort of guarantee by the server, you could, for
instance, get into nasty situations where the file changes, but the
NFSv3 client doesn't detect it.

So basically, I interpret the spec is saying that we should be able to
rely on the server figuring this out all by itself. I agree that we
have the special corner case of ftruncate(), but the VFS is still
setting the ATTR_CTIME|ATTR_MTIME flags for us in that case, right?

-- 
Trond Myklebust

Linux NFS client maintainer, PrimaryData

trond.myklebust@primarydata.com

Re: [PATCH 2/2] nfs: update time staps on truncate

[Christoph Hellwig]

On Sun, Sep 07, 2014 at 12:41:28PM -0400, Trond Myklebust wrote:
> I'm still confused as to why we need this change to the client. In
> RFC5661, Section 18.30.4 there is wording to the effect that:
> 
>    Changing the size of a file with SETATTR indirectly changes the
>    time_modify and change attributes.  A client must account for this as
>    size changes can result in data deletion.
> 
> There is similar wording in RFC3530 and even in RFC1813 (see section
> 3.3.2). Without this sort of guarantee by the server, you could, for
> instance, get into nasty situations where the file changes, but the
> NFSv3 client doesn't detect it.
> 
> So basically, I interpret the spec is saying that we should be able to
> rely on the server figuring this out all by itself. I agree that we
> have the special corner case of ftruncate(), but the VFS is still
> setting the ATTR_CTIME|ATTR_MTIME flags for us in that case, right?

Yes.  Looks like the problem is on the server side indeed.  nfsd_setattr
adds a non-conditional ATTR_CTIME for all calls, but never adds ATTR_MTIME
for ATTR_SIZE requests.  I'll send a server patch instead.

[patch 1 in the series still seems worthwhile for the client, though]

Re: [PATCH 2/2] nfs: update time staps on truncate

[Trond Myklebust]

On Sun, Sep 7, 2014 at 11:57 AM, Christoph Hellwig <hch@lst.de> wrote:
> [patch 1 in the series still seems worthwhile for the client, though]

Agreed, and I plan to apply that.

-- 
Trond Myklebust

Linux NFS client maintainer, PrimaryData

trond.myklebust@primarydata.com