Re: block: add a bi_error field to struct bio

[Christoph Hellwig <hch@lst.de>]

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

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