Date:   Mon, 29 Aug 2022 06:33:48 -0400
From:   Jeff Layton <jlayton@...nel.org>
To:     Dave Chinner <david@...morbit.com>
Cc:     Amir Goldstein <amir73il@...il.com>,
        Trond Myklebust <trondmy@...merspace.com>,
        "djwong@...nel.org" <djwong@...nel.org>,
        "zohar@...ux.ibm.com" <zohar@...ux.ibm.com>,
        "brauner@...nel.org" <brauner@...nel.org>,
        "xiubli@...hat.com" <xiubli@...hat.com>,
        "neilb@...e.de" <neilb@...e.de>,
        "linux-api@...r.kernel.org" <linux-api@...r.kernel.org>,
        "linux-xfs@...r.kernel.org" <linux-xfs@...r.kernel.org>,
        "dwysocha@...hat.com" <dwysocha@...hat.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "chuck.lever@...cle.com" <chuck.lever@...cle.com>,
        "linux-nfs@...r.kernel.org" <linux-nfs@...r.kernel.org>,
        "tytso@....edu" <tytso@....edu>,
        "viro@...iv.linux.org.uk" <viro@...iv.linux.org.uk>,
        "jack@...e.cz" <jack@...e.cz>,
        "linux-ext4@...r.kernel.org" <linux-ext4@...r.kernel.org>,
        "linux-btrfs@...r.kernel.org" <linux-btrfs@...r.kernel.org>,
        "linux-fsdevel@...r.kernel.org" <linux-fsdevel@...r.kernel.org>,
        "lczerner@...hat.com" <lczerner@...hat.com>,
        "adilger.kernel@...ger.ca" <adilger.kernel@...ger.ca>,
        "ceph-devel@...r.kernel.org" <ceph-devel@...r.kernel.org>
Subject: Re: [PATCH v3 4/7] xfs: don't bump the i_version on an atime update
 in xfs_vn_update_time

On Mon, 2022-08-29 at 15:48 +1000, Dave Chinner wrote:
> On Sun, Aug 28, 2022 at 10:37:37AM -0400, Jeff Layton wrote:
> > On Sun, 2022-08-28 at 16:25 +0300, Amir Goldstein wrote:
> > > On Sat, Aug 27, 2022 at 7:10 PM Jeff Layton <jlayton@...nel.org> wrote:
> > > > Yeah, thinking about it some more, simply changing the block allocation
> > > > is not something that should affect the ctime, so we probably don't want
> > > > to bump i_version on it. It's an implicit change, IOW, not an explicit
> > > > one.
> > > > 
> > > > The fact that xfs might do that is unfortunate, but it's not the end of
> > > > the world and it still would conform to the proposed definition for
> > > > i_version. In practice, this sort of allocation change should come soon
> > > > after the file was written, so one would hope that any damage due to the
> > > > false i_version bump would be minimized.
> > > > 
> > > 
> > > That was exactly my point.
> > > 
> > > > It would be nice to teach it not to do that however. Maybe we can insert
> > > > the NOIVER flag at a strategic place to avoid it?
> 
> No, absolutely not.
> 
> I've already explained this: The XFS *disk format specification*
> says that di_changecount is bumped for every change that is made to
> the inode.
> 
> Applications that are written from this specification expect the on
> disk format for a XFS given filesystem feature to remain the same
> until it is either deprecated and removed or we add feature flags to
> indicate it has different behaviour.  We can't just change the
> behaviour at a whim.
> 
> And that's ignoring the fact that randomly spewing NOIVER
> into transactions that modify inode metadata is a nasty hack - it
> is not desirable from a design or documentation POV, nor is it
> maintainable.
> 
> > > Why would that be nice to avoid?
> > > You did not specify any use case where incrementing i_version
> > > on block mapping change matters in practice.
> > > On the contrary, you said that NFS client writer sends COMMIT on close,
> > > which should stabilize i_version for the next readers.
> > > 
> > > Given that we already have an xfs implementation that does increment
> > > i_version on block mapping changes and it would be a pain to change
> > > that or add a new user options, I don't see the point in discussing it further
> > > unless there is a good incentive for avoiding i_version updates in those cases.
> > > 
> > 
> > Because the change to the block allocation doesn't represent an
> > "explicit" change to the inode. We will have bumped the ctime on the
> > original write (in update_time), but the follow-on changes that occur
> > due to that write needn't be counted as they aren't visible to the
> > client.
> > 
> > It's possible for a client to issue a read between the write and the
> > flush and get the interim value for i_version. Then, once the write
> > happens and the i_version gets bumped again, the client invalidates its
> > cache even though it needn't do so.
> > 
> > The race window ought to be relatively small, and this wouldn't result
> > in incorrect behavior that you'd notice (other than loss of
> > performance), but it's not ideal. We're doing more on-the-wire reads
> > than are necessary in this case.
> > 
> > It would be nice to have it not do that. If we end up taking this patch
> > to make it elide the i_version bumps on atime updates, we may be able to
> > set the the NOIVER flag in other cases as well, and avoid some of these
> > extra bumps.
> 
> 
> <sigh>
> 
> Please don't make me repeat myself for the third time.
> 
> Once we have decided on a solid, unchanging definition for the
> *statx user API variable*, we'll implement a new on-disk field that
> provides this information.  We will document it in the on-disk
> specification as "this is how di_iversion behaves" so that it is
> clear to everyone parsing the on-disk format or writing their own
> XFS driver how to implement it and when to expect it to
> change.
> 
> Then we can add a filesystem and inode feature flags that say "inode
> has new iversion" and we use that to populate the kernel iversion
> instead of di_changecount. We keep di_changecount exactly the way it
> is now for the applications and use cases we already have for that
> specific behaviour. If the kernel and/or filesystem don't support
> the new di_iversion field, then we'll use di_changecount as it
> currently exists for the kernel iversion code.
> 

