Re: [RFC PATCH v1 01/17] nvme-multipath: retarget failedover bios from requeue work

On 7/5/26 1:21 AM, Yu Kuai wrote:
> From: Yu Kuai <[email protected]>
> 
> bio_set_dev() is about to become explicitly sleepable because it can
> associate the bio with a blkg for the destination queue.  NVMe failover
> can run from request completion context, and nvme_failover_req() also holds
> head->requeue_lock with interrupts disabled while it steals bios from the
> failed request.  Calling bio_set_dev() there is not safe once the helper is
> allowed to sleep.
> 
> The requeue lock only protects head->requeue_list.  Keep the list
> manipulation under that lock, but defer retargeting to nvme_requeue_work(),
> which already drains the list from process context before resubmitting each
> bio.  The bios remain private to the requeue list until the worker pops
> them, so moving the device switch there preserves the existing retry flow
> while avoiding a sleepable helper in completion context.
> 
> Signed-off-by: Yu Kuai <[email protected]>
> ---
>   drivers/nvme/host/multipath.c | 4 +---
>   1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
> index 9b9a657fa330..76baa180ae1c 100644
> --- a/drivers/nvme/host/multipath.c
> +++ b/drivers/nvme/host/multipath.c
> @@ -149,7 +149,6 @@ void nvme_failover_req(struct request *req)
>   	struct nvme_ns *ns = req->q->queuedata;
>   	u16 status = nvme_req(req)->status & NVME_SCT_SC_MASK;
>   	unsigned long flags;
> -	struct bio *bio;
>   
>   	nvme_mpath_clear_current_path(ns);
>   	atomic_long_inc(&ns->failover);
> @@ -165,8 +164,6 @@ void nvme_failover_req(struct request *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);
>   
> @@ -684,6 +681,7 @@ static void nvme_requeue_work(struct work_struct *work)
>   		next = bio->bi_next;
>   		bio->bi_next = NULL;
>   
> +		bio_set_dev(bio, head->disk->part0);
>   		submit_bio_noacct(bio);

What happens if bio_set_dev() fails to associate a blkg? From what
I understand, bio_associate_blkg() may fail, leaving bio->bi_blkg
set to NULL. Later, submit_bio_noacct() can invoke blkcg-related
helpers such as blk_should_throtl(), which expect a valid bio->bi_blkg.
However if bio->bi_blkg is NULL then accessing it without NULL check
could crash the kernel. This is probably not a bug introduced with your
changes, but you may want to check it.

The question is, is bio_associate_blkg() guaranteed never to fail, or
should the failure be handled explicitly before the bio is resubmitted?

I also skimmed through the rest of the series. However, as Christoph
mentioned in an earlier thread, we may be moving away from non-blocking
blkg allocation altogether. If that's the direction we're taking, this
series will likely need to be reworked. I'd therefore prefer to wait for
the next revision before reviewing the other patches.

Thanks,
--Nilay