public inbox for linux-xfs@vger.kernel.org
 help / color / mirror / Atom feed
From: Tejun Heo <tj@kernel.org>
To: Christoph Hellwig <hch@infradead.org>
Cc: Stefan Priebe <s.priebe@profihost.ag>,
	Markus Trippelsdorf <markus@trippelsdorf.de>,
	xfs@oss.sgi.com
Subject: Re: [PATCH 3/4] xfs: revert to using a kthread for AIL pushing
Date: Mon, 10 Oct 2011 11:37:30 -0700	[thread overview]
Message-ID: <20111010183730.GJ8100@google.com> (raw)
In-Reply-To: <20111010132611.GA1248@infradead.org>

Hello,

On Mon, Oct 10, 2011 at 09:26:11AM -0400, Christoph Hellwig wrote:
> On Mon, Oct 10, 2011 at 07:55:46AM +0200, Markus Trippelsdorf wrote:
> > Wouldn't it be possible to verify that the problem also goes away with
> > this simple one liner?
> 
> We've been through a few variants, and none fixed it while Stefan had
> to try them on production machines.
> 
> To be honest I'm not convinced at all that a workqueue was such a good
> idea for the ail in particular.  It works extremly well for things were
> we can easily define a work item, e.g. an object that gets queued up
> and a method on it gets exectured.  But for the AIL we really have
> a changing target that needs more or less constant pushing, and the
> target keeps changing while executing our work.  Conceptually it fits
> the idea of an thread much better, with the added benefit of not relying
> on finding a combination of workqueue flags that gets the exact
> behaviour (exectuion ASAP without any limits because of other items
> or required memory allocation).
> 
> And unlike the various per-cpu threads we used to have it is only one
> thread per filesystem anyway.

I don't know xfs internals at all so I don't have too strong an
opinion at this point but don't we at least need to understand what's
going on?  CPU_INTENSIVE / HIGHPRI flags shouldn't cause deadlock
unless some work items are doing busy looping waiting for another work
item to do something (busy yielding might achieve similar effect tho).
They don't change forward progress guarantee.

The only thing which can cause stall is lack of MEM_RECLAIM.  One
thing to be careful about is that each wq has only one rescuer, so if
more than one work items have inter-dependency, it might still lead to
deadlock and they need to be served by different workqueues.

The reasons for moving away from using kthread directly are two folded
- resources and correctness.  I've gone through a number of kthread
users during auditing freezer usage recently and more than half of
them get the synchronization against kthread_stop() or freezer wrong
(to be fair, the rules are quite tricky).  The problem with those bugs
is that they are really obscure race conditions and won't trigger
easily.

Thank you.

-- 
tejun

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

  reply	other threads:[~2011-10-10 18:37 UTC|newest]

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2011-10-06 18:32 [PATCH 0/4] fix AIL pushing under heavy concurrent metadata loads Christoph Hellwig
2011-10-06 18:32 ` [PATCH 1/4] xfs: do not update xa_last_pushed_lsn for locked items Christoph Hellwig
2011-10-07 22:18   ` Alex Elder
2011-10-10  1:37   ` Dave Chinner
2011-10-06 18:32 ` [PATCH 2/4] xfs: force the log if we encounter pinned buffers in .iop_pushbuf Christoph Hellwig
2011-10-07 22:18   ` Alex Elder
2011-10-10  1:39   ` Dave Chinner
2011-10-06 18:33 ` [PATCH 3/4] xfs: revert to using a kthread for AIL pushing Christoph Hellwig
2011-10-07 22:18   ` Alex Elder
2011-10-10  1:45   ` Dave Chinner
2011-10-10  5:55     ` Markus Trippelsdorf
2011-10-10  6:06       ` Stefan Priebe - Profihost AG
2011-10-10 13:26       ` Christoph Hellwig
2011-10-10 18:37         ` Tejun Heo [this message]
2011-10-19 11:16       ` Stefan Priebe - Profihost AG
2011-10-19 11:34         ` Christoph Hellwig
2011-10-19 13:10           ` Stefan Priebe - Profihost AG
2011-10-06 18:33 ` [PATCH 4/4] xfs: add AIL pushing tracepoints Christoph Hellwig
2011-10-07 22:18   ` Alex Elder
2011-10-10  1:45   ` Dave Chinner
  -- strict thread matches above, loose matches on Subject: below --
2011-10-11 15:14 [PATCH 0/4] fix AIL pushing under heavy concurrent metadata loads V2 Christoph Hellwig
2011-10-11 15:14 ` [PATCH 3/4] xfs: revert to using a kthread for AIL pushing Christoph Hellwig

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20111010183730.GJ8100@google.com \
    --to=tj@kernel.org \
    --cc=hch@infradead.org \
    --cc=markus@trippelsdorf.de \
    --cc=s.priebe@profihost.ag \
    --cc=xfs@oss.sgi.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox