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