Aside from NFS and IMA, what applications are dependent on the current
definition and how do they rely on i_version today?

> Keep in mind that we've been doing dynamic inode format updates in
> XFS for a couple of decades - users don't even have to be aware that
> they need to perform format upgrades because often they just happen
> whenever an inode is accessed. IOWs, just because we have to change
> the on-disk format to support this new iversion definition, it
> doesn't mean users have to reformat filesystems before the new
> feature can be used.
> 
> Hence, over time, as distros update kernels, the XFS iversion
> behaviour will change automagically as we update inodes in existing
> filesystems as they are accessed to add and then use the new
> di_iversion field for the VFS change attribute field instead of the
> di_changecount field...
> 

If you want to create a whole new on-disk field for this, then that's
your prerogative, but before you do that, I'd like to better understand
why and how the constraints on this field changed.

The original log message from the commit that added a change counter
(below) stated that you were adding it for network filesystems like NFS.
When did this change and why?

    commit dc037ad7d24f3711e431a45c053b5d425995e9e4
    Author: Dave Chinner <dchinner@...hat.com>
    Date:   Thu Jun 27 16:04:59 2013 +1000

        xfs: implement inode change count

        For CRC enabled filesystems, add support for the monotonic inode
        version change counter that is needed by protocols like NFSv4 for
        determining if the inode has changed in any way at all between two
        unrelated operations on the inode.

        This bumps the change count the first time an inode is dirtied in a
        transaction. Since all modifications to the inode are logged, this
        will catch all changes that are made to the inode, including
        timestamp updates that occur during data writes.

        Signed-off-by: Dave Chinner <dchinner@...hat.com>
        Reviewed-by: Mark Tinguely <tinguely@....com>
        Reviewed-by: Chandra Seetharaman <sekharan@...ibm.com>
        Signed-off-by: Ben Myers <bpm@....com>

-- 
Jeff Layton <jlayton@...nel.org>

Powered by blists - more mailing lists