2014-08-21 15:05:29

by Christoph Hellwig

Subject: [PATCH] nfs: update time staps on truncate

The VFS handles attributes on truncate in a strange way, fix NFS to handle
it properly by copying a small code sniplet from XFS.

Signed-off-by: Christoph Hellwig <[email protected]>
---
fs/nfs/inode.c | 22 +++++++++++++++++++++-
1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
index 577a36f..5bbd991 100644
--- a/fs/nfs/inode.c
+++ b/fs/nfs/inode.c
@@ -505,8 +505,28 @@ 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))
+ if (!S_ISREG(inode->i_mode) ||
+ attr->ia_size == i_size_read(inode))
attr->ia_valid &= ~ATTR_SIZE;
+
+ /*
+ * Only change the c/mtime if we are changing the size or we
+ * are explicitly asked to change it. This handles the
+ * semantic difference between truncate() and ftruncate() as
+ * implemented in the VFS.
+ *
+ * The regular truncate() case without ATTR_CTIME and
+ * ATTR_MTIME is a special case where we need to update the
+ * times despite not having these flags set. For all other
+ * operations the VFS set these flags explicitly if it wants
+ * a timestamp update.
+ */
+ if (attr->ia_size != i_size_read(inode) &&
+ !(attr->ia_valid & (ATTR_CTIME | ATTR_MTIME))) {
+ attr->ia_ctime = attr->ia_mtime =
+ current_fs_time(inode->i_sb);
+ attr->ia_valid |= ATTR_CTIME | ATTR_MTIME;
+ }
}

/* Optimization: if the end result is no change, don't RPC */
--
1.9.1

2014-08-21 15:31:25

by Trond Myklebust

Subject: Re: [PATCH] nfs: update time staps on truncate

Hi Christoph,

On Thu, Aug 21, 2014 at 11:07 AM, Christoph Hellwig <[email protected]> wrote:
> The VFS handles attributes on truncate in a strange way, fix NFS to handle
> it properly by copying a small code sniplet from XFS.
>
> Signed-off-by: Christoph Hellwig <[email protected]>
> ---
> fs/nfs/inode.c | 22 +++++++++++++++++++++-
> 1 file changed, 21 insertions(+), 1 deletion(-)
>
> diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
> index 577a36f..5bbd991 100644
> --- a/fs/nfs/inode.c
> +++ b/fs/nfs/inode.c
> @@ -505,8 +505,28 @@ 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))
> + if (!S_ISREG(inode->i_mode) ||
> + attr->ia_size == i_size_read(inode))
> attr->ia_valid &= ~ATTR_SIZE;
> +
> + /*
> + * Only change the c/mtime if we are changing the size or we
> + * are explicitly asked to change it. This handles the
> + * semantic difference between truncate() and ftruncate() as
> + * implemented in the VFS.
> + *
> + * The regular truncate() case without ATTR_CTIME and
> + * ATTR_MTIME is a special case where we need to update the
> + * times despite not having these flags set. For all other
> + * operations the VFS set these flags explicitly if it wants
> + * a timestamp update.
> + */
> + if (attr->ia_size != i_size_read(inode) &&
> + !(attr->ia_valid & (ATTR_CTIME | ATTR_MTIME))) {
> + attr->ia_ctime = attr->ia_mtime =
> + current_fs_time(inode->i_sb);
> + attr->ia_valid |= ATTR_CTIME | ATTR_MTIME;
> + }
> }
>

We shouldn't need to set attr->ia_ctime/ia_mtime above. We should set
the file time to the server clock time.
Also note that instead of checking attr->ia_size !=
i_size_read(inode), the above should check attr->ia_valid & ATTR_SIZE
due to the optimisation above.

--
Trond Myklebust

Linux NFS client maintainer, PrimaryData

[email protected]

2014-09-06 23:28:35

by Christoph Hellwig

Subject: Re: [PATCH] nfs: update time staps on truncate

On Thu, Aug 21, 2014 at 11:31:24AM -0400, Trond Myklebust wrote:
> We shouldn't need to set attr->ia_ctime/ia_mtime above. We should set
> the file time to the server clock time.

True.

> Also note that instead of checking attr->ia_size !=
> i_size_read(inode), the above should check attr->ia_valid & ATTR_SIZE
> due to the optimisation above.

Or we could just make it an if/else clause. I'll send out an updated
patch ASAP.