On 02/19/14 18:23, Dave Chinner wrote:
On Wed, Feb 19, 2014 at 01:24:54PM -0500, Brian Foster wrote:
On 02/18/2014 11:16 PM, Dave Chinner wrote:
From: Dave Chinner<dchinner@xxxxxxxxxx>
Log forces can occur deep in the call chain when we have relatively
little stack free. Log forces can also happen at close to the call
chain leaves (e.g. xfs_buf_lock()) and hence we can trigger IO from
places where we really don't want to add more stack overhead.
This stack overhead occurs because log forces do foreground CIL
pushes (xlog_cil_push_foreground()) rather than waking the
background push wq and waiting for the for the push to complete.
This foreground push was done to avoid confusing the CFQ Io
scheduler when fsync()s were issued, as it has trouble dealing with
dependent IOs being issued from different process contexts.
Avoiding blowing the stack is much more critical than performance
optimisations for CFQ, especially as we've been recommending against
the use of CFQ for XFS since 3.2 kernels were release because of
it's problems with multi-threaded IO workloads.
Hence convert xlog_cil_push_foreground() to move the push work
to the CIL workqueue. We already do the waiting for the push to
complete in xlog_cil_force_lsn(), so there's nothing else we need to
modify to make this work.
Signed-off-by: Dave Chinner<dchinner@xxxxxxxxxx>
---
fs/xfs/xfs_log_cil.c | 52 +++++++++++++++++++++++++++++++++++++++-------------
1 file changed, 39 insertions(+), 13 deletions(-)
diff --git a/fs/xfs/xfs_log_cil.c b/fs/xfs/xfs_log_cil.c
index b57a8e0..7e54553 100644
--- a/fs/xfs/xfs_log_cil.c
+++ b/fs/xfs/xfs_log_cil.c
@@ -499,13 +499,6 @@ xlog_cil_push(
cil->xc_ctx = new_ctx;
/*
- * mirror the new sequence into the cil structure so that we can do
- * unlocked checks against the current sequence in log forces without
- * risking deferencing a freed context pointer.
- */
- cil->xc_current_sequence = new_ctx->sequence;
-
- /*
* The switch is now done, so we can drop the context lock and move out
* of a shared context. We can't just go straight to the commit record,
* though - we need to synchronise with previous and future commits so
@@ -523,8 +516,15 @@ xlog_cil_push(
* Hence we need to add this context to the committing context list so
* that higher sequences will wait for us to write out a commit record
* before they do.
+ *
+ * xfs_log_force_lsn requires us to mirror the new sequence into the cil
+ * structure atomically with the addition of this sequence to the
+ * committing list. This also ensures that we can do unlocked checks
+ * against the current sequence in log forces without risking
+ * deferencing a freed context pointer.
*/
spin_lock(&cil->xc_push_lock);
+ cil->xc_current_sequence = new_ctx->sequence;
list_add(&ctx->committing,&cil->xc_committing);
spin_unlock(&cil->xc_push_lock);
up_write(&cil->xc_ctx_lock);
@@ -662,8 +662,14 @@ xlog_cil_push_background(
}
+/*
+ * xlog_cil_push_now() is used to trigger an immediate CIL push to the sequence
+ * number that is passed. When it returns, the work will be queued for
+ * @push_seq, but it won't be completed. The caller is expected to do any
+ * waiting for push_seq to complete if it is required.
+ */
static void
-xlog_cil_push_foreground(
+xlog_cil_push_now(
struct xlog *log,
xfs_lsn_t push_seq)
{
@@ -688,10 +694,8 @@ xlog_cil_push_foreground(
}
cil->xc_push_seq = push_seq;
+ queue_work(log->l_mp->m_cil_workqueue,&cil->xc_push_work);
spin_unlock(&cil->xc_push_lock);
-
- /* do the push now */
- xlog_cil_push(log);
}
bool
@@ -795,7 +799,8 @@ xlog_cil_force_lsn(
* xlog_cil_push() handles racing pushes for the same sequence,
* so no need to deal with it here.
*/
- xlog_cil_push_foreground(log, sequence);
+restart:
+ xlog_cil_push_now(log, sequence);
/*
* See if we can find a previous sequence still committing.
@@ -803,7 +808,6 @@ xlog_cil_force_lsn(
* before allowing the force of push_seq to go ahead. Hence block
* on commits for those as well.
*/
-restart:
spin_lock(&cil->xc_push_lock);
list_for_each_entry(ctx,&cil->xc_committing, committing) {
if (ctx->sequence> sequence)
@@ -821,6 +825,28 @@ restart:
/* found it! */
commit_lsn = ctx->commit_lsn;
}
+
+ /*
+ * The call to xlog_cil_push_now() executes the push in the background.
+ * Hence by the time we have got here it our sequence may not have been
+ * pushed yet. This is true if the current sequence still matches the
+ * push sequence after the above wait loop and the CIL still contains
+ * dirty objects.
+ *
+ * When the push occurs, it will empty the CIL and
+ * atomically increment the currect sequence past the push sequence and
+ * move it into the committing list. Of course, if the CIL is clean at
+ * the time of the push, it won't have pushed the CIL at all, so in that
+ * case we should try the push for this sequence again from the start
+ * just in case.
+ */
+
+ if (sequence == cil->xc_current_sequence&&
+ !list_empty(&cil->xc_cil)) {
+ spin_unlock(&cil->xc_push_lock);
+ goto restart;
+ }
+
IIUC, the objective here is to make sure we don't leave this code path
before the push even starts and the ctx makes it onto the committing
list, due to xlog_cil_push_now() moving things to a workqueue.
Right.
Given that, what's the purpose of re-executing the background push as
opposed to restarting the wait sequence (as done previously)? It looks
like push_now() won't queue the work again due to cil->xc_push_seq, but
it will flush the queue and I suppose make it more likely the push
starts. Is that the intent?
Effectively. But the other thing that it is protecting against is
that foreground push is done without holding the cil->xc_ctx_lock,
and so we can get the situation where we try a foreground push
of the current sequence, see that the CIL is empty and return
without pushing, wait for previous sequences to commit, then find
that the CIL has items on the CIL in the sequence we are supposed to
be committing.
In this case, we don't know if this occurred because the workqueue
has not started working on our push, or whether we raced on an empty
CIL, and hence we need to make sure that everything in the sequence
we are support to commit is pushed to the log.
Hence if the current sequence is dirty after we've ensure that all
prior sequences are fully checkpointed, need to go back and
push the CIL again to ensure that when we return to the caller the
CIL is checkpointed up to the point in time of the log force
occurring.
The desired push sequence was taken from an item on the CIL (either when
added or from a pinned item). How could the CIL now be empty other than
someone else pushed to at least the desire sequence?
A flush_work() should be enough in the case where the ctx of the desire
sequence is not on the xc_committing list. The flush_work will wait for
the worker to start and place the ctx of the desired sequence into the
xc_committing list. This preventing a tight loop waiting for the cil
push worker to start.
Starting the cil push worker for every wakeup of smaller sequence in the
list_for_each_entry loop seems wasteful.
We know the later error paths in xfs_cil_push() will not do a wake, now
is a good time to fix that.
--Mark.
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
