linux-kernel - Re: [PATCH 6/7] writeback: separate starting of sync vs opportunistic writeback

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20090915114426.GJ23126@kernel.dk>
Date:	Tue, 15 Sep 2009 13:44:26 +0200
From:	Jens Axboe <jens.axboe@...cle.com>
To:	Jan Kara <jack@...e.cz>
Cc:	Christoph Hellwig <hch@...radead.org>,
	linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org,
	chris.mason@...cle.com, tytso@....edu, akpm@...ux-foundation.org,
	trond.myklebust@....uio.no
Subject: Re: [PATCH 6/7] writeback: separate starting of sync vs
	opportunistic writeback

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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/