On Thu 05-08-10 17:10:25, Andrew Morton wrote: > 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 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@xxxxxxx> SUSE Labs, CR -- 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
