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

[John Garry]

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.

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

Thanks,
John