Re: i_version changes

["J. Bruce Fields" <bfields@fieldses.org>]

On Wed, Feb 13, 2008 at 01:52:14PM +0100, Christoph Hellwig wrote:
> On Tue, Feb 12, 2008 at 03:06:25PM -0500, J. Bruce Fields wrote:
> > > First there's a complete lack of documentation on this, which is very
> > > bad.  Please document what the new semantics for i_version on regular
> > > files are supposed to be, and how it differes from the existing
> > > semantics for directories.
> > > 
> > > Second abusing one of the rather scare superblock mount flags is
> > > a bad idea.  It would be much better to set this through ->setattr
> > > and an extension of struct iattr.  Especially as we need to convert
> > > file_update_time to update c and mtime through ->setattr anyway.
> > 
> > I don't understand that comment.  (What is "this" in the second
> > sentence, for example?)
> 
> i_version.  Instead of hardcoding i_version updates in file_update_time
> it would be better to add an ia_verstion to struct iattr and update it
> that way.

Got it, thanks.

> > > Third using the MS_ flag but then actually having a filesystem
> > > mount option to enable it is more than confusing.  After all MS_
> > > options (at least the exported parts) are the mount ABI for common
> > > options.  Also this option doesn't show up in ->show_options,
> > > which is something Miklos will beat you up for :)
> > > I'm also not convinced this should be option behaviour, either you
> > > do update i_version for a given filesystem or you don't - having
> > > an obscure mount option will only give you confusion.
> > 
> > That does sound likely to be confusing.  Any chance we could just make
> > the new behavior mandatory?
> > 
> > The one thing we need in nfsd is just an easy (in-kernel) way to check
> > whether a given filesystem supports this, so nfsd can decide whether to
> > use ctime or i_version as the change attribute.
> 
> Probably through export_operations somehow.  Andreas mentioned in the
> other reply that he wants it only conditionally due to the overhead
> on extN, and enabling this from an export operation called when nfs
> exporting a filesystem.

It's not OK to update it only sometimes.  If updates are made while nfsd
isn't running, those needed to be reflected in the change attribute, so
the changes aren't missed when nfsd comes back up.

So I'm inclined to think this should be an option that's permanently
associated with the filesystem, rather than something that's changed on
or off by nfsd (or even on or off with each mount).

--b.