From: "Theodore Ts'o" <tytso@mit.edu>
To: dsterba@suse.cz, Liu Bo <bo.li.liu@oracle.com>,
linux-btrfs@vger.kernel.org, fdmanana@suse.com, kzak@redhat.com,
linux-fsdevel@vger.kernel.org, viro@zeniv.linux.org.uk,
linux-nfs@vger.kernel.org, chuck.lever@oracle.com,
mingming.cao@oracle.com
Subject: Re: i_version vs iversion (Was: Re: [RFC PATCH v2 1/2] Btrfs: add noi_version option to disable MS_I_VERSION)
Date: Wed, 24 Jun 2015 19:17:50 -0400 [thread overview]
Message-ID: <20150624231750.GE14324@thunk.org> (raw)
In-Reply-To: <20150624180215.GC726@suse.cz>
On Wed, Jun 24, 2015 at 08:02:15PM +0200, David Sterba wrote:
>
> This sounds similar to what Dave proposed, a per-inode I_VERSION
> attribute that can be changed through chattr. Though the negated meaning
> of the flag could be confusing, I had to reread the paragraph again.
Dave did not specify an I_VERSION attribute that would be stored on
disk. Instead he talked about a inode flag that would be set when the
struct inode is created by the file system.
This would allow file systems to permanently configure (on a per-inode
basis) whether or not a particular inode would require a forced
i_version update any time the inode's data or metadata is modified. I
suppose you could initialized the inode flag from an on-disk
attribute, but that wasn't implied by Dave's proposal, at least as I
understood it.
> > This should significantly improve the performance of using the
> > i_version field if the file system is being exported via NFSv4, and if
> > NFSv4 is not in use, no one will be looking at the i_version field, so
> > the performance impact will be very slight, and thus we could enable
> > i_version updates by default for btrfs and ext4.
>
> Btrfs default is to update i_version and the uscecase gets fixed by the
> per-inode attribute. But from your description above I think that this
> might not be enough for ext4. The reason I see are the different
> defaults. You want to turn it on by default but not impose any
> performance penalty for that, while for our usecase it's sufficient to
> selectively turn it off.
The problem with selectively turning it off is that the user might
decide for a particular file which is getting lots of updates to turn
off i_version updates --- but then at some subsequent time, that file
is part of a file system which is exported NFSv4, clients will
mysteriously break because i_version was manually turned off.
So this is going to be a potential support nightmare for enterprise
distro help desks --- the user will report that a particular file is
constantly getting corrupted, due to the NFSv4 cache invalidation
getting broken, and it might not be obvious why this is only happening
with this one file, and it's because with btrfs, the i_version update
for particular file was selectively turned off. I don't think it's a
good design where it is easy for the user to set a flag which breaks
functionality, and in a potentially confusing way, especially when the
net result is potential data corruption.
This is why I would much rather have the default be on, but with
minimal (preferably not measurable) performance overhead. It's the
best of both worlds.
- Ted
WARNING: multiple messages have this Message-ID (diff)
From: Theodore Ts'o <tytso-3s7WtUTddSA@public.gmane.org>
To: dsterba-AlSwsSmVLrQ@public.gmane.org,
Liu Bo <bo.li.liu-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>,
linux-btrfs-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
fdmanana-IBi9RG/b67k@public.gmane.org,
kzak-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org,
linux-fsdevel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
viro-RmSDqhL/yNMiFSDQTTA3OLVCufUGDwFn@public.gmane.org,
linux-nfs-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
chuck.lever-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org,
mingming.cao-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org
Subject: Re: i_version vs iversion (Was: Re: [RFC PATCH v2 1/2] Btrfs: add noi_version option to disable MS_I_VERSION)
Date: Wed, 24 Jun 2015 19:17:50 -0400 [thread overview]
Message-ID: <20150624231750.GE14324@thunk.org> (raw)
In-Reply-To: <20150624180215.GC726-AlSwsSmVLrQ@public.gmane.org>
On Wed, Jun 24, 2015 at 08:02:15PM +0200, David Sterba wrote:
>
> This sounds similar to what Dave proposed, a per-inode I_VERSION
> attribute that can be changed through chattr. Though the negated meaning
> of the flag could be confusing, I had to reread the paragraph again.
Dave did not specify an I_VERSION attribute that would be stored on
disk. Instead he talked about a inode flag that would be set when the
struct inode is created by the file system.
This would allow file systems to permanently configure (on a per-inode
basis) whether or not a particular inode would require a forced
i_version update any time the inode's data or metadata is modified. I
suppose you could initialized the inode flag from an on-disk
attribute, but that wasn't implied by Dave's proposal, at least as I
understood it.
> > This should significantly improve the performance of using the
> > i_version field if the file system is being exported via NFSv4, and if
> > NFSv4 is not in use, no one will be looking at the i_version field, so
> > the performance impact will be very slight, and thus we could enable
> > i_version updates by default for btrfs and ext4.
>
> Btrfs default is to update i_version and the uscecase gets fixed by the
> per-inode attribute. But from your description above I think that this
> might not be enough for ext4. The reason I see are the different
> defaults. You want to turn it on by default but not impose any
> performance penalty for that, while for our usecase it's sufficient to
> selectively turn it off.
The problem with selectively turning it off is that the user might
decide for a particular file which is getting lots of updates to turn
off i_version updates --- but then at some subsequent time, that file
is part of a file system which is exported NFSv4, clients will
mysteriously break because i_version was manually turned off.
So this is going to be a potential support nightmare for enterprise
distro help desks --- the user will report that a particular file is
constantly getting corrupted, due to the NFSv4 cache invalidation
getting broken, and it might not be obvious why this is only happening
with this one file, and it's because with btrfs, the i_version update
for particular file was selectively turned off. I don't think it's a
good design where it is easy for the user to set a flag which breaks
functionality, and in a potentially confusing way, especially when the
net result is potential data corruption.
This is why I would much rather have the default be on, but with
minimal (preferably not measurable) performance overhead. It's the
best of both worlds.
- Ted
--
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
next prev parent reply other threads:[~2015-06-24 23:17 UTC|newest]
Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-06-17 7:54 [RFC PATCH v2 1/2] Btrfs: add noi_version option to disable MS_I_VERSION Liu Bo
2015-06-17 7:54 ` [RFC PATCH v2 2/2] Btrfs: improve fsync for nocow file Liu Bo
2015-06-17 15:58 ` David Sterba
2015-06-18 3:27 ` Liu Bo
2015-06-24 18:21 ` David Sterba
2015-06-25 2:24 ` Liu Bo
2015-06-25 16:10 ` David Sterba
2015-06-17 15:33 ` [RFC PATCH v2 1/2] Btrfs: add noi_version option to disable MS_I_VERSION David Sterba
2015-06-17 15:52 ` Liu Bo
2015-06-17 17:01 ` David Sterba
2015-06-18 2:46 ` Liu Bo
2015-06-18 14:38 ` i_version vs iversion (Was: Re: [RFC PATCH v2 1/2] Btrfs: add noi_version option to disable MS_I_VERSION) David Sterba
2015-06-19 11:44 ` Karel Zak
2015-06-19 11:44 ` Karel Zak
2015-06-22 20:42 ` Dave Chinner
2015-06-22 20:42 ` Dave Chinner
2015-06-24 17:28 ` David Sterba
2015-06-23 16:32 ` Theodore Ts'o
2015-06-24 8:23 ` Liu Bo
2015-06-24 18:02 ` David Sterba
2015-06-24 23:17 ` Theodore Ts'o [this message]
2015-06-24 23:17 ` Theodore Ts'o
2015-06-24 23:59 ` Dave Chinner
2015-06-24 23:59 ` Dave Chinner
2015-06-25 18:46 ` J. Bruce Fields
2015-06-25 18:46 ` J. Bruce Fields
2015-06-25 22:12 ` Theodore Ts'o
2015-06-26 13:32 ` J. Bruce Fields
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20150624231750.GE14324@thunk.org \
--to=tytso@mit.edu \
--cc=bo.li.liu@oracle.com \
--cc=chuck.lever@oracle.com \
--cc=dsterba@suse.cz \
--cc=fdmanana@suse.com \
--cc=kzak@redhat.com \
--cc=linux-btrfs@vger.kernel.org \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-nfs@vger.kernel.org \
--cc=mingming.cao@oracle.com \
--cc=viro@zeniv.linux.org.uk \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.