diff mbox series

[07/14] sd: make use of ->free_disk to simplify refcounting

Message ID 20220304160331.399757-8-hch@lst.de
State Superseded
Headers show
Series [01/14] blk-mq: do not include passthrough requests in I/O accounting | expand

Commit Message

Christoph Hellwig March 4, 2022, 4:03 p.m. UTC
Implement the ->free_disk method to to put struct scsi_disk when the last
gendisk reference count goes away.  This removes the need to clear
->private_data and thus freeze the queue on unbind.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/scsi/sd.c | 90 +++++++++--------------------------------------
 1 file changed, 16 insertions(+), 74 deletions(-)

Comments

Bart Van Assche March 6, 2022, 2:03 a.m. UTC | #1
On 3/4/22 08:03, Christoph Hellwig wrote:
>   error_out:
> -	scsi_disk_put(sdkp);
> +	scsi_device_put(sdkp->device);
>   	return retval;	
>   }

Hmm ... why is the above scsi_device_put() call passed sdkp->device? Wouldn't 
it be more symmetric to pass 'sdev' to that function?

> @@ -1502,7 +1468,7 @@ static void sd_release(struct gendisk *disk, fmode_t mode)
>   			scsi_set_medium_removal(sdev, SCSI_REMOVAL_ALLOW);
>   	}
>   
> -	scsi_disk_put(sdkp);
> +	scsi_device_put(sdkp->device);
>   }

Same question here - why to pass sdkp->device instead of sdev?

> +static void scsi_disk_free_disk(struct gendisk *disk)
> +{
> +	struct scsi_disk *sdkp = disk->private_data;
> +
> +	put_device(&sdkp->disk_dev);
> +}

Can the body of the above function be written as 
put_device(&scsi_disk(disk)->disk_dev) ? I'm asking this because other parts of 
this patch use scsi_disk() instead of using disk->private_data directly.

Thanks,

Bart.
Ming Lei March 6, 2022, 3:54 a.m. UTC | #2
On Fri, Mar 04, 2022 at 05:03:24PM +0100, Christoph Hellwig wrote:
> Implement the ->free_disk method to to put struct scsi_disk when the last
> gendisk reference count goes away.  This removes the need to clear
> ->private_data and thus freeze the queue on unbind.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Nice cleanup:

Reviewed-by: Ming Lei <ming.lei@redhat.com>
Christoph Hellwig March 6, 2022, 8:46 a.m. UTC | #3
On Sat, Mar 05, 2022 at 06:03:39PM -0800, Bart Van Assche wrote:
>> -	scsi_disk_put(sdkp);
>> +	scsi_device_put(sdkp->device);
>>   	return retval;	
>>   }
>
> Hmm ... why is the above scsi_device_put() call passed sdkp->device? 
> Wouldn't it be more symmetric to pass 'sdev' to that function?
>
>> @@ -1502,7 +1468,7 @@ static void sd_release(struct gendisk *disk, fmode_t mode)
>>   			scsi_set_medium_removal(sdev, SCSI_REMOVAL_ALLOW);
>>   	}
>>   -	scsi_disk_put(sdkp);
>> +	scsi_device_put(sdkp->device);
>>   }
>
> Same question here - why to pass sdkp->device instead of sdev?

Yes, we can just pass sdev in both cases as that is a bit cleaner.

>
>> +static void scsi_disk_free_disk(struct gendisk *disk)
>> +{
>> +	struct scsi_disk *sdkp = disk->private_data;
>> +
>> +	put_device(&sdkp->disk_dev);
>> +}
>
> Can the body of the above function be written as 
> put_device(&scsi_disk(disk)->disk_dev) ? I'm asking this because other 
> parts of this patch use scsi_disk() instead of using disk->private_data 
> directly.

The scsi_disk() helper is a bit pointless now, but I could use it
here for now.  In the long run we should probably just remove
scsi_disk() entirely.
Martin K. Petersen March 8, 2022, 3:29 a.m. UTC | #4
Christoph,

> Implement the ->free_disk method to to put struct scsi_disk when the
> last gendisk reference count goes away.  This removes the need to
> clear ->private_data and thus freeze the queue on unbind.

Looks fine.

Acked-by: Martin K. Petersen <martin.petersen@oracle.com>
diff mbox series

Patch

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 346b8d62de7d1..498e6fdcf6cfe 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -121,11 +121,6 @@  static void scsi_disk_release(struct device *cdev);
 
 static DEFINE_IDA(sd_index_ida);
 
-/* This semaphore is used to mediate the 0->1 reference get in the
- * face of object destruction (i.e. we can't allow a get on an
- * object after last put) */
-static DEFINE_MUTEX(sd_ref_mutex);
-
 static struct kmem_cache *sd_cdb_cache;
 static mempool_t *sd_cdb_pool;
 static mempool_t *sd_page_pool;
@@ -663,33 +658,6 @@  static int sd_major(int major_idx)
 	}
 }
 
-static struct scsi_disk *scsi_disk_get(struct gendisk *disk)
-{
-	struct scsi_disk *sdkp = NULL;
-
-	mutex_lock(&sd_ref_mutex);
-
-	if (disk->private_data) {
-		sdkp = scsi_disk(disk);
-		if (scsi_device_get(sdkp->device) == 0)
-			get_device(&sdkp->disk_dev);
-		else
-			sdkp = NULL;
-	}
-	mutex_unlock(&sd_ref_mutex);
-	return sdkp;
-}
-
-static void scsi_disk_put(struct scsi_disk *sdkp)
-{
-	struct scsi_device *sdev = sdkp->device;
-
-	mutex_lock(&sd_ref_mutex);
-	put_device(&sdkp->disk_dev);
-	scsi_device_put(sdev);
-	mutex_unlock(&sd_ref_mutex);
-}
-
 #ifdef CONFIG_BLK_SED_OPAL
 static int sd_sec_submit(void *data, u16 spsp, u8 secp, void *buffer,
 		size_t len, bool send)
