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
