Re: [PATCH v3 04/13] libmultipath: Add bio handling

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] TOCTOU race in `mpath_remove_disk` can strand a bio on `requeue_list`, causing `del_gendisk` to deadlock.
- [Medium] The `trace_block_bio_remap` tracepoint is called with the wrong device argument for `old_dev`, causing the trace event to emit identical devices for source and destination.
--

commit 1f99dbf217347b66f1710f734eac617209a4e8c6
Author: John Garry <john.g.garry@oracle.com>

libmultipath: Add bio handling

This commit adds per-path bio submission support and failover requeuing
logic for libmultipath. It introduces mechanisms similar to NVMe multipath
to clone bios and submit them with the REQ_MPATH flag.

> diff --git a/lib/multipath.c b/lib/multipath.c
> index 21f7ffdb22d60..81e737c1ce469 100644
> --- a/lib/multipath.c
> +++ b/lib/multipath.c

[ ... ]

> @@ -243,6 +242,81 @@ static struct mpath_device *mpath_find_path(struct mpath_head *mpath_head)
> +static void mpath_bdev_submit_bio(struct bio *bio)
> +{

[ ... ]

> +		bio_set_dev(bio, mpath_device->disk->part0);
> +		/*
> +		 * Use BIO_REMAPPED to skip bio_check_eod() when this bio
> +		 * enters submit_bio_noacct() for the per-path device. The EOD
> +		 * check already passed on the multipath head.
> +		 */
> +		bio_set_flag(bio, BIO_REMAPPED);
> +		bio->bi_opf |= REQ_MPATH;
> +		trace_block_bio_remap(bio, disk_devt(mpath_device->disk),
> +				      bio->bi_iter.bi_sector);

[Severity: Medium]
Does this tracepoint call incorrectly use the target path device instead of
the original head device? 

By passing disk_devt(mpath_device->disk), it seems the trace event will log
the new path device as both the source and destination, which might break
trace analysis. Should this be disk_devt(mpath_head->disk)?

> +		submit_bio_noacct(bio);
> +	} else if (mpath_available_path(mpath_head)) {
> +		dev_warn_ratelimited(dev, "no usable path - requeuing I/O\n");
> +
> +		spin_lock_irq(&mpath_head->requeue_lock);
> +		bio_list_add(&mpath_head->requeue_list, bio);
> +		spin_unlock_irq(&mpath_head->requeue_lock);
> +		atomic_long_inc(&mpath_head->requeue_no_usable_path_cnt);
> +	} else {

[ ... ]

> @@ -314,11 +389,34 @@ static void multipath_partition_scan_work(struct work_struct *work)
>  void mpath_remove_disk(struct mpath_head *mpath_head)
>  {
>  	if (test_and_clear_bit(MPATH_HEAD_DISK_LIVE, &mpath_head->flags)) {
>  		struct gendisk *disk = mpath_head->disk;
>  
> +		/*
> +		 * requeue I/O after MPATH_HEAD_DISK_LIVE has been cleared
> +		 * to allow multipath to fail all I/O.
> +		 */
> +		mpath_schedule_requeue_work(mpath_head);
> +
>  		mpath_synchronize(mpath_head);

[Severity: High]
Could scheduling the requeue work before synchronizing SRCU cause a deadlock
during device removal?

If mpath_bdev_submit_bio() checks mpath_available_path() and sees the disk
is live, but then gets preempted before taking the requeue_lock,
mpath_remove_disk() might clear MPATH_HEAD_DISK_LIVE and schedule the
requeue worker.

The worker would run and find the requeue_list empty. When the submitter
wakes up and adds the bio, it will be stranded on the requeue_list.

Because the bio holds a q_usage_counter reference, will del_gendisk()
hang forever waiting for the queue to freeze?

>  		del_gendisk(disk);
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260703102918.3723667-1-john.g.garry@oracle.com?part=4