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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxx>
> Cc: Martin Wilck <mwilck@xxxxxxxx>
> Cc: Mikulas Patocka <mpatocka@xxxxxxxxxx>
> Cc: Hou Tao <houtao1@xxxxxxxxxx>
> Signed-off-by: Yufen Yu <yuyufen@xxxxxxxxxx>
> 
> ---
> 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

--
dm-devel mailing list
dm-devel@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/dm-devel



[Index of Archives]     [DM Crypt]     [Fedora Desktop]     [ATA RAID]     [Fedora Marketing]     [Fedora Packaging]     [Fedora SELinux]     [Yosemite Discussion]     [KDE Users]     [Fedora Docs]

  Powered by Linux