[PATCH v2] dm mpath: fix missing call of path selector type->end_io

[PATCH v2] dm mpath: fix missing call of path selector type->end_io

[Yufen Yu]

After commit 396eaf21ee17 ("blk-mq: improve DM's blk-mq IO merging via
blk_insert_cloned_request feedback"), map_request() will requeue the tio
when issued clone request return BLK_STS_RESOURCE or BLK_STS_DEV_RESOURCE.

Thus, if device drive status is error, a tio may be requeued multiple times
until the return value is not DM_MAPIO_REQUEUE. That means type->start_io
may be called multiple tims, while type->end_io just be called when IO
complete.

In fact, even without the commit, setup_clone() fail also can make the
tio requeue and miss call type->end_io.

As servicer-time path selector for example, it selects path based on
in_flight_size, which is increased by st_start_io() and decreased by
st_end_io(). Missing call of end_io can lead to in_flight_size count
error and let the selector make the wrong choice. In addition,
queue-length path selector will also be affected.

To fix the problem, we call type->end_io in ->release_clone_rq before
tio requeue. It pass map_info to ->release_clone_rq() for requeue path,
and pass NULL for the others path.

Fixes: 396eaf21ee17 ("blk-mq: improve DM's blk-mq IO merging via blk_insert_cloned_request feedback")
Cc: Ming Lei <ming.lei@redhat.com>
Cc: Martin Wilck <mwilck@suse.com>
Cc: Mikulas Patocka <mpatocka@redhat.com>
Cc: Hou Tao <houtao1@huawei.com>
Signed-off-by: Yufen Yu <yuyufen@huawei.com>

---
V2:
 - remove the new added interface ->end_stat() in V1
 - use ->release_clone_rq() to call path selector ->end_io
 - add dm_rq_status to show clone status

V1:
 - https://www.redhat.com/archives/dm-devel/2019-April/msg00148.html
---
 drivers/md/dm-mpath.c         | 20 +++++++++++++++++++-
 drivers/md/dm-rq.c            | 19 ++++++++++++++-----
 drivers/md/dm-target.c        |  4 +++-
 include/linux/device-mapper.h | 13 ++++++++++++-
 4 files changed, 48 insertions(+), 8 deletions(-)

diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
index 2ee5e357a0a7..e2f6e9c9363f 100644
--- a/drivers/md/dm-mpath.c
+++ b/drivers/md/dm-mpath.c
@@ -544,8 +544,26 @@ static int multipath_clone_and_map(struct dm_target *ti, struct request *rq,
 	return DM_MAPIO_REMAPPED;
 }
 
-static void multipath_release_clone(struct request *clone)
+static void multipath_release_clone(struct request *clone,
+				union map_info *map_context
+				enum dm_rq_status clone_status)
 {
+	struct dm_mpath_io *mpio;
+	struct pgpath *pgpath;
+
+	if (clone_status != DM_CLONE_RQ_OK && !map_context) {
+		mpio = get_mpio(map_context);
+		pgpath = mpio->pgpath;
+
+		if (pgpath) {
+			struct path_selector *ps = &pgpath->pg->ps;
+
+			if (ps->type->end_io)
+				ps->type->end_io(ps, &pgpath->path,
+						mpio->nr_bytes);
+		}
+	}
+
 	blk_put_request(clone);
 }
 
diff --git a/drivers/md/dm-rq.c b/drivers/md/dm-rq.c
index b66745bd08bb..4cf309775d2c 100644
--- a/drivers/md/dm-rq.c
+++ b/drivers/md/dm-rq.c
@@ -168,7 +168,7 @@ static void dm_end_request(struct request *clone, blk_status_t error)
 	struct request *rq = tio->orig;
 
 	blk_rq_unprep_clone(clone);
-	tio->ti->type->release_clone_rq(clone);
+	tio->ti->type->release_clone_rq(clone, NULL, DM_CLONE_RQ_OK);
 
 	rq_end_stats(md, rq);
 	blk_mq_end_request(rq, error);
@@ -201,7 +201,7 @@ static void dm_requeue_original_request(struct dm_rq_target_io *tio, bool delay_
 	rq_end_stats(md, rq);
 	if (tio->clone) {
 		blk_rq_unprep_clone(tio->clone);
-		tio->ti->type->release_clone_rq(tio->clone);
+		tio->ti->type->release_clone_rq(tio->clone, NULL, DM_CLONE_RQ_OK);
 	}
 
 	dm_mq_delay_requeue_request(rq, delay_ms);
@@ -397,8 +397,12 @@ static int map_request(struct dm_rq_target_io *tio)
 		break;
 	case DM_MAPIO_REMAPPED:
 		if (setup_clone(clone, rq, tio, GFP_ATOMIC)) {
-			/* -ENOMEM */
-			ti->type->release_clone_rq(clone);
+			/* -ENOMEM
+			 * For multipath, before requeue, we need to pass tio->info
+			 * to release_clone_rq, avoiding missing call of path sector
+			 * ->end_io.
+			 */
+			ti->type->release_clone_rq(clone, &tio->info, DM_CLONE_RQ_CANCAL);
 			return DM_MAPIO_REQUEUE;
 		}
 
@@ -408,7 +412,12 @@ static int map_request(struct dm_rq_target_io *tio)
 		ret = dm_dispatch_clone_request(clone, rq);
 		if (ret == BLK_STS_RESOURCE || ret == BLK_STS_DEV_RESOURCE) {
 			blk_rq_unprep_clone(clone);
-			tio->ti->type->release_clone_rq(clone);
+			/*
+			 * For multipath, before requeue, we need to pass tio->info
+			 * to release_clone_rq, avoiding missing call of path sector
+			 * ->end_io.
+			 */
+			tio->ti->type->release_clone_rq(clone, &tio->info, DM_CLONE_RQ_CANCAL);
 			tio->clone = NULL;
 			return DM_MAPIO_REQUEUE;
 		}
diff --git a/drivers/md/dm-target.c b/drivers/md/dm-target.c
index 314d17ca6466..fd0962a24a54 100644
--- a/drivers/md/dm-target.c
+++ b/drivers/md/dm-target.c
@@ -136,7 +136,9 @@ static int io_err_clone_and_map_rq(struct dm_target *ti, struct request *rq,
 	return DM_MAPIO_KILL;
 }
 
-static void io_err_release_clone_rq(struct request *clone)
+static void io_err_release_clone_rq(struct request *clone,
+				union map_info *map_context,
+				enum dm_rq_status clone_status)
 {
 }
 
diff --git a/include/linux/device-mapper.h b/include/linux/device-mapper.h
index b0672756d056..9b9ab762a992 100644
--- a/include/linux/device-mapper.h
+++ b/include/linux/device-mapper.h
@@ -31,6 +31,15 @@ enum dm_queue_mode {
 	DM_TYPE_NVME_BIO_BASED	 = 4,
 };
 
+/*
+ * clone request status
+ */
+enum dm_rq_status {
+	DM_CLONE_RQ_OK,
+	DM_CLONE_RQ_CANCAL,
+	DM_CLONE_RQ_MAX,
+};
+
 typedef enum { STATUSTYPE_INFO, STATUSTYPE_TABLE } status_type_t;
 
 union map_info {
@@ -62,7 +71,9 @@ typedef int (*dm_clone_and_map_request_fn) (struct dm_target *ti,
 					    struct request *rq,
 					    union map_info *map_context,
 					    struct request **clone);
-typedef void (*dm_release_clone_request_fn) (struct request *clone);
+typedef void (*dm_release_clone_request_fn) (struct request *clone,
+						union map_info *map_context,
+						enum dm_rq_status clone_status);
 
 /*
  * Returns:
-- 
2.16.2.dirty

Re: [PATCH v2] dm mpath: fix missing call of path selector type->end_io

[Martin Wilck]

On Wed, 2019-04-24 at 23:19 +0800, Yufen Yu wrote:
> After commit 396eaf21ee17 ("blk-mq: improve DM's blk-mq IO merging
> via
> blk_insert_cloned_request feedback"), map_request() will requeue the
> tio
> when issued clone request return BLK_STS_RESOURCE or
> BLK_STS_DEV_RESOURCE.
> 
> Thus, if device drive status is error, a tio may be requeued multiple
> times
> until the return value is not DM_MAPIO_REQUEUE. That means type-
> >start_io
> may be called multiple tims, while type->end_io just be called when
> IO
> complete.
> 
> In fact, even without the commit, setup_clone() fail also can make
> the
> tio requeue and miss call type->end_io.
> 
> As servicer-time path selector for example, it selects path based on
> in_flight_size, which is increased by st_start_io() and decreased by
> st_end_io(). Missing call of end_io can lead to in_flight_size count
> error and let the selector make the wrong choice. In addition,
> queue-length path selector will also be affected.
> 
> To fix the problem, we call type->end_io in ->release_clone_rq before
> tio requeue. It pass map_info to ->release_clone_rq() for requeue
> path,
> and pass NULL for the others path.
> 
> Fixes: 396eaf21ee17 ("blk-mq: improve DM's blk-mq IO merging via
> blk_insert_cloned_request feedback")
> Cc: Ming Lei <ming.lei@redhat.com>
> Cc: Martin Wilck <mwilck@suse.com>
> Cc: Mikulas Patocka <mpatocka@redhat.com>
> Cc: Hou Tao <houtao1@huawei.com>
> Signed-off-by: Yufen Yu <yuyufen@huawei.com>
> 
> ---
> V2:
>  - remove the new added interface ->end_stat() in V1
>  - use ->release_clone_rq() to call path selector ->end_io
>  - add dm_rq_status to show clone status
> 
> V1:
>  - https://www.redhat.com/archives/dm-devel/2019-April/msg00148.html
> ---
>  drivers/md/dm-mpath.c         | 20 +++++++++++++++++++-
>  drivers/md/dm-rq.c            | 19 ++++++++++++++-----
>  drivers/md/dm-target.c        |  4 +++-
>  include/linux/device-mapper.h | 13 ++++++++++++-
>  4 files changed, 48 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
> index 2ee5e357a0a7..e2f6e9c9363f 100644
> --- a/drivers/md/dm-mpath.c
> +++ b/drivers/md/dm-mpath.c
> @@ -544,8 +544,26 @@ static int multipath_clone_and_map(struct
> dm_target *ti, struct request *rq,
>  	return DM_MAPIO_REMAPPED;
>  }
>  
> -static void multipath_release_clone(struct request *clone)
> +static void multipath_release_clone(struct request *clone,
> +				union map_info *map_context
> +				enum dm_rq_status clone_status)
>  {
> +	struct dm_mpath_io *mpio;
> +	struct pgpath *pgpath;
> +
> +	if (clone_status != DM_CLONE_RQ_OK && !map_context) {

Should this be "... && map_context)", maybe ?

> +		mpio = get_mpio(map_context);
> +		pgpath = mpio->pgpath;
> +
> +		if (pgpath) {
> +			struct path_selector *ps = &pgpath->pg->ps;
> +
> +			if (ps->type->end_io)
> +				ps->type->end_io(ps, &pgpath->path,
> +						mpio->nr_bytes);
> +		}
> +	}
> +
>  	blk_put_request(clone);
>  }
>  
> diff --git a/drivers/md/dm-rq.c b/drivers/md/dm-rq.c
> index b66745bd08bb..4cf309775d2c 100644
> --- a/drivers/md/dm-rq.c
> +++ b/drivers/md/dm-rq.c
> @@ -168,7 +168,7 @@ static void dm_end_request(struct request *clone,
> blk_status_t error)
>  	struct request *rq = tio->orig;
>  
>  	blk_rq_unprep_clone(clone);
> -	tio->ti->type->release_clone_rq(clone);
> +	tio->ti->type->release_clone_rq(clone, NULL, DM_CLONE_RQ_OK);
>  
>  	rq_end_stats(md, rq);
>  	blk_mq_end_request(rq, error);
> @@ -201,7 +201,7 @@ static void dm_requeue_original_request(struct
> dm_rq_target_io *tio, bool delay_
>  	rq_end_stats(md, rq);
>  	if (tio->clone) {
>  		blk_rq_unprep_clone(tio->clone);
> -		tio->ti->type->release_clone_rq(tio->clone);
> +		tio->ti->type->release_clone_rq(tio->clone, NULL,
> DM_CLONE_RQ_OK);
>  	}
>  
>  	dm_mq_delay_requeue_request(rq, delay_ms);
> @@ -397,8 +397,12 @@ static int map_request(struct dm_rq_target_io
> *tio)
>  		break;
>  	case DM_MAPIO_REMAPPED:
>  		if (setup_clone(clone, rq, tio, GFP_ATOMIC)) {
> -			/* -ENOMEM */
> -			ti->type->release_clone_rq(clone);
> +			/* -ENOMEM
> +			 * For multipath, before requeue, we need to
> pass tio->info
> +			 * to release_clone_rq, avoiding missing call
> of path sector
> +			 * ->end_io.
> +			 */
> +			ti->type->release_clone_rq(clone, &tio->info,
> DM_CLONE_RQ_CANCAL);
>  			return DM_MAPIO_REQUEUE;
>  		}
>  
> @@ -408,7 +412,12 @@ static int map_request(struct dm_rq_target_io
> *tio)
>  		ret = dm_dispatch_clone_request(clone, rq);
>  		if (ret == BLK_STS_RESOURCE || ret ==
> BLK_STS_DEV_RESOURCE) {
>  			blk_rq_unprep_clone(clone);
> -			tio->ti->type->release_clone_rq(clone);
> +			/*
> +			 * For multipath, before requeue, we need to
> pass tio->info
> +			 * to release_clone_rq, avoiding missing call
> of path sector
> +			 * ->end_io.
> +			 */
> +			tio->ti->type->release_clone_rq(clone, &tio-
> >info, DM_CLONE_RQ_CANCAL);
>  			tio->clone = NULL;
>  			return DM_MAPIO_REQUEUE;
>  		}
> diff --git a/drivers/md/dm-target.c b/drivers/md/dm-target.c
> index 314d17ca6466..fd0962a24a54 100644
> --- a/drivers/md/dm-target.c
> +++ b/drivers/md/dm-target.c
> @@ -136,7 +136,9 @@ static int io_err_clone_and_map_rq(struct
> dm_target *ti, struct request *rq,
>  	return DM_MAPIO_KILL;
>  }
>  
> -static void io_err_release_clone_rq(struct request *clone)
> +static void io_err_release_clone_rq(struct request *clone,
> +				union map_info *map_context,
> +				enum dm_rq_status clone_status)
>  {
>  }
>  
> diff --git a/include/linux/device-mapper.h b/include/linux/device-
> mapper.h
> index b0672756d056..9b9ab762a992 100644
> --- a/include/linux/device-mapper.h
> +++ b/include/linux/device-mapper.h
> @@ -31,6 +31,15 @@ enum dm_queue_mode {
>  	DM_TYPE_NVME_BIO_BASED	 = 4,
>  };
>  
> +/*
> + * clone request status
> + */
> +enum dm_rq_status {
> +	DM_CLONE_RQ_OK,
> +	DM_CLONE_RQ_CANCAL,
> +	DM_CLONE_RQ_MAX,
> +};

"CANCAL" looks strange - did you mean "CANCEL"?

Anyway, what do you need this new enum for? Couldn't you just pass the 
disposition (i.e. DM_MAPIO_REQUEUE), and use a different value (e.g.
DM_ENDIO_DONE) at those call sites where end_io shouldn't be called?

Regards,
Martin

Re: [PATCH v2] dm mpath: fix missing call of path selector type->end_io

[yuyufen]

On 2019/4/24 23:38, Martin Wilck wrote:
> On Wed, 2019-04-24 at 23:19 +0800, Yufen Yu wrote:
>> After commit 396eaf21ee17 ("blk-mq: improve DM's blk-mq IO merging
>> via
>> blk_insert_cloned_request feedback"), map_request() will requeue the
>> tio
>> when issued clone request return BLK_STS_RESOURCE or
>> BLK_STS_DEV_RESOURCE.
>>
>> Thus, if device drive status is error, a tio may be requeued multiple
>> times
>> until the return value is not DM_MAPIO_REQUEUE. That means type-
>>> start_io
>> may be called multiple tims, while type->end_io just be called when
>> IO
>> complete.
>>
>> In fact, even without the commit, setup_clone() fail also can make
>> the
>> tio requeue and miss call type->end_io.
>>
>> As servicer-time path selector for example, it selects path based on
>> in_flight_size, which is increased by st_start_io() and decreased by
>> st_end_io(). Missing call of end_io can lead to in_flight_size count
>> error and let the selector make the wrong choice. In addition,
>> queue-length path selector will also be affected.
>>
>> To fix the problem, we call type->end_io in ->release_clone_rq before
>> tio requeue. It pass map_info to ->release_clone_rq() for requeue
>> path,
>> and pass NULL for the others path.
>>
>> Fixes: 396eaf21ee17 ("blk-mq: improve DM's blk-mq IO merging via
>> blk_insert_cloned_request feedback")
>> Cc: Ming Lei <ming.lei@redhat.com>
>> Cc: Martin Wilck <mwilck@suse.com>
>> Cc: Mikulas Patocka <mpatocka@redhat.com>
>> Cc: Hou Tao <houtao1@huawei.com>
>> Signed-off-by: Yufen Yu <yuyufen@huawei.com>
>>
>> ---
>> V2:
>>   - remove the new added interface ->end_stat() in V1
>>   - use ->release_clone_rq() to call path selector ->end_io
>>   - add dm_rq_status to show clone status
>>
>> V1:
>>   - https://www.redhat.com/archives/dm-devel/2019-April/msg00148.html
>> ---
>>   drivers/md/dm-mpath.c         | 20 +++++++++++++++++++-
>>   drivers/md/dm-rq.c            | 19 ++++++++++++++-----
>>   drivers/md/dm-target.c        |  4 +++-
>>   include/linux/device-mapper.h | 13 ++++++++++++-
>>   4 files changed, 48 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
>> index 2ee5e357a0a7..e2f6e9c9363f 100644
>> --- a/drivers/md/dm-mpath.c
>> +++ b/drivers/md/dm-mpath.c
>> @@ -544,8 +544,26 @@ static int multipath_clone_and_map(struct
>> dm_target *ti, struct request *rq,
>>   	return DM_MAPIO_REMAPPED;
>>   }
>>   
>> -static void multipath_release_clone(struct request *clone)
>> +static void multipath_release_clone(struct request *clone,
>> +				union map_info *map_context
>> +				enum dm_rq_status clone_status)
>>   {
>> +	struct dm_mpath_io *mpio;
>> +	struct pgpath *pgpath;
>> +
>> +	if (clone_status != DM_CLONE_RQ_OK && !map_context) {
> Should this be "... && map_context)", maybe ?

Yes, thanks for catching this! This is an obvious error.

>
>> +		mpio = get_mpio(map_context);
>> +		pgpath = mpio->pgpath;
>> +
>> +		if (pgpath) {
>> +			struct path_selector *ps = &pgpath->pg->ps;
>> +
>> +			if (ps->type->end_io)
>> +				ps->type->end_io(ps, &pgpath->path,
>> +						mpio->nr_bytes);
>> +		}
>> +	}
>> +
>>   	blk_put_request(clone);
>>   }
>>   
>> diff --git a/drivers/md/dm-rq.c b/drivers/md/dm-rq.c
>> index b66745bd08bb..4cf309775d2c 100644
>> --- a/drivers/md/dm-rq.c
>> +++ b/drivers/md/dm-rq.c
>> @@ -168,7 +168,7 @@ static void dm_end_request(struct request *clone,
>> blk_status_t error)
>>   	struct request *rq = tio->orig;
>>   
>>   	blk_rq_unprep_clone(clone);
>> -	tio->ti->type->release_clone_rq(clone);
>> +	tio->ti->type->release_clone_rq(clone, NULL, DM_CLONE_RQ_OK);
>>   
>>   	rq_end_stats(md, rq);
>>   	blk_mq_end_request(rq, error);
>> @@ -201,7 +201,7 @@ static void dm_requeue_original_request(struct
>> dm_rq_target_io *tio, bool delay_
>>   	rq_end_stats(md, rq);
>>   	if (tio->clone) {
>>   		blk_rq_unprep_clone(tio->clone);
>> -		tio->ti->type->release_clone_rq(tio->clone);
>> +		tio->ti->type->release_clone_rq(tio->clone, NULL,
>> DM_CLONE_RQ_OK);
>>   	}
>>   
>>   	dm_mq_delay_requeue_request(rq, delay_ms);
>> @@ -397,8 +397,12 @@ static int map_request(struct dm_rq_target_io
>> *tio)
>>   		break;
>>   	case DM_MAPIO_REMAPPED:
>>   		if (setup_clone(clone, rq, tio, GFP_ATOMIC)) {
>> -			/* -ENOMEM */
>> -			ti->type->release_clone_rq(clone);
>> +			/* -ENOMEM
>> +			 * For multipath, before requeue, we need to
>> pass tio->info
>> +			 * to release_clone_rq, avoiding missing call
>> of path sector
>> +			 * ->end_io.
>> +			 */
>> +			ti->type->release_clone_rq(clone, &tio->info,
>> DM_CLONE_RQ_CANCAL);
>>   			return DM_MAPIO_REQUEUE;
>>   		}
>>   
>> @@ -408,7 +412,12 @@ static int map_request(struct dm_rq_target_io
>> *tio)
>>   		ret = dm_dispatch_clone_request(clone, rq);
>>   		if (ret == BLK_STS_RESOURCE || ret ==
>> BLK_STS_DEV_RESOURCE) {
>>   			blk_rq_unprep_clone(clone);
>> -			tio->ti->type->release_clone_rq(clone);
>> +			/*
>> +			 * For multipath, before requeue, we need to
>> pass tio->info
>> +			 * to release_clone_rq, avoiding missing call
>> of path sector
>> +			 * ->end_io.
>> +			 */
>> +			tio->ti->type->release_clone_rq(clone, &tio-
>>> info, DM_CLONE_RQ_CANCAL);
>>   			tio->clone = NULL;
>>   			return DM_MAPIO_REQUEUE;
>>   		}
>> diff --git a/drivers/md/dm-target.c b/drivers/md/dm-target.c
>> index 314d17ca6466..fd0962a24a54 100644
>> --- a/drivers/md/dm-target.c
>> +++ b/drivers/md/dm-target.c
>> @@ -136,7 +136,9 @@ static int io_err_clone_and_map_rq(struct
>> dm_target *ti, struct request *rq,
>>   	return DM_MAPIO_KILL;
>>   }
>>   
>> -static void io_err_release_clone_rq(struct request *clone)
>> +static void io_err_release_clone_rq(struct request *clone,
>> +				union map_info *map_context,
>> +				enum dm_rq_status clone_status)
>>   {
>>   }
>>   
>> diff --git a/include/linux/device-mapper.h b/include/linux/device-
>> mapper.h
>> index b0672756d056..9b9ab762a992 100644
>> --- a/include/linux/device-mapper.h
>> +++ b/include/linux/device-mapper.h
>> @@ -31,6 +31,15 @@ enum dm_queue_mode {
>>   	DM_TYPE_NVME_BIO_BASED	 = 4,
>>   };
>>   
>> +/*
>> + * clone request status
>> + */
>> +enum dm_rq_status {
>> +	DM_CLONE_RQ_OK,
>> +	DM_CLONE_RQ_CANCAL,
>> +	DM_CLONE_RQ_MAX,
>> +};
> "CANCAL" looks strange - did you mean "CANCEL"?

This is a spelling error.

>
> Anyway, what do you need this new enum for? Couldn't you just pass the
> disposition (i.e. DM_MAPIO_REQUEUE), and use a different value (e.g.
> DM_ENDIO_DONE) at those call sites where end_io shouldn't be called?
>
> Regards,
> Martin
>
>
> .
>

Thanks for your suggestion. Passing DM_MAPIO_REQUEUE is a good idea.
It indicates that request will be requeued and we need to call ->end_io.

However, I am not sure if it is suitable for use DM_ENDIO_DONE.
It is strange that we use DM_ENDIO_DONE in dm_requeue_original_request(),
which is called in the case of DM_ENDIO_REQUEUE in dm_done().

Yufen
Thanks a lot.

Re: [PATCH v2] dm mpath: fix missing call of path selector type->end_io

[Mike Snitzer]

On Thu, Apr 25 2019 at  5:25am -0400,
yuyufen <yuyufen@huawei.com> wrote:

> 
> 
> On 2019/4/24 23:38, Martin Wilck wrote:
> >On Wed, 2019-04-24 at 23:19 +0800, Yufen Yu wrote:
> >>After commit 396eaf21ee17 ("blk-mq: improve DM's blk-mq IO merging
> >>via
> >>blk_insert_cloned_request feedback"), map_request() will requeue the
> >>tio
> >>when issued clone request return BLK_STS_RESOURCE or
> >>BLK_STS_DEV_RESOURCE.
> >>
> >>Thus, if device drive status is error, a tio may be requeued multiple
> >>times
> >>until the return value is not DM_MAPIO_REQUEUE. That means type-
> >>>start_io
> >>may be called multiple tims, while type->end_io just be called when
> >>IO
> >>complete.
> >>
> >>In fact, even without the commit, setup_clone() fail also can make
> >>the
> >>tio requeue and miss call type->end_io.
> >>
> >>As servicer-time path selector for example, it selects path based on
> >>in_flight_size, which is increased by st_start_io() and decreased by
> >>st_end_io(). Missing call of end_io can lead to in_flight_size count
> >>error and let the selector make the wrong choice. In addition,
> >>queue-length path selector will also be affected.
> >>
> >>To fix the problem, we call type->end_io in ->release_clone_rq before
> >>tio requeue. It pass map_info to ->release_clone_rq() for requeue
> >>path,
> >>and pass NULL for the others path.
> >>
> >>Fixes: 396eaf21ee17 ("blk-mq: improve DM's blk-mq IO merging via
> >>blk_insert_cloned_request feedback")
> >>Cc: Ming Lei <ming.lei@redhat.com>
> >>Cc: Martin Wilck <mwilck@suse.com>
> >>Cc: Mikulas Patocka <mpatocka@redhat.com>
> >>Cc: Hou Tao <houtao1@huawei.com>
> >>Signed-off-by: Yufen Yu <yuyufen@huawei.com>
> >>
> >>---
> >>V2:
> >>  - remove the new added interface ->end_stat() in V1
> >>  - use ->release_clone_rq() to call path selector ->end_io
> >>  - add dm_rq_status to show clone status
> >>
> >>V1:
> >>  - https://www.redhat.com/archives/dm-devel/2019-April/msg00148.html
> >>---
> >>  drivers/md/dm-mpath.c         | 20 +++++++++++++++++++-
> >>  drivers/md/dm-rq.c            | 19 ++++++++++++++-----
> >>  drivers/md/dm-target.c        |  4 +++-
> >>  include/linux/device-mapper.h | 13 ++++++++++++-
> >>  4 files changed, 48 insertions(+), 8 deletions(-)
> >>
> >>diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
> >>index 2ee5e357a0a7..e2f6e9c9363f 100644
> >>--- a/drivers/md/dm-mpath.c
> >>+++ b/drivers/md/dm-mpath.c
> >>@@ -544,8 +544,26 @@ static int multipath_clone_and_map(struct
> >>dm_target *ti, struct request *rq,
> >>  	return DM_MAPIO_REMAPPED;
> >>  }
> >>-static void multipath_release_clone(struct request *clone)
> >>+static void multipath_release_clone(struct request *clone,
> >>+				union map_info *map_context
> >>+				enum dm_rq_status clone_status)
> >>  {
> >>+	struct dm_mpath_io *mpio;
> >>+	struct pgpath *pgpath;
> >>+
> >>+	if (clone_status != DM_CLONE_RQ_OK && !map_context) {
> >Should this be "... && map_context)", maybe ?
> 
> Yes, thanks for catching this! This is an obvious error.
> 
> >
> >>+		mpio = get_mpio(map_context);
> >>+		pgpath = mpio->pgpath;
> >>+
> >>+		if (pgpath) {
> >>+			struct path_selector *ps = &pgpath->pg->ps;
> >>+
> >>+			if (ps->type->end_io)
> >>+				ps->type->end_io(ps, &pgpath->path,
> >>+						mpio->nr_bytes);
> >>+		}
> >>+	}
> >>+
> >>  	blk_put_request(clone);
> >>  }
> >>diff --git a/drivers/md/dm-rq.c b/drivers/md/dm-rq.c
> >>index b66745bd08bb..4cf309775d2c 100644
> >>--- a/drivers/md/dm-rq.c
> >>+++ b/drivers/md/dm-rq.c
> >>@@ -168,7 +168,7 @@ static void dm_end_request(struct request *clone,
> >>blk_status_t error)
> >>  	struct request *rq = tio->orig;
> >>  	blk_rq_unprep_clone(clone);
> >>-	tio->ti->type->release_clone_rq(clone);
> >>+	tio->ti->type->release_clone_rq(clone, NULL, DM_CLONE_RQ_OK);
> >>  	rq_end_stats(md, rq);
> >>  	blk_mq_end_request(rq, error);
> >>@@ -201,7 +201,7 @@ static void dm_requeue_original_request(struct
> >>dm_rq_target_io *tio, bool delay_
> >>  	rq_end_stats(md, rq);
> >>  	if (tio->clone) {
> >>  		blk_rq_unprep_clone(tio->clone);
> >>-		tio->ti->type->release_clone_rq(tio->clone);
> >>+		tio->ti->type->release_clone_rq(tio->clone, NULL,
> >>DM_CLONE_RQ_OK);
> >>  	}
> >>  	dm_mq_delay_requeue_request(rq, delay_ms);
> >>@@ -397,8 +397,12 @@ static int map_request(struct dm_rq_target_io
> >>*tio)
> >>  		break;
> >>  	case DM_MAPIO_REMAPPED:
> >>  		if (setup_clone(clone, rq, tio, GFP_ATOMIC)) {
> >>-			/* -ENOMEM */
> >>-			ti->type->release_clone_rq(clone);
> >>+			/* -ENOMEM
> >>+			 * For multipath, before requeue, we need to
> >>pass tio->info
> >>+			 * to release_clone_rq, avoiding missing call
> >>of path sector
> >>+			 * ->end_io.
> >>+			 */
> >>+			ti->type->release_clone_rq(clone, &tio->info,
> >>DM_CLONE_RQ_CANCAL);
> >>  			return DM_MAPIO_REQUEUE;
> >>  		}
> >>@@ -408,7 +412,12 @@ static int map_request(struct dm_rq_target_io
> >>*tio)
> >>  		ret = dm_dispatch_clone_request(clone, rq);
> >>  		if (ret == BLK_STS_RESOURCE || ret ==
> >>BLK_STS_DEV_RESOURCE) {
> >>  			blk_rq_unprep_clone(clone);
> >>-			tio->ti->type->release_clone_rq(clone);
> >>+			/*
> >>+			 * For multipath, before requeue, we need to
> >>pass tio->info
> >>+			 * to release_clone_rq, avoiding missing call
> >>of path sector
> >>+			 * ->end_io.
> >>+			 */
> >>+			tio->ti->type->release_clone_rq(clone, &tio-
> >>>info, DM_CLONE_RQ_CANCAL);
> >>  			tio->clone = NULL;
> >>  			return DM_MAPIO_REQUEUE;
> >>  		}
> >>diff --git a/drivers/md/dm-target.c b/drivers/md/dm-target.c
> >>index 314d17ca6466..fd0962a24a54 100644
> >>--- a/drivers/md/dm-target.c
> >>+++ b/drivers/md/dm-target.c
> >>@@ -136,7 +136,9 @@ static int io_err_clone_and_map_rq(struct
> >>dm_target *ti, struct request *rq,
> >>  	return DM_MAPIO_KILL;
> >>  }
> >>-static void io_err_release_clone_rq(struct request *clone)
> >>+static void io_err_release_clone_rq(struct request *clone,
> >>+				union map_info *map_context,
> >>+				enum dm_rq_status clone_status)
> >>  {
> >>  }
> >>diff --git a/include/linux/device-mapper.h b/include/linux/device-
> >>mapper.h
> >>index b0672756d056..9b9ab762a992 100644
> >>--- a/include/linux/device-mapper.h
> >>+++ b/include/linux/device-mapper.h
> >>@@ -31,6 +31,15 @@ enum dm_queue_mode {
> >>  	DM_TYPE_NVME_BIO_BASED	 = 4,
> >>  };
> >>+/*
> >>+ * clone request status
> >>+ */
> >>+enum dm_rq_status {
> >>+	DM_CLONE_RQ_OK,
> >>+	DM_CLONE_RQ_CANCAL,
> >>+	DM_CLONE_RQ_MAX,
> >>+};
> >"CANCAL" looks strange - did you mean "CANCEL"?
> 
> This is a spelling error.
> 
> >
> >Anyway, what do you need this new enum for? Couldn't you just pass the
> >disposition (i.e. DM_MAPIO_REQUEUE), and use a different value (e.g.
> >DM_ENDIO_DONE) at those call sites where end_io shouldn't be called?
> >
> >Regards,
> >Martin
> >
> >
> >.
> >
> 
> Thanks for your suggestion. Passing DM_MAPIO_REQUEUE is a good idea.
> It indicates that request will be requeued and we need to call ->end_io.
> 
> However, I am not sure if it is suitable for use DM_ENDIO_DONE.
> It is strange that we use DM_ENDIO_DONE in dm_requeue_original_request(),
> which is called in the case of DM_ENDIO_REQUEUE in dm_done().


I've staged the following, it doesn't get bogged down with passing flags
(and overloading their use with unnatural new meanings).  Please let me
know if you see any issues:

https://git.kernel.org/pub/scm/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=dm-5.2&id=5de719e3d01b4abe0de0d7b857148a880ff2a90b

Thanks,
Mike

Re: [PATCH v2] dm mpath: fix missing call of path selector type->end_io

[Martin Wilck]

On Thu, 2019-04-25 at 15:45 -0400, Mike Snitzer wrote:
> On Thu, Apr 25 2019 at  5:25am -0400,
> yuyufen <yuyufen@huawei.com> wrote:
> 
> > > "CANCAL" looks strange - did you mean "CANCEL"?
> > 
> > This is a spelling error.
> > 
> > > Anyway, what do you need this new enum for? Couldn't you just
> > > pass the
> > > disposition (i.e. DM_MAPIO_REQUEUE), and use a different value
> > > (e.g.
> > > DM_ENDIO_DONE) at those call sites where end_io shouldn't be
> > > called?
> > > 
> > > Regards,
> > > Martin
> > > 
> > > 
> > > .
> > > 
> > 
> > Thanks for your suggestion. Passing DM_MAPIO_REQUEUE is a good
> > idea.
> > It indicates that request will be requeued and we need to call
> > ->end_io.
> > 
> > However, I am not sure if it is suitable for use DM_ENDIO_DONE.
> > It is strange that we use DM_ENDIO_DONE in
> > dm_requeue_original_request(),
> > which is called in the case of DM_ENDIO_REQUEUE in dm_done().
> 
> I've staged the following, it doesn't get bogged down with passing
> flags
> (and overloading their use with unnatural new meanings).  Please let
> me
> know if you see any issues:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=dm-5.2&id=5de719e3d01b4abe0de0d7b857148a880ff2a90b

Nice and simple. Looks good to me.

Reviewed-by: Martin Wilck <mwilck@suse.com>