Date:   Tue, 18 Oct 2022 06:35:14 -0400
From:   Jeff Layton <jlayton@...nel.org>
To:     Dave Chinner <david@...morbit.com>
Cc:     tytso@....edu, adilger.kernel@...ger.ca, djwong@...nel.org,
        trondmy@...merspace.com, neilb@...e.de, viro@...iv.linux.org.uk,
        zohar@...ux.ibm.com, xiubli@...hat.com, chuck.lever@...cle.com,
        lczerner@...hat.com, jack@...e.cz, bfields@...ldses.org,
        brauner@...nel.org, fweimer@...hat.com,
        linux-btrfs@...r.kernel.org, linux-fsdevel@...r.kernel.org,
        linux-kernel@...r.kernel.org, ceph-devel@...r.kernel.org,
        linux-ext4@...r.kernel.org, linux-nfs@...r.kernel.org,
        linux-xfs@...r.kernel.org
Subject: Re: [RFC PATCH v7 9/9] vfs: expose STATX_VERSION to userland

On Tue, 2022-10-18 at 09:14 +1100, Dave Chinner wrote:
> On Mon, Oct 17, 2022 at 06:57:09AM -0400, Jeff Layton wrote:
> > From: Jeff Layton <jlayton@...hat.com>
> > 
> > Claim one of the spare fields in struct statx to hold a 64-bit inode
> > version attribute. When userland requests STATX_VERSION, copy the
> > value from the kstat struct there, and stop masking off
> > STATX_ATTR_VERSION_MONOTONIC.
> 
> Can we please make the name more sepcific than "version"? It's way
> too generic and - we already have userspace facing "version" fields
> for inodes that refer to the on-disk format version exposed in
> various UAPIs. It's common for UAPI structures used for file
> operations to have a "version" field that refers to the *UAPI
> structure version* rather than file metadata or data being retrieved
> from the file in question.
> 
> The need for an explanatory comment like this:
> 
> > +	__u64	stx_version; /* Inode change attribute */
> 
> demonstrates it is badly named. If you want it known as an inode
> change attribute, then don't name the variable "version". In
> reality, it really needs to be an opaque cookie, not something
> applications need to decode directly to make sense of.
> 

Fair enough. I started with this being named stx_change_attr and other
people objected. I then changed to stx_ino_version, but the "_ino"
seemed redundant.

I'm open to suggestions here. Naming things like this is hard.

> > Update the test-statx sample program to output the change attr and
> > MountId.
> > 
> > Reviewed-by: NeilBrown <neilb@...e.de>
> > Signed-off-by: Jeff Layton <jlayton@...nel.org>
> > ---
> >  fs/stat.c                 | 12 +++---------
> >  include/linux/stat.h      |  9 ---------
> >  include/uapi/linux/stat.h |  6 ++++--
> >  samples/vfs/test-statx.c  |  8 ++++++--
> >  4 files changed, 13 insertions(+), 22 deletions(-)
> > 
> > Posting this as an RFC as we're still trying to sort out what semantics
> > we want to present to userland. In particular, this patch leaves the
> > problem of crash resilience in to userland applications on filesystems
> > that don't report as MONOTONIC.
> 
> Firstly, if userspace wants to use the change attribute, they are
> going to have to detect crashes themselves anyway because no fs in
> the kernel can set the MONOTONIC flag right now and it may be years
> before kernels/filesystems actually support it in production
> systems.
> 

We can turn it on today in CephFS, NFS and tmpfs. Maybe also btrfs
(modulo the issue you point out with snapshots, of course).

> But more fundamentally, I think this monotonic increase guarantee is
> completely broken by the presence of snapshots and snapshot
> rollbacks. If you change something, then a while later decide it
> broke (e.g. a production system upgrade went awry) and you roll back
> the filesystem to the pre-upgrade snapshot, then all the change
> counters and m/ctimes are guaranteed to go backwards because they
> will revert to the snapshot values. Maybe the filesystem can bump
> some internal counter for the snapshot when the revert happens, but
> until that is implemented, filesystems that support snapshots and
> rollback can't assert MONOTONIC.
> 
> And that's worse for other filesystems, because if you put them on
> dm-thinp and roll them back, they are completely unaware of the fact
> that a rollback happened and there's *nothing* the filesystem can do
> about this. Indeed, snapshots are suppose to be done on clean
> filesystems so snapshot images don't require journal recovery, so
> any crash detection put in the filesystem recovery code to guarantee
> MONOTONIC behaviour will be soundly defeated by such block device
> snapshot rollbacks.
> 
> Hence I think MONOTONIC is completely unworkable for most existing
> filesystems because snapshots and rollbacks completely break the
> underlying assumption MONOTONIC relies on: that filesystem
> modifications always move forwards in both the time and modification
> order dimensions....
> 
> This means that monotonicity is probably not acheivable by any
> existing filesystem and so should not ever be mentioned in the UAPI.
> I think userspace semantics can be simplified down to "if the change
> cookie does not match exactly, caches are invalid" combined with
> "applications are responsible for detecting temporal discontiguities
> in filesystem presentation at start up (e.g. after a crash, unclean
> shutdown, restoration from backup, snapshot rollback, etc) for
> persistent cache invalidation purposes"....
> 

I don't think we can make any sort of blanket statement about
monotonicity in the face of snapshots. Restoring a snapshot (or a backup
for that matter) means restoring the filesystem to a particular point in
time in the past. I think it's reasonable to expect that the change
attrs may roll backward in the face of these sorts of events.

> > Trond is of the opinion that monotonicity is a hard requirement, and
> > that we should not allow filesystems that can't provide that quality to
> > report STATX_VERSION at all.  His rationale is that one of the main uses
> > for this is for backup applications, and for those a counter that could
> > go backward is worse than useless.
> 
> From the perspective of a backup program doing incremental backups,
> an inode with a change counter that has a different value to the
> current backup inventory means the file contains different
> information than what the current backup inventory holds. Again,
> snapshots, rollbacks, etc.
> 
> Therefore, regardless of whether the change counter has gone
> forwards or backwards, the backup program needs to back up this
> current version of the file in this backup because it is different
> to the inventory copy.  Hence if the backup program fails to back it
> up, it will not be creating an exact backup of the user's data at
> the point in time the backup is run...
> 
> Hence I don't see that MONOTONIC is a requirement for backup
> programs - they really do have to be able to handle filesystems that
> have modifications that move backwards in time as well as forwards...

Rolling backward is not a problem in and of itself. The big issue is
that after a crash, we can end up with a change attr seen before the
crash that is now associated with a completely different inode state.

The scenario is something like:

- Change attr for an empty file starts at 1

- Write "A" to file, change attr goes to 2

- Read and statx happens (client sees "A" with change attr 2)

- Crash (before last change is logged to disk)

- Machine reboots, inode is empty, change attr back to 1

- Write "B" to file, change attr goes to 2

- Client stat's file, sees change attr 2 and assumes its cache is
correct when it isn't (should be "B" not "A" now).

The real danger comes not from the thing going backward, but the fact
that it can march forward again after going backward, and then the
client can see two different inode states associated with the same
change attr value. Jumping all the change attributes forward by a
significant amount after a crash should avoid this issue.
-- 
Jeff Layton <jlayton@...nel.org>

Powered by blists - more mailing lists