@@ -1418,17 +1386,15 @@  static bool sd_need_revalidate(struct block_device *bdev,
  **/
 static int sd_open(struct block_device *bdev, fmode_t mode)
 {
-	struct scsi_disk *sdkp = scsi_disk_get(bdev->bd_disk);
-	struct scsi_device *sdev;
+	struct scsi_disk *sdkp = scsi_disk(bdev->bd_disk);
+	struct scsi_device *sdev = sdkp->device;
 	int retval;
 
-	if (!sdkp)
+	if (scsi_device_get(sdev))
 		return -ENXIO;
 
 	SCSI_LOG_HLQUEUE(3, sd_printk(KERN_INFO, sdkp, "sd_open\n"));
 
-	sdev = sdkp->device;
-
 	/*
 	 * If the device is in error recovery, wait until it is done.
 	 * If the device is offline, then disallow any access to it.
@@ -1473,7 +1439,7 @@  static int sd_open(struct block_device *bdev, fmode_t mode)
 	return 0;
 
 error_out:
-	scsi_disk_put(sdkp);
+	scsi_device_put(sdkp->device);
 	return retval;	
 }
 
@@ -1502,7 +1468,7 @@  static void sd_release(struct gendisk *disk, fmode_t mode)
 			scsi_set_medium_removal(sdev, SCSI_REMOVAL_ALLOW);
 	}
 
-	scsi_disk_put(sdkp);
+	scsi_device_put(sdkp->device);
 }
 
 static int sd_getgeo(struct block_device *bdev, struct hd_geometry *geo)
@@ -1616,7 +1582,7 @@  static int media_not_present(struct scsi_disk *sdkp,
  **/
 static unsigned int sd_check_events(struct gendisk *disk, unsigned int clearing)
 {
-	struct scsi_disk *sdkp = scsi_disk_get(disk);
+	struct scsi_disk *sdkp = disk->private_data;
 	struct scsi_device *sdp;
 	int retval;
 	bool disk_changed;
@@ -1679,7 +1645,6 @@  static unsigned int sd_check_events(struct gendisk *disk, unsigned int clearing)
 	 */
 	disk_changed = sdp->changed;
 	sdp->changed = 0;
-	scsi_disk_put(sdkp);
 	return disk_changed ? DISK_EVENT_MEDIA_CHANGE : 0;
 }
 
@@ -1887,6 +1852,13 @@  static const struct pr_ops sd_pr_ops = {
 	.pr_clear	= sd_pr_clear,
 };
 
+static void scsi_disk_free_disk(struct gendisk *disk)
+{
+	struct scsi_disk *sdkp = disk->private_data;
+
+	put_device(&sdkp->disk_dev);
+}
+
 static const struct block_device_operations sd_fops = {
 	.owner			= THIS_MODULE,
 	.open			= sd_open,
@@ -1898,6 +1870,7 @@  static const struct block_device_operations sd_fops = {
 	.unlock_native_capacity	= sd_unlock_native_capacity,
 	.report_zones		= sd_zbc_report_zones,
 	.get_unique_id		= sd_get_unique_id,
+	.free_disk		= scsi_disk_free_disk,
 	.pr_ops			= &sd_pr_ops,
 };
 
@@ -3623,54 +3596,23 @@  static int sd_probe(struct device *dev)
  **/
 static int sd_remove(struct device *dev)
 {
-	struct scsi_disk *sdkp;
+	struct scsi_disk *sdkp = dev_get_drvdata(dev);
 
-	sdkp = dev_get_drvdata(dev);
 	scsi_autopm_get_device(sdkp->device);
 
 	device_del(&sdkp->disk_dev);
 	del_gendisk(sdkp->disk);
 	sd_shutdown(dev);
 
-	mutex_lock(&sd_ref_mutex);
-	dev_set_drvdata(dev, NULL);
-	put_device(&sdkp->disk_dev);
-	mutex_unlock(&sd_ref_mutex);
-
+	put_disk(sdkp->disk);
 	return 0;
 }
 
-/**
- *	scsi_disk_release - Called to free the scsi_disk structure
- *	@dev: pointer to embedded class device
- *
- *	sd_ref_mutex must be held entering this routine.  Because it is
- *	called on last put, you should always use the scsi_disk_get()
- *	scsi_disk_put() helpers which manipulate the semaphore directly
- *	and never do a direct put_device.
- **/
 static void scsi_disk_release(struct device *dev)
 {
 	struct scsi_disk *sdkp = to_scsi_disk(dev);
-	struct gendisk *disk = sdkp->disk;
-	struct request_queue *q = disk->queue;
 
 	ida_free(&sd_index_ida, sdkp->index);
-
-	/*
-	 * Wait until all requests that are in progress have completed.
-	 * This is necessary to avoid that e.g. scsi_end_request() crashes
-	 * due to clearing the disk->private_data pointer. Wait from inside
-	 * scsi_disk_release() instead of from sd_release() to avoid that
-	 * freezing and unfreezing the request queue affects user space I/O
-	 * in case multiple processes open a /dev/sd... node concurrently.
-	 */
-	blk_mq_freeze_queue(q);
-	blk_mq_unfreeze_queue(q);
-
-	disk->private_data = NULL;
-	put_disk(disk);
-
 	sd_zbc_release_disk(sdkp);
 	put_device(&sdkp->device->sdev_gendev);
 	free_opal_dev(sdkp->opal_dev);