Linux Archive

Linux Archive (http://www.linux-archive.org/)
-   Device-mapper Development (http://www.linux-archive.org/device-mapper-development/)
-   -   block: Add submit_bio_wait(), remove from md (http://www.linux-archive.org/device-mapper-development/707148-block-add-submit_bio_wait-remove-md.html)

Hannes Reinecke 09-25-2012 05:51 AM

block: Add submit_bio_wait(), remove from md
 
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
--
Dr. Hannes Reinecke zSeries & Storage
hare@suse.de +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)


--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Kent Overstreet 09-25-2012 10:15 PM

block: Add submit_bio_wait(), remove from md
 
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

Vivek Goyal 10-02-2012 07:41 PM

block: Add submit_bio_wait(), remove from md
 
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

Kent Overstreet 10-02-2012 08:11 PM

block: Add submit_bio_wait(), remove from md
 
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

Vivek Goyal 10-02-2012 08:16 PM

block: Add submit_bio_wait(), remove from md
 
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

Kent Overstreet 10-02-2012 08:22 PM

block: Add submit_bio_wait(), remove from md
 
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

Hannes Reinecke 10-04-2012 06:07 AM

block: Add submit_bio_wait(), remove from md
 
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
--
Dr. Hannes Reinecke zSeries & Storage
hare@suse.de +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel


All times are GMT. The time now is 10:56 PM.

VBulletin, Copyright ©2000 - 2014, Jelsoft Enterprises Ltd.
Content Relevant URLs by vBSEO ©2007, Crawlability, Inc.