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

Hi,

在 2026/7/18 3:12, Nilay Shroff 写道:
> 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.

bio_set_dev() will not leave bio->bi_blkg set to NULL. blkg_lookup_create()
will iterate closest blkg start from root_blkg, if any blkg is missing then
create, and if creating failed, current closest blkg is returned.

The same iteration exist in blkcg configuration, where failure is returned if
blkg creation failed.

>
> 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
>
>
-- 
Thanks,
Kuai