diff mbox series

[v2] block: support arbitrary zone size

Message ID 20200210222045.GA1495@avx2 (mailing list archive)
State New, archived
Headers show
Series [v2] block: support arbitrary zone size | expand

Commit Message

Alexey Dobriyan Feb. 10, 2020, 10:20 p.m. UTC
SK hynix is going to ship ZNS device with zone size not being power of 2.

Signed-off-by: Alexey Dobriyan (SK hynix) <adobriyan@gmail.com>
---

	v2: fixup one ">>ilog2"/div conversion :-/

 block/blk-settings.c           |    4 +---
 block/blk-zoned.c              |   10 +++++-----
 drivers/block/null_blk_main.c  |   11 ++++++-----
 drivers/block/null_blk_zoned.c |   10 ++--------
 4 files changed, 14 insertions(+), 21 deletions(-)

Comments

Damien Le Moal Feb. 10, 2020, 10:57 p.m. UTC | #1
On 2020/02/11 7:20, Alexey Dobriyan wrote:
> SK hynix is going to ship ZNS device with zone size not being power of 2.
> 
> Signed-off-by: Alexey Dobriyan (SK hynix) <adobriyan@gmail.com>

In addition to Bart and Keith comments, I will add a clear NACK on this, at
least for this patch as is. Details below inline.

> ---
> 
> 	v2: fixup one ">>ilog2"/div conversion :-/
> 
>  block/blk-settings.c           |    4 +---
>  block/blk-zoned.c              |   10 +++++-----
>  drivers/block/null_blk_main.c  |   11 ++++++-----
>  drivers/block/null_blk_zoned.c |   10 ++--------
>  4 files changed, 14 insertions(+), 21 deletions(-)
> 
> --- a/block/blk-settings.c
> +++ b/block/blk-settings.c
> @@ -206,15 +206,13 @@ EXPORT_SYMBOL(blk_queue_max_hw_sectors);
>   *
>   * Description:
>   *    If a driver doesn't want IOs to cross a given chunk size, it can set
> - *    this limit and prevent merging across chunks. Note that the chunk size
> - *    must currently be a power-of-2 in sectors. Also note that the block
> + *    this limit and prevent merging across chunks. Note that the block
>   *    layer must accept a page worth of data at any offset. So if the
>   *    crossing of chunks is a hard limitation in the driver, it must still be
>   *    prepared to split single page bios.
>   **/
>  void blk_queue_chunk_sectors(struct request_queue *q, unsigned int chunk_sectors)
>  {
> -	BUG_ON(!is_power_of_2(chunk_sectors));
>  	q->limits.chunk_sectors = chunk_sectors;
>  }
>  EXPORT_SYMBOL(blk_queue_chunk_sectors);

Chunk sectors is used by the entire bio/request merging and splitting code.
For zoned devices, this ensures that requests do not cross over zones
boundaries due to merges. All that code relies on power of 2 arithmetics
(bit shifts), so allowing a non power of 2 value would mean changing all of
that. That is the hot path ! adding multiplications and divisions will slow
down all devices, including non zoned ones.

Furthermore, chunk sectors is also used to indicate the stripe size of
software or hardware raid arrays for use by file systems and device mappers
to do optimizations and also avoid merging of bios/request accross device
stripe boundaries. Not enforcing a power of 2 here also potentially breaks
all of that.


> --- a/block/blk-zoned.c
> +++ b/block/blk-zoned.c
> @@ -83,7 +83,7 @@ unsigned int blkdev_nr_zones(struct gendisk *disk)
>  
>  	if (!blk_queue_is_zoned(disk->queue))
>  		return 0;
> -	return (get_capacity(disk) + zone_sectors - 1) >> ilog2(zone_sectors);
> +	return div64_u64(get_capacity(disk) + zone_sectors - 1, zone_sectors);
>  }
>  EXPORT_SYMBOL_GPL(blkdev_nr_zones);

Sure, and this is the kind of modification that will be needed all over the
hot path for bio/request merging checks. Not nice at all. Slow down for
everything.

