Re: [PATCH] xfs: don't free EFIs before the EFDs are committed

Mark Tinguely
2013-04-03T21:45:16Z

On 04/03/13 16:02, Eric Sandeen wrote:
> On 4/3/13 2:46 PM, Eric Sandeen wrote:
>> On 4/3/13 2:12 PM, Mark Tinguely wrote:
>>> On 04/02/13 22:09, Dave Chinner wrote:
>>>> From: Dave Chinner<dchinner@redhat.com>
>>>>
>>>> Filesystems are occasionally being shut down with this error:
>>>>
>>>> xfs_trans_ail_delete_bulk: attempting to delete a log item that is
>>>> not in the AIL.
>>>>
>>>> It was diagnosed to be related to the EFI/EFD commit order when the
>>>> EFI and EFD are in different checkpoints and the EFD is committed
>>>> before the EFI here:
>>>>
>>>> http://oss.sgi.com/archives/xfs/2013-01/msg00082.html
>>>>
>>>> The real problem is that a single bit cannot fully describe the
>>>> states that the EFI/EFD processing can be in. These completion
>>>> states are:
>>>>
>>>> EFI EFI in AIL EFD Result
>>>> committed/unpinned Yes committed OK
>>>> committed/pinned No committed Shutdown
>>>> uncommitted No committed Shutdown
>>>>
>>>>
>>>> Note that the "result" field is what should happen, not what does
>>>> happen. The current logic is broken and handles the first two cases
>>>> correctly by luck. That is, the code will free the EFI if the
>>>> XFS_EFI_COMMITTED bit is*not* set, rather than if it is set. The
>>>> inverted logic "works" because if both EFI and EFD are committed,
>>>> then the first __xfs_efi_release() call clears the XFS_EFI_COMMITTED
>>>> bit, and the second frees the EFI item. Hence as long as
>>>> xfs_efi_item_committed() has been called, everything appears to be
>>>> fine.
>>>>
>>>> It is the third case where the logic fails - where
>>>> xfs_efd_item_committed() is called before xfs_efi_item_committed(),
>>>> and that results in the EFI being freed before it has been
>>>> committed. That is the bug that triggered the shutdown, d hence
>>>> keeping track of whether the EFI has been committed or not is
>>>> insufficient to correctly order the EFI/EFD operations w.r.t. the
>>>> AIL.
>>>>
>>>
>>> I think the exist xfs_efi routines are working correctly.
>>>
>>> the xfs_trans_committed_bulk() for the efi does the IOP_COMMITTED
>>> (sets the XFS_EFI_COMMITTED bit), add the entry to the AIL and then
>>> an IOP_UNPIN() which clears the XFS_EFI_COMMITTED since it is set, it
>>> should not release the efi and remove the entry from the AIL. It also
>>> correctly detects if the EFD is committed before the EFI by shutting
>>> down the filesystem.
>>
>> Well hang on, not everything can be cool in EFI-land here, if nothing
>> else this:
>>
>> __xfs_efi_release(
>> struct xfs_efi_log_item *efip)
>> {
>> struct xfs_ail *ailp = efip->efi_item.li_ailp;
>>
>> if (!test_and_clear_bit(XFS_EFI_COMMITTED,&efip->efi_flags)) {
>> 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);
>> xfs_efi_item_free(efip);
>> }
>> }
>>
>> seems obviously broken.
>>
>> * test_and_clear_bit - Clear a bit and return its old value
>>
>> so it is saying if XFS_EFI_COMMITED was *not* set, free the efi.
>> Which I think we must admit is broken.
>>
>> I have to get up to speed on this code, but it seems like at least
>> this much above is quite obviously broken, and the previously
>> proposed patch doesn't address it. Am I missing something?
>
> Or are you saying that it's working as designed, so that the first
> caller finds it set, clears it and does nothing, and the 2nd caller
> finds it unset, and frees it? help me out here ;)
>
> -Eric
>

Yes. The order is IOP_COMMITTED and then IOP_UNPIN:

The EFI IOP_COMMITTED:

xfs_efi_item_committed(
struct xfs_log_item *lip,
xfs_lsn_t lsn)
{
struct xfs_efi_log_item *efip = EFI_ITEM(lip);

set_bit(XFS_EFI_COMMITTED, &efip->efi_flags);
return lsn;
}

then

The EFI IOP_UNPIN() ->

STATIC void
xfs_efi_item_unpin(
struct xfs_log_item *lip,
int remove)
{
struct xfs_efi_log_item *efip = EFI_ITEM(lip);

if (remove) {
ASSERT(!(lip->li_flags & XFS_LI_IN_AIL));
if (lip->li_desc)
xfs_trans_del_item(lip);
xfs_efi_item_free(efip);
return;
}
__xfs_efi_release(efip);
}

and the the EFD's IOP_COMMITED:

STATIC xfs_lsn_t
xfs_efi_item_committed(
struct xfs_log_item *lip,
xfs_lsn_t lsn)
{
struct xfs_efi_log_item *efip = EFI_ITEM(lip);

set_bit(XFS_EFI_COMMITTED, &efip->efi_flags);
return lsn;
}

The code makes sure the sequence EFI and then EFD are done in order.

--Mark.