Re: [PATCH 2/3 v2] mm: Fix writeback_in_progress()

[Andrew Morton <akpm@linux-foundation.org>]

On Thu,  5 Aug 2010 20:53:18 +0200
Jan Kara <jack@suse.cz> wrote:

> Commit 83ba7b071f30f7c01f72518ad72d5cd203c27502 broke writeback_in_progress()
> as in that commit we started to remove work items from the list at the
> moment we start working on them and not at the moment they are finished.
> Thus if the flusher thread was doing some work but there was no other
> work queued, writeback_in_progress() returned false. This could in
> particular cause unnecessary queueing of background writeback from
> balance_dirty_pages() or writeout work from writeback_sb_if_idle().
> 
> This patch fixes the problem by introducing a bit in the bdi state which
> indicates that the flusher thread is processing some work and uses this
> bit for writeback_in_progress() test.
> 
> NOTE: Both callsites of writeback_in_progress() (namely,
> writeback_inodes_sb_if_idle() and balance_dirty_pages()) would actually
> need a different information than what writeback_in_progress() provides.
> They would need to know whether *the kind of writeback they are going
> to submit* is already queued. But this information isn't that simple
> to provide so let's fix writeback_in_progress() for the time being.
> 

Patch looks reasonable, but..  What effect does it have?

writeback_inodes_sb_if_idle() is some ext4 delalloc hack.  Shudder, no
comment.

The writeback_in_progess() test in balance_dirty_pages() is at least
eight years old.  I got bored digging back through the git record
working out why I added it (that line's been changed multiple times and
it's a pita tracking back through those things).  

I suspect it was there to say "don't bother poking pdflush if it's
already doing something".  But perhaps that logic got broken by
subsequent mauling.  Or was never right.  For example, if the kupdate
function is writing back an old inode, do we really want that to
prevent balance_dirty_pages()'s attempt to cure a dirty-memory-exceeded
situation?  Don't think so.

So I dunno.  I suspect a better patch would be "remove
writeback_in_progess()".  But first one should find the
patch which added the test to balance_dirty_pages() and
see if it was well changelogged.

But this patch's changelog doesn't make me confident that the
end-user-visible effects of this change are fully understood?