Re: [PATCH 09/12] sd: convert to the atomic queue limits API

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 29/05/2024 06:04, Christoph Hellwig wrote:
Assign all queue limits through a local queue_limits variable and
queue_limits_commit_update so that we can't race updating them from
multiple places, and free the queue when updating them so that
in-progress I/O submissions don't see half-updated limits.

Signed-off-by: Christoph Hellwig <hch@xxxxxx>
---
  drivers/scsi/sd.c     | 126 ++++++++++++++++++++++++------------------
  drivers/scsi/sd.h     |   6 +-
  drivers/scsi/sd_zbc.c |  15 ++---
  3 files changed, 84 insertions(+), 63 deletions(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 2d08b69154b995..03e67936b27928 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -101,12 +101,13 @@ MODULE_ALIAS_SCSI_DEVICE(TYPE_ZBC);
#define SD_MINORS 16 -static void sd_config_discard(struct scsi_disk *, unsigned int);
-static void sd_config_write_same(struct scsi_disk *);
+static void sd_config_discard(struct scsi_disk *sdkp, struct queue_limits *lim,
+		unsigned int mode);

Are there any reasons why we keep forward declarations like this? AFAICS, this sd_config_discard forward declaration could be removed.

+static void sd_config_write_same(struct scsi_disk *sdkp,
+		struct queue_limits *lim);
  static int  sd_revalidate_disk(struct gendisk *);
  static void sd_unlock_native_capacity(struct gendisk *disk);
  static void sd_shutdown(struct device *);
-static void sd_read_capacity(struct scsi_disk *sdkp, unsigned char *buffer);
  static void scsi_disk_release(struct device *cdev);
static DEFINE_IDA(sd_index_ida);
@@ -456,7 +457,8 @@ provisioning_mode_store(struct device *dev, struct device_attribute *attr,
  {
  	struct scsi_disk *sdkp = to_scsi_disk(dev);
  	struct scsi_device *sdp = sdkp->device;
-	int mode;
+	struct queue_limits lim;
+	int mode, err;
if (!capable(CAP_SYS_ADMIN))
  		return -EACCES;
@@ -472,8 +474,13 @@ provisioning_mode_store(struct device *dev, struct device_attribute *attr,
  	if (mode < 0)
  		return -EINVAL;
- sd_config_discard(sdkp, mode);
-
+	lim = queue_limits_start_update(sdkp->disk->queue);
+	sd_config_discard(sdkp, &lim, mode);
+	blk_mq_freeze_queue(sdkp->disk->queue);
+	err = queue_limits_commit_update(sdkp->disk->queue, &lim);
+	blk_mq_unfreeze_queue(sdkp->disk->queue);
+	if (err)
+		return err;
  	return count;
  }
  static DEVICE_ATTR_RW(provisioning_mode);
@@ -556,6 +563,7 @@ max_write_same_blocks_store(struct device *dev, struct device_attribute *attr,
  {
  	struct scsi_disk *sdkp = to_scsi_disk(dev);
  	struct scsi_device *sdp = sdkp->device;
+	struct queue_limits lim;
  	unsigned long max;
  	int err;
@@ -577,8 +585,13 @@ max_write_same_blocks_store(struct device *dev, struct device_attribute *attr,
  		sdkp->max_ws_blocks = max;
  	}
- sd_config_write_same(sdkp);
-
+	lim = queue_limits_start_update(sdkp->disk->queue);
+	sd_config_write_same(sdkp, &lim);
+	blk_mq_freeze_queue(sdkp->disk->queue);
+	err = queue_limits_commit_update(sdkp->disk->queue, &lim);
+	blk_mq_unfreeze_queue(sdkp->disk->queue);
+	if (err)
+		return err;
  	return count;
  }
  static DEVICE_ATTR_RW(max_write_same_blocks);
@@ -827,17 +840,15 @@ static void sd_disable_discard(struct scsi_disk *sdkp)
  	blk_queue_max_discard_sectors(sdkp->disk->queue, 0);
  }
-static void sd_config_discard(struct scsi_disk *sdkp, unsigned int mode)
+static void sd_config_discard(struct scsi_disk *sdkp, struct queue_limits *lim,
+		unsigned int mode)
  {
-	struct request_queue *q = sdkp->disk->queue;
  	unsigned int logical_block_size = sdkp->device->sector_size;
  	unsigned int max_blocks = 0;
- q->limits.discard_alignment =
-		sdkp->unmap_alignment * logical_block_size;
-	q->limits.discard_granularity =
-		max(sdkp->physical_block_size,
-		    sdkp->unmap_granularity * logical_block_size);
+	lim->discard_alignment = sdkp->unmap_alignment * logical_block_size;
+	lim->discard_granularity = max(sdkp->physical_block_size,
+			sdkp->unmap_granularity * logical_block_size);
  	sdkp->provisioning_mode = mode;
switch (mode) {
@@ -875,7 +886,8 @@ static void sd_config_discard(struct scsi_disk *sdkp, unsigned int mode)
  		break;
  	}
- blk_queue_max_discard_sectors(q, max_blocks * (logical_block_size >> 9));
+	lim->max_hw_discard_sectors = max_blocks *
+		(logical_block_size >> SECTOR_SHIFT);
  }
static void *sd_set_special_bvec(struct request *rq, unsigned int data_len)
@@ -1010,9 +1022,9 @@ static void sd_disable_write_same(struct scsi_disk *sdkp)
  	blk_queue_max_write_zeroes_sectors(sdkp->disk->queue, 0);
  }
-static void sd_config_write_same(struct scsi_disk *sdkp)
+static void sd_config_write_same(struct scsi_disk *sdkp,
+		struct queue_limits *lim)
  {
-	struct request_queue *q = sdkp->disk->queue;
  	unsigned int logical_block_size = sdkp->device->sector_size;
if (sdkp->device->no_write_same) {
@@ -1066,8 +1078,8 @@ static void sd_config_write_same(struct scsi_disk *sdkp)
  	}
out:
-	blk_queue_max_write_zeroes_sectors(q, sdkp->max_ws_blocks *
-					 (logical_block_size >> 9));
+	lim->max_write_zeroes_sectors =
+		sdkp->max_ws_blocks * (logical_block_size >> 9);

Would it be ok to use SECTOR_SHIFT here? A similar change is made in sd_config_discard(), above

  }
static blk_status_t sd_setup_flush_cmnd(struct scsi_cmnd *cmd)
@@ -2523,7 +2535,7 @@ static void read_capacity_error(struct scsi_disk *sdkp, struct scsi_device *sdp,
  #define READ_CAPACITY_RETRIES_ON_RESET	10
static int read_capacity_16(struct scsi_disk *sdkp, struct scsi_device *sdp,
-						unsigned char *buffer)
+		struct queue_limits *lim, unsigned char *buffer)
  {
  	unsigned char cmd[16];
  	struct scsi_sense_hdr sshdr;
@@ -2597,7 +2609,7 @@ static int read_capacity_16(struct scsi_disk *sdkp, struct scsi_device *sdp,
/* Lowest aligned logical block */
  	alignment = ((buffer[14] & 0x3f) << 8 | buffer[15]) * sector_size;
-	blk_queue_alignment_offset(sdp->request_queue, alignment);
+	lim->alignment_offset = alignment;
  	if (alignment && sdkp->first_scan)
  		sd_printk(KERN_NOTICE, sdkp,
  			  "physical block alignment offset: %u\n", alignment);
@@ -2608,7 +2620,7 @@ static int read_capacity_16(struct scsi_disk *sdkp, struct scsi_device *sdp,
  		if (buffer[14] & 0x40) /* LBPRZ */
  			sdkp->lbprz = 1;
- sd_config_discard(sdkp, SD_LBP_WS16);
+		sd_config_discard(sdkp, lim, SD_LBP_WS16);
  	}
sdkp->capacity = lba + 1;
@@ -2711,13 +2723,14 @@ static int sd_try_rc16_first(struct scsi_device *sdp)
   * read disk capacity
   */
  static void
-sd_read_capacity(struct scsi_disk *sdkp, unsigned char *buffer)
+sd_read_capacity(struct scsi_disk *sdkp, struct queue_limits *lim,
+		unsigned char *buffer)
  {
  	int sector_size;
  	struct scsi_device *sdp = sdkp->device;
if (sd_try_rc16_first(sdp)) {
-		sector_size = read_capacity_16(sdkp, sdp, buffer);
+		sector_size = read_capacity_16(sdkp, sdp, lim, buffer);
  		if (sector_size == -EOVERFLOW)
  			goto got_data;
  		if (sector_size == -ENODEV)
@@ -2737,7 +2750,7 @@ sd_read_capacity(struct scsi_disk *sdkp, unsigned char *buffer)
  			int old_sector_size = sector_size;
  			sd_printk(KERN_NOTICE, sdkp, "Very big device. "
  					"Trying to use READ CAPACITY(16).\n");
-			sector_size = read_capacity_16(sdkp, sdp, buffer);
+			sector_size = read_capacity_16(sdkp, sdp, lim, buffer);
  			if (sector_size < 0) {
  				sd_printk(KERN_NOTICE, sdkp,
  					"Using 0xffffffff as device size\n");
@@ -2796,9 +2809,8 @@ sd_read_capacity(struct scsi_disk *sdkp, unsigned char *buffer)
  		 */
  		sector_size = 512;
  	}
-	blk_queue_logical_block_size(sdp->request_queue, sector_size);
-	blk_queue_physical_block_size(sdp->request_queue,
-				      sdkp->physical_block_size);
+	lim->logical_block_size = sector_size;
+	lim->physical_block_size = sdkp->physical_block_size;
  	sdkp->device->sector_size = sector_size;
if (sdkp->capacity > 0xffffffff)
@@ -3220,11 +3232,11 @@ static unsigned int sd_discard_mode(struct scsi_disk *sdkp)
  	return SD_LBP_DISABLE;
  }
-/**
- * sd_read_block_limits - Query disk device for preferred I/O sizes.
- * @sdkp: disk to query
+/*
+ * Query disk device for preferred I/O sizes.
   */
-static void sd_read_block_limits(struct scsi_disk *sdkp)
+static void sd_read_block_limits(struct scsi_disk *sdkp,
+		struct queue_limits *lim)
  {
  	struct scsi_vpd *vpd;
@@ -3258,7 +3270,7 @@ static void sd_read_block_limits(struct scsi_disk *sdkp)
  			sdkp->unmap_alignment =
  				get_unaligned_be32(&vpd->data[32]) & ~(1 << 31);
- sd_config_discard(sdkp, sd_discard_mode(sdkp));
+		sd_config_discard(sdkp, lim, sd_discard_mode(sdkp));
  	}
out:
@@ -3278,10 +3290,10 @@ static void sd_read_block_limits_ext(struct scsi_disk *sdkp)
  }
/**

below is not a kernel doc comment

- * sd_read_block_characteristics - Query block dev. characteristics
- * @sdkp: disk to query
+ * Query block dev. characteristics
   */
-static void sd_read_block_characteristics(struct scsi_disk *sdkp)
+static void sd_read_block_characteristics(struct scsi_disk *sdkp,
+		struct queue_limits *lim)
  {
  	struct request_queue *q = sdkp->disk->queue;
  	struct scsi_vpd *vpd;
@@ -3307,29 +3319,26 @@ static void sd_read_block_characteristics(struct scsi_disk *sdkp)
#ifdef CONFIG_BLK_DEV_ZONED /* sd_probe rejects ZBD devices early otherwise */
  	if (sdkp->device->type == TYPE_ZBC) {
-		/*
-		 * Host-managed.
-		 */
-		disk_set_zoned(sdkp->disk);
+		lim->zoned = true;
/*
  		 * Per ZBC and ZAC specifications, writes in sequential write
  		 * required zones of host-managed devices must be aligned to
  		 * the device physical block size.
  		 */
-		blk_queue_zone_write_granularity(q, sdkp->physical_block_size);
+		lim->zone_write_granularity = sdkp->physical_block_size;
  	} else {
  		/*
  		 * Host-aware devices are treated as conventional.
  		 */
-		WARN_ON_ONCE(blk_queue_is_zoned(q));
+		lim->zoned = false;
  	}
  #endif /* CONFIG_BLK_DEV_ZONED */
if (!sdkp->first_scan)
  		return;
- if (blk_queue_is_zoned(q))
+	if (lim->zoned)
  		sd_printk(KERN_NOTICE, sdkp, "Host-managed zoned block device\n");
  	else if (sdkp->zoned == 1)
  		sd_printk(KERN_NOTICE, sdkp, "Host-aware SMR disk used as regular disk\n");
@@ -3605,8 +3614,10 @@ static int sd_revalidate_disk(struct gendisk *disk)
  	struct scsi_device *sdp = sdkp->device;
  	struct request_queue *q = sdkp->disk->queue;
  	sector_t old_capacity = sdkp->capacity;
+	struct queue_limits lim;
  	unsigned char *buffer;
  	unsigned int dev_max;
+	int err;
SCSI_LOG_HLQUEUE(3, sd_printk(KERN_INFO, sdkp,
  				      "sd_revalidate_disk\n"));
@@ -3627,12 +3638,14 @@ static int sd_revalidate_disk(struct gendisk *disk)
sd_spinup_disk(sdkp); + lim = queue_limits_start_update(sdkp->disk->queue);
+
  	/*
  	 * Without media there is no reason to ask; moreover, some devices
  	 * react badly if we do.
  	 */
  	if (sdkp->media_present) {
-		sd_read_capacity(sdkp, buffer);
+		sd_read_capacity(sdkp, &lim, buffer);
  		/*
  		 * Some USB/UAS devices return generic values for mode pages
  		 * until the media has been accessed. Trigger a READ operation
@@ -3651,10 +3664,10 @@ static int sd_revalidate_disk(struct gendisk *disk)
if (scsi_device_supports_vpd(sdp)) {
  			sd_read_block_provisioning(sdkp);
-			sd_read_block_limits(sdkp);
+			sd_read_block_limits(sdkp, &lim);
  			sd_read_block_limits_ext(sdkp);
-			sd_read_block_characteristics(sdkp);
-			sd_zbc_read_zones(sdkp, buffer);
+			sd_read_block_characteristics(sdkp, &lim);
+			sd_zbc_read_zones(sdkp, &lim, buffer);
  			sd_read_cpr(sdkp);
  		}
@@ -3683,28 +3696,33 @@ static int sd_revalidate_disk(struct gendisk *disk)
  	q->limits.max_dev_sectors = logical_to_sectors(sdp, dev_max);


is setting q->limits.max_dev_sectors directly proper?

if (sd_validate_min_xfer_size(sdkp))
-		blk_queue_io_min(sdkp->disk->queue,
-				 logical_to_bytes(sdp, sdkp->min_xfer_blocks));
+		lim.io_min = logical_to_bytes(sdp, sdkp->min_xfer_blocks);
  	else
-		blk_queue_io_min(sdkp->disk->queue, 0);
+		lim.io_min = 0;
/*
  	 * Limit default to SCSI host optimal sector limit if set. There may be
  	 * an impact on performance for when the size of a request exceeds this
  	 * host limit.
  	 */
-	q->limits.io_opt = sdp->host->opt_sectors << SECTOR_SHIFT;
+	lim.io_opt = sdp->host->opt_sectors << SECTOR_SHIFT;
  	if (sd_validate_opt_xfer_size(sdkp, dev_max)) {
-		q->limits.io_opt = min_not_zero(q->limits.io_opt,
+		lim.io_opt = min_not_zero(lim.io_opt,
  				logical_to_bytes(sdp, sdkp->opt_xfer_blocks));
  	}
sdkp->first_scan = 0; set_capacity_and_notify(disk, logical_to_sectors(sdp, sdkp->capacity));
-	sd_config_write_same(sdkp);
+	sd_config_write_same(sdkp, &lim);
  	kfree(buffer);





[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Index of Archives]     [SCSI Target Devel]     [Linux SCSI Target Infrastructure]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Linux IIO]     [Samba]     [Device Mapper]

  Powered by Linux