[PATCH 0/2] misc log recovery patches

[PATCH 0/2] misc log recovery patches

[Mark Tinguely]

A couple log recovery patches.

patch 1 is a correction to commit:
	commit 2a84108fe275f95fbe838b1c92b7c45258dcae5c
	Author: Mark Tinguely <tinguely@sgi.com>
	Date:   Wed Oct 2 07:51:12 2013 -0500
	xfs: free the list of recovery items on error

Errors in phase 1/2 of xlog_recover_commit_trans will free the
transaction pointer and the new call to xlog_recover_process_data
will reuse and refree the pointer. I missed it in testing of the
2a84108 patch. Found by Dan Carpenter and verified by forcing an
error in xlog_recover_process_data.

	---

patch 2 is the second version of the patch to remove the EFI from
the AIL when log recovery of the EFI item fails. If the EFI entry is
not removed from the AIL, then xfs_ail_push_all_sync will hang while
doing the forced shutdown. I moved the removal of all EFIs from the
AIL to the caller, xlog_recover_process_efis to catch all the errors
coming from xlog_recover_process_efi. This bug was found and verified
using a metadata dump of a filesystem that has an error freeing an
extent.

Mark.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 1/2] xfs: fix double free on error when cleaning log items

[Mark Tinguely]

[-- Attachment #1: xfs-fix-double-free-on-error-when-cleanning-log_items.patch --]
[-- Type: text/plain, Size: 1260 bytes --]

Commit 2a84108 cleans the remaining pending log item entries
when log recovery fails. Unfortunately, the cleaning call was
not removed from the error path in xlog_recover_commit_trans,
This can result in a use after free and a second free of the
transaction structure when the cleaning is done in
xlog_recover_process_data.

Now the log item entry cleaning in xlog_recover_commit_trans
is only performed for the non-error case.

Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Mark Tinguely <tinguely@sgi.com>
---
 fs/xfs/xfs_log_recover.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Index: b/fs/xfs/xfs_log_recover.c
===================================================================
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -3509,9 +3509,10 @@ out:
 	if (!list_empty(&done_list))
 		list_splice_init(&done_list, &trans->r_itemq);
 
-	xlog_recover_free_trans(trans);
-
 	error2 = xfs_buf_delwri_submit(&buffer_list);
+	/* caller will free transactions in the error path */
+	if (!error && !error2)
+		xlog_recover_free_trans(trans);
 	return error ? error : error2;
 }
 


_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 1/2] xfs: fix double free on error when cleaning log items

[Dave Chinner]

On Fri, Dec 06, 2013 at 03:20:28PM -0600, Mark Tinguely wrote:
> Commit 2a84108 cleans the remaining pending log item entries
> when log recovery fails. Unfortunately, the cleaning call was
> not removed from the error path in xlog_recover_commit_trans,
> This can result in a use after free and a second free of the
> transaction structure when the cleaning is done in
> xlog_recover_process_data.
> 
> Now the log item entry cleaning in xlog_recover_commit_trans
> is only performed for the non-error case.
> 
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Mark Tinguely <tinguely@sgi.com>
> ---
>  fs/xfs/xfs_log_recover.c |    5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> Index: b/fs/xfs/xfs_log_recover.c
> ===================================================================
> --- a/fs/xfs/xfs_log_recover.c
> +++ b/fs/xfs/xfs_log_recover.c
> @@ -3509,9 +3509,10 @@ out:
>  	if (!list_empty(&done_list))
>  		list_splice_init(&done_list, &trans->r_itemq);
>  
> -	xlog_recover_free_trans(trans);
> -
>  	error2 = xfs_buf_delwri_submit(&buffer_list);
> +	/* caller will free transactions in the error path */
> +	if (!error && !error2)
> +		xlog_recover_free_trans(trans);
>  	return error ? error : error2;
>  }

Mark, please stop and think about the problem being reported and
consider the wider context of it rather than just slapping band-aids
over the code that solve just the specific problem being reported.


Indeed, the above change doesn't take into account that the
transaction has already been removed from the hash by
xlog_recover_process_data(), and so it must always free the
transaction structure here regardless of whether it succeeds or not
as nobody else can ever find it by lookup.

Further, it doesn't take into account the fact that calling
xlog_recover_free_trans() without removing the transaction from the
recovery hash in the error paths leaves a vector for use-after-free
(and hence memory corruption) remaining behind. Hence the change in
2a84108 turned minor memory leaks into memory corruption vectors.

And finally, it misses the other error handling paths where
xlog_recover_process_data() leaks the trans structure that
it is working on....

So, let's go back to the "callee cleans up" semantics that were in
place before commit 2a84108 and fix all these problems in one go,
rather than slapping a bandaid on them one at a time as they are
reported.

Smoke tested patch to fix all this below.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

xfs: xlog_recover_process_data leaks like a sieve

From: Dave Chinner <dchinner@redhat.com>

Fix the double free of the transaction structure introduced by
commit 2a84108 ("xfs: free the list of recovery items on error").
In the process, make the freeing of the trans structure on error or
completion of processing consistent - i.e. the responsibility of the
the function that detected the error or completes processing. Add
comments to document this behaviour so it can be maintained more
easily in future.

Fix the rest of the memory leaks of the transaction structure used
by xlog_recover_process_data() that commit 2a84108 didn't address.

Fix potential use-after-free of the trans structure by ensuring
they are removed from the transaction recovery-in-progress hash
table before they are freed.

Remove all the shouty XFS_ERROR() macros that are used directly
after ASSERT(0) calls as they are entirely redundant and make the
code harder to read.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/xfs_log_recover.c | 155 +++++++++++++++++++++++++++--------------------
 1 file changed, 88 insertions(+), 67 deletions(-)

diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index 07ab52c..517f7ee 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -1483,6 +1483,32 @@ xlog_recover_add_item(
 	list_add_tail(&item->ri_list, head);
 }
 
+/*
+ * Free up any resources allocated by the transaction
+ *
+ * Remember that EFIs, EFDs, and IUNLINKs are handled later.
+ */
+STATIC void
+xlog_recover_free_trans(
+	struct xlog_recover	*trans)
+{
+	xlog_recover_item_t	*item, *n;
+	int			i;
+
+	hlist_del_init(&trans->r_list);
+	list_for_each_entry_safe(item, n, &trans->r_itemq, ri_list) {
+		/* Free the regions in the item. */
+		list_del(&item->ri_list);
+		for (i = 0; i < item->ri_cnt; i++)
+			kmem_free(item->ri_buf[i].i_addr);
+		/* Free the item itself */
+		kmem_free(item->ri_buf);
+		kmem_free(item);
+	}
+	/* Free the transaction recover structure */
+	kmem_free(trans);
+}
+
 STATIC int
 xlog_recover_add_to_cont_trans(
 	struct xlog		*log,
@@ -1548,7 +1574,7 @@ xlog_recover_add_to_trans(
 			xfs_warn(log->l_mp, "%s: bad header magic number",
 				__func__);
 			ASSERT(0);
-			return XFS_ERROR(EIO);
+			goto out_eio;
 		}
 		if (len == sizeof(xfs_trans_header_t))
 			xlog_recover_add_item(&trans->r_itemq);
@@ -1577,8 +1603,8 @@ xlog_recover_add_to_trans(
 		"bad number of regions (%d) in inode log format",
 				  in_f->ilf_size);
 			ASSERT(0);
-			kmem_free(ptr);
-			return XFS_ERROR(EIO);
+			goto out_free_eio;
+
 		}
 
 		item->ri_total = in_f->ilf_size;
@@ -1593,6 +1619,17 @@ xlog_recover_add_to_trans(
 	item->ri_cnt++;
 	trace_xfs_log_recover_item_add(log, trans, item, 0);
 	return 0;
+
+out_free_eio:
+	kmem_free(ptr);
+out_eio:
+	/*
+	 * This transaction is now unrecoverable, so we need to remove it from
+	 * the transaction hash so nobody else can find it and free it.  The
+	 * error we return will abort further recovery processing.
+	 */
+	xlog_recover_free_trans(trans);
+	return EIO;
 }
 
 /*
@@ -1699,7 +1736,7 @@ xlog_recover_reorder_trans(
 			 */
 			if (!list_empty(&sort_list))
 				list_splice_init(&sort_list, &trans->r_itemq);
-			error = XFS_ERROR(EIO);
+			error = EIO;
 			goto out;
 		}
 	}
@@ -1713,6 +1750,15 @@ out:
 		list_splice_tail(&inode_buffer_list, &trans->r_itemq);
 	if (!list_empty(&cancel_list))
 		list_splice_tail(&cancel_list, &trans->r_itemq);
+
+	/*
+	 * If we failed to reorder the transaction, it is now unrecoverable so
+	 * we need to remove it from the transaction hash so nobody else can
+	 * find it and free it.  The error we return will abort further recovery
+	 * processing.
+	 */
+	if (error)
+		xlog_recover_free_trans(trans);
 	return error;
 }
 
@@ -3235,31 +3281,6 @@ xlog_recover_do_icreate_pass2(
 	return 0;
 }
 
-/*
- * Free up any resources allocated by the transaction
- *
- * Remember that EFIs, EFDs, and IUNLINKs are handled later.
- */
-STATIC void
-xlog_recover_free_trans(
-	struct xlog_recover	*trans)
-{
-	xlog_recover_item_t	*item, *n;
-	int			i;
-
-	list_for_each_entry_safe(item, n, &trans->r_itemq, ri_list) {
-		/* Free the regions in the item. */
-		list_del(&item->ri_list);
-		for (i = 0; i < item->ri_cnt; i++)
-			kmem_free(item->ri_buf[i].i_addr);
-		/* Free the item itself */
-		kmem_free(item->ri_buf);
-		kmem_free(item);
-	}
-	/* Free the transaction recover structure */
-	kmem_free(trans);
-}
-
 STATIC void
 xlog_recover_buffer_ra_pass2(
 	struct xlog                     *log,
@@ -3384,7 +3405,7 @@ xlog_recover_commit_pass1(
 		xfs_warn(log->l_mp, "%s: invalid item type (%d)",
 			__func__, ITEM_TYPE(item));
 		ASSERT(0);
-		return XFS_ERROR(EIO);
+		return EIO;
 	}
 }
 
@@ -3420,7 +3441,7 @@ xlog_recover_commit_pass2(
 		xfs_warn(log->l_mp, "%s: invalid item type (%d)",
 			__func__, ITEM_TYPE(item));
 		ASSERT(0);
-		return XFS_ERROR(EIO);
+		return EIO;
 	}
 }
 
@@ -3467,7 +3488,7 @@ xlog_recover_commit_trans(
 
 	#define XLOG_RECOVER_COMMIT_QUEUE_MAX 100
 
-	hlist_del(&trans->r_list);
+	hlist_del_init(&trans->r_list);
 
 	error = xlog_recover_reorder_trans(log, trans, pass);
 	if (error)
@@ -3488,17 +3509,17 @@ xlog_recover_commit_trans(
 				list_splice_tail_init(&ra_list, &done_list);
 				items_queued = 0;
 			}
-
 			break;
 		default:
 			ASSERT(0);
+			error = ERANGE;
+			break;
 		}
 
 		if (error)
