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
