[PATCH 0/4] fix AIL pushing under heavy concurrent metadata loads

[PATCH 0/4] fix AIL pushing under heavy concurrent metadata loads

[Christoph Hellwig]

This series fixes hangs under heavy concurrent metadata workloads.  The
underlying issues seem fairly old but only really started to show when
enabling delaylog and the workqueue based AIL pushed.

A big thanks to Stefan Priebe for reporting the problem, providing a
reproducer and test filesystem image as well as testing the patches
on his system.

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

[PATCH 1/4] xfs: do not update xa_last_pushed_lsn for locked items

[Christoph Hellwig]

[-- Attachment #1: xfs-ail-retry-locked-items --]
[-- Type: text/plain, Size: 1021 bytes --]

If an item was locked we should not update xa_last_pushed_lsn and thus skip
it when restarting the AIL scan as we need to be able to lock and write it
out as soon as possible.  Otherwise heavy lock contention might starve AIL
pushing too easily, especially given the larger backoff once we moved
xa_last_pushed_lsn all the way to the target lsn.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Reported-by: Stefan Priebe <s.priebe@profihost.ag>
Tested-by: Stefan Priebe <s.priebe@profihost.ag>

Index: xfs/fs/xfs/xfs_trans_ail.c
===================================================================
--- xfs.orig/fs/xfs/xfs_trans_ail.c	2011-10-06 10:31:29.855243550 -0400
+++ xfs/fs/xfs/xfs_trans_ail.c	2011-10-06 10:31:55.832744112 -0400
@@ -452,7 +452,6 @@ xfs_ail_worker(
 
 		case XFS_ITEM_LOCKED:
 			XFS_STATS_INC(xs_push_ail_locked);
-			ailp->xa_last_pushed_lsn = lsn;
 			stuck++;
 			break;
 

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

Re: [PATCH 1/4] xfs: do not update xa_last_pushed_lsn for locked items

[Alex Elder]

On Thu, 2011-10-06 at 14:32 -0400, Christoph Hellwig wrote:
> If an item was locked we should not update xa_last_pushed_lsn and thus skip
> it when restarting the AIL scan as we need to be able to lock and write it
> out as soon as possible.  Otherwise heavy lock contention might starve AIL
> pushing too easily, especially given the larger backoff once we moved
> xa_last_pushed_lsn all the way to the target lsn.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Reported-by: Stefan Priebe <s.priebe@profihost.ag>
> Tested-by: Stefan Priebe <s.priebe@profihost.ag>

I remember wondering about that one line during review, but
I believe I reasoned something about the "already being
reflushed or relogged" made it the right thing to do.

Your explanation makes sense though (but what do I know,
the original code seemed OK too...).

Reviewed-by: Alex Elder <aelder@sgi.com>


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

Re: [PATCH 1/4] xfs: do not update xa_last_pushed_lsn for locked items

[Dave Chinner]

On Thu, Oct 06, 2011 at 02:32:58PM -0400, Christoph Hellwig wrote:
> If an item was locked we should not update xa_last_pushed_lsn and thus skip
> it when restarting the AIL scan as we need to be able to lock and write it
> out as soon as possible.  Otherwise heavy lock contention might starve AIL
> pushing too easily, especially given the larger backoff once we moved
> xa_last_pushed_lsn all the way to the target lsn.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Reported-by: Stefan Priebe <s.priebe@profihost.ag>
> Tested-by: Stefan Priebe <s.priebe@profihost.ag>

Reviewed-by: Dave Chinner <dchinner@redhat.com>
-- 
Dave Chinner
david@fromorbit.com

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

[PATCH 2/4] xfs: force the log if we encounter pinned buffers in .iop_pushbuf

[Christoph Hellwig]

[-- Attachment #1: xfs-fix-inode-ail-pushing-3 --]
[-- Type: text/plain, Size: 4801 bytes --]

We need to check for pinned buffers even in .iop_pushbuf given that inode
items flush into the same buffers that may be pinned directly due operations
on the unlinked inode list operating directly on buffers.  To do this add a
return value to .iop_pushbuf that tells the AIL push about this and use
the existing log force mechanisms to unpin it.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Reported-by: Stefan Priebe <s.priebe@profihost.ag>
Tested-by: Stefan Priebe <s.priebe@profihost.ag>

Index: xfs/fs/xfs/xfs_dquot_item.c
===================================================================
--- xfs.orig/fs/xfs/xfs_dquot_item.c	2011-10-06 14:10:53.926919219 -0400
+++ xfs/fs/xfs/xfs_dquot_item.c	2011-10-06 14:15:40.642918500 -0400
@@ -183,13 +183,14 @@ xfs_qm_dqunpin_wait(
  * search the buffer cache can be a time consuming thing, and AIL lock is a
  * spinlock.
  */
-STATIC void
+STATIC bool
 xfs_qm_dquot_logitem_pushbuf(
 	struct xfs_log_item	*lip)
 {
 	struct xfs_dq_logitem	*qlip = DQUOT_ITEM(lip);
 	struct xfs_dquot	*dqp = qlip->qli_dquot;
 	struct xfs_buf		*bp;
+	bool			ret = true;
 
 	ASSERT(XFS_DQ_IS_LOCKED(dqp));
 
@@ -201,17 +202,20 @@ xfs_qm_dquot_logitem_pushbuf(
 	if (completion_done(&dqp->q_flush) ||
 	    !(lip->li_flags & XFS_LI_IN_AIL)) {
 		xfs_dqunlock(dqp);
-		return;
+		return true;
 	}
 
 	bp = xfs_incore(dqp->q_mount->m_ddev_targp, qlip->qli_format.qlf_blkno,
 			dqp->q_mount->m_quotainfo->qi_dqchunklen, XBF_TRYLOCK);
 	xfs_dqunlock(dqp);
 	if (!bp)
-		return;
+		return true;
 	if (XFS_BUF_ISDELAYWRITE(bp))
 		xfs_buf_delwri_promote(bp);
+	if (xfs_buf_ispinned(bp))
+		ret = false;
 	xfs_buf_relse(bp);
+	return ret;
 }
 
 /*
Index: xfs/fs/xfs/xfs_buf_item.c
===================================================================
--- xfs.orig/fs/xfs/xfs_buf_item.c	2011-10-06 14:10:53.934920468 -0400
+++ xfs/fs/xfs/xfs_buf_item.c	2011-10-06 14:15:40.642918500 -0400
@@ -629,7 +629,7 @@ xfs_buf_item_push(
  * the xfsbufd to get this buffer written. We have to unlock the buffer
  * to allow the xfsbufd to write it, too.
  */
-STATIC void
+STATIC bool
 xfs_buf_item_pushbuf(
 	struct xfs_log_item	*lip)
 {
@@ -643,6 +643,7 @@ xfs_buf_item_pushbuf(
 
 	xfs_buf_delwri_promote(bp);
 	xfs_buf_relse(bp);
+	return true;
 }
 
 STATIC void
Index: xfs/fs/xfs/xfs_inode_item.c
===================================================================
--- xfs.orig/fs/xfs/xfs_inode_item.c	2011-10-06 14:10:53.946918636 -0400
+++ xfs/fs/xfs/xfs_inode_item.c	2011-10-06 14:15:40.646917968 -0400
@@ -706,13 +706,14 @@ xfs_inode_item_committed(
  * marked delayed write. If that's the case, we'll promote it and that will
  * allow the caller to write the buffer by triggering the xfsbufd to run.
  */
-STATIC void
+STATIC bool
 xfs_inode_item_pushbuf(
 	struct xfs_log_item	*lip)
 {
 	struct xfs_inode_log_item *iip = INODE_ITEM(lip);
 	struct xfs_inode	*ip = iip->ili_inode;
 	struct xfs_buf		*bp;
+	bool			ret = true;
 
 	ASSERT(xfs_isilocked(ip, XFS_ILOCK_SHARED));
 
@@ -723,7 +724,7 @@ xfs_inode_item_pushbuf(
 	if (completion_done(&ip->i_flush) ||
 	    !(lip->li_flags & XFS_LI_IN_AIL)) {
 		xfs_iunlock(ip, XFS_ILOCK_SHARED);
-		return;
+		return true;
 	}
 
 	bp = xfs_incore(ip->i_mount->m_ddev_targp, iip->ili_format.ilf_blkno,
@@ -731,10 +732,13 @@ xfs_inode_item_pushbuf(
 
 	xfs_iunlock(ip, XFS_ILOCK_SHARED);
 	if (!bp)
-		return;
+		return true;
 	if (XFS_BUF_ISDELAYWRITE(bp))
 		xfs_buf_delwri_promote(bp);
+	if (xfs_buf_ispinned(bp))
+		ret = false;
 	xfs_buf_relse(bp);
+	return ret;
 }
 
 /*
Index: xfs/fs/xfs/xfs_trans.h
===================================================================
--- xfs.orig/fs/xfs/xfs_trans.h	2011-10-06 14:10:53.954919795 -0400
+++ xfs/fs/xfs/xfs_trans.h	2011-10-06 14:15:40.646917968 -0400
@@ -350,7 +350,7 @@ typedef struct xfs_item_ops {
 	void (*iop_unlock)(xfs_log_item_t *);
 	xfs_lsn_t (*iop_committed)(xfs_log_item_t *, xfs_lsn_t);
 	void (*iop_push)(xfs_log_item_t *);
-	void (*iop_pushbuf)(xfs_log_item_t *);
+	bool (*iop_pushbuf)(xfs_log_item_t *);
 	void (*iop_committing)(xfs_log_item_t *, xfs_lsn_t);
 } xfs_item_ops_t;
 
Index: xfs/fs/xfs/xfs_trans_ail.c
===================================================================
--- xfs.orig/fs/xfs/xfs_trans_ail.c	2011-10-06 14:10:53.962919265 -0400
+++ xfs/fs/xfs/xfs_trans_ail.c	2011-10-06 14:20:46.730936535 -0400
@@ -439,8 +439,13 @@ xfs_ail_worker(
 
 		case XFS_ITEM_PUSHBUF:
 			XFS_STATS_INC(xs_push_ail_pushbuf);
-			IOP_PUSHBUF(lip);
-			ailp->xa_last_pushed_lsn = lsn;
+
+			if (!IOP_PUSHBUF(lip)) {
+				stuck++;
+				ailp->xa_log_flush++;
+			} else {
+				ailp->xa_last_pushed_lsn = lsn;
+			}
 			push_xfsbufd = 1;
 			break;
 

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

Re: [PATCH 2/4] xfs: force the log if we encounter pinned buffers in .iop_pushbuf

[Alex Elder]

On Thu, 2011-10-06 at 14:32 -0400, Christoph Hellwig wrote:
> We need to check for pinned buffers even in .iop_pushbuf given that inode
> items flush into the same buffers that may be pinned directly due operations
> on the unlinked inode list operating directly on buffers.  To do this add a
> return value to .iop_pushbuf that tells the AIL push about this and use
> the existing log force mechanisms to unpin it.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Reported-by: Stefan Priebe <s.priebe@profihost.ag>
> Tested-by: Stefan Priebe <s.priebe@profihost.ag>

Looks ok to me.

Reviewed-by: Alex Elder <aelder@sgi.com>


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

Re: [PATCH 2/4] xfs: force the log if we encounter pinned buffers in .iop_pushbuf

[Dave Chinner]

On Thu, Oct 06, 2011 at 02:32:59PM -0400, Christoph Hellwig wrote:
> We need to check for pinned buffers even in .iop_pushbuf given that inode
> items flush into the same buffers that may be pinned directly due operations
> on the unlinked inode list operating directly on buffers.  To do this add a
> return value to .iop_pushbuf that tells the AIL push about this and use
> the existing log force mechanisms to unpin it.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Reported-by: Stefan Priebe <s.priebe@profihost.ag>
> Tested-by: Stefan Priebe <s.priebe@profihost.ag>

Reviewed-by: Dave Chinner <dchinner@redhat.com>

-- 
Dave Chinner
david@fromorbit.com

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

[PATCH 3/4] xfs: revert to using a kthread for AIL pushing

[Christoph Hellwig]

[-- Attachment #1: xfs-ail-revert-to-kthread --]
[-- Type: text/plain, Size: 6481 bytes --]

Currently we have a few issues with the way the workqueue code is used to
implement AIL pushing:

 - it accidentally uses the same workqueue as the syncer action, and thus
   can be prevented from running if there are enough sync actions active
   in the system.
 - it doesn't use the HIGHPRI flag to queue at the head of the queue of
   work items

At this point I'm not confident enough in getting all the workqueue flags and
tweaks right to provide a perfectly reliable execution context for AIL
pushing, which is the most important piece in XFS to make forward progress
when the log fills.

Revert back to use a kthread per filesystem which fixes all the above issues
at the cost of having a task struct and stack around for each mounted
filesystem.  In addition this also gives us much better ways to diagnose
any issues involving hung AIL pushing and removes a small amount of code.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Reported-by: Stefan Priebe <s.priebe@profihost.ag>
Tested-by: Stefan Priebe <s.priebe@profihost.ag>

Index: xfs/fs/xfs/xfs_super.c
===================================================================
--- xfs.orig/fs/xfs/xfs_super.c	2011-10-06 14:10:53.810920565 -0400
+++ xfs/fs/xfs/xfs_super.c	2011-10-06 14:22:18.951422706 -0400
@@ -1648,24 +1648,13 @@ xfs_init_workqueues(void)
 	 */
 	xfs_syncd_wq = alloc_workqueue("xfssyncd", WQ_CPU_INTENSIVE, 8);
 	if (!xfs_syncd_wq)
-		goto out;
-
-	xfs_ail_wq = alloc_workqueue("xfsail", WQ_CPU_INTENSIVE, 8);
-	if (!xfs_ail_wq)
-		goto out_destroy_syncd;
-
+		return -ENOMEM;
 	return 0;
-
-out_destroy_syncd:
-	destroy_workqueue(xfs_syncd_wq);
-out:
-	return -ENOMEM;
 }
 
 STATIC void
 xfs_destroy_workqueues(void)
 {
-	destroy_workqueue(xfs_ail_wq);
 	destroy_workqueue(xfs_syncd_wq);
 }
 
Index: xfs/fs/xfs/xfs_trans_ail.c
===================================================================
--- xfs.orig/fs/xfs/xfs_trans_ail.c	2011-10-06 14:20:46.730936535 -0400
+++ xfs/fs/xfs/xfs_trans_ail.c	2011-10-06 14:22:18.955418059 -0400
@@ -28,8 +28,6 @@
 #include "xfs_trans_priv.h"
 #include "xfs_error.h"
 
-struct workqueue_struct	*xfs_ail_wq;	/* AIL workqueue */
-
 #ifdef DEBUG
 /*
  * Check that the list is sorted as it should be.
@@ -356,16 +354,10 @@ xfs_ail_delete(
 	xfs_trans_ail_cursor_clear(ailp, lip);
 }
 
-/*
- * xfs_ail_worker does the work of pushing on the AIL. It will requeue itself
- * to run at a later time if there is more work to do to complete the push.
- */
-STATIC void
-xfs_ail_worker(
-	struct work_struct	*work)
+static long
+xfsaild_push(
+	struct xfs_ail		*ailp)
 {
-	struct xfs_ail		*ailp = container_of(to_delayed_work(work),
-					struct xfs_ail, xa_work);
 	xfs_mount_t		*mp = ailp->xa_mount;
 	struct xfs_ail_cursor	cur;
 	xfs_log_item_t		*lip;
@@ -508,20 +500,6 @@ out_done:
 		ailp->xa_last_pushed_lsn = 0;
 		ailp->xa_log_flush = 0;
 
-		/*
-		 * We clear the XFS_AIL_PUSHING_BIT first before checking
-		 * whether the target has changed. If the target has changed,
-		 * this pushes the requeue race directly onto the result of the
-		 * atomic test/set bit, so we are guaranteed that either the
-		 * the pusher that changed the target or ourselves will requeue
-		 * the work (but not both).
-		 */
-		clear_bit(XFS_AIL_PUSHING_BIT, &ailp->xa_flags);
-		smp_rmb();
-		if (XFS_LSN_CMP(ailp->xa_target, target) == 0 ||
-		    test_and_set_bit(XFS_AIL_PUSHING_BIT, &ailp->xa_flags))
-			return;
-
 		tout = 50;
 	} else if (XFS_LSN_CMP(lsn, target) >= 0) {
 		/*
@@ -548,9 +526,30 @@ out_done:
 		ailp->xa_last_pushed_lsn = 0;
 	}
 
-	/* There is more to do, requeue us.  */
-	queue_delayed_work(xfs_syncd_wq, &ailp->xa_work,
-					msecs_to_jiffies(tout));
+	return tout;
+}
+
+static int
+xfsaild(
+	void		*data)
+{
+	struct xfs_ail	*ailp = data;
+	long		tout = 0;	/* milliseconds */
+
+	while (!kthread_should_stop()) {
+		if (tout && tout <= 20)
+			__set_current_state(TASK_KILLABLE);
+		else
+			__set_current_state(TASK_INTERRUPTIBLE);
+		schedule_timeout(tout ?
+				 msecs_to_jiffies(tout) : MAX_SCHEDULE_TIMEOUT);
+
+		try_to_freeze();
+
+		tout = xfsaild_push(ailp);
+	}
+
+	return 0;
 }
 
 /*
@@ -585,8 +584,9 @@ xfs_ail_push(
 	 */
 	smp_wmb();
 	xfs_trans_ail_copy_lsn(ailp, &ailp->xa_target, &threshold_lsn);
-	if (!test_and_set_bit(XFS_AIL_PUSHING_BIT, &ailp->xa_flags))
-		queue_delayed_work(xfs_syncd_wq, &ailp->xa_work, 0);
+	smp_wmb();
+
+	wake_up_process(ailp->xa_task);
 }
 
 /*
@@ -824,9 +824,18 @@ xfs_trans_ail_init(
 	INIT_LIST_HEAD(&ailp->xa_ail);
 	INIT_LIST_HEAD(&ailp->xa_cursors);
 	spin_lock_init(&ailp->xa_lock);
-	INIT_DELAYED_WORK(&ailp->xa_work, xfs_ail_worker);
+
+	ailp->xa_task = kthread_run(xfsaild, ailp, "xfsaild/%s",
+			ailp->xa_mount->m_fsname);
+	if (IS_ERR(ailp->xa_task))
+		goto out_free_ailp;
+
 	mp->m_ail = ailp;
 	return 0;
+
+out_free_ailp:
+	kmem_free(ailp);
+	return ENOMEM;
 }
 
 void
@@ -835,6 +844,6 @@ xfs_trans_ail_destroy(
 {
 	struct xfs_ail	*ailp = mp->m_ail;
 
-	cancel_delayed_work_sync(&ailp->xa_work);
+	kthread_stop(ailp->xa_task);
 	kmem_free(ailp);
 }
Index: xfs/fs/xfs/xfs_trans_priv.h
===================================================================
--- xfs.orig/fs/xfs/xfs_trans_priv.h	2011-10-06 14:10:53.826919208 -0400
+++ xfs/fs/xfs/xfs_trans_priv.h	2011-10-06 14:22:18.963416965 -0400
@@ -64,24 +64,19 @@ struct xfs_ail_cursor {
  */
 struct xfs_ail {
 	struct xfs_mount	*xa_mount;
+	struct task_struct	*xa_task;
 	struct list_head	xa_ail;
 	xfs_lsn_t		xa_target;
 	struct list_head	xa_cursors;
 	spinlock_t		xa_lock;
-	struct delayed_work	xa_work;
 	xfs_lsn_t		xa_last_pushed_lsn;
-	unsigned long		xa_flags;
 	int			xa_log_flush;
 };
 
-#define XFS_AIL_PUSHING_BIT	0
 
 /*
  * From xfs_trans_ail.c
  */
-
-extern struct workqueue_struct	*xfs_ail_wq;	/* AIL workqueue */
-
 void	xfs_trans_ail_update_bulk(struct xfs_ail *ailp,
 				struct xfs_ail_cursor *cur,
 				struct xfs_log_item **log_items, int nr_items,
Index: xfs/fs/xfs/xfs_linux.h
===================================================================
--- xfs.orig/fs/xfs/xfs_linux.h	2011-10-06 14:10:53.838923105 -0400
+++ xfs/fs/xfs/xfs_linux.h	2011-10-06 14:22:18.967417308 -0400
@@ -68,6 +68,8 @@
 #include <linux/ctype.h>
 #include <linux/writeback.h>
 #include <linux/capability.h>
+#include <linux/kthread.h>
+#include <linux/freezer.h>
 #include <linux/list_sort.h>
 
 #include <asm/page.h>

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

Re: [PATCH 3/4] xfs: revert to using a kthread for AIL pushing

[Alex Elder]

On Thu, 2011-10-06 at 14:33 -0400, Christoph Hellwig wrote:
> Currently we have a few issues with the way the workqueue code is used to
> implement AIL pushing:
> 
>  - it accidentally uses the same workqueue as the syncer action, and thus
>    can be prevented from running if there are enough sync actions active
>    in the system.
>  - it doesn't use the HIGHPRI flag to queue at the head of the queue of
>    work items
> 
> At this point I'm not confident enough in getting all the workqueue flags and
> tweaks right to provide a perfectly reliable execution context for AIL
> pushing, which is the most important piece in XFS to make forward progress
> when the log fills.
> 
> Revert back to use a kthread per filesystem which fixes all the above issues
> at the cost of having a task struct and stack around for each mounted
> filesystem.  In addition this also gives us much better ways to diagnose
> any issues involving hung AIL pushing and removes a small amount of code.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Reported-by: Stefan Priebe <s.priebe@profihost.ag>
> Tested-by: Stefan Priebe <s.priebe@profihost.ag>

Looks good.  Dave, I don't want to commit this (series) until you
have had a chance to review it.

Reviewed-by: Alex Elder <aelder@sgi.com>

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

Re: [PATCH 3/4] xfs: revert to using a kthread for AIL pushing

[Dave Chinner]

On Thu, Oct 06, 2011 at 02:33:00PM -0400, Christoph Hellwig wrote:
> Currently we have a few issues with the way the workqueue code is used to
> implement AIL pushing:
> 
>  - it accidentally uses the same workqueue as the syncer action, and thus
>    can be prevented from running if there are enough sync actions active
>    in the system.
>  - it doesn't use the HIGHPRI flag to queue at the head of the queue of
>    work items
> 
> At this point I'm not confident enough in getting all the workqueue flags and
> tweaks right to provide a perfectly reliable execution context for AIL
> pushing, which is the most important piece in XFS to make forward progress
> when the log fills.
> 
> Revert back to use a kthread per filesystem which fixes all the above issues
> at the cost of having a task struct and stack around for each mounted
> filesystem.  In addition this also gives us much better ways to diagnose
> any issues involving hung AIL pushing and removes a small amount of code.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Reported-by: Stefan Priebe <s.priebe@profihost.ag>
> Tested-by: Stefan Priebe <s.priebe@profihost.ag>

I'd much prefer to fix the problems with the workqueue usage than
revert back to using a thread, but seeing as I cannot reproduce the
hangs I can't really track down whatever problem there is. So,
a bit reluctantly:

Reviewed-by: Dave Chinner <dchinner@redhat.com>
-- 
Dave Chinner
david@fromorbit.com

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

Re: [PATCH 3/4] xfs: revert to using a kthread for AIL pushing

[Markus Trippelsdorf]

On 2011.10.10 at 12:45 +1100, Dave Chinner wrote:
> On Thu, Oct 06, 2011 at 02:33:00PM -0400, Christoph Hellwig wrote:
> > Currently we have a few issues with the way the workqueue code is used to
> > implement AIL pushing:
> > 
> >  - it accidentally uses the same workqueue as the syncer action, and thus
> >    can be prevented from running if there are enough sync actions active
> >    in the system.
> >  - it doesn't use the HIGHPRI flag to queue at the head of the queue of
> >    work items
> > 
> > At this point I'm not confident enough in getting all the workqueue flags and
> > tweaks right to provide a perfectly reliable execution context for AIL
> > pushing, which is the most important piece in XFS to make forward progress
> > when the log fills.
> > 
> > Revert back to use a kthread per filesystem which fixes all the above issues
> > at the cost of having a task struct and stack around for each mounted
> > filesystem.  In addition this also gives us much better ways to diagnose
> > any issues involving hung AIL pushing and removes a small amount of code.
> > 
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > Reported-by: Stefan Priebe <s.priebe@profihost.ag>
> > Tested-by: Stefan Priebe <s.priebe@profihost.ag>
> 
> I'd much prefer to fix the problems with the workqueue usage than
> revert back to using a thread, but seeing as I cannot reproduce the
> hangs I can't really track down whatever problem there is. So,
> a bit reluctantly:

Wouldn't it be possible to verify that the problem also goes away with
this simple one liner?

diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index 2366c54..daf30c9 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -1654,7 +1654,7 @@ xfs_init_workqueues(void)
 	if (!xfs_syncd_wq)
 		goto out;
 
-	xfs_ail_wq = alloc_workqueue("xfsail", WQ_CPU_INTENSIVE, 8);
+	xfs_ail_wq = alloc_workqueue("xfsail", WQ_HIGHPRI | WQ_CPU_INTENSIVE, 8);
 	if (!xfs_ail_wq)
 		goto out_destroy_syncd;
 
>From Documentation/workqueue.txt:

  WQ_HIGHPRI | WQ_CPU_INTENSIVE

	This combination makes the wq avoid interaction with
	concurrency management completely and behave as a simple
	per-CPU execution context provider.  Work items queued on a
	highpri CPU-intensive wq start execution as soon as resources
	are available and don't affect execution of other work items.

So this should be identical to reverting back to the kthread. No?
CCing Tejun, maybe he can comment on this?

-- 
Markus

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

Re: [PATCH 3/4] xfs: revert to using a kthread for AIL pushing

[Stefan Priebe - Profihost AG]

Am 10.10.2011 07:55, schrieb Markus Trippelsdorf:
> Wouldn't it be possible to verify that the problem also goes away with
> this simple one liner?
>
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index 2366c54..daf30c9 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -1654,7 +1654,7 @@ xfs_init_workqueues(void)
>   	if (!xfs_syncd_wq)
>   		goto out;
>
> -	xfs_ail_wq = alloc_workqueue("xfsail", WQ_CPU_INTENSIVE, 8);
> +	xfs_ail_wq = alloc_workqueue("xfsail", WQ_HIGHPRI | WQ_CPU_INTENSIVE, 8);
>   	if (!xfs_ail_wq)
>   		goto out_destroy_syncd;
>
>  From Documentation/workqueue.txt:
>
>    WQ_HIGHPRI | WQ_CPU_INTENSIVE
>
> 	This combination makes the wq avoid interaction with
> 	concurrency management completely and behave as a simple
> 	per-CPU execution context provider.  Work items queued on a
> 	highpri CPU-intensive wq start execution as soon as resources
> 	are available and don't affect execution of other work items.
>
> So this should be identical to reverting back to the kthread. No?
> CCing Tejun, maybe he can comment on this?

We already tested this patch and it still fails / deadlocks:

diff --git a/fs/xfs/linux-2.6/xfs_super.c b/fs/xfs/linux-2.6/xfs_super.c
index a1a881e..6377f51 100644
--- a/fs/xfs/linux-2.6/xfs_super.c
+++ b/fs/xfs/linux-2.6/xfs_super.c
@@ -1669,7 +1669,7 @@ xfs_init_workqueues(void)
  	if (!xfs_syncd_wq)
  		goto out;

-	xfs_ail_wq = alloc_workqueue("xfsail", WQ_CPU_INTENSIVE, 8);
+	xfs_ail_wq = alloc_workqueue("xfsail", WQ_MEM_RECLAIM | WQ_HIGHPRI, 512);
  	if (!xfs_ail_wq)
  		goto out_destroy_syncd;

diff --git a/fs/xfs/xfs_trans_ail.c b/fs/xfs/xfs_trans_ail.c
index 953b142..638ea8b 100644
--- a/fs/xfs/xfs_trans_ail.c
+++ b/fs/xfs/xfs_trans_ail.c
@@ -600,7 +600,7 @@ out_done:
  	}

  	/* There is more to do, requeue us.  */
-	queue_delayed_work(xfs_syncd_wq, &ailp->xa_work,
+	queue_delayed_work(xfs_ail_wq, &ailp->xa_work,
  					msecs_to_jiffies(tout));
  }

@@ -637,7 +637,7 @@ xfs_ail_push(
  	smp_wmb();
  	xfs_trans_ail_copy_lsn(ailp, &ailp->xa_target, &threshold_lsn);
  	if (!test_and_set_bit(XFS_AIL_PUSHING_BIT, &ailp->xa_flags))
-		queue_delayed_work(xfs_syncd_wq, &ailp->xa_work, 0);
+		queue_delayed_work(xfs_ail_wq, &ailp->xa_work, 0);
  }

  /*


Stefan

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

Re: [PATCH 3/4] xfs: revert to using a kthread for AIL pushing

[Christoph Hellwig]

On Mon, Oct 10, 2011 at 07:55:46AM +0200, Markus Trippelsdorf wrote:
> Wouldn't it be possible to verify that the problem also goes away with
> this simple one liner?

We've been through a few variants, and none fixed it while Stefan had
to try them on production machines.

To be honest I'm not convinced at all that a workqueue was such a good
idea for the ail in particular.  It works extremly well for things were
we can easily define a work item, e.g. an object that gets queued up
and a method on it gets exectured.  But for the AIL we really have
a changing target that needs more or less constant pushing, and the
target keeps changing while executing our work.  Conceptually it fits
the idea of an thread much better, with the added benefit of not relying
on finding a combination of workqueue flags that gets the exact
behaviour (exectuion ASAP without any limits because of other items
or required memory allocation).

And unlike the various per-cpu threads we used to have it is only one
thread per filesystem anyway.

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

Re: [PATCH 3/4] xfs: revert to using a kthread for AIL pushing

[Tejun Heo]

Hello,

On Mon, Oct 10, 2011 at 09:26:11AM -0400, Christoph Hellwig wrote:
> On Mon, Oct 10, 2011 at 07:55:46AM +0200, Markus Trippelsdorf wrote:
> > Wouldn't it be possible to verify that the problem also goes away with
> > this simple one liner?
> 
> We've been through a few variants, and none fixed it while Stefan had
> to try them on production machines.
> 
> To be honest I'm not convinced at all that a workqueue was such a good
> idea for the ail in particular.  It works extremly well for things were
> we can easily define a work item, e.g. an object that gets queued up
> and a method on it gets exectured.  But for the AIL we really have
> a changing target that needs more or less constant pushing, and the
> target keeps changing while executing our work.  Conceptually it fits
> the idea of an thread much better, with the added benefit of not relying
> on finding a combination of workqueue flags that gets the exact
> behaviour (exectuion ASAP without any limits because of other items
> or required memory allocation).
> 
> And unlike the various per-cpu threads we used to have it is only one
> thread per filesystem anyway.

I don't know xfs internals at all so I don't have too strong an
opinion at this point but don't we at least need to understand what's
going on?  CPU_INTENSIVE / HIGHPRI flags shouldn't cause deadlock
unless some work items are doing busy looping waiting for another work
item to do something (busy yielding might achieve similar effect tho).
They don't change forward progress guarantee.

The only thing which can cause stall is lack of MEM_RECLAIM.  One
thing to be careful about is that each wq has only one rescuer, so if
more than one work items have inter-dependency, it might still lead to
deadlock and they need to be served by different workqueues.

The reasons for moving away from using kthread directly are two folded
- resources and correctness.  I've gone through a number of kthread
users during auditing freezer usage recently and more than half of
them get the synchronization against kthread_stop() or freezer wrong
(to be fair, the rules are quite tricky).  The problem with those bugs
is that they are really obscure race conditions and won't trigger
easily.

Thank you.

-- 
tejun

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

Re: [PATCH 3/4] xfs: revert to using a kthread for AIL pushing

[Stefan Priebe - Profihost AG]

> On 2011.10.10 at 12:45 +1100, Dave Chinner wrote:
>> On Thu, Oct 06, 2011 at 02:33:00PM -0400, Christoph Hellwig wrote:
>>> Currently we have a few issues with the way the workqueue code is used to
>>> implement AIL pushing:
>>>
>>>   - it accidentally uses the same workqueue as the syncer action, and thus
>>>     can be prevented from running if there are enough sync actions active
>>>     in the system.
>>>   - it doesn't use the HIGHPRI flag to queue at the head of the queue of
>>>     work items
>>>
>>> At this point I'm not confident enough in getting all the workqueue flags and
>>> tweaks right to provide a perfectly reliable execution context for AIL
>>> pushing, which is the most important piece in XFS to make forward progress
>>> when the log fills.
>>>
>>> Revert back to use a kthread per filesystem which fixes all the above issues
>>> at the cost of having a task struct and stack around for each mounted
>>> filesystem.  In addition this also gives us much better ways to diagnose
>>> any issues involving hung AIL pushing and removes a small amount of code.
>>>
>>> Signed-off-by: Christoph Hellwig<hch@lst.de>
>>> Reported-by: Stefan Priebe<s.priebe@profihost.ag>
>>> Tested-by: Stefan Priebe<s.priebe@profihost.ag>
>>
>> I'd much prefer to fix the problems with the workqueue usage than
>> revert back to using a thread, but seeing as I cannot reproduce the
>> hangs I can't really track down whatever problem there is. So,
>> a bit reluctantly:

Any news on this problem? What happens with the next long term stable 
kernel 3.0.X? How do you proceed with this bug?

Stefan

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

Re: [PATCH 3/4] xfs: revert to using a kthread for AIL pushing

[Christoph Hellwig]

On Wed, Oct 19, 2011 at 01:16:23PM +0200, Stefan Priebe - Profihost AG wrote:
> Any news on this problem? What happens with the next long term
> stable kernel 3.0.X? How do you proceed with this bug?

The patches are in 3.1-rc10 and I have submitted them for 3.0-stable,
but haven't heard anything back from Greg yet.

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

Re: [PATCH 3/4] xfs: revert to using a kthread for AIL pushing

[Stefan Priebe - Profihost AG]

Am 19.10.2011 13:34, schrieb Christoph Hellwig:
> On Wed, Oct 19, 2011 at 01:16:23PM +0200, Stefan Priebe - Profihost AG wrote:
>> Any news on this problem? What happens with the next long term
>> stable kernel 3.0.X? How do you proceed with this bug?
>
> The patches are in 3.1-rc10 and I have submitted them for 3.0-stable,
> but haven't heard anything back from Greg yet.
>
Thanks!

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

[PATCH 4/4] xfs: add AIL pushing tracepoints

[Christoph Hellwig]

[-- Attachment #1: xfs-ail-tracing --]
[-- Type: text/plain, Size: 3128 bytes --]

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

Index: xfs/fs/xfs/xfs_trace.h
===================================================================
--- xfs.orig/fs/xfs/xfs_trace.h	2011-10-06 14:10:53.710918092 -0400
+++ xfs/fs/xfs/xfs_trace.h	2011-10-06 14:26:25.274918999 -0400
@@ -30,6 +30,7 @@ struct xfs_buf_log_item;
 struct xfs_da_args;
 struct xfs_da_node_entry;
 struct xfs_dquot;
+struct xfs_log_item;
 struct xlog_ticket;
 struct log;
 struct xlog_recover;
@@ -853,6 +854,42 @@ DEFINE_LOGGRANT_EVENT(xfs_log_ungrant_en
 DEFINE_LOGGRANT_EVENT(xfs_log_ungrant_exit);
 DEFINE_LOGGRANT_EVENT(xfs_log_ungrant_sub);
 
+DECLARE_EVENT_CLASS(xfs_log_item_class,
+	TP_PROTO(struct xfs_log_item *lip),
+	TP_ARGS(lip),
+	TP_STRUCT__entry(
+		__field(dev_t, dev)
+		__field(void *, lip)
+		__field(uint, type)
+		__field(uint, flags)
+		__field(xfs_lsn_t, lsn)
+	),
+	TP_fast_assign(
+		__entry->dev = lip->li_mountp->m_super->s_dev;
+		__entry->lip = lip;
+		__entry->type = lip->li_type;
+		__entry->flags = lip->li_flags;
+		__entry->lsn = lip->li_lsn;
+	),
+	TP_printk("dev %d:%d lip 0x%p lsn %d/%d type %s flags %s",
+		  MAJOR(__entry->dev), MINOR(__entry->dev),
+		  __entry->lip,
+		  CYCLE_LSN(__entry->lsn), BLOCK_LSN(__entry->lsn),
+		  __print_symbolic(__entry->type, XFS_LI_TYPE_DESC),
+		  __print_flags(__entry->flags, "|", XFS_LI_FLAGS))
+)
+
+#define DEFINE_LOG_ITEM_EVENT(name) \
+DEFINE_EVENT(xfs_log_item_class, name, \
+	TP_PROTO(struct xfs_log_item *lip), \
+	TP_ARGS(lip))
+DEFINE_LOG_ITEM_EVENT(xfs_ail_push);
+DEFINE_LOG_ITEM_EVENT(xfs_ail_pushbuf);
+DEFINE_LOG_ITEM_EVENT(xfs_ail_pushbuf_pinned);
+DEFINE_LOG_ITEM_EVENT(xfs_ail_pinned);
+DEFINE_LOG_ITEM_EVENT(xfs_ail_locked);
+
+
 DECLARE_EVENT_CLASS(xfs_file_class,
 	TP_PROTO(struct xfs_inode *ip, size_t count, loff_t offset, int flags),
 	TP_ARGS(ip, count, offset, flags),
Index: xfs/fs/xfs/xfs_trans_ail.c
===================================================================
--- xfs.orig/fs/xfs/xfs_trans_ail.c	2011-10-06 14:22:18.955418059 -0400
+++ xfs/fs/xfs/xfs_trans_ail.c	2011-10-06 14:26:25.274918999 -0400
@@ -26,6 +26,7 @@
 #include "xfs_ag.h"
 #include "xfs_mount.h"
 #include "xfs_trans_priv.h"
+#include "xfs_trace.h"
 #include "xfs_error.h"
 
 #ifdef DEBUG
@@ -425,14 +426,18 @@ xfsaild_push(
 		switch (lock_result) {
 		case XFS_ITEM_SUCCESS:
 			XFS_STATS_INC(xs_push_ail_success);
+			trace_xfs_ail_push(lip);
+
 			IOP_PUSH(lip);
 			ailp->xa_last_pushed_lsn = lsn;
 			break;
 
 		case XFS_ITEM_PUSHBUF:
 			XFS_STATS_INC(xs_push_ail_pushbuf);
+			trace_xfs_ail_pushbuf(lip);
 
 			if (!IOP_PUSHBUF(lip)) {
+				trace_xfs_ail_pushbuf_pinned(lip);
 				stuck++;
 				ailp->xa_log_flush++;
 			} else {
@@ -443,12 +448,16 @@ xfsaild_push(
 
 		case XFS_ITEM_PINNED:
 			XFS_STATS_INC(xs_push_ail_pinned);
+			trace_xfs_ail_pinned(lip);
+
 			stuck++;
 			ailp->xa_log_flush++;
 			break;
 
 		case XFS_ITEM_LOCKED:
 			XFS_STATS_INC(xs_push_ail_locked);
+			trace_xfs_ail_locked(lip);
+
 			stuck++;
 			break;
 

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

Re: [PATCH 4/4] xfs: add AIL pushing tracepoints

[Alex Elder]

On Thu, 2011-10-06 at 14:33 -0400, Christoph Hellwig wrote:
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Looks good.

Reviewed-by: Alex Elder <aelder@sgi.com>


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

Re: [PATCH 4/4] xfs: add AIL pushing tracepoints

[Dave Chinner]

On Thu, Oct 06, 2011 at 02:33:01PM -0400, Christoph Hellwig wrote:
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Reviewed-by: Dave Chinner <dchinner@redhat.com>

-- 
Dave Chinner
david@fromorbit.com

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

[PATCH 0/4] fix AIL pushing under heavy concurrent metadata loads V2

[Christoph Hellwig]

This is the same patchset that got reviewed before, just rebased to Linus'
tree instead of the XFS tree.

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

[PATCH 1/4] xfs: do not update xa_last_pushed_lsn for locked items

[Christoph Hellwig]

[-- Attachment #1: xfs-ail-retry-locked-items --]
[-- Type: text/plain, Size: 1021 bytes --]

If an item was locked we should not update xa_last_pushed_lsn and thus skip
it when restarting the AIL scan as we need to be able to lock and write it
out as soon as possible.  Otherwise heavy lock contention might starve AIL
pushing too easily, especially given the larger backoff once we moved
xa_last_pushed_lsn all the way to the target lsn.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Reported-by: Stefan Priebe <s.priebe@profihost.ag>
Tested-by: Stefan Priebe <s.priebe@profihost.ag>

Index: xfs/fs/xfs/xfs_trans_ail.c
===================================================================
--- xfs.orig/fs/xfs/xfs_trans_ail.c	2011-10-11 15:48:49.302003241 +0200
+++ xfs/fs/xfs/xfs_trans_ail.c	2011-10-11 15:49:10.307505812 +0200
@@ -440,7 +440,6 @@ xfs_ail_worker(
 
 		case XFS_ITEM_LOCKED:
 			XFS_STATS_INC(xs_push_ail_locked);
-			ailp->xa_last_pushed_lsn = lsn;
 			stuck++;
 			break;
 

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