linux-btrfs.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [RFC] add a bi_error field
@ 2015-06-03 13:42 Christoph Hellwig
       [not found] ` <1433338959-24808-2-git-send-email-hch@lst.de>
  2015-06-11  7:59 ` [RFC] add a bi_error field Liu Bo
  0 siblings, 2 replies; 17+ messages in thread
From: Christoph Hellwig @ 2015-06-03 13:42 UTC (permalink / raw)
  To: Jens Axboe; +Cc: linux-raid, linux-btrfs, dm-devel

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.


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [dm-devel] [PATCH] block: add a bi_error field to struct bio
       [not found] ` <1433338959-24808-2-git-send-email-hch@lst.de>
@ 2015-06-04  9:53   ` Martin K. Petersen
  2015-06-04 15:31   ` Mike Snitzer
  2015-06-10  2:50   ` [dm-devel] [PATCH] " Neil Brown
  2 siblings, 0 replies; 17+ messages in thread
From: Martin K. Petersen @ 2015-06-04  9:53 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Jens Axboe, device-mapper development, linux-raid, linux-btrfs

>>>>> "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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: block: add a bi_error field to struct bio
       [not found] ` <1433338959-24808-2-git-send-email-hch@lst.de>
  2015-06-04  9:53   ` [dm-devel] [PATCH] block: add a bi_error field to struct bio Martin K. Petersen
@ 2015-06-04 15:31   ` Mike Snitzer
  2015-06-10  8:11     ` Christoph Hellwig
  2015-06-10  2:50   ` [dm-devel] [PATCH] " Neil Brown
  2 siblings, 1 reply; 17+ messages in thread
From: Mike Snitzer @ 2015-06-04 15:31 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: Jens Axboe, linux-raid, dm-devel, linux-btrfs

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)

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [dm-devel] [PATCH] block: add a bi_error field to struct bio
       [not found] ` <1433338959-24808-2-git-send-email-hch@lst.de>
  2015-06-04  9:53   ` [dm-devel] [PATCH] block: add a bi_error field to struct bio Martin K. Petersen
  2015-06-04 15:31   ` Mike Snitzer
@ 2015-06-10  2:50   ` Neil Brown
  2015-06-10  8:45     ` Christoph Hellwig
  2 siblings, 1 reply; 17+ messages in thread
From: Neil Brown @ 2015-06-10  2:50 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: Jens Axboe, linux-raid, dm-devel, linux-btrfs

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: block: add a bi_error field to struct bio
  2015-06-04 15:31   ` Mike Snitzer
@ 2015-06-10  8:11     ` Christoph Hellwig
  2015-06-10 15:26       ` Mike Snitzer
  0 siblings, 1 reply; 17+ messages in thread
From: Christoph Hellwig @ 2015-06-10  8:11 UTC (permalink / raw)
  To: Mike Snitzer; +Cc: Jens Axboe, linux-raid, dm-devel, linux-btrfs

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


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [dm-devel] [PATCH] block: add a bi_error field to struct bio
  2015-06-10  2:50   ` [dm-devel] [PATCH] " Neil Brown
@ 2015-06-10  8:45     ` Christoph Hellwig
  0 siblings, 0 replies; 17+ messages in thread
From: Christoph Hellwig @ 2015-06-10  8:45 UTC (permalink / raw)
  To: Neil Brown
  Cc: Christoph Hellwig, Jens Axboe, linux-raid, dm-devel, linux-btrfs

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.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: block: add a bi_error field to struct bio
  2015-06-10  8:11     ` Christoph Hellwig
@ 2015-06-10 15:26       ` Mike Snitzer
  2015-06-10 16:01         ` Mike Snitzer
  2015-06-11  7:53         ` Christoph Hellwig
  0 siblings, 2 replies; 17+ messages in thread
From: Mike Snitzer @ 2015-06-10 15:26 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: Jens Axboe, linux-raid, dm-devel, linux-btrfs

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.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: block: add a bi_error field to struct bio
  2015-06-10 15:26       ` Mike Snitzer
@ 2015-06-10 16:01         ` Mike Snitzer
  2015-06-10 16:04           ` Christoph Hellwig
  2015-06-11  7:53         ` Christoph Hellwig
  1 sibling, 1 reply; 17+ messages in thread
From: Mike Snitzer @ 2015-06-10 16:01 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: Jens Axboe, linux-raid, dm-devel, linux-btrfs

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: block: add a bi_error field to struct bio
  2015-06-10 16:01         ` Mike Snitzer
@ 2015-06-10 16:04           ` Christoph Hellwig
  2015-06-10 16:50             ` Mike Snitzer
  0 siblings, 1 reply; 17+ messages in thread
