public inbox for linux-xfs@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/8] XFS: AIL cleanup and bug fixes
@ 2008-09-13 14:57 Dave Chinner
  2008-09-13 14:57 ` [PATCH 1/8] XFS: Allocate the struct xfs_ail Dave Chinner
                   ` (7 more replies)
  0 siblings, 8 replies; 21+ messages in thread
From: Dave Chinner @ 2008-09-13 14:57 UTC (permalink / raw)
  To: xfs

Clean up the AIL interfaces and the way it is referenced by
external code by making the struct xfs_ail the primary method
of accessing the AIL.

The main bug fix in this patch series is the second patch - it fixes
the restart bug in traversals of the AIL. The method used is
to track the next item in the list in a stack based cursor so
we don't need to trust the current log item after we've dropped
the AIL lock.

Because this changes the interface to the traversal code, it makes
sense to clean up the entire AIL subsystem interface while I am
there. The overall goal here is to make the struct xfs_ail the sole
structure used to access the AIL - for inserts, deletes, traversals,
locking, etc so that we don't need to reference the xfs_mount just
to get to the AIL.

This also enables us to move the struct xfs_ail definition out of
xfs_mount.h and reduce the visible scope of it to just the files
that need to access members of the structure.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH 1/8] XFS: Allocate the struct xfs_ail
  2008-09-13 14:57 [PATCH 0/8] XFS: AIL cleanup and bug fixes Dave Chinner
@ 2008-09-13 14:57 ` Dave Chinner
  2008-09-19  9:15   ` Christoph Hellwig
  2008-09-13 14:57 ` [PATCH 2/8] XFS: Use a cursor for AIL traversal Dave Chinner
                   ` (6 subsequent siblings)
  7 siblings, 1 reply; 21+ messages in thread
From: Dave Chinner @ 2008-09-13 14:57 UTC (permalink / raw)
  To: xfs

Rather than embedding the struct xfs_ail in the struct xfs_mount,
allocate it during AIL initialisation. Add a back pointer to
the struct xfs_ail so that we can pass around the xfs_ail
and still be able to access the xfs_mount if need be. This
is th first step involved in isolating the AIL implementation
from the surrounding filesystem code.

Signed-off-by: Dave Chinner <david@fromorbit.com>
---
 fs/xfs/linux-2.6/xfs_super.c |   28 +++++++-------
 fs/xfs/xfs_mount.h           |   10 +----
 fs/xfs/xfs_trans_ail.c       |   87 +++++++++++++++++++++++------------------
 fs/xfs/xfs_trans_priv.h      |   17 ++++++--
 4 files changed, 77 insertions(+), 65 deletions(-)

diff --git a/fs/xfs/linux-2.6/xfs_super.c b/fs/xfs/linux-2.6/xfs_super.c
index 2acf541..4e45367 100644
--- a/fs/xfs/linux-2.6/xfs_super.c
+++ b/fs/xfs/linux-2.6/xfs_super.c
@@ -813,18 +813,18 @@ xfs_setup_devices(
  */
 void
 xfsaild_wakeup(
-	xfs_mount_t		*mp,
+	struct xfs_ail		*ailp,
 	xfs_lsn_t		threshold_lsn)
 {
-	mp->m_ail.xa_target = threshold_lsn;
-	wake_up_process(mp->m_ail.xa_task);
+	ailp->xa_target = threshold_lsn;
+	wake_up_process(ailp->xa_task);
 }
 
 int
 xfsaild(
 	void	*data)
 {
-	xfs_mount_t	*mp = (xfs_mount_t *)data;
+	struct xfs_ail	*ailp = data;
 	xfs_lsn_t	last_pushed_lsn = 0;
 	long		tout = 0;
 
@@ -836,11 +836,11 @@ xfsaild(
 		/* swsusp */
 		try_to_freeze();
 
-		ASSERT(mp->m_log);
-		if (XFS_FORCED_SHUTDOWN(mp))
+		ASSERT(ailp->xa_mount->m_log);
+		if (XFS_FORCED_SHUTDOWN(ailp->xa_mount))
 			continue;
 
-		tout = xfsaild_push(mp, &last_pushed_lsn);
+		tout = xfsaild_push(ailp, &last_pushed_lsn);
 	}
 
 	return 0;
@@ -848,20 +848,20 @@ xfsaild(
 
 int
 xfsaild_start(
-	xfs_mount_t	*mp)
+	struct xfs_ail	*ailp)
 {
-	mp->m_ail.xa_target = 0;
-	mp->m_ail.xa_task = kthread_run(xfsaild, mp, "xfsaild");
-	if (IS_ERR(mp->m_ail.xa_task))
-		return -PTR_ERR(mp->m_ail.xa_task);
+	ailp->xa_target = 0;
+	ailp->xa_task = kthread_run(xfsaild, ailp, "xfsaild");
+	if (IS_ERR(ailp->xa_task))
+		return -PTR_ERR(ailp->xa_task);
 	return 0;
 }
 
 void
 xfsaild_stop(
-	xfs_mount_t	*mp)
+	struct xfs_ail	*ailp)
 {
-	kthread_stop(mp->m_ail.xa_task);
+	kthread_stop(ailp->xa_task);
 }
 
 
diff --git a/fs/xfs/xfs_mount.h b/fs/xfs/xfs_mount.h
index f52a7e3..a24b407 100644
--- a/fs/xfs/xfs_mount.h
+++ b/fs/xfs/xfs_mount.h
@@ -63,6 +63,7 @@ struct xfs_extdelta;
 struct xfs_swapext;
 struct xfs_mru_cache;
 struct xfs_nameops;
+struct xfs_ail;
 
 /*
  * Prototypes and functions for the Data Migration subsystem.
@@ -224,18 +225,11 @@ extern void	xfs_icsb_sync_counters_locked(struct xfs_mount *, int);
 #define xfs_icsb_sync_counters_locked(mp, flags) do { } while (0)
 #endif
 
-typedef struct xfs_ail {
-	struct list_head	xa_ail;
-	uint			xa_gen;
-	struct task_struct	*xa_task;
-	xfs_lsn_t		xa_target;
-} xfs_ail_t;
-
 typedef struct xfs_mount {
 	struct super_block	*m_super;
 	xfs_tid_t		m_tid;		/* next unused tid for fs */
 	spinlock_t		m_ail_lock;	/* fs AIL mutex */
-	xfs_ail_t		m_ail;		/* fs active log item list */
+	struct xfs_ail		*m_ail;		/* fs active log item list */
 	xfs_sb_t		m_sb;		/* copy of fs superblock */
 	spinlock_t		m_sb_lock;	/* sb counter lock */
 	struct xfs_buf		*m_sb_bp;	/* buffer for superblock */
diff --git a/fs/xfs/xfs_trans_ail.c b/fs/xfs/xfs_trans_ail.c
index 1f77c00..db72b52 100644
--- a/fs/xfs/xfs_trans_ail.c
+++ b/fs/xfs/xfs_trans_ail.c
@@ -28,13 +28,13 @@
 #include "xfs_trans_priv.h"
 #include "xfs_error.h"
 
-STATIC void xfs_ail_insert(xfs_ail_t *, xfs_log_item_t *);
-STATIC xfs_log_item_t * xfs_ail_delete(xfs_ail_t *, xfs_log_item_t *);
-STATIC xfs_log_item_t * xfs_ail_min(xfs_ail_t *);
-STATIC xfs_log_item_t * xfs_ail_next(xfs_ail_t *, xfs_log_item_t *);
+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 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 *);
 
 #ifdef DEBUG
-STATIC void xfs_ail_check(xfs_ail_t *, xfs_log_item_t *);
+STATIC void xfs_ail_check(struct xfs_ail *, xfs_log_item_t *);
 #else
 #define	xfs_ail_check(a,l)
 #endif /* DEBUG */
