On Tue, 1 Nov 2011 21:23:15 -0400 "J. Bruce Fields" <bfields@xxxxxxxxxxxx> wrote: > On Tue, Nov 01, 2011 at 08:43:25PM -0400, Jeff Layton wrote: > > On Tue, 1 Nov 2011 16:07:27 -0700 > > "Myklebust, Trond" <Trond.Myklebust@xxxxxxxxxx> wrote: > > > > > > -----Original Message----- > > > > From: J. Bruce Fields [mailto:bfields@xxxxxxxxxxxx] > > > > Sent: Tuesday, November 01, 2011 4:27 PM > > > > To: Myklebust, Trond > > > > Cc: J. Bruce Fields; Myklebust, Trond; linux-nfs@xxxxxxxxxxxxxxx > > > > Subject: Re: [PATCH] nfs: open-associated setattr shouldn't invalidate > > > own > > > > cache > > > > > > > > On Mon, Jul 18, 2011 at 08:09:15PM -0400, Myklebust, Trond wrote: > > > > > We should already optimize away the unnecessary setting of the size. > > > > > > > > Do you remember what commit fixed that? (Was it an nfs change or a > > > vfs > > > > change?) > > > > > > It predates the git repository. See the comment about "Optimization:" in > > > nfs_setattr(). > > > > > > > > The problem is that truncate() still requires you to set the ctime, > > > whereas > > > > ftruncate() does not iirc. > > > > > > > > Staring at the code.... I think you mean the opposite? I notice > > > > do_sys_ftruncate() calling > > > > > > > > do_truncate(dentry, length, ATTR_MTIME|ATTR_CTIME, file); > > > > > > > > and do_sys_truncate() calling > > > > > > > > do_truncate(path.dentry, length, 0, NULL); > > > > > > > > where the third argument is getting OR'd with ATTR_FILE to pass into > > > > notify_change(). > > > > > > Sorry, yes. ftruncate() is the one that unconditionally sets the > > > mtime/ctime on success according to the POSIX spec. > > > > > > > Even when it's a noop? Blech. > > > > > > Also even when a setattr does get through, I don't understand why it > > > should > > > > be invalidating our data cache. Is there some reason it needs to, or > > > is this just > > > > a case that hasn't seemed worth fixing? > > > > > > Is the problem perhaps that we should be clearing the > > > NFS_INO_INVALID_DATA flag in nfs_vmtruncate() when the size gets set to > > > zero? > > > > > > > That was my thinking too. Whenever we truncate the i_size to 0, we > > can safely assume that the pagecache is now valid, and should be able > > to clear NFS_INO_INVALID_DATA no matter when it was set, right? > > I don't understand why 0 is a special case: why should my setting the > size ever mean that I have to go reread data from the server? > If the server doesn't send pre-op attrs (and linux servers don't) then you have no way to know whether someone raced in and did writes to the remaining pages just prior to your truncate. Size 0 is a special case because there are no remaining pages. -- Jeff Layton <jlayton@xxxxxxxxxx> -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