-			goto out;
+			break;
 	}
 
-out:
 	if (!list_empty(&ra_list)) {
 		if (!error)
 			error = xlog_recover_items_pass2(log, trans,
@@ -3509,22 +3530,17 @@ out:
 	if (!list_empty(&done_list))
 		list_splice_init(&done_list, &trans->r_itemq);
 
+	/*
+	 * We've already removed the trans structure from the hash, so nobody
+	 * else will ever find this structure again. Hence we must free it here
+	 * regardless of whether we processed it successfully or not.
+	 */
 	xlog_recover_free_trans(trans);
 
 	error2 = xfs_buf_delwri_submit(&buffer_list);
 	return error ? error : error2;
 }
 
-STATIC int
-xlog_recover_unmount_trans(
-	struct xlog		*log,
-	struct xlog_recover	*trans)
-{
-	/* Do nothing now */
-	xfs_warn(log->l_mp, "%s: Unmount LR", __func__);
-	return 0;
-}
-
 /*
  * There are two valid states of the r_state field.  0 indicates that the
  * transaction structure is in a normal state.  We have either seen the
@@ -3545,9 +3561,9 @@ xlog_recover_process_data(
 	xfs_caddr_t		lp;
 	int			num_logops;
 	xlog_op_header_t	*ohead;
-	xlog_recover_t		*trans;
+	xlog_recover_t		*trans = NULL;
 	xlog_tid_t		tid;
-	int			error;
+	int			error = 0;
 	unsigned long		hash;
 	uint			flags;
 
@@ -3556,7 +3572,7 @@ xlog_recover_process_data(
 
 	/* check the log format matches our own - else we can't recover */
 	if (xlog_header_check_recover(log->l_mp, rhead))
-		return (XFS_ERROR(EIO));
+		return XFS_ERROR(EIO);
 
 	while ((dp < lp) && num_logops) {
 		ASSERT(dp + sizeof(xlog_op_header_t) <= lp);
@@ -3564,10 +3580,13 @@ xlog_recover_process_data(
 		dp += sizeof(xlog_op_header_t);
 		if (ohead->oh_clientid != XFS_TRANSACTION &&
 		    ohead->oh_clientid != XFS_LOG) {
-			xfs_warn(log->l_mp, "%s: bad clientid 0x%x",
+			xfs_warn(log->l_mp,
+				"%s: bad transaction opheader clientid 0x%x",
 					__func__, ohead->oh_clientid);
 			ASSERT(0);
-			return (XFS_ERROR(EIO));
+			if (trans)
+				xlog_recover_free_trans(trans);
+			return EIO;
 		}
 		tid = be32_to_cpu(ohead->oh_tid);
 		hash = XLOG_RHASH(tid);
@@ -3578,11 +3597,14 @@ xlog_recover_process_data(
 					be64_to_cpu(rhead->h_lsn));
 		} else {
 			if (dp + be32_to_cpu(ohead->oh_len) > lp) {
-				xfs_warn(log->l_mp, "%s: bad length 0x%x",
+				xfs_warn(log->l_mp,
+				"%s: bad transaction opheader length 0x%x",
 					__func__, be32_to_cpu(ohead->oh_len));
 				WARN_ON(1);
-				return (XFS_ERROR(EIO));
+				xlog_recover_free_trans(trans);
+				return EIO;
 			}
+
 			flags = ohead->oh_flags & ~XLOG_END_TRANS;
 			if (flags & XLOG_WAS_CONT_TRANS)
 				flags &= ~XLOG_CONTINUE_TRANS;
@@ -3591,36 +3613,35 @@ xlog_recover_process_data(
 				error = xlog_recover_commit_trans(log,
 								trans, pass);
 				break;
-			case XLOG_UNMOUNT_TRANS:
-				error = xlog_recover_unmount_trans(log, trans);
-				break;
 			case XLOG_WAS_CONT_TRANS:
 				error = xlog_recover_add_to_cont_trans(log,
 						trans, dp,
 						be32_to_cpu(ohead->oh_len));
 				break;
-			case XLOG_START_TRANS:
-				xfs_warn(log->l_mp, "%s: bad transaction",
-					__func__);
-				ASSERT(0);
-				error = XFS_ERROR(EIO);
-				break;
 			case 0:
 			case XLOG_CONTINUE_TRANS:
 				error = xlog_recover_add_to_trans(log, trans,
 						dp, be32_to_cpu(ohead->oh_len));
 				break;
+			case XLOG_UNMOUNT_TRANS:
+				xfs_warn(log->l_mp, "%s: Unmount LR", __func__);
+				break;
+			case XLOG_START_TRANS:
 			default:
-				xfs_warn(log->l_mp, "%s: bad flag 0x%x",
+				xfs_warn(log->l_mp,
+				"%s: bad transaction opheader flag 0x%x",
 					__func__, flags);
 				ASSERT(0);
-				error = XFS_ERROR(EIO);
-				break;
-			}
-			if (error) {
 				xlog_recover_free_trans(trans);
-				return error;
+				return EIO;
 			}
+			/*
+			 * If there's been an error, the trans structure has
+			 * already been freed. So there's nothing for us to do
+			 * but abort the recovery process.
+			 */
+			if (error)
+				return error;
 		}
 		dp += be32_to_cpu(ohead->oh_len);
 		num_logops--;

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 2/2] xfs: free the efi AIL entry on log recovery failure

[Mark Tinguely]

[-- Attachment #1: v2-xfs-remove-efi-entry-before-log-unmount.patch --]
[-- Type: text/plain, Size: 2853 bytes --]

If an extent free fails during recovery, the filesystem will be
forced down. The efi entry is still on the AIL and the log
shutdown function xfs_ail_push_all_sync() will hang.

This patch is similar to the patches that removed the dquot and
inode in commits 32ce90a and dea9609 but removes all the EFI
entries from the AIL.

Cleaned up the typedefs while modifying the function.

Signed-off-by: Mark Tinguely <tinguely@sgi.com>
---
v2 remove all the EFIs from the AIL rather than the current entry
   per Dave's suggestion.
   move the cleaning routine to caller.

 fs/xfs/xfs_log_recover.c |   33 +++++++++++++++++++++------------
 1 file changed, 21 insertions(+), 12 deletions(-)

Index: b/fs/xfs/xfs_log_recover.c
===================================================================
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -3635,11 +3635,11 @@ xlog_recover_process_data(
  */
 STATIC int
 xlog_recover_process_efi(
-	xfs_mount_t		*mp,
-	xfs_efi_log_item_t	*efip)
+	struct xfs_mount	*mp,
+	struct xfs_efi_log_item	*efip)
 {
-	xfs_efd_log_item_t	*efdp;
-	xfs_trans_t		*tp;
+	struct xfs_efd_log_item	*efdp;
+	struct xfs_trans	*tp;
 	int			i;
 	int			error = 0;
 	xfs_extent_t		*extp;
@@ -3660,12 +3660,7 @@ 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);
+			/* The caller will free all efi entries on error. */
 			return XFS_ERROR(EIO);
 		}
 	}
@@ -3691,6 +3686,7 @@ xlog_recover_process_efi(
 
 abort_error:
 	xfs_trans_cancel(tp, XFS_TRANS_ABORT);
+	/* The caller will free all efi entries on error. */
 	return error;
 }
 
@@ -3716,8 +3712,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;
@@ -3756,7 +3752,20 @@ xlog_recover_process_efis(
 	}
 out:
 	xfs_trans_ail_cursor_done(ailp, &cur);
+	lip = xfs_ail_min(ailp);
 	spin_unlock(&ailp->xa_lock);
+	/* Free all the EFI from the AIL upon error */
+	while (lip) {
+		if (lip->li_type == XFS_LI_EFI) {
+			efip = (xfs_efi_log_item_t *)lip;
+			if (!test_bit(XFS_EFI_RECOVERED, &efip->efi_flags))
+				set_bit(XFS_EFI_RECOVERED, &efip->efi_flags);
+			xfs_efi_release(efip, efip->efi_format.efi_nextents);
+		}
+		spin_lock(&ailp->xa_lock);
+		lip = xfs_ail_min(ailp);
+		spin_unlock(&ailp->xa_lock);
+	}
 	return error;
 }
 


_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH v3] xfs: free the efi AIL entry on log recovery failure

[Mark Tinguely]

[-- Attachment #1: v3-xfs-remove-efi-entry-before-log-unmount.patch --]
[-- Type: text/plain, Size: 2892 bytes --]

If an extent free fails during recovery, the filesystem will be
forced down. The efi entry is still on the AIL and the log
shutdown function xfs_ail_push_all_sync() will hang.

This patch is similar to the patches that removed the dquot and
inode in commits 32ce90a and dea9609 but removes all the EFI
entries from the AIL.

Signed-off-by: Mark Tinguely <tinguely@sgi.com>
---
v3 (Augh - where is my head?) only remove efi items on error.
v2 remove all the EFIs from the AIL rather than the current entry
   per Dave's suggestion.
   move the cleaning routine to caller.

 fs/xfs/xfs_log_recover.c |   36 ++++++++++++++++++++++++------------
 1 file changed, 24 insertions(+), 12 deletions(-)

Index: b/fs/xfs/xfs_log_recover.c
===================================================================
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -3635,11 +3635,11 @@ xlog_recover_process_data(
  */
 STATIC int
 xlog_recover_process_efi(
-	xfs_mount_t		*mp,
-	xfs_efi_log_item_t	*efip)
+	struct xfs_mount	*mp,
+	struct xfs_efi_log_item	*efip)
 {
-	xfs_efd_log_item_t	*efdp;
-	xfs_trans_t		*tp;
+	struct xfs_efd_log_item	*efdp;
+	struct xfs_trans	*tp;
 	int			i;
 	int			error = 0;
 	xfs_extent_t		*extp;
@@ -3660,12 +3660,7 @@ 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);
+			/* The caller will free all efi entries on error. */
 			return XFS_ERROR(EIO);
 		}
 	}
