i_version changes

i_version changes

[Christoph Hellwig]

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.

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.

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.

Re: i_version changes

[J. Bruce Fields]

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@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: i_version changes

[Andreas Dilger]

On Feb 12, 2008  15:06 -0500, J. Bruce Fields wrote:
> On Sun, Feb 10, 2008 at 08:30:41AM +0100, Christoph Hellwig wrote:
> > 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?

One of the reasons NOT to make it mandatory is that it forces updates
of the inode after every write.  On ext3/ext4 this is expensive, as the
ext3_dirty_inode() packs the inode from memory into the buffer each time,
so that it can be journaled.

Cheers, Andreas
--
Andreas Dilger
Sr. Staff Engineer, Lustre Group
Sun Microsystems of Canada, Inc.

Re: i_version changes

[Christoph Hellwig]

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.

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

Btw, stupid question:  the commit message for the i_version changes
mentions this is to work around lack of granularity for ctime updates.
But all modern filesystems (and I includ ext4 in that here) have 64bit
timestamps already, so that should be enough.  It would certainly
avoid all this additional code, and especially the additional space
used in struct inode which can hurt quite a lot.

Re: i_version changes

[Trond Myklebust]

On Wed, 2008-02-13 at 13:52 +0100, Christoph Hellwig wrote:
> Btw, stupid question:  the commit message for the i_version changes
> mentions this is to work around lack of granularity for ctime updates.
> But all modern filesystems (and I includ ext4 in that here) have 64bit
> timestamps already, so that should be enough.  It would certainly
> avoid all this additional code, and especially the additional space
> used in struct inode which can hurt quite a lot.

Support for 64-bit on-disk time stamps alone does not suffice. In order
to label all file/directory changes unambiguously, you would also need
nanosecond timekeeping support.

An example is XFS, which has had on-disk support for 64-bit time stamps
since forever, but is in practice limited by the Linux default 250Hz
internal clock. We've seen plenty of examples of NFS clients missing
updates on the resulting filesystem due to the fact that they occurred
within 1/250 sec of each other.

Cheers
  Trond

Re: i_version changes

[Andreas Dilger]

On Feb 13, 2008  09:07 -0500, Trond Myklebust wrote:
> On Wed, 2008-02-13 at 13:52 +0100, Christoph Hellwig wrote:
> > Btw, stupid question:  the commit message for the i_version changes
> > mentions this is to work around lack of granularity for ctime updates.
> > But all modern filesystems (and I includ ext4 in that here) have 64bit
> > timestamps already, so that should be enough.  It would certainly
> > avoid all this additional code, and especially the additional space
> > used in struct inode which can hurt quite a lot.
> 
> Support for 64-bit on-disk time stamps alone does not suffice. In order
> to label all file/directory changes unambiguously, you would also need
> nanosecond timekeeping support.
> 
> An example is XFS, which has had on-disk support for 64-bit time stamps
> since forever, but is in practice limited by the Linux default 250Hz
> internal clock. We've seen plenty of examples of NFS clients missing
> updates on the resulting filesystem due to the fact that they occurred
> within 1/250 sec of each other.

The other issue which unfortunately makes ctime a non-starter is the
ability of ctime to go backward due to clock changes.

Cheers, Andreas
--
Andreas Dilger
Sr. Staff Engineer, Lustre Group
Sun Microsystems of Canada, Inc.

Re: i_version changes

[J. Bruce Fields]

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.

Re: i_version changes

[NeilBrown]

On Thu, February 14, 2008 7:26 am, J. Bruce Fields wrote:

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

For NFSD's needs, it is only necessary that changes in i_version that are
potentially visible over NFS actually be stored on disk.

You could come up with an interface where NFSD sets a flag when it reads
i_version, and changes to the file only change i_version if the flag is
set (at which point the flag is cleared).

This would give fully correct NFS semantics, and no overhead when NFS access
is not in use

This flag would need to be stored in stable storage too, so probably easiest
to make it the least significant bit of i_version.

Of course then the semantics of the new i_version are very different to
the old i_version, so maybe we need two fields in the inode....

NeilBrown

Re: i_version changes

[Peter Staubach]

