ext4: turn on i_version updates by default

knfsd needs i_version updates on, as will userspace nfs servers and probably others.

The only effects are that inode->i_version is bumped (under the i_lock) in more places, and that ->dirty_inode(I_DIRTY_DATASYNC) may be called more frequently than once per jiffy on write (see file_update_time). However the latter appears to be mostly a no-op in that case.

So, simplify our life and just keep this feature turned on all the time.

Signed-off-by J. Bruce Fields <[email protected]>

---
fs/ext4/super.c | 3 ++-

1 file changed, 2 insertions(+), 1 deletion(-)

If people are worried that the performance impact isn't obvious, I'll find the time somehow to test this properly....

fs/ext4/super.c +2 -1

index e1fb1d5..a99f827 100644
@@ -1483,7 +1483,7 @@ static int handle_mount_opt(struct super_block *sb, char *opt, int token,
       sbi->s_mount_flags |= EXT4_MF_FS_ABORTED;
       return 1;
   case Opt_i_version:
-        sb->s_flags |= MS_I_VERSION;
+        /* no-op; this is on by default now */
       return 1;
   case Opt_journal_dev:
       if (is_remount) {

@@ -2979,6 +2979,7 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
       goto out_free_orig;
   }
   sb->s_fs_info = sbi;
+    sb->s_flags |= MS_I_VERSION;
   sbi->s_mount_opt = 0;
   sbi->s_resuid = EXT4_DEF_RESUID;
   sbi->s_resgid = EXT4_DEF_RESGID;

Signature · 6 lines

1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Quoted 7 lines · J. Bruce Fields

knfsd needs i_version updates on, as will userspace nfs servers and probably others.

The only effects are that inode->i_version is bumped (under the i_lock) in more places, and that ->dirty_inode(I_DIRTY_DATASYNC) may be called more frequently than once per jiffy on write (see file_update_time). However the latter appears to be mostly a no-op in that case.

I thought this can have noticeable performance impact, since ext4_mark_inode_dirty() is quite heavyweight?

This is one of the reasons that the i_version update is conditional. If someone is exporting a filesystem from userspace the should be able to turn this on as a mount option, and knfsd could do it from inside the kernel. Why add overhead when it is not needed?

Quoted 9 lines · Andreas Dilger

Quoted 7 lines · J. Bruce Fields

knfsd needs i_version updates on, as will userspace nfs servers and probably others.

The only effects are that inode->i_version is bumped (under the i_lock) in more places, and that ->dirty_inode(I_DIRTY_DATASYNC) may be called more frequently than once per jiffy on write (see file_update_time). However the latter appears to be mostly a no-op in that case.

Quoted 1 line

I thought this can have noticeable performance impact, since ext4_mark_inode_dirty() is quite heavyweight?

There's no reason it should be, should it, if we already just dirtied the inode a moment ago?

Quoted 4 lines

This is one of the reasons that the i_version update is conditional. If someone is exporting a filesystem from userspace the should be able to turn this on as a mount option, and knfsd could do it from inside the kernel. Why add overhead when it is not needed?

Any user of the change attribute also wants it to function correctly while they're away.

And if it at all possible I'd rather have it be something that Just Works rather than something that requires extra configuration.

Quoted 2 lines

And if it at all possible I'd rather have it be something that Just Works rather than something that requires extra configuration.

Sure, but this is only useful for NFSv4, but costs everyone using ext4 continuous overhead, so it isn't a clear-cut case to enable the version just on the thought that NFS might one day be used on any particular filesystem.

Quoted 7 lines · Andreas Dilger

Quoted 2 lines

And if it at all possible I'd rather have it be something that Just Works rather than something that requires extra configuration.

Sure, but this is only useful for NFSv4, but costs everyone using ext4 continuous overhead, so it isn't a clear-cut case to enable the version just on the thought that NFS might one day be used on any particular filesystem.

It's not a matter of "NFSv4 might one day be used"; if we don't turn on i_version updates until the file system is actually exported via NFSv4, there would be no deleterious effects.

I always thought that was going to be the plan; that there would be some flag that would be set in struct super_block when the file system was exported that would enable i_version updates.

That way we satisfy the "no extra configuration" needed requirement, which I agree is ideal, but we also don't waste any CPU overhead if the file system is not exported via NFSv4. I tried to implement anything along these lines because I don't care enough, and I don't use NFSv4 personally....

Quoted 17 lines · Ted Ts'o

Quoted 7 lines · Andreas Dilger

Quoted 2 lines

And if it at all possible I'd rather have it be something that Just Works rather than something that requires extra configuration.

Sure, but this is only useful for NFSv4, but costs everyone using ext4 continuous overhead, so it isn't a clear-cut case to enable the version just on the thought that NFS might one day be used on any particular filesystem.

It's not a matter of "NFSv4 might one day be used"; if we don't turn on i_version updates until the file system is actually exported via NFSv4, there would be no deleterious effects.

I always thought that was going to be the plan; that there would be some flag that would be set in struct super_block when the file system was exported that would enable i_version updates.

Quoted 6 lines

That way we satisfy the "no extra configuration" needed requirement, which I agree is ideal, but we also don't waste any CPU overhead if the file system is not exported via NFSv4. I tried to implement anything along these lines because I don't care enough, and I don't use NFSv4 personally....

Seems like this is just a bad place to be doing inode_inc_iversion(). If MS_IVERSION is set we will update iversion in file_update_time() and then call mark_inode_dirty which will jack up the iversion again. In btrfs we just change it wherever we change ctime and that way you don't really notice the extra overhead since you are doing it in paths where you are changing a bunch of stuff in the inode already, and mostly where you hold the i_mutex so you aren't going to be hitting any contention on the i_lock. Thanks,

Quoted 25 lines · On Mon, 14 May 2012 14:33:17 -0400 Josef Bacik…

Quoted 17 lines · Ted Ts'o

Quoted 7 lines · Andreas Dilger

Quoted 2 lines

And if it at all possible I'd rather have it be something that Just Works rather than something that requires extra configuration.

Sure, but this is only useful for NFSv4, but costs everyone using ext4 continuous overhead, so it isn't a clear-cut case to enable the version just on the thought that NFS might one day be used on any particular filesystem.

It's not a matter of "NFSv4 might one day be used"; if we don't turn on i_version updates until the file system is actually exported via NFSv4, there would be no deleterious effects.

I always thought that was going to be the plan; that there would be some flag that would be set in struct super_block when the file system was exported that would enable i_version updates.

Quoted 6 lines

That way we satisfy the "no extra configuration" needed requirement, which I agree is ideal, but we also don't waste any CPU overhead if the file system is not exported via NFSv4. I tried to implement anything along these lines because I don't care enough, and I don't use NFSv4 personally....

Quoted 8 lines

Seems like this is just a bad place to be doing inode_inc_iversion(). If MS_IVERSION is set we will update iversion in file_update_time() and then call mark_inode_dirty which will jack up the iversion again. In btrfs we just change it wherever we change ctime and that way you don't really notice the extra overhead since you are doing it in paths where you are changing a bunch of stuff in the inode already, and mostly where you hold the i_mutex so you aren't going to be hitting any contention on the i_lock. Thanks,

Well, you do incur a bit more overhead in btrfs too:

------------------[snip]----------------------

        if (!timespec_equal(&inode->i_mtime, &now))
                sync_it = S_MTIME;

        if (!timespec_equal(&inode->i_ctime, &now))
                sync_it |= S_CTIME;

        if (IS_I_VERSION(inode))
                sync_it |= S_VERSION;

------------------[snip]----------------------

So you'll end up with sync_it being 0 if i_version updates are disabled, and the mtime/ctime didn't visibly change.

If your jiffies are coarse-grained enough, then you might get "lucky" rather often, but is that a case worth optimizing for? How often does it happen that you mark the inode dirty, flush it to disk and then re-mark it dirty within the same jiffy?

Quoted 17 lines · Ted Ts'o

Quoted 7 lines · Andreas Dilger

Quoted 2 lines

And if it at all possible I'd rather have it be something that Just Works rather than something that requires extra configuration.

Sure, but this is only useful for NFSv4, but costs everyone using ext4 continuous overhead, so it isn't a clear-cut case to enable the version just on the thought that NFS might one day be used on any particular filesystem.

It's not a matter of "NFSv4 might one day be used"; if we don't turn on i_version updates until the file system is actually exported via NFSv4, there would be no deleterious effects.

I always thought that was going to be the plan; that there would be some flag that would be set in struct super_block when the file system was exported that would enable i_version updates.

Quoted 6 lines

That way we satisfy the "no extra configuration" needed requirement, which I agree is ideal, but we also don't waste any CPU overhead if the file system is not exported via NFSv4. I tried to implement anything along these lines because I don't care enough, and I don't use NFSv4 personally....

Seems like this is just a bad place to be doing inode_inc_iversion(). If MS_IVERSION is set we will update iversion in file_update_time() and then call mark_inode_dirty which will jack up the iversion again.

In btrfs we just change
it wherever we change ctime and that way you don't really notice the extra overhead since you are doing it in paths where you are changing a bunch of stuff in the inode already, and mostly where you hold the i_mutex so you aren't going to be hitting any contention on the i_lock. Thanks,

Quoted 25 lines · Josef Bacik

Quoted 17 lines · Ted Ts'o

Quoted 7 lines · Andreas Dilger

Quoted 2 lines

And if it at all possible I'd rather have it be something that Just Works rather than something that requires extra configuration.

Sure, but this is only useful for NFSv4, but costs everyone using ext4 continuous overhead, so it isn't a clear-cut case to enable the version just on the thought that NFS might one day be used on any particular filesystem.

It's not a matter of "NFSv4 might one day be used"; if we don't turn on i_version updates until the file system is actually exported via NFSv4, there would be no deleterious effects.

I always thought that was going to be the plan; that there would be some flag that would be set in struct super_block when the file system was exported that would enable i_version updates.

Quoted 6 lines

That way we satisfy the "no extra configuration" needed requirement, which I agree is ideal, but we also don't waste any CPU overhead if the file system is not exported via NFSv4. I tried to implement anything along these lines because I don't care enough, and I don't use NFSv4 personally....

Quoted 3 lines

Seems like this is just a bad place to be doing inode_inc_iversion(). If MS_IVERSION is set we will update iversion in file_update_time() and then call mark_inode_dirty which will jack up the iversion again.

Agreed, that's weird.

Quoted 5 lines

In btrfs we just change
it wherever we change ctime and that way you don't really notice the extra overhead since you are doing it in paths where you are changing a bunch of stuff in the inode already, and mostly where you hold the i_mutex so you aren't going to be hitting any contention on the i_lock. Thanks,

I don't think they're worried about the inode_inc_iversion() calls themselves, but the behavior of file_update_time():

        if (!timespec_equal(&inode->i_mtime, &now))
                sync_it = S_MTIME;

        if (!timespec_equal(&inode->i_ctime, &now))
                sync_it |= S_CTIME;

        if (IS_I_VERSION(inode))
                sync_it |= S_VERSION;

        if (!sync_it)
                return;
	...
	mark_inode_dirty_sync(inode);

So now mark_inode_dirty_sync() is called on every update, instead of merely on every update that sees a time change (so at most once a jiffy).

So mark_inode_dirty_sync (and hence ->dirty_inode = ext4_dirty_inode) may get called more often if you're writing very frequently.

I'm a bit surprised that's expected to add significant overhead to the write.

I guess I should stare at the code and try to follow Andreas's explanation....

Quoted 21 lines · J. Bruce Fields

I don't think they're worried about the inode_inc_iversion() calls themselves, but the behavior of file_update_time():

       if (!timespec_equal(&inode->i_mtime, &now))
               sync_it = S_MTIME;

       if (!timespec_equal(&inode->i_ctime, &now))
               sync_it |= S_CTIME;

       if (IS_I_VERSION(inode))
               sync_it |= S_VERSION;

       if (!sync_it)
               return;
	...
	mark_inode_dirty_sync(inode);

So now mark_inode_dirty_sync() is called on every update, instead of merely on every update that sees a time change (so at most once a jiffy).

Quoted 5 lines

So mark_inode_dirty_sync (and hence ->dirty_inode = ext4_dirty_inode) may get called more often if you're writing very frequently.

I'm a bit surprised that's expected to add significant overhead to the write.

It shouldn't, let's be honest, most systems aren't going to have such a coarse jiffie counter that they'll be able to get away with doing 2 calls to write() or ->page_mkwrite() in the same jiffie and skip the update to mtime/ctime anyway. If they do they are damned lucky, and again the amount of overhead added even if they are should be negligible since 99% of us all incur the overhead from having to update mtime/ctime anyway. Thanks,

Quoted 28 lines · Josef Bacik

Quoted 21 lines · J. Bruce Fields

I don't think they're worried about the inode_inc_iversion() calls themselves, but the behavior of file_update_time():

       if (!timespec_equal(&inode->i_mtime, &now))
               sync_it = S_MTIME;

       if (!timespec_equal(&inode->i_ctime, &now))
               sync_it |= S_CTIME;

       if (IS_I_VERSION(inode))
               sync_it |= S_VERSION;

       if (!sync_it)
               return;
	...
	mark_inode_dirty_sync(inode);

So now mark_inode_dirty_sync() is called on every update, instead of merely on every update that sees a time change (so at most once a jiffy).

Quoted 5 lines

So mark_inode_dirty_sync (and hence ->dirty_inode = ext4_dirty_inode) may get called more often if you're writing very frequently.

I'm a bit surprised that's expected to add significant overhead to the write.

Quoted 7 lines

It shouldn't, let's be honest, most systems aren't going to have such a coarse jiffie counter that they'll be able to get away with doing 2 calls to write() or ->page_mkwrite() in the same jiffie and skip the update to mtime/ctime anyway. If they do they are damned lucky, and again the amount of overhead added even if they are should be negligible since 99% of us all incur the overhead from having to update mtime/ctime anyway. Thanks,

Seriously? The whole reason the above checks for timespec_equal() are there is to avoid calling mark_inode_dirty_sync() thousands of times per second. If doing write() calls in the same jiffie were so rare as you suggest then I don't think such an optimization would ever have appeared in the first place.

Quoted 37 lines · Josef Bacik

Quoted 35 lines · J. Bruce Fields

Quoted 21 lines

I don't think they're worried about the inode_inc_iversion() calls themselves, but the behavior of file_update_time():

        if (!timespec_equal(&inode->i_mtime,&now))
                sync_it = S_MTIME;

        if (!timespec_equal(&inode->i_ctime,&now))
                sync_it |= S_CTIME;

        if (IS_I_VERSION(inode))
                sync_it |= S_VERSION;

        if (!sync_it)
                return;
	...
	mark_inode_dirty_sync(inode);

So now mark_inode_dirty_sync() is called on every update, instead of merely on every update that sees a time change (so at most once a jiffy).

Quoted 5 lines

So mark_inode_dirty_sync (and hence ->dirty_inode = ext4_dirty_inode) may get called more often if you're writing very frequently.

I'm a bit surprised that's expected to add significant overhead to the write.

It shouldn't, let's be honest, most systems aren't going to have such a coarse jiffie counter that they'll be able to get away with doing 2 calls to write() or ->page_mkwrite() in the same jiffie and skip the update to mtime/ctime anyway. If they do they are damned lucky, and again the amount of overhead added even if they are should be negligible since 99% of us all incur the overhead from having to update mtime/ctime anyway. Thanks,

Quoted 6 lines

Seriously? The whole reason the above checks for timespec_equal() are there is to avoid calling mark_inode_dirty_sync() thousands of times per second. If doing write() calls in the same jiffie were so rare as you suggest then I don't think such an optimization would ever have appeared in the first place.

Only a really really stupid question (I don't know NFS protocol well enough). In 3.3 kernel, I see that only ext4 uses MS_I_VERSION, so I
wonder: if i_version change it's needed for exportable fs and so for nfs, other exportable fs?

Quoted 10 lines · J. Bruce Fields

Quoted 9 lines · Andreas Dilger

Quoted 7 lines · J. Bruce Fields

knfsd needs i_version updates on, as will userspace nfs servers and probably others.

The only effects are that inode->i_version is bumped (under the i_lock) in more places, and that ->dirty_inode(I_DIRTY_DATASYNC) may be called more frequently than once per jiffy on write (see file_update_time). However the latter appears to be mostly a no-op in that case.

Quoted 4 lines

Quoted 1 line

I thought this can have noticeable performance impact, since ext4_mark_inode_dirty() is quite heavyweight?

There's no reason it should be, should it, if we already just dirtied the inode a moment ago?

Ideally not, but the way ext[34]_mark_inode_dirty() is implemented is that it copies the whole in-core inode to the on-disk inode every time it is marked dirty. That ensures that the on-disk inode is up-to-date when the journal flushes the blocks to disk, but is not an ideal implementation. It has been this way since the first ext3 implementation was done.

As a result, dirtying the inode very frequently for ext[34] is currently expensive and should be avoided.

I _think_ that the ext4 metadata checksum patches have changed this to only flag the inode dirty and run a pre-commit callback to copy the in-core inode to the on-disk inode. I'm not sure what the current status of that patch is, nor how easily it could be split from that patch series and land separately.

Quoted 23 lines · Andreas Dilger

Quoted 10 lines · J. Bruce Fields

Quoted 9 lines · Andreas Dilger

Quoted 7 lines · J. Bruce Fields

knfsd needs i_version updates on, as will userspace nfs servers and probably others.

The only effects are that inode->i_version is bumped (under the i_lock) in more places, and that ->dirty_inode(I_DIRTY_DATASYNC) may be called more frequently than once per jiffy on write (see file_update_time). However the latter appears to be mostly a no-op in that case.

Quoted 4 lines

Quoted 1 line

I thought this can have noticeable performance impact, since ext4_mark_inode_dirty() is quite heavyweight?

There's no reason it should be, should it, if we already just dirtied the inode a moment ago?

Ideally not, but the way ext[34]_mark_inode_dirty() is implemented is that it copies the whole in-core inode to the on-disk inode every time it is marked dirty. That ensures that the on-disk inode is up-to-date when the journal flushes the blocks to disk, but is not an ideal implementation. It has been this way since the first ext3 implementation was done.

Quoted 8 lines

As a result, dirtying the inode very frequently for ext[34] is currently expensive and should be avoided.

I _think_ that the ext4 metadata checksum patches have changed this to only flag the inode dirty and run a pre-commit callback to copy the in-core inode to the on-disk inode. I'm not sure what the current status of that patch is, nor how easily it could be split from that patch series and land separately.

I did some searching, found a couple of versions of the metadata checksum patches, but no patch that looked like what you're describing. Any idea where that is?

--b.
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 3 lines · J. Bruce Fields

knfsd needs i_version updates on, as will userspace nfs servers and probably others.

Quoted 4 lines

The only effects are that inode->i_version is bumped (under the i_lock) in more places, and that ->dirty_inode(I_DIRTY_DATASYNC) may be called more frequently than once per jiffy on write (see file_update_time). However the latter appears to be mostly a no-op in that case.

I thought this can have noticeable performance impact, since ext4_mark_inode_dirty() is quite heavyweight?

This is one of the reasons that the i_version update is conditional. If someone is exporting a filesystem from userspace the should be able to turn this on as a mount option, and knfsd could do it from inside the kernel. Why add overhead when it is not needed?

Quoted 9 lines · Andreas Dilger

Quoted 7 lines · J. Bruce Fields

knfsd needs i_version updates on, as will userspace nfs servers and probably others.

The only effects are that inode->i_version is bumped (under the i_lock) in more places, and that ->dirty_inode(I_DIRTY_DATASYNC) may be called more frequently than once per jiffy on write (see file_update_time). However the latter appears to be mostly a no-op in that case.

Quoted 5 lines

I thought this can have noticeable performance impact, since ext4_mark_inode_dirty() is quite heavyweight?

This is one of the reasons that the i_version update is conditional.
If someone is exporting a filesystem from userspace the should be
able to turn this on as a mount option, and knfsd could do it from inside the kernel. Why add overhead when it is not needed?

No. Having knfsd doing something like that under the covers is a BAD idea. It is way too easy to get into situations where someone starts changing files after the mount and before knfsd is started. As soon as you allow files to be changed without i_version changing, then you are setting yourself up for future corruption.

Ideally, an NFSv4-exported filesystem should be required to set the tune2fs mount_opts to include the 'i_version' flag to make it hard to inadvertently mount that filesystem without it.

I said as much in another reply - that once i_version is used on a filesystem, it should be made "sticky" (i.e. permanently enabled for that filesystem). However, until that time it shouldn't be enabled just because it might one day be used.

Even better than just blindly bumping the i_version on every change, it would be better to have users of i_version (i.e. knfsd) flag the inode with "needs i_version update" then read the version. When the filesystem/VFS bumps i_version the next time it can clear this flag and not update i_version again until after the next time i_version is actually used.

Quoted 11 lines · Andreas Dilger

I said as much in another reply - that once i_version is used on a filesystem, it should be made "sticky" (i.e. permanently enabled for that filesystem). However, until that time it shouldn't be enabled just because it might one day be used.

Even better than just blindly bumping the i_version on every change, it would be better to have users of i_version (i.e. knfsd) flag the inode with "needs i_version update" then read the version. When the filesystem/VFS bumps i_version the next time it can clear this flag and not update i_version again until after the next time i_version is actually used.

I really don't want to do anything more complicated than necessary.

What would be the worst-case test for the extra inode dirtying, so we can see what the numbers actually are?

Quoted 13 lines · J. Bruce Fields

Quoted 11 lines · Andreas Dilger

I said as much in another reply - that once i_version is used on a filesystem, it should be made "sticky" (i.e. permanently enabled for that filesystem). However, until that time it shouldn't be enabled just because it might one day be used.

Even better than just blindly bumping the i_version on every change, it would be better to have users of i_version (i.e. knfsd) flag the inode with "needs i_version update" then read the version. When the filesystem/VFS bumps i_version the next time it can clear this flag and not update i_version again until after the next time i_version is actually used.

Quoted 4 lines

I really don't want to do anything more complicated than necessary.

What would be the worst-case test for the extra inode dirtying, so we can see what the numbers actually are?

Something like:

  int fd, i;
  struct timeval tv[2];

  fd = open("file", O_CREAT | O_RDWR, 0644);
  if (fd < 0)
    return 1;
  for (i = 0; i < 1000000; i++) {
    gettimeofday(tv);
    tv[1] = tv[0];
    if (futimes(fd, tv) < 0)
      return 1;
  }
  return 0;

And see how long does it take with and without i_version?

Quoted 34 lines · Jan Kara

Quoted 13 lines · J. Bruce Fields

Quoted 11 lines · Andreas Dilger

I said as much in another reply - that once i_version is used on a filesystem, it should be made "sticky" (i.e. permanently enabled for that filesystem). However, until that time it shouldn't be enabled just because it might one day be used.

Even better than just blindly bumping the i_version on every change, it would be better to have users of i_version (i.e. knfsd) flag the inode with "needs i_version update" then read the version. When the filesystem/VFS bumps i_version the next time it can clear this flag and not update i_version again until after the next time i_version is actually used.

Quoted 4 lines

I really don't want to do anything more complicated than necessary.

What would be the worst-case test for the extra inode dirtying, so we can see what the numbers actually are?

Something like:

  int fd, i;
  struct timeval tv[2];

  fd = open("file", O_CREAT | O_RDWR, 0644);
  if (fd < 0)
    return 1;
  for (i = 0; i < 1000000; i++) {
    gettimeofday(tv);
    tv[1] = tv[0];
    if (futimes(fd, tv) < 0)
      return 1;
  }
  return 0;

Quoted 1 line

And see how long does it take with and without i_version?

The complaint I hear from Andreas is that we'll cause file_update_time() to call mark_inode_dirty() more often.

I don't believe futimes() calls file_update_time().

So maybe replace that futimes() by a one-byte write?

Even better than just blindly bumping the i_version on every change, it would be better to have users of i_version (i.e. knfsd) flag the inode with "needs i_version update" then read the version. When the filesystem/VFS bumps i_version the next time it can clear this flag and not update i_version again until after the next time i_version is actually used.