On Thu, Aug 06, 2015 at 01:44:24PM -0400, Brian Foster wrote:
....
> Update the EFI/EFD item handling code to use a more straightforward and
> reliable approach to error handling. If an error occurs after the EFI
> transaction is committed and before the EFD is constructed, release the
> EFI explicitly from xfs_bmap_finish(). If the EFI transaction is
> cancelled, release the EFI in the unlock handler.
>
> Once the EFD is constructed, it is responsible for releasing the EFI
> under any circumstances (including whether the EFI item aborts due to
> log I/O error).
Can you document these rules in a comment at the top of
fs/xfs/xfs_extfree_item.c?
> diff --git a/fs/xfs/xfs_extfree_item.c b/fs/xfs/xfs_extfree_item.c
> index 42c9b05..aceb54f 100644
> --- a/fs/xfs/xfs_extfree_item.c
> +++ b/fs/xfs/xfs_extfree_item.c
> @@ -61,9 +61,15 @@ __xfs_efi_release(
>
> if (atomic_dec_and_test(&efip->efi_refcount)) {
> spin_lock(&ailp->xa_lock);
> - /* xfs_trans_ail_delete() drops the AIL lock. */
> - xfs_trans_ail_delete(ailp, &efip->efi_item,
> - SHUTDOWN_LOG_IO_ERROR);
> + /*
> + * We don't know whether the EFI made it to the AIL. Remove it
> + * if so. Note that xfs_trans_ail_delete() drops the AIL lock.
> + */
> + if (efip->efi_item.li_flags & XFS_LI_IN_AIL)
> + xfs_trans_ail_delete(ailp, &efip->efi_item,
> + SHUTDOWN_LOG_IO_ERROR);
> + else
> + spin_unlock(&ailp->xa_lock);
> xfs_efi_item_free(efip);
> }
We now have a lot of this pattern:
spin_lock(&ailp->xa_lock);
if (lip->li_flags & XFS_LI_IN_AIL)
xfs_trans_ail_delete(ailp, lip, shutdown_reason);
else
spin_unlock(&ailp->xa_lock);
occurring in the code. Can you look into adding a helper function
for this, say xfs_trans_ail_remove()? (separate patch, of course!)
The rest of the patch looks fine - getting rid of almost all of
those aborted flag special cases is a big win :)
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
|