NeilBrown wrote:
> On Thu, February 14, 2008 7:26 am, J. Bruce Fields wrote:
>
>   
>> 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.
>>     
>
> For NFSD's needs, it is only necessary that changes in i_version that are
> potentially visible over NFS actually be stored on disk.
>
> You could come up with an interface where NFSD sets a flag when it reads
> i_version, and changes to the file only change i_version if the flag is
> set (at which point the flag is cleared).
>
> This would give fully correct NFS semantics, and no overhead when NFS access
> is not in use
>
>   

I don't think that this is quite true.  If the file is changed
when the NFS server is not running, then the value of i_version
which is used when the NFS server starts up again must be
different than the value which was previously used when the NFS
server was previously running.

Is the perceived performance hit really going to be as large
as suspected?  We already update the time fields fairly often
and we don't pay a huge penalty for those, or at least not a
penalty that we aren't willing to pay.  Has anyone measured
the cost?

    Thanx...

       ps

> This flag would need to be stored in stable storage too, so probably easiest
> to make it the least significant bit of i_version.
>
> Of course then the semantics of the new i_version are very different to
> the old i_version, so maybe we need two fields in the inode....
>
> NeilBrown
>
> -
> 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
>   

Re: i_version changes

[J. Bruce Fields]

On Thu, Feb 14, 2008 at 08:19:19AM +1100, NeilBrown wrote:
> On Thu, February 14, 2008 7:26 am, J. Bruce Fields wrote:
> 
> > 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.
> 
> For NFSD's needs, it is only necessary that changes in i_version that are
> potentially visible over NFS actually be stored on disk.
> 
> You could come up with an interface where NFSD sets a flag when it reads
> i_version, and changes to the file only change i_version if the flag is
> set (at which point the flag is cleared).
> 
> This would give fully correct NFS semantics, and no overhead when NFS access
> is not in use
> 
> This flag would need to be stored in stable storage too, so probably easiest
> to make it the least significant bit of i_version.
> 
> Of course then the semantics of the new i_version are very different to
> the old i_version, so maybe we need two fields in the inode....

Sure.  I'd rather have a per-superblock "use i_version" bit than a
per-inode "use i_version" bit, just for simplicity's sake, unless
there's a really convincing case that it helps.

--b.

Re: i_version changes

[NeilBrown]

On Thu, February 14, 2008 8:32 am, Peter Staubach wrote:
>
> I don't think that this is quite true.  If the file is changed
> when the NFS server is not running, then the value of i_version
> which is used when the NFS server starts up again must be
> different than the value which was previously used when the NFS
> server was previously running.

As I said, the "NFS has seen this i_version" flag needs to be on
stable storage, e.g. the lsb of the i_version.  This will ensure that
any change after NFSD saw the i_version will cause the i_version to
be updated.
So I think it can provide correct semantics.
Precise details:
  NFSD: when reading i_version
      take lock
      tmp = i_version
      i_version |= 1
      drop lock
      return tmp & ~1;

  VFS when making any change:
      take lock
      if (i_version & 1) {
           i_version++;
           changed=1
      }
      drop lock
      if changed, sync inode


>
> Is the perceived performance hit really going to be as large
> as suspected?  We already update the time fields fairly often
> and we don't pay a huge penalty for those, or at least not a
> penalty that we aren't willing to pay.  Has anyone measured
> the cost?

Correct NFS semantics require that the i_version be written to disk
before (or when) the change is committed.  That means lots more inodes
in the journal.
If you are already doing data=journal, it the hit probably isn't too
high.(?)

You are right:  measuring the cost is important.  However as we are
designing a generic filesystem interface, we need to understand the
cost on multiple filesystems in a variety of configuration .... or
give the filesystem complete information and let it decide the optimal
implementation.

Giving the filesystem full information means having an inode_operation
"nfsd_reads_version" which returns the number to be used as change_id.


NeilBrown

Re: i_version changes

[Jean noel Cordenner]

hi,

Peter Staubach a écrit :
> 
> Is the perceived performance hit really going to be as large
> as suspected?  We already update the time fields fairly often
> and we don't pay a huge penalty for those, or at least not a
> penalty that we aren't willing to pay.  Has anyone measured
> the cost?

