[PATCH 0/5] Misc controversial patches

[PATCH 0/5] Misc controversial patches

[Mark Tinguely]

Here is a repost of these patches.

Patch one and two deal with EFIs getting stuck on the AIL and hanging
unmount. First deals with log recovery the other xfs_bmap_finish and
log write errors:

	xfs-fix-efi-in-log-recovery-error.patch
	xfs-fix-efi-on-filesystem-errors.patch

Patch three and four deal with resource leaks in failed log recoveries.
The first is a rewrite of Dave's rewrite of my patch with ideas from
Christoph. The second is taking care of leaked inode pointers:

	xfs-fix-log-recovery-leaks.patch
	xfs-reclaim-inodes-on-recovery-fail.patch 

Lastly is the CIL sequence error. The setting of ctx->commit_lsn
in xlog_cil_init_post_recovery allows smaller cil push sequences finish
out of order:

	xfs-fix-cil-push-seq-after-recovery.patch

--Mark.

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

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

[Mark Tinguely]

[-- Attachment #1: xfs-fix-efi-in-log-recovery-error.patch --]
[-- Type: text/plain, Size: 10535 bytes --]

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>
---
 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);
 }
 
 /*
@@ -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;
 		}
 	}
 
@@ -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.
+ 			*/
+			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

[PATCH 2/5] xfs: free the EFI entries from AIL on forced shutdown

[Mark Tinguely]

[-- Attachment #1: xfs-fix-efi-on-filesystem-errors.patch --]
[-- Type: text/plain, Size: 4304 bytes --]

The extent free intention (EFI) entry and extent free done (EFD)
log items are in separate transactions. It is possible that
the EFI can be pushed to the AIL before a forced shutdown
where it gets stuck on the AIL and requires a reboot in the
following places:

1) xfs_bmap_finish:
  The EFD is not in the transaction. This can happen if the
  transaction in xfs_bmap_finish() fails to reserve space.

  The EFD will not be dirty in the transaction if the error
  happened while freeing the first extent in the list.

2) EFD IOP Abort processing:
  If xfs_trans_cancel() is called with an abort flag, or if the
  xfs_trans_commit() is called when the file system is in forced
  shutdown or if the log buffer write fails.

 
Do not free the EFI structure immediately on forced shutdowns, but
instead use the IOP calls to match the EFI/EFD entries. A small
wrinkle in the mix is the EFI is not automatically placed on the
AIL by the IOP routines in the cases but may have made it to the
AIL before the abort. We now have to check if the EFI is on the AIL
in the abort IOP cases.

Remove the debug code, because the EFD could be in the log item list
on a xfs_trans_cancel.

Signed-off-by: Mark Tinguely <tinguely@sgi.com>
---
 fs/xfs/xfs_bmap_util.c |   36 ++++++++++++++++++------------------
 fs/xfs/xfs_trans.c     |    8 --------
 2 files changed, 18 insertions(+), 26 deletions(-)

