On Sun, Feb 10, 2008 at 08:30:41AM +0100, Christoph Hellwig wrote:
quoted text
> I think the i_version changes that hit mainline about a week ago are
> not as nice as they should be.
> 
> 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?)

quoted text
> 
> 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.

--b.

quoted text
> Beyond those any good reason for making inode_inc_iversion inline,
> especially after the first patch introduced it properly out of line.
> 
> And as a last note please stop pushing these kind of core changes
> through specific filesystem trees.  If this had been in ->mm we
> would have caught this a lot earlier, and would have also meant you'd
> get input and possible even implementations from other filesystem
> maintainers.
> -
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html