xfs: fix efi/efd error handling to avoid fs shutdown hangs

[cascardo/linux.git] / fs / xfs / xfs_extfree_item.c

diff --git a/fs/xfs/xfs_extfree_item.c b/fs/xfs/xfs_extfree_item.c
index 6ff738f..475b9f0 100644 (file)
--- 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);
        }
 }
@@ -128,12 +134,12 @@ xfs_efi_item_pin(
 }
 
 /*
- * While EFIs cannot really be pinned, the unpin operation is the last place at
- * which the EFI is manipulated during a transaction.  If we are being asked to
- * 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.
+ * The unpin operation is the last place an EFI is manipulated in the log. It is
+ * either inserted in the AIL or aborted in the event of a log I/O error. In
+ * either case, the EFI transaction has been successfully committed to make it
+ * this far. Therefore, we expect whoever committed the EFI to either construct
+ * and commit the EFD or drop the EFD's reference in the event of error. Simply
+ * drop the log's EFI reference now that the log is done with it.
  */
 STATIC void
 xfs_efi_item_unpin(
@@ -141,14 +147,6 @@ xfs_efi_item_unpin(
        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);
 }
 
@@ -167,6 +165,11 @@ xfs_efi_item_push(
        return XFS_ITEM_PINNED;
 }
 
+/*
+ * The EFI has been either committed or aborted if the transaction has been
+ * cancelled. If the transaction was cancelled, an EFD isn't going to be
+ * constructed and thus we free the EFI here directly.
+ */
 STATIC void
 xfs_efi_item_unlock(
        struct xfs_log_item     *lip)
@@ -412,20 +415,27 @@ xfs_efd_item_push(
        return XFS_ITEM_PINNED;
 }
 
+/*
+ * The EFD is either committed or aborted if the transaction is cancelled. If
+ * the transaction is cancelled, drop our reference to the EFI and free the EFD.
+ */
 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) {
+               xfs_efi_release(efdp->efd_efip);
+               xfs_efd_item_free(efdp);
+       }
 }
 
 /*
- * When the efd item is committed to disk, all we need to do
- * is delete our reference to our partner efi item and then
- * free ourselves.  Since we're freeing ourselves we must
- * return -1 to keep the transaction code from further referencing
- * this item.
+ * When the efd item is committed to disk, all we need to do is delete our
+ * reference to our partner efi item and then free ourselves. Since we're
+ * freeing ourselves we must return -1 to keep the transaction code from further
+ * referencing this item.
  */
 STATIC xfs_lsn_t
 xfs_efd_item_committed(
@@ -435,13 +445,14 @@ 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.
+        * Drop the EFI reference regardless of whether the EFD has been
+        * aborted. Once the EFD transaction is constructed, it is the sole
+        * responsibility of the EFD to release the EFI (even if the EFI is
+        * aborted due to log I/O error).
         */
-       if (!(lip->li_flags & XFS_LI_ABORTED))
-               xfs_efi_release(efdp->efd_efip);
-
+       xfs_efi_release(efdp->efd_efip);
        xfs_efd_item_free(efdp);
+
        return (xfs_lsn_t)-1;
 }