@@ -57,7 +57,7 @@ xfs_trans_tail_ail(
 	xfs_log_item_t	*lip;
 
 	spin_lock(&mp->m_ail_lock);
-	lip = xfs_ail_min(&mp->m_ail);
+	lip = xfs_ail_min(mp->m_ail);
 	if (lip == NULL) {
 		lsn = (xfs_lsn_t)0;
 	} else {
@@ -91,10 +91,10 @@ xfs_trans_push_ail(
 {
 	xfs_log_item_t		*lip;
 
-	lip = xfs_ail_min(&mp->m_ail);
+	lip = xfs_ail_min(mp->m_ail);
 	if (lip && !XFS_FORCED_SHUTDOWN(mp)) {
-		if (XFS_LSN_CMP(threshold_lsn, mp->m_ail.xa_target) > 0)
-			xfsaild_wakeup(mp, threshold_lsn);
+		if (XFS_LSN_CMP(threshold_lsn, mp->m_ail->xa_target) > 0)
+			xfsaild_wakeup(mp->m_ail, threshold_lsn);
 	}
 }
 
@@ -111,12 +111,12 @@ xfs_trans_first_push_ail(
 {
 	xfs_log_item_t	*lip;
 
-	lip = xfs_ail_min(&mp->m_ail);
-	*gen = (int)mp->m_ail.xa_gen;
+	lip = xfs_ail_min(mp->m_ail);
+	*gen = (int)mp->m_ail->xa_gen;
 	if (lsn == 0)
 		return lip;
 
-	list_for_each_entry(lip, &mp->m_ail.xa_ail, li_ail) {
+	list_for_each_entry(lip, &mp->m_ail->xa_ail, li_ail) {
 		if (XFS_LSN_CMP(lip->li_lsn, lsn) >= 0)
 			return lip;
 	}
@@ -129,17 +129,18 @@ xfs_trans_first_push_ail(
  */
 long
 xfsaild_push(
-	xfs_mount_t	*mp,
+	struct xfs_ail	*ailp,
 	xfs_lsn_t	*last_lsn)
 {
 	long		tout = 1000; /* milliseconds */
 	xfs_lsn_t	last_pushed_lsn = *last_lsn;
-	xfs_lsn_t	target =  mp->m_ail.xa_target;
+	xfs_lsn_t	target =  ailp->xa_target;
 	xfs_lsn_t	lsn;
 	xfs_log_item_t	*lip;
 	int		gen;
 	int		restarts;
 	int		flush_log, count, stuck;
+	xfs_mount_t	*mp = ailp->xa_mount;
 
 #define	XFS_TRANS_PUSH_AIL_RESTARTS	10
 
@@ -331,7 +332,7 @@ xfs_trans_unlocked_item(
 	 * the call to xfs_log_move_tail() doesn't do anything if there's
 	 * not enough free space to wake people up so we're safe calling it.
 	 */
-	min_lip = xfs_ail_min(&mp->m_ail);
+	min_lip = xfs_ail_min(mp->m_ail);
 
 	if (min_lip == lip)
 		xfs_log_move_tail(mp, 1);
@@ -362,10 +363,10 @@ xfs_trans_update_ail(
 	xfs_log_item_t		*dlip=NULL;
 	xfs_log_item_t		*mlip;	/* ptr to minimum lip */
 
-	mlip = xfs_ail_min(&mp->m_ail);
+	mlip = xfs_ail_min(mp->m_ail);
 
 	if (lip->li_flags & XFS_LI_IN_AIL) {
-		dlip = xfs_ail_delete(&mp->m_ail, lip);
+		dlip = xfs_ail_delete(mp->m_ail, lip);
 		ASSERT(dlip == lip);
 	} else {
 		lip->li_flags |= XFS_LI_IN_AIL;
@@ -373,11 +374,11 @@ xfs_trans_update_ail(
 
 	lip->li_lsn = lsn;
 
-	xfs_ail_insert(&mp->m_ail, lip);
-	mp->m_ail.xa_gen++;
+	xfs_ail_insert(mp->m_ail, lip);
+	mp->m_ail->xa_gen++;
 
 	if (mlip == dlip) {
-		mlip = xfs_ail_min(&mp->m_ail);
+		mlip = xfs_ail_min(mp->m_ail);
 		spin_unlock(&mp->m_ail_lock);
 		xfs_log_move_tail(mp, mlip->li_lsn);
 	} else {
@@ -411,17 +412,17 @@ xfs_trans_delete_ail(
 	xfs_log_item_t		*mlip;
 
 	if (lip->li_flags & XFS_LI_IN_AIL) {
-		mlip = xfs_ail_min(&mp->m_ail);
-		dlip = xfs_ail_delete(&mp->m_ail, lip);
+		mlip = xfs_ail_min(mp->m_ail);
+		dlip = xfs_ail_delete(mp->m_ail, lip);
 		ASSERT(dlip == lip);
 
 
 		lip->li_flags &= ~XFS_LI_IN_AIL;
 		lip->li_lsn = 0;
-		mp->m_ail.xa_gen++;
+		mp->m_ail->xa_gen++;
 
 		if (mlip == dlip) {
-			mlip = xfs_ail_min(&mp->m_ail);
+			mlip = xfs_ail_min(mp->m_ail);
 			spin_unlock(&mp->m_ail_lock);
 			xfs_log_move_tail(mp, (mlip ? mlip->li_lsn : 0));
 		} else {
@@ -459,8 +460,8 @@ xfs_trans_first_ail(
 {
 	xfs_log_item_t	*lip;
 
-	lip = xfs_ail_min(&mp->m_ail);
-	*gen = (int)mp->m_ail.xa_gen;
+	lip = xfs_ail_min(mp->m_ail);
+	*gen = (int)mp->m_ail->xa_gen;
 
 	return lip;
 }
@@ -482,11 +483,11 @@ xfs_trans_next_ail(
 	xfs_log_item_t	*nlip;
 
 	ASSERT(mp && lip && gen);
-	if (mp->m_ail.xa_gen == *gen) {
-		nlip = xfs_ail_next(&mp->m_ail, lip);
+	if (mp->m_ail->xa_gen == *gen) {
+		nlip = xfs_ail_next(mp->m_ail, lip);
 	} else {
-		nlip = xfs_ail_min(&mp->m_ail);
-		*gen = (int)mp->m_ail.xa_gen;
+		nlip = xfs_ail_min(mp->m_ail);
+		*gen = (int)mp->m_ail->xa_gen;
 		if (restarts != NULL) {
 			XFS_STATS_INC(xs_push_ail_restarts);
 			(*restarts)++;
@@ -515,15 +516,25 @@ int
 xfs_trans_ail_init(
 	xfs_mount_t	*mp)
 {
-	INIT_LIST_HEAD(&mp->m_ail.xa_ail);
-	return xfsaild_start(mp);
+	struct xfs_ail	*ailp;
+
+	ailp = kmem_zalloc(sizeof(struct xfs_ail), KM_MAYFAIL);
+	if (!ailp)
+		return ENOMEM;
+
+	ailp->xa_mount = mp;
+	INIT_LIST_HEAD(&ailp->xa_ail);
+	return xfsaild_start(ailp);
 }
 
 void
 xfs_trans_ail_destroy(
 	xfs_mount_t	*mp)
 {
-	xfsaild_stop(mp);
+	struct xfs_ail	*ailp = mp->m_ail;
+
+	xfsaild_stop(ailp);
+	kmem_free(ailp);
 }
 
 /*
@@ -534,7 +545,7 @@ xfs_trans_ail_destroy(
  */
 STATIC void
 xfs_ail_insert(
-	xfs_ail_t	*ailp,
+	struct xfs_ail	*ailp,
 	xfs_log_item_t	*lip)
 /* ARGSUSED */
 {
@@ -568,7 +579,7 @@ xfs_ail_insert(
 /*ARGSUSED*/
 STATIC xfs_log_item_t *
 xfs_ail_delete(
-	xfs_ail_t	*ailp,
+	struct xfs_ail	*ailp,
 	xfs_log_item_t	*lip)
 /* ARGSUSED */
 {
@@ -585,7 +596,7 @@ xfs_ail_delete(
  */
 STATIC xfs_log_item_t *
 xfs_ail_min(
-	xfs_ail_t	*ailp)
+	struct xfs_ail	*ailp)
 /* ARGSUSED */
 {
 	if (list_empty(&ailp->xa_ail))
@@ -601,7 +612,7 @@ xfs_ail_min(
  */
 STATIC xfs_log_item_t *
 xfs_ail_next(
-	xfs_ail_t	*ailp,
+	struct xfs_ail	*ailp,
 	xfs_log_item_t	*lip)
 /* ARGSUSED */
 {
@@ -617,7 +628,7 @@ xfs_ail_next(
  */
 STATIC void
 xfs_ail_check(
-	xfs_ail_t 	*ailp,
+	struct xfs_ail	*ailp,
 	xfs_log_item_t	*lip)
 {
 	xfs_log_item_t	*prev_lip;
diff --git a/fs/xfs/xfs_trans_priv.h b/fs/xfs/xfs_trans_priv.h
index 3c748c4..98317fd 100644
--- a/fs/xfs/xfs_trans_priv.h
+++ b/fs/xfs/xfs_trans_priv.h
@@ -56,13 +56,20 @@ struct xfs_log_item	*xfs_trans_first_ail(struct xfs_mount *, int *);
 struct xfs_log_item	*xfs_trans_next_ail(struct xfs_mount *,
 				     struct xfs_log_item *, int *, int *);
 
-
 /*
  * AIL push thread support
  */
-long	xfsaild_push(struct xfs_mount *, xfs_lsn_t *);
-void	xfsaild_wakeup(struct xfs_mount *, xfs_lsn_t);
-int	xfsaild_start(struct xfs_mount *);
-void	xfsaild_stop(struct xfs_mount *);
+struct xfs_ail {
+	struct xfs_mount	*xa_mount;
+	struct list_head	xa_ail;
+	uint			xa_gen;
+	struct task_struct	*xa_task;
+	xfs_lsn_t		xa_target;
+};
+
+long	xfsaild_push(struct xfs_ail *, xfs_lsn_t *);
+void	xfsaild_wakeup(struct xfs_ail *, xfs_lsn_t);
+int	xfsaild_start(struct xfs_ail *);
+void	xfsaild_stop(struct xfs_ail *);
 
 #endif	/* __XFS_TRANS_PRIV_H__ */
-- 
1.5.6

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH 2/8] XFS: Use a cursor for AIL traversal.
  2008-09-13 14:57 [PATCH 0/8] XFS: AIL cleanup and bug fixes Dave Chinner
  2008-09-13 14:57 ` [PATCH 1/8] XFS: Allocate the struct xfs_ail Dave Chinner
@ 2008-09-13 14:57 ` Dave Chinner
  2008-09-19  9:24   ` Christoph Hellwig
  2008-09-13 14:57 ` [PATCH 3/8] XFS: move the AIl traversal over to a consistent interface Dave Chinner
                   ` (5 subsequent siblings)
  7 siblings, 1 reply; 21+ messages in thread
From: Dave Chinner @ 2008-09-13 14:57 UTC (permalink / raw)
  To: xfs

To replace the current generation number ensuring sanity of the AIL
traversal, replace it with an external cursor that is linked to the
AIL.

Basically, we store the next item in the cursor whenever we want to
drop the AIL lock to do something to the current item.  When we
regain the lock. the current item may already be free, so we can't
reference it, but the next item in the traversal is already held in
the cursor.

When we move or delete an object, we search all the active cursors
and if there is an item match we clear the cursor(s) that point to
the object. This forces the traversal to restart transparently.

We don't invalidate the cursor on insert because the cursor still
points to a valid item. If the intem is inserted between the current
item and the cursor it does not matter; the traversal is considered
to be past the insertion point so it will be picked up in the next
traversal.

Hence traversal restarts pretty much disappear altogether with this
method of traversal, which should substantially reduce the overhead
of pushing on a busy AIL.

Signed-off-by: Dave Chinner <david@fromorbit.com>
---
 fs/xfs/xfs_log.c         |    4 +-
 fs/xfs/xfs_log_recover.c |   46 ++++++-------
 fs/xfs/xfs_trans_ail.c   |  171 +++++++++++++++++++++++++++++++---------------
 fs/xfs/xfs_trans_priv.h  |   52 +++++++++++---
 4 files changed, 180 insertions(+), 93 deletions(-)

diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index ff2ac20..a3e0dd5 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -872,7 +872,7 @@ xfs_log_move_tail(xfs_mount_t	*mp,
 int
 xfs_log_need_covered(xfs_mount_t *mp)
 {
-	int		needed = 0, gen;
+	int		needed = 0;
 	xlog_t		*log = mp->m_log;
 
 	if (!xfs_fs_writable(mp))
@@ -881,7 +881,7 @@ xfs_log_need_covered(xfs_mount_t *mp)
 	spin_lock(&log->l_icloglock);
 	if (((log->l_covered_state == XLOG_STATE_COVER_NEED) ||
 		(log->l_covered_state == XLOG_STATE_COVER_NEED2))
-			&& !xfs_trans_first_ail(mp, &gen)
+			&& !xfs_trans_first_ail(mp, NULL)
 			&& xlog_iclogs_empty(log)) {
 		if (log->l_covered_state == XLOG_STATE_COVER_NEED)
 			log->l_covered_state = XLOG_STATE_COVER_DONE;
diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index 82d46ce..d967baf 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -54,10 +54,11 @@ STATIC void	xlog_recover_insert_item_backq(xlog_recover_item_t **q,
 					       xlog_recover_item_t *item);
 #if defined(DEBUG)
 STATIC void	xlog_recover_check_summary(xlog_t *);
-STATIC void	xlog_recover_check_ail(xfs_mount_t *, xfs_log_item_t *, int);
+STATIC void	xlog_recover_check_ail(xfs_mount_t *, xfs_log_item_t *, 
+					struct xfs_ail_cursor *);
 #else
 #define	xlog_recover_check_summary(log)
-#define	xlog_recover_check_ail(mp, lip, gen)
+#define	xlog_recover_check_ail(mp, lip, cur)
 #endif
 
 
@@ -2710,8 +2711,8 @@ xlog_recover_do_efd_trans(
 	xfs_efd_log_format_t	*efd_formatp;
 	xfs_efi_log_item_t	*efip = NULL;
 	xfs_log_item_t		*lip;
-	int			gen;
 	__uint64_t		efi_id;
+	struct xfs_ail_cursor	cur;
 
 	if (pass == XLOG_RECOVER_PASS1) {
 		return;
@@ -2730,7 +2731,8 @@ xlog_recover_do_efd_trans(
 	 */
 	mp = log->l_mp;
 	spin_lock(&mp->m_ail_lock);
-	lip = xfs_trans_first_ail(mp, &gen);
+	xfs_trans_ail_cursor_init(mp->m_ail, &cur);
+	lip = xfs_trans_first_ail(mp, &cur);
 	while (lip != NULL) {
 		if (lip->li_type == XFS_LI_EFI) {
 			efip = (xfs_efi_log_item_t *)lip;
@@ -2741,11 +2743,13 @@ xlog_recover_do_efd_trans(
 				 */
 				xfs_trans_delete_ail(mp, lip);
 				xfs_efi_item_free(efip);
-				return;
+				spin_lock(&mp->m_ail_lock);
+				break;
 			}
 		}
-		lip = xfs_trans_next_ail(mp, lip, &gen, NULL);
+		lip = xfs_trans_next_ail(mp, &cur);
 	}
+	xfs_trans_ail_cursor_done(mp->m_ail, &cur);
 	spin_unlock(&mp->m_ail_lock);
 }
 
@@ -3038,20 +3042,11 @@ STATIC void
 xlog_recover_check_ail(
 	xfs_mount_t		*mp,
 	xfs_log_item_t		*lip,
-	int			gen)
+	struct xfs_ail_cursor	*cur)
 {
-	int			orig_gen = gen;
-
 	do {
 		ASSERT(lip->li_type != XFS_LI_EFI);
-		lip = xfs_trans_next_ail(mp, lip, &gen, NULL);
-		/*
-		 * The check will be bogus if we restart from the
-		 * beginning of the AIL, so ASSERT that we don't.
-		 * We never should since we're holding the AIL lock
-		 * the entire time.
-		 */
-		ASSERT(gen == orig_gen);
+		lip = xfs_trans_next_ail(mp, cur);
 	} while (lip != NULL);
 }
 #endif	/* DEBUG */
@@ -3080,20 +3075,21 @@ xlog_recover_process_efis(
 {
 	xfs_log_item_t		*lip;
 	xfs_efi_log_item_t	*efip;
-	int			gen;
 	xfs_mount_t		*mp;
 	int			error = 0;
+	struct xfs_ail_cursor	cur;
 
 	mp = log->l_mp;
 	spin_lock(&mp->m_ail_lock);
 
-	lip = xfs_trans_first_ail(mp, &gen);
+	xfs_trans_ail_cursor_init(mp->m_ail, &cur);
+	lip = xfs_trans_first_ail(mp, &cur);
 	while (lip != NULL) {
 		/*
 		 * We're done when we see something other than an EFI.
 		 */
 		if (lip->li_type != XFS_LI_EFI) {
-			xlog_recover_check_ail(mp, lip, gen);
+			xlog_recover_check_ail(mp, lip, &cur);
 			break;
 		}
 
@@ -3102,17 +3098,19 @@ xlog_recover_process_efis(
 		 */
 		efip = (xfs_efi_log_item_t *)lip;
 		if (efip->efi_flags & XFS_EFI_RECOVERED) {
-			lip = xfs_trans_next_ail(mp, lip, &gen, NULL);
+			lip = xfs_trans_next_ail(mp, &cur);
 			continue;
 		}
 
 		spin_unlock(&mp->m_ail_lock);
 		error = xlog_recover_process_efi(mp, efip);
-		if (error)
-			return error;
 		spin_lock(&mp->m_ail_lock);
-		lip = xfs_trans_next_ail(mp, lip, &gen, NULL);
+		if (error)
+			goto out;
+		lip = xfs_trans_next_ail(mp, &cur);
 	}
+out:
+	xfs_trans_ail_cursor_done(mp->m_ail, &cur);
 	spin_unlock(&mp->m_ail_lock);
 	return error;
 }
diff --git a/fs/xfs/xfs_trans_ail.c b/fs/xfs/xfs_trans_ail.c
index db72b52..668f2af 100644
--- a/fs/xfs/xfs_trans_ail.c
+++ b/fs/xfs/xfs_trans_ail.c
@@ -98,6 +98,78 @@ xfs_trans_push_ail(
 	}
 }
 
+void
+xfs_trans_ail_cursor_init(
+	struct xfs_ail		*ailp,
+	struct xfs_ail_cursor	*cur)
+{
+	cur->item = NULL;
+	if (cur == &ailp->xa_cursors)
+		return;
+
+	cur->next = ailp->xa_cursors.next;
+	ailp->xa_cursors.next = cur;
+}
+
+/*
+ * Set the cursor to the next item, because when we look
+ * up the cursor the current item may have been freed.
+ */
+STATIC void
+xfs_trans_ail_cursor_set(
+	struct xfs_ail		*ailp,
+	struct xfs_ail_cursor	*cur,
+	struct xfs_log_item	*lip)
+{
+	if (lip)
+		cur->item = xfs_ail_next(ailp, lip);
+}
+
+STATIC struct xfs_log_item *
+xfs_trans_ail_cursor_next(
+	struct xfs_ail		*ailp,
+	struct xfs_ail_cursor	*cur)
+{
+	struct xfs_log_item	*lip = cur->item;
+
+	xfs_trans_ail_cursor_set(ailp, cur, lip);
+	return lip;
+}
+
+STATIC void
+xfs_trans_ail_cursor_clear(
+	struct xfs_ail		*ailp,
+	struct xfs_log_item	*lip)
+{
+	struct xfs_ail_cursor	*cur;
+
+	/* need to search all cursors */
+	for (cur = &ailp->xa_cursors; cur; cur = cur->next) {
+		if (cur->item == lip)
+			cur->item = NULL;
+	}
+}
+
+void
+xfs_trans_ail_cursor_done(
+	struct xfs_ail		*ailp,
+	struct xfs_ail_cursor	*done)
+{
+	struct xfs_ail_cursor	*prev = NULL;
+	struct xfs_ail_cursor	*cur;
+
+	done->item = NULL;
+	if (done == &ailp->xa_cursors)
+		return;
+	prev = &ailp->xa_cursors;
+	for (cur = prev->next; cur; prev = cur, cur = prev->next) {
+		if (cur == done) {
+			prev->next = cur->next;
+			break;
+		}
+	}
+}
+
 /*
  * Return the item in the AIL with the current lsn.
  * Return the current tree generation number for use
@@ -105,20 +177,23 @@ xfs_trans_push_ail(
  */
 STATIC xfs_log_item_t *
 xfs_trans_first_push_ail(
-	xfs_mount_t	*mp,
-	int		*gen,
-	xfs_lsn_t	lsn)
+	struct xfs_mount	*mp,
+	struct xfs_ail_cursor	*cur,
+	xfs_lsn_t		lsn)
 {
-	xfs_log_item_t	*lip;
+	struct xfs_ail		*ailp = mp->m_ail;
+	xfs_log_item_t		*lip;
 
-	lip = xfs_ail_min(mp->m_ail);
-	*gen = (int)mp->m_ail->xa_gen;
+	lip = xfs_ail_min(ailp);
+	xfs_trans_ail_cursor_set(ailp, cur, lip);
 	if (lsn == 0)
 		return lip;
 
-	list_for_each_entry(lip, &mp->m_ail->xa_ail, li_ail) {
-		if (XFS_LSN_CMP(lip->li_lsn, lsn) >= 0)
+	list_for_each_entry(lip, &ailp->xa_ail, li_ail) {
+		if (XFS_LSN_CMP(lip->li_lsn, lsn) >= 0) {
+			xfs_trans_ail_cursor_set(ailp, cur, lip);
 			return lip;
+		}
 	}
 
 	return NULL;
@@ -137,22 +212,21 @@ xfsaild_push(
 	xfs_lsn_t	target =  ailp->xa_target;
 	xfs_lsn_t	lsn;
 	xfs_log_item_t	*lip;
-	int		gen;
-	int		restarts;
 	int		flush_log, count, stuck;
 	xfs_mount_t	*mp = ailp->xa_mount;
-
-#define	XFS_TRANS_PUSH_AIL_RESTARTS	10
+	struct xfs_ail_cursor	*cur = &ailp->xa_cursors;
 
 	spin_lock(&mp->m_ail_lock);
-	lip = xfs_trans_first_push_ail(mp, &gen, *last_lsn);
+	xfs_trans_ail_cursor_init(ailp, cur);
+	lip = xfs_trans_first_push_ail(mp, cur, *last_lsn);
 	if (!lip || XFS_FORCED_SHUTDOWN(mp)) {
 		/*
 		 * AIL is empty or our push has reached the end.
 		 */
+		xfs_trans_ail_cursor_done(ailp, cur);
 		spin_unlock(&mp->m_ail_lock);
 		last_pushed_lsn = 0;
-		goto out;
+		return tout;
 	}
 
 	XFS_STATS_INC(xs_push_ail);
@@ -170,7 +244,7 @@ xfsaild_push(
 	 */
 	tout = 10;
 	lsn = lip->li_lsn;
-	flush_log = stuck = count = restarts = 0;
+	flush_log = stuck = count = 0;
 	while ((XFS_LSN_CMP(lip->li_lsn, target) < 0)) {
 		int	lock_result;
 		/*
@@ -245,13 +319,12 @@ xfsaild_push(
 		if (stuck > 100)
 			break;
 
-		lip = xfs_trans_next_ail(mp, lip, &gen, &restarts);
+		lip = xfs_trans_next_ail(mp, cur);
 		if (lip == NULL)
 			break;
-		if (restarts > XFS_TRANS_PUSH_AIL_RESTARTS)
-			break;
 		lsn = lip->li_lsn;
 	}
+	xfs_trans_ail_cursor_done(ailp, cur);
 	spin_unlock(&mp->m_ail_lock);
 
 	if (flush_log) {
@@ -275,8 +348,7 @@ xfsaild_push(
 		 */
 		tout += 20;
 		last_pushed_lsn = 0;
-	} else if ((restarts > XFS_TRANS_PUSH_AIL_RESTARTS) ||
-		   ((stuck * 100) / count > 90)) {
+	} else if ((stuck * 100) / count > 90) {
 		/*
 		 * Either there is a lot of contention on the AIL or we
 		 * are stuck due to operations in progress. "Stuck" in this
@@ -288,7 +360,6 @@ xfsaild_push(
 		 */
 		tout += 10;
 	}
-out:
 	*last_lsn = last_pushed_lsn;
 	return tout;
 }	/* xfsaild_push */
@@ -348,9 +419,6 @@ xfs_trans_unlocked_item(
  * we move in the AIL is the minimum one, update the tail lsn in the
  * log manager.
  *
- * Increment 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.
  */
@@ -368,14 +436,13 @@ xfs_trans_update_ail(
 	if (lip->li_flags & XFS_LI_IN_AIL) {
 		dlip = xfs_ail_delete(mp->m_ail, lip);
 		ASSERT(dlip == lip);
+		xfs_trans_ail_cursor_clear(mp->m_ail, dlip);
 	} else {
 		lip->li_flags |= XFS_LI_IN_AIL;
 	}
 
 	lip->li_lsn = lsn;
-
 	xfs_ail_insert(mp->m_ail, lip);
-	mp->m_ail->xa_gen++;
 
 	if (mlip == dlip) {
 		mlip = xfs_ail_min(mp->m_ail);
@@ -415,11 +482,11 @@ xfs_trans_delete_ail(
 		mlip = xfs_ail_min(mp->m_ail);
 		dlip = xfs_ail_delete(mp->m_ail, lip);
 		ASSERT(dlip == lip);
+		xfs_trans_ail_cursor_clear(mp->m_ail, dlip);
 
 
 		lip->li_flags &= ~XFS_LI_IN_AIL;
 		lip->li_lsn = 0;
-		mp->m_ail->xa_gen++;
 
 		if (mlip == dlip) {
 			mlip = xfs_ail_min(mp->m_ail);
@@ -455,46 +522,29 @@ xfs_trans_delete_ail(
  */
 xfs_log_item_t *
 xfs_trans_first_ail(
-	xfs_mount_t	*mp,
-	int		*gen)
+	struct xfs_mount	*mp,
+	struct xfs_ail_cursor	*cur)
 {
-	xfs_log_item_t	*lip;
+	xfs_log_item_t		*lip;
+	struct xfs_ail		*ailp = mp->m_ail;
 
-	lip = xfs_ail_min(mp->m_ail);
-	*gen = (int)mp->m_ail->xa_gen;
+	lip = xfs_ail_min(ailp);
+	xfs_trans_ail_cursor_set(ailp, cur, lip);
 
 	return lip;
 }
 
 /*
- * If the generation count of the tree has not changed since the
- * caller last took something from the AIL, then return the elmt
- * in the tree which follows the one given.  If the count has changed,
- * then return the minimum elmt of the AIL and bump the restarts counter
- * if one is given.
+ * Grab the next item in the AIL from the cursor passed in.
  */
 xfs_log_item_t *
 xfs_trans_next_ail(
-	xfs_mount_t	*mp,
-	xfs_log_item_t	*lip,
-	int		*gen,
-	int		*restarts)
+	struct xfs_mount	*mp,
+	struct xfs_ail_cursor	*cur)
 {
-	xfs_log_item_t	*nlip;
+	struct xfs_ail		*ailp = mp->m_ail;
 
-	ASSERT(mp && lip && gen);
-	if (mp->m_ail->xa_gen == *gen) {
-		nlip = xfs_ail_next(mp->m_ail, lip);
-	} else {
-		nlip = xfs_ail_min(mp->m_ail);
-		*gen = (int)mp->m_ail->xa_gen;
-		if (restarts != NULL) {
-			XFS_STATS_INC(xs_push_ail_restarts);
-			(*restarts)++;
-		}
-	}
-
-	return (nlip);
+	return xfs_trans_ail_cursor_next(ailp, cur);
 }
 
 
@@ -517,6 +567,7 @@ xfs_trans_ail_init(
 	xfs_mount_t	*mp)
 {
 	struct xfs_ail	*ailp;
+	int		error;
 
 	ailp = kmem_zalloc(sizeof(struct xfs_ail), KM_MAYFAIL);
 	if (!ailp)
@@ -524,7 +575,15 @@ xfs_trans_ail_init(
 
 	ailp->xa_mount = mp;
 	INIT_LIST_HEAD(&ailp->xa_ail);
-	return xfsaild_start(ailp);
+	error = xfsaild_start(ailp);
+	if (error)
+		goto out_free_ailp;
+	mp->m_ail = ailp;
+	return 0;
+
+out_free_ailp:
+	kmem_free(ailp);
+	return error;
 }
 
 void
diff --git a/fs/xfs/xfs_trans_priv.h b/fs/xfs/xfs_trans_priv.h
index 98317fd..c596e34 100644
--- a/fs/xfs/xfs_trans_priv.h
+++ b/fs/xfs/xfs_trans_priv.h
@@ -44,20 +44,30 @@ xfs_log_busy_slot_t		*xfs_trans_add_busy(xfs_trans_t *tp,
 						    xfs_extlen_t idx);
 
 /*
- * From xfs_trans_ail.c
+ * AIL traversal cursor.
+ *
+ * Rather than using a generation number for detecting changes in the ail, use
+ * a cursor that is protected by the ail lock. The aild cursor exists in the
+ * struct xfs_ail, but other traversals can declare it on the stack and link it
+ * to the ail list.
+ *
+ * When an object is deleted from or moved int the AIL, the cursor list is
+ * searched to see if the object is a designated cursor item. If it is, it is
+ * deleted from the cursor so taht the next time the cursor is used traversal
+ * will return to the start.
+ *
+ * This means a traversal colliding with a removal will cause a restart of the
+ * list scan, rather than any insertion or deletion anywhere in the list.
  */
-void			xfs_trans_update_ail(struct xfs_mount *mp,
-				     struct xfs_log_item *lip, xfs_lsn_t lsn)
-				     __releases(mp->m_ail_lock);
-void			xfs_trans_delete_ail(struct xfs_mount *mp,
-				     struct xfs_log_item *lip)
-				     __releases(mp->m_ail_lock);
-struct xfs_log_item	*xfs_trans_first_ail(struct xfs_mount *, int *);
-struct xfs_log_item	*xfs_trans_next_ail(struct xfs_mount *,
-				     struct xfs_log_item *, int *, int *);
+struct xfs_ail_cursor {
+	struct xfs_ail_cursor	*next;
+	struct xfs_log_item	*item;
+};
 
 /*
- * AIL push thread support
+ * Private AIL structures.
+ *
+ * Eventually we need to drive the locking in here as well.
  */
 struct xfs_ail {
 	struct xfs_mount	*xa_mount;
@@ -65,8 +75,28 @@ struct xfs_ail {
 	uint			xa_gen;
 	struct task_struct	*xa_task;
 	xfs_lsn_t		xa_target;
+	struct xfs_ail_cursor	xa_cursors;
 };
 
+/*
+ * From xfs_trans_ail.c
+ */
+void			xfs_trans_update_ail(struct xfs_mount *mp,
+				     struct xfs_log_item *lip, xfs_lsn_t lsn)
+				     __releases(mp->m_ail_lock);
+void			xfs_trans_delete_ail(struct xfs_mount *mp,
+				     struct xfs_log_item *lip)
+				     __releases(mp->m_ail_lock);
+struct xfs_log_item	*xfs_trans_first_ail(struct xfs_mount *mp,
+					struct xfs_ail_cursor *cur);
+struct xfs_log_item	*xfs_trans_next_ail(struct xfs_mount *mp,
+					struct xfs_ail_cursor *cur);
+
+void xfs_trans_ail_cursor_init(struct xfs_ail *ailp,
+					struct xfs_ail_cursor *cur);
+void xfs_trans_ail_cursor_done(struct xfs_ail *ailp,
+					struct xfs_ail_cursor *cur);
+
 long	xfsaild_push(struct xfs_ail *, xfs_lsn_t *);
 void	xfsaild_wakeup(struct xfs_ail *, xfs_lsn_t);
 int	xfsaild_start(struct xfs_ail *);
-- 
1.5.6

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH 3/8] XFS: move the AIl traversal over to a consistent interface
  2008-09-13 14:57 [PATCH 0/8] XFS: AIL cleanup and bug fixes Dave Chinner
  2008-09-13 14:57 ` [PATCH 1/8] XFS: Allocate the struct xfs_ail Dave Chinner
  2008-09-13 14:57 ` [PATCH 2/8] XFS: Use a cursor for AIL traversal Dave Chinner
@ 2008-09-13 14:57 ` Dave Chinner
  2008-09-19  9:26   ` Christoph Hellwig
  2008-09-13 14:57 ` [PATCH 4/8] XFS: Allow 64 bit machines to avoid the AIL lock during flushes Dave Chinner
                   ` (4 subsequent siblings)
  7 siblings, 1 reply; 21+ messages in thread
From: Dave Chinner @ 2008-09-13 14:57 UTC (permalink / raw)
  To: xfs

With the new cursor interface, it makes sense to make all the
traversing code use the cursor interface and make the old one go
away. This means more of the AIL interfacing is done by passing
struct xfs_ail pointers around the place instead of struct
xfs_mount pointers.

We can replace the use of xfs_trans_first_ail() in
xfs_log_need_covered() as it is only checking if the AIL is empty.
We can do that with a call to xfs_trans_ail_tail() instead, where a
zero LSN returned indicates and empty AIL...

Signed-off-by: Dave Chinner <david@fromorbit.com>
---
 fs/xfs/xfs_log.c         |    4 +-
 fs/xfs/xfs_log_recover.c |   14 +++----
 fs/xfs/xfs_trans.h       |    1 -
 fs/xfs/xfs_trans_ail.c   |  102 +++++++++++++++-------------------------------
 fs/xfs/xfs_trans_priv.h  |   13 +++---
 5 files changed, 48 insertions(+), 86 deletions(-)

diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index a3e0dd5..70f5063 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -881,7 +881,7 @@ xfs_log_need_covered(xfs_mount_t *mp)
 	spin_lock(&log->l_icloglock);
 	if (((log->l_covered_state == XLOG_STATE_COVER_NEED) ||
 		(log->l_covered_state == XLOG_STATE_COVER_NEED2))
-			&& !xfs_trans_first_ail(mp, NULL)
+			&& !xfs_trans_ail_tail(mp->m_ail)
 			&& xlog_iclogs_empty(log)) {
 		if (log->l_covered_state == XLOG_STATE_COVER_NEED)
 			log->l_covered_state = XLOG_STATE_COVER_DONE;
@@ -918,7 +918,7 @@ xlog_assign_tail_lsn(xfs_mount_t *mp)
 	xfs_lsn_t tail_lsn;
 	xlog_t	  *log = mp->m_log;
 
-	tail_lsn = xfs_trans_tail_ail(mp);
+	tail_lsn = xfs_trans_ail_tail(mp->m_ail);
 	spin_lock(&log->l_grant_lock);
 	if (tail_lsn != 0) {
 		log->l_tail_lsn = tail_lsn;
diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index d967baf..46ef060 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -2731,8 +2731,7 @@ xlog_recover_do_efd_trans(
 	 */
 	mp = log->l_mp;
 	spin_lock(&mp->m_ail_lock);
-	xfs_trans_ail_cursor_init(mp->m_ail, &cur);
-	lip = xfs_trans_first_ail(mp, &cur);
+	lip = xfs_trans_ail_cursor_first(mp->m_ail, &cur, 0);
 	while (lip != NULL) {
 		if (lip->li_type == XFS_LI_EFI) {
 			efip = (xfs_efi_log_item_t *)lip;
@@ -2747,7 +2746,7 @@ xlog_recover_do_efd_trans(
 				break;
 			}
 		}
-		lip = xfs_trans_next_ail(mp, &cur);
+		lip = xfs_trans_ail_cursor_next(mp->m_ail, &cur);
 	}
 	xfs_trans_ail_cursor_done(mp->m_ail, &cur);
 	spin_unlock(&mp->m_ail_lock);
@@ -3046,7 +3045,7 @@ xlog_recover_check_ail(
 {
 	do {
 		ASSERT(lip->li_type != XFS_LI_EFI);
-		lip = xfs_trans_next_ail(mp, cur);
+		lip = xfs_trans_ail_cursor_next(mp->m_ail, cur);
 	} while (lip != NULL);
 }
 #endif	/* DEBUG */
@@ -3082,8 +3081,7 @@ xlog_recover_process_efis(
 	mp = log->l_mp;
 	spin_lock(&mp->m_ail_lock);
 
-	xfs_trans_ail_cursor_init(mp->m_ail, &cur);
-	lip = xfs_trans_first_ail(mp, &cur);
+	lip = xfs_trans_ail_cursor_first(mp->m_ail, &cur, 0);
 	while (lip != NULL) {
 		/*
 		 * We're done when we see something other than an EFI.
@@ -3098,7 +3096,7 @@ xlog_recover_process_efis(
 		 */
 		efip = (xfs_efi_log_item_t *)lip;
 		if (efip->efi_flags & XFS_EFI_RECOVERED) {
-			lip = xfs_trans_next_ail(mp, &cur);
+			lip = xfs_trans_ail_cursor_next(mp->m_ail, &cur);
 			continue;
 		}
 
@@ -3107,7 +3105,7 @@ xlog_recover_process_efis(
 		spin_lock(&mp->m_ail_lock);
 		if (error)
 			goto out;
-		lip = xfs_trans_next_ail(mp, &cur);
+		lip = xfs_trans_ail_cursor_next(mp->m_ail, &cur);
 	}
 out:
 	xfs_trans_ail_cursor_done(mp->m_ail, &cur);
diff --git a/fs/xfs/xfs_trans.h b/fs/xfs/xfs_trans.h
index 74c80bd..ead53bd 100644
--- a/fs/xfs/xfs_trans.h
+++ b/fs/xfs/xfs_trans.h
@@ -979,7 +979,6 @@ int		xfs_trans_roll(struct xfs_trans **, struct xfs_inode *);
 int		xfs_trans_ail_init(struct xfs_mount *);
 void		xfs_trans_ail_destroy(struct xfs_mount *);
 void		xfs_trans_push_ail(struct xfs_mount *, xfs_lsn_t);
-xfs_lsn_t	xfs_trans_tail_ail(struct xfs_mount *);
 void		xfs_trans_unlocked_item(struct xfs_mount *,
 					xfs_log_item_t *);
 xfs_log_busy_slot_t *xfs_trans_add_busy(xfs_trans_t *tp,
diff --git a/fs/xfs/xfs_trans_ail.c b/fs/xfs/xfs_trans_ail.c
index 668f2af..0591888 100644
--- a/fs/xfs/xfs_trans_ail.c
+++ b/fs/xfs/xfs_trans_ail.c
@@ -50,20 +50,20 @@ STATIC void xfs_ail_check(struct xfs_ail *, xfs_log_item_t *);
  * lsn of the last item in the AIL.
  */
 xfs_lsn_t
-xfs_trans_tail_ail(
-	xfs_mount_t	*mp)
+xfs_trans_ail_tail(
+	struct xfs_ail	*ailp)
 {
 	xfs_lsn_t	lsn;
 	xfs_log_item_t	*lip;
 
-	spin_lock(&mp->m_ail_lock);
-	lip = xfs_ail_min(mp->m_ail);
+	spin_lock(&ailp->xa_mount->m_ail_lock);
+	lip = xfs_ail_min(ailp);
 	if (lip == NULL) {
 		lsn = (xfs_lsn_t)0;
 	} else {
 		lsn = lip->li_lsn;
 	}
-	spin_unlock(&mp->m_ail_lock);
+	spin_unlock(&ailp->xa_mount->m_ail_lock);
 
 	return lsn;
 }
@@ -98,7 +98,7 @@ xfs_trans_push_ail(
 	}
 }
 
-void
+STATIC void
 xfs_trans_ail_cursor_init(
 	struct xfs_ail		*ailp,
 	struct xfs_ail_cursor	*cur)
@@ -125,7 +125,7 @@ xfs_trans_ail_cursor_set(
 		cur->item = xfs_ail_next(ailp, lip);
 }
 
-STATIC struct xfs_log_item *
+struct xfs_log_item *
 xfs_trans_ail_cursor_next(
 	struct xfs_ail		*ailp,
 	struct xfs_ail_cursor	*cur)
@@ -136,20 +136,6 @@ xfs_trans_ail_cursor_next(
 	return lip;
 }
 
-STATIC void
-xfs_trans_ail_cursor_clear(
-	struct xfs_ail		*ailp,
-	struct xfs_log_item	*lip)
-{
-	struct xfs_ail_cursor	*cur;
-
-	/* need to search all cursors */
-	for (cur = &ailp->xa_cursors; cur; cur = cur->next) {
-		if (cur->item == lip)
-			cur->item = NULL;
-	}
-}
-
 void
 xfs_trans_ail_cursor_done(
 	struct xfs_ail		*ailp,
@@ -170,33 +156,45 @@ xfs_trans_ail_cursor_done(
 	}
 }
 
+STATIC void
+xfs_trans_ail_cursor_clear(
+	struct xfs_ail		*ailp,
+	struct xfs_log_item	*lip)
+{
+	struct xfs_ail_cursor	*cur;
+
+	/* need to search all cursors */
+	for (cur = &ailp->xa_cursors; cur; cur = cur->next) {
+		if (cur->item == lip)
+			cur->item = NULL;
+	}
+}
+
 /*
  * Return the item in the AIL with the current lsn.
  * Return the current tree generation number for use
  * in calls to xfs_trans_next_ail().
  */
-STATIC xfs_log_item_t *
-xfs_trans_first_push_ail(
-	struct xfs_mount	*mp,
+xfs_log_item_t *
+xfs_trans_ail_cursor_first(
+	struct xfs_ail		*ailp,
 	struct xfs_ail_cursor	*cur,
 	xfs_lsn_t		lsn)
 {
-	struct xfs_ail		*ailp = mp->m_ail;
 	xfs_log_item_t		*lip;
 
+	xfs_trans_ail_cursor_init(ailp, cur);
 	lip = xfs_ail_min(ailp);
-	xfs_trans_ail_cursor_set(ailp, cur, lip);
 	if (lsn == 0)
-		return lip;
+		goto out;
 
 	list_for_each_entry(lip, &ailp->xa_ail, li_ail) {
-		if (XFS_LSN_CMP(lip->li_lsn, lsn) >= 0) {
-			xfs_trans_ail_cursor_set(ailp, cur, lip);
-			return lip;
-		}
+		if (XFS_LSN_CMP(lip->li_lsn, lsn) >= 0)
+			break;
 	}
-
-	return NULL;
+out:
+	xfs_trans_ail_cursor_set(ailp, cur, lip);
+	return lip;
 }
 
 /*
@@ -217,8 +215,7 @@ xfsaild_push(
 	struct xfs_ail_cursor	*cur = &ailp->xa_cursors;
 
 	spin_lock(&mp->m_ail_lock);
-	xfs_trans_ail_cursor_init(ailp, cur);
-	lip = xfs_trans_first_push_ail(mp, cur, *last_lsn);
+	lip = xfs_trans_ail_cursor_first(ailp, cur, *last_lsn);
 	if (!lip || XFS_FORCED_SHUTDOWN(mp)) {
 		/*
 		 * AIL is empty or our push has reached the end.
@@ -319,7 +316,7 @@ xfsaild_push(
 		if (stuck > 100)
 			break;
 
-		lip = xfs_trans_next_ail(mp, cur);
+		lip = xfs_trans_ail_cursor_next(ailp, cur);
 		if (lip == NULL)
 			break;
 		lsn = lip->li_lsn;
@@ -516,39 +513,6 @@ xfs_trans_delete_ail(
 
 
 /*
- * Return the item in the AIL with the smallest lsn.
- * Return the current tree generation number for use
- * in calls to xfs_trans_next_ail().
- */
-xfs_log_item_t *
-xfs_trans_first_ail(
-	struct xfs_mount	*mp,
-	struct xfs_ail_cursor	*cur)
-{
-	xfs_log_item_t		*lip;
-	struct xfs_ail		*ailp = mp->m_ail;
-
-	lip = xfs_ail_min(ailp);
-	xfs_trans_ail_cursor_set(ailp, cur, lip);
-
-	return lip;
-}
-
-/*
- * Grab the next item in the AIL from the cursor passed in.
- */
-xfs_log_item_t *
-xfs_trans_next_ail(
-	struct xfs_mount	*mp,
-	struct xfs_ail_cursor	*cur)
-{
-	struct xfs_ail		*ailp = mp->m_ail;
-
-	return xfs_trans_ail_cursor_next(ailp, cur);
-}
-
-
-/*
  * The active item list (AIL) is a doubly linked list of log
  * items sorted by ascending lsn.  The base of the list is
  * a forw/back pointer pair embedded in the xfs mount structure.
diff --git a/fs/xfs/xfs_trans_priv.h b/fs/xfs/xfs_trans_priv.h
index c596e34..8b84167 100644
--- a/fs/xfs/xfs_trans_priv.h
+++ b/fs/xfs/xfs_trans_priv.h
@@ -87,14 +87,15 @@ void			xfs_trans_update_ail(struct xfs_mount *mp,
 void			xfs_trans_delete_ail(struct xfs_mount *mp,
 				     struct xfs_log_item *lip)
 				     __releases(mp->m_ail_lock);
-struct xfs_log_item	*xfs_trans_first_ail(struct xfs_mount *mp,
-					struct xfs_ail_cursor *cur);
-struct xfs_log_item	*xfs_trans_next_ail(struct xfs_mount *mp,
-					struct xfs_ail_cursor *cur);
 
-void xfs_trans_ail_cursor_init(struct xfs_ail *ailp,
+xfs_lsn_t		xfs_trans_ail_tail(struct xfs_ail *ailp);
+
+struct xfs_log_item	*xfs_trans_ail_cursor_first(struct xfs_ail *ailp,
+					struct xfs_ail_cursor *cur,
+					xfs_lsn_t lsn);
+struct xfs_log_item	*xfs_trans_ail_cursor_next(struct xfs_ail *ailp,
 					struct xfs_ail_cursor *cur);
-void xfs_trans_ail_cursor_done(struct xfs_ail *ailp,
+void			xfs_trans_ail_cursor_done(struct xfs_ail *ailp,
 					struct xfs_ail_cursor *cur);
 
 long	xfsaild_push(struct xfs_ail *, xfs_lsn_t *);
-- 
1.5.6

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH 4/8] XFS: Allow 64 bit machines to avoid the AIL lock during flushes
  2008-09-13 14:57 [PATCH 0/8] XFS: AIL cleanup and bug fixes Dave Chinner
                   ` (2 preceding siblings ...)
  2008-09-13 14:57 ` [PATCH 3/8] XFS: move the AIl traversal over to a consistent interface Dave Chinner
@ 2008-09-13 14:57 ` Dave Chinner
  2008-09-19  9:26   ` Christoph Hellwig
  2008-09-13 14:57 ` [PATCH 5/8] XFS: Move the AIL lock into the struct xfs_ail Dave Chinner
                   ` (3 subsequent siblings)
  7 siblings, 1 reply; 21+ messages in thread
From: Dave Chinner @ 2008-09-13 14:57 UTC (permalink / raw)
  To: xfs

When copying lsn's from the log item to the inode or dquot
flush lsn, we currently grab the AIL lock. We do this because the
LSN is a 64 bit quantity and it needs to be read atomically.
The lock is used to guarantee atomicity for 32 bit platforms.

Make the LSN copying a small function, and make the function
used conditional on BITS_PER_LONG so that 64 bit machines don't
need to take the AIL lock in these places.

Signed-off-by: Dave Chinner <david@fromorbit.com>
---
 fs/xfs/quota/xfs_dquot.c |    6 ++----
 fs/xfs/xfs_inode.c       |   17 +++++++----------
 fs/xfs/xfs_trans_priv.h  |   23 +++++++++++++++++++++++
 3 files changed, 32 insertions(+), 14 deletions(-)

diff --git a/fs/xfs/quota/xfs_dquot.c b/fs/xfs/quota/xfs_dquot.c
index f2705f2..8e30a10 100644
--- a/fs/xfs/quota/xfs_dquot.c
+++ b/fs/xfs/quota/xfs_dquot.c
@@ -1274,10 +1274,8 @@ xfs_qm_dqflush(
 	dqp->dq_flags &= ~(XFS_DQ_DIRTY);
 	mp = dqp->q_mount;
 
-	/* lsn is 64 bits */
-	spin_lock(&mp->m_ail_lock);
-	dqp->q_logitem.qli_flush_lsn = dqp->q_logitem.qli_item.li_lsn;
-	spin_unlock(&mp->m_ail_lock);
+	xfs_trans_ail_copy_lsn(mp->m_ail, &dqp->q_logitem.qli_flush_lsn,
+					&dqp->q_logitem.qli_item.li_lsn);
 
 	/*
 	 * Attach an iodone routine so that we can remove this dquot from the
diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index 401b92c..448f507 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -2213,9 +2213,9 @@ xfs_ifree_cluster(
 				iip = (xfs_inode_log_item_t *)lip;
 				ASSERT(iip->ili_logged == 1);
 				lip->li_cb = (void(*)(xfs_buf_t*,xfs_log_item_t*)) xfs_istale_done;
-				spin_lock(&mp->m_ail_lock);
-				iip->ili_flush_lsn = iip->ili_item.li_lsn;
-				spin_unlock(&mp->m_ail_lock);
+				xfs_trans_ail_copy_lsn(mp->m_ail,
+							&iip->ili_flush_lsn,
+							&iip->ili_item.li_lsn);
 				xfs_iflags_set(iip->ili_inode, XFS_ISTALE);
 				pre_flushed++;
 			}
@@ -2236,9 +2236,8 @@ xfs_ifree_cluster(
 			iip->ili_last_fields = iip->ili_format.ilf_fields;
 			iip->ili_format.ilf_fields = 0;
 			iip->ili_logged = 1;
-			spin_lock(&mp->m_ail_lock);
-			iip->ili_flush_lsn = iip->ili_item.li_lsn;
-			spin_unlock(&mp->m_ail_lock);
+			xfs_trans_ail_copy_lsn(mp->m_ail, &iip->ili_flush_lsn,
+						&iip->ili_item.li_lsn);
 
 			xfs_buf_attach_iodone(bp,
 				(void(*)(xfs_buf_t*,xfs_log_item_t*))
@@ -3479,10 +3478,8 @@ xfs_iflush_int(
 		iip->ili_format.ilf_fields = 0;
 		iip->ili_logged = 1;
 
-		ASSERT(sizeof(xfs_lsn_t) == 8);	/* don't lock if it shrinks */
-		spin_lock(&mp->m_ail_lock);
-		iip->ili_flush_lsn = iip->ili_item.li_lsn;
-		spin_unlock(&mp->m_ail_lock);
+		xfs_trans_ail_copy_lsn(mp->m_ail, &iip->ili_flush_lsn,
+					&iip->ili_item.li_lsn);
 
 		/*
 		 * Attach the function xfs_iflush_done to the inode's
diff --git a/fs/xfs/xfs_trans_priv.h b/fs/xfs/xfs_trans_priv.h
index 8b84167..c6ea7fe 100644
--- a/fs/xfs/xfs_trans_priv.h
+++ b/fs/xfs/xfs_trans_priv.h
@@ -103,4 +103,27 @@ void	xfsaild_wakeup(struct xfs_ail *, xfs_lsn_t);
 int	xfsaild_start(struct xfs_ail *);
 void	xfsaild_stop(struct xfs_ail *);
 
+#if BITS_PER_LONG != 64
+static inline void
+xfs_trans_ail_copy_lsn(
+	struct xfs_ail	*ailp,
+	xfs_lsn_t	*dst,
+	xfs_lsn_t	*src)
+{
+	ASSERT(sizeof(xfs_lsn_t) == 8);	/* don't lock if it shrinks */
+	spin_lock(&ailp->xa_mount->m_ail_lock);
+	*dst = *src;
+	spin_unlock(&ailp->xa_mount->m_ail_lock);
+}
+#else
+static inline void
+xfs_trans_ail_copy_lsn(
+	struct xfs_ail	*ailp,
+	xfs_lsn_t	*dst,
+	xfs_lsn_t	*src)
+{
+	ASSERT(sizeof(xfs_lsn_t) == 8);
+	*dst = *src;
+}
+#endif
 #endif	/* __XFS_TRANS_PRIV_H__ */
-- 
1.5.6

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH 5/8] XFS: Move the AIL lock into the struct xfs_ail
  2008-09-13 14:57 [PATCH 0/8] XFS: AIL cleanup and bug fixes Dave Chinner
                   ` (3 preceding siblings ...)
  2008-09-13 14:57 ` [PATCH 4/8] XFS: Allow 64 bit machines to avoid the AIL lock during flushes Dave Chinner
@ 2008-09-13 14:57 ` Dave Chinner
  2008-09-19  9:26   ` Christoph Hellwig
  2008-09-13 14:57 ` [PATCH 6/8] XFS: Given the log a pointer to the AIL Dave Chinner
                   ` (2 subsequent siblings)
  7 siblings, 1 reply; 21+ messages in thread
From: Dave Chinner @ 2008-09-13 14:57 UTC (permalink / raw)
  To: xfs

Bring the ail lock inside the struct xfs_ail. This means
the AIL can be entirely manipulated via the struct xfs_ail rather
than needing both the struct xfs_mount and the struct xfs_ail.

Signed-off-by: Dave Chinner <david@fromorbit.com>
---
 fs/xfs/quota/xfs_dquot.c      |    4 +-
 fs/xfs/quota/xfs_dquot_item.c |    2 +-
 fs/xfs/xfs_buf_item.c         |    4 +-
 fs/xfs/xfs_extfree_item.c     |   12 ++++----
 fs/xfs/xfs_inode.c            |    4 +-
 fs/xfs/xfs_inode_item.c       |    8 +++---
 fs/xfs/xfs_log.c              |    1 -
 fs/xfs/xfs_log_recover.c      |   16 ++++++------
 fs/xfs/xfs_mount.h            |    1 -
 fs/xfs/xfs_trans.c            |    4 +-
 fs/xfs/xfs_trans_ail.c        |   56 +++++++++++++++++++++-------------------
 fs/xfs/xfs_trans_priv.h       |    5 ++-
 12 files changed, 59 insertions(+), 58 deletions(-)

diff --git a/fs/xfs/quota/xfs_dquot.c b/fs/xfs/quota/xfs_dquot.c
index 8e30a10..29a7aa7 100644
--- a/fs/xfs/quota/xfs_dquot.c
+++ b/fs/xfs/quota/xfs_dquot.c
@@ -1335,7 +1335,7 @@ xfs_qm_dqflush_done(
 	if ((qip->qli_item.li_flags & XFS_LI_IN_AIL) &&
 	    qip->qli_item.li_lsn == qip->qli_flush_lsn) {
 
-		spin_lock(&dqp->q_mount->m_ail_lock);
+		spin_lock(&dqp->q_mount->m_ail->xa_lock);
 		/*
 		 * xfs_trans_delete_ail() drops the AIL lock.
 		 */
@@ -1343,7 +1343,7 @@ xfs_qm_dqflush_done(
 			xfs_trans_delete_ail(dqp->q_mount,
 					     (xfs_log_item_t*)qip);
 		else
-			spin_unlock(&dqp->q_mount->m_ail_lock);
+			spin_unlock(&dqp->q_mount->m_ail->xa_lock);
 	}
 
 	/*
diff --git a/fs/xfs/quota/xfs_dquot_item.c b/fs/xfs/quota/xfs_dquot_item.c
index f028644..478077c 100644
--- a/fs/xfs/quota/xfs_dquot_item.c
+++ b/fs/xfs/quota/xfs_dquot_item.c
@@ -570,7 +570,7 @@ xfs_qm_qoffend_logitem_committed(
 	xfs_qoff_logitem_t	*qfs;
 
 	qfs = qfe->qql_start_lip;
-	spin_lock(&qfs->qql_item.li_mountp->m_ail_lock);
+	spin_lock(&qfs->qql_item.li_mountp->m_ail->xa_lock);
 	/*
 	 * Delete the qoff-start logitem from the AIL.
 	 * xfs_trans_delete_ail() drops the AIL lock.
diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
index 608c30c..7f1e266 100644
--- a/fs/xfs/xfs_buf_item.c
+++ b/fs/xfs/xfs_buf_item.c
@@ -408,7 +408,7 @@ xfs_buf_item_unpin(
 			XFS_BUF_SET_FSPRIVATE(bp, NULL);
 			XFS_BUF_CLR_IODONE_FUNC(bp);
 		} else {
-			spin_lock(&mp->m_ail_lock);
+			spin_lock(&mp->m_ail->xa_lock);
 			xfs_trans_delete_ail(mp, (xfs_log_item_t *)bip);
 			xfs_buf_item_relse(bp);
 			ASSERT(XFS_BUF_FSPRIVATE(bp, void *) == NULL);
@@ -1131,7 +1131,7 @@ xfs_buf_iodone(
 	 *
 	 * Either way, AIL is useless if we're forcing a shutdown.
 	 */
-	spin_lock(&mp->m_ail_lock);
+	spin_lock(&mp->m_ail->xa_lock);
 	/*
 	 * xfs_trans_delete_ail() drops the AIL lock.
 	 */
diff --git a/fs/xfs/xfs_extfree_item.c b/fs/xfs/xfs_extfree_item.c
index 8aa28f7..f1dcd80 100644
--- a/fs/xfs/xfs_extfree_item.c
+++ b/fs/xfs/xfs_extfree_item.c
@@ -111,7 +111,7 @@ xfs_efi_item_unpin(xfs_efi_log_item_t *efip, int stale)
 	xfs_mount_t	*mp;
 
 	mp = efip->efi_item.li_mountp;
-	spin_lock(&mp->m_ail_lock);
+	spin_lock(&mp->m_ail->xa_lock);
 	if (efip->efi_flags & XFS_EFI_CANCELED) {
 		/*
 		 * xfs_trans_delete_ail() drops the AIL lock.
@@ -120,7 +120,7 @@ xfs_efi_item_unpin(xfs_efi_log_item_t *efip, int stale)
 		xfs_efi_item_free(efip);
 	} else {
 		efip->efi_flags |= XFS_EFI_COMMITTED;
-		spin_unlock(&mp->m_ail_lock);
+		spin_unlock(&mp->m_ail->xa_lock);
 	}
 }
 
@@ -138,7 +138,7 @@ xfs_efi_item_unpin_remove(xfs_efi_log_item_t *efip, xfs_trans_t *tp)
 	xfs_log_item_desc_t	*lidp;
 
 	mp = efip->efi_item.li_mountp;
-	spin_lock(&mp->m_ail_lock);
+	spin_lock(&mp->m_ail->xa_lock);
 	if (efip->efi_flags & XFS_EFI_CANCELED) {
 		/*
 		 * free the xaction descriptor pointing to this item
@@ -153,7 +153,7 @@ xfs_efi_item_unpin_remove(xfs_efi_log_item_t *efip, xfs_trans_t *tp)
 		xfs_efi_item_free(efip);
 	} else {
 		efip->efi_flags |= XFS_EFI_COMMITTED;
-		spin_unlock(&mp->m_ail_lock);
+		spin_unlock(&mp->m_ail->xa_lock);
 	}
 }
 
@@ -352,7 +352,7 @@ xfs_efi_release(xfs_efi_log_item_t	*efip,
 	ASSERT(efip->efi_next_extent > 0);
 	ASSERT(efip->efi_flags & XFS_EFI_COMMITTED);
 
-	spin_lock(&mp->m_ail_lock);
+	spin_lock(&mp->m_ail->xa_lock);
 	ASSERT(efip->efi_next_extent >= nextents);
 	efip->efi_next_extent -= nextents;
 	extents_left = efip->efi_next_extent;
@@ -363,7 +363,7 @@ xfs_efi_release(xfs_efi_log_item_t	*efip,
 		xfs_trans_delete_ail(mp, (xfs_log_item_t *)efip);
 		xfs_efi_item_free(efip);
 	} else {
-		spin_unlock(&mp->m_ail_lock);
+		spin_unlock(&mp->m_ail->xa_lock);
 	}
 }
 
diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index 448f507..884f22f 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -2718,11 +2718,11 @@ xfs_idestroy(
 		ASSERT(((lip->li_flags & XFS_LI_IN_AIL) == 0) ||
 				       XFS_FORCED_SHUTDOWN(ip->i_mount));
 		if (lip->li_flags & XFS_LI_IN_AIL) {
-			spin_lock(&mp->m_ail_lock);
+			spin_lock(&mp->m_ail->xa_lock);
 			if (lip->li_flags & XFS_LI_IN_AIL)
 				xfs_trans_delete_ail(mp, lip);
 			else
-				spin_unlock(&mp->m_ail_lock);
+				spin_unlock(&mp->m_ail->xa_lock);
 		}
 		xfs_inode_item_destroy(ip);
 		ip->i_itemp = NULL;
diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c
index 97c7452..291d30a 100644
--- a/fs/xfs/xfs_inode_item.c
+++ b/fs/xfs/xfs_inode_item.c
@@ -991,7 +991,7 @@ xfs_iflush_done(
 	 */
 	if (iip->ili_logged &&
 	    (iip->ili_item.li_lsn == iip->ili_flush_lsn)) {
-		spin_lock(&ip->i_mount->m_ail_lock);
+		spin_lock(&ip->i_mount->m_ail->xa_lock);
 		if (iip->ili_item.li_lsn == iip->ili_flush_lsn) {
 			/*
 			 * xfs_trans_delete_ail() drops the AIL lock.
@@ -999,7 +999,7 @@ xfs_iflush_done(
 			xfs_trans_delete_ail(ip->i_mount,
 					     (xfs_log_item_t*)iip);
 		} else {
-			spin_unlock(&ip->i_mount->m_ail_lock);
+			spin_unlock(&ip->i_mount->m_ail->xa_lock);
 		}
 	}
 
@@ -1038,14 +1038,14 @@ xfs_iflush_abort(
 	mp = ip->i_mount;
 	if (iip) {
 		if (iip->ili_item.li_flags & XFS_LI_IN_AIL) {
-			spin_lock(&mp->m_ail_lock);
+			spin_lock(&mp->m_ail->xa_lock);
 			if (iip->ili_item.li_flags & XFS_LI_IN_AIL) {
 				/*
 				 * xfs_trans_delete_ail() drops the AIL lock.
 				 */
 				xfs_trans_delete_ail(mp, (xfs_log_item_t *)iip);
 			} else
-				spin_unlock(&mp->m_ail_lock);
+				spin_unlock(&mp->m_ail->xa_lock);
 		}
 		iip->ili_logged = 0;
 		/*
diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index 70f5063..d3d7042 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -539,7 +539,6 @@ xfs_log_mount(
 	/*
 	 * Initialize the AIL now we have a log.
 	 */
-	spin_lock_init(&mp->m_ail_lock);
 	error = xfs_trans_ail_init(mp);
 	if (error) {
 		cmn_err(CE_WARN, "XFS: AIL initialisation failed: error %d", error);
diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index 46ef060..a2d6d78 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -2684,7 +2684,7 @@ xlog_recover_do_efi_trans(
 	efip->efi_next_extent = efi_formatp->efi_nextents;
 	efip->efi_flags |= XFS_EFI_COMMITTED;
 
-	spin_lock(&mp->m_ail_lock);
+	spin_lock(&mp->m_ail->xa_lock);
 	/*
 	 * xfs_trans_update_ail() drops the AIL lock.
 	 */
@@ -2730,7 +2730,7 @@ xlog_recover_do_efd_trans(
 	 * in the AIL.
 	 */
 	mp = log->l_mp;
-	spin_lock(&mp->m_ail_lock);
+	spin_lock(&mp->m_ail->xa_lock);
 	lip = xfs_trans_ail_cursor_first(mp->m_ail, &cur, 0);
 	while (lip != NULL) {
 		if (lip->li_type == XFS_LI_EFI) {
@@ -2742,14 +2742,14 @@ xlog_recover_do_efd_trans(
 				 */
 				xfs_trans_delete_ail(mp, lip);
 				xfs_efi_item_free(efip);
-				spin_lock(&mp->m_ail_lock);
+				spin_lock(&mp->m_ail->xa_lock);
 				break;
 			}
 		}
 		lip = xfs_trans_ail_cursor_next(mp->m_ail, &cur);
 	}
 	xfs_trans_ail_cursor_done(mp->m_ail, &cur);
-	spin_unlock(&mp->m_ail_lock);
+	spin_unlock(&mp->m_ail->xa_lock);
 }
 
 /*
@@ -3079,7 +3079,7 @@ xlog_recover_process_efis(
 	struct xfs_ail_cursor	cur;
 
 	mp = log->l_mp;
-	spin_lock(&mp->m_ail_lock);
+	spin_lock(&mp->m_ail->xa_lock);
 
 	lip = xfs_trans_ail_cursor_first(mp->m_ail, &cur, 0);
 	while (lip != NULL) {
@@ -3100,16 +3100,16 @@ xlog_recover_process_efis(
 			continue;
 		}
 
-		spin_unlock(&mp->m_ail_lock);
+		spin_unlock(&mp->m_ail->xa_lock);
 		error = xlog_recover_process_efi(mp, efip);
-		spin_lock(&mp->m_ail_lock);
+		spin_lock(&mp->m_ail->xa_lock);
 		if (error)
 			goto out;
 		lip = xfs_trans_ail_cursor_next(mp->m_ail, &cur);
 	}
 out:
 	xfs_trans_ail_cursor_done(mp->m_ail, &cur);
-	spin_unlock(&mp->m_ail_lock);
+	spin_unlock(&mp->m_ail->xa_lock);
 	return error;
 }
 
diff --git a/fs/xfs/xfs_mount.h b/fs/xfs/xfs_mount.h
index a24b407..b9c24c3 100644
--- a/fs/xfs/xfs_mount.h
+++ b/fs/xfs/xfs_mount.h
@@ -228,7 +228,6 @@ extern void	xfs_icsb_sync_counters_locked(struct xfs_mount *, int);
 typedef struct xfs_mount {
 	struct super_block	*m_super;
 	xfs_tid_t		m_tid;		/* next unused tid for fs */
-	spinlock_t		m_ail_lock;	/* fs AIL mutex */
 	struct xfs_ail		*m_ail;		/* fs active log item list */
 	xfs_sb_t		m_sb;		/* copy of fs superblock */
 	spinlock_t		m_sb_lock;	/* sb counter lock */
diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c
index 4e1c22a..99ba0e2 100644
--- a/fs/xfs/xfs_trans.c
+++ b/fs/xfs/xfs_trans.c
@@ -1425,7 +1425,7 @@ xfs_trans_chunk_committed(
 		 * the test below.
 		 */
 		mp = lip->li_mountp;
-		spin_lock(&mp->m_ail_lock);
+		spin_lock(&mp->m_ail->xa_lock);
 		if (XFS_LSN_CMP(item_lsn, lip->li_lsn) > 0) {
 			/*
 			 * This will set the item's lsn to item_lsn
@@ -1436,7 +1436,7 @@ xfs_trans_chunk_committed(
 			 */
 			xfs_trans_update_ail(mp, lip, item_lsn);
 		} else {
-			spin_unlock(&mp->m_ail_lock);
+			spin_unlock(&mp->m_ail->xa_lock);
 		}
 
 		/*
diff --git a/fs/xfs/xfs_trans_ail.c b/fs/xfs/xfs_trans_ail.c
index 0591888..e06b62f 100644
--- a/fs/xfs/xfs_trans_ail.c
+++ b/fs/xfs/xfs_trans_ail.c
@@ -1,5 +1,6 @@
 /*
  * Copyright (c) 2000-2002,2005 Silicon Graphics, Inc.
+ * Copyright (c) 2008 Dave Chinner
  * All Rights Reserved.
  *
  * This program is free software; you can redistribute it and/or
@@ -56,14 +57,14 @@ xfs_trans_ail_tail(
 	xfs_lsn_t	lsn;
 	xfs_log_item_t	*lip;
 
-	spin_lock(&ailp->xa_mount->m_ail_lock);
+	spin_lock(&ailp->xa_lock);
 	lip = xfs_ail_min(ailp);
 	if (lip == NULL) {
 		lsn = (xfs_lsn_t)0;
 	} else {
 		lsn = lip->li_lsn;
 	}
-	spin_unlock(&ailp->xa_mount->m_ail_lock);
+	spin_unlock(&ailp->xa_lock);
 
 	return lsn;
 }
@@ -214,14 +215,14 @@ xfsaild_push(
 	xfs_mount_t	*mp = ailp->xa_mount;
 	struct xfs_ail_cursor	*cur = &ailp->xa_cursors;
 
-	spin_lock(&mp->m_ail_lock);
+	spin_lock(&ailp->xa_lock);
 	lip = xfs_trans_ail_cursor_first(ailp, cur, *last_lsn);
 	if (!lip || XFS_FORCED_SHUTDOWN(mp)) {
 		/*
 		 * AIL is empty or our push has reached the end.
 		 */
 		xfs_trans_ail_cursor_done(ailp, cur);
-		spin_unlock(&mp->m_ail_lock);
+		spin_unlock(&ailp->xa_lock);
 		last_pushed_lsn = 0;
 		return tout;
 	}
@@ -256,7 +257,7 @@ xfsaild_push(
 		 * skip to the next item in the list.
 		 */
 		lock_result = IOP_TRYLOCK(lip);
-		spin_unlock(&mp->m_ail_lock);
+		spin_unlock(&ailp->xa_lock);
 		switch (lock_result) {
 		case XFS_ITEM_SUCCESS:
 			XFS_STATS_INC(xs_push_ail_success);
@@ -293,7 +294,7 @@ xfsaild_push(
 			break;
 		}
 
-		spin_lock(&mp->m_ail_lock);
+		spin_lock(&ailp->xa_lock);
 		/* should we bother continuing? */
 		if (XFS_FORCED_SHUTDOWN(mp))
 			break;
@@ -322,7 +323,7 @@ xfsaild_push(
 		lsn = lip->li_lsn;
 	}
 	xfs_trans_ail_cursor_done(ailp, cur);
-	spin_unlock(&mp->m_ail_lock);
+	spin_unlock(&ailp->xa_lock);
 
 	if (flush_log) {
 		/*
@@ -423,30 +424,31 @@ void
 xfs_trans_update_ail(
 	xfs_mount_t	*mp,
 	xfs_log_item_t	*lip,
-	xfs_lsn_t	lsn) __releases(mp->m_ail_lock)
+	xfs_lsn_t	lsn) __releases(ailp->xa_lock)
 {
-	xfs_log_item_t		*dlip=NULL;
+	struct xfs_ail		*ailp = mp->m_ail;
+	xfs_log_item_t		*dlip = NULL;
 	xfs_log_item_t		*mlip;	/* ptr to minimum lip */
 
-	mlip = xfs_ail_min(mp->m_ail);
+	mlip = xfs_ail_min(ailp);
 
 	if (lip->li_flags & XFS_LI_IN_AIL) {
-		dlip = xfs_ail_delete(mp->m_ail, lip);
+		dlip = xfs_ail_delete(ailp, lip);
 		ASSERT(dlip == lip);
-		xfs_trans_ail_cursor_clear(mp->m_ail, dlip);
+		xfs_trans_ail_cursor_clear(ailp, dlip);
 	} else {
 		lip->li_flags |= XFS_LI_IN_AIL;
 	}
 
 	lip->li_lsn = lsn;
-	xfs_ail_insert(mp->m_ail, lip);
+	xfs_ail_insert(ailp, lip);
 
 	if (mlip == dlip) {
-		mlip = xfs_ail_min(mp->m_ail);
-		spin_unlock(&mp->m_ail_lock);
+		mlip = xfs_ail_min(ailp);
+		spin_unlock(&ailp->xa_lock);
 		xfs_log_move_tail(mp, mlip->li_lsn);
 	} else {
-		spin_unlock(&mp->m_ail_lock);
+		spin_unlock(&ailp->xa_lock);
 	}
 
 
@@ -470,27 +472,28 @@ xfs_trans_update_ail(
 void
 xfs_trans_delete_ail(
 	xfs_mount_t	*mp,
-	xfs_log_item_t	*lip) __releases(mp->m_ail_lock)
+	xfs_log_item_t	*lip) __releases(ailp->xa_lock)
 {
+	struct xfs_ail		*ailp = mp->m_ail;
 	xfs_log_item_t		*dlip;
 	xfs_log_item_t		*mlip;
 
 	if (lip->li_flags & XFS_LI_IN_AIL) {
-		mlip = xfs_ail_min(mp->m_ail);
-		dlip = xfs_ail_delete(mp->m_ail, lip);
+		mlip = xfs_ail_min(ailp);
+		dlip = xfs_ail_delete(ailp, lip);
 		ASSERT(dlip == lip);
-		xfs_trans_ail_cursor_clear(mp->m_ail, dlip);
+		xfs_trans_ail_cursor_clear(ailp, dlip);
 
 
 		lip->li_flags &= ~XFS_LI_IN_AIL;
 		lip->li_lsn = 0;
 
 		if (mlip == dlip) {
-			mlip = xfs_ail_min(mp->m_ail);
-			spin_unlock(&mp->m_ail_lock);
+			mlip = xfs_ail_min(ailp);
+			spin_unlock(&ailp->xa_lock);
 			xfs_log_move_tail(mp, (mlip ? mlip->li_lsn : 0));
 		} else {
-			spin_unlock(&mp->m_ail_lock);
+			spin_unlock(&ailp->xa_lock);
 		}
 	}
 	else {
@@ -498,13 +501,11 @@ xfs_trans_delete_ail(
 		 * If the file system is not being shutdown, we are in
 		 * serious trouble if we get to this stage.
 		 */
-		if (XFS_FORCED_SHUTDOWN(mp))
-			spin_unlock(&mp->m_ail_lock);
-		else {
+		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__);
-			spin_unlock(&mp->m_ail_lock);
 			xfs_force_shutdown(mp, SHUTDOWN_CORRUPT_INCORE);
 		}
 	}
@@ -539,6 +540,7 @@ xfs_trans_ail_init(
 
 	ailp->xa_mount = mp;
 	INIT_LIST_HEAD(&ailp->xa_ail);
+	spin_lock_init(&ailp->xa_lock);
 	error = xfsaild_start(ailp);
 	if (error)
 		goto out_free_ailp;
diff --git a/fs/xfs/xfs_trans_priv.h b/fs/xfs/xfs_trans_priv.h
index c6ea7fe..32f67fa 100644
--- a/fs/xfs/xfs_trans_priv.h
+++ b/fs/xfs/xfs_trans_priv.h
@@ -76,6 +76,7 @@ struct xfs_ail {
 	struct task_struct	*xa_task;
 	xfs_lsn_t		xa_target;
 	struct xfs_ail_cursor	xa_cursors;
+	spinlock_t		xa_lock;
 };
 
 /*
@@ -111,9 +112,9 @@ xfs_trans_ail_copy_lsn(
 	xfs_lsn_t	*src)
 {
 	ASSERT(sizeof(xfs_lsn_t) == 8);	/* don't lock if it shrinks */
-	spin_lock(&ailp->xa_mount->m_ail_lock);
+	spin_lock(&ailp->xa_lock);
 	*dst = *src;
-	spin_unlock(&ailp->xa_mount->m_ail_lock);
+	spin_unlock(&ailp->xa_lock);
 }
 #else
 static inline void
-- 
1.5.6

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH 6/8] XFS: Given the log a pointer to the AIL
  2008-09-13 14:57 [PATCH 0/8] XFS: AIL cleanup and bug fixes Dave Chinner
                   ` (4 preceding siblings ...)
  2008-09-13 14:57 ` [PATCH 5/8] XFS: Move the AIL lock into the struct xfs_ail Dave Chinner
@ 2008-09-13 14:57 ` Dave Chinner
  2008-09-19  9:27   ` Christoph Hellwig
  2008-09-13 14:57 ` [PATCH 7/8] XFS: Add ail pointer into log items Dave Chinner
  2008-09-13 14:57 ` [PATCH 8/8] XFS: Finish removing the mount pointer from the AIL API Dave Chinner
  7 siblings, 1 reply; 21+ messages in thread
From: Dave Chinner @ 2008-09-13 14:57 UTC (permalink / raw)
  To: xfs

When we need to go from the log to the AIL, we have
to go via the xfs_mount. Add a xfs_ail pointer to the log
so we can go directly to the AIL associated with the log.

Signed-off-by: Dave Chinner <david@fromorbit.com>
---
 fs/xfs/xfs_log.c         |    3 ++-
 fs/xfs/xfs_log_priv.h    |    1 +
 fs/xfs/xfs_log_recover.c |   42 +++++++++++++++++++++++-------------------
 3 files changed, 26 insertions(+), 20 deletions(-)

diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index d3d7042..1fb22f8 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -544,6 +544,7 @@ xfs_log_mount(
 		cmn_err(CE_WARN, "XFS: AIL initialisation failed: error %d", error);
 		goto error;
 	}
+	mp->m_log->l_ailp = mp->m_ail;
 
 	/*
 	 * skip log recovery on a norecovery mount.  pretend it all
@@ -880,7 +881,7 @@ xfs_log_need_covered(xfs_mount_t *mp)
 	spin_lock(&log->l_icloglock);
 	if (((log->l_covered_state == XLOG_STATE_COVER_NEED) ||
 		(log->l_covered_state == XLOG_STATE_COVER_NEED2))
-			&& !xfs_trans_ail_tail(mp->m_ail)
+			&& !xfs_trans_ail_tail(log->l_ailp)
 			&& xlog_iclogs_empty(log)) {
 		if (log->l_covered_state == XLOG_STATE_COVER_NEED)
 			log->l_covered_state = XLOG_STATE_COVER_DONE;
diff --git a/fs/xfs/xfs_log_priv.h b/fs/xfs/xfs_log_priv.h
index c8a5b22..641c8f3 100644
--- a/fs/xfs/xfs_log_priv.h
+++ b/fs/xfs/xfs_log_priv.h
@@ -404,6 +404,7 @@ typedef struct xlog_in_core {
 typedef struct log {
 	/* The following fields don't need locking */
 	struct xfs_mount	*l_mp;	        /* mount point */
+	struct xfs_ail		*l_ailp;	/* AIL log is working with */
 	struct xfs_buf		*l_xbuf;        /* extra buffer for log
 						 * wrapping */
 	struct xfs_buftarg	*l_targ;        /* buftarg of log */
diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index a2d6d78..b9c1331 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -54,7 +54,7 @@ STATIC void	xlog_recover_insert_item_backq(xlog_recover_item_t **q,
 					       xlog_recover_item_t *item);
 #if defined(DEBUG)
 STATIC void	xlog_recover_check_summary(xlog_t *);
-STATIC void	xlog_recover_check_ail(xfs_mount_t *, xfs_log_item_t *, 
+STATIC void	xlog_recover_check_ail(struct xfs_ail *, xfs_log_item_t *,
 					struct xfs_ail_cursor *);
 #else
 #define	xlog_recover_check_summary(log)
@@ -2684,7 +2684,7 @@ xlog_recover_do_efi_trans(
 	efip->efi_next_extent = efi_formatp->efi_nextents;
 	efip->efi_flags |= XFS_EFI_COMMITTED;
 
-	spin_lock(&mp->m_ail->xa_lock);
+	spin_lock(&log->l_ailp->xa_lock);
 	/*
 	 * xfs_trans_update_ail() drops the AIL lock.
 	 */
@@ -2713,6 +2713,7 @@ xlog_recover_do_efd_trans(
 	xfs_log_item_t		*lip;
 	__uint64_t		efi_id;
 	struct xfs_ail_cursor	cur;
+	struct xfs_ail		*ailp;
 
 	if (pass == XLOG_RECOVER_PASS1) {
 		return;
@@ -2730,8 +2731,9 @@ xlog_recover_do_efd_trans(
 	 * in the AIL.
 	 */
 	mp = log->l_mp;
-	spin_lock(&mp->m_ail->xa_lock);
-	lip = xfs_trans_ail_cursor_first(mp->m_ail, &cur, 0);
+	ailp = log->l_ailp;
+	spin_lock(&ailp->xa_lock);
+	lip = xfs_trans_ail_cursor_first(ailp, &cur, 0);
 	while (lip != NULL) {
 		if (lip->li_type == XFS_LI_EFI) {
 			efip = (xfs_efi_log_item_t *)lip;
@@ -2742,14 +2744,14 @@ xlog_recover_do_efd_trans(
 				 */
 				xfs_trans_delete_ail(mp, lip);
 				xfs_efi_item_free(efip);
-				spin_lock(&mp->m_ail->xa_lock);
+				spin_lock(&ailp->xa_lock);
 				break;
 			}
 		}
-		lip = xfs_trans_ail_cursor_next(mp->m_ail, &cur);
+		lip = xfs_trans_ail_cursor_next(ailp, &cur);
 	}
-	xfs_trans_ail_cursor_done(mp->m_ail, &cur);
-	spin_unlock(&mp->m_ail->xa_lock);
+	xfs_trans_ail_cursor_done(ailp, &cur);
+	spin_unlock(&ailp->xa_lock);
 }
 
 /*
@@ -3039,13 +3041,13 @@ abort_error:
 #if defined(DEBUG)
 STATIC void
 xlog_recover_check_ail(
-	xfs_mount_t		*mp,
+	struct xfs_ail		*ailp,
 	xfs_log_item_t		*lip,
 	struct xfs_ail_cursor	*cur)
 {
 	do {
 		ASSERT(lip->li_type != XFS_LI_EFI);
-		lip = xfs_trans_ail_cursor_next(mp->m_ail, cur);
+		lip = xfs_trans_ail_cursor_next(ailp, cur);
 	} while (lip != NULL);
 }
 #endif	/* DEBUG */
@@ -3077,17 +3079,19 @@ xlog_recover_process_efis(
 	xfs_mount_t		*mp;
 	int			error = 0;
 	struct xfs_ail_cursor	cur;
+	struct xfs_ail		*ailp;
 
 	mp = log->l_mp;
-	spin_lock(&mp->m_ail->xa_lock);
+	ailp = log->l_ailp;
+	spin_lock(&ailp->xa_lock);
 
-	lip = xfs_trans_ail_cursor_first(mp->m_ail, &cur, 0);
+	lip = xfs_trans_ail_cursor_first(ailp, &cur, 0);
 	while (lip != NULL) {
 		/*
 		 * We're done when we see something other than an EFI.
 		 */
 		if (lip->li_type != XFS_LI_EFI) {
-			xlog_recover_check_ail(mp, lip, &cur);
+			xlog_recover_check_ail(ailp, lip, &cur);
 			break;
 		}
 
@@ -3096,20 +3100,20 @@ xlog_recover_process_efis(
 		 */
 		efip = (xfs_efi_log_item_t *)lip;
 		if (efip->efi_flags & XFS_EFI_RECOVERED) {
-			lip = xfs_trans_ail_cursor_next(mp->m_ail, &cur);
+			lip = xfs_trans_ail_cursor_next(ailp, &cur);
 			continue;
 		}
 
-		spin_unlock(&mp->m_ail->xa_lock);
+		spin_unlock(&ailp->xa_lock);
 		error = xlog_recover_process_efi(mp, efip);
-		spin_lock(&mp->m_ail->xa_lock);
+		spin_lock(&ailp->xa_lock);
 		if (error)
 			goto out;
-		lip = xfs_trans_ail_cursor_next(mp->m_ail, &cur);
+		lip = xfs_trans_ail_cursor_next(ailp, &cur);
 	}
 out:
-	xfs_trans_ail_cursor_done(mp->m_ail, &cur);
-	spin_unlock(&mp->m_ail->xa_lock);
+	xfs_trans_ail_cursor_done(ailp, &cur);
+	spin_unlock(&ailp->xa_lock);
 	return error;
 }
 
-- 
1.5.6

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH 7/8] XFS: Add ail pointer into log items
  2008-09-13 14:57 [PATCH 0/8] XFS: AIL cleanup and bug fixes Dave Chinner
                   ` (5 preceding siblings ...)
  2008-09-13 14:57 ` [PATCH 6/8] XFS: Given the log a pointer to the AIL Dave Chinner
@ 2008-09-13 14:57 ` Dave Chinner
  2008-09-19  9:28   ` Christoph Hellwig
  2008-09-13 14:57 ` [PATCH 8/8] XFS: Finish removing the mount pointer from the AIL API Dave Chinner
  7 siblings, 1 reply; 21+ messages in thread
From: Dave Chinner @ 2008-09-13 14:57 UTC (permalink / raw)
  To: xfs

Add an xfs_ail pointer to log items so that the log items can
reference the AIL directly during callbacks without needed a
struct xfs_mount.

Signed-off-by: Dave Chinner <david@fromorbit.com>
---
 fs/xfs/xfs_buf_item.c     |    5 ++++-
 fs/xfs/xfs_extfree_item.c |   28 ++++++++++++++++++----------
 fs/xfs/xfs_inode_item.c   |    1 +
 fs/xfs/xfs_trans.c        |    9 ++++++---
 fs/xfs/xfs_trans.h        |    1 +
 fs/xfs/xfs_trans_item.c   |   10 ++++++++++
 6 files changed, 40 insertions(+), 14 deletions(-)

diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
index 7f1e266..c170421 100644
--- a/fs/xfs/xfs_buf_item.c
+++ b/fs/xfs/xfs_buf_item.c
@@ -731,6 +731,7 @@ xfs_buf_item_init(
 	bip->bli_item.li_type = XFS_LI_BUF;
 	bip->bli_item.li_ops = &xfs_buf_item_ops;
 	bip->bli_item.li_mountp = mp;
+	bip->bli_item.li_ailp = mp->m_ail;
 	bip->bli_buf = bp;
 	bip->bli_format.blf_type = XFS_LI_BUF;
 	bip->bli_format.blf_blkno = (__int64_t)XFS_BUF_ADDR(bp);
@@ -1117,10 +1118,12 @@ xfs_buf_iodone(
 	xfs_buf_log_item_t	*bip)
 {
 	struct xfs_mount	*mp;
+	struct xfs_ail		*ailp;
 
 	ASSERT(bip->bli_buf == bp);
 
 	mp = bip->bli_item.li_mountp;
+	ailp = bip->bli_item.li_ailp;
 
 	/*
 	 * If we are forcibly shutting down, this may well be
@@ -1131,7 +1134,7 @@ xfs_buf_iodone(
 	 *
 	 * Either way, AIL is useless if we're forcing a shutdown.
 	 */
-	spin_lock(&mp->m_ail->xa_lock);
+	spin_lock(&ailp->xa_lock);
 	/*
 	 * xfs_trans_delete_ail() drops the AIL lock.
 	 */
diff --git a/fs/xfs/xfs_extfree_item.c b/fs/xfs/xfs_extfree_item.c
index f1dcd80..dab5737 100644
--- a/fs/xfs/xfs_extfree_item.c
+++ b/fs/xfs/xfs_extfree_item.c
@@ -108,10 +108,12 @@ xfs_efi_item_pin(xfs_efi_log_item_t *efip)
 STATIC void
 xfs_efi_item_unpin(xfs_efi_log_item_t *efip, int stale)
 {
-	xfs_mount_t	*mp;
+	xfs_mount_t		*mp;
+	struct xfs_ail		*ailp;
 
 	mp = efip->efi_item.li_mountp;
-	spin_lock(&mp->m_ail->xa_lock);
+	ailp = efip->efi_item.li_ailp;
+	spin_lock(&ailp->xa_lock);
 	if (efip->efi_flags & XFS_EFI_CANCELED) {
 		/*
 		 * xfs_trans_delete_ail() drops the AIL lock.
@@ -120,7 +122,7 @@ xfs_efi_item_unpin(xfs_efi_log_item_t *efip, int stale)
 		xfs_efi_item_free(efip);
 	} else {
 		efip->efi_flags |= XFS_EFI_COMMITTED;
-		spin_unlock(&mp->m_ail->xa_lock);
+		spin_unlock(&ailp->xa_lock);
 	}
 }
 
@@ -134,11 +136,13 @@ xfs_efi_item_unpin(xfs_efi_log_item_t *efip, int stale)
 STATIC void
 xfs_efi_item_unpin_remove(xfs_efi_log_item_t *efip, xfs_trans_t *tp)
 {
-	xfs_mount_t	*mp;
+	xfs_mount_t		*mp;
+	struct xfs_ail		*ailp;
 	xfs_log_item_desc_t	*lidp;
 
 	mp = efip->efi_item.li_mountp;
-	spin_lock(&mp->m_ail->xa_lock);
+	ailp = efip->efi_item.li_ailp;
+	spin_lock(&ailp->xa_lock);
 	if (efip->efi_flags & XFS_EFI_CANCELED) {
 		/*
 		 * free the xaction descriptor pointing to this item
@@ -153,7 +157,7 @@ xfs_efi_item_unpin_remove(xfs_efi_log_item_t *efip, xfs_trans_t *tp)
 		xfs_efi_item_free(efip);
 	} else {
 		efip->efi_flags |= XFS_EFI_COMMITTED;
-		spin_unlock(&mp->m_ail->xa_lock);
+		spin_unlock(&ailp->xa_lock);
 	}
 }
 
@@ -268,6 +272,7 @@ xfs_efi_init(xfs_mount_t	*mp,
 	efip->efi_item.li_type = XFS_LI_EFI;
 	efip->efi_item.li_ops = &xfs_efi_item_ops;
 	efip->efi_item.li_mountp = mp;
+	efip->efi_item.li_ailp = mp->m_ail;
 	efip->efi_format.efi_nextents = nextents;
 	efip->efi_format.efi_id = (__psint_t)(void*)efip;
 
@@ -345,14 +350,16 @@ void
 xfs_efi_release(xfs_efi_log_item_t	*efip,
 		uint			nextents)
 {
-	xfs_mount_t	*mp;
-	int		extents_left;
+	xfs_mount_t		*mp;
+	struct xfs_ail		*ailp;
+	int			extents_left;
 
 	mp = efip->efi_item.li_mountp;
+	ailp = efip->efi_item.li_ailp;
 	ASSERT(efip->efi_next_extent > 0);
 	ASSERT(efip->efi_flags & XFS_EFI_COMMITTED);
 
-	spin_lock(&mp->m_ail->xa_lock);
+	spin_lock(&ailp->xa_lock);
 	ASSERT(efip->efi_next_extent >= nextents);
 	efip->efi_next_extent -= nextents;
 	extents_left = efip->efi_next_extent;
@@ -363,7 +370,7 @@ xfs_efi_release(xfs_efi_log_item_t	*efip,
 		xfs_trans_delete_ail(mp, (xfs_log_item_t *)efip);
 		xfs_efi_item_free(efip);
 	} else {
-		spin_unlock(&mp->m_ail->xa_lock);
+		spin_unlock(&ailp->xa_lock);
 	}
 }
 
@@ -565,6 +572,7 @@ xfs_efd_init(xfs_mount_t	*mp,
 	efdp->efd_item.li_type = XFS_LI_EFD;
 	efdp->efd_item.li_ops = &xfs_efd_item_ops;
 	efdp->efd_item.li_mountp = mp;
+	efdp->efd_item.li_ailp = mp->m_ail;
 	efdp->efd_efip = efip;
 	efdp->efd_format.efd_nextents = nextents;
 	efdp->efd_format.efd_efi_id = efip->efi_format.efi_id;
diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c
index 291d30a..47594f4 100644
--- a/fs/xfs/xfs_inode_item.c
+++ b/fs/xfs/xfs_inode_item.c
@@ -932,6 +932,7 @@ xfs_inode_item_init(
 	iip->ili_item.li_type = XFS_LI_INODE;
 	iip->ili_item.li_ops = &xfs_inode_item_ops;
 	iip->ili_item.li_mountp = mp;
+	iip->ili_item.li_ailp = mp->m_ail;
 	iip->ili_inode = ip;
 
 	/*
diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c
index 99ba0e2..5163e12 100644
--- a/fs/xfs/xfs_trans.c
+++ b/fs/xfs/xfs_trans.c
@@ -1383,11 +1383,13 @@ xfs_trans_chunk_committed(
 	xfs_log_item_desc_t	*lidp;
 	xfs_log_item_t		*lip;
 	xfs_lsn_t		item_lsn;
-	struct xfs_mount	*mp;
 	int			i;
 
 	lidp = licp->lic_descs;
 	for (i = 0; i < licp->lic_unused; i++, lidp++) {
+		struct xfs_mount	*mp;
+		struct xfs_ail		*ailp;
+
 		if (xfs_lic_isfree(licp, i)) {
 			continue;
 		}
@@ -1425,7 +1427,8 @@ xfs_trans_chunk_committed(
 		 * the test below.
 		 */
 		mp = lip->li_mountp;
-		spin_lock(&mp->m_ail->xa_lock);
+		ailp = lip->li_ailp;
+		spin_lock(&ailp->xa_lock);
 		if (XFS_LSN_CMP(item_lsn, lip->li_lsn) > 0) {
 			/*
 			 * This will set the item's lsn to item_lsn
@@ -1436,7 +1439,7 @@ xfs_trans_chunk_committed(
 			 */
 			xfs_trans_update_ail(mp, lip, item_lsn);
 		} else {
-			spin_unlock(&mp->m_ail->xa_lock);
+			spin_unlock(&ailp->xa_lock);
 		}
 
 		/*
diff --git a/fs/xfs/xfs_trans.h b/fs/xfs/xfs_trans.h
index ead53bd..d6a9c5c 100644
--- a/fs/xfs/xfs_trans.h
+++ b/fs/xfs/xfs_trans.h
@@ -118,6 +118,7 @@ typedef struct xfs_log_item {
 	xfs_lsn_t			li_lsn;		/* last on-disk lsn */
 	struct xfs_log_item_desc	*li_desc;	/* ptr to current desc*/
 	struct xfs_mount		*li_mountp;	/* ptr to fs mount */
+	struct xfs_ail			*li_ailp;	/* ptr to AIL */
 	uint				li_type;	/* item type */
 	uint				li_flags;	/* misc flags */
 	struct xfs_log_item		*li_bio_list;	/* buffer item list */
diff --git a/fs/xfs/xfs_trans_item.c b/fs/xfs/xfs_trans_item.c
index 3c666e8..e110bf5 100644
--- a/fs/xfs/xfs_trans_item.c
+++ b/fs/xfs/xfs_trans_item.c
@@ -22,6 +22,14 @@
 #include "xfs_inum.h"
 #include "xfs_trans.h"
 #include "xfs_trans_priv.h"
+/* XXX: from here down needed until struct xfs_trans has it's own ailp */
+#include "xfs_bit.h"
+#include "xfs_buf_item.h"
+#include "xfs_sb.h"
+#include "xfs_ag.h"
+#include "xfs_dir2.h"
+#include "xfs_dmapi.h"
+#include "xfs_mount.h"
 
 STATIC int	xfs_trans_unlock_chunk(xfs_log_item_chunk_t *,
 					int, int, xfs_lsn_t);
@@ -79,6 +87,7 @@ xfs_trans_add_item(xfs_trans_t *tp, xfs_log_item_t *lip)
 		lidp->lid_size = 0;
 		lip->li_desc = lidp;
 		lip->li_mountp = tp->t_mountp;
+		lip->li_ailp = tp->t_mountp->m_ail;
 		return lidp;
 	}
 
@@ -120,6 +129,7 @@ xfs_trans_add_item(xfs_trans_t *tp, xfs_log_item_t *lip)
 	lidp->lid_size = 0;
 	lip->li_desc = lidp;
 	lip->li_mountp = tp->t_mountp;
+	lip->li_ailp = tp->t_mountp->m_ail;
 	return lidp;
 }
 
-- 
1.5.6

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH 8/8] XFS: Finish removing the mount pointer from the AIL API
  2008-09-13 14:57 [PATCH 0/8] XFS: AIL cleanup and bug fixes Dave Chinner
                   ` (6 preceding siblings ...)
  2008-09-13 14:57 ` [PATCH 7/8] XFS: Add ail pointer into log items Dave Chinner
@ 2008-09-13 14:57 ` Dave Chinner
  2008-09-19  9:28   ` Christoph Hellwig
  7 siblings, 1 reply; 21+ messages in thread
From: Dave Chinner @ 2008-09-13 14:57 UTC (permalink / raw)
  To: xfs

Change all the remaining AIL API functions that are passed
struct xfs_mount pointers to pass pointers directly to the
struct xfs_ail being used. With this conversion, all external
access to the AIL is via the struct xfs_ail. Hence the operation
and referencing of the AIL is almost entirely independent of
the xfs_mount that is using it - it is now much more tightly
tied to the log and the items it is tracking in the log than
it is tied to the xfs_mount.

Signed-off-by: Dave Chinner <david@fromorbit.com>
---
 fs/xfs/quota/xfs_dquot.c      |   15 +++++++--------
 fs/xfs/quota/xfs_dquot_item.c |    8 +++++---
 fs/xfs/xfs_buf_item.c         |   26 +++++++++-----------------
 fs/xfs/xfs_extfree_item.c     |   35 ++++++++++-------------------------
 fs/xfs/xfs_iget.c             |    4 +++-
 fs/xfs/xfs_inode.c            |    8 ++++----
 fs/xfs/xfs_inode_item.c       |   29 ++++++++++++-----------------
 fs/xfs/xfs_log.c              |    2 +-
 fs/xfs/xfs_log_recover.c      |   13 +++++--------
 fs/xfs/xfs_trans.c            |    6 ++----
 fs/xfs/xfs_trans.h            |    3 ---
 fs/xfs/xfs_trans_ail.c        |   41 +++++++++++++++++++++--------------------
 fs/xfs/xfs_trans_buf.c        |    7 +++----
 fs/xfs/xfs_trans_priv.h       |   15 +++++++++------
 14 files changed, 91 insertions(+), 121 deletions(-)

diff --git a/fs/xfs/quota/xfs_dquot.c b/fs/xfs/quota/xfs_dquot.c
index 29a7aa7..d738d37 100644
--- a/fs/xfs/quota/xfs_dquot.c
+++ b/fs/xfs/quota/xfs_dquot.c
@@ -1321,8 +1321,10 @@ xfs_qm_dqflush_done(
 	xfs_dq_logitem_t	*qip)
 {
 	xfs_dquot_t		*dqp;
+	struct xfs_ail		*ailp;
 
 	dqp = qip->qli_dquot;
+	ailp = qip->qli_item.li_ailp;
 
 	/*
 	 * We only want to pull the item from the AIL if its
@@ -1335,15 +1337,12 @@ xfs_qm_dqflush_done(
 	if ((qip->qli_item.li_flags & XFS_LI_IN_AIL) &&
 	    qip->qli_item.li_lsn == qip->qli_flush_lsn) {
 
-		spin_lock(&dqp->q_mount->m_ail->xa_lock);
-		/*
-		 * xfs_trans_delete_ail() drops the AIL lock.
-		 */
+		/* xfs_trans_ail_delete() drops the AIL lock. */
+		spin_lock(&ailp->xa_lock);
 		if (qip->qli_item.li_lsn == qip->qli_flush_lsn)
-			xfs_trans_delete_ail(dqp->q_mount,
-					     (xfs_log_item_t*)qip);
+			xfs_trans_ail_delete(ailp, (xfs_log_item_t*)qip);
 		else
-			spin_unlock(&dqp->q_mount->m_ail->xa_lock);
+			spin_unlock(&ailp->xa_lock);
 	}
 
 	/*
@@ -1373,7 +1372,7 @@ xfs_dqunlock(
 	mutex_unlock(&(dqp->q_qlock));
 	if (dqp->q_logitem.qli_dquot == dqp) {
 		/* Once was dqp->q_mount, but might just have been cleared */
-		xfs_trans_unlocked_item(dqp->q_logitem.qli_item.li_mountp,
+		xfs_trans_unlocked_item(dqp->q_logitem.qli_item.li_ailp,
 					(xfs_log_item_t*)&(dqp->q_logitem));
 	}
 }
diff --git a/fs/xfs/quota/xfs_dquot_item.c b/fs/xfs/quota/xfs_dquot_item.c
index 478077c..b2f0394 100644
--- a/fs/xfs/quota/xfs_dquot_item.c
+++ b/fs/xfs/quota/xfs_dquot_item.c
@@ -568,14 +568,16 @@ xfs_qm_qoffend_logitem_committed(
 	xfs_lsn_t lsn)
 {
 	xfs_qoff_logitem_t	*qfs;
+	struct xfs_ail		*ailp;
 
 	qfs = qfe->qql_start_lip;
-	spin_lock(&qfs->qql_item.li_mountp->m_ail->xa_lock);
+	ailp = qfs->qql_item.li_ailp;
+	spin_lock(&ailp->xa_lock);
 	/*
 	 * Delete the qoff-start logitem from the AIL.
-	 * xfs_trans_delete_ail() drops the AIL lock.
+	 * xfs_trans_ail_delete() drops the AIL lock.
 	 */
-	xfs_trans_delete_ail(qfs->qql_item.li_mountp, (xfs_log_item_t *)qfs);
+	xfs_trans_ail_delete(ailp, (xfs_log_item_t *)qfs);
 	kmem_free(qfs);
 	kmem_free(qfe);
 	return (xfs_lsn_t)-1;
diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
index c170421..ce80e2c 100644
--- a/fs/xfs/xfs_buf_item.c
+++ b/fs/xfs/xfs_buf_item.c
@@ -375,7 +375,7 @@ xfs_buf_item_unpin(
 	xfs_buf_log_item_t	*bip,
 	int			stale)
 {
-	xfs_mount_t	*mp;
+	struct xfs_ail	*ailp;
 	xfs_buf_t	*bp;
 	int		freed;
 
@@ -387,7 +387,7 @@ xfs_buf_item_unpin(
 	xfs_buftrace("XFS_UNPIN", bp);
 
 	freed = atomic_dec_and_test(&bip->bli_refcount);
-	mp = bip->bli_item.li_mountp;
+	ailp = bip->bli_item.li_ailp;
 	xfs_bunpin(bp);
 	if (freed && stale) {
 		ASSERT(bip->bli_flags & XFS_BLI_STALE);
@@ -399,17 +399,17 @@ xfs_buf_item_unpin(
 		xfs_buftrace("XFS_UNPIN STALE", bp);
 		/*
 		 * If we get called here because of an IO error, we may
-		 * or may not have the item on the AIL. xfs_trans_delete_ail()
+		 * or may not have the item on the AIL. xfs_trans_ail_delete()
 		 * will take care of that situation.
-		 * xfs_trans_delete_ail() drops the AIL lock.
+		 * 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_SET_FSPRIVATE(bp, NULL);
 			XFS_BUF_CLR_IODONE_FUNC(bp);
 		} else {
-			spin_lock(&mp->m_ail->xa_lock);
-			xfs_trans_delete_ail(mp, (xfs_log_item_t *)bip);
+			spin_lock(&ailp->xa_lock);
+			xfs_trans_ail_delete(ailp, (xfs_log_item_t *)bip);
 			xfs_buf_item_relse(bp);
 			ASSERT(XFS_BUF_FSPRIVATE(bp, void *) == NULL);
 		}
@@ -1117,28 +1117,20 @@ xfs_buf_iodone(
 	xfs_buf_t		*bp,
 	xfs_buf_log_item_t	*bip)
 {
-	struct xfs_mount	*mp;
-	struct xfs_ail		*ailp;
+	struct xfs_ail		*ailp = bip->bli_item.li_ailp;
 
 	ASSERT(bip->bli_buf == bp);
-
-	mp = bip->bli_item.li_mountp;
-	ailp = bip->bli_item.li_ailp;
-
 	/*
 	 * If we are forcibly shutting down, this may well be
 	 * off the AIL already. That's because we simulate the
 	 * log-committed callbacks to unpin these buffers. Or we may never
 	 * have put this item on AIL because of the transaction was
-	 * aborted forcibly. xfs_trans_delete_ail() takes care of these.
+	 * aborted forcibly. xfs_trans_ail_delete() takes care of these.
 	 *
 	 * Either way, AIL is useless if we're forcing a shutdown.
 	 */
 	spin_lock(&ailp->xa_lock);
-	/*
-	 * xfs_trans_delete_ail() drops the AIL lock.
-	 */
-	xfs_trans_delete_ail(mp, (xfs_log_item_t *)bip);
+	xfs_trans_ail_delete(ailp, (xfs_log_item_t *)bip);
 
 #ifdef XFS_TRANS_DEBUG
 	kmem_free(bip->bli_orig);
diff --git a/fs/xfs/xfs_extfree_item.c b/fs/xfs/xfs_extfree_item.c
index dab5737..05a4bdd 100644
--- a/fs/xfs/xfs_extfree_item.c
+++ b/fs/xfs/xfs_extfree_item.c
@@ -108,17 +108,12 @@ xfs_efi_item_pin(xfs_efi_log_item_t *efip)
 STATIC void
 xfs_efi_item_unpin(xfs_efi_log_item_t *efip, int stale)
 {
-	xfs_mount_t		*mp;
-	struct xfs_ail		*ailp;
+	struct xfs_ail		*ailp = efip->efi_item.li_ailp;
 
-	mp = efip->efi_item.li_mountp;
-	ailp = efip->efi_item.li_ailp;
 	spin_lock(&ailp->xa_lock);
 	if (efip->efi_flags & XFS_EFI_CANCELED) {
-		/*
-		 * xfs_trans_delete_ail() drops the AIL lock.
-		 */
-		xfs_trans_delete_ail(mp, (xfs_log_item_t *)efip);
+		/* xfs_trans_ail_delete() drops the AIL lock. */
+		xfs_trans_ail_delete(ailp, (xfs_log_item_t *)efip);
 		xfs_efi_item_free(efip);
 	} else {
 		efip->efi_flags |= XFS_EFI_COMMITTED;
@@ -136,12 +131,9 @@ xfs_efi_item_unpin(xfs_efi_log_item_t *efip, int stale)
 STATIC void
 xfs_efi_item_unpin_remove(xfs_efi_log_item_t *efip, xfs_trans_t *tp)
 {
-	xfs_mount_t		*mp;
-	struct xfs_ail		*ailp;
+	struct xfs_ail		*ailp = efip->efi_item.li_ailp;
 	xfs_log_item_desc_t	*lidp;
 
-	mp = efip->efi_item.li_mountp;
-	ailp = efip->efi_item.li_ailp;
 	spin_lock(&ailp->xa_lock);
 	if (efip->efi_flags & XFS_EFI_CANCELED) {
 		/*
@@ -149,11 +141,9 @@ xfs_efi_item_unpin_remove(xfs_efi_log_item_t *efip, xfs_trans_t *tp)
 		 */
 		lidp = xfs_trans_find_item(tp, (xfs_log_item_t *) efip);
 		xfs_trans_free_item(tp, lidp);
-		/*
-		 * pull the item off the AIL.
-		 * xfs_trans_delete_ail() drops the AIL lock.
-		 */
-		xfs_trans_delete_ail(mp, (xfs_log_item_t *)efip);
+
+		/* xfs_trans_ail_delete() drops the AIL lock. */
+		xfs_trans_ail_delete(ailp, (xfs_log_item_t *)efip);
 		xfs_efi_item_free(efip);
 	} else {
 		efip->efi_flags |= XFS_EFI_COMMITTED;
@@ -350,12 +340,9 @@ void
 xfs_efi_release(xfs_efi_log_item_t	*efip,
 		uint			nextents)
 {
-	xfs_mount_t		*mp;
-	struct xfs_ail		*ailp;
+	struct xfs_ail		*ailp = efip->efi_item.li_ailp;
 	int			extents_left;
 
-	mp = efip->efi_item.li_mountp;
-	ailp = efip->efi_item.li_ailp;
 	ASSERT(efip->efi_next_extent > 0);
 	ASSERT(efip->efi_flags & XFS_EFI_COMMITTED);
 
@@ -364,10 +351,8 @@ xfs_efi_release(xfs_efi_log_item_t	*efip,
 	efip->efi_next_extent -= nextents;
 	extents_left = efip->efi_next_extent;
 	if (extents_left == 0) {
-		/*
-		 * xfs_trans_delete_ail() drops the AIL lock.
-		 */
-		xfs_trans_delete_ail(mp, (xfs_log_item_t *)efip);
+		/* 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_iget.c b/fs/xfs/xfs_iget.c
index cd17db9..aceeb7c 100644
--- a/fs/xfs/xfs_iget.c
+++ b/fs/xfs/xfs_iget.c
@@ -38,6 +38,8 @@
 #include "xfs_ialloc.h"
 #include "xfs_quota.h"
 #include "xfs_utils.h"
+#include "xfs_trans_priv.h"
+#include "xfs_inode_item.h"
 
 /*
  * Check the validity of the inode we just found it the cache
@@ -615,7 +617,7 @@ xfs_iunlock(
 		 * it is in the AIL and anyone is waiting on it.  Don't do
 		 * this if the caller has asked us not to.
 		 */
-		xfs_trans_unlocked_item(ip->i_mount,
+		xfs_trans_unlocked_item(ip->i_itemp->ili_item.li_ailp,
 					(xfs_log_item_t*)(ip->i_itemp));
 	}
 	xfs_ilock_trace(ip, 3, lock_flags, (inst_t *)__return_address);
diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index 884f22f..44671dd 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -2712,17 +2712,17 @@ xfs_idestroy(
 		 * inode still in the AIL. If it is there, we should remove
 		 * it to prevent a use-after-free from occurring.
 		 */
-		xfs_mount_t	*mp = ip->i_mount;
 		xfs_log_item_t	*lip = &ip->i_itemp->ili_item;
+		struct xfs_ail	*ailp = lip->li_ailp;
 
 		ASSERT(((lip->li_flags & XFS_LI_IN_AIL) == 0) ||
 				       XFS_FORCED_SHUTDOWN(ip->i_mount));
 		if (lip->li_flags & XFS_LI_IN_AIL) {
-			spin_lock(&mp->m_ail->xa_lock);
+			spin_lock(&ailp->xa_lock);
 			if (lip->li_flags & XFS_LI_IN_AIL)
-				xfs_trans_delete_ail(mp, lip);
+				xfs_trans_ail_delete(ailp, lip);
 			else
-				spin_unlock(&mp->m_ail->xa_lock);
+				spin_unlock(&ailp->xa_lock);
 		}
 		xfs_inode_item_destroy(ip);
 		ip->i_itemp = NULL;
diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c
index 47594f4..aa9bf05 100644
--- a/fs/xfs/xfs_inode_item.c
+++ b/fs/xfs/xfs_inode_item.c
@@ -977,9 +977,8 @@ xfs_iflush_done(
 	xfs_buf_t		*bp,
 	xfs_inode_log_item_t	*iip)
 {
-	xfs_inode_t	*ip;
-
-	ip = iip->ili_inode;
+	xfs_inode_t		*ip = iip->ili_inode;
+	struct xfs_ail		*ailp = iip->ili_item.li_ailp;
 
 	/*
 	 * We only want to pull the item from the AIL if it is
@@ -992,15 +991,12 @@ xfs_iflush_done(
 	 */
 	if (iip->ili_logged &&
 	    (iip->ili_item.li_lsn == iip->ili_flush_lsn)) {
-		spin_lock(&ip->i_mount->m_ail->xa_lock);
+		spin_lock(&ailp->xa_lock);
 		if (iip->ili_item.li_lsn == iip->ili_flush_lsn) {
-			/*
-			 * xfs_trans_delete_ail() drops the AIL lock.
-			 */
-			xfs_trans_delete_ail(ip->i_mount,
-					     (xfs_log_item_t*)iip);
+			/* xfs_trans_ail_delete() drops the AIL lock. */
+			xfs_trans_ail_delete(ailp, (xfs_log_item_t*)iip);
 		} else {
-			spin_unlock(&ip->i_mount->m_ail->xa_lock);
+			spin_unlock(&ailp->xa_lock);
 		}
 	}
 
@@ -1032,21 +1028,20 @@ void
 xfs_iflush_abort(
 	xfs_inode_t		*ip)
 {
-	xfs_inode_log_item_t	*iip;
+	xfs_inode_log_item_t	*iip = ip->i_itemp;
 	xfs_mount_t		*mp;
 
 	iip = ip->i_itemp;
 	mp = ip->i_mount;
 	if (iip) {
+		struct xfs_ail	*ailp = iip->ili_item.li_ailp;
 		if (iip->ili_item.li_flags & XFS_LI_IN_AIL) {
-			spin_lock(&mp->m_ail->xa_lock);
+			spin_lock(&ailp->xa_lock);
 			if (iip->ili_item.li_flags & XFS_LI_IN_AIL) {
-				/*
-				 * xfs_trans_delete_ail() drops the AIL lock.
-				 */
-				xfs_trans_delete_ail(mp, (xfs_log_item_t *)iip);
+				/* xfs_trans_ail_delete() drops the AIL lock. */
+				xfs_trans_ail_delete(ailp, (xfs_log_item_t *)iip);
 			} else
-				spin_unlock(&mp->m_ail->xa_lock);
+				spin_unlock(&ailp->xa_lock);
 		}
 		iip->ili_logged = 0;
 		/*
diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index 1fb22f8..ccf5ea6 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -1381,7 +1381,7 @@ xlog_grant_push_ail(xfs_mount_t	*mp,
      */
     if (threshold_lsn &&
 	!XLOG_FORCED_SHUTDOWN(log))
-	    xfs_trans_push_ail(mp, threshold_lsn);
+	    xfs_trans_ail_push(log->l_ailp, threshold_lsn);
 }	/* xlog_grant_push_ail */
 
 
diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index b9c1331..37c2bf9 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -2686,9 +2686,9 @@ xlog_recover_do_efi_trans(
 
 	spin_lock(&log->l_ailp->xa_lock);
 	/*
-	 * xfs_trans_update_ail() drops the AIL lock.
+	 * xfs_trans_ail_update() drops the AIL lock.
 	 */
-	xfs_trans_update_ail(mp, (xfs_log_item_t *)efip, lsn);
+	xfs_trans_ail_update(log->l_ailp, (xfs_log_item_t *)efip, lsn);
 	return 0;
 }
 
@@ -2707,13 +2707,12 @@ xlog_recover_do_efd_trans(
 	xlog_recover_item_t	*item,
 	int			pass)
 {
-	xfs_mount_t		*mp;
 	xfs_efd_log_format_t	*efd_formatp;
 	xfs_efi_log_item_t	*efip = NULL;
 	xfs_log_item_t		*lip;
 	__uint64_t		efi_id;
 	struct xfs_ail_cursor	cur;
-	struct xfs_ail		*ailp;
+	struct xfs_ail		*ailp = log->l_ailp;
 
 	if (pass == XLOG_RECOVER_PASS1) {
 		return;
@@ -2730,8 +2729,6 @@ xlog_recover_do_efd_trans(
 	 * Search for the efi with the id in the efd format structure
 	 * in the AIL.
 	 */
-	mp = log->l_mp;
-	ailp = log->l_ailp;
 	spin_lock(&ailp->xa_lock);
 	lip = xfs_trans_ail_cursor_first(ailp, &cur, 0);
 	while (lip != NULL) {
@@ -2739,10 +2736,10 @@ xlog_recover_do_efd_trans(
 			efip = (xfs_efi_log_item_t *)lip;
 			if (efip->efi_format.efi_id == efi_id) {
 				/*
-				 * xfs_trans_delete_ail() drops the
+				 * xfs_trans_ail_delete() drops the
 				 * AIL lock.
 				 */
-				xfs_trans_delete_ail(mp, lip);
+				xfs_trans_ail_delete(ailp, lip);
 				xfs_efi_item_free(efip);
 				spin_lock(&ailp->xa_lock);
 				break;
diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c
index 5163e12..ad137ef 100644
--- a/fs/xfs/xfs_trans.c
+++ b/fs/xfs/xfs_trans.c
@@ -1387,7 +1387,6 @@ xfs_trans_chunk_committed(
 
 	lidp = licp->lic_descs;
 	for (i = 0; i < licp->lic_unused; i++, lidp++) {
-		struct xfs_mount	*mp;
 		struct xfs_ail		*ailp;
 
 		if (xfs_lic_isfree(licp, i)) {
@@ -1426,7 +1425,6 @@ xfs_trans_chunk_committed(
 		 * This would cause the earlier transaction to fail
 		 * the test below.
 		 */
-		mp = lip->li_mountp;
 		ailp = lip->li_ailp;
 		spin_lock(&ailp->xa_lock);
 		if (XFS_LSN_CMP(item_lsn, lip->li_lsn) > 0) {
@@ -1435,9 +1433,9 @@ xfs_trans_chunk_committed(
 			 * and update the position of the item in
 			 * the AIL.
 			 *
-			 * xfs_trans_update_ail() drops the AIL lock.
+			 * xfs_trans_ail_update() drops the AIL lock.
 			 */
-			xfs_trans_update_ail(mp, lip, item_lsn);
+			xfs_trans_ail_update(ailp, lip, item_lsn);
 		} else {
 			spin_unlock(&ailp->xa_lock);
 		}
diff --git a/fs/xfs/xfs_trans.h b/fs/xfs/xfs_trans.h
index d6a9c5c..ce28294 100644
--- a/fs/xfs/xfs_trans.h
+++ b/fs/xfs/xfs_trans.h
@@ -979,9 +979,6 @@ void		xfs_trans_cancel(xfs_trans_t *, int);
 int		xfs_trans_roll(struct xfs_trans **, struct xfs_inode *);
 int		xfs_trans_ail_init(struct xfs_mount *);
 void		xfs_trans_ail_destroy(struct xfs_mount *);
-void		xfs_trans_push_ail(struct xfs_mount *, xfs_lsn_t);
-void		xfs_trans_unlocked_item(struct xfs_mount *,
-					xfs_log_item_t *);
 xfs_log_busy_slot_t *xfs_trans_add_busy(xfs_trans_t *tp,
 					xfs_agnumber_t ag,
 					xfs_extlen_t idx);
diff --git a/fs/xfs/xfs_trans_ail.c b/fs/xfs/xfs_trans_ail.c
index e06b62f..929fb58 100644
--- a/fs/xfs/xfs_trans_ail.c
+++ b/fs/xfs/xfs_trans_ail.c
@@ -86,16 +86,16 @@ xfs_trans_ail_tail(
  * any of the objects, so the lock is not needed.
  */
 void
-xfs_trans_push_ail(
-	xfs_mount_t		*mp,
-	xfs_lsn_t		threshold_lsn)
+xfs_trans_ail_push(
+	struct xfs_ail	*ailp,
+	xfs_lsn_t	threshold_lsn)
 {
-	xfs_log_item_t		*lip;
+	xfs_log_item_t	*lip;
 
-	lip = xfs_ail_min(mp->m_ail);
-	if (lip && !XFS_FORCED_SHUTDOWN(mp)) {
-		if (XFS_LSN_CMP(threshold_lsn, mp->m_ail->xa_target) > 0)
-			xfsaild_wakeup(mp->m_ail, threshold_lsn);
+	lip = xfs_ail_min(ailp);
+	if (lip && !XFS_FORCED_SHUTDOWN(ailp->xa_mount)) {
+		if (XFS_LSN_CMP(threshold_lsn, ailp->xa_target) > 0)
+			xfsaild_wakeup(ailp, threshold_lsn);
 	}
 }
 
@@ -373,7 +373,7 @@ xfsaild_push(
  */
 void
 xfs_trans_unlocked_item(
-	xfs_mount_t	*mp,
+	struct xfs_ail	*ailp,
 	xfs_log_item_t	*lip)
 {
 	xfs_log_item_t	*min_lip;
@@ -385,7 +385,7 @@ xfs_trans_unlocked_item(
 	 * over some potentially valid data.
 	 */
 	if (!(lip->li_flags & XFS_LI_IN_AIL) ||
-	    XFS_FORCED_SHUTDOWN(mp)) {
+	    XFS_FORCED_SHUTDOWN(ailp->xa_mount)) {
 		return;
 	}
 
@@ -401,10 +401,10 @@ xfs_trans_unlocked_item(
 	 * the call to xfs_log_move_tail() doesn't do anything if there's
 	 * not enough free space to wake people up so we're safe calling it.
 	 */
-	min_lip = xfs_ail_min(mp->m_ail);
+	min_lip = xfs_ail_min(ailp);
 
 	if (min_lip == lip)
-		xfs_log_move_tail(mp, 1);
+		xfs_log_move_tail(ailp->xa_mount, 1);
 }	/* xfs_trans_unlocked_item */
 
 
@@ -421,12 +421,11 @@ xfs_trans_unlocked_item(
  * is dropped before returning.
  */
 void
-xfs_trans_update_ail(
-	xfs_mount_t	*mp,
+xfs_trans_ail_update(
+	struct xfs_ail	*ailp,
 	xfs_log_item_t	*lip,
 	xfs_lsn_t	lsn) __releases(ailp->xa_lock)
 {
-	struct xfs_ail		*ailp = mp->m_ail;
 	xfs_log_item_t		*dlip = NULL;
 	xfs_log_item_t		*mlip;	/* ptr to minimum lip */
 
@@ -446,7 +445,7 @@ xfs_trans_update_ail(
 	if (mlip == dlip) {
 		mlip = xfs_ail_min(ailp);
 		spin_unlock(&ailp->xa_lock);
-		xfs_log_move_tail(mp, mlip->li_lsn);
+		xfs_log_move_tail(ailp->xa_mount, mlip->li_lsn);
 	} else {
 		spin_unlock(&ailp->xa_lock);
 	}
@@ -470,11 +469,10 @@ xfs_trans_update_ail(
  * is dropped before returning.
  */
 void
-xfs_trans_delete_ail(
-	xfs_mount_t	*mp,
+xfs_trans_ail_delete(
+	struct xfs_ail	*ailp,
 	xfs_log_item_t	*lip) __releases(ailp->xa_lock)
 {
-	struct xfs_ail		*ailp = mp->m_ail;
 	xfs_log_item_t		*dlip;
 	xfs_log_item_t		*mlip;
 
@@ -491,7 +489,8 @@ xfs_trans_delete_ail(
 		if (mlip == dlip) {
 			mlip = xfs_ail_min(ailp);
 			spin_unlock(&ailp->xa_lock);
-			xfs_log_move_tail(mp, (mlip ? mlip->li_lsn : 0));
+			xfs_log_move_tail(ailp->xa_mount,
+						(mlip ? mlip->li_lsn : 0));
 		} else {
 			spin_unlock(&ailp->xa_lock);
 		}
@@ -501,6 +500,8 @@ xfs_trans_delete_ail(
 		 * 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,
diff --git a/fs/xfs/xfs_trans_buf.c b/fs/xfs/xfs_trans_buf.c
index 4e855b5..8ee2f8c 100644
--- a/fs/xfs/xfs_trans_buf.c
+++ b/fs/xfs/xfs_trans_buf.c
@@ -527,9 +527,8 @@ xfs_trans_brelse(xfs_trans_t	*tp,
 			lip = XFS_BUF_FSPRIVATE(bp, xfs_log_item_t *);
 			if (lip->li_type == XFS_LI_BUF) {
 				bip = XFS_BUF_FSPRIVATE(bp,xfs_buf_log_item_t*);
-				xfs_trans_unlocked_item(
-						bip->bli_item.li_mountp,
-						lip);
+				xfs_trans_unlocked_item(bip->bli_item.li_ailp,
+							lip);
 			}
 		}
 		xfs_buf_relse(bp);
@@ -626,7 +625,7 @@ xfs_trans_brelse(xfs_trans_t	*tp,
 	 * tell the AIL that the buffer is being unlocked.
 	 */
 	if (bip != NULL) {
-		xfs_trans_unlocked_item(bip->bli_item.li_mountp,
+		xfs_trans_unlocked_item(bip->bli_item.li_ailp,
 					(xfs_log_item_t*)bip);
 	}
 
diff --git a/fs/xfs/xfs_trans_priv.h b/fs/xfs/xfs_trans_priv.h
index 32f67fa..dc10af4 100644
--- a/fs/xfs/xfs_trans_priv.h
+++ b/fs/xfs/xfs_trans_priv.h
@@ -82,12 +82,15 @@ struct xfs_ail {
 /*
  * From xfs_trans_ail.c
  */
-void			xfs_trans_update_ail(struct xfs_mount *mp,
-				     struct xfs_log_item *lip, xfs_lsn_t lsn)
-				     __releases(mp->m_ail_lock);
-void			xfs_trans_delete_ail(struct xfs_mount *mp,
-				     struct xfs_log_item *lip)
-				     __releases(mp->m_ail_lock);
+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_delete(struct xfs_ail *ailp,
+					struct xfs_log_item *lip)
+					__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 *);
 
 xfs_lsn_t		xfs_trans_ail_tail(struct xfs_ail *ailp);
 
-- 
1.5.6

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* Re: [PATCH 1/8] XFS: Allocate the struct xfs_ail
  2008-09-13 14:57 ` [PATCH 1/8] XFS: Allocate the struct xfs_ail Dave Chinner
@ 2008-09-19  9:15   ` Christoph Hellwig
  0 siblings, 0 replies; 21+ messages in thread
From: Christoph Hellwig @ 2008-09-19  9:15 UTC (permalink / raw)
  To: Dave Chinner; +Cc: xfs

On Sun, Sep 14, 2008 at 12:57:50AM +1000, Dave Chinner wrote:
> Rather than embedding the struct xfs_ail in the struct xfs_mount,
> allocate it during AIL initialisation. Add a back pointer to
> the struct xfs_ail so that we can pass around the xfs_ail
> and still be able to access the xfs_mount if need be. This
> is th first step involved in isolating the AIL implementation
> from the surrounding filesystem code.

Makes sense.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 2/8] XFS: Use a cursor for AIL traversal.
  2008-09-13 14:57 ` [PATCH 2/8] XFS: Use a cursor for AIL traversal Dave Chinner
@ 2008-09-19  9:24   ` Christoph Hellwig
  2008-09-24  3:11     ` Dave Chinner
  0 siblings, 1 reply; 21+ messages in thread
From: Christoph Hellwig @ 2008-09-19  9:24 UTC (permalink / raw)
  To: Dave Chinner; +Cc: xfs

On Sun, Sep 14, 2008 at 12:57:51AM +1000, Dave Chinner wrote:
> To replace the current generation number ensuring sanity of the AIL
> traversal, replace it with an external cursor that is linked to the
> AIL.
> 
> Basically, we store the next item in the cursor whenever we want to
> drop the AIL lock to do something to the current item.  When we
> regain the lock. the current item may already be free, so we can't
> reference it, but the next item in the traversal is already held in
> the cursor.
> 
> When we move or delete an object, we search all the active cursors
> and if there is an item match we clear the cursor(s) that point to
> the object. This forces the traversal to restart transparently.
> 
> We don't invalidate the cursor on insert because the cursor still
> points to a valid item. If the intem is inserted between the current
> item and the cursor it does not matter; the traversal is considered
> to be past the insertion point so it will be picked up in the next
> traversal.
> 
> Hence traversal restarts pretty much disappear altogether with this
> method of traversal, which should substantially reduce the overhead
> of pushing on a busy AIL.

>  {
> -	int			orig_gen = gen;
> -
>  	do {
>  		ASSERT(lip->li_type != XFS_LI_EFI);
> -		lip = xfs_trans_next_ail(mp, lip, &gen, NULL);
> -		/*
> -		 * The check will be bogus if we restart from the
> -		 * beginning of the AIL, so ASSERT that we don't.
> -		 * We never should since we're holding the AIL lock
> -		 * the entire time.
> -		 */
> -		ASSERT(gen == orig_gen);
> +		lip = xfs_trans_next_ail(mp, cur);
>  	} while (lip != NULL);

	for (tmp = lip ; tmp = xfs_rans_next_ail(mp, tmp, &gen, NULL); tmp)
		ASSERT(tmp->li_type != XFS_LI_EFI);

?
		
> +void
> +xfs_trans_ail_cursor_init(

A little comment describing the function would be nice.

> +	struct xfs_ail		*ailp,
> +	struct xfs_ail_cursor	*cur)
> +{
> +	cur->item = NULL;
> +	if (cur == &ailp->xa_cursors)
> +		return;

What is this check for?  It mans we'll do nothing if the cursor is the
one embedded into the ail.  But we should never do this anyway, should
we?

> +	cur->next = ailp->xa_cursors.next;
> +	ailp->xa_cursors.next = cur;

> +/*
> + * Set the cursor to the next item, because when we look
> + * up the cursor the current item may have been freed.
> + */
> +STATIC void
> +xfs_trans_ail_cursor_set(
> +	struct xfs_ail		*ailp,
> +	struct xfs_ail_cursor	*cur,
> +	struct xfs_log_item	*lip)
> +{
> +	if (lip)
> +		cur->item = xfs_ail_next(ailp, lip);
> +}

Does it make sense to have the NULL check here and not in the caller?

> +STATIC struct xfs_log_item *
> +xfs_trans_ail_cursor_next(
> +	struct xfs_ail		*ailp,
> +	struct xfs_ail_cursor	*cur)
> +{
> +	struct xfs_log_item	*lip = cur->item;
> +
> +	xfs_trans_ail_cursor_set(ailp, cur, lip);
> +	return lip;
> +}

I'd say kill this wrapper, it's only used once anyway.

> +void
> +xfs_trans_ail_cursor_done(
> +	struct xfs_ail		*ailp,
> +	struct xfs_ail_cursor	*done)

A little comment describing it, please.

> +	if (done == &ailp->xa_cursors)
> +		return;
> +	prev = &ailp->xa_cursors;
> +	for (cur = prev->next; cur; prev = cur, cur = prev->next) {
> +		if (cur == done) {
> +			prev->next = cur->next;
> +			break;
> +		}
> +	}
> +}

Add an assert somewhere that the cursor actually is on the list?

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 3/8] XFS: move the AIl traversal over to a consistent interface
  2008-09-13 14:57 ` [PATCH 3/8] XFS: move the AIl traversal over to a consistent interface Dave Chinner
@ 2008-09-19  9:26   ` Christoph Hellwig
  0 siblings, 0 replies; 21+ messages in thread
From: Christoph Hellwig @ 2008-09-19  9:26 UTC (permalink / raw)
  To: Dave Chinner; +Cc: xfs

On Sun, Sep 14, 2008 at 12:57:52AM +1000, Dave Chinner wrote:
> With the new cursor interface, it makes sense to make all the
> traversing code use the cursor interface and make the old one go
> away. This means more of the AIL interfacing is done by passing
> struct xfs_ail pointers around the place instead of struct
> xfs_mount pointers.
> 
> We can replace the use of xfs_trans_first_ail() in
> xfs_log_need_covered() as it is only checking if the AIL is empty.
> We can do that with a call to xfs_trans_ail_tail() instead, where a
> zero LSN returned indicates and empty AIL...

Looks good.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 4/8] XFS: Allow 64 bit machines to avoid the AIL lock during flushes
  2008-09-13 14:57 ` [PATCH 4/8] XFS: Allow 64 bit machines to avoid the AIL lock during flushes Dave Chinner
@ 2008-09-19  9:26   ` Christoph Hellwig
  0 siblings, 0 replies; 21+ messages in thread
From: Christoph Hellwig @ 2008-09-19  9:26 UTC (permalink / raw)
  To: Dave Chinner; +Cc: xfs

On Sun, Sep 14, 2008 at 12:57:53AM +1000, Dave Chinner wrote:
> When copying lsn's from the log item to the inode or dquot
> flush lsn, we currently grab the AIL lock. We do this because the
> LSN is a 64 bit quantity and it needs to be read atomically.
> The lock is used to guarantee atomicity for 32 bit platforms.
> 
> Make the LSN copying a small function, and make the function
> used conditional on BITS_PER_LONG so that 64 bit machines don't
> need to take the AIL lock in these places.

Looks good.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 5/8] XFS: Move the AIL lock into the struct xfs_ail
  2008-09-13 14:57 ` [PATCH 5/8] XFS: Move the AIL lock into the struct xfs_ail Dave Chinner
@ 2008-09-19  9:26   ` Christoph Hellwig
  0 siblings, 0 replies; 21+ messages in thread
From: Christoph Hellwig @ 2008-09-19  9:26 UTC (permalink / raw)
  To: Dave Chinner; +Cc: xfs

On Sun, Sep 14, 2008 at 12:57:54AM +1000, Dave Chinner wrote:
> Bring the ail lock inside the struct xfs_ail. This means
> the AIL can be entirely manipulated via the struct xfs_ail rather
> than needing both the struct xfs_mount and the struct xfs_ail.

Looks good.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 6/8] XFS: Given the log a pointer to the AIL
  2008-09-13 14:57 ` [PATCH 6/8] XFS: Given the log a pointer to the AIL Dave Chinner
@ 2008-09-19  9:27   ` Christoph Hellwig
  2008-09-20  6:50     ` Dave Chinner
  0 siblings, 1 reply; 21+ messages in thread
From: Christoph Hellwig @ 2008-09-19  9:27 UTC (permalink / raw)
  To: Dave Chinner; +Cc: xfs

On Sun, Sep 14, 2008 at 12:57:55AM +1000, Dave Chinner wrote:
> When we need to go from the log to the AIL, we have
> to go via the xfs_mount. Add a xfs_ail pointer to the log
> so we can go directly to the AIL associated with the log.

Looks correct, but I wonder what it actually buys us.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 7/8] XFS: Add ail pointer into log items
  2008-09-13 14:57 ` [PATCH 7/8] XFS: Add ail pointer into log items Dave Chinner
@ 2008-09-19  9:28   ` Christoph Hellwig
  2008-09-20  6:34     ` Dave Chinner
  0 siblings, 1 reply; 21+ messages in thread
From: Christoph Hellwig @ 2008-09-19  9:28 UTC (permalink / raw)
  To: Dave Chinner; +Cc: xfs

On Sun, Sep 14, 2008 at 12:57:56AM +1000, Dave Chinner wrote:
> Add an xfs_ail pointer to log items so that the log items can
> reference the AIL directly during callbacks without needed a
> struct xfs_mount.

Does it matter? I'd be a nice cleanup if you managed to get rid of
li_mountp, but without that I don't quite see the point.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 8/8] XFS: Finish removing the mount pointer from the AIL API
  2008-09-13 14:57 ` [PATCH 8/8] XFS: Finish removing the mount pointer from the AIL API Dave Chinner
@ 2008-09-19  9:28   ` Christoph Hellwig
  0 siblings, 0 replies; 21+ messages in thread
From: Christoph Hellwig @ 2008-09-19  9:28 UTC (permalink / raw)
  To: Dave Chinner; +Cc: xfs

On Sun, Sep 14, 2008 at 12:57:57AM +1000, Dave Chinner wrote:
> Change all the remaining AIL API functions that are passed
> struct xfs_mount pointers to pass pointers directly to the
> struct xfs_ail being used. With this conversion, all external
> access to the AIL is via the struct xfs_ail. Hence the operation
> and referencing of the AIL is almost entirely independent of
> the xfs_mount that is using it - it is now much more tightly
> tied to the log and the items it is tracking in the log than
> it is tied to the xfs_mount.

Looks good.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 7/8] XFS: Add ail pointer into log items
  2008-09-19  9:28   ` Christoph Hellwig
@ 2008-09-20  6:34     ` Dave Chinner
  0 siblings, 0 replies; 21+ messages in thread
From: Dave Chinner @ 2008-09-20  6:34 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: xfs

On Fri, Sep 19, 2008 at 05:28:20AM -0400, Christoph Hellwig wrote:
> On Sun, Sep 14, 2008 at 12:57:56AM +1000, Dave Chinner wrote:
> > Add an xfs_ail pointer to log items so that the log items can
> > reference the AIL directly during callbacks without needed a
> > struct xfs_mount.
> 
> Does it matter? I'd be a nice cleanup if you managed to get rid of
> li_mountp, but without that I don't quite see the point.

I haven't gone as far as removing the li_mountp yet - my intention
is to do so, but I haven't written all the patches to do it yet.

The way the li_mountp is used in some subsystems was very indirect
in some cases (e.g. in the quota code) so I wanted to look a little
more at it before deciding the best way to remove as much
indirection as possible in those cases instead of adding more. It
may be that adding xfs_mount pointers into some other structures is
needed to clean this up totally...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 6/8] XFS: Given the log a pointer to the AIL
  2008-09-19  9:27   ` Christoph Hellwig
@ 2008-09-20  6:50     ` Dave Chinner
  0 siblings, 0 replies; 21+ messages in thread
From: Dave Chinner @ 2008-09-20  6:50 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: xfs

On Fri, Sep 19, 2008 at 05:27:22AM -0400, Christoph Hellwig wrote:
> On Sun, Sep 14, 2008 at 12:57:55AM +1000, Dave Chinner wrote:
> > When we need to go from the log to the AIL, we have
> > to go via the xfs_mount. Add a xfs_ail pointer to the log
> > so we can go directly to the AIL associated with the log.
> 
> Looks correct, but I wonder what it actually buys us.

Not much initially.

The original intent of the log manager was that each
log could host transactions for multiple XFS filesystems,
which cannot be done if the log directly references a specific
filesystem. The log needs to be independent of an xfs_mount
for this sort of arrangement to work properly, and the only
time the log should reference a struct xfs_mount is through
a log item that needs to reference filesystem specific information
(another reason I haven't removed the li_mountp yet).

And, of course, then there's the other way around - multiple
logs in the one filesystem - the log needs a direct pointer
to the AIL that it puts all it's items into rather than the
xfs_mount, likewise the AIL needs a backpointer to the log....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 2/8] XFS: Use a cursor for AIL traversal.
  2008-09-19  9:24   ` Christoph Hellwig
@ 2008-09-24  3:11     ` Dave Chinner
  0 siblings, 0 replies; 21+ messages in thread
From: Dave Chinner @ 2008-09-24  3:11 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: xfs

On Fri, Sep 19, 2008 at 05:24:44AM -0400, Christoph Hellwig wrote:
> On Sun, Sep 14, 2008 at 12:57:51AM +1000, Dave Chinner wrote:
> > -	int			orig_gen = gen;
> > -
> >  	do {
> >  		ASSERT(lip->li_type != XFS_LI_EFI);
> > -		lip = xfs_trans_next_ail(mp, lip, &gen, NULL);
> > -		/*
> > -		 * The check will be bogus if we restart from the
> > -		 * beginning of the AIL, so ASSERT that we don't.
> > -		 * We never should since we're holding the AIL lock
> > -		 * the entire time.
> > -		 */
> > -		ASSERT(gen == orig_gen);
> > +		lip = xfs_trans_next_ail(mp, cur);
> >  	} while (lip != NULL);
> 
> 	for (tmp = lip ; tmp = xfs_rans_next_ail(mp, tmp, &gen, NULL); tmp)
> 		ASSERT(tmp->li_type != XFS_LI_EFI);
> 
> ?

Doesn't really matter. I'd prefer to avoid needing another temporary
variable, so it can become:

	for (; lip; lip = xfs_trans_next_ail(mp, cur))
		ASSERT(lip->li_type != XFS_LI_EFI);

And at that point, I can probably kill the debug wrapper function
altogether. Hmmm - will the compiler optimise out the empty loop?
(i.e. do i need #ifdef DEBUG around it?)

> > +void
> > +xfs_trans_ail_cursor_init(
> 
> A little comment describing the function would be nice.

Sure.

> > +	struct xfs_ail		*ailp,
> > +	struct xfs_ail_cursor	*cur)
> > +{
> > +	cur->item = NULL;
> > +	if (cur == &ailp->xa_cursors)
> > +		return;
> 
> What is this check for?  It mans we'll do nothing if the cursor is the
> one embedded into the ail.  But we should never do this anyway, should
> we?

It means that we don't link the embedded push cursor into the list.
That is, initialisation of the push cursor simply clears the "next
item".

> > +/*
> > + * Set the cursor to the next item, because when we look
> > + * up the cursor the current item may have been freed.
> > + */
> > +STATIC void
> > +xfs_trans_ail_cursor_set(
> > +	struct xfs_ail		*ailp,
> > +	struct xfs_ail_cursor	*cur,
> > +	struct xfs_log_item	*lip)
> > +{
> > +	if (lip)
> > +		cur->item = xfs_ail_next(ailp, lip);
> > +}
> 
> Does it make sense to have the NULL check here and not in the caller?

Doesn't really matter - this saves having to check in every place
we call the function....

> 
> > +STATIC struct xfs_log_item *
> > +xfs_trans_ail_cursor_next(
> > +	struct xfs_ail		*ailp,
> > +	struct xfs_ail_cursor	*cur)
> > +{
> > +	struct xfs_log_item	*lip = cur->item;
> > +
> > +	xfs_trans_ail_cursor_set(ailp, cur, lip);
> > +	return lip;
> > +}
> 
> I'd say kill this wrapper, it's only used once anyway.

Ah - should be at least twice - I note that the push code uses
the xfs_mount version rather than the xfs_ail version.

Also, I realised that I forgot to add the traversal restart code
into this function, so it's definitely necessary....

> > +void
> > +xfs_trans_ail_cursor_done(
> > +	struct xfs_ail		*ailp,
> > +	struct xfs_ail_cursor	*done)
> 
> A little comment describing it, please.

Done.

> > +	if (done == &ailp->xa_cursors)
> > +		return;
> > +	prev = &ailp->xa_cursors;
> > +	for (cur = prev->next; cur; prev = cur, cur = prev->next) {
> > +		if (cur == done) {
> > +			prev->next = cur->next;
> > +			break;
> > +		}
> > +	}
> > +}
> 
> Add an assert somewhere that the cursor actually is on the list?

Done.

I'll repost the entire patch series with the fixes in a while

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH 6/8] XFS: Given the log a pointer to the AIL
  2008-10-07 22:13 [PATCH 0/8] XFS: AIL cleanup and bug fixes Dave Chinner
@ 2008-10-07 22:13 ` Dave Chinner
  0 siblings, 0 replies; 21+ messages in thread
From: Dave Chinner @ 2008-10-07 22:13 UTC (permalink / raw)
  To: xfs

When we need to go from the log to the AIL, we have
to go via the xfs_mount. Add a xfs_ail pointer to the log
so we can go directly to the AIL associated with the log.

Signed-off-by: Dave Chinner <david@fromorbit.com>
---
 fs/xfs/xfs_log.c         |    3 ++-
 fs/xfs/xfs_log_priv.h    |    1 +
 fs/xfs/xfs_log_recover.c |   42 +++++++++++++++++++++---------------------
 3 files changed, 24 insertions(+), 22 deletions(-)

diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index ef84ad7..fc327a9 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -572,6 +572,7 @@ xfs_log_mount(
 		cmn_err(CE_WARN, "XFS: AIL initialisation failed: error %d", error);
 		goto error;
 	}
+	mp->m_log->l_ailp = mp->m_ail;
 
 	/*
 	 * skip log recovery on a norecovery mount.  pretend it all
@@ -908,7 +909,7 @@ xfs_log_need_covered(xfs_mount_t *mp)
 	spin_lock(&log->l_icloglock);
 	if (((log->l_covered_state == XLOG_STATE_COVER_NEED) ||
 		(log->l_covered_state == XLOG_STATE_COVER_NEED2))
-			&& !xfs_trans_ail_tail(mp->m_ail)
+			&& !xfs_trans_ail_tail(log->l_ailp)
 			&& xlog_iclogs_empty(log)) {
 		if (log->l_covered_state == XLOG_STATE_COVER_NEED)
 			log->l_covered_state = XLOG_STATE_COVER_DONE;
diff --git a/fs/xfs/xfs_log_priv.h b/fs/xfs/xfs_log_priv.h
index e7d8f84..de7ef6c 100644
--- a/fs/xfs/xfs_log_priv.h
+++ b/fs/xfs/xfs_log_priv.h
@@ -404,6 +404,7 @@ typedef struct xlog_in_core {
 typedef struct log {
 	/* The following fields don't need locking */
 	struct xfs_mount	*l_mp;	        /* mount point */
+	struct xfs_ail		*l_ailp;	/* AIL log is working with */
 	struct xfs_buf		*l_xbuf;        /* extra buffer for log
 						 * wrapping */
 	struct xfs_buftarg	*l_targ;        /* buftarg of log */
diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index 58de100..22751f2 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -2681,7 +2681,7 @@ xlog_recover_do_efi_trans(
 	efip->efi_next_extent = efi_formatp->efi_nextents;
 	efip->efi_flags |= XFS_EFI_COMMITTED;
 
-	spin_lock(&mp->m_ail->xa_lock);
+	spin_lock(&log->l_ailp->xa_lock);
 	/*
 	 * xfs_trans_update_ail() drops the AIL lock.
 	 */
@@ -2710,6 +2710,7 @@ xlog_recover_do_efd_trans(
 	xfs_log_item_t		*lip;
 	__uint64_t		efi_id;
 	struct xfs_ail_cursor	cur;
+	struct xfs_ail		*ailp;
 
 	if (pass == XLOG_RECOVER_PASS1) {
 		return;
@@ -2727,8 +2728,9 @@ xlog_recover_do_efd_trans(
 	 * in the AIL.
 	 */
 	mp = log->l_mp;
-	spin_lock(&mp->m_ail->xa_lock);
-	lip = xfs_trans_ail_cursor_first(mp->m_ail, &cur, 0);
+	ailp = log->l_ailp;
+	spin_lock(&ailp->xa_lock);
+	lip = xfs_trans_ail_cursor_first(ailp, &cur, 0);
 	while (lip != NULL) {
 		if (lip->li_type == XFS_LI_EFI) {
 			efip = (xfs_efi_log_item_t *)lip;
@@ -2739,14 +2741,14 @@ xlog_recover_do_efd_trans(
 				 */
 				xfs_trans_delete_ail(mp, lip);
 				xfs_efi_item_free(efip);
-				spin_lock(&mp->m_ail->xa_lock);
+				spin_lock(&ailp->xa_lock);
 				break;
 			}
 		}
-		lip = xfs_trans_ail_cursor_next(mp->m_ail, &cur);
+		lip = xfs_trans_ail_cursor_next(ailp, &cur);
 	}
-	xfs_trans_ail_cursor_done(mp->m_ail, &cur);
-	spin_unlock(&mp->m_ail->xa_lock);
+	xfs_trans_ail_cursor_done(ailp, &cur);
+	spin_unlock(&ailp->xa_lock);
 }
 
 /*
@@ -3053,14 +3055,13 @@ xlog_recover_process_efis(
 {
 	xfs_log_item_t		*lip;
 	xfs_efi_log_item_t	*efip;
-	xfs_mount_t		*mp;
 	int			error = 0;
 	struct xfs_ail_cursor	cur;
+	struct xfs_ail		*ailp;
 
-	mp = log->l_mp;
-	spin_lock(&mp->m_ail->xa_lock);
-
-	lip = xfs_trans_ail_cursor_first(mp->m_ail, &cur, 0);
+	ailp = log->l_ailp;
+	spin_lock(&ailp->xa_lock);
+	lip = xfs_trans_ail_cursor_first(ailp, &cur, 0);
 	while (lip != NULL) {
 		/*
 		 * We're done when we see something other than an EFI.
@@ -3068,8 +3069,7 @@ xlog_recover_process_efis(
 		 */
 		if (lip->li_type != XFS_LI_EFI) {
 #ifdef DEBUG
-			for (; lip;
-			       lip = xfs_trans_ail_cursor_next(mp->m_ail, &cur))
+			for (; lip; lip = xfs_trans_ail_cursor_next(ailp, &cur))
 				ASSERT(lip->li_type != XFS_LI_EFI);
 #endif
 			break;
@@ -3080,20 +3080,20 @@ xlog_recover_process_efis(
 		 */
 		efip = (xfs_efi_log_item_t *)lip;
 		if (efip->efi_flags & XFS_EFI_RECOVERED) {
-			lip = xfs_trans_ail_cursor_next(mp->m_ail, &cur);
+			lip = xfs_trans_ail_cursor_next(ailp, &cur);
 			continue;
 		}
 
-		spin_unlock(&mp->m_ail->xa_lock);
-		error = xlog_recover_process_efi(mp, efip);
-		spin_lock(&mp->m_ail->xa_lock);
+		spin_unlock(&ailp->xa_lock);
+		error = xlog_recover_process_efi(log->l_mp, efip);
+		spin_lock(&ailp->xa_lock);
 		if (error)
 			goto out;
-		lip = xfs_trans_ail_cursor_next(mp->m_ail, &cur);
+		lip = xfs_trans_ail_cursor_next(ailp, &cur);
 	}
 out:
-	xfs_trans_ail_cursor_done(mp->m_ail, &cur);
-	spin_unlock(&mp->m_ail->xa_lock);
+	xfs_trans_ail_cursor_done(ailp, &cur);
+	spin_unlock(&ailp->xa_lock);
 	return error;
 }
 
-- 
1.5.6.5

^ permalink raw reply related	[flat|nested] 21+ messages in thread

end of thread, other threads:[~2008-10-07 22:12 UTC | newest]

Thread overview: 21+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2008-09-13 14:57 [PATCH 0/8] XFS: AIL cleanup and bug fixes Dave Chinner
2008-09-13 14:57 ` [PATCH 1/8] XFS: Allocate the struct xfs_ail Dave Chinner
2008-09-19  9:15   ` Christoph Hellwig
2008-09-13 14:57 ` [PATCH 2/8] XFS: Use a cursor for AIL traversal Dave Chinner
2008-09-19  9:24   ` Christoph Hellwig
2008-09-24  3:11     ` Dave Chinner
2008-09-13 14:57 ` [PATCH 3/8] XFS: move the AIl traversal over to a consistent interface Dave Chinner
2008-09-19  9:26   ` Christoph Hellwig
2008-09-13 14:57 ` [PATCH 4/8] XFS: Allow 64 bit machines to avoid the AIL lock during flushes Dave Chinner
2008-09-19  9:26   ` Christoph Hellwig
2008-09-13 14:57 ` [PATCH 5/8] XFS: Move the AIL lock into the struct xfs_ail Dave Chinner
2008-09-19  9:26   ` Christoph Hellwig
2008-09-13 14:57 ` [PATCH 6/8] XFS: Given the log a pointer to the AIL Dave Chinner
2008-09-19  9:27   ` Christoph Hellwig
2008-09-20  6:50     ` Dave Chinner
2008-09-13 14:57 ` [PATCH 7/8] XFS: Add ail pointer into log items Dave Chinner
2008-09-19  9:28   ` Christoph Hellwig
2008-09-20  6:34     ` Dave Chinner
2008-09-13 14:57 ` [PATCH 8/8] XFS: Finish removing the mount pointer from the AIL API Dave Chinner
2008-09-19  9:28   ` Christoph Hellwig
  -- strict thread matches above, loose matches on Subject: below --
2008-10-07 22:13 [PATCH 0/8] XFS: AIL cleanup and bug fixes Dave Chinner
2008-10-07 22:13 ` [PATCH 6/8] XFS: Given the log a pointer to the AIL Dave Chinner

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox