Re: [PATCH] nfs: open-associated setattr shouldn't invalidate own cache

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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


[Index of Archives]     [Linux Filesystem Development]     [Linux USB Development]     [Linux Media Development]     [Video for Linux]     [Linux NILFS]     [Linux Audio Users]     [Yosemite Info]     [Linux SCSI]

  Powered by Linux