| Message ID | 1410104201-32517-2-git-send-email-hch@lst.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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?
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] -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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 */
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(+)