>  
> @@ -363,14 +363,14 @@ static int blk_revalidate_zone_cb(struct blk_zone *zone, unsigned int idx,
>  	 * smaller last zone.
>  	 */
>  	if (zone->start == 0) {
> -		if (zone->len == 0 || !is_power_of_2(zone->len)) {
> -			pr_warn("%s: Invalid zoned device with non power of two zone size (%llu)\n",
> -				disk->disk_name, zone->len);
> +		if (zone->len == 0) {
> +			pr_warn("%s: Invalid zoned device with length 0\n",
> +				disk->disk_name);
>  			return -ENODEV;
>  		}
>  
>  		args->zone_sectors = zone->len;
> -		args->nr_zones = (capacity + zone->len - 1) >> ilog2(zone->len);
> +		args->nr_zones = div64_u64(capacity + zone->len - 1, zone->len);
>  	} else if (zone->start + args->zone_sectors < capacity) {
>  		if (zone->len != args->zone_sectors) {
>  			pr_warn("%s: Invalid zoned device with non constant zone size\n",

You only have these 2 changes for the generic block layer part, but there
are a lot more to add. Anything that has ilog2(chunk_sectors) (e.g.
blk_queue_zone_no()) needs to be changed. mq-deadline and zone write
locking handling, and all bio/request split & merge checks handling. And
again, even with such changes, allowing chunk_sectors to not be a power of
2 will cause problems for non-zoned use cases such as RAID and in all file
systems relying on that value to be a power of 2.

> --- a/drivers/block/null_blk_main.c
> +++ b/drivers/block/null_blk_main.c
> @@ -187,7 +187,7 @@ MODULE_PARM_DESC(zoned, "Make device as a host-managed zoned block device. Defau
>  
>  static unsigned long g_zone_size = 256;
>  module_param_named(zone_size, g_zone_size, ulong, S_IRUGO);
> -MODULE_PARM_DESC(zone_size, "Zone size in MB when block device is zoned. Must be power-of-two: Default: 256");
> +MODULE_PARM_DESC(zone_size, "Zone size in MB when block device is zoned. Default: 256");
>  
>  static unsigned int g_zone_nr_conv;
>  module_param_named(zone_nr_conv, g_zone_nr_conv, uint, 0444);
> @@ -1641,10 +1641,11 @@ static int null_validate_conf(struct nullb_device *dev)
>  	if (dev->queue_mode == NULL_Q_BIO)
>  		dev->mbps = 0;
>  
> -	if (dev->zoned &&
> -	    (!dev->zone_size || !is_power_of_2(dev->zone_size))) {
> -		pr_err("zone_size must be power-of-two\n");
> -		return -EINVAL;
> +	if (dev->zoned) {
> +		if (dev->zone_size == 0) {
> +			pr_err("zone_size must be positive\n");
> +			return -EINVAL;
> +		}
>  	}
>  
>  	return 0;
> --- a/drivers/block/null_blk_zoned.c
> +++ b/drivers/block/null_blk_zoned.c
> @@ -7,7 +7,7 @@
>  
>  static inline unsigned int null_zone_no(struct nullb_device *dev, sector_t sect)
>  {
> -	return sect >> ilog2(dev->zone_size_sects);
> +	return div64_u64(sect, dev->zone_size_sects);
>  }
>  
>  int null_zone_init(struct nullb_device *dev)
> @@ -16,14 +16,8 @@ int null_zone_init(struct nullb_device *dev)
>  	sector_t sector = 0;
>  	unsigned int i;
>  
> -	if (!is_power_of_2(dev->zone_size)) {
> -		pr_err("zone_size must be power-of-two\n");
> -		return -EINVAL;
> -	}
> -
>  	dev->zone_size_sects = dev->zone_size << ZONE_SIZE_SHIFT;
> -	dev->nr_zones = dev_size >>
> -				(SECTOR_SHIFT + ilog2(dev->zone_size_sects));
> +	dev->nr_zones = div64_u64(dev_size, dev->zone_size_sects * SECTOR_SIZE);
>  	dev->zones = kvmalloc_array(dev->nr_zones, sizeof(struct blk_zone),
>  			GFP_KERNEL | __GFP_ZERO);
>  	if (!dev->zones)
> 

And beyond null_blk, other drivers are affected:
* DM core, dm-linear, dm-flakey and dm-zoned
* f2fs and zonefs file systems will not work as is on such drive. Ongoing
brtfs work is affected too.
* probably some things get broken in the scsi stack too.
* And user space will suffer badly too, at the very least all user space
tools associated with file systems and DM format/check and sys-utils.
Martin K. Petersen Feb. 10, 2020, 11:06 p.m. UTC | #2
Alexey,

> SK hynix is going to ship ZNS device with zone size not being power of 2.

That is going to wreak havoc all over the place. We have always stated
to device vendors that Linux only supports block and zone sizes that are
powers of two.
Matias Bjorling Feb. 10, 2020, 11:28 p.m. UTC | #3
On 10/02/2020 23.20, Alexey Dobriyan wrote:
> SK hynix is going to ship ZNS device with zone size not being power of 2.
>
> Signed-off-by: Alexey Dobriyan (SK hynix) <adobriyan@gmail.com>
> ---

Nack from here as well. The power of 2 is a strict requirement. All SMR 
HDDs Zone Sizes are a power of two, and ZNS SSDs should be as well if 
they want Linux eco-system support.

The ZNS specification is specifically written to accommodate this. It 
has two type of zone sizes, the Zone Size (which is what should be a 
power of two) and the Zone Capacity (that is the writable capacity of 
the zone). For Linux support, your ZNS SSD implemention must have a zone 
size that is power of two, and instead use the Zone Capacity field to be 
the "Zone Size" that you have now. Then it works as expected, making 
both the host and device happy. The host gets power of two, and device 
can communicate the non-power of two writeable LBAs within the zone.

Best, Matias
Javier González Feb. 11, 2020, 9:16 a.m. UTC | #4
On 11.02.2020 01:20, Alexey Dobriyan wrote:
>SK hynix is going to ship ZNS device with zone size not being power of 2.
>
>Signed-off-by: Alexey Dobriyan (SK hynix) <adobriyan@gmail.com>
>---
>
>	v2: fixup one ">>ilog2"/div conversion :-/
>
> block/blk-settings.c           |    4 +---
> block/blk-zoned.c              |   10 +++++-----
> drivers/block/null_blk_main.c  |   11 ++++++-----
> drivers/block/null_blk_zoned.c |   10 ++--------
> 4 files changed, 14 insertions(+), 21 deletions(-)
>
>--- a/block/blk-settings.c
>+++ b/block/blk-settings.c
>@@ -206,15 +206,13 @@ EXPORT_SYMBOL(blk_queue_max_hw_sectors);
>  *
>  * Description:
>  *    If a driver doesn't want IOs to cross a given chunk size, it can set
>- *    this limit and prevent merging across chunks. Note that the chunk size
>- *    must currently be a power-of-2 in sectors. Also note that the block
>+ *    this limit and prevent merging across chunks. Note that the block
>  *    layer must accept a page worth of data at any offset. So if the
>  *    crossing of chunks is a hard limitation in the driver, it must still be
>  *    prepared to split single page bios.
>  **/
> void blk_queue_chunk_sectors(struct request_queue *q, unsigned int chunk_sectors)
> {
>-	BUG_ON(!is_power_of_2(chunk_sectors));
> 	q->limits.chunk_sectors = chunk_sectors;
> }
> EXPORT_SYMBOL(blk_queue_chunk_sectors);
>--- a/block/blk-zoned.c
>+++ b/block/blk-zoned.c
>@@ -83,7 +83,7 @@ unsigned int blkdev_nr_zones(struct gendisk *disk)
>
> 	if (!blk_queue_is_zoned(disk->queue))
> 		return 0;
>-	return (get_capacity(disk) + zone_sectors - 1) >> ilog2(zone_sectors);
>+	return div64_u64(get_capacity(disk) + zone_sectors - 1, zone_sectors);
> }
> EXPORT_SYMBOL_GPL(blkdev_nr_zones);
>
>@@ -363,14 +363,14 @@ static int blk_revalidate_zone_cb(struct blk_zone *zone, unsigned int idx,
> 	 * smaller last zone.
> 	 */
> 	if (zone->start == 0) {
>-		if (zone->len == 0 || !is_power_of_2(zone->len)) {
>-			pr_warn("%s: Invalid zoned device with non power of two zone size (%llu)\n",
>-				disk->disk_name, zone->len);
>+		if (zone->len == 0) {
>+			pr_warn("%s: Invalid zoned device with length 0\n",
>+				disk->disk_name);
> 			return -ENODEV;
> 		}
>
> 		args->zone_sectors = zone->len;
>-		args->nr_zones = (capacity + zone->len - 1) >> ilog2(zone->len);
>+		args->nr_zones = div64_u64(capacity + zone->len - 1, zone->len);
> 	} else if (zone->start + args->zone_sectors < capacity) {
> 		if (zone->len != args->zone_sectors) {
> 			pr_warn("%s: Invalid zoned device with non constant zone size\n",
>--- a/drivers/block/null_blk_main.c
>+++ b/drivers/block/null_blk_main.c
>@@ -187,7 +187,7 @@ MODULE_PARM_DESC(zoned, "Make device as a host-managed zoned block device. Defau
>
> static unsigned long g_zone_size = 256;
> module_param_named(zone_size, g_zone_size, ulong, S_IRUGO);
>-MODULE_PARM_DESC(zone_size, "Zone size in MB when block device is zoned. Must be power-of-two: Default: 256");
>+MODULE_PARM_DESC(zone_size, "Zone size in MB when block device is zoned. Default: 256");
>
> static unsigned int g_zone_nr_conv;
> module_param_named(zone_nr_conv, g_zone_nr_conv, uint, 0444);
>@@ -1641,10 +1641,11 @@ static int null_validate_conf(struct nullb_device *dev)
> 	if (dev->queue_mode == NULL_Q_BIO)
> 		dev->mbps = 0;
>
>-	if (dev->zoned &&
>-	    (!dev->zone_size || !is_power_of_2(dev->zone_size))) {
>-		pr_err("zone_size must be power-of-two\n");
>-		return -EINVAL;
>+	if (dev->zoned) {
>+		if (dev->zone_size == 0) {
>+			pr_err("zone_size must be positive\n");
>+			return -EINVAL;
>+		}
> 	}
>
> 	return 0;
>--- a/drivers/block/null_blk_zoned.c
>+++ b/drivers/block/null_blk_zoned.c
>@@ -7,7 +7,7 @@
>
> static inline unsigned int null_zone_no(struct nullb_device *dev, sector_t sect)
> {
>-	return sect >> ilog2(dev->zone_size_sects);
>+	return div64_u64(sect, dev->zone_size_sects);
> }
>
> int null_zone_init(struct nullb_device *dev)
>@@ -16,14 +16,8 @@ int null_zone_init(struct nullb_device *dev)
> 	sector_t sector = 0;
> 	unsigned int i;
>
>-	if (!is_power_of_2(dev->zone_size)) {
>-		pr_err("zone_size must be power-of-two\n");
>-		return -EINVAL;
>-	}
>-
> 	dev->zone_size_sects = dev->zone_size << ZONE_SIZE_SHIFT;
>-	dev->nr_zones = dev_size >>
>-				(SECTOR_SHIFT + ilog2(dev->zone_size_sects));
>+	dev->nr_zones = div64_u64(dev_size, dev->zone_size_sects * SECTOR_SIZE);
> 	dev->zones = kvmalloc_array(dev->nr_zones, sizeof(struct blk_zone),
> 			GFP_KERNEL | __GFP_ZERO);
> 	if (!dev->zones)

Same response from here. You will need to add some changes to support
arbitrary zone capacities, but the zone size needs to be a power-of-2 to
plug into the zoned framework.

Javier
Alexey Dobriyan Feb. 11, 2020, 5:16 p.m. UTC | #5
On Tue, Feb 11, 2020 at 12:28:17AM +0100, Matias Bjørling wrote:
> The ZNS specification is specifically written to accommodate this. It 
> has two type of zone sizes, the Zone Size (which is what should be a 
> power of two) and the Zone Capacity (that is the writable capacity of 
> the zone).

Ouch, what an embarassing patch I've sent.
For some reason holes in LBA space completely escaped my mind.

	Alexey
Christoph Hellwig Feb. 12, 2020, 8 p.m. UTC | #6
On Tue, Feb 11, 2020 at 01:20:45AM +0300, Alexey Dobriyan wrote:
> SK hynix is going to ship ZNS device with zone size not being power of 2.

NAK.  We clearly stated we require power of two zone sizes in Linux.
(Not Zone Capacities for ZNS, though - which is the whole point the
zone capacity concept was added).
diff mbox series

Patch

--- a/block/blk-settings.c
+++ b/block/blk-settings.c
@@ -206,15 +206,13 @@  EXPORT_SYMBOL(blk_queue_max_hw_sectors);
  *
  * Description:
  *    If a driver doesn't want IOs to cross a given chunk size, it can set
- *    this limit and prevent merging across chunks. Note that the chunk size
- *    must currently be a power-of-2 in sectors. Also note that the block
+ *    this limit and prevent merging across chunks. Note that the block
  *    layer must accept a page worth of data at any offset. So if the
  *    crossing of chunks is a hard limitation in the driver, it must still be
  *    prepared to split single page bios.
  **/
 void blk_queue_chunk_sectors(struct request_queue *q, unsigned int chunk_sectors)
 {
-	BUG_ON(!is_power_of_2(chunk_sectors));
 	q->limits.chunk_sectors = chunk_sectors;
 }
 EXPORT_SYMBOL(blk_queue_chunk_sectors);
--- a/block/blk-zoned.c
+++ b/block/blk-zoned.c
@@ -83,7 +83,7 @@  unsigned int blkdev_nr_zones(struct gendisk *disk)
 
 	if (!blk_queue_is_zoned(disk->queue))
 		return 0;
-	return (get_capacity(disk) + zone_sectors - 1) >> ilog2(zone_sectors);
+	return div64_u64(get_capacity(disk) + zone_sectors - 1, zone_sectors);
 }
 EXPORT_SYMBOL_GPL(blkdev_nr_zones);
 
@@ -363,14 +363,14 @@  static int blk_revalidate_zone_cb(struct blk_zone *zone, unsigned int idx,
 	 * smaller last zone.
 	 */
 	if (zone->start == 0) {
-		if (zone->len == 0 || !is_power_of_2(zone->len)) {
-			pr_warn("%s: Invalid zoned device with non power of two zone size (%llu)\n",
-				disk->disk_name, zone->len);
+		if (zone->len == 0) {
+			pr_warn("%s: Invalid zoned device with length 0\n",
+				disk->disk_name);
 			return -ENODEV;
 		}
 
 		args->zone_sectors = zone->len;
-		args->nr_zones = (capacity + zone->len - 1) >> ilog2(zone->len);
+		args->nr_zones = div64_u64(capacity + zone->len - 1, zone->len);
 	} else if (zone->start + args->zone_sectors < capacity) {
 		if (zone->len != args->zone_sectors) {
 			pr_warn("%s: Invalid zoned device with non constant zone size\n",
--- a/drivers/block/null_blk_main.c
+++ b/drivers/block/null_blk_main.c
@@ -187,7 +187,7 @@  MODULE_PARM_DESC(zoned, "Make device as a host-managed zoned block device. Defau
 
 static unsigned long g_zone_size = 256;
 module_param_named(zone_size, g_zone_size, ulong, S_IRUGO);
-MODULE_PARM_DESC(zone_size, "Zone size in MB when block device is zoned. Must be power-of-two: Default: 256");
+MODULE_PARM_DESC(zone_size, "Zone size in MB when block device is zoned. Default: 256");
 
 static unsigned int g_zone_nr_conv;
 module_param_named(zone_nr_conv, g_zone_nr_conv, uint, 0444);
@@ -1641,10 +1641,11 @@  static int null_validate_conf(struct nullb_device *dev)
 	if (dev->queue_mode == NULL_Q_BIO)
 		dev->mbps = 0;
 
-	if (dev->zoned &&
-	    (!dev->zone_size || !is_power_of_2(dev->zone_size))) {
-		pr_err("zone_size must be power-of-two\n");
-		return -EINVAL;
+	if (dev->zoned) {
+		if (dev->zone_size == 0) {
+			pr_err("zone_size must be positive\n");
+			return -EINVAL;
+		}
 	}
 
 	return 0;
--- a/drivers/block/null_blk_zoned.c
+++ b/drivers/block/null_blk_zoned.c
@@ -7,7 +7,7 @@ 
 
 static inline unsigned int null_zone_no(struct nullb_device *dev, sector_t sect)
 {
-	return sect >> ilog2(dev->zone_size_sects);
+	return div64_u64(sect, dev->zone_size_sects);
 }
 
 int null_zone_init(struct nullb_device *dev)
@@ -16,14 +16,8 @@  int null_zone_init(struct nullb_device *dev)
 	sector_t sector = 0;
 	unsigned int i;
 
-	if (!is_power_of_2(dev->zone_size)) {
-		pr_err("zone_size must be power-of-two\n");
-		return -EINVAL;
-	}
-
 	dev->zone_size_sects = dev->zone_size << ZONE_SIZE_SHIFT;
-	dev->nr_zones = dev_size >>
-				(SECTOR_SHIFT + ilog2(dev->zone_size_sects));
+	dev->nr_zones = div64_u64(dev_size, dev->zone_size_sects * SECTOR_SIZE);
 	dev->zones = kvmalloc_array(dev->nr_zones, sizeof(struct blk_zone),
 			GFP_KERNEL | __GFP_ZERO);
 	if (!dev->zones)