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

[John Garry <john.g.garry@oracle.com>]

On 02/03/2026 02:50, Benjamin Marzinski wrote:
> 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".

Yes

> 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.

Maybe that is better - I'll consider it further.

> 
>> +	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.

OK, I can use pr_err or pr_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.

ok, I'll drop this duplicated code

> 
>> +
>> +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

Yes, I will fix the locking/unlocking

> 
>> +	}
>> +
>> +	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. 

It should be possible for different sdevs

> 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.

Yeah, I think that there might be a problem here. Let me check it further.

Thanks!