Re: [PATCH 1/5] xfs: remove efi from AIL in log recovery

Mark Tinguely
2014-07-07T15:29:04Z

On 07/07/14 09:30, Brian Foster wrote:
> On Wed, Jul 02, 2014 at 09:32:07AM -0500, Mark Tinguely wrote:
>> The log recovery functions xlog_recover_process_efi{s} are
>> responsible for freeing extents that did not complete in
>> xfs_bmap_finish before a forced shutdown or system crash.
>> If the extent removal fails in log recovery, then the EFI
>> will stay on the AIL and is will hang the filesystem
>> requiring a system reboot.
>>
>> This patch removes the special log recovery flag, XFS_EFI_RECOVERED.
>> That flag used to be used to decrement the EFI/EFD counter. Instead
>> call the decrement function just like we do in the log IOP sequence.
>>
>> Remove all other unprocessed EFIs from the log recovery AIL
>> when one is discovered in error.
>>
>> Signed-off-by: Mark Tinguely<tinguely@sgi.com>
>> ---
>
> This one mostly looks Ok to me, a few minor comments follow...
>
>> fs/xfs/xfs_extfree_item.c | 54 +++++++++++++++++++++++--------------
>> fs/xfs/xfs_extfree_item.h | 5 ---
>> fs/xfs/xfs_log_recover.c | 67 ++++++++++++++++++++++++++--------------------
>> fs/xfs/xfs_trans.h | 1
>> 4 files changed, 73 insertions(+), 54 deletions(-)
>>
>> Index: b/fs/xfs/xfs_extfree_item.c
>> ===================================================================
>> --- a/fs/xfs/xfs_extfree_item.c
>> +++ b/fs/xfs/xfs_extfree_item.c
>> @@ -62,9 +62,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);
>> + /*
>> + * The EFI may not be on the AIL on abort.
>> + * 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);
>> }
>> }
>> @@ -134,9 +140,10 @@ xfs_efi_item_pin(
>> * remove the EFI it's because the transaction has been cancelled and by
>> * definition that means the EFI cannot be in the AIL so remove it from the
>> * transaction and free it. Otherwise coordinate with xfs_efi_release()
>> - * to determine who gets to free the EFI.
>> + * to determine who gets to free the EFI. Call from log recovery of EFI
>> + * entries so the EFD or error handling will remove the entry.
>> */
>> -STATIC void
>> +void
>> xfs_efi_item_unpin(
>> struct xfs_log_item *lip,
>> int remove)
>> @@ -147,8 +154,6 @@ xfs_efi_item_unpin(
>> 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);
>> }
>> @@ -168,12 +173,17 @@ xfs_efi_item_push(
>> return XFS_ITEM_PINNED;
>> }
>>
>> +/*
>> + * Remove EFI entry on abort.
>> + */
>> STATIC void
>> xfs_efi_item_unlock(
>> struct xfs_log_item *lip)
>> {
>> - if (lip->li_flags& XFS_LI_ABORTED)
>> - xfs_efi_item_free(EFI_ITEM(lip));
>> + if (lip->li_flags& XFS_LI_ABORTED) {
>> + ASSERT(!(lip->li_flags& XFS_LI_IN_AIL));
>> + __xfs_efi_release(EFI_ITEM(lip));
>> + }
>> }
>>
>> /*
>> @@ -313,10 +323,6 @@ xfs_efi_release(xfs_efi_log_item_t *efip
>> {
>> ASSERT(atomic_read(&efip->efi_next_extent)>= nextents);
>> if (atomic_sub_and_test(nextents,&efip->efi_next_extent)) {
>> - /* recovery needs us to drop the EFI reference, too */
>> - if (test_bit(XFS_EFI_RECOVERED,&efip->efi_flags))
>> - __xfs_efi_release(efip);
>> -
>> __xfs_efi_release(efip);
>> /* efip may now have been freed, do not reference it again. */
>> }
>> @@ -420,8 +426,17 @@ STATIC void
>> xfs_efd_item_unlock(
>> struct xfs_log_item *lip)
>> {
>> - if (lip->li_flags& XFS_LI_ABORTED)
>> - xfs_efd_item_free(EFD_ITEM(lip));
>> + struct xfs_efd_log_item *efdp = EFD_ITEM(lip);
>> +
>> + if (!(lip->li_flags& XFS_LI_ABORTED))
>> + return;
>> +
>> + /* Free the EFI when aborting a commit. The EFI will be either
>> + * added to the AIL in a CIL push before this abort or unlocked
>> + * before the EFD unlock.
>> + */
>> + xfs_efi_release(efdp->efd_efip, efdp->efd_format.efd_nextents);
>> + xfs_efd_item_free(efdp);
>
> Given that the goal here is to lose the special efi handling and use the
> common helpers, it seems like it also makes sense now to call
> xfs_efi_release() in xfs_recover_efd_pass2() rather than open coding it.

make sense.

>> }
>>
>> /*
>> @@ -439,12 +454,11 @@ xfs_efd_item_committed(
>> struct xfs_efd_log_item *efdp = EFD_ITEM(lip);
>>
>> /*
>> - * If we got a log I/O error, it's always the case that the LR with the
>> - * EFI got unpinned and freed before the EFD got aborted.
>> + * EFI and EFDs can be in different CIL pushes. Therefore the EFI could
>> + * be on the AIL when an abort occurs, so try to release the EFI in
>> + * all cases.
>> */
>> - if (!(lip->li_flags& XFS_LI_ABORTED))
>> - xfs_efi_release(efdp->efd_efip, efdp->efd_format.efd_nextents);
>> -
>> + xfs_efi_release(efdp->efd_efip, efdp->efd_format.efd_nextents);
>> xfs_efd_item_free(efdp);
>> return (xfs_lsn_t)-1;
>> }
>> Index: b/fs/xfs/xfs_extfree_item.h
>> ===================================================================
>> --- a/fs/xfs/xfs_extfree_item.h
>> +++ b/fs/xfs/xfs_extfree_item.h
>> @@ -29,11 +29,6 @@ struct kmem_zone;
>> #define XFS_EFI_MAX_FAST_EXTENTS 16
>>
>> /*
>> - * Define EFI flag bits. Manipulated by set/clear/test_bit operators.
>> - */
>> -#define XFS_EFI_RECOVERED 1
>> -
>> -/*
>> * This is the "extent free intention" log item. It is used to log the fact
>> * that some extents need to be free. It is used in conjunction with the
>> * "extent free done" log item described below.
>> Index: b/fs/xfs/xfs_log_recover.c
>> ===================================================================
>> --- a/fs/xfs/xfs_log_recover.c
>> +++ b/fs/xfs/xfs_log_recover.c
>> @@ -3093,6 +3093,12 @@ xlog_recover_efi_pass2(
>> * xfs_trans_ail_update() drops the AIL lock.
>> */
>> xfs_trans_ail_update(log->l_ailp,&efip->efi_item, lsn);
>> +
>> + /*
>> + * Decrement the EFI/EFD counter so the EFI is removed after
>> + * processing the EFD or error handling in the caller.
>> + */
>> + xfs_efi_item_unpin(&efip->efi_item, 0);
>> return 0;
>> }
>>
>> @@ -3635,6 +3641,8 @@ xlog_recover_process_data(
>> /*
>> * Process an extent free intent item that was recovered from
>> * the log. We need to free the extents that it describes.
>> + * The processing of the EFD will free the EFI and remove it from the AIL.
>> + * The caller will remove any other EFIs on the the AIL.
>> */
>> STATIC int
>> xlog_recover_process_efi(
>> @@ -3648,8 +3656,6 @@ xlog_recover_process_efi(
>> xfs_extent_t *extp;
>> xfs_fsblock_t startblock_fsb;
>>
>> - ASSERT(!test_bit(XFS_EFI_RECOVERED,&efip->efi_flags));
>> -
>> /*
>> * First check the validity of the extents described by the
>> * EFI. If any are bad, then assume that all are bad and
>> @@ -3663,13 +3669,8 @@ xlog_recover_process_efi(
>> (extp->ext_len == 0) ||
>> (startblock_fsb>= mp->m_sb.sb_dblocks) ||
>> (extp->ext_len>= mp->m_sb.sb_agblocks)) {
>> - /*
>> - * This will pull the EFI from the AIL and
>> - * free the memory associated with it.
>> - */
>> - set_bit(XFS_EFI_RECOVERED,&efip->efi_flags);
>> - xfs_efi_release(efip, efip->efi_format.efi_nextents);
>> - return XFS_ERROR(EIO);
>> + error = XFS_ERROR(EIO);
>> + goto return_free;
>
> This bit doesn't apply to for-next. I get similar problems with less
> trivial hunks on the subsequent patch as well. Looks like you might need
> to rebase the series onto the recent error negation patches..?

WTF? Why are the changes not in the dev tree? Why do people have to do
development to the for-next tree?

>
>> }
>> }
>>
>> @@ -3682,45 +3683,58 @@ xlog_recover_process_efi(
>> for (i = 0; i< efip->efi_format.efi_nextents; i++) {
>> extp =&(efip->efi_format.efi_extents[i]);
>> error = xfs_free_extent(tp, extp->ext_start, extp->ext_len);
>> - if (error)
>> - goto abort_error;
>> xfs_trans_log_efd_extent(tp, efdp, extp->ext_start,
>> extp->ext_len);
>> + if (error) {
>> + /* The error may be the first extent or there may the
>> + * EFD may be dirty on the transaction by another
>> + * extent. Make the EFD dirty on the transactions
>> + * so the xfs_trans_cancel frees EFI/EFD and removes
>> + * EFI from AIL.
>> + */
>
> I can't quite follow this comment. Is it referring to the unconditional
> xfs_trans_log_efd_extent() call? If so, it might make more sense to move
> it before that call.

Yeah, okay. We have to force it dirty in the transaction even on error.

>
> Brian
>
>> + xfs_trans_cancel(tp, XFS_TRANS_ABORT);
>> + return error;
>> + }
>> }
>>
>> - set_bit(XFS_EFI_RECOVERED,&efip->efi_flags);
>> error = xfs_trans_commit(tp, 0);
>> - return error;
>> + if (error)
>> + goto return_free;
>> + return 0;
>>
>> abort_error:
>> xfs_trans_cancel(tp, XFS_TRANS_ABORT);
>> +return_free:
>> + xfs_efi_release(efip, efip->efi_format.efi_nextents);
>> return error;
>> }
>>
>> /*
>> * When this is called, all of the EFIs which did not have
>> - * corresponding EFDs should be in the AIL. What we do now
>> - * is free the extents associated with each one.
>> + * corresponding EFDs should be in the AIL. The initial decrement
>> + * on the EFI/EFD sequence counter has been done when the EFI is placed
>> + * on the AIL. What we do now is free the extents associated with each one.
>> *
>> * Since we process the EFIs in normal transactions, they
>> * will be removed at some point after the commit. This prevents
>> * us from just walking down the list processing each one.
>> - * We'll use a flag in the EFI to skip those that we've already
>> - * processed and use the AIL iteration mechanism's generation
>> - * count to try to speed this up at least a bit.
>> *
>> * When we start, we know that the EFIs are the only things in
>> * the AIL. As we process them, however, other items are added
>> * to the AIL. Since everything added to the AIL must come after
>> * everything already in the AIL, we stop processing as soon as
>> * we see something other than an EFI in the AIL.
>> + *
>> + * If an error is detected while freeing extents in the EFI, discard all
>> + * future EFI on the AIL. This is done by the xfs_efi_release() which is
>> + * the same processing as a successful EFD completion processing.
>> */
>> STATIC int
>> xlog_recover_process_efis(
>> struct xlog *log)
>> {
>> - xfs_log_item_t *lip;
>> - xfs_efi_log_item_t *efip;
>> + struct xfs_log_item *lip;
>> + struct xfs_efi_log_item *efip;
>> int error = 0;
>> struct xfs_ail_cursor cur;
>> struct xfs_ail *ailp;
>> @@ -3745,19 +3759,14 @@ xlog_recover_process_efis(
>> * Skip EFIs that we've already processed.
>> */
>> efip = (xfs_efi_log_item_t *)lip;
>> - if (test_bit(XFS_EFI_RECOVERED,&efip->efi_flags)) {
>> - lip = xfs_trans_ail_cursor_next(ailp,&cur);
>> - continue;
>> - }
>> -
>> spin_unlock(&ailp->xa_lock);
>> - error = xlog_recover_process_efi(log->l_mp, efip);
>> + if (!error)
>> + error = xlog_recover_process_efi(log->l_mp, efip);
>> + else
>> + xfs_efi_release(efip, efip->efi_format.efi_nextents);
>> spin_lock(&ailp->xa_lock);
>> - if (error)
>> - goto out;
>> lip = xfs_trans_ail_cursor_next(ailp,&cur);
>> }
>> -out:
>> xfs_trans_ail_cursor_done(&cur);
>> spin_unlock(&ailp->xa_lock);
>> return error;
>> Index: b/fs/xfs/xfs_trans.h
>> ===================================================================
>> --- a/fs/xfs/xfs_trans.h
>> +++ b/fs/xfs/xfs_trans.h
>> @@ -216,6 +216,7 @@ void xfs_trans_ijoin(struct xfs_trans *
>> void xfs_trans_log_buf(xfs_trans_t *, struct xfs_buf *, uint, uint);
>> void xfs_trans_log_inode(xfs_trans_t *, struct xfs_inode *, uint);
>> struct xfs_efi_log_item *xfs_trans_get_efi(xfs_trans_t *, uint);
>> +void xfs_efi_item_unpin(struct xfs_log_item *, int);
>> void xfs_efi_release(struct xfs_efi_log_item *, uint);
>> void xfs_trans_log_efi_extent(xfs_trans_t *,
>> struct xfs_efi_log_item *,
>>
>>
>> _______________________________________________
>> xfs mailing list
>> xfs@oss.sgi.com
>> http://oss.sgi.com/mailman/listinfo/xfs