[PATCH 0/8] Support zoned devices with gap zones

[PATCH 0/8] Support zoned devices with gap zones

[Bart Van Assche]

Hi Martin,

In ZBC-2 support has been improved for zones with a size that is not a power
of two by allowing host-managed devices to report gap zones. This patch adds
support for zoned devices for which data zones and gap zones alternate if the
distance between zone start LBAs is a power of two.

Please consider this patch series for kernel v5.19.

Thanks,

Bart.

Bart Van Assche (8):
  scsi: sd_zbc: Improve source code documentation
  scsi: sd_zbc: Rename a local variable
  scsi: sd_zbc: Verify that the zone size is a power of two
  scsi: sd_zbc: Introduce struct zoned_disk_info
  scsi: sd_zbc: Hide gap zones
  scsi_debug: Fix a typo
  scsi_debug: Rename zone type constants
  scsi_debug: Add gap zone support

 drivers/scsi/scsi_debug.c | 147 +++++++++++++++++++++-------
 drivers/scsi/sd.h         |  28 ++++--
 drivers/scsi/sd_zbc.c     | 199 ++++++++++++++++++++++++++++++--------
 include/scsi/scsi_proto.h |   8 +-
 4 files changed, 301 insertions(+), 81 deletions(-)

[PATCH 1/8] scsi: sd_zbc: Improve source code documentation

[Bart Van Assche]

Add several kernel-doc headers. Declare input arrays const. Specify the
array size in function declarations.

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/sd.h     |  5 ++--
 drivers/scsi/sd_zbc.c | 54 ++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 53 insertions(+), 6 deletions(-)

diff --git a/drivers/scsi/sd.h b/drivers/scsi/sd.h
index 0a33a4b68ffb..4849cbe771a7 100644
--- a/drivers/scsi/sd.h
+++ b/drivers/scsi/sd.h
@@ -222,7 +222,7 @@ static inline int sd_is_zoned(struct scsi_disk *sdkp)
 #ifdef CONFIG_BLK_DEV_ZONED
 
 void sd_zbc_release_disk(struct scsi_disk *sdkp);
-int sd_zbc_read_zones(struct scsi_disk *sdkp, unsigned char *buffer);
+int sd_zbc_read_zones(struct scsi_disk *sdkp, u8 buf[SD_BUF_SIZE]);
 int sd_zbc_revalidate_zones(struct scsi_disk *sdkp);
 blk_status_t sd_zbc_setup_zone_mgmt_cmnd(struct scsi_cmnd *cmd,
 					 unsigned char op, bool all);
@@ -238,8 +238,7 @@ blk_status_t sd_zbc_prepare_zone_append(struct scsi_cmnd *cmd, sector_t *lba,
 
 static inline void sd_zbc_release_disk(struct scsi_disk *sdkp) {}
 
-static inline int sd_zbc_read_zones(struct scsi_disk *sdkp,
-				    unsigned char *buf)
+static inline int sd_zbc_read_zones(struct scsi_disk *sdkp, u8 buf[SD_BUF_SIZE])
 {
 	return 0;
 }
diff --git a/drivers/scsi/sd_zbc.c b/drivers/scsi/sd_zbc.c
index 7f466280993b..925976ac5113 100644
--- a/drivers/scsi/sd_zbc.c
+++ b/drivers/scsi/sd_zbc.c
@@ -20,6 +20,12 @@
 
 #include "sd.h"
 
+/**
+ * sd_zbc_get_zone_wp_offset - Get zone write pointer offset.
+ * @zone: Zone for which to return the write pointer offset.
+ *
+ * Return: offset of the write pointer from the start of the zone.
+ */
 static unsigned int sd_zbc_get_zone_wp_offset(struct blk_zone *zone)
 {
 	if (zone->type == ZBC_ZONE_TYPE_CONV)
@@ -44,7 +50,20 @@ static unsigned int sd_zbc_get_zone_wp_offset(struct blk_zone *zone)
 	}
 }
 
