linux-kernel - Re: [PATCH 3/8] writeback: switch to per-bdi threads for flushing data v2

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: <20090907184517.GF18599@kernel.dk>
Date:	Mon, 7 Sep 2009 20:45:18 +0200
From:	Jens Axboe <jens.axboe@...cle.com>
To:	Jan Kara <jack@...e.cz>
Cc:	linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org,
	chris.mason@...cle.com, david@...morbit.com, hch@...radead.org,
	tytso@....edu, akpm@...ux-foundation.org
Subject: Re: [PATCH 3/8] writeback: switch to per-bdi threads for flushing
	data v2

On Mon, Sep 07 2009, Jan Kara wrote:
>   Hi Jens,
> 
>   now I've found just two minor things (see below). Besides them the only
> thing which is blocking my ack is a way to effectively lookup a BDI from a
> superblock so that we can reasonably effectively fsync a superblock...

Can we side step that as an inclusion criteria, please? I'd really like
to work on adding that quick lookup, but I'd also hate to potentiall
destabilize anything at this point. So we can probably quite easily make
.32 as well for that, but I'd rather not risk doing a version and
include that from the beginning. OK?

> > +	/*
> > +	 * Check for periodic writeback, kupdated() style
> > +	 */
> > +	if (!wrote)
> > +		wrote = wb_check_old_data_flush(wb);
>   Why is here the !wrote check? It would feel safer if we just did
> wrote += wb_check_old_data_flush(wb);
>   Otherwise we cannot guarantee syncing of inodes every writeback_interval.

Yes good point, that check should not be there or the logic is still
broken wrt old data flushing. Will fix that, thanks!

> > +		/*
> > +		 * If work allocation fails, do the writes inline. We drop
> > +		 * the lock and restart the list writeout. This should be OK,
> > +		 * since this happens rarely and because the writeout should
> > +		 * eventually make more free memory available.
> > +		 */
> > +		work = bdi_alloc_work(wbc);
> > +		if (!work) {
> > +			struct writeback_control __wbc = *wbc;
> >
> > -			cond_resched();
> > +			/*
> > +			 * Not a data integrity writeout, just continue
> > +			 */
> > +			if (!must_wait)
> > +				continue;
> >  
> > -			spin_lock(&inode_lock);
> > +			spin_unlock(&bdi_lock);
> > +			__wbc = *wbc;
>   You initialize the variable twice...

Indeed, the latter should just go. Good spotting, fixed.

-- 
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/