| Message ID | 1348526106-17074-12-git-send-email-koverstreet@google.com (mailing list archive) |
|---|---|
| State | Deferred, archived |
| Headers | show |
On 09/25/2012 12:34 AM, Kent Overstreet wrote: > Random cleanup - this code was duplicated and it's not really specific > to md. > > Also added the ability to return the actual error code. > > Signed-off-by: Kent Overstreet <koverstreet@google.com> > CC: Jens Axboe <axboe@kernel.dk> > CC: NeilBrown <neilb@suse.de> > Acked-by: Tejun Heo <tj@kernel.org> > --- > drivers/md/raid1.c | 19 ------------------- > drivers/md/raid10.c | 19 ------------------- > fs/bio.c | 36 ++++++++++++++++++++++++++++++++++++ > include/linux/bio.h | 1 + > 4 files changed, 37 insertions(+), 38 deletions(-) > > diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c > index deff0cd..28f506a 100644 > --- a/drivers/md/raid1.c > +++ b/drivers/md/raid1.c > @@ -2048,25 +2048,6 @@ static void fix_read_error(struct r1conf *conf, int read_disk, > } > } > > -static void bi_complete(struct bio *bio, int error) > -{ > - complete((struct completion *)bio->bi_private); > -} > - > -static int submit_bio_wait(int rw, struct bio *bio) > -{ > - struct completion event; > - rw |= REQ_SYNC; > - > - init_completion(&event); > - bio->bi_private = &event; > - bio->bi_end_io = bi_complete; > - submit_bio(rw, bio); > - wait_for_completion(&event); > - > - return test_bit(BIO_UPTODATE, &bio->bi_flags); > -} > - > static int narrow_write_error(struct r1bio *r1_bio, int i) > { > struct mddev *mddev = r1_bio->mddev; > diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c > index 6d06d83..f001c1b 100644 > --- a/drivers/md/raid10.c > +++ b/drivers/md/raid10.c > @@ -2410,25 +2410,6 @@ static void fix_read_error(struct r10conf *conf, struct mddev *mddev, struct r10 > } > } > > -static void bi_complete(struct bio *bio, int error) > -{ > - complete((struct completion *)bio->bi_private); > -} > - > -static int submit_bio_wait(int rw, struct bio *bio) > -{ > - struct completion event; > - rw |= REQ_SYNC; > - > - init_completion(&event); > - bio->bi_private = &event; > - bio->bi_end_io = bi_complete; > - submit_bio(rw, bio); > - wait_for_completion(&event); > - > - return test_bit(BIO_UPTODATE, &bio->bi_flags); > -} > - > static int narrow_write_error(struct r10bio *r10_bio, int i) > { > struct bio *bio = r10_bio->master_bio; > diff --git a/fs/bio.c b/fs/bio.c > index 062ba8f..9a30dcf 100644 > --- a/fs/bio.c > +++ b/fs/bio.c > @@ -750,6 +750,42 @@ int bio_add_page(struct bio *bio, struct page *page, unsigned int len, > } > EXPORT_SYMBOL(bio_add_page); > > +struct submit_bio_ret { > + struct completion event; > + int error; > +}; > + > +static void submit_bio_wait_endio(struct bio *bio, int error) > +{ > + struct submit_bio_ret *ret = bio->bi_private; > + > + ret->error = error; > + complete(&ret->event); > +} > + > +/** > + * 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. > + submit_bio(rw, bio); > + wait_for_completion(&ret.event); > + > + return ret.error; > +} > +EXPORT_SYMBOL(submit_bio_wait); > + > /** > * bio_advance - increment/complete a bio by some number of bytes > * @bio: bio to advance > diff --git a/include/linux/bio.h b/include/linux/bio.h > index d985e90..3bf696b 100644 > --- a/include/linux/bio.h > +++ b/include/linux/bio.h > @@ -252,6 +252,7 @@ extern void bio_endio(struct bio *, int); > struct request_queue; > extern int bio_phys_segments(struct request_queue *, struct bio *); > > +extern int submit_bio_wait(int rw, struct bio *bio); > extern void bio_advance(struct bio *, unsigned); > > extern void bio_init(struct bio *); > Cheers, Hannes
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. -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
On Mon, Sep 24, 2012 at 03:34:51PM -0700, Kent Overstreet wrote: > Random cleanup - this code was duplicated and it's not really specific > to md. > > Also added the ability to return the actual error code. Who is going to make use of actual error code and why checking BIO_UPTODATE is not sufficient (as existing code is doing)? Thanks Vivek -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
On Tue, Oct 02, 2012 at 03:41:32PM -0400, Vivek Goyal wrote: > On Mon, Sep 24, 2012 at 03:34:51PM -0700, Kent Overstreet wrote: > > Random cleanup - this code was duplicated and it's not really specific > > to md. > > > > Also added the ability to return the actual error code. > > Who is going to make use of actual error code and why checking > BIO_UPTODATE is not sufficient (as existing code is doing)? Some things do, though it's not common and I forget where I saw it - checking for -ENOTSUPPORTED vs. other stuff -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
On Tue, Oct 02, 2012 at 01:11:05PM -0700, Kent Overstreet wrote: > On Tue, Oct 02, 2012 at 03:41:32PM -0400, Vivek Goyal wrote: > > On Mon, Sep 24, 2012 at 03:34:51PM -0700, Kent Overstreet wrote: > > > Random cleanup - this code was duplicated and it's not really specific > > > to md. > > > > > > Also added the ability to return the actual error code. > > > > Who is going to make use of actual error code and why checking > > BIO_UPTODATE is not sufficient (as existing code is doing)? > > Some things do, though it's not common and I forget where I saw it - > checking for -ENOTSUPPORTED vs. other stuff May be we can introduce "submit_bio_ret" stuff when we find the actual user in the series. Justifying code change becomes easier. Thanks Vivek -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
On Tue, Oct 02, 2012 at 04:16:30PM -0400, Vivek Goyal wrote: > On Tue, Oct 02, 2012 at 01:11:05PM -0700, Kent Overstreet wrote: > > On Tue, Oct 02, 2012 at 03:41:32PM -0400, Vivek Goyal wrote: > > > On Mon, Sep 24, 2012 at 03:34:51PM -0700, Kent Overstreet wrote: > > > > Random cleanup - this code was duplicated and it's not really specific > > > > to md. > > > > > > > > Also added the ability to return the actual error code. > > > > > > Who is going to make use of actual error code and why checking > > > BIO_UPTODATE is not sufficient (as existing code is doing)? > > > > Some things do, though it's not common and I forget where I saw it - > > checking for -ENOTSUPPORTED vs. other stuff > > May be we can introduce "submit_bio_ret" stuff when we find the actual > user in the series. Justifying code change becomes easier. Eh, IMO as generic code it's just better/more sensible that way; bio_endio() does pass an actual error code, so the sync version should pass it up too. Otherwise it's a needless inconsistency. Honestly I would prefer sticking an error field in struct bio. That'd be useful for other things, too. -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
On 10/02/2012 10:11 PM, Kent Overstreet wrote: > On Tue, Oct 02, 2012 at 03:41:32PM -0400, Vivek Goyal wrote: >> On Mon, Sep 24, 2012 at 03:34:51PM -0700, Kent Overstreet wrote: >>> Random cleanup - this code was duplicated and it's not really specific >>> to md. >>> >>> Also added the ability to return the actual error code. >> >> Who is going to make use of actual error code and why checking >> BIO_UPTODATE is not sufficient (as existing code is doing)? > > Some things do, though it's not common and I forget where I saw it - > checking for -ENOTSUPPORTED vs. other stuff > There are also additional error codes like -ENOLINK when the nexus is dropped. Quite useful in multipathed or clustered environs. Cheers, Hannes
diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c index deff0cd..28f506a 100644 --- a/drivers/md/raid1.c +++ b/drivers/md/raid1.c @@ -2048,25 +2048,6 @@ static void fix_read_error(struct r1conf *conf, int read_disk, } } -static void bi_complete(struct bio *bio, int error) -{ - complete((struct completion *)bio->bi_private); -} - -static int submit_bio_wait(int rw, struct bio *bio) -{ - struct completion event; - rw |= REQ_SYNC; - - init_completion(&event); - bio->bi_private = &event; - bio->bi_end_io = bi_complete; - submit_bio(rw, bio); - wait_for_completion(&event); - - return test_bit(BIO_UPTODATE, &bio->bi_flags); -} - static int narrow_write_error(struct r1bio *r1_bio, int i) { struct mddev *mddev = r1_bio->mddev; diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c index 6d06d83..f001c1b 100644 --- a/drivers/md/raid10.c +++ b/drivers/md/raid10.c @@ -2410,25 +2410,6 @@ static void fix_read_error(struct r10conf *conf, struct mddev *mddev, struct r10 } } -static void bi_complete(struct bio *bio, int error) -{ - complete((struct completion *)bio->bi_private); -} - -static int submit_bio_wait(int rw, struct bio *bio) -{ - struct completion event; - rw |= REQ_SYNC; - - init_completion(&event); - bio->bi_private = &event; - bio->bi_end_io = bi_complete; - submit_bio(rw, bio); - wait_for_completion(&event); - - return test_bit(BIO_UPTODATE, &bio->bi_flags); -} - static int narrow_write_error(struct r10bio *r10_bio, int i) { struct bio *bio = r10_bio->master_bio; diff --git a/fs/bio.c b/fs/bio.c index 062ba8f..9a30dcf 100644 --- a/fs/bio.c +++ b/fs/bio.c @@ -750,6 +750,42 @@ int bio_add_page(struct bio *bio, struct page *page, unsigned int len, } EXPORT_SYMBOL(bio_add_page); +struct submit_bio_ret { + struct completion event; + int error; +}; + +static void submit_bio_wait_endio(struct bio *bio, int error) +{ + struct submit_bio_ret *ret = bio->bi_private; + + ret->error = error; + complete(&ret->event); +} + +/** + * 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; + submit_bio(rw, bio); + wait_for_completion(&ret.event); + + return ret.error; +} +EXPORT_SYMBOL(submit_bio_wait); + /** * bio_advance - increment/complete a bio by some number of bytes * @bio: bio to advance diff --git a/include/linux/bio.h b/include/linux/bio.h index d985e90..3bf696b 100644 --- a/include/linux/bio.h +++ b/include/linux/bio.h @@ -252,6 +252,7 @@ extern void bio_endio(struct bio *, int); struct request_queue; extern int bio_phys_segments(struct request_queue *, struct bio *); +extern int submit_bio_wait(int rw, struct bio *bio); extern void bio_advance(struct bio *, unsigned); extern void bio_init(struct bio *);