xfs
[Top] [All Lists]

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

To: Brian Foster <bfoster@xxxxxxxxxx>
Subject: Re: [PATCH 1/2] xfs: remove efi from AIL in log recovery error
From: Mark Tinguely <tinguely@xxxxxxx>
Date: Fri, 28 Mar 2014 10:41:06 -0500
Cc: xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <20140328152434.GB21961@xxxxxxxxxxxxxxx>
References: <20140325195733.510384972@xxxxxxx> <20140325195819.638326569@xxxxxxx> <20140328152434.GB21961@xxxxxxxxxxxxxxx>
User-agent: Mozilla/5.0 (X11; FreeBSD amd64; rv:9.0) Gecko/20120122 Thunderbird/9.0
On 03/28/14 10:24, Brian Foster wrote:
On Tue, Mar 25, 2014 at 03:06:34PM -0500, Mark Tinguely wrote:
xlog_recover_process_efi{s}() functions are completing the
second half of xfs_bmap_finish() which frees extents. If this
operation fails, the EFI will stay on the AIL and prevents
the xfs_ail_push all_sync() from completing and the mount will
fail to unmount.

Rather than have a special log recovery flag XFS_EFI_RECOVERED
to decrement the EFI/EFD counter, call the same decrement function
from the log recovery routine that is called then the EFI is added
to the AIL from a log write.

Remove all other unprocessed EFIs from the log recovery AIL
when one is discovered in error.

Signed-off-by: Mark Tinguely<tinguely@xxxxxxx>
---
Rewritten with suggestions from Dave.
Note: calling xfs_efi_item_unpin() seemed more explainatory than calling
       the helper __xfs_efi_release().

  fs/xfs/xfs_extfree_item.c |    9 +++------
  fs/xfs/xfs_log_recover.c  |   28 +++++++++++++++-------------
  fs/xfs/xfs_trans.h        |    1 +
  3 files changed, 19 insertions(+), 19 deletions(-)

Index: b/fs/xfs/xfs_extfree_item.c
===================================================================
--- a/fs/xfs/xfs_extfree_item.c
+++ b/fs/xfs/xfs_extfree_item.c
@@ -134,9 +134,10 @@ xfs_efi_item_pin(
   * remove the EFI it's because the transaction has been canceled 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)
@@ -313,10 +314,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. */
        }
Index: b/fs/xfs/xfs_log_recover.c
===================================================================
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -3634,6 +3634,7 @@ 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 caller will release this and any following EFIs upon error.
   */
  STATIC int
  xlog_recover_process_efi(
@@ -3648,6 +3649,13 @@ xlog_recover_process_efi(
        xfs_fsblock_t           startblock_fsb;

        ASSERT(!test_bit(XFS_EFI_RECOVERED,&efip->efi_flags));
+       set_bit(XFS_EFI_RECOVERED,&efip->efi_flags);
+
+       /*
+        * 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);

        /*
         * First check the validity of the extents described by the
@@ -3662,12 +3670,6 @@ 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);
                }
        }
@@ -3687,7 +3689,6 @@ xlog_recover_process_efi(
                                         extp->ext_len);
        }

-       set_bit(XFS_EFI_RECOVERED,&efip->efi_flags);
        error = xfs_trans_commit(tp, 0);
        return error;

@@ -3718,8 +3719,8 @@ 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;
@@ -3750,13 +3751,14 @@ xlog_recover_process_efis(
                }

                spin_unlock(&ailp->xa_lock);
-               error = xlog_recover_process_efi(log->l_mp, efip);
-               spin_lock(&ailp->xa_lock);
+               /* Skip all EFIs after first EFI in error. */
+               if (!error)
+                       error = xlog_recover_process_efi(log->l_mp, efip);
                if (error)
-                       goto out;
+                       xfs_efi_release(efip, efip->efi_format.efi_nextents);

Hi Mark,

If we hit the scenario where we start skipping EFIs after an error, is
the equivalent unpin() call from process_efi() not necessary on the
subsequent EFIs?

Brian

yes, good catch. They will have to be decremented twice. something like:
+               if (!error)
+                       error = xlog_recover_process_efi(log->l_mp, efip);
+               else
+                       xfs_efi_item_unpin(&efip->efi_item, 0);
+               if (error)
...

--Mark

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