dm fixes for 4.12-rc

dm fixes for 4.12-rc

[Christoph Hellwig]

Hi Mike,

the first three patches sort out a few issues with how my dm cleanups and
the multipath fixes from Bart interacted.  The fourth is something I found
in an audit, it might or might not be worth to go into 4.12.  If not please
don't apply it yet as it will be needed as the baseline for some block work
I'll post in a few days.

[PATCH 1/4] dm rq: add a missing break to map_request

[Christoph Hellwig]

We don't want to bug when receiving a DM_MAPIO_KILL value..

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/md/dm-rq.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/md/dm-rq.c b/drivers/md/dm-rq.c
index 2af27026aa2e..b639fa7246ee 100644
--- a/drivers/md/dm-rq.c
+++ b/drivers/md/dm-rq.c
@@ -507,6 +507,7 @@ static int map_request(struct dm_rq_target_io *tio)
 	case DM_MAPIO_KILL:
 		/* The target wants to complete the I/O */
 		dm_kill_unmapped_request(rq, -EIO);
+		break;
 	default:
 		DMWARN("unimplemented target map return value: %d", r);
 		BUG();
-- 
2.11.0

Re: [PATCH 1/4] dm rq: add a missing break to map_request

[Mike Snitzer]

On Mon, May 15 2017 at 11:28am -0400,
Christoph Hellwig <hch@lst.de> wrote:

> We don't want to bug when receiving a DM_MAPIO_KILL value..
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  drivers/md/dm-rq.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/md/dm-rq.c b/drivers/md/dm-rq.c
> index 2af27026aa2e..b639fa7246ee 100644
> --- a/drivers/md/dm-rq.c
> +++ b/drivers/md/dm-rq.c
> @@ -507,6 +507,7 @@ static int map_request(struct dm_rq_target_io *tio)
>  	case DM_MAPIO_KILL:
>  		/* The target wants to complete the I/O */
>  		dm_kill_unmapped_request(rq, -EIO);
> +		break;
>  	default:
>  		DMWARN("unimplemented target map return value: %d", r);
>  		BUG();
> -- 
> 2.11.0
> 

Thanks, but I have no idea how this happened.  I specifically recall
fixing this very same issue (missing break).  Grr...

[PATCH 2/4] dm mpath: don't return -EIO from dm_report_EIO

[Christoph Hellwig]

Instead just turn the macro into a helper for the warning message.
This removes an unessecary assignment in this patch, and will allow
to fix a place where -EIO is the wrong error value in th next.

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

diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
index 926a6bcb32c8..d55454f98b59 100644
--- a/drivers/md/dm-mpath.c
+++ b/drivers/md/dm-mpath.c
@@ -447,7 +447,7 @@ static struct pgpath *choose_pgpath(struct multipath *m, size_t nr_bytes)
  * it has been invoked.
  */
 #define dm_report_EIO(m)						\
