Skip to content

Commit 0853b5d

Browse files
Dave ChinnerDarrick J. Wong
authored andcommitted
xfs: allow extent free intents to be retried
Extent freeing neeeds to be able to avoid a busy extent deadlock when the transaction itself holds the only busy extents in the allocation group. This may occur if we have an EFI that contains multiple extents to be freed, and the freeing the second intent requires the space the first extent free released to expand the AGFL. If we block on the busy extent at this point, we deadlock. We hold a dirty transaction that contains a entire atomic extent free operations within it, so if we can abort the extent free operation and commit the progress that we've made, the busy extent can be resolved by a log force. Hence we can restart the aborted extent free with a new transaction and continue to make progress without risking deadlocks. To enable this, we need the EFI processing code to be able to handle an -EAGAIN error to tell it to commit the current transaction and retry again. This mechanism is already built into the defer ops processing (used bythe refcount btree modification intents), so there's relatively little handling we need to add to the EFI code to enable this. Signed-off-by: Dave Chinner <dchinner@redhat.com> Reviewed-by: Darrick J. Wong <djwong@kernel.org> Signed-off-by: Darrick J. Wong <djwong@kernel.org> Reviewed-by: Chandan Babu R <chandan.babu@oracle.com>
1 parent 6a2a9d7 commit 0853b5d

File tree

1 file changed

+69
-3
lines changed

1 file changed

+69
-3
lines changed

fs/xfs/xfs_extfree_item.c

Lines changed: 69 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -336,6 +336,34 @@ xfs_trans_get_efd(
336336
return efdp;
337337
}
338338

339+
/*
340+
* Fill the EFD with all extents from the EFI when we need to roll the
341+
* transaction and continue with a new EFI.
342+
*
343+
* This simply copies all the extents in the EFI to the EFD rather than make
344+
* assumptions about which extents in the EFI have already been processed. We
345+
* currently keep the xefi list in the same order as the EFI extent list, but
346+
* that may not always be the case. Copying everything avoids leaving a landmine
347+
* were we fail to cancel all the extents in an EFI if the xefi list is
348+
* processed in a different order to the extents in the EFI.
349+
*/
350+
static void
351+
xfs_efd_from_efi(
352+
struct xfs_efd_log_item *efdp)
353+
{
354+
struct xfs_efi_log_item *efip = efdp->efd_efip;
355+
uint i;
356+
357+
ASSERT(efip->efi_format.efi_nextents > 0);
358+
ASSERT(efdp->efd_next_extent < efip->efi_format.efi_nextents);
359+
360+
for (i = 0; i < efip->efi_format.efi_nextents; i++) {
361+
efdp->efd_format.efd_extents[i] =
362+
efip->efi_format.efi_extents[i];
363+
}
364+
efdp->efd_next_extent = efip->efi_format.efi_nextents;
365+
}
366+
339367
/*
340368
* Free an extent and log it to the EFD. Note that the transaction is marked
341369
* dirty regardless of whether the extent free succeeds or fails to support the
@@ -378,6 +406,17 @@ xfs_trans_free_extent(
378406
tp->t_flags |= XFS_TRANS_DIRTY | XFS_TRANS_HAS_INTENT_DONE;
379407
set_bit(XFS_LI_DIRTY, &efdp->efd_item.li_flags);
380408

409+
/*
410+
* If we need a new transaction to make progress, the caller will log a
411+
* new EFI with the current contents. It will also log an EFD to cancel
412+
* the existing EFI, and so we need to copy all the unprocessed extents
413+
* in this EFI to the EFD so this works correctly.
414+
*/
415+
if (error == -EAGAIN) {
416+
xfs_efd_from_efi(efdp);
417+
return error;
418+
}
419+
381420
next_extent = efdp->efd_next_extent;
382421
ASSERT(next_extent < efdp->efd_format.efd_nextents);
383422
extp = &(efdp->efd_format.efd_extents[next_extent]);
@@ -495,6 +534,13 @@ xfs_extent_free_finish_item(
495534

496535
error = xfs_trans_free_extent(tp, EFD_ITEM(done), xefi);
497536

537+
/*
538+
* Don't free the XEFI if we need a new transaction to complete
539+
* processing of it.
540+
*/
541+
if (error == -EAGAIN)
542+
return error;
543+
498544
xfs_extent_free_put_group(xefi);
499545
kmem_cache_free(xfs_extfree_item_cache, xefi);
500546
return error;
@@ -620,6 +666,7 @@ xfs_efi_item_recover(
620666
struct xfs_trans *tp;
621667
int i;
622668
int error = 0;
669+
bool requeue_only = false;
623670

624671
/*
625672
* First check the validity of the extents described by the
@@ -653,9 +700,28 @@ xfs_efi_item_recover(
653700
fake.xefi_startblock = extp->ext_start;
654701
fake.xefi_blockcount = extp->ext_len;
655702

656-
xfs_extent_free_get_group(mp, &fake);
657-
error = xfs_trans_free_extent(tp, efdp, &fake);
658-
xfs_extent_free_put_group(&fake);
703+
if (!requeue_only) {
704+
xfs_extent_free_get_group(mp, &fake);
705+
error = xfs_trans_free_extent(tp, efdp, &fake);
706+
xfs_extent_free_put_group(&fake);
707+
}
708+
709+
/*
710+
* If we can't free the extent without potentially deadlocking,
711+
* requeue the rest of the extents to a new so that they get
712+
* run again later with a new transaction context.
713+
*/
714+
if (error == -EAGAIN || requeue_only) {
715+
error = xfs_free_extent_later(tp, fake.xefi_startblock,
716+
fake.xefi_blockcount,
717+
&XFS_RMAP_OINFO_ANY_OWNER,
718+
fake.xefi_agresv);
719+
if (!error) {
720+
requeue_only = true;
721+
continue;
722+
}
723+
};
724+
659725
if (error == -EFSCORRUPTED)
660726
XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp,
661727
extp, sizeof(*extp));

0 commit comments

Comments
 (0)