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

[Jens Axboe <jens.axboe@oracle.com>]

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