Re: [PATCH 4/5] writeback: avoid livelocking WB_SYNC_ALL writeback

[Wu Fengguang <fengguang.wu@intel.com>]

On Wed, Nov 10, 2010 at 07:18:40AM +0800, Jan Kara wrote:
> On Tue 09-11-10 14:43:46, Andrew Morton wrote:

> > I don't really see how this patch changes anything.  For WB_SYNC_ALL
> > requests the code will still try to write out 2^63 pages, only it does
> > it all in a single writeback_inodes_wb() call.  What prevents that call

Sorry sync() works on one super block after another, so it's some
__writeback_inodes_sb() call. I'll update the comment.

> > itself from getting livelocked?

__writeback_inodes_sb() livelock is prevented by

- working on a finite set of files by doing queue_io() once at the beginning
- working on a finite set of pages by PAGECACHE_TAG_TOWRITE page tagging

>   I'm referring to the livelock avoidance using page tagging. Fengguang
> actually added a note about this into a comment in the code but it's not
> in the changelog. And you're right it should be here.

OK, I'll add the above to changelog.

> > Perhaps the unmentioned problem here is that each call to
> > writeback_inodes_wb(MAX_WRITEBACK_PAGES) will restart its walk across
> > the inode lists.  So instead of giving up on a being-written-to-file,
> > we continuously revisit it again and again and again.
> > 
> > Correct?  If so, please add the description.  If incorrect, please add
> > the description as well ;)
>   Yes, that's the problem.

writeback_inodes_wb(MAX_WRITEBACK_PAGES) will put the not full written
inode to head of b_more_io, and pick up the next inode from tail of
b_io next time it is called. Here the tail of b_io serves as the
cursor.

         b_io             b_more_io
        |----------------|-----------------|
        ^head            ^cursor           ^tail

> > Root cause time: it's those damn per-sb inode lists *again*.  They're
> > just awful.  We need some data structure there which is more amenable
> > to being iterated over.  Something against which we can store cursors,
> > for a start.
>   This would be definitely nice. But in this particular case, since we have
> that page tagging livelock avoidance, we can just do all we need in a one
> big sweep so we are OK.

The main problem of list_head is the awkward superblock walks in
move_expired_inodes(). It may take inode_lock for too long time.

It helps to break up b_dirty into a rb-tree. That will make
redirty_tail() more straightforward, too.

> Suggestion for the new changelog:
> When wb_writeback() is called in WB_SYNC_ALL mode, work->nr_to_write is
> usually set to LONG_MAX. The logic in wb_writeback() then calls
> __writeback_inodes_sb() with nr_to_write == MAX_WRITEBACK_PAGES and

> we easily end up with negative nr_to_write after the function returns.
> This is because write_cache_pages() does not stop writing when
> nr_to_write drops to zero in WB_SYNC_ALL mode.

It will return with (nr_to_write <=0) regardless of the
write_cache_pages() trick to ignore nr_to_write. So I changed the
above to:

        we easily end up with non-positive nr_to_write after the function
        returns, if the inode has more than MAX_WRITEBACK_PAGES dirty pages
        at the moment.

Others look good. I'll repost the series with updated changelog.

Thanks,
Fengguang