Re: [PATCH 15/14] xfs: capture inode generation numbers in the ondisk exchmaps log item

[Dave Chinner <david@fromorbit.com>]

On Tue, Apr 09, 2024 at 05:05:28PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Per some very late review comments, capture the generation numbers of
> both inodes involved in a file content exchange operation so that we
> don't accidentally target files with have been reallocated.

<sigh>

That's a really poor commit message, and that's ignoring the fact
the change is actually unnecessary.

The issue raised during review didn't need code to be added - it was
a question about inode lifecycles and interactions with user driven
intent chains.  Instead of discussing and working through the issue
raised to determine if it was a real issue or not, you immediately
assumed everything had to change everywhere and started changing
code. Then it turned into a hot mess and you started ranting and
lecturing people about how you do want critical reviews of this
code, and now we have this completely unexplained, unnecessary patch
in the series.

Yes, I did perform a followup investigation to that was needed
to answer the question I had posed during review. The question was
whether the intent recovery at the end of replay is subject to inode
life-cycle events during the post-intent, pre-done portion of
recovery.

Fundamentally, intent chains run in a context that holds an inode
reference aren't subject to inode life cycle issues and so we don't
need the generation number in the intent to identify the inode. I'd
largely forgotten all this because I haven't looked at BUIs and
intent extent maps for a -long- time and so I forgot all about the
inode numbers they encode and the reasons they don't need generation
numbers.

i.e. because we can't free an inode while there is an open,
unresolved intent chain running, there can't be any life cycle
issues with inode numbers in the journal. In the case of exchange:

- exchange is done with a reference to the inode via open file
  descriptors.
- the ofds cannot be released until the exchange operation returns to
  userspace.
- the last reference to the inode is therefore held until after the
  entire intent chain is committed to the journal.
- therefore, inode freeing can only occur after the exchange returns
  to userspace and so can only occur in the journal -after- the
  intent chain is complete in the journal.

Therefore: if the intent chain in the journal is not complete
we are guaranteed that the inode in the exchange items is live and
valid in the filesytem and the intent chain is acting on the current
lifecycle instance of the inode.

So, yeah, we don't need inode generation numbers in intent items
that are acting on an inode, and we probably should document that
somewhere so we don't forget about it again...

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