-static int sd_zbc_parse_report(struct scsi_disk *sdkp, u8 *buf,
+/**
+ * sd_zbc_parse_report - Parse a SCSI zone descriptor
+ * @sdkp: SCSI disk pointer.
+ * @buf: SCSI zone descriptor.
+ * @idx: Index of the zone in the output of the REPORT ZONES command.
+ * @cb: Callback function pointer.
+ * @data: Second argument passed to @cb.
+ *
+ * Return: Value returned by @cb.
+ *
+ * Convert a SCSI zone descriptor into struct blk_zone format. Additionally,
+ * call @cb(blk_zone, @data).
+ */
+static int sd_zbc_parse_report(struct scsi_disk *sdkp, const u8 buf[64],
 			       unsigned int idx, report_zones_cb cb, void *data)
 {
 	struct scsi_device *sdp = sdkp->device;
@@ -189,6 +208,17 @@ static inline sector_t sd_zbc_zone_sectors(struct scsi_disk *sdkp)
 	return logical_to_sectors(sdkp->device, sdkp->zone_blocks);
 }
 
+/**
+ * sd_zbc_report_zones - SCSI .report_zones() callback.
+ * @disk: Disk to report zones for.
+ * @sector: Start sector.
+ * @nr_zones: Maximum number of zones to report.
+ * @cb: Callback function called to report zone information.
+ * @data: Second argument passed to @cb.
+ *
+ * Called by the block layer to iterate over zone information. See also the
+ * disk->fops->report_zones() calls in block/blk-zoned.c.
+ */
 int sd_zbc_report_zones(struct gendisk *disk, sector_t sector,
 			unsigned int nr_zones, report_zones_cb cb, void *data)
 {
@@ -276,6 +306,10 @@ static int sd_zbc_update_wp_offset_cb(struct blk_zone *zone, unsigned int idx,
 	return 0;
 }
 
+/*
+ * An attempt to append a zone triggered an invalid write pointer error.
+ * Reread the write pointer of the zone(s) in which the append failed.
+ */
 static void sd_zbc_update_wp_offset_workfn(struct work_struct *work)
 {
 	struct scsi_disk *sdkp;
@@ -585,7 +619,7 @@ static int sd_zbc_check_zoned_characteristics(struct scsi_disk *sdkp,
  * sd_zbc_check_capacity - Check the device capacity
  * @sdkp: Target disk
  * @buf: command buffer
- * @zblocks: zone size in number of blocks
+ * @zblocks: zone size in logical blocks
  *
  * Get the device zone size and check that the device capacity as reported
  * by READ CAPACITY matches the max_lba value (plus one) of the report zones
@@ -696,6 +730,11 @@ static void sd_zbc_revalidate_zones_cb(struct gendisk *disk)
 	swap(sdkp->zones_wp_offset, sdkp->rev_wp_offset);
 }
 
+/*
+ * Call blk_revalidate_disk_zones() if any of the zoned disk properties have
+ * changed that make it necessary to call that function. Called by
+ * sd_revalidate_disk() after the gendisk capacity has been set.
+ */
 int sd_zbc_revalidate_zones(struct scsi_disk *sdkp)
 {
 	struct gendisk *disk = sdkp->disk;
@@ -774,7 +813,16 @@ int sd_zbc_revalidate_zones(struct scsi_disk *sdkp)
 	return ret;
 }
 
-int sd_zbc_read_zones(struct scsi_disk *sdkp, unsigned char *buf)
+/**
+ * sd_zbc_read_zones - Read zone information and update the request queue
+ * @sdkp: SCSI disk pointer.
+ * @buf: 512 byte buffer used for storing SCSI command output.
+ *
+ * Read zone information and update the request queue zone characteristics and
+ * also the zoned device information in *sdkp. Called by sd_revalidate_disk()
+ * before the gendisk capacity has been set.
+ */
+int sd_zbc_read_zones(struct scsi_disk *sdkp, u8 buf[SD_BUF_SIZE])
 {
 	struct gendisk *disk = sdkp->disk;
 	struct request_queue *q = disk->queue;

[PATCH 2/8] scsi: sd_zbc: Rename a local variable

[Bart Van Assche]

For zoned storage the word 'capacity' can either refer to the device
capacity or to the zone capacity. Prevent confusion between these two
concepts by renaming 'capacity' into 'device_capacity'.

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/sd_zbc.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/sd_zbc.c b/drivers/scsi/sd_zbc.c
index 925976ac5113..d0275855b16c 100644
--- a/drivers/scsi/sd_zbc.c
+++ b/drivers/scsi/sd_zbc.c
@@ -223,7 +223,8 @@ int sd_zbc_report_zones(struct gendisk *disk, sector_t sector,
 			unsigned int nr_zones, report_zones_cb cb, void *data)
 {
 	struct scsi_disk *sdkp = scsi_disk(disk);
-	sector_t capacity = logical_to_sectors(sdkp->device, sdkp->capacity);
+	sector_t device_capacity = logical_to_sectors(sdkp->device,
+						      sdkp->capacity);
 	unsigned int nr, i;
 	unsigned char *buf;
 	size_t offset, buflen = 0;
@@ -234,7 +235,7 @@ int sd_zbc_report_zones(struct gendisk *disk, sector_t sector,
 		/* Not a zoned device */
 		return -EOPNOTSUPP;
 
-	if (!capacity)
+	if (!device_capacity)
 		/* Device gone or invalid */
 		return -ENODEV;
 
@@ -242,7 +243,7 @@ int sd_zbc_report_zones(struct gendisk *disk, sector_t sector,
 	if (!buf)
 		return -ENOMEM;
 
-	while (zone_idx < nr_zones && sector < capacity) {
+	while (zone_idx < nr_zones && sector < device_capacity) {
 		ret = sd_zbc_do_report_zones(sdkp, buf, buflen,
 				sectors_to_logical(sdkp->device, sector), true);
 		if (ret)

[PATCH 3/8] scsi: sd_zbc: Verify that the zone size is a power of two

[Bart Van Assche]

The following check in sd_zbc_cmnd_checks() can only work correctly if
the zone size is a power of two:

	if (sector & (sd_zbc_zone_sectors(sdkp) - 1))
		/* Unaligned request */
		return BLK_STS_IOERR;

Hence this patch that verifies that the zone size is a power of two.

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/sd_zbc.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/scsi/sd_zbc.c b/drivers/scsi/sd_zbc.c
index d0275855b16c..c1f295859b27 100644
--- a/drivers/scsi/sd_zbc.c
+++ b/drivers/scsi/sd_zbc.c
@@ -666,6 +666,13 @@ static int sd_zbc_check_capacity(struct scsi_disk *sdkp, unsigned char *buf,
 
 	*zblocks = zone_blocks;
 
+	if (!is_power_of_2(*zblocks)) {
+		sd_printk(KERN_ERR, sdkp,
+			  "Zone size %llu is not a power of two.\n",
+			  zone_blocks);
+		return -EINVAL;
+	}
+
 	return 0;
 }
 

[PATCH 4/8] scsi: sd_zbc: Introduce struct zoned_disk_info

[Bart Van Assche]

Deriving the meaning of the nr_zones, rev_nr_zones, zone_blocks and
rev_zone_blocks member variables requires careful analysis of the source
code. Make the meaning of these member variables easier to understand by
introducing struct zoned_disk_info.

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/sd.h     | 18 +++++++++++++----
 drivers/scsi/sd_zbc.c | 47 ++++++++++++++++++++-----------------------
 2 files changed, 36 insertions(+), 29 deletions(-)

diff --git a/drivers/scsi/sd.h b/drivers/scsi/sd.h
index 4849cbe771a7..2e983578a699 100644
--- a/drivers/scsi/sd.h
+++ b/drivers/scsi/sd.h
@@ -67,6 +67,16 @@ enum {
 	SD_ZERO_WS10_UNMAP,	/* Use WRITE SAME(10) with UNMAP */
 };
 
+/**
+ * struct zoned_disk_info - Properties of a zoned SCSI disk.
+ * @nr_zones: number of zones.
+ * @zone_blocks: number of logical blocks per zone.
+ */
+struct zoned_disk_info {
+	u32		nr_zones;
+	u32		zone_blocks;
+};
+
 struct scsi_disk {
 	struct scsi_device *device;
 
@@ -78,10 +88,10 @@ struct scsi_disk {
 	struct gendisk	*disk;
 	struct opal_dev *opal_dev;
 #ifdef CONFIG_BLK_DEV_ZONED
-	u32		nr_zones;
-	u32		rev_nr_zones;
-	u32		zone_blocks;
-	u32		rev_zone_blocks;
+	/* Updated during revalidation before the gendisk capacity is known. */
+	struct zoned_disk_info	early_zone_info;
+	/* Updated during revalidation after the gendisk capacity is known. */
+	struct zoned_disk_info	zone_info;
 	u32		zones_optimal_open;
 	u32		zones_optimal_nonseq;
 	u32		zones_max_open;
diff --git a/drivers/scsi/sd_zbc.c b/drivers/scsi/sd_zbc.c
index c1f295859b27..fe46b4b9913a 100644
--- a/drivers/scsi/sd_zbc.c
+++ b/drivers/scsi/sd_zbc.c
@@ -180,7 +180,7 @@ static void *sd_zbc_alloc_report_buffer(struct scsi_disk *sdkp,
 	 * sure that the allocated buffer can always be mapped by limiting the
 	 * number of pages allocated to the HBA max segments limit.
 	 */
-	nr_zones = min(nr_zones, sdkp->nr_zones);
+	nr_zones = min(nr_zones, sdkp->zone_info.nr_zones);
 	bufsize = roundup((nr_zones + 1) * 64, SECTOR_SIZE);
 	bufsize = min_t(size_t, bufsize,
 			queue_max_hw_sectors(q) << SECTOR_SHIFT);
@@ -205,7 +205,7 @@ static void *sd_zbc_alloc_report_buffer(struct scsi_disk *sdkp,
  */
 static inline sector_t sd_zbc_zone_sectors(struct scsi_disk *sdkp)
 {
-	return logical_to_sectors(sdkp->device, sdkp->zone_blocks);
+	return logical_to_sectors(sdkp->device, sdkp->zone_info.zone_blocks);
 }
 
 /**
@@ -321,14 +321,14 @@ static void sd_zbc_update_wp_offset_workfn(struct work_struct *work)
 	sdkp = container_of(work, struct scsi_disk, zone_wp_offset_work);
 
 	spin_lock_irqsave(&sdkp->zones_wp_offset_lock, flags);
-	for (zno = 0; zno < sdkp->nr_zones; zno++) {
+	for (zno = 0; zno < sdkp->zone_info.nr_zones; zno++) {
 		if (sdkp->zones_wp_offset[zno] != SD_ZBC_UPDATING_WP_OFST)
 			continue;
 
 		spin_unlock_irqrestore(&sdkp->zones_wp_offset_lock, flags);
 		ret = sd_zbc_do_report_zones(sdkp, sdkp->zone_wp_update_buf,
 					     SD_BUF_SIZE,
-					     zno * sdkp->zone_blocks, true);
+					     zno * sdkp->zone_info.zone_blocks, true);
 		spin_lock_irqsave(&sdkp->zones_wp_offset_lock, flags);
 		if (!ret)
 			sd_zbc_parse_report(sdkp, sdkp->zone_wp_update_buf + 64,
@@ -395,7 +395,7 @@ blk_status_t sd_zbc_prepare_zone_append(struct scsi_cmnd *cmd, sector_t *lba,
 		break;
 	default:
 		wp_offset = sectors_to_logical(sdkp->device, wp_offset);
-		if (wp_offset + nr_blocks > sdkp->zone_blocks) {
+		if (wp_offset + nr_blocks > sdkp->zone_info.zone_blocks) {
 			ret = BLK_STS_IOERR;
 			break;
 		}
@@ -524,7 +524,7 @@ static unsigned int sd_zbc_zone_wp_update(struct scsi_cmnd *cmd,
 		break;
 	case REQ_OP_ZONE_RESET_ALL:
 		memset(sdkp->zones_wp_offset, 0,
-		       sdkp->nr_zones * sizeof(unsigned int));
+		       sdkp->zone_info.nr_zones * sizeof(unsigned int));
 		break;
 	default:
 		break;
@@ -681,16 +681,16 @@ static void sd_zbc_print_zones(struct scsi_disk *sdkp)
 	if (!sd_is_zoned(sdkp) || !sdkp->capacity)
 		return;
 
-	if (sdkp->capacity & (sdkp->zone_blocks - 1))
+	if (sdkp->capacity & (sdkp->zone_info.zone_blocks - 1))
 		sd_printk(KERN_NOTICE, sdkp,
 			  "%u zones of %u logical blocks + 1 runt zone\n",
-			  sdkp->nr_zones - 1,
-			  sdkp->zone_blocks);
+			  sdkp->zone_info.nr_zones - 1,
+			  sdkp->zone_info.zone_blocks);
 	else
 		sd_printk(KERN_NOTICE, sdkp,
 			  "%u zones of %u logical blocks\n",
-			  sdkp->nr_zones,
-			  sdkp->zone_blocks);
+			  sdkp->zone_info.nr_zones,
+			  sdkp->zone_info.zone_blocks);
 }
 
 static int sd_zbc_init_disk(struct scsi_disk *sdkp)
@@ -717,10 +717,8 @@ static void sd_zbc_clear_zone_info(struct scsi_disk *sdkp)
 	kfree(sdkp->zone_wp_update_buf);
 	sdkp->zone_wp_update_buf = NULL;
 
-	sdkp->nr_zones = 0;
-	sdkp->rev_nr_zones = 0;
-	sdkp->zone_blocks = 0;
-	sdkp->rev_zone_blocks = 0;
+	sdkp->early_zone_info = (struct zoned_disk_info){ };
+	sdkp->zone_info = (struct zoned_disk_info){ };
 
 	mutex_unlock(&sdkp->rev_mutex);
 }
@@ -747,8 +745,8 @@ int sd_zbc_revalidate_zones(struct scsi_disk *sdkp)
 {
 	struct gendisk *disk = sdkp->disk;
 	struct request_queue *q = disk->queue;
-	u32 zone_blocks = sdkp->rev_zone_blocks;
-	unsigned int nr_zones = sdkp->rev_nr_zones;
+	u32 zone_blocks = sdkp->early_zone_info.zone_blocks;
+	unsigned int nr_zones = sdkp->early_zone_info.nr_zones;
 	u32 max_append;
 	int ret = 0;
 	unsigned int flags;
@@ -779,14 +777,14 @@ int sd_zbc_revalidate_zones(struct scsi_disk *sdkp)
 	 */
 	mutex_lock(&sdkp->rev_mutex);
 
-	if (sdkp->zone_blocks == zone_blocks &&
-	    sdkp->nr_zones == nr_zones &&
+	if (sdkp->zone_info.zone_blocks == zone_blocks &&
+	    sdkp->zone_info.nr_zones == nr_zones &&
 	    disk->queue->nr_zones == nr_zones)
 		goto unlock;
 
 	flags = memalloc_noio_save();
-	sdkp->zone_blocks = zone_blocks;
-	sdkp->nr_zones = nr_zones;
+	sdkp->zone_info.zone_blocks = zone_blocks;
+	sdkp->zone_info.nr_zones = nr_zones;
 	sdkp->rev_wp_offset = kvcalloc(nr_zones, sizeof(u32), GFP_KERNEL);
 	if (!sdkp->rev_wp_offset) {
 		ret = -ENOMEM;
@@ -801,8 +799,7 @@ int sd_zbc_revalidate_zones(struct scsi_disk *sdkp)
 	sdkp->rev_wp_offset = NULL;
 
 	if (ret) {
-		sdkp->zone_blocks = 0;
-		sdkp->nr_zones = 0;
+		sdkp->zone_info = (struct zoned_disk_info){ };
 		sdkp->capacity = 0;
 		goto unlock;
 	}
@@ -888,8 +885,8 @@ int sd_zbc_read_zones(struct scsi_disk *sdkp, u8 buf[SD_BUF_SIZE])
 	if (blk_queue_zoned_model(q) == BLK_ZONED_HM)
 		blk_queue_zone_write_granularity(q, sdkp->physical_block_size);
 
-	sdkp->rev_nr_zones = nr_zones;
-	sdkp->rev_zone_blocks = zone_blocks;
+	sdkp->early_zone_info.nr_zones = nr_zones;
+	sdkp->early_zone_info.zone_blocks = zone_blocks;
 
 	return 0;
 

[PATCH 5/8] scsi: sd_zbc: Hide gap zones

[Bart Van Assche]

ZBC-2 allows host-managed disks to report gap zones. Another new feature
in ZBC-2 is support for constant zone starting LBA offsets. These two
features allow zoned disks to report a starting LBA offset that is a
power of two even if the number of logical blocks with data per zone is
not a power of two.

For zoned disks that report a constant zone starting LBA offset, hide
the gap zones from the block layer. Report the starting LBA offset as
zone size and report the number of logical blocks with data per zone as
the zone capacity.

Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
[ bvanassche: Reworked this patch ]
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/sd.h         |  5 +++
 drivers/scsi/sd_zbc.c     | 84 +++++++++++++++++++++++++++++++++++----
 include/scsi/scsi_proto.h |  8 +++-
 3 files changed, 88 insertions(+), 9 deletions(-)

diff --git a/drivers/scsi/sd.h b/drivers/scsi/sd.h
index 2e983578a699..e0793e789fdc 100644
--- a/drivers/scsi/sd.h
+++ b/drivers/scsi/sd.h
@@ -95,6 +95,11 @@ struct scsi_disk {
 	u32		zones_optimal_open;
 	u32		zones_optimal_nonseq;
 	u32		zones_max_open;
+	/*
+	 * Either zero or a power of two. If not zero it means that the offset
+	 * between zone starting LBAs is constant.
+	 */
+	u32		zone_starting_lba_gran;
 	u32		*zones_wp_offset;
 	spinlock_t	zones_wp_offset_lock;
 	u32		*rev_wp_offset;
diff --git a/drivers/scsi/sd_zbc.c b/drivers/scsi/sd_zbc.c
index fe46b4b9913a..92eace611364 100644
--- a/drivers/scsi/sd_zbc.c
+++ b/drivers/scsi/sd_zbc.c
@@ -37,7 +37,7 @@ static unsigned int sd_zbc_get_zone_wp_offset(struct blk_zone *zone)
 	case BLK_ZONE_COND_CLOSED:
 		return zone->wp - zone->start;
 	case BLK_ZONE_COND_FULL:
-		return zone->len;
+		return zone->capacity;
 	case BLK_ZONE_COND_EMPTY:
 	case BLK_ZONE_COND_OFFLINE:
 	case BLK_ZONE_COND_READONLY:
@@ -50,6 +50,12 @@ static unsigned int sd_zbc_get_zone_wp_offset(struct blk_zone *zone)
 	}
 }
 
+/* Whether or not a SCSI zone descriptor describes a gap zone. */
+static bool sd_zbc_is_gap_zone(const u8 buf[64])
+{
+	return (buf[0] & 0xf) == ZBC_ZONE_TYPE_GAP;
+}
+
 /**
  * sd_zbc_parse_report - Parse a SCSI zone descriptor
  * @sdkp: SCSI disk pointer.
@@ -68,8 +74,12 @@ static int sd_zbc_parse_report(struct scsi_disk *sdkp, const u8 buf[64],
 {
 	struct scsi_device *sdp = sdkp->device;
 	struct blk_zone zone = { 0 };
+	sector_t gran;
+	u64 start_lba;
 	int ret;
 
+	if (WARN_ON_ONCE(sd_zbc_is_gap_zone(buf)))
+		return -EINVAL;
 	zone.type = buf[0] & 0x0f;
 	zone.cond = (buf[1] >> 4) & 0xf;
 	if (buf[1] & 0x01)
@@ -79,9 +89,25 @@ static int sd_zbc_parse_report(struct scsi_disk *sdkp, const u8 buf[64],
 
 	zone.len = logical_to_sectors(sdp, get_unaligned_be64(&buf[8]));
 	zone.capacity = zone.len;
-	zone.start = logical_to_sectors(sdp, get_unaligned_be64(&buf[16]));
+	start_lba = get_unaligned_be64(&buf[16]);
+	zone.start = logical_to_sectors(sdp, start_lba);
+	if (sdkp->zone_starting_lba_gran) {
+		idx = start_lba >> ilog2(sdkp->zone_starting_lba_gran);
+		gran = logical_to_sectors(sdp, sdkp->zone_starting_lba_gran);
+		if (zone.capacity > zone.len || zone.len > gran) {
+			sd_printk(KERN_ERR, sdkp,
+				  "Invalid zone for LBA %llu: zone capacity %llu; zone length %llu; granularity %llu\n",
+				  start_lba, zone.capacity, zone.len, gran);
+			return -EINVAL;
+		}
+		/*
+		 * Change the zone length obtained from REPORT ZONES into the
+		 * zone starting LBA granularity.
+		 */
+		zone.len = gran;
+	}
 	if (zone.cond == ZBC_ZONE_COND_FULL)
-		zone.wp = zone.start + zone.len;
+		zone.wp = zone.start + zone.capacity;
 	else
 		zone.wp = logical_to_sectors(sdp, get_unaligned_be64(&buf[24]));
 
@@ -227,6 +253,7 @@ int sd_zbc_report_zones(struct gendisk *disk, sector_t sector,
 						      sdkp->capacity);
 	unsigned int nr, i;
 	unsigned char *buf;
+	u64 zone_length, start_lba;
 	size_t offset, buflen = 0;
 	int zone_idx = 0;
 	int ret;
@@ -254,16 +281,33 @@ int sd_zbc_report_zones(struct gendisk *disk, sector_t sector,
 		if (!nr)
 			break;
 
-		for (i = 0; i < nr && zone_idx < nr_zones; i++) {
+		for (i = 0; i < nr && zone_idx < nr_zones; i++, zone_idx++) {
 			offset += 64;
+			zone_length = get_unaligned_be64(&buf[offset + 8]);
+			start_lba = get_unaligned_be64(&buf[offset + 16]);
+			if (start_lba < sectors_to_logical(sdkp->device, sector)
+			    || start_lba + zone_length < start_lba) {
+				sd_printk(KERN_ERR, sdkp,
+					  "Zone %d is invalid: %llu + %llu\n",
+					  zone_idx, start_lba, zone_length);
+				ret = -EINVAL;
+				goto out;
+			}
+			sector = logical_to_sectors(sdkp->device, start_lba +
+						    zone_length);
+			if (sd_zbc_is_gap_zone(&buf[offset])) {
+				if (sdkp->zone_starting_lba_gran)
+					continue;
+				sd_printk(KERN_ERR, sdkp,
+					  "Gap zone without constant LBA offsets\n");
+				ret = -EINVAL;
+				goto out;
+			}
 			ret = sd_zbc_parse_report(sdkp, buf + offset, zone_idx,
 						  cb, data);
 			if (ret)
 				goto out;
-			zone_idx++;
 		}
-
-		sector += sd_zbc_zone_sectors(sdkp) * i;
 	}
 
 	ret = zone_idx;
@@ -580,6 +624,8 @@ unsigned int sd_zbc_complete(struct scsi_cmnd *cmd, unsigned int good_bytes,
 static int sd_zbc_check_zoned_characteristics(struct scsi_disk *sdkp,
 					      unsigned char *buf)
 {
+	u64 zone_starting_lba_gran;
+	u8 zone_alignment_method;
 
 	if (scsi_get_vpd_page(sdkp->device, 0xb6, buf, 64)) {
 		sd_printk(KERN_NOTICE, sdkp,
@@ -599,6 +645,28 @@ static int sd_zbc_check_zoned_characteristics(struct scsi_disk *sdkp,
 		sdkp->zones_optimal_open = 0;
 		sdkp->zones_optimal_nonseq = 0;
 		sdkp->zones_max_open = get_unaligned_be32(&buf[16]);
+		zone_alignment_method = buf[23] & 0xf;
+		if (zone_alignment_method ==
+		    ZBC_CONSTANT_ZONE_STARTING_LBA_OFFSETS) {
+			zone_starting_lba_gran =
+				get_unaligned_be64(&buf[24]);
+			if (zone_starting_lba_gran == 0) {
+				sd_printk(KERN_ERR, sdkp,
+					  "Inconsistent: zone starting LBA granularity is zero\n");
+			}
+			if (!is_power_of_2(zone_starting_lba_gran) ||
+			    logical_to_sectors(sdkp->device,
+					       zone_starting_lba_gran) >
+			    UINT_MAX) {
+				sd_printk(KERN_ERR, sdkp,
+					  "Invalid zone starting LBA granularity %llu\n",
+					  zone_starting_lba_gran);
+				return -EINVAL;
+			}
+			sdkp->zone_starting_lba_gran = zone_starting_lba_gran;
+			WARN_ON_ONCE(sdkp->zone_starting_lba_gran !=
+				     zone_starting_lba_gran);
+		}
 	}
 
 	/*
@@ -664,7 +732,7 @@ static int sd_zbc_check_capacity(struct scsi_disk *sdkp, unsigned char *buf,
 		return -EFBIG;
 	}
 
-	*zblocks = zone_blocks;
+	*zblocks = sdkp->zone_starting_lba_gran ? : zone_blocks;
 
 	if (!is_power_of_2(*zblocks)) {
 		sd_printk(KERN_ERR, sdkp,
diff --git a/include/scsi/scsi_proto.h b/include/scsi/scsi_proto.h
index f017843a8124..521f9feac778 100644
--- a/include/scsi/scsi_proto.h
+++ b/include/scsi/scsi_proto.h
@@ -307,7 +307,9 @@ enum zbc_zone_type {
 	ZBC_ZONE_TYPE_CONV		= 0x1,
 	ZBC_ZONE_TYPE_SEQWRITE_REQ	= 0x2,
 	ZBC_ZONE_TYPE_SEQWRITE_PREF	= 0x3,
-	/* 0x4 to 0xf are reserved */
+	ZBC_ZONE_TYPE_SEQ_OR_BEFORE_REQ	= 0x4,
+	ZBC_ZONE_TYPE_GAP		= 0x5,
+	/* 0x6 to 0xf are reserved */
 };
 
 /* Zone conditions of REPORT ZONES zone descriptors */
@@ -323,6 +325,10 @@ enum zbc_zone_cond {
 	ZBC_ZONE_COND_OFFLINE		= 0xf,
 };
 
+enum zbc_zone_alignment_method {
+	ZBC_CONSTANT_ZONE_STARTING_LBA_OFFSETS = 8,
+};
+
 /* Version descriptor values for INQUIRY */
 enum scsi_version_descriptor {
 	SCSI_VERSION_DESCRIPTOR_FCP4	= 0x0a40,

[PATCH 6/8] scsi_debug: Fix a typo

[Bart Van Assche]

Change a single occurrence of "nad" into "and".

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/scsi_debug.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index c607755cce00..7cfae8206a4b 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -4408,7 +4408,7 @@ static int resp_verify(struct scsi_cmnd *scp, struct sdebug_dev_info *devip)
 
 #define RZONES_DESC_HD 64
 
-/* Report zones depending on start LBA nad reporting options */
+/* Report zones depending on start LBA and reporting options */
 static int resp_report_zones(struct scsi_cmnd *scp,
 			     struct sdebug_dev_info *devip)
 {

[PATCH 7/8] scsi_debug: Rename zone type constants

[Bart Van Assche]

Rename the scsi_debug zone type constants to prevent a conflict with the
ZBC_ZONE_TYPE_GAP constant from include/scsi/scsi_proto.h.

Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
[ bvanassche: Extracted these changes from a larger patch ]
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/scsi_debug.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index 7cfae8206a4b..47cec83a4b7c 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -252,9 +252,9 @@ static const char *sdebug_version_date = "20210520";
 
 /* Zone types (zbcr05 table 25) */
 enum sdebug_z_type {
-	ZBC_ZONE_TYPE_CNV	= 0x1,
-	ZBC_ZONE_TYPE_SWR	= 0x2,
-	ZBC_ZONE_TYPE_SWP	= 0x3,
+	ZBC_ZTYPE_CNV	= 0x1,
+	ZBC_ZTYPE_SWR	= 0x2,
+	ZBC_ZTYPE_SWP	= 0x3,
 };
 
 /* enumeration names taken from table 26, zbcr05 */
@@ -2720,7 +2720,7 @@ static struct sdeb_zone_state *zbc_zone(struct sdebug_dev_info *devip,
 
 static inline bool zbc_zone_is_conv(struct sdeb_zone_state *zsp)
 {
-	return zsp->z_type == ZBC_ZONE_TYPE_CNV;
+	return zsp->z_type == ZBC_ZTYPE_CNV;
 }
 
 static void zbc_close_zone(struct sdebug_dev_info *devip,
@@ -2801,7 +2801,7 @@ static void zbc_inc_wp(struct sdebug_dev_info *devip,
 	if (zbc_zone_is_conv(zsp))
 		return;
 
-	if (zsp->z_type == ZBC_ZONE_TYPE_SWR) {
+	if (zsp->z_type == ZBC_ZTYPE_SWR) {
 		zsp->z_wp += num;
 		if (zsp->z_wp >= zend)
 			zsp->z_cond = ZC5_FULL;
@@ -2868,7 +2868,7 @@ static int check_zbc_access_params(struct scsi_cmnd *scp,
 		return 0;
 	}
 
-	if (zsp->z_type == ZBC_ZONE_TYPE_SWR) {
+	if (zsp->z_type == ZBC_ZTYPE_SWR) {
 		/* Writes cannot cross sequential zone boundaries */
 		if (zsp_end != zsp) {
 			mk_sense_buffer(scp, ILLEGAL_REQUEST,
@@ -5005,14 +5005,14 @@ static int sdebug_device_create_zones(struct sdebug_dev_info *devip)
 		zsp->z_start = zstart;
 
 		if (i < devip->nr_conv_zones) {
-			zsp->z_type = ZBC_ZONE_TYPE_CNV;
+			zsp->z_type = ZBC_ZTYPE_CNV;
 			zsp->z_cond = ZBC_NOT_WRITE_POINTER;
 			zsp->z_wp = (sector_t)-1;
 		} else {
 			if (devip->zmodel == BLK_ZONED_HM)
-				zsp->z_type = ZBC_ZONE_TYPE_SWR;
+				zsp->z_type = ZBC_ZTYPE_SWR;
 			else
-				zsp->z_type = ZBC_ZONE_TYPE_SWP;
+				zsp->z_type = ZBC_ZTYPE_SWP;
 			zsp->z_cond = ZC1_EMPTY;
 			zsp->z_wp = zsp->z_start;
 		}

[PATCH 8/8] scsi_debug: Add gap zone support

[Bart Van Assche]

Add the 'zone_cap_mb' kernel module parameter. This parameter defines
the zone capacity. The zone capacity must be less than or equal to the
zone size.

Report that sequential write zones and gap zones are paired in the Zoned
Block Device Characteristics VPD page (page B6h).

This patch has been tested as follows:

modprobe scsi_debug delay=0 sector_size=512 dev_size_mb=128 zbc=host-managed zone_nr_conv=16 zone_size_mb=4 zone_cap_mb=3
modprobe brd rd_nr=1 rd_size=$((1<<20))
mkfs.f2fs -m /dev/ram0 -c /dev/${scsi_debug_dev}
mount /dev/ram0 /mnt
 # Run a fio job that uses /mnt

Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
[ bvanassche: Switched to reporting a constant zone starting LBA granularity ]
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/scsi_debug.c | 127 +++++++++++++++++++++++++++++++-------
 1 file changed, 103 insertions(+), 24 deletions(-)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index 47cec83a4b7c..08bb735b5590 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -99,6 +99,7 @@ static const char *sdebug_version_date = "20210520";
 #define WRITE_BOUNDARY_ASCQ 0x5
 #define READ_INVDATA_ASCQ 0x6
 #define READ_BOUNDARY_ASCQ 0x7
+#define ATTEMPT_ACCESS_GAP 0x9
 #define INSUFF_ZONE_ASCQ 0xe
 
 /* Additional Sense Code Qualifier (ASCQ) */
@@ -255,6 +256,8 @@ enum sdebug_z_type {
 	ZBC_ZTYPE_CNV	= 0x1,
 	ZBC_ZTYPE_SWR	= 0x2,
 	ZBC_ZTYPE_SWP	= 0x3,
+	/* ZBC_ZTYPE_SOBR = 0x4, */
+	ZBC_ZTYPE_GAP	= 0x5,
 };
 
 /* enumeration names taken from table 26, zbcr05 */
@@ -292,10 +295,12 @@ struct sdebug_dev_info {
 
 	/* For ZBC devices */
 	enum blk_zoned_model zmodel;
+	unsigned int zcap;
 	unsigned int zsize;
 	unsigned int zsize_shift;
 	unsigned int nr_zones;
 	unsigned int nr_conv_zones;
+	unsigned int nr_seq_zones;
 	unsigned int nr_imp_open;
 	unsigned int nr_exp_open;
 	unsigned int nr_closed;
@@ -833,6 +838,7 @@ static int dif_errors;
 
 /* ZBC global data */
 static bool sdeb_zbc_in_use;	/* true for host-aware and host-managed disks */
+static int sdeb_zbc_zone_cap_mb;
 static int sdeb_zbc_zone_size_mb;
 static int sdeb_zbc_max_open = DEF_ZBC_MAX_OPEN_ZONES;
 static int sdeb_zbc_nr_conv = DEF_ZBC_NR_CONV_ZONES;
@@ -1563,6 +1569,12 @@ static int inquiry_vpd_b6(struct sdebug_dev_info *devip, unsigned char *arr)
 		put_unaligned_be32(devip->max_open, &arr[12]);
 	else
 		put_unaligned_be32(0xffffffff, &arr[12]);
+	if (devip->zcap < devip->zsize) {
+		arr[19] = ZBC_CONSTANT_ZONE_STARTING_LBA_OFFSETS;
+		put_unaligned_be64(devip->zsize, &arr[20]);
+	} else {
+		arr[19] = 0;
+	}
 	return 0x3c;
 }
 
@@ -2715,7 +2727,23 @@ static inline bool sdebug_dev_is_zoned(struct sdebug_dev_info *devip)
 static struct sdeb_zone_state *zbc_zone(struct sdebug_dev_info *devip,
 					unsigned long long lba)
 {
-	return &devip->zstate[lba >> devip->zsize_shift];
+	u32 zno = lba >> devip->zsize_shift;
+	struct sdeb_zone_state *zsp;
+
+	if (devip->zcap == devip->zsize || zno < devip->nr_conv_zones)
+		return &devip->zstate[zno];
+
+	/*
+	 * If the zone capacity is less than the zone size, adjust for gap
+	 * zones.
+	 */
+	zno = 2 * zno - devip->nr_conv_zones;
+	WARN_ONCE(zno >= devip->nr_zones, "%u > %u\n", zno, devip->nr_zones);
+	zsp = &devip->zstate[zno];
+	if (lba >= zsp->z_start + zsp->z_size)
+		zsp++;
+	WARN_ON_ONCE(lba >= zsp->z_start + zsp->z_size);
+	return zsp;
 }
 
 static inline bool zbc_zone_is_conv(struct sdeb_zone_state *zsp)
@@ -2723,12 +2751,22 @@ static inline bool zbc_zone_is_conv(struct sdeb_zone_state *zsp)
 	return zsp->z_type == ZBC_ZTYPE_CNV;
 }
 
+static inline bool zbc_zone_is_gap(struct sdeb_zone_state *zsp)
+{
+	return zsp->z_type == ZBC_ZTYPE_GAP;
+}
+
+static inline bool zbc_zone_is_seq(struct sdeb_zone_state *zsp)
+{
+	return !zbc_zone_is_conv(zsp) && !zbc_zone_is_gap(zsp);
+}
+
 static void zbc_close_zone(struct sdebug_dev_info *devip,
 			   struct sdeb_zone_state *zsp)
 {
 	enum sdebug_z_cond zc;
 
-	if (zbc_zone_is_conv(zsp))
+	if (!zbc_zone_is_seq(zsp))
 		return;
 
 	zc = zsp->z_cond;
@@ -2766,7 +2804,7 @@ static void zbc_open_zone(struct sdebug_dev_info *devip,
 {
 	enum sdebug_z_cond zc;
 
-	if (zbc_zone_is_conv(zsp))
+	if (!zbc_zone_is_seq(zsp))
 		return;
 
 	zc = zsp->z_cond;
@@ -2798,7 +2836,7 @@ static void zbc_inc_wp(struct sdebug_dev_info *devip,
 	struct sdeb_zone_state *zsp = zbc_zone(devip, lba);
 	unsigned long long n, end, zend = zsp->z_start + zsp->z_size;
 
-	if (zbc_zone_is_conv(zsp))
+	if (!zbc_zone_is_seq(zsp))
 		return;
 
 	if (zsp->z_type == ZBC_ZTYPE_SWR) {
@@ -2846,9 +2884,7 @@ static int check_zbc_access_params(struct scsi_cmnd *scp,
 		if (devip->zmodel == BLK_ZONED_HA)
 			return 0;
 		/* For host-managed, reads cannot cross zone types boundaries */
-		if (zsp_end != zsp &&
-		    zbc_zone_is_conv(zsp) &&
-		    !zbc_zone_is_conv(zsp_end)) {
+		if (zsp->z_type != zsp_end->z_type) {
 			mk_sense_buffer(scp, ILLEGAL_REQUEST,
 					LBA_OUT_OF_RANGE,
 					READ_INVDATA_ASCQ);
@@ -2857,6 +2893,13 @@ static int check_zbc_access_params(struct scsi_cmnd *scp,
 		return 0;
 	}
 
+	/* Writing into a gap zone is not allowed */
+	if (zbc_zone_is_gap(zsp)) {
+		mk_sense_buffer(scp, ILLEGAL_REQUEST, LBA_OUT_OF_RANGE,
+				ATTEMPT_ACCESS_GAP);
+		return check_condition_result;
+	}
+
 	/* No restrictions for writes within conventional zones */
 	if (zbc_zone_is_conv(zsp)) {
 		if (!zbc_zone_is_conv(zsp_end)) {
@@ -4412,14 +4455,14 @@ static int resp_verify(struct scsi_cmnd *scp, struct sdebug_dev_info *devip)
 static int resp_report_zones(struct scsi_cmnd *scp,
 			     struct sdebug_dev_info *devip)
 {
-	unsigned int i, max_zones, rep_max_zones, nrz = 0;
+	unsigned int rep_max_zones, nrz = 0;
 	int ret = 0;
 	u32 alloc_len, rep_opts, rep_len;
 	bool partial;
 	u64 lba, zs_lba;
 	u8 *arr = NULL, *desc;
 	u8 *cmd = scp->cmnd;
-	struct sdeb_zone_state *zsp;
+	struct sdeb_zone_state *zsp = NULL;
 	struct sdeb_store_info *sip = devip2sip(devip, false);
 
 	if (!sdebug_dev_is_zoned(devip)) {
@@ -4438,9 +4481,7 @@ static int resp_report_zones(struct scsi_cmnd *scp,
 		return check_condition_result;
 	}
 
-	max_zones = devip->nr_zones - (zs_lba >> devip->zsize_shift);
-	rep_max_zones = min((alloc_len - 64) >> ilog2(RZONES_DESC_HD),
-			    max_zones);
+	rep_max_zones = (alloc_len - 64) >> ilog2(RZONES_DESC_HD);
 
 	arr = kzalloc(alloc_len, GFP_ATOMIC);
 	if (!arr) {
@@ -4452,9 +4493,9 @@ static int resp_report_zones(struct scsi_cmnd *scp,
 	sdeb_read_lock(sip);
 
 	desc = arr + 64;
-	for (i = 0; i < max_zones; i++) {
-		lba = zs_lba + devip->zsize * i;
-		if (lba > sdebug_capacity)
+	for (lba = zs_lba; lba < sdebug_capacity;
+	     lba = zsp->z_start + zsp->z_size) {
+		if (WARN_ONCE(zbc_zone(devip, lba) == zsp, "lba = %llu\n", lba))
 			break;
 		zsp = zbc_zone(devip, lba);
 		switch (rep_opts) {
@@ -4499,9 +4540,14 @@ static int resp_report_zones(struct scsi_cmnd *scp,
 			if (!zsp->z_non_seq_resource)
 				continue;
 			break;
+		case 0x3e:
+			/* All zones except gap zones. */
+			if (zbc_zone_is_gap(zsp))
+				continue;
+			break;
 		case 0x3f:
 			/* Not write pointer (conventional) zones */
-			if (!zbc_zone_is_conv(zsp))
+			if (zbc_zone_is_seq(zsp))
 				continue;
 			break;
 		default:
@@ -4530,8 +4576,13 @@ static int resp_report_zones(struct scsi_cmnd *scp,
 	}
 
 	/* Report header */
+	/* Zone list length. */
 	put_unaligned_be32(nrz * RZONES_DESC_HD, arr + 0);
+	/* Maximum LBA */
 	put_unaligned_be64(sdebug_capacity - 1, arr + 8);
+	/* Zone starting LBA granularity. */
+	if (devip->zcap < devip->zsize)
+		put_unaligned_be64(devip->zsize, arr + 16);
 
 	rep_len = (unsigned long)desc - (unsigned long)arr;
 	ret = fill_from_dev_buffer(scp, arr, min_t(u32, alloc_len, rep_len));
@@ -4756,7 +4807,7 @@ static void zbc_rwp_zone(struct sdebug_dev_info *devip,
 	enum sdebug_z_cond zc;
 	struct sdeb_store_info *sip = devip2sip(devip, false);
 
-	if (zbc_zone_is_conv(zsp))
+	if (!zbc_zone_is_seq(zsp))
 		return;
 
 	zc = zsp->z_cond;
@@ -4946,6 +4997,7 @@ static int sdebug_device_create_zones(struct sdebug_dev_info *devip)
 {
 	struct sdeb_zone_state *zsp;
 	sector_t capacity = get_sdebug_capacity();
+	sector_t conv_capacity;
 	sector_t zstart = 0;
 	unsigned int i;
 
@@ -4980,11 +5032,30 @@ static int sdebug_device_create_zones(struct sdebug_dev_info *devip)
 	devip->zsize_shift = ilog2(devip->zsize);
 	devip->nr_zones = (capacity + devip->zsize - 1) >> devip->zsize_shift;
 
-	if (sdeb_zbc_nr_conv >= devip->nr_zones) {
+	if (sdeb_zbc_zone_cap_mb == 0) {
+		devip->zcap = devip->zsize;
+	} else {
+		devip->zcap = (sdeb_zbc_zone_cap_mb * SZ_1M) >>
+			      ilog2(sdebug_sector_size);
+		if (devip->zcap > devip->zsize) {
+			pr_err("Zone capacity too large\n");
+			return -EINVAL;
+		}
+	}
+
+	conv_capacity = (sector_t)sdeb_zbc_nr_conv << devip->zsize_shift;
+	if (conv_capacity >= capacity) {
 		pr_err("Number of conventional zones too large\n");
 		return -EINVAL;
 	}
 	devip->nr_conv_zones = sdeb_zbc_nr_conv;
+	devip->nr_seq_zones = roundup(capacity - conv_capacity, devip->zsize) >>
+			      devip->zsize_shift;
+	devip->nr_zones = devip->nr_conv_zones + devip->nr_seq_zones;
+
+	/* Add gap zones if zone capacity is smaller than the zone size */
+	if (devip->zcap < devip->zsize)
+		devip->nr_zones += devip->nr_seq_zones;
 
 	if (devip->zmodel == BLK_ZONED_HM) {
 		/* zbc_max_open_zones can be 0, meaning "not reported" */
@@ -5008,20 +5079,26 @@ static int sdebug_device_create_zones(struct sdebug_dev_info *devip)
 			zsp->z_type = ZBC_ZTYPE_CNV;
 			zsp->z_cond = ZBC_NOT_WRITE_POINTER;
 			zsp->z_wp = (sector_t)-1;
-		} else {
+			zsp->z_size =
+				min_t(u64, devip->zsize, capacity - zstart);
+		} else if ((zstart & (devip->zsize - 1)) == 0) {
 			if (devip->zmodel == BLK_ZONED_HM)
 				zsp->z_type = ZBC_ZTYPE_SWR;
 			else
 				zsp->z_type = ZBC_ZTYPE_SWP;
 			zsp->z_cond = ZC1_EMPTY;
 			zsp->z_wp = zsp->z_start;
+			zsp->z_size =
+				min_t(u64, devip->zcap, capacity - zstart);
+		} else {
+			zsp->z_type = ZBC_ZTYPE_GAP;
+			zsp->z_cond = ZBC_NOT_WRITE_POINTER;
+			zsp->z_wp = (sector_t)-1;
+			zsp->z_size = min_t(u64, devip->zsize - devip->zcap,
+					    capacity - zstart);
 		}
 
-		if (zsp->z_start + devip->zsize < capacity)
-			zsp->z_size = devip->zsize;
-		else
-			zsp->z_size = capacity - zsp->z_start;
-
+		WARN_ON_ONCE((int)zsp->z_size <= 0);
 		zstart += zsp->z_size;
 	}
 
@@ -5856,6 +5933,7 @@ module_param_named(wp, sdebug_wp, bool, S_IRUGO | S_IWUSR);
 module_param_named(write_same_length, sdebug_write_same_length, int,
 		   S_IRUGO | S_IWUSR);
 module_param_named(zbc, sdeb_zbc_model_s, charp, S_IRUGO);
+module_param_named(zone_cap_mb, sdeb_zbc_zone_cap_mb, int, S_IRUGO);
 module_param_named(zone_max_open, sdeb_zbc_max_open, int, S_IRUGO);
 module_param_named(zone_nr_conv, sdeb_zbc_nr_conv, int, S_IRUGO);
 module_param_named(zone_size_mb, sdeb_zbc_zone_size_mb, int, S_IRUGO);
@@ -5927,6 +6005,7 @@ MODULE_PARM_DESC(vpd_use_hostno, "0 -> dev ids ignore hostno (def=1 -> unique de
 MODULE_PARM_DESC(wp, "Write Protect (def=0)");
 MODULE_PARM_DESC(write_same_length, "Maximum blocks per WRITE SAME cmd (def=0xffff)");
 MODULE_PARM_DESC(zbc, "'none' [0]; 'aware' [1]; 'managed' [2] (def=0). Can have 'host-' prefix");
+MODULE_PARM_DESC(zone_cap_mb, "Zone capacity in MiB (def=zone size)");
 MODULE_PARM_DESC(zone_max_open, "Maximum number of open zones; [0] for no limit (def=auto)");
 MODULE_PARM_DESC(zone_nr_conv, "Number of conventional zones (def=1)");
 MODULE_PARM_DESC(zone_size_mb, "Zone size in MiB (def=auto)");

Re: [PATCH 1/8] scsi: sd_zbc: Improve source code documentation

[Damien Le Moal]

On 4/16/22 05:17, Bart Van Assche wrote:
> Add several kernel-doc headers. Declare input arrays const. Specify the
> array size in function declarations.
> 
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>

Looks good.

Reviewed-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>

> ---
>  drivers/scsi/sd.h     |  5 ++--
>  drivers/scsi/sd_zbc.c | 54 ++++++++++++++++++++++++++++++++++++++++---
>  2 files changed, 53 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/scsi/sd.h b/drivers/scsi/sd.h
> index 0a33a4b68ffb..4849cbe771a7 100644
> --- a/drivers/scsi/sd.h
> +++ b/drivers/scsi/sd.h
> @@ -222,7 +222,7 @@ static inline int sd_is_zoned(struct scsi_disk *sdkp)
>  #ifdef CONFIG_BLK_DEV_ZONED
>  
>  void sd_zbc_release_disk(struct scsi_disk *sdkp);
> -int sd_zbc_read_zones(struct scsi_disk *sdkp, unsigned char *buffer);
> +int sd_zbc_read_zones(struct scsi_disk *sdkp, u8 buf[SD_BUF_SIZE]);
>  int sd_zbc_revalidate_zones(struct scsi_disk *sdkp);
>  blk_status_t sd_zbc_setup_zone_mgmt_cmnd(struct scsi_cmnd *cmd,
>  					 unsigned char op, bool all);
> @@ -238,8 +238,7 @@ blk_status_t sd_zbc_prepare_zone_append(struct scsi_cmnd *cmd, sector_t *lba,
>  
>  static inline void sd_zbc_release_disk(struct scsi_disk *sdkp) {}
>  
> -static inline int sd_zbc_read_zones(struct scsi_disk *sdkp,
> -				    unsigned char *buf)
> +static inline int sd_zbc_read_zones(struct scsi_disk *sdkp, u8 buf[SD_BUF_SIZE])
>  {
>  	return 0;
>  }
> diff --git a/drivers/scsi/sd_zbc.c b/drivers/scsi/sd_zbc.c
> index 7f466280993b..925976ac5113 100644
> --- a/drivers/scsi/sd_zbc.c
> +++ b/drivers/scsi/sd_zbc.c
> @@ -20,6 +20,12 @@
>  
>  #include "sd.h"
>  
> +/**
> + * sd_zbc_get_zone_wp_offset - Get zone write pointer offset.
> + * @zone: Zone for which to return the write pointer offset.
> + *
> + * Return: offset of the write pointer from the start of the zone.
> + */
>  static unsigned int sd_zbc_get_zone_wp_offset(struct blk_zone *zone)
>  {
>  	if (zone->type == ZBC_ZONE_TYPE_CONV)
> @@ -44,7 +50,20 @@ static unsigned int sd_zbc_get_zone_wp_offset(struct blk_zone *zone)
>  	}
>  }
>  
> -static int sd_zbc_parse_report(struct scsi_disk *sdkp, u8 *buf,
> +/**
> + * sd_zbc_parse_report - Parse a SCSI zone descriptor
> + * @sdkp: SCSI disk pointer.
> + * @buf: SCSI zone descriptor.
> + * @idx: Index of the zone in the output of the REPORT ZONES command.
> + * @cb: Callback function pointer.
> + * @data: Second argument passed to @cb.
> + *
> + * Return: Value returned by @cb.
> + *
> + * Convert a SCSI zone descriptor into struct blk_zone format. Additionally,
> + * call @cb(blk_zone, @data).
> + */
> +static int sd_zbc_parse_report(struct scsi_disk *sdkp, const u8 buf[64],
>  			       unsigned int idx, report_zones_cb cb, void *data)
>  {
>  	struct scsi_device *sdp = sdkp->device;
> @@ -189,6 +208,17 @@ static inline sector_t sd_zbc_zone_sectors(struct scsi_disk *sdkp)
>  	return logical_to_sectors(sdkp->device, sdkp->zone_blocks);
>  }
>  
> +/**
> + * sd_zbc_report_zones - SCSI .report_zones() callback.
> + * @disk: Disk to report zones for.
> + * @sector: Start sector.
> + * @nr_zones: Maximum number of zones to report.
> + * @cb: Callback function called to report zone information.
> + * @data: Second argument passed to @cb.
> + *
> + * Called by the block layer to iterate over zone information. See also the
> + * disk->fops->report_zones() calls in block/blk-zoned.c.
> + */
>  int sd_zbc_report_zones(struct gendisk *disk, sector_t sector,
>  			unsigned int nr_zones, report_zones_cb cb, void *data)
>  {
> @@ -276,6 +306,10 @@ static int sd_zbc_update_wp_offset_cb(struct blk_zone *zone, unsigned int idx,
>  	return 0;
>  }
>  
> +/*
> + * An attempt to append a zone triggered an invalid write pointer error.
> + * Reread the write pointer of the zone(s) in which the append failed.
> + */
>  static void sd_zbc_update_wp_offset_workfn(struct work_struct *work)
>  {
>  	struct scsi_disk *sdkp;
> @@ -585,7 +619,7 @@ static int sd_zbc_check_zoned_characteristics(struct scsi_disk *sdkp,
>   * sd_zbc_check_capacity - Check the device capacity
>   * @sdkp: Target disk
>   * @buf: command buffer
> - * @zblocks: zone size in number of blocks
> + * @zblocks: zone size in logical blocks
>   *
>   * Get the device zone size and check that the device capacity as reported
>   * by READ CAPACITY matches the max_lba value (plus one) of the report zones
> @@ -696,6 +730,11 @@ static void sd_zbc_revalidate_zones_cb(struct gendisk *disk)
>  	swap(sdkp->zones_wp_offset, sdkp->rev_wp_offset);
>  }
>  
> +/*
> + * Call blk_revalidate_disk_zones() if any of the zoned disk properties have
> + * changed that make it necessary to call that function. Called by
> + * sd_revalidate_disk() after the gendisk capacity has been set.
> + */
>  int sd_zbc_revalidate_zones(struct scsi_disk *sdkp)
>  {
>  	struct gendisk *disk = sdkp->disk;
> @@ -774,7 +813,16 @@ int sd_zbc_revalidate_zones(struct scsi_disk *sdkp)
>  	return ret;
>  }
>  
> -int sd_zbc_read_zones(struct scsi_disk *sdkp, unsigned char *buf)
> +/**
> + * sd_zbc_read_zones - Read zone information and update the request queue
> + * @sdkp: SCSI disk pointer.
> + * @buf: 512 byte buffer used for storing SCSI command output.
> + *
> + * Read zone information and update the request queue zone characteristics and
> + * also the zoned device information in *sdkp. Called by sd_revalidate_disk()
> + * before the gendisk capacity has been set.
> + */
> +int sd_zbc_read_zones(struct scsi_disk *sdkp, u8 buf[SD_BUF_SIZE])
>  {
>  	struct gendisk *disk = sdkp->disk;
>  	struct request_queue *q = disk->queue;


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 2/8] scsi: sd_zbc: Rename a local variable

[Damien Le Moal]

On 4/16/22 05:17, Bart Van Assche wrote:
> For zoned storage the word 'capacity' can either refer to the device
> capacity or to the zone capacity. Prevent confusion between these two
> concepts by renaming 'capacity' into 'device_capacity'.
> 
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>

Looks good.

Reviewed-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>

> ---
>  drivers/scsi/sd_zbc.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/scsi/sd_zbc.c b/drivers/scsi/sd_zbc.c
> index 925976ac5113..d0275855b16c 100644
> --- a/drivers/scsi/sd_zbc.c
> +++ b/drivers/scsi/sd_zbc.c
> @@ -223,7 +223,8 @@ int sd_zbc_report_zones(struct gendisk *disk, sector_t sector,
>  			unsigned int nr_zones, report_zones_cb cb, void *data)
>  {
>  	struct scsi_disk *sdkp = scsi_disk(disk);
> -	sector_t capacity = logical_to_sectors(sdkp->device, sdkp->capacity);
> +	sector_t device_capacity = logical_to_sectors(sdkp->device,
> +						      sdkp->capacity);
>  	unsigned int nr, i;
>  	unsigned char *buf;
>  	size_t offset, buflen = 0;
> @@ -234,7 +235,7 @@ int sd_zbc_report_zones(struct gendisk *disk, sector_t sector,
>  		/* Not a zoned device */
>  		return -EOPNOTSUPP;
>  
> -	if (!capacity)
> +	if (!device_capacity)
>  		/* Device gone or invalid */
>  		return -ENODEV;
>  
> @@ -242,7 +243,7 @@ int sd_zbc_report_zones(struct gendisk *disk, sector_t sector,
>  	if (!buf)
>  		return -ENOMEM;
>  
> -	while (zone_idx < nr_zones && sector < capacity) {
> +	while (zone_idx < nr_zones && sector < device_capacity) {
>  		ret = sd_zbc_do_report_zones(sdkp, buf, buflen,
>  				sectors_to_logical(sdkp->device, sector), true);
>  		if (ret)


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 3/8] scsi: sd_zbc: Verify that the zone size is a power of two

[Damien Le Moal]

On 4/16/22 05:17, Bart Van Assche wrote:
> The following check in sd_zbc_cmnd_checks() can only work correctly if
> the zone size is a power of two:
> 
> 	if (sector & (sd_zbc_zone_sectors(sdkp) - 1))
> 		/* Unaligned request */
> 		return BLK_STS_IOERR;
> 
> Hence this patch that verifies that the zone size is a power of two.
> 
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>

Note that this is already checked in blk_revalidate_disk_zones(), but it
does not hurt to add the check.

Reviewed-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>

> ---
>  drivers/scsi/sd_zbc.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/scsi/sd_zbc.c b/drivers/scsi/sd_zbc.c
> index d0275855b16c..c1f295859b27 100644
> --- a/drivers/scsi/sd_zbc.c
> +++ b/drivers/scsi/sd_zbc.c
> @@ -666,6 +666,13 @@ static int sd_zbc_check_capacity(struct scsi_disk *sdkp, unsigned char *buf,
>  
>  	*zblocks = zone_blocks;
>  
> +	if (!is_power_of_2(*zblocks)) {
> +		sd_printk(KERN_ERR, sdkp,
> +			  "Zone size %llu is not a power of two.\n",
> +			  zone_blocks);
> +		return -EINVAL;
> +	}
> +
>  	return 0;
>  }
>  


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 4/8] scsi: sd_zbc: Introduce struct zoned_disk_info

[Damien Le Moal]

On 4/16/22 05:17, Bart Van Assche wrote:
> Deriving the meaning of the nr_zones, rev_nr_zones, zone_blocks and
> rev_zone_blocks member variables requires careful analysis of the source
> code. Make the meaning of these member variables easier to understand by
> introducing struct zoned_disk_info.
> 
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>  drivers/scsi/sd.h     | 18 +++++++++++++----
>  drivers/scsi/sd_zbc.c | 47 ++++++++++++++++++++-----------------------
>  2 files changed, 36 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/scsi/sd.h b/drivers/scsi/sd.h
> index 4849cbe771a7..2e983578a699 100644
> --- a/drivers/scsi/sd.h
> +++ b/drivers/scsi/sd.h
> @@ -67,6 +67,16 @@ enum {
>  	SD_ZERO_WS10_UNMAP,	/* Use WRITE SAME(10) with UNMAP */
>  };
>  
> +/**
> + * struct zoned_disk_info - Properties of a zoned SCSI disk.

Nit: you could say "ZBC SCSI device" to be more inline with standards
vocabulary here instead of "zoned SCSI disk".

> + * @nr_zones: number of zones.
> + * @zone_blocks: number of logical blocks per zone.
> + */
> +struct zoned_disk_info {
> +	u32		nr_zones;
> +	u32		zone_blocks;
> +};
> +
>  struct scsi_disk {
>  	struct scsi_device *device;
>  
> @@ -78,10 +88,10 @@ struct scsi_disk {
>  	struct gendisk	*disk;
>  	struct opal_dev *opal_dev;
>  #ifdef CONFIG_BLK_DEV_ZONED
> -	u32		nr_zones;
> -	u32		rev_nr_zones;
> -	u32		zone_blocks;
> -	u32		rev_zone_blocks;
> +	/* Updated during revalidation before the gendisk capacity is known. */
> +	struct zoned_disk_info	early_zone_info;
> +	/* Updated during revalidation after the gendisk capacity is known. */
> +	struct zoned_disk_info	zone_info;
>  	u32		zones_optimal_open;
>  	u32		zones_optimal_nonseq;
>  	u32		zones_max_open;

Nit: It would be nice to pack everything under the #ifdef into the same
structure...

> diff --git a/drivers/scsi/sd_zbc.c b/drivers/scsi/sd_zbc.c
> index c1f295859b27..fe46b4b9913a 100644
> --- a/drivers/scsi/sd_zbc.c
> +++ b/drivers/scsi/sd_zbc.c
> @@ -180,7 +180,7 @@ static void *sd_zbc_alloc_report_buffer(struct scsi_disk *sdkp,
>  	 * sure that the allocated buffer can always be mapped by limiting the
>  	 * number of pages allocated to the HBA max segments limit.
>  	 */
> -	nr_zones = min(nr_zones, sdkp->nr_zones);
> +	nr_zones = min(nr_zones, sdkp->zone_info.nr_zones);
>  	bufsize = roundup((nr_zones + 1) * 64, SECTOR_SIZE);
>  	bufsize = min_t(size_t, bufsize,
>  			queue_max_hw_sectors(q) << SECTOR_SHIFT);
> @@ -205,7 +205,7 @@ static void *sd_zbc_alloc_report_buffer(struct scsi_disk *sdkp,
>   */
>  static inline sector_t sd_zbc_zone_sectors(struct scsi_disk *sdkp)
>  {
> -	return logical_to_sectors(sdkp->device, sdkp->zone_blocks);
> +	return logical_to_sectors(sdkp->device, sdkp->zone_info.zone_blocks);
>  }
>  
>  /**
> @@ -321,14 +321,14 @@ static void sd_zbc_update_wp_offset_workfn(struct work_struct *work)
>  	sdkp = container_of(work, struct scsi_disk, zone_wp_offset_work);
>  
>  	spin_lock_irqsave(&sdkp->zones_wp_offset_lock, flags);
> -	for (zno = 0; zno < sdkp->nr_zones; zno++) {
> +	for (zno = 0; zno < sdkp->zone_info.nr_zones; zno++) {
>  		if (sdkp->zones_wp_offset[zno] != SD_ZBC_UPDATING_WP_OFST)
>  			continue;
>  
>  		spin_unlock_irqrestore(&sdkp->zones_wp_offset_lock, flags);
>  		ret = sd_zbc_do_report_zones(sdkp, sdkp->zone_wp_update_buf,
>  					     SD_BUF_SIZE,
> -					     zno * sdkp->zone_blocks, true);
> +					     zno * sdkp->zone_info.zone_blocks, true);
>  		spin_lock_irqsave(&sdkp->zones_wp_offset_lock, flags);
>  		if (!ret)
>  			sd_zbc_parse_report(sdkp, sdkp->zone_wp_update_buf + 64,
> @@ -395,7 +395,7 @@ blk_status_t sd_zbc_prepare_zone_append(struct scsi_cmnd *cmd, sector_t *lba,
>  		break;
>  	default:
>  		wp_offset = sectors_to_logical(sdkp->device, wp_offset);
> -		if (wp_offset + nr_blocks > sdkp->zone_blocks) {
> +		if (wp_offset + nr_blocks > sdkp->zone_info.zone_blocks) {
>  			ret = BLK_STS_IOERR;
>  			break;
>  		}
> @@ -524,7 +524,7 @@ static unsigned int sd_zbc_zone_wp_update(struct scsi_cmnd *cmd,
>  		break;
>  	case REQ_OP_ZONE_RESET_ALL:
>  		memset(sdkp->zones_wp_offset, 0,
> -		       sdkp->nr_zones * sizeof(unsigned int));
> +		       sdkp->zone_info.nr_zones * sizeof(unsigned int));
>  		break;
>  	default:
>  		break;
> @@ -681,16 +681,16 @@ static void sd_zbc_print_zones(struct scsi_disk *sdkp)
>  	if (!sd_is_zoned(sdkp) || !sdkp->capacity)
>  		return;
>  
> -	if (sdkp->capacity & (sdkp->zone_blocks - 1))
> +	if (sdkp->capacity & (sdkp->zone_info.zone_blocks - 1))
>  		sd_printk(KERN_NOTICE, sdkp,
>  			  "%u zones of %u logical blocks + 1 runt zone\n",
> -			  sdkp->nr_zones - 1,
> -			  sdkp->zone_blocks);
> +			  sdkp->zone_info.nr_zones - 1,
> +			  sdkp->zone_info.zone_blocks);
>  	else
>  		sd_printk(KERN_NOTICE, sdkp,
>  			  "%u zones of %u logical blocks\n",
> -			  sdkp->nr_zones,
> -			  sdkp->zone_blocks);
> +			  sdkp->zone_info.nr_zones,
> +			  sdkp->zone_info.zone_blocks);
>  }
>  
>  static int sd_zbc_init_disk(struct scsi_disk *sdkp)
> @@ -717,10 +717,8 @@ static void sd_zbc_clear_zone_info(struct scsi_disk *sdkp)
>  	kfree(sdkp->zone_wp_update_buf);
>  	sdkp->zone_wp_update_buf = NULL;
>  
> -	sdkp->nr_zones = 0;
> -	sdkp->rev_nr_zones = 0;
> -	sdkp->zone_blocks = 0;
> -	sdkp->rev_zone_blocks = 0;
> +	sdkp->early_zone_info = (struct zoned_disk_info){ };
> +	sdkp->zone_info = (struct zoned_disk_info){ };
>  
>  	mutex_unlock(&sdkp->rev_mutex);
>  }
> @@ -747,8 +745,8 @@ int sd_zbc_revalidate_zones(struct scsi_disk *sdkp)
>  {
>  	struct gendisk *disk = sdkp->disk;
>  	struct request_queue *q = disk->queue;
> -	u32 zone_blocks = sdkp->rev_zone_blocks;
> -	unsigned int nr_zones = sdkp->rev_nr_zones;
> +	u32 zone_blocks = sdkp->early_zone_info.zone_blocks;
> +	unsigned int nr_zones = sdkp->early_zone_info.nr_zones;
>  	u32 max_append;
>  	int ret = 0;
>  	unsigned int flags;
> @@ -779,14 +777,14 @@ int sd_zbc_revalidate_zones(struct scsi_disk *sdkp)
>  	 */
>  	mutex_lock(&sdkp->rev_mutex);
>  
> -	if (sdkp->zone_blocks == zone_blocks &&
> -	    sdkp->nr_zones == nr_zones &&
> +	if (sdkp->zone_info.zone_blocks == zone_blocks &&
> +	    sdkp->zone_info.nr_zones == nr_zones &&
>  	    disk->queue->nr_zones == nr_zones)
>  		goto unlock;
>  
>  	flags = memalloc_noio_save();
> -	sdkp->zone_blocks = zone_blocks;
> -	sdkp->nr_zones = nr_zones;
> +	sdkp->zone_info.zone_blocks = zone_blocks;
> +	sdkp->zone_info.nr_zones = nr_zones;
>  	sdkp->rev_wp_offset = kvcalloc(nr_zones, sizeof(u32), GFP_KERNEL);
>  	if (!sdkp->rev_wp_offset) {
>  		ret = -ENOMEM;
> @@ -801,8 +799,7 @@ int sd_zbc_revalidate_zones(struct scsi_disk *sdkp)
>  	sdkp->rev_wp_offset = NULL;
>  
>  	if (ret) {
> -		sdkp->zone_blocks = 0;
> -		sdkp->nr_zones = 0;
> +		sdkp->zone_info = (struct zoned_disk_info){ };
>  		sdkp->capacity = 0;
>  		goto unlock;
>  	}
> @@ -888,8 +885,8 @@ int sd_zbc_read_zones(struct scsi_disk *sdkp, u8 buf[SD_BUF_SIZE])
>  	if (blk_queue_zoned_model(q) == BLK_ZONED_HM)
>  		blk_queue_zone_write_granularity(q, sdkp->physical_block_size);
>  
> -	sdkp->rev_nr_zones = nr_zones;
> -	sdkp->rev_zone_blocks = zone_blocks;
> +	sdkp->early_zone_info.nr_zones = nr_zones;
> +	sdkp->early_zone_info.zone_blocks = zone_blocks;
>  
>  	return 0;
>  

With or without the nit addressed,

Reviewed-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 5/8] scsi: sd_zbc: Hide gap zones

[Damien Le Moal]

On 4/16/22 05:17, Bart Van Assche wrote:
> ZBC-2 allows host-managed disks to report gap zones. Another new feature
> in ZBC-2 is support for constant zone starting LBA offsets. These two
> features allow zoned disks to report a starting LBA offset that is a
> power of two even if the number of logical blocks with data per zone is
> not a power of two.

I think this last sentence would be clearer phrased like this:

These two features allow zoned disks to report zone start LBAs that are a
power of two even if the number of logical blocks with data per zone is
not a power of two.


> 
> For zoned disks that report a constant zone starting LBA offset, hide
> the gap zones from the block layer. Report the starting LBA offset as
> zone size and report the number of logical blocks with data per zone as
> the zone capacity.
> 
> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
> [ bvanassche: Reworked this patch ]
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>  drivers/scsi/sd.h         |  5 +++
>  drivers/scsi/sd_zbc.c     | 84 +++++++++++++++++++++++++++++++++++----
>  include/scsi/scsi_proto.h |  8 +++-
>  3 files changed, 88 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/scsi/sd.h b/drivers/scsi/sd.h
> index 2e983578a699..e0793e789fdc 100644
> --- a/drivers/scsi/sd.h
> +++ b/drivers/scsi/sd.h
> @@ -95,6 +95,11 @@ struct scsi_disk {
>  	u32		zones_optimal_open;
>  	u32		zones_optimal_nonseq;
>  	u32		zones_max_open;
> +	/*
> +	 * Either zero or a power of two. If not zero it means that the offset
> +	 * between zone starting LBAs is constant.
> +	 */
> +	u32		zone_starting_lba_gran;
>  	u32		*zones_wp_offset;
>  	spinlock_t	zones_wp_offset_lock;
>  	u32		*rev_wp_offset;
> diff --git a/drivers/scsi/sd_zbc.c b/drivers/scsi/sd_zbc.c
> index fe46b4b9913a..92eace611364 100644
> --- a/drivers/scsi/sd_zbc.c
> +++ b/drivers/scsi/sd_zbc.c
> @@ -37,7 +37,7 @@ static unsigned int sd_zbc_get_zone_wp_offset(struct blk_zone *zone)
>  	case BLK_ZONE_COND_CLOSED:
>  		return zone->wp - zone->start;
>  	case BLK_ZONE_COND_FULL:
> -		return zone->len;
> +		return zone->capacity;
>  	case BLK_ZONE_COND_EMPTY:
>  	case BLK_ZONE_COND_OFFLINE:
>  	case BLK_ZONE_COND_READONLY:
> @@ -50,6 +50,12 @@ static unsigned int sd_zbc_get_zone_wp_offset(struct blk_zone *zone)
>  	}
>  }
>  
> +/* Whether or not a SCSI zone descriptor describes a gap zone. */
> +static bool sd_zbc_is_gap_zone(const u8 buf[64])
> +{
> +	return (buf[0] & 0xf) == ZBC_ZONE_TYPE_GAP;
> +}
> +
>  /**
>   * sd_zbc_parse_report - Parse a SCSI zone descriptor
>   * @sdkp: SCSI disk pointer.
> @@ -68,8 +74,12 @@ static int sd_zbc_parse_report(struct scsi_disk *sdkp, const u8 buf[64],
>  {
>  	struct scsi_device *sdp = sdkp->device;
>  	struct blk_zone zone = { 0 };
> +	sector_t gran;
> +	u64 start_lba;
>  	int ret;
>  
> +	if (WARN_ON_ONCE(sd_zbc_is_gap_zone(buf)))
> +		return -EINVAL;
>  	zone.type = buf[0] & 0x0f;
>  	zone.cond = (buf[1] >> 4) & 0xf;
>  	if (buf[1] & 0x01)
> @@ -79,9 +89,25 @@ static int sd_zbc_parse_report(struct scsi_disk *sdkp, const u8 buf[64],
>  
>  	zone.len = logical_to_sectors(sdp, get_unaligned_be64(&buf[8]));
>  	zone.capacity = zone.len;
> -	zone.start = logical_to_sectors(sdp, get_unaligned_be64(&buf[16]));
> +	start_lba = get_unaligned_be64(&buf[16]);
> +	zone.start = logical_to_sectors(sdp, start_lba);
> +	if (sdkp->zone_starting_lba_gran) {
> +		idx = start_lba >> ilog2(sdkp->zone_starting_lba_gran);
> +		gran = logical_to_sectors(sdp, sdkp->zone_starting_lba_gran);
> +		if (zone.capacity > zone.len || zone.len > gran) {
> +			sd_printk(KERN_ERR, sdkp,
> +				  "Invalid zone for LBA %llu: zone capacity %llu; zone length %llu; granularity %llu\n",
> +				  start_lba, zone.capacity, zone.len, gran);
> +			return -EINVAL;
> +		}
> +		/*
> +		 * Change the zone length obtained from REPORT ZONES into the
> +		 * zone starting LBA granularity.
> +		 */
> +		zone.len = gran;
> +	}
>  	if (zone.cond == ZBC_ZONE_COND_FULL)
> -		zone.wp = zone.start + zone.len;
> +		zone.wp = zone.start + zone.capacity;
>  	else
>  		zone.wp = logical_to_sectors(sdp, get_unaligned_be64(&buf[24]));
>  
> @@ -227,6 +253,7 @@ int sd_zbc_report_zones(struct gendisk *disk, sector_t sector,
>  						      sdkp->capacity);
>  	unsigned int nr, i;
>  	unsigned char *buf;
> +	u64 zone_length, start_lba;
>  	size_t offset, buflen = 0;
>  	int zone_idx = 0;
>  	int ret;
> @@ -254,16 +281,33 @@ int sd_zbc_report_zones(struct gendisk *disk, sector_t sector,
>  		if (!nr)
>  			break;
>  
> -		for (i = 0; i < nr && zone_idx < nr_zones; i++) {
> +		for (i = 0; i < nr && zone_idx < nr_zones; i++, zone_idx++) {
>  			offset += 64;
> +			zone_length = get_unaligned_be64(&buf[offset + 8]);
> +			start_lba = get_unaligned_be64(&buf[offset + 16]);
> +			if (start_lba < sectors_to_logical(sdkp->device, sector)
> +			    || start_lba + zone_length < start_lba) {
> +				sd_printk(KERN_ERR, sdkp,
> +					  "Zone %d is invalid: %llu + %llu\n",
> +					  zone_idx, start_lba, zone_length);
> +				ret = -EINVAL;
> +				goto out;
> +			}
> +			sector = logical_to_sectors(sdkp->device, start_lba +
> +						    zone_length);
> +			if (sd_zbc_is_gap_zone(&buf[offset])) {
> +				if (sdkp->zone_starting_lba_gran)
> +					continue;
> +				sd_printk(KERN_ERR, sdkp,
> +					  "Gap zone without constant LBA offsets\n");
> +				ret = -EINVAL;
> +				goto out;
> +			}
>  			ret = sd_zbc_parse_report(sdkp, buf + offset, zone_idx,
>  						  cb, data);
>  			if (ret)
>  				goto out;
> -			zone_idx++;
>  		}
> -
> -		sector += sd_zbc_zone_sectors(sdkp) * i;
>  	}
>  
>  	ret = zone_idx;
> @@ -580,6 +624,8 @@ unsigned int sd_zbc_complete(struct scsi_cmnd *cmd, unsigned int good_bytes,
>  static int sd_zbc_check_zoned_characteristics(struct scsi_disk *sdkp,
>  					      unsigned char *buf)
>  {
> +	u64 zone_starting_lba_gran;
> +	u8 zone_alignment_method;
>  
>  	if (scsi_get_vpd_page(sdkp->device, 0xb6, buf, 64)) {
>  		sd_printk(KERN_NOTICE, sdkp,
> @@ -599,6 +645,28 @@ static int sd_zbc_check_zoned_characteristics(struct scsi_disk *sdkp,
>  		sdkp->zones_optimal_open = 0;
>  		sdkp->zones_optimal_nonseq = 0;
>  		sdkp->zones_max_open = get_unaligned_be32(&buf[16]);
> +		zone_alignment_method = buf[23] & 0xf;
> +		if (zone_alignment_method ==
> +		    ZBC_CONSTANT_ZONE_STARTING_LBA_OFFSETS) {
> +			zone_starting_lba_gran =
> +				get_unaligned_be64(&buf[24]);
> +			if (zone_starting_lba_gran == 0) {
> +				sd_printk(KERN_ERR, sdkp,
> +					  "Inconsistent: zone starting LBA granularity is zero\n");
> +			}
> +			if (!is_power_of_2(zone_starting_lba_gran) ||
> +			    logical_to_sectors(sdkp->device,
> +					       zone_starting_lba_gran) >
> +			    UINT_MAX) {
> +				sd_printk(KERN_ERR, sdkp,
> +					  "Invalid zone starting LBA granularity %llu\n",
> +					  zone_starting_lba_gran);
> +				return -EINVAL;
> +			}
> +			sdkp->zone_starting_lba_gran = zone_starting_lba_gran;
> +			WARN_ON_ONCE(sdkp->zone_starting_lba_gran !=
> +				     zone_starting_lba_gran);
> +		}
>  	}
>  
>  	/*
> @@ -664,7 +732,7 @@ static int sd_zbc_check_capacity(struct scsi_disk *sdkp, unsigned char *buf,
>  		return -EFBIG;
>  	}
>  
> -	*zblocks = zone_blocks;
> +	*zblocks = sdkp->zone_starting_lba_gran ? : zone_blocks;
>  
>  	if (!is_power_of_2(*zblocks)) {
>  		sd_printk(KERN_ERR, sdkp,
> diff --git a/include/scsi/scsi_proto.h b/include/scsi/scsi_proto.h
> index f017843a8124..521f9feac778 100644
> --- a/include/scsi/scsi_proto.h
> +++ b/include/scsi/scsi_proto.h
> @@ -307,7 +307,9 @@ enum zbc_zone_type {
>  	ZBC_ZONE_TYPE_CONV		= 0x1,
>  	ZBC_ZONE_TYPE_SEQWRITE_REQ	= 0x2,
>  	ZBC_ZONE_TYPE_SEQWRITE_PREF	= 0x3,
> -	/* 0x4 to 0xf are reserved */
> +	ZBC_ZONE_TYPE_SEQ_OR_BEFORE_REQ	= 0x4,
> +	ZBC_ZONE_TYPE_GAP		= 0x5,
> +	/* 0x6 to 0xf are reserved */
>  };
>  
>  /* Zone conditions of REPORT ZONES zone descriptors */
> @@ -323,6 +325,10 @@ enum zbc_zone_cond {
>  	ZBC_ZONE_COND_OFFLINE		= 0xf,
>  };
>  
> +enum zbc_zone_alignment_method {
> +	ZBC_CONSTANT_ZONE_STARTING_LBA_OFFSETS = 8,
> +};
> +
>  /* Version descriptor values for INQUIRY */
>  enum scsi_version_descriptor {
>  	SCSI_VERSION_DESCRIPTOR_FCP4	= 0x0a40,


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 6/8] scsi_debug: Fix a typo

[Damien Le Moal]

On 4/16/22 05:17, Bart Van Assche wrote:
> Change a single occurrence of "nad" into "and".
> 
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>  drivers/scsi/scsi_debug.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
> index c607755cce00..7cfae8206a4b 100644
> --- a/drivers/scsi/scsi_debug.c
> +++ b/drivers/scsi/scsi_debug.c
> @@ -4408,7 +4408,7 @@ static int resp_verify(struct scsi_cmnd *scp, struct sdebug_dev_info *devip)
>  
>  #define RZONES_DESC_HD 64
>  
> -/* Report zones depending on start LBA nad reporting options */
> +/* Report zones depending on start LBA and reporting options */
>  static int resp_report_zones(struct scsi_cmnd *scp,
>  			     struct sdebug_dev_info *devip)
>  {

Reviewed-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 4/8] scsi: sd_zbc: Introduce struct zoned_disk_info

[Bart Van Assche]

On 4/17/22 16:16, Damien Le Moal wrote:
> On 4/16/22 05:17, Bart Van Assche wrote:
>> +/**
>> + * struct zoned_disk_info - Properties of a zoned SCSI disk.
> 
> Nit: you could say "ZBC SCSI device" to be more inline with standards
> vocabulary here instead of "zoned SCSI disk".

Will fix.

>> + * @nr_zones: number of zones.
>> + * @zone_blocks: number of logical blocks per zone.
>> + */
>> +struct zoned_disk_info {
>> +	u32		nr_zones;
>> +	u32		zone_blocks;
>> +};
>> +
>>   struct scsi_disk {
>>   	struct scsi_device *device;
>>   
>> @@ -78,10 +88,10 @@ struct scsi_disk {
>>   	struct gendisk	*disk;
>>   	struct opal_dev *opal_dev;
>>   #ifdef CONFIG_BLK_DEV_ZONED
>> -	u32		nr_zones;
>> -	u32		rev_nr_zones;
>> -	u32		zone_blocks;
>> -	u32		rev_zone_blocks;
>> +	/* Updated during revalidation before the gendisk capacity is known. */
>> +	struct zoned_disk_info	early_zone_info;
>> +	/* Updated during revalidation after the gendisk capacity is known. */
>> +	struct zoned_disk_info	zone_info;
>>   	u32		zones_optimal_open;
>>   	u32		zones_optimal_nonseq;
>>   	u32		zones_max_open;
> 
> Nit: It would be nice to pack everything under the #ifdef into the same
> structure...

Hmm ... my goal is to only include the ZBC SCSI device properties in struct
zoned_disk_info that are evaluated twice (before and after the gendisk
capacity is known). Most ZBC-related members of struct scsi_disk are only
evaluated once.

Thanks,

Bart.

Re: [PATCH 3/8] scsi: sd_zbc: Verify that the zone size is a power of two

[Bart Van Assche]

On 4/17/22 16:09, Damien Le Moal wrote:
> On 4/16/22 05:17, Bart Van Assche wrote:
>> The following check in sd_zbc_cmnd_checks() can only work correctly if
>> the zone size is a power of two:
>>
>> 	if (sector & (sd_zbc_zone_sectors(sdkp) - 1))
>> 		/* Unaligned request */
>> 		return BLK_STS_IOERR;
>>
>> Hence this patch that verifies that the zone size is a power of two.
> 
> Note that this is already checked in blk_revalidate_disk_zones(), but it
> does not hurt to add the check.

If the block layer would be modified such that support is added for zones with
a size that is not a power of two I think we will really need this check.

Thanks,

Bart.

Re: [PATCH 5/8] scsi: sd_zbc: Hide gap zones

[Bart Van Assche]

On 4/17/22 16:22, Damien Le Moal wrote:
> On 4/16/22 05:17, Bart Van Assche wrote:
>> ZBC-2 allows host-managed disks to report gap zones. Another new feature
>> in ZBC-2 is support for constant zone starting LBA offsets. These two
>> features allow zoned disks to report a starting LBA offset that is a
>> power of two even if the number of logical blocks with data per zone is
>> not a power of two.
> 
> I think this last sentence would be clearer phrased like this:
> 
> These two features allow zoned disks to report zone start LBAs that are a
> power of two even if the number of logical blocks with data per zone is
> not a power of two.

I will modify the patch description.

Thanks,

Bart.