> -----Original Message----- > From: J. Bruce Fields [mailto:bfields@xxxxxxxxxxxx] > Sent: Tuesday, November 01, 2011 9:23 PM > To: Jeff Layton > Cc: Myklebust, Trond; J. Bruce Fields; linux-nfs@xxxxxxxxxxxxxxx > Subject: Re: [PATCH] nfs: open-associated setattr shouldn't invalidate own > cache > > 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? Your test case was: open(O_RDWR|O_TRUNCATE) write() close() open(O_RDONLY) read() ... That truncates _all_ data, so there is nothing left in the data cache when we get to the write(). The question therefore is not "Why does setattr clear my cache?". The question is "Why isn't that write() cached?". That's why I'm exploring reasons why the cache might get cleared after that write(). The other obvious one would be if the server is failing to return post-op attributes in the WRITE reply. Trond -- 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
