xfs
[Top] [All Lists]

Re: On-stack work item completion race? (was Re: XFS crash?)

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: On-stack work item completion race? (was Re: XFS crash?)
From: Tejun Heo <tj@xxxxxxxxxx>
Date: Mon, 23 Jun 2014 23:25:21 -0400
Cc: Austin Schuh <austin@xxxxxxxxxxxxxxxx>, xfs <xfs@xxxxxxxxxxx>, linux-kernel@xxxxxxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
Dkim-signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=sender:date:from:to:cc:subject:message-id:references:mime-version :content-type:content-disposition:in-reply-to:user-agent; bh=VCf1srj3zk4OmtGPVtu9WjCwwd2VREcEm8w2NjOJanU=; b=e+3C/R8K4f7TrAuPTUva2AyoZx0GFQMJIvupOAMsbyJay3rD9Dr/iW3HpFlwI2i/kK 7RVSteG9hA8N77Bzxjj+X4UlqlvV0eLfvO98WMUMvOh+3xJREE57JVKVMNgXpM5Sgehp BYXS3GXoj6vyQupOszel33aEMG4czgvaJIvFDvG5aax3KrblQlz65EqX6NtMw+2Xr+VG v9TyAD82K5LBNO7MZnuwVXlWHn1Vx4UhlhoxJqh0kU/aldcWlfTdepTP5DrSifYEOoY7 5Q7BaOX5ZRcQSd54VjMI3ID0+x7wf8S29ySCRyeQseCZNB/CW1P66DkQNrisCAbBAXTx 6P6Q==
In-reply-to: <20140624030240.GB9508@dastard>
References: <CANGgnMb=2dYGQO4K36pQ9LEb8E4rT6S_VskLF+n=ndd0_kJr_g@xxxxxxxxxxxxxx> <CANGgnMa80WwQ8zSkL52yYegmQURVQeZiBFv41=FQXMZJ_NaEDw@xxxxxxxxxxxxxx> <20140513034647.GA5421@dastard> <CANGgnMZ0q9uE3NHj2i0SBK1d0vdKLx7QBJeFNb+YwP-5EAmejQ@xxxxxxxxxxxxxx> <20140513063943.GQ26353@dastard> <CANGgnMYn++1++UyX+D2d9GxPxtytpQJv0ThFwdxM-yX7xDWqiA@xxxxxxxxxxxxxx> <20140513090321.GR26353@dastard> <CANGgnMZqQc_NeaDpO_aX+bndmHrQ9VWo9mkfxhPBkRD-J=N6sQ@xxxxxxxxxxxxxx> <CANGgnMZ8OwzfBj5m9H7c6q2yahGhU7oFZLsJfVxnWoqZExkZmQ@xxxxxxxxxxxxxx> <20140624030240.GB9508@dastard>
Sender: Tejun Heo <htejun@xxxxxxxxx>
User-agent: Mutt/1.5.23 (2014-03-12)
Hello,

On Tue, Jun 24, 2014 at 01:02:40PM +1000, Dave Chinner wrote:
> start_flush_work() is effectively a special queue_work()
> implementation, so if if it's not safe to call complete() from the
> workqueue as the above patch implies then this code has the same
> problem.
> 
> Tejun - is this "do it yourself completion" a known issue w.r.t.
> workqueues? I can't find any documentation that says "don't do
> that" so...?

It's more complex than using flush_work() but there's nothing
fundamentally wrong with it.  A work item is completely unlinked
before its execution starts.  It's safe to free the work item once its
work function started, whether through kfree() or returning.

One difference between flush_work() and manual completion would be
that if the work item gets requeued, flush_work() would wait for the
queued one to finish but given the work item is one-shot this doesn't
make any difference.

I can see no reason why manual completion would behave differently
from flush_work() in this case.

> As I understand it, what then happens is that the workqueue code
> grabs another kworker thread and runs the next work item in it's
> queue. IOWs, work items can block, but doing that does not prevent
> execution of other work items queued on other work queues or even on
> the same work queue. Tejun, did I get that correct?

Yes, as long as the workqueue is under its @max_active limit and has
access to an existing kworker or can create a new one, it'll start
executing the next work item immediately; however, the guaranteed
level of concurrency is 1 even for WQ_RECLAIM workqueues.  IOW, the
work items queued on a workqueue must be able to make forward progress
with single work item if the work items are being depended upon for
memory reclaim.

> Hence the work on the xfs-data queue will block until another
> kworker processes the item on the xfs-alloc-wq which means progress
> is made and the inode gets unlocked. Then the kworker for the work
> on the xfs-data queue will get the lock, complete it's work and
> everything has resolved itself.

As long as a WQ_RECLAIM workqueue dosen't depend upon itself,
forward-progress is guaranteed.

Thanks.

-- 
tejun

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