[RFC] add a bi_error field

[RFC] add a bi_error field

[Christoph Hellwig]

Bio error reporting has been a mess for a while, and the increasing
use of chained bios makes it worse.

This series attempts to add a proper error field to struct bio
to sort this out.  It's working fine for me, but MD and btrfs were
doing pretty nasty things with the BIO_UPTODATE flag, so I would
appreciate some detailed review there.

Re: [dm-devel] [PATCH] block: add a bi_error field to struct bio

[Martin K. Petersen]

>>>>> "Christoph" == Christoph Hellwig <hch@lst.de> writes:

Christoph> The first one has the drawback of only communicating a single
Christoph> possible error (-EIO), and the second one has the drawback of
Christoph> not beeing persistent when bios are queued up, and are not
Christoph> passed along from child to parent bio in the ever more
Christoph> popular chaining scenario.

Christoph> So add a new bi_error field to store an errno value directly
Christoph> in struct bio and remove the existing mechanisms to clean all
Christoph> this up.

Having the error status separate from the bio has been a major headache.
I am entirely in favor of this patch.

It was a big chunk of changes to read through but I did not spot any
obvious problems or polarity reversals. It would be nice to get the
respective fs/md/target driver folks to check their portions, though.

Reviewed-by: Martin K. Petersen <martin.petersen@oracle.com>

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: block: add a bi_error field to struct bio

[Mike Snitzer]

On Wed, Jun 03 2015 at  9:42P -0400,
Christoph Hellwig <hch@lst.de> 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@lst.de>

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@redhat.com> 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)

Re: block: add a bi_error field to struct bio

[Christoph Hellwig]

On Thu, Jun 04, 2015 at 11:31:07AM -0400, Mike Snitzer wrote:
> 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)

Point taken.  I already wanted to complain about the mess due to the bio
error abuse with it's own values in DM in the first posting, guess I
need to add that to the second one.  I don't think overloading common
interfaces with your private error codes is a good idea, but let's
leave that for a separate discussion.

> the other issue being you added extra branching that isn't needed and
> made review more tedious (dm.c:clone_endio).

I think the code is better than what it was before, but it's still
a bit of a mess.  What do you think of the patch below which I'd
like to add before the big bi_error patch as a preparatory one?

> For DM, please add Signed-off-by: Mike Snitzer <snitzer@redhat.com> once
> you've folded in this patch, thanks!

FYI, that wasn't a foldable patch but updated hunks of the old one.  Not
really a problem, but a little confusing.


>From f095cbeba5135afa6cf102718319f0d0c1e7b422 Mon Sep 17 00:00:00 2001
From: Christoph Hellwig <hch@lst.de>
Date: Wed, 10 Jun 2015 10:04:45 +0200
Subject: dm: use a single error code variable in clone_endio
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

clone_endio currently uses two variables for tracking error state, with
values getting bounceѕ forth and back between the two, which makes the
code hard to read.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/md/dm.c | 22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)

diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index 2161ed9..8467976 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -956,7 +956,6 @@ static void disable_write_same(struct mapped_device *md)
 
 static void clone_endio(struct bio *bio, int error)
 {
-	int r = 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;
@@ -966,23 +965,22 @@ static void clone_endio(struct bio *bio, int error)
 		error = -EIO;
 
 	if (endio) {
-		r = endio(tio->ti, bio, 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)
+		error = endio(tio->ti, bio, error);
+		if (error == DM_ENDIO_INCOMPLETE) {
 			/* The target will handle the io */
 			return;
-		else if (r) {
-			DMWARN("unimplemented target endio return value: %d", r);
+		}
+
+		if (error > 0 && error != DM_ENDIO_REQUEUE) {
+			DMWARN("unimplemented target endio return value: %d",
+				error);
 			BUG();
 		}
+
+		/* Error and requeue request are handled in dec_pending(). */
 	}
 
-	if (unlikely(r == -EREMOTEIO && (bio->bi_rw & REQ_WRITE_SAME) &&
+	if (unlikely(error == -EREMOTEIO && (bio->bi_rw & REQ_WRITE_SAME) &&
 		     !bdev_get_queue(bio->bi_bdev)->limits.max_write_same_sectors))
 		disable_write_same(md);
 
-- 
1.9.1

Re: block: add a bi_error field to struct bio

[Mike Snitzer]

On Wed, Jun 10 2015 at  4:11am -0400,
Christoph Hellwig <hch@lst.de> wrote:

> On Thu, Jun 04, 2015 at 11:31:07AM -0400, Mike Snitzer wrote:
> > 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)
> 
> Point taken.  I already wanted to complain about the mess due to the bio
> error abuse with it's own values in DM in the first posting, guess I
> need to add that to the second one.  I don't think overloading common
> interfaces with your private error codes is a good idea, but let's
> leave that for a separate discussion.

I'll queue a patch to rename 'error' to 'error_bits' where appropriate.

> > the other issue being you added extra branching that isn't needed and
> > made review more tedious (dm.c:clone_endio).
> 
> I think the code is better than what it was before, but it's still
> a bit of a mess.  What do you think of the patch below which I'd
> like to add before the big bi_error patch as a preparatory one?

If you're referring to the mix of error variables I totally agree.  Just
don't think we need the extra branching.
 
> > For DM, please add Signed-off-by: Mike Snitzer <snitzer@redhat.com> once
> > you've folded in this patch, thanks!
> 
> FYI, that wasn't a foldable patch but updated hunks of the old one.  Not
> really a problem, but a little confusing.

Yeap, should have been clearer they were meant to replace your hunks.

> >From f095cbeba5135afa6cf102718319f0d0c1e7b422 Mon Sep 17 00:00:00 2001
> From: Christoph Hellwig <hch@lst.de>
> Date: Wed, 10 Jun 2015 10:04:45 +0200
> Subject: dm: use a single error code variable in clone_endio
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
> 
> clone_endio currently uses two variables for tracking error state, with
> values getting bounceѕ forth and back between the two, which makes the
> code hard to read.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  drivers/md/dm.c | 22 ++++++++++------------
>  1 file changed, 10 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/md/dm.c b/drivers/md/dm.c
> index 2161ed9..8467976 100644
> --- a/drivers/md/dm.c
> +++ b/drivers/md/dm.c
> @@ -956,7 +956,6 @@ static void disable_write_same(struct mapped_device *md)
>  
>  static void clone_endio(struct bio *bio, int error)
>  {
> -	int r = 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;
> @@ -966,23 +965,22 @@ static void clone_endio(struct bio *bio, int error)
>  		error = -EIO;
>  
>  	if (endio) {
> -		r = endio(tio->ti, bio, 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)
> +		error = endio(tio->ti, bio, error);
> +		if (error == DM_ENDIO_INCOMPLETE) {
>  			/* The target will handle the io */
>  			return;
> -		else if (r) {
> -			DMWARN("unimplemented target endio return value: %d", r);
> +		}
> +
> +		if (error > 0 && error != DM_ENDIO_REQUEUE) {
> +			DMWARN("unimplemented target endio return value: %d",
> +				error);
>  			BUG();
>  		}
> +
> +		/* Error and requeue request are handled in dec_pending(). */
>  	}
>  
> -	if (unlikely(r == -EREMOTEIO && (bio->bi_rw & REQ_WRITE_SAME) &&
> +	if (unlikely(error == -EREMOTEIO && (bio->bi_rw & REQ_WRITE_SAME) &&
>  		     !bdev_get_queue(bio->bi_bdev)->limits.max_write_same_sectors))
>  		disable_write_same(md);
>  
> -- 
> 1.9.1

Unfortunately by dropping the original error (e.g. -EREMOTEIO) on the
floor (in the 'if (endio) {' branch) you're breaking the REQ_WRITE_SAME
check.

Your new bi_error patch gets away with the redundant error code cleanup
because we can directly check the bio's bi_error for -EREMOTEIO.  So
feel free to fold the simplified 'if (error > 0 && error != DM_ENDIO_REQUEUE) {'
in to your new patch -- but not seeing the point of making this prep
patch in advance.

Re: block: add a bi_error field to struct bio

[Mike Snitzer]

On Wed, Jun 10 2015 at 11:26am -0400,
Mike Snitzer <snitzer@redhat.com> wrote:

> On Wed, Jun 10 2015 at  4:11am -0400,
> Christoph Hellwig <hch@lst.de> wrote:
> 
> > On Thu, Jun 04, 2015 at 11:31:07AM -0400, Mike Snitzer wrote:
> > > 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)
> > 
> > Point taken.  I already wanted to complain about the mess due to the bio
> > error abuse with it's own values in DM in the first posting, guess I
> > need to add that to the second one.  I don't think overloading common
> > interfaces with your private error codes is a good idea, but let's
> > leave that for a separate discussion.
> 
> I'll queue a patch to rename 'error' to 'error_bits' where appropriate.

See: https://git.kernel.org/cgit/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=for-next&id=f368f463e4cef696ad6b102dbaf5c10dfca7cc63

Re: block: add a bi_error field to struct bio

[Christoph Hellwig]

On Wed, Jun 10, 2015 at 12:01:12PM -0400, Mike Snitzer wrote:
> > I'll queue a patch to rename 'error' to 'error_bits' where appropriate.
> 
> See: https://git.kernel.org/cgit/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=for-next&id=f368f463e4cef696ad6b102dbaf5c10dfca7cc63

Can we wait with this until we're done with bi_error?

Re: block: add a bi_error field to struct bio

[Mike Snitzer]

On Wed, Jun 10 2015 at 12:04pm -0400,
Christoph Hellwig <hch@lst.de> wrote:

> On Wed, Jun 10, 2015 at 12:01:12PM -0400, Mike Snitzer wrote:
> > > I'll queue a patch to rename 'error' to 'error_bits' where appropriate.
> > 
> > See: https://git.kernel.org/cgit/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=for-next&id=f368f463e4cef696ad6b102dbaf5c10dfca7cc63
> 
> Can we wait with this until we're done with bi_error?

Sure, I've moved it out to dm-4.3

Re: block: add a bi_error field to struct bio

[anup modak]

So Is it safe to use raid 6 with btrfs for home storage now? I will
have backup on ZFS.

On Wed, Jun 10, 2015 at 11:50 AM, Mike Snitzer <snitzer@redhat.com> wrote:
> On Wed, Jun 10 2015 at 12:04pm -0400,
> Christoph Hellwig <hch@lst.de> wrote:
>
>> On Wed, Jun 10, 2015 at 12:01:12PM -0400, Mike Snitzer wrote:
>> > > I'll queue a patch to rename 'error' to 'error_bits' where appropriate.
>> >
>> > See: https://git.kernel.org/cgit/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=for-next&id=f368f463e4cef696ad6b102dbaf5c10dfca7cc63
>>
>> Can we wait with this until we're done with bi_error?
>
> Sure, I've moved it out to dm-4.3
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: block: add a bi_error field to struct bio

[Christoph Hellwig]

On Wed, Jun 10, 2015 at 11:26:49AM -0400, Mike Snitzer wrote:
> Unfortunately by dropping the original error (e.g. -EREMOTEIO) on the
> floor (in the 'if (endio) {' branch) you're breaking the REQ_WRITE_SAME
> check.

I think this also happens in the old code before my patch, e.g.:

static void clone_endio(struct bio *bio, int error)
{
        int r = error;

	...

	if (endio) {
		r = endio(tio->ti, bio, error);
		...
	}

	if (unlikely(r == -EREMOTEIO && (bio->bi_rw & REQ_WRITE_SAME) &&

so we already check the return value that comes from the endio handler,
not any different from my patch.

In the original code r and error are basically always the same, execept
after this:

	if (!bio_flagged(bio, BIO_UPTODATE) && !error)
		error = -EIO;

error might be -EIO and r might be 0. Now if we take the endio branch
we replace both error and r with the return value of endio for all
branches that don't immediately return or BUG().  For the non
endio branch we might check in REQ_WRITE_SAME branch, but r can
only be -EREMOTEIO if it got that value from error, so it doesn't
matter which one we test there.

Based on that I'm pretty sure my prep patch is transformation that
does not change behavior.

Re: block: add a bi_error field to struct bio

[Christoph Hellwig]

On Thu, Jun 11, 2015 at 09:53:27AM +0200, Christoph Hellwig wrote:
> On Wed, Jun 10, 2015 at 11:26:49AM -0400, Mike Snitzer wrote:
> > Unfortunately by dropping the original error (e.g. -EREMOTEIO) on the
> > floor (in the 'if (endio) {' branch) you're breaking the REQ_WRITE_SAME
> > check.
> 
> I think this also happens in the old code before my patch, e.g.:

Ok, after and audit of the instance I noticed that returning 0 from
->end_io even if an error is passed is a valid calling convention,
and with that the below analysis is invalid.  I'll take it back and
will do the simplest possible conversion for now and return my
attention to sorting out this mess later.

Re: [dm-devel] [PATCH] block: add a bi_error field to struct bio

[Neil Brown]

On Wed,  3 Jun 2015 15:42:39 +0200
Christoph Hellwig <hch@lst.de> 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@lst.de>

I really like this clean up.
It is unfortunate that the patch is so big, but I guess it has to be.

It mostly looks good, but review is hard and testing is harder :-(

I found:

> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> index f80f1af..1bad16f 100644
> --- a/drivers/md/raid1.c
> +++ b/drivers/md/raid1.c
....
> @@ -1800,7 +1799,7 @@ static void end_sync_write(struct bio *bio, int error)
>  			reschedule_retry(r1_bio);
>  		else {
>  			put_buf(r1_bio);
> -			md_done_sync(mddev, s, uptodate);
> +			md_done_sync(mddev, s, !bio->bi_error);
>  		}
>  	}
>  }

This introduces a use-after-free.  put_buf(r1_bio) can  result in bio_put on
'bio'.
It is safe to move the put_buf call after the md_done_sync(), but it is
probably best to leave the 'update' variable as it.  i.e. Just change:

-	int uptodate = test_bit(BIO_UPTODATE, &bio->bi_flags);
+	int uptodate = !bio->bi_error;


I can't see any other problems with the md changes.

Reviewed-by: NeilBrown <neilb@suse.de> (md/raid parts)

Thanks,
NeilBrown

Re: [dm-devel] [PATCH] block: add a bi_error field to struct bio

[Christoph Hellwig]

On Wed, Jun 10, 2015 at 12:50:54PM +1000, Neil Brown wrote:
> This introduces a use-after-free.  put_buf(r1_bio) can  result in bio_put on
> 'bio'.
> It is safe to move the put_buf call after the md_done_sync(), but it is
> probably best to leave the 'update' variable as it.  i.e. Just change:
> 
> -	int uptodate = test_bit(BIO_UPTODATE, &bio->bi_flags);
> +	int uptodate = !bio->bi_error;
> 
> 
> I can't see any other problems with the md changes.

Thanks, I'll keep the local uptodate variable for now.

Re: [RFC] add a bi_error field

[Liu Bo]

On Wed, Jun 03, 2015 at 03:42:38PM +0200, Christoph Hellwig wrote:
> Bio error reporting has been a mess for a while, and the increasing
> use of chained bios makes it worse.
> 
> This series attempts to add a proper error field to struct bio
> to sort this out.  It's working fine for me, but MD and btrfs were
> doing pretty nasty things with the BIO_UPTODATE flag, so I would
> appreciate some detailed review there.

Somehow the patch is missing from my mailbox, but this cover letter
remains, I have to review it from online archive.

Anyway, I went through btrfs part and it looks good.

Reviewed-by: Liu Bo <bo.li.liu@oracle.com> (btrfs part)

Thanks,

-liubo

Re: [RFC] add a bi_error field

[Liu Bo]

On Thu, Jun 11, 2015 at 03:59:22PM +0800, Liu Bo wrote:
> On Wed, Jun 03, 2015 at 03:42:38PM +0200, Christoph Hellwig wrote:
> > Bio error reporting has been a mess for a while, and the increasing
> > use of chained bios makes it worse.
> > 
> > This series attempts to add a proper error field to struct bio
> > to sort this out.  It's working fine for me, but MD and btrfs were
> > doing pretty nasty things with the BIO_UPTODATE flag, so I would
> > appreciate some detailed review there.
> 
> Somehow the patch is missing from my mailbox, but this cover letter
> remains, I have to review it from online archive.
> 
> Anyway, I went through btrfs part and it looks good.
> 
> Reviewed-by: Liu Bo <bo.li.liu@oracle.com> (btrfs part)

Also there're some conflicts in btrfs_end_empty_barrier():

...
static void btrfs_end_empty_barrier(struct bio *bio, int err)
{
	if (err) {
		if (err == -EOPNOTSUPP)
			set_bit(BIO_EOPNOTSUPP, &bio->bi_flags);
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
		clear_bit(BIO_UPTODATE, &bio->bi_flags);
	}
	if (bio->bi_private)
		complete(bio->bi_private);
	bio_put(bio);
}

Thanks,

-liubo

Re: [RFC] add a bi_error field

[Christoph Hellwig]

On Thu, Jun 11, 2015 at 04:05:30PM +0800, Liu Bo wrote:
> > Reviewed-by: Liu Bo <bo.li.liu@oracle.com> (btrfs part)
> 
> Also there're some conflicts in btrfs_end_empty_barrier():

Hi Liu,

BIO_EOPNOTSUPP has been removed in the block tree:

http://git.kernel.dk/?p=linux-block.git;a=commitdiff;h=b25de9d6da49b1a8760a89672283128aa8c78345

Re: [RFC] add a bi_error field

[Liu Bo]

On Thu, Jun 11, 2015 at 10:08:35AM +0200, Christoph Hellwig wrote:
> On Thu, Jun 11, 2015 at 04:05:30PM +0800, Liu Bo wrote:
> > > Reviewed-by: Liu Bo <bo.li.liu@oracle.com> (btrfs part)
> > 
> > Also there're some conflicts in btrfs_end_empty_barrier():
> 
> Hi Liu,
> 
> BIO_EOPNOTSUPP has been removed in the block tree:
> 
> http://git.kernel.dk/?p=linux-block.git;a=commitdiff;h=b25de9d6da49b1a8760a89672283128aa8c78345
> 

OK, we're good now.

Thanks,

-liubo

add a bi_error field to struct bio V3

[Christoph Hellwig]

Bio error reporting has been a mess for a while, and the increasing
use of chained bios makes it worse.  Add a bi_error field to struct
bio to fix this.

Note that the rebase to 4.2-rc means a lot of context changes, so I've
dropped the Reviewed-by tags from V2 as it will need a re-review.

Re: [dm-devel] [PATCH] block: add a bi_error field to struct bio

[Hannes Reinecke]

On 07/20/2015 03:29 PM, Christoph Hellwig 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@lst.de>
> ---
Very good improvement.

Reviewed-by: Hannes Reinecke <hare@suse.de>

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)