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:

quoted

On Sat, Aug 27, 2022 at 7:10 PM Jeff Layton [off-list ref] wrote:

quoted

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.

quoted

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.

quoted

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.

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...

-Dave.
-- 
Dave Chinner
david@fromorbit.com