Index: b/fs/xfs/xfs_bmap_util.c
===================================================================
--- a/fs/xfs/xfs_bmap_util.c
+++ b/fs/xfs/xfs_bmap_util.c
@@ -79,7 +79,6 @@ xfs_bmap_finish(
 	int			error;		/* error return value */
 	xfs_bmap_free_item_t	*free;		/* free extent item */
 	struct xfs_trans_res	tres;		/* new log reservation */
-	xfs_mount_t		*mp;		/* filesystem mount structure */
 	xfs_bmap_free_item_t	*next;		/* next item on free list */
 	xfs_trans_t		*ntp;		/* new transaction pointer */
 
@@ -115,31 +114,32 @@ xfs_bmap_finish(
 	xfs_log_ticket_put(ntp->t_ticket);
 
 	error = xfs_trans_reserve(ntp, &tres, 0, 0);
-	if (error)
+	if (error) {
+		/*
+		 * EFD is not in the transaction. Manually release the EFI
+		 * and remove it from AIL if necessary.
+		 */
+		xfs_efi_release(efi, efi->efi_format.efi_nextents);
 		return error;
+	}
+
 	efd = xfs_trans_get_efd(ntp, efi, flist->xbf_count);
 	for (free = flist->xbf_first; free != NULL; free = next) {
 		next = free->xbfi_next;
-		if ((error = xfs_free_extent(ntp, free->xbfi_startblock,
-				free->xbfi_blockcount))) {
+		error = xfs_free_extent(ntp, free->xbfi_startblock,
+				free->xbfi_blockcount);
+		xfs_trans_log_efd_extent(ntp, efd, free->xbfi_startblock,
+					 free->xbfi_blockcount);
+		if (error) {
 			/*
-			 * The bmap free list will be cleaned up at a
-			 * higher level.  The EFI will be canceled when
-			 * this transaction is aborted.
-			 * Need to force shutdown here to make sure it
-			 * happens, since this transaction may not be
-			 * dirty yet.
+			 * Encountered an error while freeing the extent.
+			 * The bmap free list will be cleaned up at a higher
+			 * level. The caller will call xfs_trans_cancel on
+			 * error, the filesystem will be shutdown and the
+			 * EFI and EFD will be freed.
 			 */
-			mp = ntp->t_mountp;
-			if (!XFS_FORCED_SHUTDOWN(mp))
-				xfs_force_shutdown(mp,
-						   (error == EFSCORRUPTED) ?
-						   SHUTDOWN_CORRUPT_INCORE :
-						   SHUTDOWN_META_IO_ERROR);
 			return error;
 		}
-		xfs_trans_log_efd_extent(ntp, efd, free->xbfi_startblock,
-			free->xbfi_blockcount);
 		xfs_bmap_del_free(flist, NULL, free);
 	}
 	return 0;
Index: b/fs/xfs/xfs_trans.c
===================================================================
--- a/fs/xfs/xfs_trans.c
+++ b/fs/xfs/xfs_trans.c
@@ -958,14 +958,6 @@ xfs_trans_cancel(
 		XFS_ERROR_REPORT("xfs_trans_cancel", XFS_ERRLEVEL_LOW, mp);
 		xfs_force_shutdown(mp, SHUTDOWN_CORRUPT_INCORE);
 	}
-#ifdef DEBUG
-	if (!(flags & XFS_TRANS_ABORT) && !XFS_FORCED_SHUTDOWN(mp)) {
-		struct xfs_log_item_desc *lidp;
-
-		list_for_each_entry(lidp, &tp->t_items, lid_trans)
-			ASSERT(!(lidp->lid_item->li_type == XFS_LI_EFD));
-	}
-#endif
 	xfs_trans_unreserve_and_mod_sb(tp);
 	xfs_trans_unreserve_and_mod_dquots(tp);
 


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

[PATCH 3/5] xfs: free the list of recovery items on error

[Mark Tinguely]

[-- Attachment #1: xfs-fix-log-recovery-leaks.patch --]
[-- Type: text/plain, Size: 7161 bytes --]

Recovery builds a list of items on the transaction's
r_itemq head. Normally these items are committed and freed.
But in the event of a recovery error, these allocations
are leaked.

If the error occurs during item reordering, then reconstruct
the r_itemq list before deleting the list to avoid leaking
the entries that were on one of the temporary lists.

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

History:
 My first version corrected the xlog_recover_free_trans for
 the error path of xlog_recover_commit_trans.

 Dave Chinner removed most of the XFS_ERROR(), changed messages
 in xlog_recover_process_data and pushed the xlog_recover_free_trans
 calls into the lower layers.

 This has all those patches plus suggestions from Christoph Hellwig.

Signed-off-by: Mark Tinguely <tinguely@sgi.com>
Signed-off-by: Dave Chinner <dchinner@redhat.com>
Signed-off-by: Mark Tinguely <tinguely@sgi.com>
---
 fs/xfs/xfs_log_recover.c |   88 ++++++++++++++++++++++++-----------------------
 1 file changed, 45 insertions(+), 43 deletions(-)

Index: b/fs/xfs/xfs_log_recover.c
===================================================================
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -539,7 +539,7 @@ xlog_find_verify_log_record(
 			xfs_warn(log->l_mp,
 		"Log inconsistent (didn't find previous header)");
 			ASSERT(0);
-			error = XFS_ERROR(EIO);
+			error = EIO;
 			goto out;
 		}
 
@@ -961,7 +961,7 @@ xlog_find_tail(
 		xfs_warn(log->l_mp, "%s: couldn't find sync record", __func__);
 		xlog_put_bp(bp);
 		ASSERT(0);
-		return XFS_ERROR(EIO);
+		return EIO;
 	}
 
 	/* find blk_no of tail of log */
@@ -1551,7 +1551,7 @@ xlog_recover_add_to_trans(
 			xfs_warn(log->l_mp, "%s: bad header magic number",
 				__func__);
 			ASSERT(0);
-			return XFS_ERROR(EIO);
+			return EIO;
 		}
 		if (len == sizeof(xfs_trans_header_t))
 			xlog_recover_add_item(&trans->r_itemq);
@@ -1581,7 +1581,7 @@ xlog_recover_add_to_trans(
 				  in_f->ilf_size);
 			ASSERT(0);
 			kmem_free(ptr);
-			return XFS_ERROR(EIO);
+			return EIO;
 		}
 
 		item->ri_total = in_f->ilf_size;
@@ -1702,7 +1702,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;
 		}
 	}
@@ -3395,7 +3395,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;
 	}
 }
 
