Re: [PATCH 3/3] xfs: optimize bio handling in the buffer writeback path

[Christoph Hellwig <hch@lst.de>]

On Thu, Mar 03, 2016 at 10:17:30AM -0500, Brian Foster wrote:
> > +
> > +		/*
> > +		 * For the last bio, bi_private points to the ioend, so we
> > +		 * need to explicitly end the iteration here.
> > +		 */
> 
> Do you mean the last bio is pointed to by the ioend?

No, bio->bi_private of the last bio points to the ioend.

> > @@ -541,10 +457,8 @@ xfs_submit_ioend(
> >  	 * at this point in time.
> >  	 */
> >  error_finish:
> > -	if (ioend->io_bio)
> > -		bio_put(ioend->io_bio);
> > -	ioend->io_error = status;
> > -	xfs_finish_ioend(ioend);
> > +	ioend->io_bio->bi_error = status;
> > +	bio_endio(ioend->io_bio);
> >  	return status;
> 
> bi_end_io is not set here, so what happens to the buffers added to the
> ioend in this case?

We're not calling the end_io function we should be, good catch and fixed.

> Trailing whitespace on the above line.

Ok, fixed.

> And FWIW, I'm not a huge fan of
> open coding both the bio and ioend allocations. It makes it easier to
> distinguish the higher level algorithm from all of the structure
> initialization noise. It looks to me that alloc_ioend() could remain
> mostly as is, using the new bioset allocation, and alloc_ioend_bio()
> could be inlined and renamed to something like init_bio_from_bh() or
> some such.

Hmm, not a huge fan of these single use function in general, but
I'll see if I can do something sensible.

> I'm trying to make sure I grok how this works without knowing much about
> the block layer. So we chain the current bio to the new one, the latter
> becoming the parent, and submit the old one. It looks to me that this
> could result in bio_endio() on the parent, which seems fishy... what am
> I missing? IOW, is it safe to submit bios like this before the entire
> chain is created?

Ignoring this for now and jumping to the next reply..

> > + out_free_ioend_bioset:
> > + 	bioset_free(xfs_ioend_bioset);
> 
> Space before tab ^.

Fixed.

> > + 	bioset_free(xfs_ioend_bioset);
> 
> Space before tab ^.

Fixed.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs