On Sun, Feb 10, 2008 at 08:30:41AM +0100, Christoph Hellwig wrote: > 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?) > > 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. > 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@xxxxxxxxxxxxxxx > 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@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