@@ -3431,7 +3431,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;
 	}
 }
 
@@ -3478,7 +3478,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)
@@ -3503,13 +3503,14 @@ xlog_recover_commit_trans(
 			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,
@@ -3520,21 +3521,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);
 	return error ? error : error2;
 }
 
-STATIC int
-xlog_recover_unmount_trans(
-	struct xlog		*log)
-{
-	/* 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
@@ -3555,9 +3545,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;
 
@@ -3566,7 +3556,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);
@@ -3574,10 +3564,12 @@ 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 bad transaction opheader clientid 0x%x",
 					__func__, ohead->oh_clientid);
 			ASSERT(0);
-			return (XFS_ERROR(EIO));
+			error = EIO;
+			goto out_error;
 		}
 		tid = be32_to_cpu(ohead->oh_tid);
 		hash = XLOG_RHASH(tid);
@@ -3588,10 +3580,12 @@ 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 bad transaction opheader length 0x%x",
 					__func__, be32_to_cpu(ohead->oh_len));
 				WARN_ON(1);
-				return (XFS_ERROR(EIO));
+				error = XFS_ERROR(EIO);
+				goto out_error;
 			}
 			flags = ohead->oh_flags & ~XLOG_END_TRANS;
 			if (flags & XLOG_WAS_CONT_TRANS)
@@ -3600,42 +3594,50 @@ xlog_recover_process_data(
 			case XLOG_COMMIT_TRANS:
 				error = xlog_recover_commit_trans(log,
 								trans, pass);
-				break;
-			case XLOG_UNMOUNT_TRANS:
-				error = xlog_recover_unmount_trans(log);
+				if (error)
+					goto out_error;
+				/*
+				 * xlog_recover_commit_trans removed the trans
+				 * structure from the hash, so nobody else will
+				 * ever find this structure again. Hence we
+				 * must free it here.
+				 */
+				xlog_recover_free_trans(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 bad transaction opheader flag 0x%x",
 					__func__, flags);
 				ASSERT(0);
-				error = XFS_ERROR(EIO);
+				error = EIO;
 				break;
 			}
-			if (error) {
-				xlog_recover_free_trans(trans);
-				return error;
-			}
+			if (error)
+				goto out_error;
 		}
 		dp += be32_to_cpu(ohead->oh_len);
 		num_logops--;
 	}
 	return 0;
