small dm mpath cleanups

small dm mpath cleanups

[Christoph Hellwig]

Hi Mike,

this series has some prep patches for my work to have proper, type
checked block errors codes.  One fallout of that is that we need to
get rid of how dm overloads a few return values with either internal
positive error codes or negative errno values.  This patches does
that, which happens to clean things up a bit, and also allows us
dm to propagate the actual error code in one case where it currently
is dropped on the floor.

[PATCH 1/4] dm mpath: merge do_end_io into multipath_end_io

[Christoph Hellwig]

This simplifies the I/O completion path a bit.

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

diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
index 2950b145443d..1973670ea102 100644
--- a/drivers/md/dm-mpath.c
+++ b/drivers/md/dm-mpath.c
@@ -1473,12 +1473,12 @@ static int noretry_error(int error)
 	return 0;
 }
 
-/*
- * end_io handling
- */
-static int do_end_io(struct multipath *m, struct request *clone,
-		     int error, struct dm_mpath_io *mpio)
+static int multipath_end_io(struct dm_target *ti, struct request *clone,
+			    int error, union map_info *map_context)
 {
+	struct dm_mpath_io *mpio = get_mpio(map_context);
+	struct pgpath *pgpath = mpio->pgpath;
+
 	/*
 	 * We don't queue any clone request inside the multipath target
 	 * during end I/O handling, since those clone requests don't have
@@ -1490,47 +1490,30 @@ static int do_end_io(struct multipath *m, struct request *clone,
 	 * request into dm core, which will remake a clone request and
 	 * clone bios for it and resubmit it later.
 	 */
-	int r = DM_ENDIO_REQUEUE;
-
-	if (!error)
-		return 0;	/* I/O complete */
+	if (error && !noretry_error(error)) {
+		struct multipath *m = ti->private;
 
-	if (noretry_error(error))
-		return error;
+		error = DM_ENDIO_REQUEUE;
 
-	if (mpio->pgpath)
-		fail_path(mpio->pgpath);
+		if (pgpath)
+			fail_path(pgpath);
 
-	if (!atomic_read(&m->nr_valid_paths)) {
-		if (!test_bit(MPATHF_QUEUE_IF_NO_PATH, &m->flags)) {
-			if (!must_push_back_rq(m))
-				r = -EIO;
+		if (!atomic_read(&m->nr_valid_paths)) {
+			if (!test_bit(MPATHF_QUEUE_IF_NO_PATH, &m->flags)) {
+				if (!must_push_back_rq(m))
+					error = -EIO;
+			}
 		}
 	}
 
-	return r;
-}
-
-static int multipath_end_io(struct dm_target *ti, struct request *clone,
-			    int error, union map_info *map_context)
-{
-	struct multipath *m = ti->private;
-	struct dm_mpath_io *mpio = get_mpio(map_context);
-	struct pgpath *pgpath;
-	struct path_selector *ps;
-	int r;
-
-	BUG_ON(!mpio);
-
-	r = do_end_io(m, clone, error, mpio);
-	pgpath = mpio->pgpath;
 	if (pgpath) {
-		ps = &pgpath->pg->ps;
+		struct path_selector *ps = &pgpath->pg->ps;
+
 		if (ps->type->end_io)
 			ps->type->end_io(ps, &pgpath->path, mpio->nr_bytes);
 	}
 
-	return r;
+	return error;
 }
 
 static int do_end_io_bio(struct multipath *m, struct bio *clone,
-- 
2.11.0

[PATCH 2/4] dm rq: change ->rq_end_io calling conventions

[Christoph Hellwig]

Instead of returning either a DM_ENDIO_* constant or an error code, add
a new DM_ENDIO_DONE value that means keep errno as is.  This allows us
to easily keep the existing error code in case where we can't push back,
and it also preparares for the new block level status codes with strict
type checking.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/md/dm-mpath.c         |  8 +++++---
 drivers/md/dm-rq.c            | 17 ++++++++++-------
 include/linux/device-mapper.h |  1 +
 3 files changed, 16 insertions(+), 10 deletions(-)

diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
index 1973670ea102..9e971be254f4 100644
--- a/drivers/md/dm-mpath.c
+++ b/drivers/md/dm-mpath.c
@@ -1478,6 +1478,7 @@ static int multipath_end_io(struct dm_target *ti, struct request *clone,
 {
 	struct dm_mpath_io *mpio = get_mpio(map_context);
 	struct pgpath *pgpath = mpio->pgpath;
+	int r = DM_ENDIO_DONE;
 
 	/*
 	 * We don't queue any clone request inside the multipath target
@@ -1493,15 +1494,16 @@ static int multipath_end_io(struct dm_target *ti, struct request *clone,
 	if (error && !noretry_error(error)) {
 		struct multipath *m = ti->private;
 
-		error = DM_ENDIO_REQUEUE;
+		r = DM_ENDIO_REQUEUE;
 
 		if (pgpath)
 			fail_path(pgpath);
 
 		if (!atomic_read(&m->nr_valid_paths)) {
 			if (!test_bit(MPATHF_QUEUE_IF_NO_PATH, &m->flags)) {
+				/* complete with the original error */
 				if (!must_push_back_rq(m))
-					error = -EIO;
+					r = DM_ENDIO_DONE;
 			}
 		}
 	}
@@ -1513,7 +1515,7 @@ static int multipath_end_io(struct dm_target *ti, struct request *clone,
 			ps->type->end_io(ps, &pgpath->path, mpio->nr_bytes);
 	}
 
-	return error;
+	return r;
 }
 
 static int do_end_io_bio(struct multipath *m, struct bio *clone,
diff --git a/drivers/md/dm-rq.c b/drivers/md/dm-rq.c
index bff7e3bdb4ed..769f04f83ca2 100644
--- a/drivers/md/dm-rq.c
+++ b/drivers/md/dm-rq.c
@@ -287,7 +287,7 @@ static void dm_requeue_original_request(struct dm_rq_target_io *tio, bool delay_
 
 static void dm_done(struct request *clone, int error, bool mapped)
 {
-	int r = error;
+	int r = DM_ENDIO_DONE;
 	struct dm_rq_target_io *tio = clone->end_io_data;
 	dm_request_endio_fn rq_end_io = NULL;
 
@@ -298,7 +298,7 @@ static void dm_done(struct request *clone, int error, bool mapped)
 			r = rq_end_io(tio->ti, clone, error, &tio->info);
 	}
 
-	if (unlikely(r == -EREMOTEIO)) {
+	if (unlikely(error == -EREMOTEIO)) {
 		if (req_op(clone) == REQ_OP_WRITE_SAME &&
 		    !clone->q->limits.max_write_same_sectors)
 			disable_write_same(tio->md);
@@ -307,16 +307,19 @@ static void dm_done(struct request *clone, int error, bool mapped)
 			disable_write_zeroes(tio->md);
 	}
 
-	if (r <= 0)
+	switch (r) {
+	case DM_ENDIO_DONE:
 		/* The target wants to complete the I/O */
-		dm_end_request(clone, r);
-	else if (r == DM_ENDIO_INCOMPLETE)
+		dm_end_request(clone, error);
+		break;
+	case DM_ENDIO_INCOMPLETE:
 		/* The target will handle the I/O */
 		return;
-	else if (r == DM_ENDIO_REQUEUE)
+	case DM_ENDIO_REQUEUE:
 		/* The target wants to requeue the I/O */
 		dm_requeue_original_request(tio, false);
-	else {
+		break;
+	default:
 		DMWARN("unimplemented target endio return value: %d", r);
 		BUG();
 	}
diff --git a/include/linux/device-mapper.h b/include/linux/device-mapper.h
index c7ea33e38fb9..5bafe3af27cc 100644
--- a/include/linux/device-mapper.h
+++ b/include/linux/device-mapper.h
@@ -579,6 +579,7 @@ extern struct ratelimit_state dm_ratelimit_state;
 /*
  * Definitions of return values from target end_io function.
  */
+#define DM_ENDIO_DONE		0
 #define DM_ENDIO_INCOMPLETE	1
 #define DM_ENDIO_REQUEUE	2
 
-- 
2.11.0

[PATCH 3/4] dm mpath: simplify multipath_clone_and_map a little bit

[Christoph Hellwig]

Remove the r variable and just expand the value at the two places where
it is used.

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

diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
index 9e971be254f4..33ecda737376 100644
--- a/drivers/md/dm-mpath.c
+++ b/drivers/md/dm-mpath.c
@@ -484,7 +484,6 @@ static int multipath_clone_and_map(struct dm_target *ti, struct request *rq,
 				   struct request **__clone)
 {
 	struct multipath *m = ti->private;
-	int r = DM_MAPIO_REQUEUE;
 	size_t nr_bytes = blk_rq_bytes(rq);
 	struct pgpath *pgpath;
 	struct block_device *bdev;
@@ -503,7 +502,7 @@ static int multipath_clone_and_map(struct dm_target *ti, struct request *rq,
 	} else if (test_bit(MPATHF_QUEUE_IO, &m->flags) ||
 		   test_bit(MPATHF_PG_INIT_REQUIRED, &m->flags)) {
 		pg_init_all_paths(m);
-		return r;
+		return DM_MAPIO_REQUEUE;
 	}
 
 	memset(mpio, 0, sizeof(*mpio));
@@ -517,7 +516,7 @@ static int multipath_clone_and_map(struct dm_target *ti, struct request *rq,
 			GFP_ATOMIC);
 	if (IS_ERR(clone)) {
 		/* EBUSY, ENODEV or EWOULDBLOCK: requeue */
-		return r;
+		return DM_MAPIO_REQUEUE;
 	}
 	clone->bio = clone->biotail = NULL;
 	clone->rq_disk = bdev->bd_disk;
-- 
2.11.0

[PATCH 4/4] dm: introduce a new DM_MAPIO_KILL return value

[Christoph Hellwig]

This untangles the DM_MAPIO_* values returned from ->clone_and_map_rq
from the error codes used by the block layer.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/md/dm-rq.c            | 12 +++++-------
 drivers/md/dm-target.c        |  2 +-
 include/linux/device-mapper.h |  1 +
 3 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/md/dm-rq.c b/drivers/md/dm-rq.c
index 769f04f83ca2..ea4fdf68a0ed 100644
--- a/drivers/md/dm-rq.c
+++ b/drivers/md/dm-rq.c
@@ -504,14 +504,12 @@ static int map_request(struct dm_rq_target_io *tio)
 		/* The target wants to requeue the I/O after a delay */
 		dm_requeue_original_request(tio, true);
 		break;
-	default:
-		if (r > 0) {
-			DMWARN("unimplemented target map return value: %d", r);
-			BUG();
-		}
-
+	case DM_MAPIO_KILL:
 		/* The target wants to complete the I/O */
-		dm_kill_unmapped_request(rq, r);
+		dm_kill_unmapped_request(rq, -EIO);
+	default:
+		DMWARN("unimplemented target map return value: %d", r);
+		BUG();
 	}
 
 	return r;
diff --git a/drivers/md/dm-target.c b/drivers/md/dm-target.c
index 43d3445b121d..6264ff00dcf0 100644
--- a/drivers/md/dm-target.c
+++ b/drivers/md/dm-target.c
@@ -135,7 +135,7 @@ static int io_err_clone_and_map_rq(struct dm_target *ti, struct request *rq,
 				   union map_info *map_context,
 				   struct request **clone)
 {
-	return -EIO;
+	return DM_MAPIO_KILL;
 }
 
 static void io_err_release_clone_rq(struct request *clone)
diff --git a/include/linux/device-mapper.h b/include/linux/device-mapper.h
index 5bafe3af27cc..0dc4d9cce128 100644
--- a/include/linux/device-mapper.h
+++ b/include/linux/device-mapper.h
@@ -590,6 +590,7 @@ extern struct ratelimit_state dm_ratelimit_state;
 #define DM_MAPIO_REMAPPED	1
 #define DM_MAPIO_REQUEUE	DM_ENDIO_REQUEUE
 #define DM_MAPIO_DELAY_REQUEUE	3
+#define DM_MAPIO_KILL		4
 
 #define dm_sector_div64(x, y)( \
 { \
-- 
2.11.0

Re: small dm mpath cleanups

[Bart Van Assche]

On Wed, 2017-04-26 at 09:40 +0200, Christoph Hellwig wrote:
> this series has some prep patches for my work to have proper, type
> checked block errors codes.  One fallout of that is that we need to
> get rid of how dm overloads a few return values with either internal
> positive error codes or negative errno values.  This patches does
> that, which happens to clean things up a bit, and also allows us
> dm to propagate the actual error code in one case where it currently
> is dropped on the floor.

Hello Christoph,

Some patches in this series conflict with patches I would like to end up in
the stable kernel series. If I would rebase my patch series on top of your
series then that would make it harder to apply my patches on the stable
kernel trees. Mike and Christoph, please advise how to proceed.

Bart.=

Re: small dm mpath cleanups

[Mike Snitzer]

On Wed, Apr 26 2017 at  2:41pm -0400,
Bart Van Assche <Bart.VanAssche@sandisk.com> wrote:

> On Wed, 2017-04-26 at 09:40 +0200, Christoph Hellwig wrote:
> > this series has some prep patches for my work to have proper, type
> > checked block errors codes.  One fallout of that is that we need to
> > get rid of how dm overloads a few return values with either internal
> > positive error codes or negative errno values.  This patches does
> > that, which happens to clean things up a bit, and also allows us
> > dm to propagate the actual error code in one case where it currently
> > is dropped on the floor.
> 
> Hello Christoph,
> 
> Some patches in this series conflict with patches I would like to end up in
> the stable kernel series. If I would rebase my patch series on top of your
> series then that would make it harder to apply my patches on the stable
> kernel trees. Mike and Christoph, please advise how to proceed.

I'll go over your series and christoph's tomorrow (thursday).  I'll pull
all fixes to the front and rebase later patches as needed.

Mike

Re: small dm mpath cleanups

[hch]

On Wed, Apr 26, 2017 at 06:41:27PM +0000, Bart Van Assche wrote:
> On Wed, 2017-04-26 at 09:40 +0200, Christoph Hellwig wrote:
> > this series has some prep patches for my work to have proper, type
> > checked block errors codes.  One fallout of that is that we need to
> > get rid of how dm overloads a few return values with either internal
> > positive error codes or negative errno values.  This patches does
> > that, which happens to clean things up a bit, and also allows us
> > dm to propagate the actual error code in one case where it currently
> > is dropped on the floor.
> 
> Hello Christoph,
> 
> Some patches in this series conflict with patches I would like to end up in
> the stable kernel series. If I would rebase my patch series on top of your
> series then that would make it harder to apply my patches on the stable
> kernel trees. Mike and Christoph, please advise how to proceed.

Bugfixes always go before cleanups.  I'd be happy to delay and/or rebase
any of my patches as needed.

Re: small dm mpath cleanups

[Mike Snitzer]

On Thu, Apr 27 2017 at  2:33am -0400,
hch@lst.de <hch@lst.de> wrote:

> On Wed, Apr 26, 2017 at 06:41:27PM +0000, Bart Van Assche wrote:
> > On Wed, 2017-04-26 at 09:40 +0200, Christoph Hellwig wrote:
> > > this series has some prep patches for my work to have proper, type
> > > checked block errors codes.  One fallout of that is that we need to
> > > get rid of how dm overloads a few return values with either internal
> > > positive error codes or negative errno values.  This patches does
> > > that, which happens to clean things up a bit, and also allows us
> > > dm to propagate the actual error code in one case where it currently
> > > is dropped on the floor.
> > 
> > Hello Christoph,
> > 
> > Some patches in this series conflict with patches I would like to end up in
> > the stable kernel series. If I would rebase my patch series on top of your
> > series then that would make it harder to apply my patches on the stable
> > kernel trees. Mike and Christoph, please advise how to proceed.
> 
> Bugfixes always go before cleanups.  I'd be happy to delay and/or rebase
> any of my patches as needed.

I rebased your patchset ontop of Bart's patchset that I've already
staged in linux-dm.git's 'dm-4.12' branch.

When I try to apply this rebased patchset, it turns out 'dm-4.12' is
missing linux-block.git commit 8fc779805 ("dm mpath: don't check for
req->errors").  Because of this your first patch ("dm mpath: merge
do_end_io into multipath_end_io") won't apply due to conflict.

Not sure how we skin this cat.  Unfortunately Jens cannot easily pick
this rebased patchset up because he'll be missing all of Bart's changes
that I've staged in linux-dm.git's 'dm-4.12'.

In hindsight, linux-block.git commit 8fc779805 likely should've been
routed through linux-dm.git.  But not a big deal.

How should we skin this cat of getting your changes into 4.12?  I could
send a 2nd pull request to Linus after both linux-block.git and
linux-dm.git are merged... sound OK?

Until then, and/or for now, I've staged the fully merged result in
linux-next via linux-dm.git's 'for-next', see:
https://git.kernel.org/pub/scm/linux/kernel/git/device-mapper/linux-dm.git/log/?h=for-next

Mike

Re: small dm mpath cleanups

[Bart Van Assche]

On Fri, 2017-04-28 at 16:23 -0400, Mike Snitzer wrote:
> On Thu, Apr 27 2017 at  2:33am -0400, hch@lst.de <hch@lst.de> wrote:
> > On Wed, Apr 26, 2017 at 06:41:27PM +0000, Bart Van Assche wrote:
> > > On Wed, 2017-04-26 at 09:40 +0200, Christoph Hellwig wrote:
> > > > this series has some prep patches for my work to have proper, type
> > > > checked block errors codes.  One fallout of that is that we need to
> > > > get rid of how dm overloads a few return values with either interna=
l
> > > > positive error codes or negative errno values.  This patches does
> > > > that, which happens to clean things up a bit, and also allows us
> > > > dm to propagate the actual error code in one case where it currentl=
y
> > > > is dropped on the floor.
> > >=20
> > > Hello Christoph,
> > >=20
> > > Some patches in this series conflict with patches I would like to end=
 up in
> > > the stable kernel series. If I would rebase my patch series on top of=
 your
> > > series then that would make it harder to apply my patches on the stab=
le
> > > kernel trees. Mike and Christoph, please advise how to proceed.
> >=20
> > Bugfixes always go before cleanups.  I'd be happy to delay and/or rebas=
e
> > any of my patches as needed.
>=20
> I rebased your patchset ontop of Bart's patchset that I've already
> staged in linux-dm.git's 'dm-4.12' branch.
>=20
> When I try to apply this rebased patchset, it turns out 'dm-4.12' is
> missing linux-block.git commit 8fc779805 ("dm mpath: don't check for
> req->errors").  Because of this your first patch ("dm mpath: merge
> do_end_io into multipath_end_io") won't apply due to conflict.
>=20
> Not sure how we skin this cat.  Unfortunately Jens cannot easily pick
> this rebased patchset up because he'll be missing all of Bart's changes
> that I've staged in linux-dm.git's 'dm-4.12'.
>=20
> In hindsight, linux-block.git commit 8fc779805 likely should've been
> routed through linux-dm.git.  But not a big deal.
>=20
> How should we skin this cat of getting your changes into 4.12?  I could
> send a 2nd pull request to Linus after both linux-block.git and
> linux-dm.git are merged... sound OK?
>=20
> Until then, and/or for now, I've staged the fully merged result in
> linux-next via linux-dm.git's 'for-next', see:
> https://git.kernel.org/pub/scm/linux/kernel/git/device-mapper/linux-dm.gi=
t/log/?h=3Dfor-next

Hello Mike,

A few days ago Linus wrote that he is OK with occasional cherry-picking of
patches to resolve scenarios like the one described above (see also
https://lkml.org/lkml/2017/4/14/484). How about cherry-picking the necessar=
y
commit(s) from Jens' tree into the dm for-next branch?

Bart.=