Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 16 Sep 2022 02:54:16 -0400
From:   "Theodore Ts'o" <tytso@mit.edu>
To:     NeilBrown <neilb@suse.de>
Cc:     Jeff Layton <jlayton@kernel.org>,
        Trond Myklebust <trondmy@hammerspace.com>,
        "bfields@fieldses.org" <bfields@fieldses.org>,
        "zohar@linux.ibm.com" <zohar@linux.ibm.com>,
        "djwong@kernel.org" <djwong@kernel.org>,
        "brauner@kernel.org" <brauner@kernel.org>,
        "linux-xfs@vger.kernel.org" <linux-xfs@vger.kernel.org>,
        "linux-api@vger.kernel.org" <linux-api@vger.kernel.org>,
        "david@fromorbit.com" <david@fromorbit.com>,
        "fweimer@redhat.com" <fweimer@redhat.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "chuck.lever@oracle.com" <chuck.lever@oracle.com>,
        "linux-man@vger.kernel.org" <linux-man@vger.kernel.org>,
        "linux-nfs@vger.kernel.org" <linux-nfs@vger.kernel.org>,
        "linux-ext4@vger.kernel.org" <linux-ext4@vger.kernel.org>,
        "jack@suse.cz" <jack@suse.cz>,
        "viro@zeniv.linux.org.uk" <viro@zeniv.linux.org.uk>,
        "xiubli@redhat.com" <xiubli@redhat.com>,
        "linux-fsdevel@vger.kernel.org" <linux-fsdevel@vger.kernel.org>,
        "adilger.kernel@dilger.ca" <adilger.kernel@dilger.ca>,
        "lczerner@redhat.com" <lczerner@redhat.com>,
        "ceph-devel@vger.kernel.org" <ceph-devel@vger.kernel.org>,
        "linux-btrfs@vger.kernel.org" <linux-btrfs@vger.kernel.org>
Subject: Re: [man-pages RFC PATCH v4] statx, inode: document the new
 STATX_INO_VERSION field
Message-ID: <YyQdmLpiAMvl5EkU@mit.edu>
References: <20220912134208.GB9304@fieldses.org>
 <166302447257.30452.6751169887085269140@noble.neil.brown.name>
 <20220915140644.GA15754@fieldses.org>
 <577b6d8a7243aeee37eaa4bbb00c90799586bc48.camel@hammerspace.com>
 <1a968b8e87f054e360877c9ab8cdfc4cfdfc8740.camel@kernel.org>
 <0646410b6d2a5d19d3315f339b2928dfa9f2d922.camel@hammerspace.com>
 <34e91540c92ad6980256f6b44115cf993695d5e1.camel@kernel.org>
 <871f9c5153ddfe760854ca31ee36b84655959b83.camel@hammerspace.com>
 <e8922bc821a40f5a3f0a1301583288ed19b6891b.camel@kernel.org>
 <166328063547.15759.12797959071252871549@noble.neil.brown.name>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <166328063547.15759.12797959071252871549@noble.neil.brown.name>
Precedence: bulk

On Fri, Sep 16, 2022 at 08:23:55AM +1000, NeilBrown wrote:
> > > If the answer is that 'all values change', then why store the crash
> > > counter in the inode at all? Why not just add it as an offset when
> > > you're generating the user-visible change attribute?
> > > 
> > > i.e. statx.change_attr = inode->i_version + (crash counter * offset)

I had suggested just hashing the crash counter with the file system's
on-disk i_version number, which is essentially what you are suggested.

> > Yes, if we plan to ensure that all the change attrs change after a
> > crash, we can do that.
> > 
> > So what would make sense for an offset? Maybe 2**12? One would hope that
> > there wouldn't be more than 4k increments before one of them made it to
> > disk. OTOH, maybe that can happen with teeny-tiny writes.
> 
> Leave it up the to filesystem to decide.  The VFS and/or NFSD should
> have not have part in calculating the i_version.  It should be entirely
> in the filesystem - though support code could be provided if common
> patterns exist across filesystems.

Oh, *heck* no.  This parameter is for the NFS implementation to
decide, because it's NFS's caching algorithms which are at stake here.

As a the file system maintainer, I had offered to make an on-disk
"crash counter" which would get updated when the journal had gotten
replayed, in addition to the on-disk i_version number.  This will be
available for the Linux implementation of NFSD to use, but that's up
to *you* to decide how you want to use them.

I was perfectly happy with hashing the crash counter and the i_version
because I had assumed that not *that* much stuff was going to be
cached, and so invalidating all of the caches in the unusual case
where there was a crash was acceptable.  After all it's a !@#?!@
cache.  Caches sometimmes get invalidated.  "That is the order of
things." (as Ramata'Klan once said in "Rocks and Shoals")

But if people expect that multiple TB's of data is going to be stored;
that cache invalidation is unacceptable; and that a itsy-weeny chance
of false negative failures which might cause data corruption might be
acceptable tradeoff, hey, that's for the system which is providing
caching semantics to determine.

PLEASE don't put this tradeoff on the file system authors; I would
much prefer to leave this tradeoff in the hands of the system which is
trying to do the caching.

						- Ted