From: Christoph Hellwig @ 2015-06-10 16:04 UTC (permalink / raw)
  To: Mike Snitzer
  Cc: Christoph Hellwig, Jens Axboe, linux-raid, dm-devel, linux-btrfs

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?

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: block: add a bi_error field to struct bio
  2015-06-10 16:04           ` Christoph Hellwig
@ 2015-06-10 16:50             ` Mike Snitzer
  2015-06-10 18:29               ` anup modak
  0 siblings, 1 reply; 17+ messages in thread
From: Mike Snitzer @ 2015-06-10 16:50 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: Jens Axboe, linux-raid, dm-devel, linux-btrfs

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: block: add a bi_error field to struct bio
  2015-06-10 16:50             ` Mike Snitzer
@ 2015-06-10 18:29               ` anup modak
  0 siblings, 0 replies; 17+ messages in thread
From: anup modak @ 2015-06-10 18:29 UTC (permalink / raw)
  To: Mike Snitzer
  Cc: Christoph Hellwig, Jens Axboe, linux-raid, dm-devel, linux-btrfs

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: block: add a bi_error field to struct bio
  2015-06-10 15:26       ` Mike Snitzer
  2015-06-10 16:01         ` Mike Snitzer
@ 2015-06-11  7:53         ` Christoph Hellwig
  2015-06-11  7:59           ` Christoph Hellwig
  1 sibling, 1 reply; 17+ messages in thread
From: Christoph Hellwig @ 2015-06-11  7:53 UTC (permalink / raw)
  To: Mike Snitzer
  Cc: Christoph Hellwig, Jens Axboe, linux-raid, dm-devel, linux-btrfs

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.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: block: add a bi_error field to struct bio
  2015-06-11  7:53         ` Christoph Hellwig
@ 2015-06-11  7:59           ` Christoph Hellwig
  0 siblings, 0 replies; 17+ messages in thread
From: Christoph Hellwig @ 2015-06-11  7:59 UTC (permalink / raw)
  To: Mike Snitzer
  Cc: Christoph Hellwig, Jens Axboe, linux-raid, dm-devel, linux-btrfs

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.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [RFC] add a bi_error field
  2015-06-03 13:42 [RFC] add a bi_error field Christoph Hellwig
       [not found] ` <1433338959-24808-2-git-send-email-hch@lst.de>
@ 2015-06-11  7:59 ` Liu Bo
  2015-06-11  8:05   ` Liu Bo
  1 sibling, 1 reply; 17+ messages in thread
From: Liu Bo @ 2015-06-11  7:59 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: Jens Axboe, linux-raid, linux-btrfs, dm-devel

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [RFC] add a bi_error field
  2015-06-11  7:59 ` [RFC] add a bi_error field Liu Bo
@ 2015-06-11  8:05   ` Liu Bo
  2015-06-11  8:08     ` Christoph Hellwig
  0 siblings, 1 reply; 17+ messages in thread
From: Liu Bo @ 2015-06-11  8:05 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: Jens Axboe, linux-raid, linux-btrfs, dm-devel

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [RFC] add a bi_error field
  2015-06-11  8:05   ` Liu Bo
@ 2015-06-11  8:08     ` Christoph Hellwig
  2015-06-11  9:42       ` Liu Bo
  0 siblings, 1 reply; 17+ messages in thread
From: Christoph Hellwig @ 2015-06-11  8:08 UTC (permalink / raw)
  To: Liu Bo; +Cc: Christoph Hellwig, Jens Axboe, linux-raid, linux-btrfs, dm-devel

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


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [RFC] add a bi_error field
  2015-06-11  8:08     ` Christoph Hellwig
@ 2015-06-11  9:42       ` Liu Bo
  0 siblings, 0 replies; 17+ messages in thread
From: Liu Bo @ 2015-06-11  9:42 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: Jens Axboe, linux-raid, linux-btrfs, dm-devel

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2015-06-11  9:43 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-06-03 13:42 [RFC] add a bi_error field Christoph Hellwig
     [not found] ` <1433338959-24808-2-git-send-email-hch@lst.de>
2015-06-04  9:53   ` [dm-devel] [PATCH] block: add a bi_error field to struct bio Martin K. Petersen
2015-06-04 15:31   ` Mike Snitzer
2015-06-10  8:11     ` Christoph Hellwig
2015-06-10 15:26       ` Mike Snitzer
2015-06-10 16:01         ` Mike Snitzer
2015-06-10 16:04           ` Christoph Hellwig
2015-06-10 16:50             ` Mike Snitzer
2015-06-10 18:29               ` anup modak
2015-06-11  7:53         ` Christoph Hellwig
2015-06-11  7:59           ` Christoph Hellwig
2015-06-10  2:50   ` [dm-devel] [PATCH] " Neil Brown
2015-06-10  8:45     ` Christoph Hellwig
2015-06-11  7:59 ` [RFC] add a bi_error field Liu Bo
2015-06-11  8:05   ` Liu Bo
2015-06-11  8:08     ` Christoph Hellwig
2015-06-11  9:42       ` Liu Bo

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).