Re: [dm-devel] [PATCH v3 11/26] block: Add submit_bio_wait(), remove from md

[Kent Overstreet <koverstreet@google.com>]

On Tue, Sep 25, 2012 at 07:51:07AM +0200, Hannes Reinecke wrote:
> On 09/25/2012 12:34 AM, Kent Overstreet wrote:
> > +/**
> > + * submit_bio_wait - submit a bio, and wait until it completes
> > + * @rw: whether to %READ or %WRITE, or maybe to %READA (read ahead)
> > + * @bio: The &struct bio which describes the I/O
> > + *
> > + * Simple wrapper around submit_bio(). Returns 0 on success, or the error from
> > + * bio_endio() on failure.
> > + */
> > +int submit_bio_wait(int rw, struct bio *bio)
> > +{
> > +	struct submit_bio_ret ret;
> > +
> > +	rw |= REQ_SYNC;
> > +	init_completion(&ret.event);
> > +	bio->bi_private = &ret;
> > +	bio->bi_end_io = submit_bio_wait_endio;
> 
> Hmm. As this is meant to be a generic function, blindly overwriting
> the bi_end_io pointer doesn't look like a good idea; the caller
> could have set something there.
> 
> Please add at least a WARN_ON(bio->bi_end_io) prior to modifying it.

Nah, the general rule with bios is after it's completed anything
could've been modified; we don't document or enforce otherwise with
bi_end_io (and there's a fair amount of code that saves/sets bi_end_io,
and I don't think it all restores the original before calling it).
I'm not going to special case this unless we start documenting/enforcing
it in general.

Besides that, setting a callback on something that's being used
synchronously is just dumb. Personally, I make damn sure to read and
understand code I'm using. I mean, maybe if this restriction was in the
slightest way subtle, but... how else would submit_bio_wait() be
implemented? It's kind of obvious if you think for two seconds about it.