-({									\
+do {									\
 	struct mapped_device *md = dm_table_get_md((m)->ti->table);	\
 									\
 	pr_debug("%s: returning EIO; QIFNP = %d; SQIFNP = %d; DNFS = %d\n", \
@@ -455,8 +455,7 @@ static struct pgpath *choose_pgpath(struct multipath *m, size_t nr_bytes)
 		 test_bit(MPATHF_QUEUE_IF_NO_PATH, &(m)->flags),	\
 		 test_bit(MPATHF_SAVED_QUEUE_IF_NO_PATH, &(m)->flags),	\
 		 dm_noflush_suspending((m)->ti));			\
-	-EIO;								\
-})
+} while (0)
 
 /*
  * Map cloned requests (request-based multipath)
@@ -481,7 +480,8 @@ static int multipath_clone_and_map(struct dm_target *ti, struct request *rq,
 	if (!pgpath) {
 		if (test_bit(MPATHF_QUEUE_IF_NO_PATH, &m->flags))
 			return DM_MAPIO_DELAY_REQUEUE;
-		return dm_report_EIO(m);	/* Failed */
+		dm_report_EIO(m);	/* Failed */
+		return -EIO;
 	} else if (test_bit(MPATHF_QUEUE_IO, &m->flags) ||
 		   test_bit(MPATHF_PG_INIT_REQUIRED, &m->flags)) {
 		if (pg_init_all_paths(m))
@@ -558,7 +558,8 @@ static int __multipath_map_bio(struct multipath *m, struct bio *bio, struct dm_m
 	if (!pgpath) {
 		if (test_bit(MPATHF_QUEUE_IF_NO_PATH, &m->flags))
 			return DM_MAPIO_REQUEUE;
-		return dm_report_EIO(m);
+		dm_report_EIO(m);
+		return -EIO;
 	}
 
 	mpio->pgpath = pgpath;
@@ -1493,7 +1494,7 @@ static int multipath_end_io(struct dm_target *ti, struct request *clone,
 		if (atomic_read(&m->nr_valid_paths) == 0 &&
 		    !test_bit(MPATHF_QUEUE_IF_NO_PATH, &m->flags)) {
 			if (error == -EIO)
-				error = dm_report_EIO(m);
+				dm_report_EIO(m);
 			/* complete with the original error */
 			r = DM_ENDIO_DONE;
 		}
@@ -1524,8 +1525,10 @@ static int do_end_io_bio(struct multipath *m, struct bio *clone,
 		fail_path(mpio->pgpath);
 
 	if (atomic_read(&m->nr_valid_paths) == 0 &&
-	    !test_bit(MPATHF_QUEUE_IF_NO_PATH, &m->flags))
-		return dm_report_EIO(m);
+	    !test_bit(MPATHF_QUEUE_IF_NO_PATH, &m->flags)) {
+		dm_report_EIO(m);
+		return -EIO;
+	}
 
 	/* Queue for the daemon to resubmit */
 	dm_bio_restore(get_bio_details_from_bio(clone), clone);
-- 
2.11.0

Re: [PATCH 2/4] dm mpath: don't return -EIO from dm_report_EIO

[Mike Snitzer]

On Mon, May 15 2017 at 11:28am -0400,
Christoph Hellwig <hch@lst.de> wrote:

> Instead just turn the macro into a helper for the warning message.
> This removes an unessecary assignment in this patch, and will allow
> to fix a place where -EIO is the wrong error value in th next.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Something clearlyy got screwed up in my rebase.  Because I specifically
recall casting the multipath_clone_and_map()'s !pgpath call with:
(void) dm_report_EIO(m);

Very weird.  Anyway, thanks for catching these issues.

Mike

[PATCH 3/4] dm mpath: multipath_clone_and_map must not return -EIO

[Christoph Hellwig]

Since 412445ac ("dm: introduce a new DM_MAPIO_KILL return value"),  the
clone_and_map_rq methods must not return errno values, so fix it up
to properly return DM_MAPIO_KILL instead, instead of the -EIO value
that sneaked in due to a conflict between two patches.

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

diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
index d55454f98b59..3df056b73b66 100644
--- a/drivers/md/dm-mpath.c
+++ b/drivers/md/dm-mpath.c
@@ -481,7 +481,7 @@ static int multipath_clone_and_map(struct dm_target *ti, struct request *rq,
 		if (test_bit(MPATHF_QUEUE_IF_NO_PATH, &m->flags))
 			return DM_MAPIO_DELAY_REQUEUE;
 		dm_report_EIO(m);	/* Failed */
-		return -EIO;
+		return DM_MAPIO_KILL;
 	} else if (test_bit(MPATHF_QUEUE_IO, &m->flags) ||
 		   test_bit(MPATHF_PG_INIT_REQUIRED, &m->flags)) {
 		if (pg_init_all_paths(m))
-- 
2.11.0

[PATCH 4/4] dm: fix REQ_RAHEAD handling

[Christoph Hellwig]

A few (but not all) dm targets use a special EWOULDBLOCK error code for
failing REQ_RAHEAD requests that fail due to a lack of available resources.
But no one else knows about this magic code, and lower level drivers also
don't generate it when failing read-ahead requests for similar reasons.

So remove this special casing and ignore all additional error handling for
REQ_RAHEAD - if this was a real underlying error we'd get a normal read
once the real read comes in.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/md/dm-raid1.c  | 4 ++--
 drivers/md/dm-stripe.c | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/md/dm-raid1.c b/drivers/md/dm-raid1.c
index a95cbb80fb34..5e30b08b91d9 100644
--- a/drivers/md/dm-raid1.c
+++ b/drivers/md/dm-raid1.c
@@ -1214,7 +1214,7 @@ static int mirror_map(struct dm_target *ti, struct bio *bio)
 	 */
 	if (!r || (r == -EWOULDBLOCK)) {
 		if (bio->bi_opf & REQ_RAHEAD)
-			return -EWOULDBLOCK;
+			return -EIO;
 
 		queue_bio(ms, bio, rw);
 		return DM_MAPIO_SUBMITTED;
@@ -1258,7 +1258,7 @@ static int mirror_end_io(struct dm_target *ti, struct bio *bio, int error)
 	if (error == -EOPNOTSUPP)
 		return error;
 
-	if ((error == -EWOULDBLOCK) && (bio->bi_opf & REQ_RAHEAD))
+	if (bio->bi_opf & REQ_RAHEAD)
 		return error;
 
 	if (unlikely(error)) {
diff --git a/drivers/md/dm-stripe.c b/drivers/md/dm-stripe.c
index 75152482f3ad..780e95889a7c 100644
--- a/drivers/md/dm-stripe.c
+++ b/drivers/md/dm-stripe.c
@@ -384,7 +384,7 @@ static int stripe_end_io(struct dm_target *ti, struct bio *bio, int error)
 	if (!error)
 		return 0; /* I/O complete */
 
-	if ((error == -EWOULDBLOCK) && (bio->bi_opf & REQ_RAHEAD))
+	if (bio->bi_opf & REQ_RAHEAD)
 		return error;
 
 	if (error == -EOPNOTSUPP)
-- 
2.11.0

Re: [PATCH 4/4] dm: fix REQ_RAHEAD handling

[Mike Snitzer]

On Mon, May 15 2017 at 11:28P -0400,
Christoph Hellwig <hch@lst.de> wrote:

> A few (but not all) dm targets use a special EWOULDBLOCK error code for
> failing REQ_RAHEAD requests that fail due to a lack of available resources.
> But no one else knows about this magic code, and lower level drivers also
> don't generate it when failing read-ahead requests for similar reasons.
> 
> So remove this special casing and ignore all additional error handling for
> REQ_RAHEAD - if this was a real underlying error we'd get a normal read
> once the real read comes in.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  drivers/md/dm-raid1.c  | 4 ++--
>  drivers/md/dm-stripe.c | 2 +-
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/md/dm-raid1.c b/drivers/md/dm-raid1.c
> index a95cbb80fb34..5e30b08b91d9 100644
> --- a/drivers/md/dm-raid1.c
> +++ b/drivers/md/dm-raid1.c
> @@ -1214,7 +1214,7 @@ static int mirror_map(struct dm_target *ti, struct bio *bio)
>  	 */
>  	if (!r || (r == -EWOULDBLOCK)) {
>  		if (bio->bi_opf & REQ_RAHEAD)
> -			return -EWOULDBLOCK;
> +			return -EIO;
>  
>  		queue_bio(ms, bio, rw);
>  		return DM_MAPIO_SUBMITTED;
> @@ -1258,7 +1258,7 @@ static int mirror_end_io(struct dm_target *ti, struct bio *bio, int error)
>  	if (error == -EOPNOTSUPP)
>  		return error;
>  
> -	if ((error == -EWOULDBLOCK) && (bio->bi_opf & REQ_RAHEAD))
> +	if (bio->bi_opf & REQ_RAHEAD)
>  		return error;
>  
>  	if (unlikely(error)) {
> diff --git a/drivers/md/dm-stripe.c b/drivers/md/dm-stripe.c
> index 75152482f3ad..780e95889a7c 100644
> --- a/drivers/md/dm-stripe.c
> +++ b/drivers/md/dm-stripe.c
> @@ -384,7 +384,7 @@ static int stripe_end_io(struct dm_target *ti, struct bio *bio, int error)
>  	if (!error)
>  		return 0; /* I/O complete */
>  
> -	if ((error == -EWOULDBLOCK) && (bio->bi_opf & REQ_RAHEAD))
> +	if (bio->bi_opf & REQ_RAHEAD)
>  		return error;
>  
>  	if (error == -EOPNOTSUPP)
> -- 
> 2.11.0
> 

I'll let this one go for now.. meaning I won't pick it up, nor will I
send it for 4.12.  Please just roll this into your broader block work
that you mentioned.