linux-fsdevel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Christoph Hellwig <hch@lst.de>
To: axboe@kernel.dk
Cc: linux-fsdevel@vger.kernel.org
Subject: [PATCH 0/6] writeback fixes - slow unmount and others
Date: Tue, 8 Jun 2010 18:14:24 +0200	[thread overview]
Message-ID: <20100608161424.GA11735@lst.de> (raw)

I've started looking into the slow unmount issue again, and digged a
bit deeper into the writeback code to look at the root cause.

All inode writeback is performed by writeback_inodes_wb, which
has the following callgraph (annotated by a few important facts):

writeback_inodes_wb
  <- writeback_inodes_wbc
       <- bdi_flush_io			(WB_SYNC_NONE, no sb)
       <- balance_dirty_pages		(WB_SYNC_NONE, no sb)
  <- wb_writeback
       <- wb_check_old_data_flush	(WB_SYNC_NONE, no sb)
       <- wb_do_writeback
           <- bdi_writeback_task	\
	   <- bdi_start_fn		| take bdi_work item from list
	   <- bdi_forker_task		/

So we either get a bdi_work item submitted by bdi_queue_work, or
we do WB_SYNC_NONE I/O ourselves without specifying a superblock.

For bdi_queue_work the same annotated callchain looks like:

bdi_queue_work
  <- bdi_sync_writeback			(WB_SYNC_ALL, mandatory sb, onstack)
       <- sync_inodes_sb		(..)
  <- bdi_alloc_queue_work
       <- bdi_start_writeback		(WB_SYNC_NONE, optional sb)
            <- balance_dirty_pages	(..          , no sb)
	    <- laptop_mode_timer_fn	(..          , no sb)
	    <- writeback_inodes_sb	(..          , mandatory sb)
       <- bdi_writeback_all		(WB_SYNC_NONE, optional sb)
            <- wakeup_flusher_threads	(..          , no sb)

So in general we can say that the case of WB_SYNC_NONE without a
superblock is the normal case.  And now if we look at the callers of
the functions that take a superblock we see that this is basically
sync_filesystem, which always has s_umount held.  So instead of
trying to add hacks for when it is held, we can assume that we
always have it held if a superblock is specified, and fix up the
two callers inside filesystems that crept in recently to also
hold it.  With that we can replace various ad-hoc checks with
a very clear scheme that only checks the oncase caller, and if
we have a superblock or not after switching all writeback
that has a superblock to submit the I/O on stack.

After my series the annotated callgraph for bdi_queue_work is the
following:


bdi_queue_work
  <- bdi_queue_work_onstack		 (WB_SYNC_ALL/NONE, mandatory sb, onstack)
       <- sync_inodes_sb		 (WB_SYNC_ALL, ..)
       <- writeback_inodes_sb		 (WB_SYNC_NONE, ..)
  <- bdi_alloc_queue_work
       <- bdi_start_writeback		 (WB_SYNC_NONE, no sb)
       <- bdi_start_background_writeback (WB_SYNC_NONE, no sb)
       <- wakeup_flusher_threads	 (WB_SYNC_NONE, no sb)

So it's a lot simpler and just as important flatter.  But when we look
how we use the onstack / sb specified callers I'm not sure this is the
optimal case yet.  We're beeing called from synchronous callers (
we need to wait for completion for the first WB_SYNC_NONE pass later on,
too), so there really is no need to hand this work off to the flusher
threads.  In the end it might be much simpler to just issue this I/O
from the caller context and remove the code for the on-stack
submissions, but that's left for a later patch.

             reply	other threads:[~2010-06-08 16:14 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2010-06-08 16:14 Christoph Hellwig [this message]
2010-06-08 16:14 ` [PATCH 1/6] writeback: fix writeback completion notifications Christoph Hellwig
2010-06-08 19:50   ` Jens Axboe
2010-06-15 17:25   ` Jan Kara
2010-06-15 17:30     ` Christoph Hellwig
2010-06-08 16:14 ` [PATCH 2/6] writeback: queue work on stack in writeback_inodes_sb Christoph Hellwig
2010-06-08 19:51   ` Jens Axboe
2010-06-08 16:14 ` [PATCH 3/6] writeback: enforce s_umount locking " Christoph Hellwig
2010-06-15 17:54   ` Jan Kara
2010-06-15 17:59     ` Christoph Hellwig
2010-06-15 18:04       ` Jan Kara
2010-06-08 16:14 ` [PATCH 4/6] writeback: fix writeback_inodes_wb from writeback_inodes_sb Christoph Hellwig
2010-06-08 19:51   ` Jens Axboe
2010-06-09 12:25     ` Christoph Hellwig
2010-06-09 12:29       ` Jens Axboe
2010-06-08 16:15 ` [PATCH 5/6] writeback: simplify wakeup_flusher_threads Christoph Hellwig
2010-06-08 19:51   ` Jens Axboe
2010-06-08 16:15 ` [PATCH 6/6] writeback: simplify and split bdi_start_writeback Christoph Hellwig
2010-06-08 19:52   ` Jens Axboe

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=20100608161424.GA11735@lst.de \
    --to=hch@lst.de \
    --cc=axboe@kernel.dk \
    --cc=linux-fsdevel@vger.kernel.org \
    /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;
as well as URLs for NNTP newsgroup(s).