@@ -3691,6 +3686,7 @@ xlog_recover_process_efi(
 
 abort_error:
 	xfs_trans_cancel(tp, XFS_TRANS_ABORT);
+	/* The caller will free all efi entries on error. */
 	return error;
 }
 
@@ -3716,8 +3712,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;
@@ -3756,7 +3752,23 @@ xlog_recover_process_efis(
 	}
 out:
 	xfs_trans_ail_cursor_done(ailp, &cur);
+	lip = xfs_ail_min(ailp);
 	spin_unlock(&ailp->xa_lock);
+	if (!error)
+		return 0;
+
+	/* Free all the EFI from the AIL upon error */
+	while (lip) {
+		if (lip->li_type == XFS_LI_EFI) {
+			efip = (xfs_efi_log_item_t *)lip;
+			if (!test_bit(XFS_EFI_RECOVERED, &efip->efi_flags))
+				set_bit(XFS_EFI_RECOVERED, &efip->efi_flags);
+			xfs_efi_release(efip, efip->efi_format.efi_nextents);
+		}
+		spin_lock(&ailp->xa_lock);
+		lip = xfs_ail_min(ailp);
+		spin_unlock(&ailp->xa_lock);
+	}
 	return error;
 }
 


_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH v3] xfs: free the efi AIL entry on log recovery failure

[Dave Chinner]

On Sat, Dec 07, 2013 at 06:52:12PM -0600, Mark Tinguely wrote:
> If an extent free fails during recovery, the filesystem will be
> forced down. The efi entry is still on the AIL and the log
> shutdown function xfs_ail_push_all_sync() will hang.
> 
> This patch is similar to the patches that removed the dquot and
> inode in commits 32ce90a and dea9609 but removes all the EFI
> entries from the AIL.
> 
> Signed-off-by: Mark Tinguely <tinguely@sgi.com>
> ---
> v3 (Augh - where is my head?) only remove efi items on error.
> v2 remove all the EFIs from the AIL rather than the current entry
>    per Dave's suggestion.
>    move the cleaning routine to caller.
> 
>  fs/xfs/xfs_log_recover.c |   36 ++++++++++++++++++++++++------------
>  1 file changed, 24 insertions(+), 12 deletions(-)
> 
> Index: b/fs/xfs/xfs_log_recover.c
> ===================================================================
> --- a/fs/xfs/xfs_log_recover.c
> +++ b/fs/xfs/xfs_log_recover.c
> @@ -3635,11 +3635,11 @@ xlog_recover_process_data(
>   */
>  STATIC int
>  xlog_recover_process_efi(
> -	xfs_mount_t		*mp,
> -	xfs_efi_log_item_t	*efip)
> +	struct xfs_mount	*mp,
> +	struct xfs_efi_log_item	*efip)
>  {
> -	xfs_efd_log_item_t	*efdp;
> -	xfs_trans_t		*tp;
> +	struct xfs_efd_log_item	*efdp;
> +	struct xfs_trans	*tp;
>  	int			i;
>  	int			error = 0;
>  	xfs_extent_t		*extp;
> @@ -3660,12 +3660,7 @@ 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);
> +			/* The caller will free all efi entries on error. */
>  			return XFS_ERROR(EIO);
>  		}
>  	}
> @@ -3691,6 +3686,7 @@ xlog_recover_process_efi(
>  
>  abort_error:
>  	xfs_trans_cancel(tp, XFS_TRANS_ABORT);
> +	/* The caller will free all efi entries on error. */
>  	return error;
>  }

That sort of comment belongs in the function header, not there.

Also, like I said previously, XFS_EFI_RECOVERED should be set
unconditionally at the start of the function and the error handling
should always release it, so that the state of the EFI on leaving
this function is always consistent. This is especially important if
we have an EFD pointing at the EFI - right now we can leave the
function on error with an EFD pointing at the EFI, but the EFI may
or may not ahve the XFS_EFI_RECOVERED bit set. i.e. consider that
if xfs_trans_commit() fails, it's the same case as calling
xfs_trans_cancel(XFS_TRANS_ABORT).


> @@ -3716,8 +3712,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;
> @@ -3756,7 +3752,23 @@ xlog_recover_process_efis(
>  	}
>  out:
>  	xfs_trans_ail_cursor_done(ailp, &cur);
> +	lip = xfs_ail_min(ailp);
>  	spin_unlock(&ailp->xa_lock);
> +	if (!error)
> +		return 0;
> +
> +	/* Free all the EFI from the AIL upon error */
> +	while (lip) {
> +		if (lip->li_type == XFS_LI_EFI) {
> +			efip = (xfs_efi_log_item_t *)lip;
> +			if (!test_bit(XFS_EFI_RECOVERED, &efip->efi_flags))
> +				set_bit(XFS_EFI_RECOVERED, &efip->efi_flags);
> +			xfs_efi_release(efip, efip->efi_format.efi_nextents);
> +		}
> +		spin_lock(&ailp->xa_lock);
> +		lip = xfs_ail_min(ailp);
> +		spin_unlock(&ailp->xa_lock);
> +	}
>  	return error;

It's never valid to walk the AIL without a cursor. Especially here,
where we could be racing with log IO completion doing shutdown
processing and modifying the AIL.

What's to stop log IO completion from freeing the log item at the
tail of the log just after we drop the AIL lock here?

What happens if the tail of the log is not an EFI? We just spin on
it and then, potentially, access it after it's been removed from the
AIL and freed....

IOWs, the only safe entries for us to touch here are EFIs and we
have to safely traverse the AIL because there are items other than
EFIs in the AIL and they may be concurrently removed by the log.
Only a cursor-based traversal makes that AIL traversal safe as it
detects list perturbations while the AIL has been dropped and
prevents us from accessing objects that we shouldn't be touching.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH v3] xfs: free the efi AIL entry on log recovery failure

[Christoph Hellwig]

Btw, I really think we need to get rid of abusing the AIL for
tracking EFIs during log recovery before looking at the various
leaks in this area.  I've actually got a lightly tested patch for
that, but didn't make much progress on that series.  If you have
and interest in that area and some spare QA cycles I'd recommend
to base this on the patch below:

---
>From b90935eaba9eb13c67101e5d723513bc6ca6e722 Mon Sep 17 00:00:00 2001
From: Christoph Hellwig <hch@lst.de>
Date: Sat, 23 Nov 2013 20:11:09 +0100
Subject: [PATCH] xfs: simplify EFI/EFD recovery

Use a cancellation table, similar to how we handle buffers instead of
abusing the AIL during recovery.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_extfree_item.c |    8 +-
 fs/xfs/xfs_extfree_item.h |    6 --
 fs/xfs/xfs_log_priv.h     |    1 +
 fs/xfs/xfs_log_recover.c  |  183 +++++++++++++--------------------------------
 4 files changed, 56 insertions(+), 142 deletions(-)

