diff mbox

[18/47] xfs: refactor redo intent item processing

Message ID 146907708421.25461.405239727630066080.stgit@birch.djwong.org (mailing list archive)
State Accepted
Headers show

Commit Message

Darrick J. Wong July 21, 2016, 4:58 a.m. UTC
Refactor the EFI intent item recovery (and cancellation) functions
into a general function that scans the AIL and an intent item type
specific handler.  Move the function that recovers a single EFI item
into the extent free item code.  We'll want the generalized function
when we start wiring up more redo item types.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/xfs_extfree_item.c |   65 ++++++++++++++++
 fs/xfs/xfs_extfree_item.h |    3 +
 fs/xfs/xfs_log_recover.c  |  182 +++++++++++++++++++++------------------------
 3 files changed, 151 insertions(+), 99 deletions(-)

Comments

Christoph Hellwig Aug. 1, 2016, 8:10 a.m. UTC | #1
> +int
> +xfs_efi_recover(
> +	struct xfs_mount	*mp,
> +	struct xfs_efi_log_item	*efip)
> +{
> +	struct xfs_efd_log_item	*efdp;
> +	struct xfs_trans	*tp;
> +	int			i;
> +	int			error = 0;
> +	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
> +	 * just toss the EFI.
> +	 */
> +	for (i = 0; i < efip->efi_format.efi_nextents; i++) {
> +		extp = &(efip->efi_format.efi_extents[i]);
> +		startblock_fsb = XFS_BB_TO_FSB(mp,
> +				   XFS_FSB_TO_DADDR(mp, extp->ext_start));
> +		if ((startblock_fsb == 0) ||
> +		    (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);
> +			return -EIO;
> +		}
> +	}

I know it's just a code move, but there are lots of superflous braces
here, please remove them.

> +	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_itruncate, 0, 0, 0, &tp);
> +	if (error)
> +		return error;
> +	efdp = xfs_trans_get_efd(tp, efip, efip->efi_format.efi_nextents);
> +
> +	for (i = 0; i < efip->efi_format.efi_nextents; i++) {
> +		extp = &(efip->efi_format.efi_extents[i]);

and here..

> +		ASSERT(XFS_LSN_CMP(last_lsn, lip->li_lsn) >= 0);

This new check seems useful, but nothing in the changelog mentions
why it has been added.

Otherwise this looks fine to me.
Brian Foster Aug. 2, 2016, 6:47 p.m. UTC | #2
On Wed, Jul 20, 2016 at 09:58:04PM -0700, Darrick J. Wong wrote:
> Refactor the EFI intent item recovery (and cancellation) functions
> into a general function that scans the AIL and an intent item type
> specific handler.  Move the function that recovers a single EFI item
> into the extent free item code.  We'll want the generalized function
> when we start wiring up more redo item types.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  fs/xfs/xfs_extfree_item.c |   65 ++++++++++++++++
>  fs/xfs/xfs_extfree_item.h |    3 +
>  fs/xfs/xfs_log_recover.c  |  182 +++++++++++++++++++++------------------------
>  3 files changed, 151 insertions(+), 99 deletions(-)
> 
> 
...
> diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> index 080b54b..af608aa 100644
> --- a/fs/xfs/xfs_log_recover.c
> +++ b/fs/xfs/xfs_log_recover.c
> @@ -24,7 +24,6 @@
>  #include "xfs_bit.h"
>  #include "xfs_sb.h"
>  #include "xfs_mount.h"
> -#include "xfs_defer.h"
>  #include "xfs_da_format.h"
>  #include "xfs_da_btree.h"
>  #include "xfs_inode.h"
> @@ -4165,126 +4164,112 @@ xlog_recover_process_data(
...
>  STATIC int
> -xlog_recover_process_efis(
> +xlog_recover_process_intents(
>  	struct xlog		*log)
>  {
>  	struct xfs_log_item	*lip;
> -	struct xfs_efi_log_item	*efip;
>  	int			error = 0;
>  	struct xfs_ail_cursor	cur;
>  	struct xfs_ail		*ailp;
> +	xfs_lsn_t		last_lsn;
>  
>  	ailp = log->l_ailp;
>  	spin_lock(&ailp->xa_lock);
>  	lip = xfs_trans_ail_cursor_first(ailp, &cur, 0);
> +	last_lsn = xlog_assign_lsn(log->l_curr_cycle, log->l_curr_block);
>  	while (lip != NULL) {
>  		/*
> -		 * We're done when we see something other than an EFI.
> -		 * There should be no EFIs left in the AIL now.
> +		 * We're done when we see something other than an intent.
> +		 * There should be no intents left in the AIL now.
>  		 */
> -		if (lip->li_type != XFS_LI_EFI) {
> +		if (!xlog_item_is_intent(lip)) {
>  #ifdef DEBUG
>  			for (; lip; lip = xfs_trans_ail_cursor_next(ailp, &cur))
> -				ASSERT(lip->li_type != XFS_LI_EFI);
> +				ASSERT(!xlog_item_is_intent(lip));
>  #endif
>  			break;
>  		}
>  
>  		/*
> -		 * Skip EFIs that we've already processed.
> +		 * We should never see a redo item with a LSN higher than
> +		 * the last transaction we found in the log at the start
> +		 * of recovery.
>  		 */
> -		efip = container_of(lip, struct xfs_efi_log_item, efi_item);
> -		if (test_bit(XFS_EFI_RECOVERED, &efip->efi_flags)) {
> -			lip = xfs_trans_ail_cursor_next(ailp, &cur);
> -			continue;
> -		}
> +		ASSERT(XFS_LSN_CMP(last_lsn, lip->li_lsn) >= 0);
>  
> -		spin_unlock(&ailp->xa_lock);
> -		error = xlog_recover_process_efi(log->l_mp, efip);
> -		spin_lock(&ailp->xa_lock);
> +		switch (lip->li_type) {
> +		case XFS_LI_EFI:
> +			error = xlog_recover_process_efi(log->l_mp, ailp, lip);
> +			break;
> +		}

I think I'd prefer to see the locking remain here rather than get buried
in the helper functions. That said, this allows us to check the
recovered bit without a lock cycle. Meh, either way:

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  		if (error)
>  			goto out;
>  		lip = xfs_trans_ail_cursor_next(ailp, &cur);
> @@ -4296,15 +4281,14 @@ out:
>  }
>  
>  /*
> - * A cancel occurs when the mount has failed and we're bailing out. Release all
> - * pending EFIs so they don't pin the AIL.
> + * A cancel occurs when the mount has failed and we're bailing out.
> + * Release all pending log intent items so they don't pin the AIL.
>   */
>  STATIC int
> -xlog_recover_cancel_efis(
> +xlog_recover_cancel_intents(
>  	struct xlog		*log)
>  {
>  	struct xfs_log_item	*lip;
> -	struct xfs_efi_log_item	*efip;
>  	int			error = 0;
>  	struct xfs_ail_cursor	cur;
>  	struct xfs_ail		*ailp;
> @@ -4314,22 +4298,22 @@ xlog_recover_cancel_efis(
>  	lip = xfs_trans_ail_cursor_first(ailp, &cur, 0);
>  	while (lip != NULL) {
>  		/*
> -		 * We're done when we see something other than an EFI.
> -		 * There should be no EFIs left in the AIL now.
> +		 * We're done when we see something other than an intent.
> +		 * There should be no intents left in the AIL now.
>  		 */
> -		if (lip->li_type != XFS_LI_EFI) {
> +		if (!xlog_item_is_intent(lip)) {
>  #ifdef DEBUG
>  			for (; lip; lip = xfs_trans_ail_cursor_next(ailp, &cur))
> -				ASSERT(lip->li_type != XFS_LI_EFI);
> +				ASSERT(!xlog_item_is_intent(lip));
>  #endif
>  			break;
>  		}
>  
> -		efip = container_of(lip, struct xfs_efi_log_item, efi_item);
> -
> -		spin_unlock(&ailp->xa_lock);
> -		xfs_efi_release(efip);
> -		spin_lock(&ailp->xa_lock);
> +		switch (lip->li_type) {
> +		case XFS_LI_EFI:
> +			xlog_recover_cancel_efi(log->l_mp, ailp, lip);
> +			break;
> +		}
>  
>  		lip = xfs_trans_ail_cursor_next(ailp, &cur);
>  	}
> @@ -5140,16 +5124,16 @@ xlog_recover_finish(
>  	 */
>  	if (log->l_flags & XLOG_RECOVERY_NEEDED) {
>  		int	error;
> -		error = xlog_recover_process_efis(log);
> +		error = xlog_recover_process_intents(log);
>  		if (error) {
> -			xfs_alert(log->l_mp, "Failed to recover EFIs");
> +			xfs_alert(log->l_mp, "Failed to recover intents");
>  			return error;
>  		}
>  		/*
> -		 * Sync the log to get all the EFIs out of the AIL.
> +		 * Sync the log to get all the intents out of the AIL.
>  		 * This isn't absolutely necessary, but it helps in
>  		 * case the unlink transactions would have problems
> -		 * pushing the EFIs out of the way.
> +		 * pushing the intents out of the way.
>  		 */
>  		xfs_log_force(log->l_mp, XFS_LOG_SYNC);
>  
> @@ -5174,7 +5158,7 @@ xlog_recover_cancel(
>  	int		error = 0;
>  
>  	if (log->l_flags & XLOG_RECOVERY_NEEDED)
> -		error = xlog_recover_cancel_efis(log);
> +		error = xlog_recover_cancel_intents(log);
>  
>  	return error;
>  }
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
Darrick J. Wong Aug. 2, 2016, 8:35 p.m. UTC | #3
On Mon, Aug 01, 2016 at 01:10:23AM -0700, Christoph Hellwig wrote:
> > +int
> > +xfs_efi_recover(
> > +	struct xfs_mount	*mp,
> > +	struct xfs_efi_log_item	*efip)
> > +{
> > +	struct xfs_efd_log_item	*efdp;
> > +	struct xfs_trans	*tp;
> > +	int			i;
> > +	int			error = 0;
> > +	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
> > +	 * just toss the EFI.
> > +	 */
> > +	for (i = 0; i < efip->efi_format.efi_nextents; i++) {
> > +		extp = &(efip->efi_format.efi_extents[i]);
> > +		startblock_fsb = XFS_BB_TO_FSB(mp,
> > +				   XFS_FSB_TO_DADDR(mp, extp->ext_start));
> > +		if ((startblock_fsb == 0) ||
> > +		    (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);
> > +			return -EIO;
> > +		}
> > +	}
> 
> I know it's just a code move, but there are lots of superflous braces
> here, please remove them.

Ok.  It took me a while to figure out that you were referring to the
parentheses on the if tests and not the curly braces around the code
blocks.

> > +	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_itruncate, 0, 0, 0, &tp);
> > +	if (error)
> > +		return error;
> > +	efdp = xfs_trans_get_efd(tp, efip, efip->efi_format.efi_nextents);
> > +
> > +	for (i = 0; i < efip->efi_format.efi_nextents; i++) {
> > +		extp = &(efip->efi_format.efi_extents[i]);
> 
> and here..
> 
> > +		ASSERT(XFS_LSN_CMP(last_lsn, lip->li_lsn) >= 0);
> 
> This new check seems useful, but nothing in the changelog mentions
> why it has been added.
> 
> Otherwise this looks fine to me.

I'm adding the following to the commit message:

"Furthermore, ensure that log recovery only replays the redo items
that were in the AIL prior to recovery by checking the item LSN
against the largest LSN seen during log scanning.  As written this
should never happen, but we can be defensive anyway."

--D
diff mbox

Patch

diff --git a/fs/xfs/xfs_extfree_item.c b/fs/xfs/xfs_extfree_item.c
index 4aa0153..042efae 100644
--- a/fs/xfs/xfs_extfree_item.c
+++ b/fs/xfs/xfs_extfree_item.c
@@ -20,6 +20,7 @@ 
 #include "xfs_format.h"
 #include "xfs_log_format.h"
 #include "xfs_trans_resv.h"
+#include "xfs_bit.h"
 #include "xfs_mount.h"
 #include "xfs_trans.h"
 #include "xfs_trans_priv.h"
@@ -484,3 +485,67 @@  xfs_efd_init(
 
 	return efdp;
 }
+
+/*
+ * Process an extent free intent item that was recovered from
+ * the log.  We need to free the extents that it describes.
+ */
+int
+xfs_efi_recover(
+	struct xfs_mount	*mp,
+	struct xfs_efi_log_item	*efip)
+{
+	struct xfs_efd_log_item	*efdp;
+	struct xfs_trans	*tp;
+	int			i;
+	int			error = 0;
+	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
+	 * just toss the EFI.
+	 */
+	for (i = 0; i < efip->efi_format.efi_nextents; i++) {
+		extp = &(efip->efi_format.efi_extents[i]);
+		startblock_fsb = XFS_BB_TO_FSB(mp,
+				   XFS_FSB_TO_DADDR(mp, extp->ext_start));
+		if ((startblock_fsb == 0) ||
+		    (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);
+			return -EIO;
+		}
+	}
+
+	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_itruncate, 0, 0, 0, &tp);
+	if (error)
+		return error;
+	efdp = xfs_trans_get_efd(tp, efip, efip->efi_format.efi_nextents);
+
+	for (i = 0; i < efip->efi_format.efi_nextents; i++) {
+		extp = &(efip->efi_format.efi_extents[i]);
+		error = xfs_trans_free_extent(tp, efdp, extp->ext_start,
+					      extp->ext_len);
+		if (error)
+			goto abort_error;
+
+	}
+
+	set_bit(XFS_EFI_RECOVERED, &efip->efi_flags);
+	error = xfs_trans_commit(tp);
+	return error;
+
+abort_error:
+	xfs_trans_cancel(tp);
+	return error;
+}
diff --git a/fs/xfs/xfs_extfree_item.h b/fs/xfs/xfs_extfree_item.h
index 8fa8651..a32c794 100644
--- a/fs/xfs/xfs_extfree_item.h
+++ b/fs/xfs/xfs_extfree_item.h
@@ -98,4 +98,7 @@  int			xfs_efi_copy_format(xfs_log_iovec_t *buf,
 void			xfs_efi_item_free(xfs_efi_log_item_t *);
 void			xfs_efi_release(struct xfs_efi_log_item *);
 
+int			xfs_efi_recover(struct xfs_mount *mp,
+					struct xfs_efi_log_item *efip);
+
 #endif	/* __XFS_EXTFREE_ITEM_H__ */
diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index 080b54b..af608aa 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -24,7 +24,6 @@ 
 #include "xfs_bit.h"
 #include "xfs_sb.h"
 #include "xfs_mount.h"
-#include "xfs_defer.h"
 #include "xfs_da_format.h"
 #include "xfs_da_btree.h"
 #include "xfs_inode.h"
@@ -4165,126 +4164,112 @@  xlog_recover_process_data(
 	return 0;
 }
 
-/*
- * Process an extent free intent item that was recovered from
- * the log.  We need to free the extents that it describes.
- */
+/* Recover the EFI if necessary. */
 STATIC int
 xlog_recover_process_efi(
-	xfs_mount_t		*mp,
-	xfs_efi_log_item_t	*efip)
+	struct xfs_mount		*mp,
+	struct xfs_ail			*ailp,
+	struct xfs_log_item		*lip)
 {
-	xfs_efd_log_item_t	*efdp;
-	xfs_trans_t		*tp;
-	int			i;
-	int			error = 0;
-	xfs_extent_t		*extp;
-	xfs_fsblock_t		startblock_fsb;
-
-	ASSERT(!test_bit(XFS_EFI_RECOVERED, &efip->efi_flags));
+	struct xfs_efi_log_item		*efip;
+	int				error;
 
 	/*
-	 * First check the validity of the extents described by the
-	 * EFI.  If any are bad, then assume that all are bad and
-	 * just toss the EFI.
+	 * Skip EFIs that we've already processed.
 	 */
-	for (i = 0; i < efip->efi_format.efi_nextents; i++) {
-		extp = &(efip->efi_format.efi_extents[i]);
-		startblock_fsb = XFS_BB_TO_FSB(mp,
-				   XFS_FSB_TO_DADDR(mp, extp->ext_start));
-		if ((startblock_fsb == 0) ||
-		    (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);
-			return -EIO;
-		}
-	}
+	efip = container_of(lip, struct xfs_efi_log_item, efi_item);
+	if (test_bit(XFS_EFI_RECOVERED, &efip->efi_flags))
+		return 0;
 
-	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_itruncate, 0, 0, 0, &tp);
-	if (error)
-		return error;
-	efdp = xfs_trans_get_efd(tp, efip, efip->efi_format.efi_nextents);
+	spin_unlock(&ailp->xa_lock);
+	error = xfs_efi_recover(mp, efip);
+	spin_lock(&ailp->xa_lock);
 
-	for (i = 0; i < efip->efi_format.efi_nextents; i++) {
-		extp = &(efip->efi_format.efi_extents[i]);
-		error = xfs_trans_free_extent(tp, efdp, extp->ext_start,
-					      extp->ext_len);
-		if (error)
-			goto abort_error;
+	return error;
+}
 
-	}
+/* Release the EFI since we're cancelling everything. */
+STATIC void
+xlog_recover_cancel_efi(
+	struct xfs_mount		*mp,
+	struct xfs_ail			*ailp,
+	struct xfs_log_item		*lip)
+{
+	struct xfs_efi_log_item		*efip;
 
-	set_bit(XFS_EFI_RECOVERED, &efip->efi_flags);
-	error = xfs_trans_commit(tp);
-	return error;
+	efip = container_of(lip, struct xfs_efi_log_item, efi_item);
 
-abort_error:
-	xfs_trans_cancel(tp);
-	return error;
+	spin_unlock(&ailp->xa_lock);
+	xfs_efi_release(efip);
+	spin_lock(&ailp->xa_lock);
+}
+
+/* Is this log item a deferred action intent? */
+static inline bool xlog_item_is_intent(struct xfs_log_item *lip)
+{
+	switch (lip->li_type) {
+	case XFS_LI_EFI:
+		return true;
+	default:
+		return false;
+	}
 }
 
 /*
- * 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.
+ * When this is called, all of the log intent items which did not have
+ * corresponding log done items should be in the AIL.  What we do now
+ * is update the data structures 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.
+ * Since we process the log intent items 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 intent item 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.
+ * When we start, we know that the intents are the only things in the
+ * AIL.  As we process them, however, other items are added to the
+ * AIL.
  */
 STATIC int
-xlog_recover_process_efis(
+xlog_recover_process_intents(
 	struct xlog		*log)
 {
 	struct xfs_log_item	*lip;
-	struct xfs_efi_log_item	*efip;
 	int			error = 0;
 	struct xfs_ail_cursor	cur;
 	struct xfs_ail		*ailp;
+	xfs_lsn_t		last_lsn;
 
 	ailp = log->l_ailp;
 	spin_lock(&ailp->xa_lock);
 	lip = xfs_trans_ail_cursor_first(ailp, &cur, 0);
+	last_lsn = xlog_assign_lsn(log->l_curr_cycle, log->l_curr_block);
 	while (lip != NULL) {
 		/*
-		 * We're done when we see something other than an EFI.
-		 * There should be no EFIs left in the AIL now.
+		 * We're done when we see something other than an intent.
+		 * There should be no intents left in the AIL now.
 		 */
-		if (lip->li_type != XFS_LI_EFI) {
+		if (!xlog_item_is_intent(lip)) {
 #ifdef DEBUG
 			for (; lip; lip = xfs_trans_ail_cursor_next(ailp, &cur))
-				ASSERT(lip->li_type != XFS_LI_EFI);
+				ASSERT(!xlog_item_is_intent(lip));
 #endif
 			break;
 		}
 
 		/*
-		 * Skip EFIs that we've already processed.
+		 * We should never see a redo item with a LSN higher than
+		 * the last transaction we found in the log at the start
+		 * of recovery.
 		 */
-		efip = container_of(lip, struct xfs_efi_log_item, efi_item);
-		if (test_bit(XFS_EFI_RECOVERED, &efip->efi_flags)) {
-			lip = xfs_trans_ail_cursor_next(ailp, &cur);
-			continue;
-		}
+		ASSERT(XFS_LSN_CMP(last_lsn, lip->li_lsn) >= 0);
 
-		spin_unlock(&ailp->xa_lock);
-		error = xlog_recover_process_efi(log->l_mp, efip);
-		spin_lock(&ailp->xa_lock);
+		switch (lip->li_type) {
+		case XFS_LI_EFI:
+			error = xlog_recover_process_efi(log->l_mp, ailp, lip);
+			break;
+		}
 		if (error)
 			goto out;
 		lip = xfs_trans_ail_cursor_next(ailp, &cur);
@@ -4296,15 +4281,14 @@  out:
 }
 
 /*
- * A cancel occurs when the mount has failed and we're bailing out. Release all
- * pending EFIs so they don't pin the AIL.
+ * A cancel occurs when the mount has failed and we're bailing out.
+ * Release all pending log intent items so they don't pin the AIL.
  */
 STATIC int
-xlog_recover_cancel_efis(
+xlog_recover_cancel_intents(
 	struct xlog		*log)
 {
 	struct xfs_log_item	*lip;
-	struct xfs_efi_log_item	*efip;
 	int			error = 0;
 	struct xfs_ail_cursor	cur;
 	struct xfs_ail		*ailp;
@@ -4314,22 +4298,22 @@  xlog_recover_cancel_efis(
 	lip = xfs_trans_ail_cursor_first(ailp, &cur, 0);
 	while (lip != NULL) {
 		/*
-		 * We're done when we see something other than an EFI.
-		 * There should be no EFIs left in the AIL now.
+		 * We're done when we see something other than an intent.
+		 * There should be no intents left in the AIL now.
 		 */
-		if (lip->li_type != XFS_LI_EFI) {
+		if (!xlog_item_is_intent(lip)) {
 #ifdef DEBUG
 			for (; lip; lip = xfs_trans_ail_cursor_next(ailp, &cur))
-				ASSERT(lip->li_type != XFS_LI_EFI);
+				ASSERT(!xlog_item_is_intent(lip));
 #endif
 			break;
 		}
 
-		efip = container_of(lip, struct xfs_efi_log_item, efi_item);
-
-		spin_unlock(&ailp->xa_lock);
-		xfs_efi_release(efip);
-		spin_lock(&ailp->xa_lock);
+		switch (lip->li_type) {
+		case XFS_LI_EFI:
+			xlog_recover_cancel_efi(log->l_mp, ailp, lip);
+			break;
+		}
 
 		lip = xfs_trans_ail_cursor_next(ailp, &cur);
 	}
@@ -5140,16 +5124,16 @@  xlog_recover_finish(
 	 */
 	if (log->l_flags & XLOG_RECOVERY_NEEDED) {
 		int	error;
-		error = xlog_recover_process_efis(log);
+		error = xlog_recover_process_intents(log);
 		if (error) {
-			xfs_alert(log->l_mp, "Failed to recover EFIs");
+			xfs_alert(log->l_mp, "Failed to recover intents");
 			return error;
 		}
 		/*
-		 * Sync the log to get all the EFIs out of the AIL.
+		 * Sync the log to get all the intents out of the AIL.
 		 * This isn't absolutely necessary, but it helps in
 		 * case the unlink transactions would have problems
-		 * pushing the EFIs out of the way.
+		 * pushing the intents out of the way.
 		 */
 		xfs_log_force(log->l_mp, XFS_LOG_SYNC);
 
@@ -5174,7 +5158,7 @@  xlog_recover_cancel(
 	int		error = 0;
 
 	if (log->l_flags & XLOG_RECOVERY_NEEDED)
-		error = xlog_recover_cancel_efis(log);
+		error = xlog_recover_cancel_intents(log);
 
 	return error;
 }