* [PATCH 1/2] nfs: setattr can only change regular file sizes @ 2014-09-07 15:36 Christoph Hellwig 2014-09-07 15:36 ` [PATCH 2/2] nfs: update time staps on truncate Christoph Hellwig 0 siblings, 1 reply; 5+ messages in thread From: Christoph Hellwig @ 2014-09-07 15:36 UTC (permalink / raw) To: linux-nfs 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH 2/2] nfs: update time staps on truncate 2014-09-07 15:36 [PATCH 1/2] nfs: setattr can only change regular file sizes Christoph Hellwig @ 2014-09-07 15:36 ` Christoph Hellwig 2014-09-07 16:41 ` Trond Myklebust 0 siblings, 1 reply; 5+ messages in thread From: Christoph Hellwig @ 2014-09-07 15:36 UTC (permalink / raw) To: linux-nfs 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 2/2] nfs: update time staps on truncate 2014-09-07 15:36 ` [PATCH 2/2] nfs: update time staps on truncate Christoph Hellwig @ 2014-09-07 16:41 ` Trond Myklebust 2014-09-07 16:57 ` Christoph Hellwig 0 siblings, 1 reply; 5+ messages in thread From: Trond Myklebust @ 2014-09-07 16:41 UTC (permalink / raw) To: Christoph Hellwig; +Cc: Linux NFS Mailing List 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 2/2] nfs: update time staps on truncate 2014-09-07 16:41 ` Trond Myklebust @ 2014-09-07 16:57 ` Christoph Hellwig 2014-09-07 20:59 ` Trond Myklebust 0 siblings, 1 reply; 5+ messages in thread From: Christoph Hellwig @ 2014-09-07 16:57 UTC (permalink / raw) To: Trond Myklebust; +Cc: Christoph Hellwig, Linux NFS Mailing List 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] ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 2/2] nfs: update time staps on truncate 2014-09-07 16:57 ` Christoph Hellwig @ 2014-09-07 20:59 ` Trond Myklebust 0 siblings, 0 replies; 5+ messages in thread From: Trond Myklebust @ 2014-09-07 20:59 UTC (permalink / raw) To: Christoph Hellwig; +Cc: Linux NFS Mailing List 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2014-09-07 20:59 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-09-07 15:36 [PATCH 1/2] nfs: setattr can only change regular file sizes Christoph Hellwig 2014-09-07 15:36 ` [PATCH 2/2] nfs: update time staps on truncate Christoph Hellwig 2014-09-07 16:41 ` Trond Myklebust 2014-09-07 16:57 ` Christoph Hellwig 2014-09-07 20:59 ` Trond Myklebust
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.