Re: [PATCH 03/24] scsi-multipath: introduce scsi_device head structure

[Benjamin Marzinski <bmarzins@redhat.com>]

On Wed, Feb 25, 2026 at 03:36:06PM +0000, John Garry wrote:
> Introduce a scsi_device head structure - scsi_mpath_head - to manage
> multipathing for a scsi_device. This is similar to nvme_ns_head structure.
> 
> There is no reference in scsi_mpath_head to any disk, as this would be
> mananged by the scsi_disk driver.
> 
> A list of scsi_mpath_head structures is managed to lookup for matching
> multipathed scsi_device's. Matching is done through the scsi_device
> unique id.
> 
> Signed-off-by: John Garry <john.g.garry@oracle.com>
> ---
>  drivers/scsi/scsi_multipath.c | 147 ++++++++++++++++++++++++++++++++++
>  drivers/scsi/scsi_sysfs.c     |   3 +
>  include/scsi/scsi_multipath.h |  29 +++++++
>  3 files changed, 179 insertions(+)
> 
> diff --git a/drivers/scsi/scsi_multipath.c b/drivers/scsi/scsi_multipath.c
> index 04e0bad3d9204..49316269fad8e 100644
> --- a/drivers/scsi/scsi_multipath.c
> +++ b/drivers/scsi/scsi_multipath.c
>
> @@ -107,6 +178,7 @@ static void scsi_multipath_sdev_uninit(struct scsi_device *sdev)
>  
>  int scsi_mpath_dev_alloc(struct scsi_device *sdev)
>  {
> +	struct scsi_mpath_head *scsi_mpath_head;
>  	int ret;
>  
>  	if (!scsi_multipath)
> @@ -127,13 +199,75 @@ int scsi_mpath_dev_alloc(struct scsi_device *sdev)
>  		goto out_uninit;
>  	}
>  
> +	scsi_mpath_head = scsi_mpath_find_head(sdev->scsi_mpath_dev);
> +	if (scsi_mpath_head)
> +		goto found;
> +	/* scsi_mpath_disks_list lock held */

Typo. It should be "scsi_mpath_heads_list lock still held". Also, why
split the locking between this function and scsi_mpath_find_head()? It
seems like it would be clearer if you did in all here.

> +	scsi_mpath_head = scsi_mpath_alloc_head();
> +	if (!scsi_mpath_head)
> +		goto out_uninit;

It seems resonable to failback to treating the device as non-multipathed
if you can't setup the multipathing resources. But you should probably
warn if that happens.

> +
> +	strcpy(scsi_mpath_head->wwid, sdev->scsi_mpath_dev->device_id_str);
> +
> +	ret = device_add(&scsi_mpath_head->dev);
> +	if (ret)
> +		goto out_put_head;
> +
> +	list_add_tail(&scsi_mpath_head->entry, &scsi_mpath_heads_list);
> +
> +	mutex_unlock(&scsi_mpath_heads_lock);
> +	sdev->scsi_mpath_dev->scsi_mpath_head = scsi_mpath_head;

You already set sdev->scsi_mpath_dev->scsi_mpath_head right before you
return.

> +
> +found:
> +	sdev->scsi_mpath_dev->index = ida_alloc(&scsi_mpath_head->ida, GFP_KERNEL);
> +	if (sdev->scsi_mpath_dev->index < 0) {
> +		ret = sdev->scsi_mpath_dev->index;
> +		goto out_put_head;

&scsi_mpath_heads_lock is already unlocked here, but it will get
unlocked again in out_uninit

> +	}
> +
> +	mutex_lock(&scsi_mpath_head->lock);
> +	scsi_mpath_head->dev_count++;
> +	mutex_unlock(&scsi_mpath_head->lock);
> +
> +	sdev->scsi_mpath_dev->scsi_mpath_head = scsi_mpath_head;
>  	return 0;
>  
> +out_put_head:
> +	scsi_mpath_put_head(scsi_mpath_head);
>  out_uninit:
> +	mutex_unlock(&scsi_mpath_heads_lock);
>  	scsi_multipath_sdev_uninit(sdev);
>  	return ret;
>  }
>  
> +static void scsi_mpath_remove_head(struct scsi_mpath_device *scsi_mpath_dev)
> +{
> +	struct scsi_mpath_head *scsi_mpath_head =
> +			scsi_mpath_dev->scsi_mpath_head;
> +	bool last_path = false;
> +
> +	mutex_lock(&scsi_mpath_head->lock);
> +	scsi_mpath_head->dev_count--;
> +	if (scsi_mpath_head->dev_count == 0)
> +		last_path = true;
> +	mutex_unlock(&scsi_mpath_head->lock);

The locking of scsi_mpath_head->lock makes it appear that
scsi_mpath_remove_head() and scsi_mpath_dev_alloc() can both happen at
the same time. I didn't check enough to verify if that's actually the
case, but if it's not, then the lock is unnecessary. If they can run at
the same time, then I don't see anything keeping scsi_mpath_dev_alloc()
from calling scsi_mpath_find_head() and finding a scsi_mpath_head that
is just about to have its device deleted by
device_del(&scsi_mpath_head->dev). If this happens, the device won't
get re-added.

-Ben

> +
> +	if (last_path)
> +		device_del(&scsi_mpath_head->dev);
> +
> +	scsi_mpath_dev->scsi_mpath_head = NULL;
> +	scsi_mpath_put_head(scsi_mpath_head);
> +}