Re: [PATCH 6/7] writeback: separate starting of sync vsopportunistic writeback

[Jens Axboe]

On Tue, Sep 15 2009, Jens Axboe wrote:
> On Tue, Sep 15 2009, Jan Kara wrote:
> > On Mon 14-09-09 21:42:43, Jens Axboe wrote:
> > > On Mon, Sep 14 2009, Jens Axboe wrote:
> > > > On Mon, Sep 14 2009, Christoph Hellwig wrote:
> > > > > On Mon, Sep 14, 2009 at 03:33:07PM +0200, Jan Kara wrote:
> > > > > > On Mon 14-09-09 11:36:33, Jens Axboe wrote:
> > > > > > > bdi_start_writeback() is currently split into two paths, one for
> > > > > > > WB_SYNC_NONE and one for WB_SYNC_ALL. Add bdi_sync_writeback()
> > > > > > > for WB_SYNC_ALL writeback and let bdi_start_writeback() handle
> > > > > > > only WB_SYNC_NONE.
> > > > > >   What I don't like about this patch is that if somebody sets up
> > > > > > writeback_control with WB_SYNC_ALL mode set and then submits it to disk via
> > > > > > bdi_start_writeback() it will just silently convert his writeback to an
> > > > > > asynchronous one.
> > > > > >   So I'd maybe leave setting of sync_mode to the caller and just WARN_ON if
> > > > > > it does not match the purpose of the function...
> > > > > 
> > > > > Or initialize the wb entirely inside these functions.  For the sync case
> > > > > we really only need a superblock as argument, and for writeback it's
> > > > > bdi + nr_pages.  And also make sure they consistenly return void as
> > > > > no one cares about the return value.
> > > > 
> > > > Yes, I thought about doing that and like that better than the warning.
> > > > Just pass in the needed args and allocate+fill the wbc on stack. I'll
> > > > make that change.
> > > 
> > > That works out much better, imho:
> > > 
> > > http://git.kernel.dk/?p=linux-2.6-block.git;a=commit;h=270c12655d7d11e234d335a8ab0540c02c034b66
> >   Yeah, the code looks better. BTW, how about converting also
> > bdi_writeback_all() to get superblock and nr_pages as an argument?
> > Currently it seems to be the only place "above" flusher thread which uses
> > wbc and it's just constructed in the callers of bdi_writeback_all() and
> > then disassembled inside the function...
> 
> Yes good point, I'll include that too. Thanks!

One small problem there, though... Currently all queued writeback is
range cyclic, however with this change then we drop that bit from
sync_inodes_sb() which isn't range_cyclic and instead just specifies the
whole range.

-- 
Jens Axboe

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/