On Wed, Jun 03 2015 at 9:42P -0400,
Christoph Hellwig <hch@xxxxxx> wrote:
> Currently we have two different ways to signal an I/O error on a BIO:
>
> (1) by clearing the BIO_UPTODATE flag
> (2) by returning a Linux errno value to the bi_end_io callback
>
> The first one has the drawback of only communicating a single possible
> error (-EIO), and the second one has the drawback of not beeing persistent
> when bios are queued up, and are not passed along from child to parent
> bio in the ever more popular chaining scenario. Having both mechanisms
> available has the additional drawback of utterly confusing driver authors
> and introducing bugs where various I/O submitters only deal with one of
> them, and the others have to add boilerplate code to deal with both kinds
> of error returns.
>
> So add a new bi_error field to store an errno value directly in struct
> bio and remove the existing mechanisms to clean all this up.
>
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>
This patch _really_ concerns me because just in DM alone I found you
took liberties that you shouldn't have and created a regression. First
issue is a real bug (your proposed dm-io.c:dmio_complete change missed
that dm-io uses error_bits and not traditional error code like expected)
the other issue being you added extra branching that isn't needed and
made review more tedious (dm.c:clone_endio).
I'll defer to others to check their respective areas of expertise but my
experience with this review should really underscore the need for more
eyes on this patch. That said, I do appreciate your effort on cleaning
this up!
For DM, please add Signed-off-by: Mike Snitzer <snitzer@xxxxxxxxxx> once
you've folded in this patch, thanks!
diff --git a/drivers/md/dm-bufio.c b/drivers/md/dm-bufio.c
index 86dbbc7..d2fc49f 100644
--- a/drivers/md/dm-bufio.c
+++ b/drivers/md/dm-bufio.c
@@ -545,7 +545,8 @@ static void dmio_complete(unsigned long error, void *context)
{
struct dm_buffer *b = context;
- b->bio.bi_end_io(&b->bio, error ? -EIO : 0);
+ b->bio.bi_error = error ? -EIO : 0;
+ b->bio.bi_end_io(&b->bio);
}
static void use_dmio(struct dm_buffer *b, int rw, sector_t block,
diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index 767bce9..85a7c3d 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -954,25 +954,22 @@ static void disable_write_same(struct mapped_device *md)
limits->max_write_same_sectors = 0;
}
-static void clone_endio(struct bio *bio, int error)
+static void clone_endio(struct bio *bio)
{
- int r = error;
+ int r = bio->bi_error;
struct dm_target_io *tio = container_of(bio, struct dm_target_io, clone);
struct dm_io *io = tio->io;
struct mapped_device *md = tio->io->md;
dm_endio_fn endio = tio->ti->type->end_io;
- if (!bio_flagged(bio, BIO_UPTODATE) && !error)
- error = -EIO;
-
if (endio) {
- r = endio(tio->ti, bio, error);
+ r = endio(tio->ti, bio, bio->bi_error);
if (r < 0 || r == DM_ENDIO_REQUEUE)
/*
* error and requeue request are handled
* in dec_pending().
*/
- error = r;
+ ;
else if (r == DM_ENDIO_INCOMPLETE)
/* The target will handle the io */
return;
@@ -987,7 +984,7 @@ static void clone_endio(struct bio *bio, int error)
disable_write_same(md);
free_tio(md, tio);
- dec_pending(io, error);
+ dec_pending(io, r);
}
static struct dm_rq_target_io *tio_from_request(struct request *rq)
--
dm-devel mailing list
dm-devel@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/dm-devel