+
+ out_error:
+	if (trans)
+		xlog_recover_free_trans(trans);
+	return error;
 }
 
 /*


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

[PATCH 4/5] xfs: free inodes on log recovery error

[Mark Tinguely]

[-- Attachment #1: xfs-reclaim-inodes-on-recovery-fail.patch --]
[-- Type: text/plain, Size: 1455 bytes --]

Recovery may free inodes that end up on the inode
reclaim RCU. If recovery fails, we leak these inodes.
The filesystem should be in forced shutdown at this
point, so a call to xfs_reclaim_inode is a fast path
to freeing the inodes and RCU entries.

Signed-off-by: Mark Tinguely <tinguely@sgi.com>
---
 fs/xfs/xfs_log.c   |    2 ++
 fs/xfs/xfs_mount.c |    1 +
 2 files changed, 3 insertions(+)

Index: b/fs/xfs/xfs_log.c
===================================================================
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -31,6 +31,7 @@
 #include "xfs_log_priv.h"
 #include "xfs_log_recover.h"
 #include "xfs_inode.h"
+#include "xfs_icache.h"
 #include "xfs_trace.h"
 #include "xfs_fsops.h"
 #include "xfs_cksum.h"
@@ -720,6 +721,7 @@ xfs_log_mount(
 	return 0;
 
 out_destroy_ail:
+	xfs_reclaim_inodes(mp, SYNC_WAIT);
 	xfs_trans_ail_destroy(mp);
 out_free_log:
 	xlog_dealloc_log(mp->m_log);
Index: b/fs/xfs/xfs_mount.c
===================================================================
--- a/fs/xfs/xfs_mount.c
+++ b/fs/xfs/xfs_mount.c
@@ -980,6 +980,7 @@ xfs_mountfs(
  out_log_dealloc:
 	xfs_log_unmount(mp);
  out_fail_wait:
+	xfs_reclaim_inodes(mp, SYNC_WAIT);
 	if (mp->m_logdev_targp && mp->m_logdev_targp != mp->m_ddev_targp)
 		xfs_wait_buftarg(mp->m_logdev_targp);
 	xfs_wait_buftarg(mp->m_ddev_targp);


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

[PATCH 5/5] xfs: fix cil push sequence after log recovery

[Mark Tinguely]

[-- Attachment #1: xfs-fix-cil-push-seq-after-recovery.patch --]
[-- Type: text/plain, Size: 1203 bytes --]

The CIL pushes are marked complete with transaction
tickets and should be in the the correct sequence order.
The back end of the cil push code uses the ctx->commit_lsn
to make sure all previous pushes are complete before adding
the commit ticket for the current cil push. Because
xlog_cil_init_post_recovery sets the ctx->commit_lsn,
the later pushes can incorrectly think that the first
sequence push is complete and allow out of order cil
completion records to be written to the log. If the
system crashes, the log will be replayed in the
wrong order.

Signed-off-by: Mark Tinguely <tinguely@sgi.com>
---
 fs/xfs/xfs_log_cil.c |    2 --
 1 file changed, 2 deletions(-)

Index: b/fs/xfs/xfs_log_cil.c
===================================================================
--- a/fs/xfs/xfs_log_cil.c
+++ b/fs/xfs/xfs_log_cil.c
@@ -78,8 +78,6 @@ xlog_cil_init_post_recovery(
 {
 	log->l_cilp->xc_ctx->ticket = xlog_cil_ticket_alloc(log);
 	log->l_cilp->xc_ctx->sequence = 1;
-	log->l_cilp->xc_ctx->commit_lsn = xlog_assign_lsn(log->l_curr_cycle,
-								log->l_curr_block);
 }
 
 /*


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

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

[Brian Foster]

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.

>  }
>  
>  /*
> @@ -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..?

>  		}
>  	}
>  
> @@ -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.

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

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

Re: [PATCH 4/5] xfs: free inodes on log recovery error

[Brian Foster]

On Wed, Jul 02, 2014 at 09:32:10AM -0500, Mark Tinguely wrote:
> Recovery may free inodes that end up on the inode
> reclaim RCU. If recovery fails, we leak these inodes.
> The filesystem should be in forced shutdown at this
> point, so a call to xfs_reclaim_inode is a fast path
> to freeing the inodes and RCU entries.
> 
> Signed-off-by: Mark Tinguely <tinguely@sgi.com>
> ---
>  fs/xfs/xfs_log.c   |    2 ++
>  fs/xfs/xfs_mount.c |    1 +
>  2 files changed, 3 insertions(+)
> 
> Index: b/fs/xfs/xfs_log.c
> ===================================================================
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -31,6 +31,7 @@
>  #include "xfs_log_priv.h"
>  #include "xfs_log_recover.h"
>  #include "xfs_inode.h"
> +#include "xfs_icache.h"
>  #include "xfs_trace.h"
>  #include "xfs_fsops.h"
>  #include "xfs_cksum.h"
> @@ -720,6 +721,7 @@ xfs_log_mount(
>  	return 0;
>  
>  out_destroy_ail:
> +	xfs_reclaim_inodes(mp, SYNC_WAIT);

So an inode in the perag cache means an xfs_iget(). I see that in
xlog_recover_process_one_iunlink(), which is via
xfs_log_mount_finish(). Assuming I'm following that correctly, why the
reclaim here (and in response to failure here by the caller) as opposed
to closer to a failure of xfs_log_mount_finish()?

Brian

>  	xfs_trans_ail_destroy(mp);
>  out_free_log:
>  	xlog_dealloc_log(mp->m_log);
> Index: b/fs/xfs/xfs_mount.c
> ===================================================================
> --- a/fs/xfs/xfs_mount.c
> +++ b/fs/xfs/xfs_mount.c
> @@ -980,6 +980,7 @@ xfs_mountfs(
>   out_log_dealloc:
>  	xfs_log_unmount(mp);
>   out_fail_wait:
> +	xfs_reclaim_inodes(mp, SYNC_WAIT);
>  	if (mp->m_logdev_targp && mp->m_logdev_targp != mp->m_ddev_targp)
>  		xfs_wait_buftarg(mp->m_logdev_targp);
>  	xfs_wait_buftarg(mp->m_ddev_targp);
> 
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs

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

Re: [PATCH 4/5] xfs: free inodes on log recovery error

[Mark Tinguely]

On 07/07/14 09:30, Brian Foster wrote:
> On Wed, Jul 02, 2014 at 09:32:10AM -0500, Mark Tinguely wrote:
>> Recovery may free inodes that end up on the inode
>> reclaim RCU. If recovery fails, we leak these inodes.
>> The filesystem should be in forced shutdown at this
>> point, so a call to xfs_reclaim_inode is a fast path
>> to freeing the inodes and RCU entries.
>>
>> Signed-off-by: Mark Tinguely<tinguely@sgi.com>
>> ---
>>   fs/xfs/xfs_log.c   |    2 ++
>>   fs/xfs/xfs_mount.c |    1 +
>>   2 files changed, 3 insertions(+)
>>
>> Index: b/fs/xfs/xfs_log.c
>> ===================================================================
>> --- a/fs/xfs/xfs_log.c
>> +++ b/fs/xfs/xfs_log.c
>> @@ -31,6 +31,7 @@
>>   #include "xfs_log_priv.h"
>>   #include "xfs_log_recover.h"
>>   #include "xfs_inode.h"
>> +#include "xfs_icache.h"
>>   #include "xfs_trace.h"
>>   #include "xfs_fsops.h"
>>   #include "xfs_cksum.h"
>> @@ -720,6 +721,7 @@ xfs_log_mount(
>>   	return 0;
>>
>>   out_destroy_ail:
>> +	xfs_reclaim_inodes(mp, SYNC_WAIT);
>
> So an inode in the perag cache means an xfs_iget(). I see that in
> xlog_recover_process_one_iunlink(), which is via
> xfs_log_mount_finish(). Assuming I'm following that correctly, why the
> reclaim here (and in response to failure here by the caller) as opposed
> to closer to a failure of xfs_log_mount_finish()?
>
> Brian
>
>>   	xfs_trans_ail_destroy(mp);
>>   out_free_log:
>>   	xlog_dealloc_log(mp->m_log);
>> Index: b/fs/xfs/xfs_mount.c
>> ===================================================================
>> --- a/fs/xfs/xfs_mount.c
>> +++ b/fs/xfs/xfs_mount.c
>> @@ -980,6 +980,7 @@ xfs_mountfs(
>>    out_log_dealloc:
>>   	xfs_log_unmount(mp);
>>    out_fail_wait:
>> +	xfs_reclaim_inodes(mp, SYNC_WAIT);
>>   	if (mp->m_logdev_targp&&  mp->m_logdev_targp != mp->m_ddev_targp)
>>   		xfs_wait_buftarg(mp->m_logdev_targp);
>>   	xfs_wait_buftarg(mp->m_ddev_targp);
>>
>>
>> _______________________________________________

You are right, the reclaim in xfs_log_mount() is now redundant.

The xfs_reclaim_inodes call location in xfs_mount error path had changed 
when I learned that the xfs_log_unmount() left inodes in the RCU. I did 
not notice that with the new location that they were the same.

--Mark.

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

Re: [PATCH 5/5] xfs: fix cil push sequence after log recovery

[Brian Foster]

On Wed, Jul 02, 2014 at 09:32:11AM -0500, Mark Tinguely wrote:
> The CIL pushes are marked complete with transaction
> tickets and should be in the the correct sequence order.
> The back end of the cil push code uses the ctx->commit_lsn
> to make sure all previous pushes are complete before adding
> the commit ticket for the current cil push. Because
> xlog_cil_init_post_recovery sets the ctx->commit_lsn,
> the later pushes can incorrectly think that the first
> sequence push is complete and allow out of order cil
> completion records to be written to the log. If the
> system crashes, the log will be replayed in the
> wrong order.
> 
> Signed-off-by: Mark Tinguely <tinguely@sgi.com>
> ---
>  fs/xfs/xfs_log_cil.c |    2 --
>  1 file changed, 2 deletions(-)
> 
> Index: b/fs/xfs/xfs_log_cil.c
> ===================================================================
> --- a/fs/xfs/xfs_log_cil.c
> +++ b/fs/xfs/xfs_log_cil.c
> @@ -78,8 +78,6 @@ xlog_cil_init_post_recovery(
>  {
>  	log->l_cilp->xc_ctx->ticket = xlog_cil_ticket_alloc(log);
>  	log->l_cilp->xc_ctx->sequence = 1;
> -	log->l_cilp->xc_ctx->commit_lsn = xlog_assign_lsn(log->l_curr_cycle,
> -								log->l_curr_block);

So we set ctx->commit_lsn here, this ctx is open for business and
sometime later it's committed. If a subsequent ctx is pushed before this
one has committed, commit_lsn is already set and thus the wait checks in
xlog_cil_push(), etc. are bypassed.

The fix seems logical to me, though I'm curious if there was some
original reason for setting commit_lsn here (it looks like this and the
xlog_wait() bits both go back to the original delayed logging commit).

It also seems that the dependence on l_curr_cycle and l_curr_block is
the only reason for the existence of this post-recovery function. Can we
move the ticket alloc and kill it if the commit_lsn assignment goes
away?

Brian

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

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

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

[Mark Tinguely]

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

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

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

[Dave Chinner]

On Mon, Jul 07, 2014 at 10:29:02AM -0500, Mark Tinguely wrote:
> On 07/07/14 09:30, Brian Foster wrote:
> >On Wed, Jul 02, 2014 at 09:32:07AM -0500, Mark Tinguely wrote:
> >>  		    (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?

You don't. Brian just asked you to rebase against it because of
merge conflicts against the current for-next tree he tried to apply
it to. It's obvious to me that you didn't read the discussions about
the plans for this dev cycle that went on a month ago:

"[DISCUSS] Planning for new dev cycle (3.17)"

http://oss.sgi.com/pipermail/xfs/2014-June/036739.html

Fact is, this major restructure has been discussed several times
over the past couple of months, both on the mailing list and on IRC,
and there's been plenty of warning and requests for comments about
it.

As to being asked to rebase the patch set on the for-next tree,
I normally do the merges without anybody having to care about the
mismatches due to patch merge order, but we don't normally have a
massive set of changes queued up. Most conflicts are trivial, but
because of the massive change already queued up, the changes this
time are not trivial and so rather than have everyone who wants to
review your code have to mangle them to test with the for-next code,
he's asked you rebase the patchset against for-next.

There's nothing wrong with that - this is a pretty normal thing to
have to do when working with topic branches in the upstream repo.  I
was just about to send an email to ask you rebase - lucky I read
this first. ;)

FWIW, you should be doing all your testing against for-next, not
against the master branch. i.e.

$ git checkout -b dev-branch master
<hack hackity hack>
$ git commit
<build, test>
$ git checkout -b testing for-next
$ git merge dev-branch
<build, test, test, test>

Otherwise you aren't testing your changes with all the other changes
that have been committed in the current dev cycle....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

Re: [PATCH 4/5] xfs: free inodes on log recovery error

[Christoph Hellwig]

On Wed, Jul 02, 2014 at 09:32:10AM -0500, Mark Tinguely wrote:
> Recovery may free inodes that end up on the inode
> reclaim RCU. If recovery fails, we leak these inodes.
> The filesystem should be in forced shutdown at this
> point, so a call to xfs_reclaim_inode is a fast path
> to freeing the inodes and RCU entries.

I haven't really started reviewing the series, but your terminology
here seems wrong.  RCU is just a way to synchronize updates - do you
mean inodes are marked as reclaimable in the radix tree?

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