[PATCH 0/8] xfs: AIL lock contention reduction V2

[PATCH 0/8] xfs: AIL lock contention reduction V2

[Dave Chinner]

This series reduces AIL locking overhead by introducing bulk operations for
insert and delete. With delayed logging, we can be doing insert operations on
thousands of items at a time, so using a bulk operation to reduce AIL lock
traffic is a major win in terms of reducing contention on the lock.

Similarly, when we have inode buffer IO completion running on multiple CPUs at
once, the AIl lock contention from each individual AIL removal is significant.
By batching all the inodes on the buffer to be removed from the AIL in a single
bulk operation, contention on the AIL is reduced by an order of magnitude.

Finally, there is a modification to the xfsaild wakeup code to prevent too many
wakeups when the xfsaild is pausing waiting for some work to complete. This
prevents an excessive number of AIL scanning traversals by the xfsaild when
tail pushing is backing off and waiting for some IO to complete.

In combination, these modification reduce AIL lock contention sufficiently
to drop the AIL lock out of the "top 10" contended locks on 8-way fs_mark
and dbench workloads.

Version 2:

- split out stale inode AIL insertion bug fix
- split bulk AIL insertion into multiple patches
	- EFI/EFD handling changes
	- stale inode AIL insertion bug fix
	- simplify code calling xfs_ail_delete
	- bulk ail insertion modifications
- use atomic_inc_return for extent recording in xfs_trans_log_efi_extent()
- modified AIL sleep code to explicitly set task state as suggested by Christoph.
- split AIL bulk deletion into two patches
	- buffer iodone callback modifications
	- bulk AIL deletion and inode iodone callback mods.
- removed lip parameter from xfs_buf_do_callbacks() as the loop fetches
          it from the buffer anyway.
- fixed unused variable warning in xfs_buf_do_callbacks().
- added two new patches to reduce code duplication
        - use AIL bulk insertion function to implement single updates
        - use AIL bulk delete function to implement single deletes

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

[PATCH 1/8] xfs: Pull EFI/EFD handling out from under the AIL lock

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

EFI/EFD interactions are protected from races by the AIL lock. They
are the only type of log items that require the the AIL lock to
serialise internal state, so they need to be separated from the AIL
lock before we can do bulk insert operations on the AIL.

To acheive this, convert the counter of the number of extents in the
EFI to an atomic so it can be safely manipulated by EFD processing
without locks. Also, convert the EFI state flag manipulations to use
atomic bit operations so no locks are needed to record state
changes. Finally, use the state bits to determine when it is safe to
free the EFI and clean up the code to do this neatly.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/xfs_extfree_item.c  |   83 ++++++++++++++++++++++++--------------------
 fs/xfs/xfs_extfree_item.h  |   12 +++---
 fs/xfs/xfs_log_recover.c   |    4 +-
 fs/xfs/xfs_trans_extfree.c |    8 +++-
 4 files changed, 59 insertions(+), 48 deletions(-)

