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

On Mon, May 14, 2012 at 02:48:02PM -0400, Jeff Layton wrote:

On Mon, 14 May 2012 14:33:17 -0400
Josef Bacik [off-list ref] wrote:

quoted

On Mon, May 14, 2012 at 01:58:22PM -0400, Ted Ts'o wrote:

quoted

On Mon, May 14, 2012 at 11:27:42AM -0600, Andreas Dilger wrote:

quoted

quoted

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

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?

Well sync_it just means do we need to call mark_inode_dirty, not necessarily do
we need to write it to disk, so you are just updating a field in memory.  Now if
your jiffies are coarse enough for you to not notice the ctime update then yes
you are incurring an extra lock/inc/unlock, but this is called in the write path
where you are going to do much more latency inducting operations than locking
and unlocking a generally uncontended spin lock.  Thanks,

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