Date:   Fri, 09 Sep 2022 16:41:41 +1000
From:   "NeilBrown" <neilb@...e.de>
To:     "Trond Myklebust" <trondmy@...merspace.com>
Cc:     "zohar@...ux.ibm.com" <zohar@...ux.ibm.com>,
        "djwong@...nel.org" <djwong@...nel.org>,
        "xiubli@...hat.com" <xiubli@...hat.com>,
        "brauner@...nel.org" <brauner@...nel.org>,
        "linux-xfs@...r.kernel.org" <linux-xfs@...r.kernel.org>,
        "linux-api@...r.kernel.org" <linux-api@...r.kernel.org>,
        "bfields@...ldses.org" <bfields@...ldses.org>,
        "david@...morbit.com" <david@...morbit.com>,
        "fweimer@...hat.com" <fweimer@...hat.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "chuck.lever@...cle.com" <chuck.lever@...cle.com>,
        "jlayton@...nel.org" <jlayton@...nel.org>,
        "linux-man@...r.kernel.org" <linux-man@...r.kernel.org>,
        "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>,
        "adilger.kernel@...ger.ca" <adilger.kernel@...ger.ca>,
        "lczerner@...hat.com" <lczerner@...hat.com>,
        "ceph-devel@...r.kernel.org" <ceph-devel@...r.kernel.org>
Subject: Re: [man-pages RFC PATCH v4] statx, inode: document the new
 STATX_INO_VERSION field

On Fri, 09 Sep 2022, Trond Myklebust wrote:
> On Fri, 2022-09-09 at 01:10 +0000, Trond Myklebust wrote:
> > On Fri, 2022-09-09 at 11:07 +1000, NeilBrown wrote:
> > > On Fri, 09 Sep 2022, NeilBrown wrote:
> > > > On Fri, 09 Sep 2022, Trond Myklebust wrote:
> > > > 
> > > > > 
> > > > > IOW: the minimal condition needs to be that for all cases
> > > > > below,
> > > > > the
> > > > > application reads 'state B' as having occurred if any data was
> > > > > committed to disk before the crash.
> > > > > 
> > > > > Application                             Filesystem
> > > > > ===========                             =========
> > > > > read change attr <- 'state A'
> > > > > read data <- 'state A'
> > > > >                                         write data -> 'state B'
> > > > >                                         <crash>+<reboot>
> > > > > read change attr <- 'state B'
> > > > 
> > > > The important thing here is to not see 'state A'.  Seeing 'state
> > > > C'
> > > > should be acceptable.  Worst case we could merge in wall-clock
> > > > time
> > > > of
> > > > system boot, but the filesystem should be able to be more helpful
> > > > than
> > > > that.
> > > > 
> > > 
> > > Actually, without the crash+reboot it would still be acceptable to
> > > see
> > > "state A" at the end there - but preferably not for long.
> > > From the NFS perspective, the changeid needs to update by the time
> > > of
> > > a
> > > close or unlock (so it is visible to open or lock), but before that
> > > it
> > > is just best-effort.
> > 
> > Nope. That will inevitably lead to data corruption, since the
> > application might decide to use the data from state A instead of
> > revalidating it.
> > 
> 
> The point is, NFS is not the only potential use case for change
> attributes. We wouldn't be bothering to discuss statx() if it was.

My understanding is that it was primarily a desire to add fstests to
exercise the i_version which motivated the statx extension.
Obviously we should prepare for other uses though.

> 
> I could be using O_DIRECT, and all the tricks in order to ensure that
> my stock broker application (to choose one example) has access to the
> absolute very latest prices when I'm trying to execute a trade.
> When the filesystem then says 'the prices haven't changed since your
> last read because the change attribute on the database file is the
> same' in response to a statx() request with the AT_STATX_FORCE_SYNC
> flag set, then why shouldn't my application be able to assume it can
> serve those prices right out of memory instead of having to go to disk?

I would think that such an application would be using inotify rather
than having to poll.  But certainly we should have a clear statement of
quality-of-service parameters in the documentation.
If we agree that perfect atomicity is what we want to promise, and that
the cost to the filesystem and the statx call is acceptable, then so be it.

My point wasn't to say that atomicity is bad.  It was that:
 - if the i_version change is visible before the change itself is
   visible, then that is a correctness problem.
 - if the i_version change is only visible some time after the change
   itself is visible, then that is a quality-of-service issue.
I cannot see any room for debating the first.  I do see some room to
debate the second.

Cached writes, directory ops, and attribute changes are, I think, easy
enough to provide truly atomic i_version updates with the change being
visible.

Changes to a shared memory-mapped files is probably the hardest to
provide timely i_version updates for.  We might want to document an
explicit exception for those.  Alternately each request for i_version
would need to find all pages that are writable, remap them read-only to
catch future writes, then update i_version if any were writable (i.e.
->mkwrite had been called).  That is the only way I can think of to
provide atomicity.

O_DIRECT writes are a little easier than mmapped files.  I suspect we
should update the i_version once the device reports that the write is
complete, but a parallel reader could have seem some of the write before
that moment.  True atomicity could only be provided by taking some
exclusive lock that blocked all O_DIRECT writes.  Jeff seems to be
suggesting this, but I doubt the stock broker application would be
willing to make the call in that case.  I don't think I would either.

NeilBrown

> 
> -- 
> Trond Myklebust
> Linux NFS client maintainer, Hammerspace
> trond.myklebust@...merspace.com
> 
> 
> 

Powered by blists - more mailing lists