Few month ago, I ran a FFSB test on a 2.6.23 kernel enabling or not the 
i_version flag.
http://bullopensource.org/ext4/20071116/ffsb-write.html

regards.

-
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

Re: i_version changes

[Peter Staubach]

NeilBrown wrote:
> On Thu, February 14, 2008 8:32 am, Peter Staubach wrote:
>   
>> I don't think that this is quite true.  If the file is changed
>> when the NFS server is not running, then the value of i_version
>> which is used when the NFS server starts up again must be
>> different than the value which was previously used when the NFS
>> server was previously running.
>>     
>
> As I said, the "NFS has seen this i_version" flag needs to be on
> stable storage, e.g. the lsb of the i_version.  This will ensure that
> any change after NFSD saw the i_version will cause the i_version to
> be updated.
> So I think it can provide correct semantics.
> Precise details:
>   NFSD: when reading i_version
>       take lock
>       tmp = i_version
>       i_version |= 1
>       drop lock
>       return tmp & ~1;
>
>   VFS when making any change:
>       take lock
>       if (i_version & 1) {
>            i_version++;
>            changed=1
>       }
>       drop lock
>       if changed, sync inode
>
>   

Yes, this does seem like it would do the job.  It could perhaps
be optimized somewhat to avoid lock contention, but I do think
that this would suffice.

>   
>> Is the perceived performance hit really going to be as large
>> as suspected?  We already update the time fields fairly often
>> and we don't pay a huge penalty for those, or at least not a
>> penalty that we aren't willing to pay.  Has anyone measured
>> the cost?
>>     
>
> Correct NFS semantics require that the i_version be written to disk
> before (or when) the change is committed.  That means lots more inodes
> in the journal.
> If you are already doing data=journal, it the hit probably isn't too
> high.(?)
>
>   

Correct NFS semantics also require that any modified metadata,
including file times and file size, also be written to stable
storage.  Isn't this just another piece of modified metadata
that would go hand-in-hand with updated file times?

We should also require that the file mtime change when the
contents of the file are modified.  This should happen whether
or not the clock has ticked.  Unfortunately, to implement this,
we would need file time resolutions which are smaller granularity
than the system clock.  We could probably get away with nano-
second resolutions in the file system.

    Thanx...

       ps

> You are right:  measuring the cost is important.  However as we are
> designing a generic filesystem interface, we need to understand the
> cost on multiple filesystems in a variety of configuration .... or
> give the filesystem complete information and let it decide the optimal
> implementation.
>
> Giving the filesystem full information means having an inode_operation
> "nfsd_reads_version" which returns the number to be used as change_id.
>
>
> NeilBrown
>
>   

Re: i_version changes

[Peter Staubach]

Jean noel Cordenner wrote:
> hi,
>
> Peter Staubach a écrit :
>>
>> Is the perceived performance hit really going to be as large
>> as suspected?  We already update the time fields fairly often
>> and we don't pay a huge penalty for those, or at least not a
>> penalty that we aren't willing to pay.  Has anyone measured
>> the cost?
>
> Few month ago, I ran a FFSB test on a 2.6.23 kernel enabling or not 
> the i_version flag.
> http://bullopensource.org/ext4/20071116/ffsb-write.html

This is good information.

A couple of questions -- what is the "-I 256" option used for the ext4
mkfs?

What was the variance between the results of the 5 runs?  Is 2%
significant or not?

    Thanx...

       ps
-
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

Re: i_version changes

[Jean noel Cordenner]

Peter Staubach a écrit :

>> Few month ago, I ran a FFSB test on a 2.6.23 kernel enabling or not 
>> the i_version flag.
>> http://bullopensource.org/ext4/20071116/ffsb-write.html
> 
> This is good information.
> 
> A couple of questions -- what is the "-I 256" option used for the ext4
> mkfs?
> 

This option force the inode size to 256Bytes instead of 128. The 
requirement is to have a 64bits i_version counter, therefore the ext4 
inode as to be extended to 256Bytes.

> What was the variance between the results of the 5 runs?  Is 2%
> significant or not?
> 

I didn't keep the raw results, but they were all very close to the 
average (less than 1% of the average).

-
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