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

Hi,

=E5=9C=A8 2026/7/18 3:12, Nilay Shroff =E5=86=99=E9=81=93:
> 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.=C2=A0 NVMe fail=
over
>> can run from request completion context, and nvme_failover_req() also=20
>> holds
>> head->requeue_lock with interrupts disabled while it steals bios from=20
>> the
>> failed request.=C2=A0 Calling bio_set_dev() there is not safe once the=
=20
>> helper is
>> allowed to sleep.
>>
>> The requeue lock only protects head->requeue_list.=C2=A0 Keep the list
>> manipulation under that lock, but defer retargeting to=20
>> nvme_requeue_work(),
>> which already drains the list from process context before=20
>> resubmitting each
>> bio.=C2=A0 The bios remain private to the requeue list until the worker =
pops
>> them, so moving the device switch there preserves the existing retry=20
>> flow
>> while avoiding a sleepable helper in completion context.
>>
>> Signed-off-by: Yu Kuai <[email protected]>
>> ---
>> =C2=A0 drivers/nvme/host/multipath.c | 4 +---
>> =C2=A0 1 file changed, 1 insertion(+), 3 deletions(-)
>>
>> diff --git a/drivers/nvme/host/multipath.c=20
>> 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)
>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 struct nvme_ns *ns =3D req->q->queuedata;
>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 u16 status =3D nvme_req(req)->status & NV=
ME_SCT_SC_MASK;
>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 unsigned long flags;
>> -=C2=A0=C2=A0=C2=A0 struct bio *bio;
>> =C2=A0 =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 nvme_mpath_clear_current_path(ns);
>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 atomic_long_inc(&ns->failover);
>> @@ -165,8 +164,6 @@ void nvme_failover_req(struct request *req)
>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 }
>> =C2=A0 =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 spin_lock_irqsave(&ns->head->reque=
ue_lock, flags);
>> -=C2=A0=C2=A0=C2=A0 for (bio =3D req->bio; bio; bio =3D bio->bi_next)
>> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 bio_set_dev(bio, ns->head->d=
isk->part0);
>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 blk_steal_bios(&ns->head->requeue_list, r=
eq);
>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 spin_unlock_irqrestore(&ns->head->requeue=
_lock, flags);
>> =C2=A0 @@ -684,6 +681,7 @@ static void nvme_requeue_work(struct=20
>> work_struct *work)
>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 next =3D bio->bi_=
next;
>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 bio->bi_next =3D =
NULL;
>> =C2=A0 +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 bio_set_dev(bio, head=
->disk->part0);
>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 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
>
>
--=20
Thanks,
Kuai