diff --git a/fs/xfs/xfs_extfree_item.c b/fs/xfs/xfs_extfree_item.c
index fb7a4c1..cc5e9fd 100644
--- a/fs/xfs/xfs_extfree_item.c
+++ b/fs/xfs/xfs_extfree_item.c
@@ -312,14 +312,8 @@ xfs_efi_release(xfs_efi_log_item_t	*efip,
 		uint			nextents)
 {
 	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);
-
+	if (atomic_sub_and_test(nextents, &efip->efi_next_extent))
 		__xfs_efi_release(efip);
-		/* efip may now have been freed, do not reference it again. */
-	}
 }
 
 static inline struct xfs_efd_log_item *EFD_ITEM(struct xfs_log_item *lip)
diff --git a/fs/xfs/xfs_extfree_item.h b/fs/xfs/xfs_extfree_item.h
index 0ffbce3..6a70bde 100644
--- 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.
@@ -47,7 +42,6 @@ typedef struct xfs_efi_log_item {
 	xfs_log_item_t		efi_item;
 	atomic_t		efi_refcount;
 	atomic_t		efi_next_extent;
-	unsigned long		efi_flags;	/* misc flags */
 	xfs_efi_log_format_t	efi_format;
 } xfs_efi_log_item_t;
 
diff --git a/fs/xfs/xfs_log_priv.h b/fs/xfs/xfs_log_priv.h
index 9bc403a..0df5ec3 100644
--- a/fs/xfs/xfs_log_priv.h
+++ b/fs/xfs/xfs_log_priv.h
@@ -368,6 +368,7 @@ struct xlog {
 	uint			l_flags;
 	uint			l_quotaoffs_flag; /* XFS_DQ_*, for QUOTAOFFs */
 	struct list_head	*l_buf_cancel_table;
+	struct list_head	l_efi_cancel;
 	int			l_iclog_hsize;  /* size of iclog header */
 	int			l_iclog_heads;  /* # of iclog header sectors */
 	uint			l_sectBBsize;   /* sector size in BBs (2^n) */
diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index b6b669d..1a83739 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -75,6 +75,12 @@ struct xfs_buf_cancel {
 	struct list_head	bc_list;
 };
 
+struct xfs_efi_cancel {
+	struct xfs_efi_log_item *ec_efip;
+	xfs_lsn_t		ec_lsn;
+	struct list_head	ec_list;
+};
+
 /*
  * Sector aligned buffer routines for buffer create/read/write/access
  */
@@ -3059,82 +3065,50 @@ xlog_recover_efi_pass2(
 	struct xlog_recover_item	*item,
 	xfs_lsn_t			lsn)
 {
-	int			error;
-	xfs_mount_t		*mp = log->l_mp;
-	xfs_efi_log_item_t	*efip;
-	xfs_efi_log_format_t	*efi_formatp;
+	struct xfs_efi_cancel		*ecp;
+	int				error;
+	xfs_mount_t			*mp = log->l_mp;
+	xfs_efi_log_format_t		*efi_formatp;
 
 	efi_formatp = item->ri_buf[0].i_addr;
 
-	efip = xfs_efi_init(mp, efi_formatp->efi_nextents);
-	if ((error = xfs_efi_copy_format(&(item->ri_buf[0]),
-					 &(efip->efi_format)))) {
-		xfs_efi_item_free(efip);
+	ecp = kmem_alloc(sizeof(struct xfs_efi_cancel), KM_SLEEP);
+	ecp->ec_lsn = lsn;
+	ecp->ec_efip = xfs_efi_init(mp, efi_formatp->efi_nextents);
+
+	error = xfs_efi_copy_format(&item->ri_buf[0],
+				    &ecp->ec_efip->efi_format);
+	if (error) {
+		xfs_efi_item_free(ecp->ec_efip);
 		return error;
 	}
-	atomic_set(&efip->efi_next_extent, efi_formatp->efi_nextents);
-
-	spin_lock(&log->l_ailp->xa_lock);
-	/*
-	 * xfs_trans_ail_update() drops the AIL lock.
-	 */
-	xfs_trans_ail_update(log->l_ailp, &efip->efi_item, lsn);
+	atomic_set(&ecp->ec_efip->efi_next_extent, efi_formatp->efi_nextents);
+	list_add(&ecp->ec_list, &log->l_efi_cancel);
 	return 0;
 }
 
-
-/*
- * This routine is called when an efd format structure is found in
- * a committed transaction in the log.  It's purpose is to cancel
- * the corresponding efi if it was still in the log.  To do this
- * it searches the AIL for the efi with an id equal to that in the
- * efd format structure.  If we find it, we remove the efi from the
- * AIL and free it.
- */
 STATIC int
 xlog_recover_efd_pass2(
 	struct xlog			*log,
 	struct xlog_recover_item	*item)
 {
-	xfs_efd_log_format_t	*efd_formatp;
-	xfs_efi_log_item_t	*efip = NULL;
-	xfs_log_item_t		*lip;
-	__uint64_t		efi_id;
-	struct xfs_ail_cursor	cur;
-	struct xfs_ail		*ailp = log->l_ailp;
-
-	efd_formatp = item->ri_buf[0].i_addr;
+	struct xfs_efd_log_format	*efd_formatp = item->ri_buf[0].i_addr;
+	__uint64_t			efi_id = efd_formatp->efd_efi_id;
+	struct xfs_efi_cancel		*ecp;
+
 	ASSERT((item->ri_buf[0].i_len == (sizeof(xfs_efd_log_format_32_t) +
 		((efd_formatp->efd_nextents - 1) * sizeof(xfs_extent_32_t)))) ||
 	       (item->ri_buf[0].i_len == (sizeof(xfs_efd_log_format_64_t) +
 		((efd_formatp->efd_nextents - 1) * sizeof(xfs_extent_64_t)))));
-	efi_id = efd_formatp->efd_efi_id;
 
-	/*
-	 * Search for the efi with the id in the efd format structure
-	 * in the AIL.
-	 */
-	spin_lock(&ailp->xa_lock);
-	lip = xfs_trans_ail_cursor_first(ailp, &cur, 0);
-	while (lip != NULL) {
-		if (lip->li_type == XFS_LI_EFI) {
-			efip = (xfs_efi_log_item_t *)lip;
-			if (efip->efi_format.efi_id == efi_id) {
-				/*
-				 * xfs_trans_ail_delete() drops the
-				 * AIL lock.
-				 */
-				xfs_trans_ail_delete(ailp, lip,
-						     SHUTDOWN_CORRUPT_INCORE);
-				xfs_efi_item_free(efip);
-				spin_lock(&ailp->xa_lock);
-				break;
-			}
+	list_for_each_entry(ecp, &log->l_efi_cancel, ec_list) {
+		if (ecp->ec_efip->efi_format.efi_id == efi_id) {
+			list_del(&ecp->ec_list);
+			xfs_efi_item_free(ecp->ec_efip);
+			kmem_free(ecp);
+			break;
 		}
-		lip = xfs_trans_ail_cursor_next(ailp, &cur);
 	}
-	xfs_trans_ail_cursor_done(ailp, &cur);
-	spin_unlock(&ailp->xa_lock);
 
 	return 0;
 }
@@ -3618,27 +3592,26 @@ 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.
+ * Process an extent free intent item that was recovered from the log.
+ * We need to free the extents that it describes.
  */
 STATIC int
 xlog_recover_process_efi(
-	xfs_mount_t		*mp,
-	xfs_efi_log_item_t	*efip)
+	struct xlog		*log,
+	struct xfs_efi_cancel	*ecp)
 {
-	xfs_efd_log_item_t	*efdp;
-	xfs_trans_t		*tp;
-	int			i;
-	int			error = 0;
-	xfs_extent_t		*extp;
+	struct xfs_mount	*mp = log->l_mp;
+	struct xfs_efi_log_item	*efip = ecp->ec_efip;
+	struct xfs_efd_log_item	*efdp;
+	struct xfs_trans	*tp;
+	struct xfs_extent	*extp;
 	xfs_fsblock_t		startblock_fsb;
-
-	ASSERT(!test_bit(XFS_EFI_RECOVERED, &efip->efi_flags));
+	int			error = 0;
+	int			i;
 
 	/*
-	 * 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.
+	 * 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]);
@@ -3652,12 +3625,14 @@ xlog_recover_process_efi(
 			 * 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);
 		}
 	}
 
+	spin_lock(&log->l_ailp->xa_lock);
+	xfs_trans_ail_update(log->l_ailp, &efip->efi_item, ecp->ec_lsn);
+
 	tp = xfs_trans_alloc(mp, 0);
 	error = xfs_trans_reserve(tp, &M_RES(mp)->tr_itruncate, 0, 0);
 	if (error)
@@ -3673,78 +3648,27 @@ xlog_recover_process_efi(
 					 extp->ext_len);
 	}
 
-	set_bit(XFS_EFI_RECOVERED, &efip->efi_flags);
-	error = xfs_trans_commit(tp, 0);
-	return error;
+	return xfs_trans_commit(tp, 0);
 
 abort_error:
 	xfs_trans_cancel(tp, XFS_TRANS_ABORT);
 	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.
- *
- * 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.
- */
 STATIC int
 xlog_recover_process_efis(
 	struct xlog	*log)
 {
-	xfs_log_item_t		*lip;
-	xfs_efi_log_item_t	*efip;
+	struct xfs_efi_cancel	*ecp, *next;
 	int			error = 0;
-	struct xfs_ail_cursor	cur;
-	struct xfs_ail		*ailp;
-
-	ailp = log->l_ailp;
-	spin_lock(&ailp->xa_lock);
-	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.
-		 */
-		if (lip->li_type != XFS_LI_EFI) {
-#ifdef DEBUG
-			for (; lip; lip = xfs_trans_ail_cursor_next(ailp, &cur))
-				ASSERT(lip->li_type != XFS_LI_EFI);
-#endif
-			break;
-		}
 
-		/*
-		 * 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);
-		spin_lock(&ailp->xa_lock);
+	list_for_each_entry_safe(ecp, next, &log->l_efi_cancel, ec_list) {
+		list_del(&ecp->ec_list);
+		error = xlog_recover_process_efi(log, ecp);
 		if (error)
-			goto out;
-		lip = xfs_trans_ail_cursor_next(ailp, &cur);
+			break; /* XXX: will leak remaining EFIs.. */
 	}
-out:
-	xfs_trans_ail_cursor_done(ailp, &cur);
-	spin_unlock(&ailp->xa_lock);
+
 	return error;
 }
 
@@ -4320,6 +4244,7 @@ xlog_do_log_recovery(
 						 KM_SLEEP);
 	for (i = 0; i < XLOG_BC_TABLE_SIZE; i++)
 		INIT_LIST_HEAD(&log->l_buf_cancel_table[i]);
+	INIT_LIST_HEAD(&log->l_efi_cancel);
 
 	error = xlog_do_recovery_pass(log, head_blk, tail_blk,
 				      XLOG_RECOVER_PASS1);
-- 
1.7.10.4

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH v3] xfs: free the efi AIL entry on log recovery failure

[Mark Tinguely]

On 12/11/13 05:31, Christoph Hellwig wrote:
> Btw, I really think we need to get rid of abusing the AIL for
> tracking EFIs during log recovery before looking at the various
> leaks in this area.  I've actually got a lightly tested patch for
> that, but didn't make much progress on that series.  If you have
> and interest in that area and some spare QA cycles I'd recommend
> to base this on the patch below:
>
> ---
>> From b90935eaba9eb13c67101e5d723513bc6ca6e722 Mon Sep 17 00:00:00 2001
> From: Christoph Hellwig<hch@lst.de>
> Date: Sat, 23 Nov 2013 20:11:09 +0100
> Subject: [PATCH] xfs: simplify EFI/EFD recovery
>
> Use a cancellation table, similar to how we handle buffers instead of
> abusing the AIL during recovery.
>
> Signed-off-by: Christoph Hellwig<hch@lst.de>
> ---
>   fs/xfs/xfs_extfree_item.c |    8 +-
>   fs/xfs/xfs_extfree_item.h |    6 --
>   fs/xfs/xfs_log_priv.h     |    1 +
>   fs/xfs/xfs_log_recover.c  |  183 +++++++++++++--------------------------------
>   4 files changed, 56 insertions(+), 142 deletions(-)


Good idea to remove EFI item recovery from the AIL. It may take some 
time before I get to it.

--Mark.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH v3] xfs: free the efi AIL entry on log recovery failure

[Mark Tinguely]

Sorry it took me so long to get to this.

I like the idea to keep the majority of EFI items off the AIL, but it 
does not simply the EFI recovery processing. xfs_recover_process_efi() 
will place the EFI entry on the AIL and recreate the hanging on unmount 
problem when there is an error in the extent free or transaction commit.

Since the patch removes the XFS_EFI_RECOVERED flag/code, it is harder to 
remove this entry. If you leave the XFS_EFI_RECOVERED flag/code in XFS, 
the patch to avoid a hang in xfs_ail_push_all_sync() will be the same as 
before.

As you commented, it does leak some memory, which can be easily fixed...

On 12/11/13 05:31, Christoph Hellwig wrote

> Use a cancellation table, similar to how we handle buffers instead of
> abusing the AIL during recovery.
>
> Signed-off-by: Christoph Hellwig<hch@lst.de>
> ---

...

> @@ -3059,82 +3065,50 @@ xlog_recover_efi_pass2(
>   	struct xlog_recover_item	*item,
>   	xfs_lsn_t			lsn)
>   {
> -	int			error;
> -	xfs_mount_t		*mp = log->l_mp;
> -	xfs_efi_log_item_t	*efip;
> -	xfs_efi_log_format_t	*efi_formatp;
> +	struct xfs_efi_cancel		*ecp;
> +	int				error;
> +	xfs_mount_t			*mp = log->l_mp;
> +	xfs_efi_log_format_t		*efi_formatp;

typedef got leaked back in.

...

> @@ -3618,27 +3592,26 @@ 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.
> + * Process an extent free intent item that was recovered from the log.
> + * We need to free the extents that it describes.
>    */
>   STATIC int
>   xlog_recover_process_efi(
> -	xfs_mount_t		*mp,
> -	xfs_efi_log_item_t	*efip)
> +	struct xlog		*log,
> +	struct xfs_efi_cancel	*ecp)
>   {
> -	xfs_efd_log_item_t	*efdp;
> -	xfs_trans_t		*tp;
> -	int			i;
> -	int			error = 0;
> -	xfs_extent_t		*extp;
> +	struct xfs_mount	*mp = log->l_mp;
> +	struct xfs_efi_log_item	*efip = ecp->ec_efip;
> +	struct xfs_efd_log_item	*efdp;
> +	struct xfs_trans	*tp;
> +	struct xfs_extent	*extp;
>   	xfs_fsblock_t		startblock_fsb;
> -
> -	ASSERT(!test_bit(XFS_EFI_RECOVERED,&efip->efi_flags));
> +	int			error = 0;
> +	int			i;
>
>   	/*
> -	 * 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.
> +	 * 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]);
> @@ -3652,12 +3625,14 @@ xlog_recover_process_efi(
>   			 * 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);
>   		}
>   	}

Soo at this point we are dealing with the EFIs that do not have matching 
EFD. The EFI are in the new l_efi_cancel and not on the AIL. I am 
correct in thinking that there is no reason to do a xfs_efi_release() above?
>
> +	spin_lock(&log->l_ailp->xa_lock);
> +	xfs_trans_ail_update(log->l_ailp,&efip->efi_item, ecp->ec_lsn);

we now added the EFI to the AIL. If anything fails in the 
xfs_free_extent()/xfs_trans_commit then EFI has to be removed which is 
now trickier without the XFS_EFI_RECOVERED flag.

I suppose we could:

  1) leave the XFS_EFI_RECOVERED counter/code.
  2) manually decrease the counter. *
  3) manually remove the entry from the AIL. *
* terrible hack that everyone will NACK.

> - */
>   STATIC int
>   xlog_recover_process_efis(
>   	struct xlog	*log)
>   {
...

> -		spin_lock(&ailp->xa_lock);
> +	list_for_each_entry_safe(ecp, next,&log->l_efi_cancel, ec_list) {
> +		list_del(&ecp->ec_list);
> +		error = xlog_recover_process_efi(log, ecp);
>   		if (error)
> -			goto out;
> -		lip = xfs_trans_ail_cursor_next(ailp,&cur);
> +			break; /* XXX: will leak remaining EFIs.. */

...rather than leaking the rest of the efi cancel entries:

		/* comment here about removing entries on error */
		if (!error)
			error = xlog_recover_process_efi(log, ecp);

Again, sorry for the delay. I will wait to see what you want to do with 
this patch before resubmitting the patch to free EFI from the AIL.

--Mark.



_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs