xfs
[Top] [All Lists]

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

To: Eric Sandeen <sandeen@xxxxxxxxxxx>
Subject: Re: [PATCH] xfs: don't free EFIs before the EFDs are committed
From: Mark Tinguely <tinguely@xxxxxxx>
Date: Wed, 03 Apr 2013 16:45:18 -0500
Cc: xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <515C98E1.2010700@xxxxxxxxxxx>
References: <1364958561-12440-1-git-send-email-david@xxxxxxxxxxxxx> <515C7F09.2080201@xxxxxxx> <515C8732.3000902@xxxxxxxxxxx> <515C98E1.2010700@xxxxxxxxxxx>
User-agent: Mozilla/5.0 (X11; FreeBSD amd64; rv:9.0) Gecko/20120122 Thunderbird/9.0
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@xxxxxxxxxx>

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.

<Prev in Thread] Current Thread [Next in Thread>