On Thu, 5 Aug 2010 20:53:18 +0200 Jan Kara <jack@xxxxxxx> 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? -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
