Re: [PATCH v2 1/3] nvme-multipath: implement service-time iopolicy

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] nvme-multipath: in_flight_size leak in nvme_mpath_end_request() on failover
- [Medium] nvme-multipath: in_flight_size underflow wraps in nvme_st_compare_load()
--

commit 211891d7cca0785689865f3a7650f5305abaef90
Author: Guixin Liu <kanie@linux.alibaba.com>

nvme-multipath: implement service-time iopolicy

This commit adds a service-time multipath I/O policy that selects paths
based on the ratio of in-flight bytes to a configurable relative throughput
weight, exposing in_flight_bytes and relative_throughput sysfs attributes.

> diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
> index 75dbb58286a32..3901654d8226b 100644
> --- a/drivers/nvme/host/multipath.c
> +++ b/drivers/nvme/host/multipath.c
[ ... ]
> @@ -179,11 +180,16 @@ void nvme_mpath_start_request(struct request *rq)
>  {
>  	struct nvme_ns *ns = rq->q->queuedata;
>  	struct gendisk *disk = ns->head->disk;
> +	int iopolicy = READ_ONCE(ns->head->subsys->iopolicy);
>  
> -	if ((READ_ONCE(ns->head->subsys->iopolicy) == NVME_IOPOLICY_QD) &&
> +	if (iopolicy == NVME_IOPOLICY_QD &&
>  	    !(nvme_req(rq)->flags & NVME_MPATH_CNT_ACTIVE)) {
>  		atomic_inc(&ns->ctrl->nr_active);
>  		nvme_req(rq)->flags |= NVME_MPATH_CNT_ACTIVE;
> +	} else if (iopolicy == NVME_IOPOLICY_ST &&
> +		   !(nvme_req(rq)->flags & NVME_MPATH_CNT_ACTIVE_BYTES)) {
> +		atomic64_add(blk_rq_bytes(rq), &ns->ctrl->in_flight_size);
> +		nvme_req(rq)->flags |= NVME_MPATH_CNT_ACTIVE_BYTES;
>  	}
>  
>  	if (!blk_queue_io_stat(disk->queue) ||
> @@ -205,6 +211,10 @@ void nvme_mpath_end_request(struct request *rq)
>  
>  	if (nvme_req(rq)->flags & NVME_MPATH_CNT_ACTIVE)
>  		atomic_dec_if_positive(&ns->ctrl->nr_active);
> +	if (nvme_req(rq)->flags & NVME_MPATH_CNT_ACTIVE_BYTES) {
> +		atomic64_sub(blk_rq_bytes(rq), &ns->ctrl->in_flight_size);
> +		nvme_req(rq)->flags &= ~NVME_MPATH_CNT_ACTIVE_BYTES;
> +	}

[Severity: Medium]
Can this leak bytes in ns->ctrl->in_flight_size when a request fails over to
another path?

When a request fails with a path error (such as NVME_SC_ANA_TRANSITION,
NVME_SC_ANA_INACCESSIBLE, NVME_SC_ANA_PERSISTENT_LOSS,
NVME_SC_HOST_PATH_ERROR, or NVME_SC_HOST_ABORTED_CMD) or on a dying queue,
__nvme_complete_rq() calls nvme_failover_req():

drivers/nvme/host/multipath.c:nvme_failover_req() {
	...
	spin_lock_irqsave(&ns->head->requeue_lock, flags);
	for (bio = req->bio; bio; bio = bio->bi_next)
		bio_set_dev(bio, ns->head->disk->part0);
	blk_steal_bios(&ns->head->requeue_list, req);
	spin_unlock_irqrestore(&ns->head->requeue_lock, flags);

	nvme_req(req)->status = 0;
	nvme_end_req(req);
}

blk_steal_bios() detaches all bios from the request and sets
req->__data_len = 0 before nvme_end_req() -> __nvme_end_req() ->
nvme_mpath_end_request() is called:

block/blk-mq.c:blk_steal_bios() {
	...
	rq->bio = NULL;
	rq->biotail = NULL;
	rq->__data_len = 0;
}

When nvme_mpath_end_request() subsequently calls blk_rq_bytes(rq), it reads
rq->__data_len as 0 and subtracts 0 from ns->ctrl->in_flight_size while
clearing NVME_MPATH_CNT_ACTIVE_BYTES. Whenever failover occurs without a
controller reset (for example during an ANA state transition, or on a
multipath controller without NVME_CTRL_CMIC_ANA where
nvme_mpath_init_identify() returns early), those bytes remain permanently
counted in ns->ctrl->in_flight_size, skewing path selection in
nvme_service_time_path() and leaving in_flight_bytes non-zero when idle.

>  
>  	if (!(nvme_req(rq)->flags & NVME_MPATH_IO_STATS))
>  		return;
> @@ -464,6 +474,109 @@ static struct nvme_ns *nvme_queue_depth_path(struct nvme_ns_head *head)
>  	return best_opt ? best_opt : best_nonopt;
>  }
>  
> +#define ST_MAX_RELATIVE_THROUGHPUT	100
> +#define ST_MAX_RELATIVE_THROUGHPUT_SHIFT	7
> +#define ST_MAX_INFLIGHT_SIZE \
> +	((size_t)-1 >> ST_MAX_RELATIVE_THROUGHPUT_SHIFT)
> +
> +/*
> + * Compare estimated service time of two paths.
> + * Returns negative if ns1 is better, positive if ns2 is better, 0 if equal.
> + *
> + * Service time = in_flight_size / relative_throughput
> + * Cross-multiply to avoid division.
> + */
> +static int nvme_st_compare_load(struct nvme_ns *ns1, struct nvme_ns *ns2)
> +{
> +	size_t sz1, sz2, st1, st2;
> +	unsigned int tp1 = ns1->ctrl->relative_throughput;
> +	unsigned int tp2 = ns2->ctrl->relative_throughput;
> +
> +	sz1 = atomic64_read(&ns1->ctrl->in_flight_size);
> +	sz2 = atomic64_read(&ns2->ctrl->in_flight_size);

[Severity: Medium]
What happens here if ns->ctrl->in_flight_size underflows below zero after a
controller reset?

When a request is started under NVME_IOPOLICY_ST, nvme_setup_cmd() calls
nvme_clear_nvme_request() (which sets RQF_DONTPREP), and
nvme_mpath_start_request() adds blk_rq_bytes(rq) to ns->ctrl->in_flight_size
and sets NVME_MPATH_CNT_ACTIVE_BYTES.

If that request is then placed onto q->requeue_list in MQ_RQ_IDLE state
without calling nvme_mpath_end_request() (either via nvme_retry_req() ->
blk_mq_requeue_request(), or when a transport queue_rq callback such as
nvme_rdma_queue_rq(), nvme_fc_start_fcp_op(), or nvme_loop_queue_rq()
returns BLK_STS_RESOURCE after nvme_start_request()) and a controller reset
occurs:

1. nvme_cancel_tagset() -> nvme_cancel_request() skips the request because
   blk_mq_rq_state(req) != MQ_RQ_IN_FLIGHT.
2. nvme_mpath_init_identify() resets ctrl->in_flight_size to 0 via
   atomic64_set(&ctrl->in_flight_size, 0).
3. When the queue is unquiesced and the request is re-dispatched,
   nvme_setup_cmd() preserves nvme_req(req)->flags because RQF_DONTPREP is
   set, and nvme_mpath_start_request() skips adding blk_rq_bytes(rq) because
   NVME_MPATH_CNT_ACTIVE_BYTES is already set.
4. When the request completes, nvme_mpath_end_request() unconditionally
   executes atomic64_sub(blk_rq_bytes(rq), &ns->ctrl->in_flight_size),
   driving ctrl->in_flight_size negative (unlike ctrl->nr_active, which uses
   atomic_dec_if_positive()).

In nvme_st_compare_load(), the negative s64 returned by atomic64_read() is
assigned to the unsigned size_t variables sz1 and sz2, wrapping to a large
positive value near SIZE_MAX and causing nvme_service_time_path() to treat
the path as heavily overloaded until enough new I/O leaks or resets it.
Should in_flight_size be prevented from going negative or clamped when read?

[ ... ]
> @@ -1453,6 +1610,7 @@ int nvme_mpath_init_identify(struct nvme_ctrl *ctrl, struct nvme_id_ctrl *id)
>  
>  	/* initialize this in the identify path to cover controller resets */
>  	atomic_set(&ctrl->nr_active, 0);
> +	atomic64_set(&ctrl->in_flight_size, 0);
>  
>  	if (!ctrl->max_namespaces ||
>  	    ctrl->max_namespaces > le32_to_cpu(id->nn)) {
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019075607.808947-1-kanie@linux.alibaba.com?part=1