Re: [PATCH] ext4: turn on i_version updates by default

On 2012-05-14, at 1:05 PM, Josef Bacik wrote:

On Mon, May 14, 2012 at 02:54:00PM -0400, J. Bruce Fields wrote:

quoted

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.

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.

For writes to a high-IOPS device (e.g. SSD) can run far higher than
1000 IOPS, and this is an important use case that people care about
today, so why add useless overhead when it isn't needed?

Cheers, Andreas





--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Mon, May 14, 2012 at 03:27:47PM -0600, Andreas Dilger wrote:

On 2012-05-14, at 1:05 PM, Josef Bacik wrote:

quoted

On Mon, May 14, 2012 at 02:54:00PM -0400, J. Bruce Fields wrote:

quoted

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.

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.

So here is the commit log

commit ce06e0b21d6732a2bab10a585a3ec6909499be28
Author: Andi Kleen [off-list ref]
Date:   Fri Sep 18 13:05:48 2009 -0700

    vfs: optimize touch_time() too
    
    Do a similar optimization as earlier for touch_atime.  Getting the lock in
    mnt_get_write is relatively costly, so try all avenues to avoid it first.
    
    This patch is careful to still only update inode fields inside the lock
    region.
    
    This didn't show up in benchmarks, but it's easy enough to do.

Notice that last bit?  I'm sure maybe at some point in the future we'll be able
to see the overhead, but right now I highly doubt we can, and if we can I'd like
to see benchmarks before blanket dismissing a feature that would be super
helpful for people exporting nfs volumes.  Thanks,

Josef

Il 15/05/2012 15:28, Josef Bacik ha scritto:

On Mon, May 14, 2012 at 03:27:47PM -0600, Andreas Dilger wrote:

quoted

On 2012-05-14, at 1:05 PM, Josef Bacik wrote:

quoted

On Mon, May 14, 2012 at 02:54:00PM -0400, J. Bruce Fields wrote:

quoted

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.

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.

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? Is this only a particular problem for ext4? I 
mean, it doesn't seems a blocking problem (or we could have a lot of 
traffic on fs-devel :) ), it seems a "more compliant behavior". If this 
considerations is right, I think the current behavior of ext4 is ok.

Marco

Marco

On Tue, May 15, 2012 at 07:59:11PM +0200, Marco Stornelli wrote:

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?

Yes, it's needed for others as well.  I believe btrfs and xfs are both
adding it.

We're currently using ctime for the nfs change attribute.  That's
effectively jiffy granularity.  So to see the problem at a minimum you'd
need two writes to be processed within one jiffy, and a stat to come
between them.  But that's a correctness problem, and we'd like to see it
fixed before it becomes more common.

More generally, it's useful to be able to ask whether a file changed
without rereading all its data, and a clock that registers every change
and is consistent across a filesystem sounds difficult to scale.  We may
eventually find we need something like this outside nfs.

--b.