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

[Jan Kara <jack@suse.cz>]

On Thu 05-08-10 17:10:25, Andrew Morton wrote:
> 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
  Yeah, it seems it never was quite right. I dug through BK history and
the logic seems to be introduced by commit
faa74c6f880b4a28d916a4c3b15594bbb93b57c0 sometime aroung *2.5.16* and at
that time it seems it already had this issue.

> 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.
  I think it would be good if balance_dirty_pages() made sure that at least
one backround writeback work is either being processed or in a queue. We
cannot just unconditionally queue a background writeback because that would
make queue really long full of requests for background writeback when the
device gets congested.

> 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.
  I definitely agree that we want to get rid of writeback_in_progress() as
soon as someone writes a patch to do it ;).

> But this patch's changelog doesn't make me confident that the
> end-user-visible effects of this change are fully understood?
  I just noticed that the function does something else than what its comment
suggests and that the change was unintentionally caused by commit
83ba7b071f30f7c01f72518ad72d5cd203c27502 short time ago. So I just figured
it is worth to restore the behavior to the one before the cleanup... About
user visible effects: It's hard for me to say if there are any. We may do
more writeback with buggy writeback_in_progress() which is sometimes
desirable (e.g. when flusher thread is just doing writeback of old inodes
and we want full background cleaning) and sometimes it needen't be. But
most of the time I'd say you just won't notice (at least in my testing I
was able to notice the difference only because of writeback code tracing).

									Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR