xfs
[Top] [All Lists]

Re: [PATCH 1/4] xfs: wake up cil->xc_commit_wait while removing ctx from

To: Jeff Liu <jeff.liu@xxxxxxxxxx>
Subject: Re: [PATCH 1/4] xfs: wake up cil->xc_commit_wait while removing ctx from cil->xc_committing
From: Mark Tinguely <tinguely@xxxxxxx>
Date: Fri, 03 Jan 2014 09:30:38 -0600
Cc: Dave Chinner <david@xxxxxxxxxxxxx>, "xfs@xxxxxxxxxxx" <xfs@xxxxxxxxxxx>
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <52C6B86C.6060407@xxxxxxxxxx>
References: <52B98292.5040002@xxxxxxxxxx> <52C18F56.70709@xxxxxxx> <52C4286C.5040007@xxxxxxxxxx> <20140102004503.GN20579@dastard> <52C69035.7010606@xxxxxxxxxx> <52C6B86C.6060407@xxxxxxxxxx>
User-agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:10.0.2) Gecko/20120216 Thunderbird/10.0.2
On 1/3/2014 7:17 AM, Jeff Liu wrote:
On 01/03 2014 18:25 PM, Jeff Liu wrote:
On 01/02 2014 08:45, Dave Chinner wrote:
On Wed, Jan 01, 2014 at 10:38:36PM +0800, Jeff Liu wrote:
On 12/30 2013 23:20 PM, Mark Tinguely wrote:
On 12/24/13 06:48, Jeff Liu wrote:
From: Jie Liu<jeff.liu@xxxxxxxxxx>

I can easily to hit a hang up while running fsstress and shutting down
XFS on SSD via the tests below:
<snip>
     Task1                    Task2

                      list_add(&ctx->committing,&cil->xc_committing);

xlog_wait(&cil->xc_commit_wait..)
schedule()...

                      Aborting!! list_del(&ctx->committing);
                      wake_up_all(&cil->xc_commit_wait);<-- MISSING!

As a result, we should handle this situation in xlog_cil_committed().

Signed-off-by: Jie Liu<jeff.liu@xxxxxxxxxx>
---
   fs/xfs/xfs_log_cil.c | 2 ++
   1 file changed, 2 insertions(+)

diff --git a/fs/xfs/xfs_log_cil.c b/fs/xfs/xfs_log_cil.c
index 5eb51fc..8c7e9c7 100644
--- a/fs/xfs/xfs_log_cil.c
+++ b/fs/xfs/xfs_log_cil.c
@@ -406,6 +406,8 @@ xlog_cil_committed(

       spin_lock(&ctx->cil->xc_push_lock);
       list_del(&ctx->committing);
+    if (abort)
+        wake_up_all(&ctx->cil->xc_commit_wait);
       spin_unlock(&ctx->cil->xc_push_lock);

       xlog_cil_free_logvec(ctx->lv_chain);
Hi Jeff, I hope you had a good break,
Thanks :)
So you are saying the wakeup in the CIL push error path missing?
Yes.

I agree with that. But I don't like adding a new wake up to
xlog_cil_committed(), which is after the log buffer is written.
Hi Mark, any particular reason why you don't like this? It would be
great if you could explain why you don't like something up front so
we don't have to guess at your reasons or wait for another round

My concern is consistency, with the patch there will be two paths that could do the wake up.

Originally, the wakeup happened before the iclog write. With the patch, if the cil push sequence successfully wrote it's ticket, woke up the waiters, wrote back the iclog, and then had an error writing the iclog, it would wakeup the xc_commit_wait a second time. Not too drastic of a problem, because the zeroed commit_lsn will prevent an premature write of the next cil push. I just prefer to handle the error in the cil push routine and avoid
a second form of wake up.

--Mark.


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