diff --git a/fs/xfs/xfs_extfree_item.c b/fs/xfs/xfs_extfree_item.c
index a55e687..7062576 100644
--- a/fs/xfs/xfs_extfree_item.c
+++ b/fs/xfs/xfs_extfree_item.c
@@ -74,7 +74,8 @@ xfs_efi_item_format(
 	struct xfs_efi_log_item	*efip = EFI_ITEM(lip);
 	uint			size;
 
-	ASSERT(efip->efi_next_extent == efip->efi_format.efi_nextents);
+	ASSERT(atomic_read(&efip->efi_next_extent) ==
+				efip->efi_format.efi_nextents);
 
 	efip->efi_format.efi_type = XFS_LI_EFI;
 
@@ -99,10 +100,10 @@ xfs_efi_item_pin(
 }
 
 /*
- * While EFIs cannot really be pinned, the unpin operation is the
- * last place at which the EFI is manipulated during a transaction.
- * Here we coordinate with xfs_efi_cancel() to determine who gets to
- * free the EFI.
+ * While EFIs cannot really be pinned, the unpin operation is the last place at
+ * which the EFI is manipulated during a transaction.  Here we coordinate with
+ * xfs_efi_release() (via XFS_EFI_COMMITTED) to determine who gets to free
+ * the EFI.
  */
 STATIC void
 xfs_efi_item_unpin(
@@ -112,18 +113,18 @@ xfs_efi_item_unpin(
 	struct xfs_efi_log_item	*efip = EFI_ITEM(lip);
 	struct xfs_ail		*ailp = lip->li_ailp;
 
-	spin_lock(&ailp->xa_lock);
-	if (efip->efi_flags & XFS_EFI_CANCELED) {
-		if (remove)
-			xfs_trans_del_item(lip);
-
-		/* xfs_trans_ail_delete() drops the AIL lock. */
-		xfs_trans_ail_delete(ailp, lip);
-		xfs_efi_item_free(efip);
-	} else {
-		efip->efi_flags |= XFS_EFI_COMMITTED;
-		spin_unlock(&ailp->xa_lock);
+	if (remove) {
+		/* transaction cancel - delete and free the item */
+		xfs_trans_del_item(lip);
+	} else if (test_and_clear_bit(XFS_EFI_COMMITTED, &efip->efi_flags)) {
+		/* efd has not be processed yet, it will free the efi */
+		return;
 	}
+
+	spin_lock(&ailp->xa_lock);
+	/* xfs_trans_ail_delete() drops the AIL lock. */
+	xfs_trans_ail_delete(ailp, lip);
+	xfs_efi_item_free(efip);
 }
 
 /*
@@ -152,16 +153,22 @@ xfs_efi_item_unlock(
 }
 
 /*
- * The EFI is logged only once and cannot be moved in the log, so
- * simply return the lsn at which it's been logged.  The canceled
- * flag is not paid any attention here.  Checking for that is delayed
- * until the EFI is unpinned.
+ * The EFI is logged only once and cannot be moved in the log, so simply return
+ * the lsn at which it's been logged.  The canceled flag is not paid any
+ * attention here.  Checking for that is delayed until the EFI is unpinned.
+ *
+ * For bulk transaction committed processing, the EFI may be processed but not
+ * yet unpinned prior to the EFD being processed. Set the XFS_EFI_COMMITTED
+ * flag so this case can be detected when processing the EFD.
  */
 STATIC xfs_lsn_t
 xfs_efi_item_committed(
 	struct xfs_log_item	*lip,
 	xfs_lsn_t		lsn)
 {
+	struct xfs_efi_log_item	*efip = EFI_ITEM(lip);
+
+	set_bit(XFS_EFI_COMMITTED, &efip->efi_flags);
 	return lsn;
 }
 
@@ -289,36 +296,36 @@ xfs_efi_copy_format(xfs_log_iovec_t *buf, xfs_efi_log_format_t *dst_efi_fmt)
 }
 
 /*
- * This is called by the efd item code below to release references to
- * the given efi item.  Each efd calls this with the number of
- * extents that it has logged, and when the sum of these reaches
- * the total number of extents logged by this efi item we can free
- * the efi item.
+ * This is called by the efd item code below to release references to the given
+ * efi item.  Each efd calls this with the number of extents that it has
+ * logged, and when the sum of these reaches the total number of extents logged
+ * by this efi item we can free the efi item.
+ *
+ * Freeing the efi item requires that we remove it from the AIL if it has
+ * already been placed there. However, the EFI may not yet have been placed in
+ * the AIL due to a bulk insert operation, so we have to be careful here. This
+ * case is detected if the XFS_EFI_COMMITTED flag is set. This code is
+ * tricky - both xfs_efi_item_unpin() and this code do test_and_clear_bit()
+ * operations on this flag - if it is not set here, then it means that the
+ * unpin has run and we don't need to free it. If it is set here, then we clear
+ * it to tell the unpin we have run and that the unpin needs to free the EFI.
  *
- * Freeing the efi item requires that we remove it from the AIL.
- * We'll use the AIL lock to protect our counters as well as
- * the removal from the AIL.
  */
 void
 xfs_efi_release(xfs_efi_log_item_t	*efip,
 		uint			nextents)
 {
 	struct xfs_ail		*ailp = efip->efi_item.li_ailp;
-	int			extents_left;
 
-	ASSERT(efip->efi_next_extent > 0);
-	ASSERT(efip->efi_flags & XFS_EFI_COMMITTED);
+	ASSERT(atomic_read(&efip->efi_next_extent) >= nextents);
+	if (!atomic_sub_and_test(nextents, &efip->efi_next_extent))
+		return;
 
-	spin_lock(&ailp->xa_lock);
-	ASSERT(efip->efi_next_extent >= nextents);
-	efip->efi_next_extent -= nextents;
-	extents_left = efip->efi_next_extent;
-	if (extents_left == 0) {
+	if (!test_and_clear_bit(XFS_EFI_COMMITTED, &efip->efi_flags)) {
+		spin_lock(&ailp->xa_lock);
 		/* xfs_trans_ail_delete() drops the AIL lock. */
 		xfs_trans_ail_delete(ailp, (xfs_log_item_t *)efip);
 		xfs_efi_item_free(efip);
-	} else {
-		spin_unlock(&ailp->xa_lock);
 	}
 }
 
diff --git a/fs/xfs/xfs_extfree_item.h b/fs/xfs/xfs_extfree_item.h
index 0d22c56..26a7550 100644
--- a/fs/xfs/xfs_extfree_item.h
+++ b/fs/xfs/xfs_extfree_item.h
@@ -111,11 +111,11 @@ typedef struct xfs_efd_log_format_64 {
 #define	XFS_EFI_MAX_FAST_EXTENTS	16
 
 /*
- * Define EFI flags.
+ * Define EFI flag bits. Manipulated by set/clear/test_bit operators.
  */
-#define	XFS_EFI_RECOVERED	0x1
-#define	XFS_EFI_COMMITTED	0x2
-#define	XFS_EFI_CANCELED	0x4
+#define	XFS_EFI_RECOVERED	1
+#define	XFS_EFI_CANCELED	2
+#define	XFS_EFI_COMMITTED	3
 
 /*
  * This is the "extent free intention" log item.  It is used
@@ -125,8 +125,8 @@ typedef struct xfs_efd_log_format_64 {
  */
 typedef struct xfs_efi_log_item {
 	xfs_log_item_t		efi_item;
-	uint			efi_flags;	/* misc flags */
-	uint			efi_next_extent;
+	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_recover.c b/fs/xfs/xfs_log_recover.c
index 966d3f9..baad94a 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -2717,8 +2717,8 @@ xlog_recover_do_efi_trans(
 		xfs_efi_item_free(efip);
 		return error;
 	}
-	efip->efi_next_extent = efi_formatp->efi_nextents;
-	efip->efi_flags |= XFS_EFI_COMMITTED;
+	atomic_set(&efip->efi_next_extent, efi_formatp->efi_nextents);
+	clear_bit(XFS_EFI_COMMITTED, &efip->efi_flags);
 
 	spin_lock(&log->l_ailp->xa_lock);
 	/*
diff --git a/fs/xfs/xfs_trans_extfree.c b/fs/xfs/xfs_trans_extfree.c
index f783d5e..f7590f5 100644
--- a/fs/xfs/xfs_trans_extfree.c
+++ b/fs/xfs/xfs_trans_extfree.c
@@ -69,12 +69,16 @@ xfs_trans_log_efi_extent(xfs_trans_t		*tp,
 	tp->t_flags |= XFS_TRANS_DIRTY;
 	efip->efi_item.li_desc->lid_flags |= XFS_LID_DIRTY;
 
-	next_extent = efip->efi_next_extent;
+	/*
+	 * atomic_inc_return gives us the value after the increment;
+	 * we want to use it as an array index so we need to subtract 1 from
+	 * it.
+	 */
+	next_extent = atomic_inc_return(&efip->efi_next_extent) - 1;
 	ASSERT(next_extent < efip->efi_format.efi_nextents);
 	extp = &(efip->efi_format.efi_extents[next_extent]);
 	extp->ext_start = start_block;
 	extp->ext_len = ext_len;
-	efip->efi_next_extent++;
 }
 
 
-- 
1.7.2.3

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

[PATCH 2/8] xfs: clean up xfs_ail_delete()

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

xfs_ail_delete() has a needlessly complex interface. It returns the log item
that was passed in for deletion (which the callers then assert is identical to
the one passed in), and callers of xfs_ail_delete() still need to invalidate
current traversal cursors.

Make xfs_ail_delete() return void, move the cursor invalidation inside it, and
clean up the callers just to use the log item pointer they passed in.

While cleaning up, remove the messy and unnecessary "/* ARGUSED */" comments
around all these functions.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/xfs_trans_ail.c |   27 +++++++--------------------
 1 files changed, 7 insertions(+), 20 deletions(-)

diff --git a/fs/xfs/xfs_trans_ail.c b/fs/xfs/xfs_trans_ail.c
index dc90695..645928c 100644
--- a/fs/xfs/xfs_trans_ail.c
+++ b/fs/xfs/xfs_trans_ail.c
@@ -29,7 +29,7 @@
 #include "xfs_error.h"
 
 STATIC void xfs_ail_insert(struct xfs_ail *, xfs_log_item_t *);
-STATIC xfs_log_item_t * xfs_ail_delete(struct xfs_ail *, xfs_log_item_t *);
+STATIC void xfs_ail_delete(struct xfs_ail *, xfs_log_item_t *);
 STATIC xfs_log_item_t * xfs_ail_min(struct xfs_ail *);
 STATIC xfs_log_item_t * xfs_ail_next(struct xfs_ail *, xfs_log_item_t *);
 
@@ -468,16 +468,13 @@ xfs_trans_ail_update(
 	xfs_log_item_t	*lip,
 	xfs_lsn_t	lsn) __releases(ailp->xa_lock)
 {
-	xfs_log_item_t		*dlip = NULL;
 	xfs_log_item_t		*mlip;	/* ptr to minimum lip */
 	xfs_lsn_t		tail_lsn;
 
 	mlip = xfs_ail_min(ailp);
 
 	if (lip->li_flags & XFS_LI_IN_AIL) {
-		dlip = xfs_ail_delete(ailp, lip);
-		ASSERT(dlip == lip);
-		xfs_trans_ail_cursor_clear(ailp, dlip);
+		xfs_ail_delete(ailp, lip);
 	} else {
 		lip->li_flags |= XFS_LI_IN_AIL;
 	}
@@ -485,7 +482,7 @@ xfs_trans_ail_update(
 	lip->li_lsn = lsn;
 	xfs_ail_insert(ailp, lip);
 
-	if (mlip == dlip) {
+	if (mlip == lip) {
 		mlip = xfs_ail_min(ailp);
 		/*
 		 * It is not safe to access mlip after the AIL lock is
@@ -524,21 +521,18 @@ xfs_trans_ail_delete(
 	struct xfs_ail	*ailp,
 	xfs_log_item_t	*lip) __releases(ailp->xa_lock)
 {
-	xfs_log_item_t		*dlip;
 	xfs_log_item_t		*mlip;
 	xfs_lsn_t		tail_lsn;
 
 	if (lip->li_flags & XFS_LI_IN_AIL) {
 		mlip = xfs_ail_min(ailp);
-		dlip = xfs_ail_delete(ailp, lip);
-		ASSERT(dlip == lip);
-		xfs_trans_ail_cursor_clear(ailp, dlip);
+		xfs_ail_delete(ailp, lip);
 
 
 		lip->li_flags &= ~XFS_LI_IN_AIL;
 		lip->li_lsn = 0;
 
-		if (mlip == dlip) {
+		if (mlip == lip) {
 			mlip = xfs_ail_min(ailp);
 			/*
 			 * It is not safe to access mlip after the AIL lock
@@ -632,7 +626,6 @@ STATIC void
 xfs_ail_insert(
 	struct xfs_ail	*ailp,
 	xfs_log_item_t	*lip)
-/* ARGSUSED */
 {
 	xfs_log_item_t	*next_lip;
 
@@ -661,18 +654,14 @@ xfs_ail_insert(
 /*
  * Delete the given item from the AIL.  Return a pointer to the item.
  */
-/*ARGSUSED*/
-STATIC xfs_log_item_t *
+STATIC void
 xfs_ail_delete(
 	struct xfs_ail	*ailp,
 	xfs_log_item_t	*lip)
-/* ARGSUSED */
 {
 	xfs_ail_check(ailp, lip);
-
 	list_del(&lip->li_ail);
-
-	return lip;
+	xfs_trans_ail_cursor_clear(ailp, lip);
 }
 
 /*
@@ -682,7 +671,6 @@ xfs_ail_delete(
 STATIC xfs_log_item_t *
 xfs_ail_min(
 	struct xfs_ail	*ailp)
-/* ARGSUSED */
 {
 	if (list_empty(&ailp->xa_ail))
 		return NULL;
@@ -699,7 +687,6 @@ STATIC xfs_log_item_t *
 xfs_ail_next(
 	struct xfs_ail	*ailp,
 	xfs_log_item_t	*lip)
-/* ARGSUSED */
 {
 	if (lip->li_ail.next == &ailp->xa_ail)
 		return NULL;
-- 
1.7.2.3

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

[PATCH 3/8] xfs: bulk AIL insertion during transaction commit

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

When inserting items into the AIL from the transaction committed
callbacks, we take the AIL lock for every single item that is to be
inserted. For a CIL checkpoint commit, this can be tens of thousands
of individual inserts, yet almost all of the items will be inserted
at the same point in the AIL because they have the same index.

To reduce the overhead and contention on the AIL lock for such
operations, introduce a "bulk insert" operation which allows a list
of log items with the same LSN to be inserted in a single operation
via a list splice. To do this, we need to pre-sort the log items
being committed into a temporary list for insertion.

The complexity is that not every log item will end up with the same
LSN, and not every item is actually inserted into the AIL. Items
that don't match the commit LSN will be inserted and unpinned as per
the current one-at-a-time method (relatively rare), while items that
are not to be inserted will be unpinned and freed immediately. Items
that are to be inserted at the given commit lsn are placed in a
temporary array and inserted into the AIL in bulk each time the
array fills up.

As a result of this, we trade off AIL hold time for a significant
reduction in traffic. lock_stat output shows that the worst case
hold time is unchanged, but contention from AIL inserts drops by an
order of magnitude and the number of lock traversal decreases
significantly.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/xfs_log_cil.c    |    9 +---
 fs/xfs/xfs_trans.c      |   72 +++++++++++++++++++++++++++++++++-
 fs/xfs/xfs_trans_ail.c  |  101 +++++++++++++++++++++++++++++++++++++++++++++++
 fs/xfs/xfs_trans_priv.h |   10 ++++-
 4 files changed, 182 insertions(+), 10 deletions(-)

diff --git a/fs/xfs/xfs_log_cil.c b/fs/xfs/xfs_log_cil.c
index 23d6ceb..f36f1a2 100644
--- a/fs/xfs/xfs_log_cil.c
+++ b/fs/xfs/xfs_log_cil.c
@@ -361,15 +361,10 @@ xlog_cil_committed(
 	int	abort)
 {
 	struct xfs_cil_ctx	*ctx = args;
-	struct xfs_log_vec	*lv;
-	int			abortflag = abort ? XFS_LI_ABORTED : 0;
 	struct xfs_busy_extent	*busyp, *n;
 
-	/* unpin all the log items */
-	for (lv = ctx->lv_chain; lv; lv = lv->lv_next ) {
-		xfs_trans_item_committed(lv->lv_item, ctx->start_lsn,
-							abortflag);
-	}
+	xfs_trans_committed_bulk(ctx->cil->xc_log->l_ailp, ctx->lv_chain,
+					ctx->start_lsn, abort);
 
 	list_for_each_entry_safe(busyp, n, &ctx->busy_extents, list)
 		xfs_alloc_busy_clear(ctx->cil->xc_log->l_mp, busyp);
diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c
index cf2214d..fc45287 100644
--- a/fs/xfs/xfs_trans.c
+++ b/fs/xfs/xfs_trans.c
@@ -1349,7 +1349,7 @@ xfs_trans_fill_vecs(
  * they could be immediately flushed and we'd have to race with the flusher
  * trying to pull the item from the AIL as we add it.
  */
-void
+static void
 xfs_trans_item_committed(
 	struct xfs_log_item	*lip,
 	xfs_lsn_t		commit_lsn,
@@ -1425,6 +1425,76 @@ xfs_trans_committed(
 }
 
 /*
+ * Bulk operation version of xfs_trans_committed that takes a log vector of
+ * items to insert into the AIL. This uses bulk AIL insertion techniques to
+ * minimise lock traffic.
+ */
+void
+xfs_trans_committed_bulk(
+	struct xfs_ail		*ailp,
+	struct xfs_log_vec	*log_vector,
+	xfs_lsn_t		commit_lsn,
+	int			aborted)
+{
+#define LGIA_SIZE	32
+	struct xfs_log_item	*lgia[LGIA_SIZE];
+	struct xfs_log_vec	*lv;
+	int			i = 0;
+
+	/* unpin all the log items */
+	for (lv = log_vector; lv; lv = lv->lv_next ) {
+		struct xfs_log_item	*lip = lv->lv_item;
+		xfs_lsn_t		item_lsn;
+
+		if (aborted)
+			lip->li_flags |= XFS_LI_ABORTED;
+		item_lsn = IOP_COMMITTED(lip, commit_lsn);
+
+		/* item_lsn of -1 means the item was freed */
+		if (XFS_LSN_CMP(item_lsn, (xfs_lsn_t)-1) == 0)
+			continue;
+
+		if (item_lsn != commit_lsn) {
+
+			/*
+			 * Not a bulk update option due to unusual item_lsn.
+			 * Push into AIL immediately, rechecking the lsn once
+			 * we have the ail lock. Then unpin the item.
+			 */
+			spin_lock(&ailp->xa_lock);
+			if (XFS_LSN_CMP(item_lsn, lip->li_lsn) > 0) {
+				xfs_trans_ail_update(ailp, lip, item_lsn);
+			} else {
+				spin_unlock(&ailp->xa_lock);
+			}
+			IOP_UNPIN(lip, 0);
+			continue;
+		}
+
+		/* Item is a candidate for bulk AIL insert.  */
+		lgia[i++] = lv->lv_item;
+		if (i >= LGIA_SIZE) {
+			spin_lock(&ailp->xa_lock);
+			xfs_trans_ail_update_bulk(ailp, lgia, LGIA_SIZE,
+							commit_lsn);
+			for (i = 0; i < LGIA_SIZE; i++)
+				IOP_UNPIN(lgia[i], 0);
+			i = 0;
+		}
+	}
+
+	/* make sure we insert the remainder! */
+	if (i) {
+		int j;
+
+		spin_lock(&ailp->xa_lock);
+		xfs_trans_ail_update_bulk(ailp, lgia, i, commit_lsn);
+		for (j = 0; j < i; j++)
+			IOP_UNPIN(lgia[j], 0);
+	}
+}
+
+/*
  * Called from the trans_commit code when we notice that
  * the filesystem is in the middle of a forced shutdown.
  */
diff --git a/fs/xfs/xfs_trans_ail.c b/fs/xfs/xfs_trans_ail.c
index 645928c..f70f295 100644
--- a/fs/xfs/xfs_trans_ail.c
+++ b/fs/xfs/xfs_trans_ail.c
@@ -29,6 +29,7 @@
 #include "xfs_error.h"
 
 STATIC void xfs_ail_insert(struct xfs_ail *, xfs_log_item_t *);
+STATIC void xfs_ail_splice(struct xfs_ail *, struct list_head *, xfs_lsn_t);
 STATIC void xfs_ail_delete(struct xfs_ail *, xfs_log_item_t *);
 STATIC xfs_log_item_t * xfs_ail_min(struct xfs_ail *);
 STATIC xfs_log_item_t * xfs_ail_next(struct xfs_ail *, xfs_log_item_t *);
@@ -502,6 +503,75 @@ xfs_trans_ail_update(
 }	/* xfs_trans_update_ail */
 
 /*
+ * Bulk update version of xfs_trans_ail_update.
+ *
+ * This version takes an array of log items that all need to be positioned at
+ * the same LSN in the AIL. This function takes the AIL lock once to execute
+ * the update operations on all the items in the array, and as such shoul dnot
+ * be called with the AIL lock held. As a result, once we have the AIL lock,
+ * we need to check each log item LSN to confirm it needs to be moved forward
+ * in the AIL.
+ *
+ * To optimise the insert operation, we delete all the items from the AIL in
+ * the first pass, moving them into a temporary list, then splice the temporary
+ * list into the correct position in the AIL. THis avoids needing to do an
+ * insert operation on every item.
+ *
+ * This function must be called with the AIL lock held.  The lock
+ * is dropped before returning.
+ */
+void
+xfs_trans_ail_update_bulk(
+	struct xfs_ail		*ailp,
+	struct xfs_log_item	**lgia,
+	int			nr_items,
+	xfs_lsn_t		lsn) __releases(ailp->xa_lock)
+{
+	xfs_log_item_t		*mlip;
+	xfs_lsn_t		tail_lsn;
+	int			mlip_changed = 0;
+	int			i;
+	LIST_HEAD(tmp);
+
+	mlip = xfs_ail_min(ailp);
+
+	for (i = 0; i < nr_items; i++) {
+		struct xfs_log_item *lip = lgia[i];
+		if (lip->li_flags & XFS_LI_IN_AIL) {
+			/* check if we really need to move the item */
+			if (XFS_LSN_CMP(lsn, lip->li_lsn) <= 0)
+				continue;
+
+			xfs_ail_delete(ailp, lip);
+			if (mlip == lip)
+				mlip_changed = 1;
+		} else {
+			lip->li_flags |= XFS_LI_IN_AIL;
+		}
+		lip->li_lsn = lsn;
+		list_add(&lip->li_ail, &tmp);
+	}
+
+	xfs_ail_splice(ailp, &tmp, lsn);
+
+	if (!mlip_changed) {
+		spin_unlock(&ailp->xa_lock);
+		return;
+	}
+
+	/*
+	 * It is not safe to access mlip after the AIL lock is dropped, so we
+	 * must get a copy of li_lsn before we do so.  This is especially
+	 * important on 32-bit platforms where accessing and updating 64-bit
+	 * values like li_lsn is not atomic.
+	 */
+	mlip = xfs_ail_min(ailp);
+	tail_lsn = mlip->li_lsn;
+	spin_unlock(&ailp->xa_lock);
+	xfs_log_move_tail(ailp->xa_mount, tail_lsn);
+}
+
+/*
  * Delete the given item from the AIL.  It must already be in
  * the AIL.
  *
@@ -652,6 +722,37 @@ xfs_ail_insert(
 }
 
 /*
+ * splice the log item list into the AIL at the given LSN.
+ */
+STATIC void
+xfs_ail_splice(
+	struct xfs_ail	*ailp,
+	struct list_head *list,
+	xfs_lsn_t	lsn)
+{
+	xfs_log_item_t	*next_lip;
+
+	/*
+	 * If the list is empty, just insert the item.
+	 */
+	if (list_empty(&ailp->xa_ail)) {
+		list_splice(list, &ailp->xa_ail);
+		return;
+	}
+
+	list_for_each_entry_reverse(next_lip, &ailp->xa_ail, li_ail) {
+		if (XFS_LSN_CMP(next_lip->li_lsn, lsn) <= 0)
+			break;
+	}
+
+	ASSERT((&next_lip->li_ail == &ailp->xa_ail) ||
+	       (XFS_LSN_CMP(next_lip->li_lsn, lsn) <= 0));
+
+	list_splice_init(list, &next_lip->li_ail);
+	return;
+}
+
+/*
  * Delete the given item from the AIL.  Return a pointer to the item.
  */
 STATIC void
diff --git a/fs/xfs/xfs_trans_priv.h b/fs/xfs/xfs_trans_priv.h
index 62da86c..07ea4b7 100644
--- a/fs/xfs/xfs_trans_priv.h
+++ b/fs/xfs/xfs_trans_priv.h
@@ -22,15 +22,17 @@ struct xfs_log_item;
 struct xfs_log_item_desc;
 struct xfs_mount;
 struct xfs_trans;
+struct xfs_ail;
+struct xfs_log_vec;
 
 void	xfs_trans_add_item(struct xfs_trans *, struct xfs_log_item *);
 void	xfs_trans_del_item(struct xfs_log_item *);
 void	xfs_trans_free_items(struct xfs_trans *tp, xfs_lsn_t commit_lsn,
 				int flags);
-void	xfs_trans_item_committed(struct xfs_log_item *lip,
-				xfs_lsn_t commit_lsn, int aborted);
 void	xfs_trans_unreserve_and_mod_sb(struct xfs_trans *tp);
 
+void	xfs_trans_committed_bulk(struct xfs_ail *ailp, struct xfs_log_vec *lv,
+				xfs_lsn_t commit_lsn, int aborted);
 /*
  * AIL traversal cursor.
  *
@@ -76,6 +78,10 @@ struct xfs_ail {
 void			xfs_trans_ail_update(struct xfs_ail *ailp,
 					struct xfs_log_item *lip, xfs_lsn_t lsn)
 					__releases(ailp->xa_lock);
+void			 xfs_trans_ail_update_bulk(struct xfs_ail *ailp,
+					struct xfs_log_item **lgia,
+					int nr_items, xfs_lsn_t lsn)
+					__releases(ailp->xa_lock);
 void			xfs_trans_ail_delete(struct xfs_ail *ailp,
 					struct xfs_log_item *lip)
 					__releases(ailp->xa_lock);
-- 
1.7.2.3

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

[PATCH 4/8] xfs: reduce the number of AIL push wakeups

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

The xfaild often tries to rest to wait for congestion to pass of for
IO to complete, but is regularly woken in tail-pushing situations.
In severe cases, the xfsaild is getting woken tens of thousands of
times a second. Reduce the number needless wakeups by only waking
the xfsaild if the new target is larger than the old one. Further
make short sleeps uninterruptible as they occur when the xfsaild has
decided it needs to back off to allow some IO to complete and being
woken early is counter-productive.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/linux-2.6/xfs_super.c |   20 ++++++++++++++++----
 1 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/fs/xfs/linux-2.6/xfs_super.c b/fs/xfs/linux-2.6/xfs_super.c
index 6cbebb2..8546805 100644
--- a/fs/xfs/linux-2.6/xfs_super.c
+++ b/fs/xfs/linux-2.6/xfs_super.c
@@ -834,8 +834,11 @@ xfsaild_wakeup(
 	struct xfs_ail		*ailp,
 	xfs_lsn_t		threshold_lsn)
 {
-	ailp->xa_target = threshold_lsn;
-	wake_up_process(ailp->xa_task);
+	/* only ever move the target forwards */
+	if (XFS_LSN_CMP(threshold_lsn, ailp->xa_target) > 0) {
+		ailp->xa_target = threshold_lsn;
+		wake_up_process(ailp->xa_task);
+	}
 }
 
 STATIC int
@@ -847,8 +850,17 @@ xfsaild(
 	long		tout = 0; /* milliseconds */
 
 	while (!kthread_should_stop()) {
-		schedule_timeout_interruptible(tout ?
-				msecs_to_jiffies(tout) : MAX_SCHEDULE_TIMEOUT);
+		/*
+		 * for short sleeps indicating congestion, don't allow us to
+		 * get woken early. Otherwise all we do is bang on the AIL lock
+		 * without making progress.
+		 */
+		if (tout && tout <= 20)
+			__set_current_state(TASK_KILLABLE);
+		else
+			__set_current_state(TASK_UNINTERRUPTIBLE);
+		schedule_timeout(tout ?
+				 msecs_to_jiffies(tout) : MAX_SCHEDULE_TIMEOUT);
 
 		/* swsusp */
 		try_to_freeze();
-- 
1.7.2.3

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

[PATCH 5/8] xfs: consume iodone callback items on buffers as they are processed

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

To allow buffer iodone callbacks to consume multiple items off the
callback list, first we need to convert the xfs_buf_do_callbacks()
to consume items and always pull the next item from the head of the
list.

The means the item list walk is never dependent on knowing the
next item on the list and hence allows callbacks to remove itesm
from the list as well. This allows callbacks to do bulk operations
by scanning the list for identical callbacks, consuming them all
and then processing them in bulk, negating the need for multiple
callbacks of that type.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/xfs_buf_item.c |   32 +++++++++++++++++++++-----------
 1 files changed, 21 insertions(+), 11 deletions(-)

diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
index 2686d0d..ed2b65f 100644
--- a/fs/xfs/xfs_buf_item.c
+++ b/fs/xfs/xfs_buf_item.c
@@ -142,7 +142,7 @@ xfs_buf_item_log_check(
 #endif
 
 STATIC void	xfs_buf_error_relse(xfs_buf_t *bp);
-STATIC void	xfs_buf_do_callbacks(xfs_buf_t *bp, xfs_log_item_t *lip);
+STATIC void	xfs_buf_do_callbacks(struct xfs_buf *bp);
 
 /*
  * This returns the number of log iovecs needed to log the
@@ -450,7 +450,7 @@ xfs_buf_item_unpin(
 		 * xfs_trans_ail_delete() drops the AIL lock.
 		 */
 		if (bip->bli_flags & XFS_BLI_STALE_INODE) {
-			xfs_buf_do_callbacks(bp, (xfs_log_item_t *)bip);
+			xfs_buf_do_callbacks(bp);
 			XFS_BUF_SET_FSPRIVATE(bp, NULL);
 			XFS_BUF_CLR_IODONE_FUNC(bp);
 		} else {
@@ -918,15 +918,26 @@ xfs_buf_attach_iodone(
 	XFS_BUF_SET_IODONE_FUNC(bp, xfs_buf_iodone_callbacks);
 }
 
+/*
+ * We can have many callbacks on a buffer. Running the callbacks individually
+ * can cause a lot of contention on the AIL lock, so we allow for a single
+ * callback to be able to scan the remaining lip->li_bio_list for other items
+ * of the same type and callback to be processed in the first call.
+ *
+ * As a result, the loop walking the callback list below will also modify the
+ * list. it removes the first item from the list and then runs the callback.
+ * The loop then restarts from the new head of the list. This allows the
+ * callback to scan and modify the list attached to the buffer and we don't
+ * have to care about maintaining a next item pointer.
+ */
 STATIC void
 xfs_buf_do_callbacks(
-	xfs_buf_t	*bp,
-	xfs_log_item_t	*lip)
+	struct xfs_buf		*bp)
 {
-	xfs_log_item_t	*nlip;
+	struct xfs_log_item	*lip;
 
-	while (lip != NULL) {
-		nlip = lip->li_bio_list;
+	while ((lip = XFS_BUF_FSPRIVATE(bp, xfs_log_item_t *)) != NULL) {
+		XFS_BUF_SET_FSPRIVATE(bp, lip->li_bio_list);
 		ASSERT(lip->li_cb != NULL);
 		/*
 		 * Clear the next pointer so we don't have any
@@ -936,7 +947,6 @@ xfs_buf_do_callbacks(
 		 */
 		lip->li_bio_list = NULL;
 		lip->li_cb(bp, lip);
-		lip = nlip;
 	}
 }
 
@@ -970,7 +980,7 @@ xfs_buf_iodone_callbacks(
 			ASSERT(XFS_BUF_TARGET(bp) == mp->m_ddev_targp);
 			XFS_BUF_SUPER_STALE(bp);
 			trace_xfs_buf_item_iodone(bp, _RET_IP_);
-			xfs_buf_do_callbacks(bp, lip);
+			xfs_buf_do_callbacks(bp);
 			XFS_BUF_SET_FSPRIVATE(bp, NULL);
 			XFS_BUF_CLR_IODONE_FUNC(bp);
 			xfs_buf_ioend(bp, 0);
@@ -1029,7 +1039,7 @@ xfs_buf_iodone_callbacks(
 		return;
 	}
 
-	xfs_buf_do_callbacks(bp, lip);
+	xfs_buf_do_callbacks(bp);
 	XFS_BUF_SET_FSPRIVATE(bp, NULL);
 	XFS_BUF_CLR_IODONE_FUNC(bp);
 	xfs_buf_ioend(bp, 0);
@@ -1063,7 +1073,7 @@ xfs_buf_error_relse(
 	 * We have to unpin the pinned buffers so do the
 	 * callbacks.
 	 */
-	xfs_buf_do_callbacks(bp, lip);
+	xfs_buf_do_callbacks(bp);
 	XFS_BUF_SET_FSPRIVATE(bp, NULL);
 	XFS_BUF_CLR_IODONE_FUNC(bp);
 	XFS_BUF_SET_BRELSE_FUNC(bp,NULL);
-- 
1.7.2.3

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

[PATCH 6/8] xfs: remove all the inodes on a buffer from the AIL in bulk

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

When inode buffer IO completes, usually all of the inodes are removed from the
AIL. This involves processing them one at a time and taking the AIL lock once
for every inode. When all CPUs are processing inode IO completions, this causes
excessive amount sof contention on the AIL lock.

Instead, change the way we process inode IO completion in the buffer
IO done callback. Allow the inode IO done callback to walk the list
of IO done callbacks and pull all the inodes off the buffer in one
go and then process them as a batch.

Once all the inodes for removal are collected, take the AIL lock
once and do a bulk removal operation to minimise traffic on the AIL
lock.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/xfs_inode_item.c |   92 ++++++++++++++++++++++++++++++++++++++---------
 fs/xfs/xfs_trans_ail.c  |   64 ++++++++++++++++++++++++++++++++
 fs/xfs/xfs_trans_priv.h |    4 ++
 3 files changed, 143 insertions(+), 17 deletions(-)

diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c
index 7c8d30c..1d72bb1 100644
--- a/fs/xfs/xfs_inode_item.c
+++ b/fs/xfs/xfs_inode_item.c
@@ -842,15 +842,64 @@ xfs_inode_item_destroy(
  * flushed to disk.  It is responsible for removing the inode item
  * from the AIL if it has not been re-logged, and unlocking the inode's
  * flush lock.
+ *
+ * To reduce AIL lock traffic as much as possible, we scan the buffer log item
+ * list for other inodes that will run this function. We remove them from the
+ * buffer list so we can process all the inode IO completions in one AIL lock
+ * traversal.
  */
 void
 xfs_iflush_done(
 	struct xfs_buf		*bp,
 	struct xfs_log_item	*lip)
 {
-	struct xfs_inode_log_item *iip = INODE_ITEM(lip);
-	xfs_inode_t		*ip = iip->ili_inode;
+	struct xfs_inode_log_item *iip;
+	struct xfs_log_item	*blip;
+	struct xfs_log_item	*next;
+	struct xfs_log_item	*prev;
 	struct xfs_ail		*ailp = lip->li_ailp;
+	int			need_ail = 0;
+
+	/*
+	 * Scan the buffer IO completions for other inodes being completed and
+	 * attach them to the current inode log item.
+	 */
+	blip = XFS_BUF_FSPRIVATE(bp, xfs_log_item_t *);
+	prev = NULL;
+	while (blip != NULL) {
+		if (lip->li_cb != xfs_iflush_done) {
+			prev = blip;
+			blip = blip->li_bio_list;
+			continue;
+		}
+
+		/* remove from list */
+		next = blip->li_bio_list;
+		if (!prev) {
+			XFS_BUF_SET_FSPRIVATE(bp, next);
+		} else {
+			prev->li_bio_list = next;
+		}
+
+		/* add to current list */
+		blip->li_bio_list = lip->li_bio_list;
+		lip->li_bio_list = blip;
+
+		/*
+		 * while we have the item, do the unlocked check for needing
+		 * the AIL lock.
+		 */
+		iip = INODE_ITEM(blip);
+		if (iip->ili_logged && blip->li_lsn == iip->ili_flush_lsn)
+			need_ail++;
+
+		blip = next;
+	}
+
+	/* make sure we capture the state of the initial inode. */
+	iip = INODE_ITEM(lip);
+	if (iip->ili_logged && lip->li_lsn == iip->ili_flush_lsn)
+		need_ail++;
 
 	/*
 	 * We only want to pull the item from the AIL if it is
@@ -861,28 +910,37 @@ xfs_iflush_done(
 	 * the lock since it's cheaper, and then we recheck while
 	 * holding the lock before removing the inode from the AIL.
 	 */
-	if (iip->ili_logged && lip->li_lsn == iip->ili_flush_lsn) {
+	if (need_ail) {
+		struct xfs_log_item *lgia[need_ail];
+		int i = 0;
 		spin_lock(&ailp->xa_lock);
-		if (lip->li_lsn == iip->ili_flush_lsn) {
-			/* xfs_trans_ail_delete() drops the AIL lock. */
-			xfs_trans_ail_delete(ailp, lip);
-		} else {
-			spin_unlock(&ailp->xa_lock);
+		for (blip = lip; blip; blip = blip->li_bio_list) {
+			iip = INODE_ITEM(blip);
+			if (iip->ili_logged &&
+			    blip->li_lsn == iip->ili_flush_lsn) {
+				lgia[i++] = blip;
+			}
+			ASSERT(i <= need_ail);
 		}
+		/* xfs_trans_ail_delete_bulk() drops the AIL lock. */
+		xfs_trans_ail_delete_bulk(ailp, lgia, i);
 	}
 
-	iip->ili_logged = 0;
 
 	/*
-	 * Clear the ili_last_fields bits now that we know that the
-	 * data corresponding to them is safely on disk.
+	 * clean up and unlock the flush lock now we are done. We can clear the
+	 * ili_last_fields bits now that we know that the data corresponding to
+	 * them is safely on disk.
 	 */
-	iip->ili_last_fields = 0;
+	for (blip = lip; blip; blip = next) {
+		next = blip->li_bio_list;
+		blip->li_bio_list = NULL;
 
-	/*
-	 * Release the inode's flush lock since we're done with it.
-	 */
-	xfs_ifunlock(ip);
+		iip = INODE_ITEM(blip);
+		iip->ili_logged = 0;
+		iip->ili_last_fields = 0;
+		xfs_ifunlock(iip->ili_inode);
+	}
 }
 
 /*
diff --git a/fs/xfs/xfs_trans_ail.c b/fs/xfs/xfs_trans_ail.c
index f70f295..d450092 100644
--- a/fs/xfs/xfs_trans_ail.c
+++ b/fs/xfs/xfs_trans_ail.c
@@ -635,6 +635,70 @@ xfs_trans_ail_delete(
 	}
 }
 
+/*
+ * Bulk update version of xfs_trans_ail_delete
+ *
+ * This version takes an array of log items that all need to removed from the
+ * AIL. The caller is already holding the AIL lock, and done all the checks
+ * necessary to ensure the items passed in via @lgia are ready for deletion.
+ *
+ * This function will not drop the AIL lock until all items are removed from
+ * the AIL to minimise the amount of lock traffic on the AIL. This does not
+ * greatly increase the AIL hold time, but does significantly reduce the amount
+ * of traffic on the lock, especially during IO completion.
+ */
+void
+xfs_trans_ail_delete_bulk(
+	struct xfs_ail		*ailp,
+	struct xfs_log_item	**lgia,
+	int			nr_items) __releases(ailp->xa_lock)
+{
+	xfs_log_item_t		*mlip;
+	xfs_lsn_t		tail_lsn;
+	int			mlip_changed = 0;
+	int			i;
+
+	mlip = xfs_ail_min(ailp);
+
+	for (i = 0; i < nr_items; i++) {
+		struct xfs_log_item *lip = lgia[i];
+		if (!(lip->li_flags & XFS_LI_IN_AIL)) {
+			struct xfs_mount	*mp = ailp->xa_mount;
+
+			spin_unlock(&ailp->xa_lock);
+			if (!XFS_FORCED_SHUTDOWN(mp)) {
+				xfs_cmn_err(XFS_PTAG_AILDELETE, CE_ALERT, mp,
+		"%s: attempting to delete a log item that is not in the AIL",
+						__func__);
+				xfs_force_shutdown(mp, SHUTDOWN_CORRUPT_INCORE);
+			}
+			return;
+		}
+
+		xfs_ail_delete(ailp, lip);
+		lip->li_flags &= ~XFS_LI_IN_AIL;
+		lip->li_lsn = 0;
+		if (mlip == lip)
+			mlip_changed = 1;
+	}
+
+	if (!mlip_changed) {
+		spin_unlock(&ailp->xa_lock);
+		return;
+	}
+
+	/*
+	 * It is not safe to access mlip after the AIL lock is dropped, so we
+	 * must get a copy of li_lsn before we do so.  This is especially
+	 * important on 32-bit platforms where accessing and updating 64-bit
+	 * values like li_lsn is not atomic. It is possible we've emptied the
+	 * AIL here, so if that is the case, pass an LSN of 0 to the tail move.
+	 */
+	mlip = xfs_ail_min(ailp);
+	tail_lsn = mlip ? mlip->li_lsn : 0;
+	spin_unlock(&ailp->xa_lock);
+	xfs_log_move_tail(ailp->xa_mount, tail_lsn);
+}
 
 
 /*
diff --git a/fs/xfs/xfs_trans_priv.h b/fs/xfs/xfs_trans_priv.h
index 07ea4b7..4ab7377 100644
--- a/fs/xfs/xfs_trans_priv.h
+++ b/fs/xfs/xfs_trans_priv.h
@@ -85,6 +85,10 @@ void			 xfs_trans_ail_update_bulk(struct xfs_ail *ailp,
 void			xfs_trans_ail_delete(struct xfs_ail *ailp,
 					struct xfs_log_item *lip)
 					__releases(ailp->xa_lock);
+void			xfs_trans_ail_delete_bulk(struct xfs_ail *ailp,
+					struct xfs_log_item **lgia,
+					int nr_items)
+					__releases(ailp->xa_lock);
 void			xfs_trans_ail_push(struct xfs_ail *, xfs_lsn_t);
 void			xfs_trans_unlocked_item(struct xfs_ail *,
 					xfs_log_item_t *);
-- 
1.7.2.3

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

[PATCH 8/8] xfs: use AIL bulk delete function to implement single delete

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

We now have two copies of AIL delete operations that are mostly
duplicate functionality. The single log item deletes can be
implemented via the bulk updates by turning xfs_trans_ail_delete()
into a simple wrapper. This removes all the duplicate delete
functionality and associated helpers.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/xfs_trans_ail.c  |   70 +++-------------------------------------------
 fs/xfs/xfs_trans_priv.h |   20 +++++++++-----
 2 files changed, 18 insertions(+), 72 deletions(-)

diff --git a/fs/xfs/xfs_trans_ail.c b/fs/xfs/xfs_trans_ail.c
index 2f44f92..91aaf7b 100644
--- a/fs/xfs/xfs_trans_ail.c
+++ b/fs/xfs/xfs_trans_ail.c
@@ -520,80 +520,21 @@ xfs_trans_ail_update_bulk(
 }
 
 /*
- * Delete the given item from the AIL.  It must already be in
- * the AIL.
- *
- * Wakeup anyone with an lsn less than item's lsn.    If the item
- * we delete in the AIL is the minimum one, update the tail lsn in the
- * log manager.
- *
- * Clear the IN_AIL flag from the item, reset its lsn to 0, and
- * bump the AIL's generation count to indicate that the tree
- * has changed.
- *
- * This function must be called with the AIL lock held.  The lock
- * is dropped before returning.
- */
-void
-xfs_trans_ail_delete(
-	struct xfs_ail	*ailp,
-	xfs_log_item_t	*lip) __releases(ailp->xa_lock)
-{
-	xfs_log_item_t		*mlip;
-	xfs_lsn_t		tail_lsn;
-
-	if (lip->li_flags & XFS_LI_IN_AIL) {
-		mlip = xfs_ail_min(ailp);
-		xfs_ail_delete(ailp, lip);
-
-
-		lip->li_flags &= ~XFS_LI_IN_AIL;
-		lip->li_lsn = 0;
-
-		if (mlip == lip) {
-			mlip = xfs_ail_min(ailp);
-			/*
-			 * It is not safe to access mlip after the AIL lock
-			 * is dropped, so we must get a copy of li_lsn
-			 * before we do so.  This is especially important
-			 * on 32-bit platforms where accessing and updating
-			 * 64-bit values like li_lsn is not atomic.
-			 */
-			tail_lsn = mlip ? mlip->li_lsn : 0;
-			spin_unlock(&ailp->xa_lock);
-			xfs_log_move_tail(ailp->xa_mount, tail_lsn);
-		} else {
-			spin_unlock(&ailp->xa_lock);
-		}
-	}
-	else {
-		/*
-		 * If the file system is not being shutdown, we are in
-		 * serious trouble if we get to this stage.
-		 */
-		struct xfs_mount	*mp = ailp->xa_mount;
-
-		spin_unlock(&ailp->xa_lock);
-		if (!XFS_FORCED_SHUTDOWN(mp)) {
-			xfs_cmn_err(XFS_PTAG_AILDELETE, CE_ALERT, mp,
-		"%s: attempting to delete a log item that is not in the AIL",
-					__func__);
-			xfs_force_shutdown(mp, SHUTDOWN_CORRUPT_INCORE);
-		}
-	}
-}
-
-/*
  * Bulk update version of xfs_trans_ail_delete
  *
  * This version takes an array of log items that all need to removed from the
  * AIL. The caller is already holding the AIL lock, and done all the checks
  * necessary to ensure the items passed in via @lgia are ready for deletion.
+ * If an item we delete in the AIL is the minimum one, update the tail lsn in
+ * the log manager.
  *
  * This function will not drop the AIL lock until all items are removed from
  * the AIL to minimise the amount of lock traffic on the AIL. This does not
  * greatly increase the AIL hold time, but does significantly reduce the amount
  * of traffic on the lock, especially during IO completion.
+ *
+ * This function must be called with the AIL lock held.  The lock is dropped
+ * before returning.
  */
 void
 xfs_trans_ail_delete_bulk(
@@ -648,7 +589,6 @@ xfs_trans_ail_delete_bulk(
 	xfs_log_move_tail(ailp->xa_mount, tail_lsn);
 }
 
-
 /*
  * The active item list (AIL) is a doubly linked list of log
  * items sorted by ascending lsn.  The base of the list is
diff --git a/fs/xfs/xfs_trans_priv.h b/fs/xfs/xfs_trans_priv.h
index 1cf42e7..2bd3c19 100644
--- a/fs/xfs/xfs_trans_priv.h
+++ b/fs/xfs/xfs_trans_priv.h
@@ -89,13 +89,19 @@ xfs_trans_ail_update(
 	xfs_trans_ail_update_bulk(ailp, lgia, 1, lsn);
 }
 
-void			xfs_trans_ail_delete(struct xfs_ail *ailp,
-					struct xfs_log_item *lip)
-					__releases(ailp->xa_lock);
-void			xfs_trans_ail_delete_bulk(struct xfs_ail *ailp,
-					struct xfs_log_item **lgia,
-					int nr_items)
-					__releases(ailp->xa_lock);
+void	xfs_trans_ail_delete_bulk(struct xfs_ail *ailp,
+				struct xfs_log_item **lgia, int nr_items)
+				__releases(ailp->xa_lock);
+static inline void
+xfs_trans_ail_delete(
+	struct xfs_ail	*ailp,
+	xfs_log_item_t	*lip) __releases(ailp->xa_lock)
+{
+	struct xfs_log_item	*lgia[1] = { lip, };
+
+	xfs_trans_ail_delete_bulk(ailp, lgia, 1);
+}
+
 void			xfs_trans_ail_push(struct xfs_ail *, xfs_lsn_t);
 void			xfs_trans_unlocked_item(struct xfs_ail *,
 					xfs_log_item_t *);
-- 
1.7.2.3

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

Re: [PATCH 1/8] xfs: Pull EFI/EFD handling out from under the AIL lock

[Christoph Hellwig]

 - xfs_efi_init needs to initialize efi_next_extent using ATOMIC_INIT
 - there is a behaviour change about the xfs_trans_del_item call
   in xfs_efi_item_unpin - before it was protected by the
   XFS_EFI_CANCELED which was never set, and now it's not.
 - what happened to XFS_EFI_RECOVERED?  You changed it to be indexed
   for the atomic bit-ops, but it's still used non-atomic in the log
   recovery code.
 - Why is XFS_EFI_COMMITTED cleared in xlog_recover_do_efi_trans,
   where it can't ever be set?
 - can you please add a shared helper for xfs_efi_item_unpin and
   xfs_efi_release, ala:

STATIC void
__xfs_efi_release(
	xfs_efi_log_item_t	*efip)
{
	if (!test_and_clear_bit(XFS_EFI_COMMITTED, &efip->efi_flags)) {
		struct xfs_ail          *ailp = efip->efi_item.li_ailp;

		spin_lock(&ailp->xa_lock);
		/* xfs_trans_ail_delete() drops the AIL lock. */
		xfs_trans_ail_delete(ailp, &efip->efi_item);
		xfs_efi_item_free(efip);
	}

   so that it's obvious they do the same release operation?

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

Re: [PATCH 2/8] xfs: clean up xfs_ail_delete()

[Christoph Hellwig]

On Mon, Nov 29, 2010 at 12:12:26PM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> xfs_ail_delete() has a needlessly complex interface. It returns the log item
> that was passed in for deletion (which the callers then assert is identical to
> the one passed in), and callers of xfs_ail_delete() still need to invalidate
> current traversal cursors.
> 
> Make xfs_ail_delete() return void, move the cursor invalidation inside it, and
> clean up the callers just to use the log item pointer they passed in.
> 
> While cleaning up, remove the messy and unnecessary "/* ARGUSED */" comments
> around all these functions.

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

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

Re: [PATCH 4/8] xfs: reduce the number of AIL push wakeups

[Christoph Hellwig]

On Mon, Nov 29, 2010 at 12:12:28PM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> The xfaild often tries to rest to wait for congestion to pass of for
> IO to complete, but is regularly woken in tail-pushing situations.
> In severe cases, the xfsaild is getting woken tens of thousands of
> times a second. Reduce the number needless wakeups by only waking
> the xfsaild if the new target is larger than the old one. Further
> make short sleeps uninterruptible as they occur when the xfsaild has
> decided it needs to back off to allow some IO to complete and being
> woken early is counter-productive.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/linux-2.6/xfs_super.c |   20 ++++++++++++++++----
>  1 files changed, 16 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/xfs/linux-2.6/xfs_super.c b/fs/xfs/linux-2.6/xfs_super.c
> index 6cbebb2..8546805 100644
> --- a/fs/xfs/linux-2.6/xfs_super.c
> +++ b/fs/xfs/linux-2.6/xfs_super.c
> @@ -834,8 +834,11 @@ xfsaild_wakeup(
>  	struct xfs_ail		*ailp,
>  	xfs_lsn_t		threshold_lsn)
>  {
> -	ailp->xa_target = threshold_lsn;
> -	wake_up_process(ailp->xa_task);
> +	/* only ever move the target forwards */
> +	if (XFS_LSN_CMP(threshold_lsn, ailp->xa_target) > 0) {
> +		ailp->xa_target = threshold_lsn;
> +		wake_up_process(ailp->xa_task);
> +	}
>  }
>  
>  STATIC int
> @@ -847,8 +850,17 @@ xfsaild(
>  	long		tout = 0; /* milliseconds */
>  
>  	while (!kthread_should_stop()) {
> -		schedule_timeout_interruptible(tout ?
> -				msecs_to_jiffies(tout) : MAX_SCHEDULE_TIMEOUT);
> +		/*
> +		 * for short sleeps indicating congestion, don't allow us to
> +		 * get woken early. Otherwise all we do is bang on the AIL lock
> +		 * without making progress.
> +		 */
> +		if (tout && tout <= 20)
> +			__set_current_state(TASK_KILLABLE);
> +		else
> +			__set_current_state(TASK_UNINTERRUPTIBLE);
> +		schedule_timeout(tout ?
> +				 msecs_to_jiffies(tout) : MAX_SCHEDULE_TIMEOUT);
>  
>  		/* swsusp */
>  		try_to_freeze();
> -- 
> 1.7.2.3
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
---end quoted text---

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

Re: [PATCH 5/8] xfs: consume iodone callback items on buffers as they are processed

[Christoph Hellwig]

On Mon, Nov 29, 2010 at 12:12:29PM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> To allow buffer iodone callbacks to consume multiple items off the
> callback list, first we need to convert the xfs_buf_do_callbacks()
> to consume items and always pull the next item from the head of the
> list.
> 
> The means the item list walk is never dependent on knowing the
> next item on the list and hence allows callbacks to remove itesm
> from the list as well. This allows callbacks to do bulk operations
> by scanning the list for identical callbacks, consuming them all
> and then processing them in bulk, negating the need for multiple
> callbacks of that type.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>

Looks good,


Reviewed-by: Christoph Hellwig <hch@lst.de>

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

Re: [PATCH 3/8] xfs: bulk AIL insertion during transaction commit

[Christoph Hellwig]

Generally looks good, a few minor comments below:

> +void
> +xfs_trans_committed_bulk(
> +	struct xfs_ail		*ailp,
> +	struct xfs_log_vec	*log_vector,
> +	xfs_lsn_t		commit_lsn,
> +	int			aborted)
> +{
> +#define LGIA_SIZE	32
> +	struct xfs_log_item	*lgia[LGIA_SIZE];

The lgia name seems a bit to obscure to me.  What about just log_items
as variable name, and LOG_ITEM_BATCH_SIZE or similar for the size?

> +			spin_lock(&ailp->xa_lock);
> +			xfs_trans_ail_update_bulk(ailp, lgia, LGIA_SIZE,
> +							commit_lsn);
> +			for (i = 0; i < LGIA_SIZE; i++)
> +				IOP_UNPIN(lgia[i], 0);

> +		spin_lock(&ailp->xa_lock);
> +		xfs_trans_ail_update_bulk(ailp, lgia, i, commit_lsn);
> +		for (j = 0; j < i; j++)
> +			IOP_UNPIN(lgia[j], 0);

It might be worth to factor these two little sniplets into a little
helper.

> + * Bulk update version of xfs_trans_ail_update.

That won't be a very useful comment anymore once xfs_trans_ail_update is
implemented as a wrapper around this function..

> +	struct xfs_log_item	**lgia,

Same naming comment here.

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

Re: [PATCH 1/8] xfs: Pull EFI/EFD handling out from under the AIL lock

[Dave Chinner]

On Tue, Nov 30, 2010 at 03:17:34PM -0500, Christoph Hellwig wrote:
> 
>  - xfs_efi_init needs to initialize efi_next_extent using ATOMIC_INIT

OK.

>  - there is a behaviour change about the xfs_trans_del_item call
>    in xfs_efi_item_unpin - before it was protected by the
>    XFS_EFI_CANCELED which was never set, and now it's not.

XFS_EFI_CANCELED has not been set in the code base since
xfs_efi_cancel() was removed back in 2006 by commit
065d312e15902976d256ddaf396a7950ec0350a8 ("[XFS] Remove unused
iop_abort log item operation), and even then xfs_efi_cancel() was
never called. I haven't tracked it back further than that (beyond
git history), but handling of efis in cancelled transactions has
been broken for a long time.

Basically, when we get an IOP_UNPIN(lip, 1); call from
xfs_trans_uncommit() (i.e. remove == 1), if we don't free the log
item descriptor we leak it. IOWs, the new behaviour introduced in
this patch is actually the correct behaviour.

>  - what happened to XFS_EFI_RECOVERED?  You changed it to be indexed
>    for the atomic bit-ops, but it's still used non-atomic in the log
>    recovery code.

Ah, I forgot to convert it.

>  - Why is XFS_EFI_COMMITTED cleared in xlog_recover_do_efi_trans,
>    where it can't ever be set?

It was just a straight transformation. I'll kill it.

>  - can you please add a shared helper for xfs_efi_item_unpin and
>    xfs_efi_release, ala:

Ok. Will do.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

Re: [PATCH 3/8] xfs: bulk AIL insertion during transaction commit

[Dave Chinner]

On Tue, Nov 30, 2010 at 05:40:57PM -0500, Christoph Hellwig wrote:
> Generally looks good, a few minor comments below:
> 
> > +void
> > +xfs_trans_committed_bulk(
> > +	struct xfs_ail		*ailp,
> > +	struct xfs_log_vec	*log_vector,
> > +	xfs_lsn_t		commit_lsn,
> > +	int			aborted)
> > +{
> > +#define LGIA_SIZE	32
> > +	struct xfs_log_item	*lgia[LGIA_SIZE];
> 
> The lgia name seems a bit to obscure to me.

lgia == "log item array"

> What about just log_items
> as variable name, and LOG_ITEM_BATCH_SIZE or similar for the size?

Ok, that's easy to do.

> > +			spin_lock(&ailp->xa_lock);
> > +			xfs_trans_ail_update_bulk(ailp, lgia, LGIA_SIZE,
> > +							commit_lsn);
> > +			for (i = 0; i < LGIA_SIZE; i++)
> > +				IOP_UNPIN(lgia[i], 0);
> 
> > +		spin_lock(&ailp->xa_lock);
> > +		xfs_trans_ail_update_bulk(ailp, lgia, i, commit_lsn);
> > +		for (j = 0; j < i; j++)
> > +			IOP_UNPIN(lgia[j], 0);
> 
> It might be worth to factor these two little sniplets into a little
> helper.

I thought about it, but didn't because there was more code in
writing a helper. I'll do it anyway...

> 
> > + * Bulk update version of xfs_trans_ail_update.
> 
> That won't be a very useful comment anymore once xfs_trans_ail_update is
> implemented as a wrapper around this function..

Yup, but when the wrapper is introduced the comment is updated
appropriately.

> 
> > +	struct xfs_log_item	**lgia,
> 
> Same naming comment here.

Will fix.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

Re: [PATCH 1/8] xfs: Pull EFI/EFD handling out from under the AIL lock

[Christoph Hellwig]

On Thu, Dec 02, 2010 at 12:28:41PM +1100, Dave Chinner wrote:
> >  - there is a behaviour change about the xfs_trans_del_item call
> >    in xfs_efi_item_unpin - before it was protected by the
> >    XFS_EFI_CANCELED which was never set, and now it's not.
> 
> XFS_EFI_CANCELED has not been set in the code base since
> xfs_efi_cancel() was removed back in 2006 by commit
> 065d312e15902976d256ddaf396a7950ec0350a8 ("[XFS] Remove unused
> iop_abort log item operation), and even then xfs_efi_cancel() was
> never called. I haven't tracked it back further than that (beyond
> git history), but handling of efis in cancelled transactions has
> been broken for a long time.
> 
> Basically, when we get an IOP_UNPIN(lip, 1); call from
> xfs_trans_uncommit() (i.e. remove == 1), if we don't free the log
> item descriptor we leak it. IOWs, the new behaviour introduced in
> this patch is actually the correct behaviour.

Maybe fix this issue first in a separate patch, instead of hiding it
in a bigger one.

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

Re: [PATCH 1/8] xfs: Pull EFI/EFD handling out from under the AIL lock

[Dave Chinner]

On Thu, Dec 02, 2010 at 06:38:49AM -0500, Christoph Hellwig wrote:
> On Thu, Dec 02, 2010 at 12:28:41PM +1100, Dave Chinner wrote:
> > >  - there is a behaviour change about the xfs_trans_del_item call
> > >    in xfs_efi_item_unpin - before it was protected by the
> > >    XFS_EFI_CANCELED which was never set, and now it's not.
> > 
> > XFS_EFI_CANCELED has not been set in the code base since
> > xfs_efi_cancel() was removed back in 2006 by commit
> > 065d312e15902976d256ddaf396a7950ec0350a8 ("[XFS] Remove unused
> > iop_abort log item operation), and even then xfs_efi_cancel() was
> > never called. I haven't tracked it back further than that (beyond
> > git history), but handling of efis in cancelled transactions has
> > been broken for a long time.
> > 
> > Basically, when we get an IOP_UNPIN(lip, 1); call from
> > xfs_trans_uncommit() (i.e. remove == 1), if we don't free the log
> > item descriptor we leak it. IOWs, the new behaviour introduced in
> > this patch is actually the correct behaviour.
> 
> Maybe fix this issue first in a separate patch, instead of hiding it
> in a bigger one.

Ok, I'll split it out.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

Re: [PATCH 6/8] xfs: remove all the inodes on a buffer from the AIL in bulk

[Christoph Hellwig]

While the patch looks good for the ail lock contetion removal, I don't
quite like the model with the double iteration over the log item list
on the buffer.  What do you think about the following plan:

 (1) merge xfs_istale_done into xfs_iflush_done by checking for
     XFS_ISTALE
 (2) convert not only the inode log item completion to your new scheme,
     but also the dquots
 (3) replace xfs_buf_do_callbacks with a callback in the buffer, which
     now points to the inode and dqout routines, or calls the completion
     for the only items in "normal" buf items.

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

Re: [PATCH 8/8] xfs: use AIL bulk delete function to implement single delete

[Christoph Hellwig]

> -/*
>   * Bulk update version of xfs_trans_ail_delete
>   *
>   * This version takes an array of log items that all need to removed from the
>   * AIL. The caller is already holding the AIL lock, and done all the checks

Not overly useful now that the non-bulk version is gone.

>   * necessary to ensure the items passed in via @lgia are ready for deletion.
> + * If an item we delete in the AIL is the minimum one, update the tail lsn in
> + * the log manager.
>   *
>   * This function will not drop the AIL lock until all items are removed from
>   * the AIL to minimise the amount of lock traffic on the AIL. This does not
>   * greatly increase the AIL hold time, but does significantly reduce the amount
>   * of traffic on the lock, especially during IO completion.
> + *
> + * This function must be called with the AIL lock held.  The lock is dropped
> + * before returning.

These should be in the patch introducing xfs_trans_ail_delete_bulk.

Otherwise looks good.

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

Re: [PATCH 6/8] xfs: remove all the inodes on a buffer from the AIL in bulk

[Dave Chinner]

On Mon, Dec 06, 2010 at 09:33:08AM -0500, Christoph Hellwig wrote:
> While the patch looks good for the ail lock contetion removal, I don't
> quite like the model with the double iteration over the log item list
> on the buffer.  What do you think about the following plan:
> 
>  (1) merge xfs_istale_done into xfs_iflush_done by checking for
>      XFS_ISTALE
>  (2) convert not only the inode log item completion to your new scheme,
>      but also the dquots
>  (3) replace xfs_buf_do_callbacks with a callback in the buffer, which
>      now points to the inode and dqout routines, or calls the completion
>      for the only items in "normal" buf items.

Seems like a reasonable approach. However, what I'd prefer to do is
make these changes as a separate set of changes on top of this patch
series rather than try to integrate them into the existing series.
If there are problems, that should make it more bisectable. Do you
have any concerns with such an approach?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

Re: [PATCH 6/8] xfs: remove all the inodes on a buffer from the AIL in bulk

[Christoph Hellwig]

On Tue, Dec 07, 2010 at 02:44:56PM +1100, Dave Chinner wrote:
> On Mon, Dec 06, 2010 at 09:33:08AM -0500, Christoph Hellwig wrote:
> > While the patch looks good for the ail lock contetion removal, I don't
> > quite like the model with the double iteration over the log item list
> > on the buffer.  What do you think about the following plan:
> > 
> >  (1) merge xfs_istale_done into xfs_iflush_done by checking for
> >      XFS_ISTALE
> >  (2) convert not only the inode log item completion to your new scheme,
> >      but also the dquots
> >  (3) replace xfs_buf_do_callbacks with a callback in the buffer, which
> >      now points to the inode and dqout routines, or calls the completion
> >      for the only items in "normal" buf items.
> 
> Seems like a reasonable approach. However, what I'd prefer to do is
> make these changes as a separate set of changes on top of this patch
> series rather than try to integrate them into the existing series.
> If there are problems, that should make it more bisectable. Do you
> have any concerns with such an approach?

Sounds fine, although I think getting rid of xfs_istale_done might be
worth doing before this patch.

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