Re: [PATCH 2/5] xfs: use a cursor for bulk AIL insertion

Christoph Hellwig
2011-07-04T21:21:03Z

FYI: the following patch implementing my suggested cleanups survived a
few rounds of xfsqa:

Index: xfs/fs/xfs/xfs_trans_ail.c
===================================================================
--- xfs.orig/fs/xfs/xfs_trans_ail.c 2011-07-04 16:04:08.932174424 +0200
+++ xfs/fs/xfs/xfs_trans_ail.c 2011-07-04 16:12:51.402677292 +0200
@@ -267,26 +267,14 @@ out:
static struct xfs_log_item *
__xfs_trans_ail_cursor_last(
struct xfs_ail *ailp,
- struct xfs_ail_cursor *cur,
- xfs_lsn_t lsn,
- bool do_init)
+ xfs_lsn_t lsn)
{
- xfs_log_item_t *lip = NULL;
-
- if (do_init)
- xfs_trans_ail_cursor_init(ailp, cur);
+ struct xfs_log_item *lip;

- if (list_empty(&ailp->xa_ail))
- goto out;
-
- list_for_each_entry_reverse(lip, &ailp->xa_ail, li_ail) {
+ list_for_each_entry_reverse(lip, &ailp->xa_ail, li_ail)
if (XFS_LSN_CMP(lip->li_lsn, lsn) <= 0)
- break;
- }
-out:
- if (cur)
- cur->item = lip;
- return lip;
+ return lip;
+ return NULL;
}

/*
@@ -300,7 +288,9 @@ xfs_trans_ail_cursor_last(
struct xfs_ail_cursor *cur,
xfs_lsn_t lsn)
{
- return __xfs_trans_ail_cursor_last(ailp, cur, lsn, true);
+ xfs_trans_ail_cursor_init(ailp, cur);
+ cur->item = __xfs_trans_ail_cursor_last(ailp, lsn);
+ return cur->item;
}

/*
@@ -319,27 +309,22 @@ xfs_ail_splice(
struct xfs_log_item *lip = cur ? cur->item : NULL;
struct xfs_log_item *next_lip;

- do {
- /* no placeholder, so get our insert location */
- if (!lip)
- lip = __xfs_trans_ail_cursor_last(ailp, cur,
- lsn, false);
-
+ /*
+ * Get a new cursor if we do not have a placeholder or an
+ * invalidated one.
+ */
+ if (!lip || (__psint_t)lip & 1) {
+ lip = __xfs_trans_ail_cursor_last(ailp, lsn);
if (!lip) {
/*
- * The list is empty, so just splice and return. Our
- * cursor is already guaranteed to be up to date, so we
- * don't need to touch it here.
+ * The list is empty, so just splice and return.
*/
+ if (cur)
+ cur->item = NULL;
list_splice(list, &ailp->xa_ail);
return;
}
-
- /* The placeholder was invalidated, need to get a new cursor */
- if ((__psint_t)lip & 1)
- lip = NULL;
-
- } while (lip == NULL);
+ }

/*
* Our cursor points to the item we want to insert _after_, so we have