[Top] [All Lists]

Re: [patch 01/12] xfs: split tail_lsn assignments from log space wakeups

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Subject: Re: [patch 01/12] xfs: split tail_lsn assignments from log space wakeups
From: Mark Tinguely <tinguely@xxxxxxx>
Date: Tue, 17 Jan 2012 12:48:10 -0600
Cc: xfs@xxxxxxxxxxx
In-reply-to: <20111212141433.542846138@xxxxxxxxxxxxxxxxxxxxxx>
References: <20111212141346.986825692@xxxxxxxxxxxxxxxxxxxxxx> <20111212141433.542846138@xxxxxxxxxxxxxxxxxxxxxx>
User-agent: Mozilla/5.0 (X11; U; FreeBSD amd64; en-US; rv: Gecko/20111206 Thunderbird/3.1.16
On 01/-10/63 13:59, Christoph Hellwig wrote:
Currently xfs_log_move_tail has a tail_lsn argument that is horribly
overloaded: it may contain either an actual lsn to assign to the log tail,
0 as a special case to use the last sync LSN, or 1 to indicate that no tail
LSN assignment should be performed, and we should opportunisticly wake up
at least one task waiting for log space.

I read the code as opportunistically waking at MOST one task per call.

Remove the tail lsn assigned from xfs_log_move_tail and make the two callers
use xlog_assign_tail_lsn instead of the current variant of partially using
the code in xfs_log_move_tail and partially opencoding it.  Note that means
we grow an addition lock roundtrip on the AIL lock for each bulk update
or delete, which is still far less than what we had before introducing the
bulk operations.  If this proves to be a problem we can still add a variant
of xlog_assign_tail_lsn that expects the lock to be held already.

Just looking at it the additional unlock/lock sequence did not appear too major.

Also rename the remainder of xfs_log_move_tail to xfs_log_space_wake as
that name describes its functionality much better.


Index: xfs/fs/xfs/xfs_trans_ail.c
--- xfs.orig/fs/xfs/xfs_trans_ail.c     2011-11-29 08:38:46.870067201 +0100
+++ xfs/fs/xfs/xfs_trans_ail.c  2011-11-29 08:38:48.580057936 +0100
@@ -643,15 +643,15 @@ xfs_trans_unlocked_item(
         * at the tail, it doesn't matter what result we get back.  This
         * is slightly racy because since we were just unlocked, we could
         * go to sleep between the call to xfs_ail_min and the call to
-        * xfs_log_move_tail, have someone else lock us, commit to us disk,
+        * xfs_log_space_wake, have someone else lock us, commit to us disk,
         * move us out of the tail of the AIL, and then we wake up.  However,
-        * the call to xfs_log_move_tail() doesn't do anything if there's
+        * the call to xfs_log_space_wake() 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(ailp);

        if (min_lip == lip)
-               xfs_log_move_tail(ailp->xa_mount, 1);
+               xfs_log_space_wake(ailp->xa_mount, 1);
  }     /* xfs_trans_unlocked_item */

Looks great. Just to be consistent, you could call the above as:

+               xfs_log_space_wake(ailp->xa_mount, true);

Reviewed-by: Mark Tinguely <tinguely@xxxxxxx>

<Prev in Thread] Current Thread [Next in Thread>