Re: [PATCH 21/24] scsi: sd: support multipath disk

[Benjamin Marzinski <bmarzins@redhat.com>]

On Tue, Mar 10, 2026 at 10:12:07AM +0000, John Garry wrote:
> On 10/03/2026 02:40, Benjamin Marzinski wrote:
> > > +static int sd_mpath_probe(struct scsi_disk *sdkp)
> > > +{
> > > +	struct scsi_device *sdp = sdkp->device;
> > > +	struct scsi_mpath_device *scsi_mpath_dev = sdp->scsi_mpath_dev;
> > > +	struct device *dma_dev = sdp->host->dma_dev;
> > > +	struct scsi_mpath_head *scsi_mpath_head =
> > > +				scsi_mpath_dev->scsi_mpath_head;
> > > +	struct sd_mpath_disk *sd_mpath_disk;
> > > +	struct mpath_head *mpath_head = scsi_mpath_head->mpath_head;
> > > +	struct queue_limits lim;
> > > +	struct gendisk *disk;
> > > +	int error;
> > > +
> > > +	/*
> > > +	 * sd_mpath_disks_list is kept locked if no disk found.
> > > +	 * Otherwise an extra reference is taken.
> > > +	 */
> > Again, I personally think the logic is easier to follow when all the
> > locking isn't split over multiple functions.
> 
> Sure, but I am considering removing the mpath_disk structure, so things may
> change here anyway. Removing mpath_disk should simplify things for the nvme
> driver transition.

Sure.

> 
> > 
> > > +	sd_mpath_disk = sd_mpath_find_disk(sdp);
> > > +	if (sd_mpath_disk) {
> > > +		mutex_lock(&sd_mpath_disk->lock);
> > > +		sd_mpath_disk->disk_count++;
> > > +		mutex_unlock(&sd_mpath_disk->lock);
> > > +		goto found;
> > > +	}
> > > +
> > > +	sd_mpath_disk = kzalloc(sizeof(*sd_mpath_disk), GFP_KERNEL);
> > > +	if (!sd_mpath_disk) {
> > > +		error = -ENOMEM;
> > > +		goto out_unlock;
> > > +	}
> > > +
> > > +	sd_mpath_disk->scsi_mpath_head = scsi_mpath_head;
> > > +	device_initialize(&sd_mpath_disk->dev);
> > > +	mutex_init(&sd_mpath_disk->lock);
> > > +	sd_mpath_disk->dev.class = &sd_mpath_disk_class;
> > > +
> > > +	blk_set_stacking_limits(&lim);
> > > +	lim.dma_alignment = 3;
> > > +	lim.features |= BLK_FEAT_IO_STAT | BLK_FEAT_NOWAIT |
> > > +		BLK_FEAT_POLL | BLK_FEAT_ATOMIC_WRITES;
> > > +
> > > +	sd_mpath_disk->mpath_disk = mpath_alloc_head_disk(&lim,
> > > +						dev_to_node(dma_dev));
> > > +	if (!sd_mpath_disk->mpath_disk) {
> > > +		error = -ENOMEM;
> > > +		goto out_free_disk;
> > > +	}
> > > +	disk = sd_mpath_disk->mpath_disk->disk;
> > > +	mpath_get_head(mpath_head); /* undone in mpath_free_disk() */
> > > +
> > > +	sd_mpath_disk->mpath_disk->mpath_head = mpath_head;
> > > +	sd_mpath_disk->mpath_disk->parent = &sd_mpath_disk->dev;
> > > +
> > > +	error = ida_alloc(&sd_index_ida, GFP_KERNEL);
> > > +	if (error < 0) {
> > > +		sdev_printk(KERN_WARNING, sdp, "sd_probe: memory exhausted.\n");
> > > +		goto out_put_disk;
> > > +	}
> > > +	sd_mpath_disk->disk_index = error;
> > > +	error = sd_format_disk_name("sd", sd_mpath_disk->disk_index,
> > > +				disk->disk_name, DISK_NAME_LEN);
> > > +	if (error)
> > > +		goto out_free_index;
> > > +
> > > +	error = dev_set_name(&sd_mpath_disk->dev, "%s",
> > > +				dev_name(&scsi_mpath_head->dev));
> > > +	if (error)
> > > +		goto out_free_index;
> > > +
> > > +	/* undone in sd_mpath_disk_release() */
> > > +	scsi_mpath_get_head(scsi_mpath_head);
> > > +
> > > +	error = device_add(&sd_mpath_disk->dev);
> > > +	if (error) {
> > > +		put_device(&sd_mpath_disk->dev);
> > > +		goto out_unlock;
> > We should clean up when we fail here, instead of just unlocking without
> > fully setting things up.
> 
> I think that the release function is called from put_device(), which should
> do the class tidy up. Something similar is done in sd_probe() for the
> disk_dev.

Oops. You are correct.

-Ben

> Thanks,
> John