Re: [PATCH 2/3] xfs: allow extent free intents to be retried

[Dave Chinner <david@fromorbit.com>]

On Thu, Jun 15, 2023 at 07:41:50AM -0700, Darrick J. Wong wrote:
> On Thu, Jun 15, 2023 at 01:57:22PM +1000, Dave Chinner wrote:
> > On Wed, Jun 14, 2023 at 08:38:37PM -0700, Darrick J. Wong wrote:
> > > On Thu, Jun 15, 2023 at 11:42:00AM +1000, Dave Chinner wrote:
> > > > +/*
> > > > + * Fill the EFD with all extents from the EFI when we need to roll the
> > > > + * transaction and continue with a new EFI.
> > > > + */
> > > > +static void
> > > > +xfs_efd_from_efi(
> > > > +	struct xfs_efd_log_item	*efdp)
> > > > +{
> > > > +	struct xfs_efi_log_item *efip = efdp->efd_efip;
> > > > +	uint                    i;
> > > > +
> > > > +	ASSERT(efip->efi_format.efi_nextents > 0);
> > > > +
> > > > +	if (efdp->efd_next_extent == efip->efi_format.efi_nextents)
> > > > +		return;
> > > > +
> > > > +	for (i = 0; i < efip->efi_format.efi_nextents; i++) {
> > > > +	       efdp->efd_format.efd_extents[i] =
> > > > +		       efip->efi_format.efi_extents[i];
> > > > +	}
> > > > +	efdp->efd_next_extent = efip->efi_format.efi_nextents;
> > > 
> > > Odd question -- if we managed to free half the extents mentioned in an
> > > EFI before hitting -EAGAIN, then efdp->efd_next_extent should already be
> > > half of efip->efi_format.efi_nextents, right?
> > 
> > Yes, on success we normally update the EFD with the extent we just
> > completed and move the EFI/EFD cursors forwards.
> > 
> > > I suppose it's duplicative (or maybe just careful) to recopy the entire
> > > thing... but maybe that doesn't even really matter since no modern xlog
> > > code actually pays attention to what's in the EFD aside from the ID
> > > number.
> > 
> > *nod*
> > 
> > On second thoughts, now that you've questioned this behaviour, I
> > need to go back and check my assumptions about what the intent
> > creation is doing vs the current EFI vs the XEFI we are processing.
> > The new EFI we log shouldn't have the extents we've completed in it,
> > just the ones we haven't run, and I need to make sure that is
> > actually what is happening here.
> 
> That shouldn't be happening -- each of the xfs_free_extent_later calls
> below adds new incore EFIs to an xfs_defer_pending.dfp_work list and
> each xfs_defer_pending itself gets added to xfs_trans.t_dfops.  The
> defer_capture_and_commit function will turn the xfs_defer_pending into a
> new EFI log item with the queued dfp_work items attached.

Yup, I came to that conclusion when I went back over it again last
night. I added a few comments to the function about the methods
we might make to optimise it (i.e. only fill out from
efdp->efd_next_extent to efip->efi_format.efi_nextents) but also the
assumptions they rely on (xefis are always ordered in the same order
the efi extents are ordered) and the landmines that changing xefi
order processing might leave. Hence it's best just to copy the
entire EFI into the EFD and avoid all the possible silent corruption
problems that out-of-order xefi processing might cause...

> IOWs, as long as you don't call xfs_free_extent_later on any of the
> xefi_startblock/blockcount pairs where xfs_trans_free_extent returned 0,
> your assumptions are correct.
> 
> The code presented in this patch is correct.

Thanks for the double-check, I'll get the updated patches out in a
short while...

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