Checking if a given sector is aligned to a zone is a common
operation that is performed for zoned devices. Add
bdev_is_zone_start helper to check for this instead of opencoding it
everywhere.
Convert the calculations on zone size to be generic instead of relying on
power_of_2 based logic in the block layer using the helpers wherever
possible.
The only hot path affected by this change for power_of_2 zoned devices
is in blk_check_zone_append() but bdev_is_zone_start() helper is
used to optimize the calculation for po2 zone sizes. Note that the append
path cannot be accessed by direct raw access to the block device but only
through a filesystem abstraction.
Finally, allow non power of 2 zoned devices provided that their zone
capacity and zone size are equal. The main motivation to allow non
power_of_2 zoned device is to remove the unmapped LBA between zcap and
zsze for devices that cannot have a power_of_2 zcap.
To make this work bdev_get_queue(), bdev_zone_sectors() and
bdev_is_zoned() are moved earlier without modifications.
Reviewed-by: Luis Chamberlain <[email protected]>
Reviewed-by: Hannes Reinecke <[email protected]>
Signed-off-by: Pankaj Raghav <[email protected]>
---
block/blk-core.c | 2 +-
block/blk-zoned.c | 24 +++++++++---
include/linux/blkdev.h | 84 ++++++++++++++++++++++++++++++------------
3 files changed, 79 insertions(+), 31 deletions(-)
diff --git a/block/blk-core.c b/block/blk-core.c
index 3d286a256d3d..1f7e9a90e198 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -570,7 +570,7 @@ static inline blk_status_t blk_check_zone_append(struct request_queue *q,
return BLK_STS_NOTSUPP;
/* The bio sector must point to the start of a sequential zone */
- if (bio->bi_iter.bi_sector & (bdev_zone_sectors(bio->bi_bdev) - 1) ||
+ if (!bdev_is_zone_aligned(bio->bi_bdev, bio->bi_iter.bi_sector) ||
!bio_zone_is_seq(bio))
return BLK_STS_IOERR;
diff --git a/block/blk-zoned.c b/block/blk-zoned.c
index dce9c95b4bcd..a01a231ad328 100644
--- a/block/blk-zoned.c
+++ b/block/blk-zoned.c
@@ -285,10 +285,10 @@ int blkdev_zone_mgmt(struct block_device *bdev, enum req_op op,
return -EINVAL;
/* Check alignment (handle eventual smaller last zone) */
- if (sector & (zone_sectors - 1))
+ if (!bdev_is_zone_aligned(bdev, sector))
return -EINVAL;
- if ((nr_sectors & (zone_sectors - 1)) && end_sector != capacity)
+ if (!bdev_is_zone_aligned(bdev, nr_sectors) && end_sector != capacity)
return -EINVAL;
/*
@@ -486,14 +486,26 @@ 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 zone size", disk->disk_name);
+ return -ENODEV;
+ }
+
+ /*
+ * Non power-of-2 zone size support was added to remove the
+ * gap between zone capacity and zone size. Though it is technically
+ * possible to have gaps in a non power-of-2 device, Linux requires
+ * the zone size to be equal to zone capacity for non power-of-2
+ * zoned devices.
+ */
+ if (!is_power_of_2(zone->len) && zone->capacity < zone->len) {
+ pr_warn("%s: Invalid zone capacity for non power of 2 zone size",
+ 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",
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 85b832908f28..1be805223026 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -634,6 +634,11 @@ static inline bool queue_is_mq(struct request_queue *q)
return q->mq_ops;
}
+static inline struct request_queue *bdev_get_queue(struct block_device *bdev)
+{
+ return bdev->bd_queue; /* this is never NULL */
+}
+
#ifdef CONFIG_PM
static inline enum rpm_status queue_rpm_status(struct request_queue *q)
{
@@ -665,6 +670,25 @@ static inline bool blk_queue_is_zoned(struct request_queue *q)
}
}
+static inline bool bdev_is_zoned(struct block_device *bdev)
+{
+ struct request_queue *q = bdev_get_queue(bdev);
+
+ if (q)
+ return blk_queue_is_zoned(q);
+
+ return false;
+}
+
+static inline sector_t bdev_zone_sectors(struct block_device *bdev)
+{
+ struct request_queue *q = bdev_get_queue(bdev);
+
+ if (!blk_queue_is_zoned(q))
+ return 0;
+ return q->limits.chunk_sectors;
+}
+
#ifdef CONFIG_BLK_DEV_ZONED
static inline unsigned int disk_nr_zones(struct gendisk *disk)
{
@@ -684,6 +708,30 @@ static inline unsigned int disk_zone_no(struct gendisk *disk, sector_t sector)
return div64_u64(sector, zone_sectors);
}
+static inline sector_t bdev_offset_from_zone_start(struct block_device *bdev,
+ sector_t sec)
+{
+ sector_t zone_sectors = bdev_zone_sectors(bdev);
+ u64 remainder = 0;
+
+ if (!bdev_is_zoned(bdev))
+ return 0;
+
+ if (is_power_of_2(zone_sectors))
+ return sec & (zone_sectors - 1);
+
+ div64_u64_rem(sec, zone_sectors, &remainder);
+ return remainder;
+}
+
+static inline bool bdev_is_zone_aligned(struct block_device *bdev, sector_t sec)
+{
+ if (!bdev_is_zoned(bdev))
+ return false;
+
+ return bdev_offset_from_zone_start(bdev, sec) == 0;
+}
+
static inline bool disk_zone_is_seq(struct gendisk *disk, sector_t sector)
{
if (!blk_queue_is_zoned(disk->queue))
@@ -728,6 +776,18 @@ static inline unsigned int disk_zone_no(struct gendisk *disk, sector_t sector)
{
return 0;
}
+
+static inline sector_t bdev_offset_from_zone_start(struct block_device *bdev,
+ sector_t sec)
+{
+ return 0;
+}
+
+static inline bool bdev_is_zone_aligned(struct block_device *bdev, sector_t sec)
+{
+ return false;
+}
+
static inline unsigned int bdev_max_open_zones(struct block_device *bdev)
{
return 0;
@@ -891,11 +951,6 @@ int bio_poll(struct bio *bio, struct io_comp_batch *iob, unsigned int flags);
int iocb_bio_iopoll(struct kiocb *kiocb, struct io_comp_batch *iob,
unsigned int flags);
-static inline struct request_queue *bdev_get_queue(struct block_device *bdev)
-{
- return bdev->bd_queue; /* this is never NULL */
-}
-
/* Helper to convert BLK_ZONE_ZONE_XXX to its string format XXX */
const char *blk_zone_cond_str(enum blk_zone_cond zone_cond);
@@ -1295,25 +1350,6 @@ static inline enum blk_zoned_model bdev_zoned_model(struct block_device *bdev)
return BLK_ZONED_NONE;
}
-static inline bool bdev_is_zoned(struct block_device *bdev)
-{
- struct request_queue *q = bdev_get_queue(bdev);
-
- if (q)
- return blk_queue_is_zoned(q);
-
- return false;
-}
-
-static inline sector_t bdev_zone_sectors(struct block_device *bdev)
-{
- struct request_queue *q = bdev_get_queue(bdev);
-
- if (!blk_queue_is_zoned(q))
- return 0;
- return q->limits.chunk_sectors;
-}
-
static inline int queue_dma_alignment(const struct request_queue *q)
{
return q ? q->dma_alignment : 511;
--
2.25.1
On 7/27/22 09:22, Pankaj Raghav wrote:
> Checking if a given sector is aligned to a zone is a common
> operation that is performed for zoned devices. Add
> bdev_is_zone_start helper to check for this instead of opencoding it
> everywhere.
I can't find the bdev_is_zone_start() function in this patch?
> To make this work bdev_get_queue(), bdev_zone_sectors() and
> bdev_is_zoned() are moved earlier without modifications.
Can that change perhaps be isolated into a separate patch?
> diff --git a/block/blk-core.c b/block/blk-core.c
> index 3d286a256d3d..1f7e9a90e198 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -570,7 +570,7 @@ static inline blk_status_t blk_check_zone_append(struct request_queue *q,
> return BLK_STS_NOTSUPP;
>
> /* The bio sector must point to the start of a sequential zone */
> - if (bio->bi_iter.bi_sector & (bdev_zone_sectors(bio->bi_bdev) - 1) ||
> + if (!bdev_is_zone_aligned(bio->bi_bdev, bio->bi_iter.bi_sector) ||
> !bio_zone_is_seq(bio))
> return BLK_STS_IOERR;
The bdev_is_zone_start() name seems more clear to me than
bdev_is_zone_aligned(). Has there already been a discussion about which
name to use for this function?
> + /*
> + * Non power-of-2 zone size support was added to remove the
> + * gap between zone capacity and zone size. Though it is technically
> + * possible to have gaps in a non power-of-2 device, Linux requires
> + * the zone size to be equal to zone capacity for non power-of-2
> + * zoned devices.
> + */
> + if (!is_power_of_2(zone->len) && zone->capacity < zone->len) {
> + pr_warn("%s: Invalid zone capacity for non power of 2 zone size",
> + disk->disk_name);
Given the severity of this error, shouldn't the zone capacity and length
be reported in the error message?
Thanks,
Bart.
On 7/28/22 01:22, Pankaj Raghav wrote:
> Checking if a given sector is aligned to a zone is a common
> operation that is performed for zoned devices. Add
> bdev_is_zone_start helper to check for this instead of opencoding it
The patch actually introduces bdev_is_zone_aligned(). I agree with Bart
that bdev_is_zone_start() is a better name.
> everywhere.
>
> Convert the calculations on zone size to be generic instead of relying on
> power_of_2 based logic in the block layer using the helpers wherever
s/based logic/arithmetics
> possible.
>
> The only hot path affected by this change for power_of_2 zoned devices
> is in blk_check_zone_append() but bdev_is_zone_start() helper is
> used to optimize the calculation for po2 zone sizes. Note that the append
> path cannot be accessed by direct raw access to the block device but only
> through a filesystem abstraction.
And so what ? What is the point here ?
>
> Finally, allow non power of 2 zoned devices provided that their zone
Please spell things out clearly: ...allow zoned devices with a zone size
that is not a power of 2 number of sectors...
> capacity and zone size are equal. The main motivation to allow non
> power_of_2 zoned device is to remove the unmapped LBA between zcap and
> zsze for devices that cannot have a power_of_2 zcap.
zcap, zsze are nvme field names. Please phrase these in plain english to
clarify.
>
> To make this work bdev_get_queue(), bdev_zone_sectors() and
> bdev_is_zoned() are moved earlier without modifications.
"moved earlier" -> declared earlier in xxx.h ?
>
> Reviewed-by: Luis Chamberlain <[email protected]>
> Reviewed-by: Hannes Reinecke <[email protected]>
> Signed-off-by: Pankaj Raghav <[email protected]>
> ---
> block/blk-core.c | 2 +-
> block/blk-zoned.c | 24 +++++++++---
> include/linux/blkdev.h | 84 ++++++++++++++++++++++++++++++------------
> 3 files changed, 79 insertions(+), 31 deletions(-)
>
> diff --git a/block/blk-core.c b/block/blk-core.c
> index 3d286a256d3d..1f7e9a90e198 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -570,7 +570,7 @@ static inline blk_status_t blk_check_zone_append(struct request_queue *q,
> return BLK_STS_NOTSUPP;
>
> /* The bio sector must point to the start of a sequential zone */
> - if (bio->bi_iter.bi_sector & (bdev_zone_sectors(bio->bi_bdev) - 1) ||
> + if (!bdev_is_zone_aligned(bio->bi_bdev, bio->bi_iter.bi_sector) ||
> !bio_zone_is_seq(bio))
> return BLK_STS_IOERR;
>
> diff --git a/block/blk-zoned.c b/block/blk-zoned.c
> index dce9c95b4bcd..a01a231ad328 100644
> --- a/block/blk-zoned.c
> +++ b/block/blk-zoned.c
> @@ -285,10 +285,10 @@ int blkdev_zone_mgmt(struct block_device *bdev, enum req_op op,
> return -EINVAL;
>
> /* Check alignment (handle eventual smaller last zone) */
> - if (sector & (zone_sectors - 1))
> + if (!bdev_is_zone_aligned(bdev, sector))
> return -EINVAL;
>
> - if ((nr_sectors & (zone_sectors - 1)) && end_sector != capacity)
> + if (!bdev_is_zone_aligned(bdev, nr_sectors) && end_sector != capacity)
> return -EINVAL;
>
> /*
> @@ -486,14 +486,26 @@ 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 zone size", disk->disk_name);
You removed the zone size value print, so please update the message to
something like:
pr_warn("%s: Invalid zero zone size", disk->disk_name);
> + return -ENODEV;
> + }
> +
> + /*
> + * Non power-of-2 zone size support was added to remove the
> + * gap between zone capacity and zone size. Though it is technically
> + * possible to have gaps in a non power-of-2 device, Linux requires
> + * the zone size to be equal to zone capacity for non power-of-2
> + * zoned devices.
> + */
> + if (!is_power_of_2(zone->len) && zone->capacity < zone->len) {
> + pr_warn("%s: Invalid zone capacity for non power of 2 zone size",
> + disk->disk_name);
As Bart suggested, please print the zone capacity and zone size values.
> 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);
args->nr_zones = disk_zone_no(disk, capacity);
> } 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",
> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
> index 85b832908f28..1be805223026 100644
> --- a/include/linux/blkdev.h
> +++ b/include/linux/blkdev.h
> @@ -634,6 +634,11 @@ static inline bool queue_is_mq(struct request_queue *q)
> return q->mq_ops;
> }
>
> +static inline struct request_queue *bdev_get_queue(struct block_device *bdev)
> +{
> + return bdev->bd_queue; /* this is never NULL */
> +}
> +
> #ifdef CONFIG_PM
> static inline enum rpm_status queue_rpm_status(struct request_queue *q)
> {
> @@ -665,6 +670,25 @@ static inline bool blk_queue_is_zoned(struct request_queue *q)
> }
> }
>
> +static inline bool bdev_is_zoned(struct block_device *bdev)
> +{
> + struct request_queue *q = bdev_get_queue(bdev);
> +
> + if (q)
> + return blk_queue_is_zoned(q);
> +
> + return false;
> +}
> +
> +static inline sector_t bdev_zone_sectors(struct block_device *bdev)
> +{
> + struct request_queue *q = bdev_get_queue(bdev);
> +
> + if (!blk_queue_is_zoned(q))
> + return 0;
> + return q->limits.chunk_sectors;
> +}
> +
> #ifdef CONFIG_BLK_DEV_ZONED
> static inline unsigned int disk_nr_zones(struct gendisk *disk)
> {
> @@ -684,6 +708,30 @@ static inline unsigned int disk_zone_no(struct gendisk *disk, sector_t sector)
> return div64_u64(sector, zone_sectors);
> }
>
> +static inline sector_t bdev_offset_from_zone_start(struct block_device *bdev,
> + sector_t sec)
> +{
> + sector_t zone_sectors = bdev_zone_sectors(bdev);
> + u64 remainder = 0;
> +
> + if (!bdev_is_zoned(bdev))
> + return 0;
> +
> + if (is_power_of_2(zone_sectors))
> + return sec & (zone_sectors - 1);
> +
> + div64_u64_rem(sec, zone_sectors, &remainder);
> + return remainder;
> +}
> +
> +static inline bool bdev_is_zone_aligned(struct block_device *bdev, sector_t sec)
> +{
> + if (!bdev_is_zoned(bdev))
> + return false;
This is checked in bdev_offset_from_zone_start(). No need to add it again
here.
> +
> + return bdev_offset_from_zone_start(bdev, sec) == 0;
> +}
> +
> static inline bool disk_zone_is_seq(struct gendisk *disk, sector_t sector)
> {
> if (!blk_queue_is_zoned(disk->queue))
> @@ -728,6 +776,18 @@ static inline unsigned int disk_zone_no(struct gendisk *disk, sector_t sector)
> {
> return 0;
> }
> +
> +static inline sector_t bdev_offset_from_zone_start(struct block_device *bdev,
> + sector_t sec)
> +{
> + return 0;
> +}
This one is not used when CONFIG_BLK_DEV_ZONED is not set. No need to
define it.
> +
> +static inline bool bdev_is_zone_aligned(struct block_device *bdev, sector_t sec)
> +{
> + return false;
> +}
> +
> static inline unsigned int bdev_max_open_zones(struct block_device *bdev)
> {
> return 0;
> @@ -891,11 +951,6 @@ int bio_poll(struct bio *bio, struct io_comp_batch *iob, unsigned int flags);
> int iocb_bio_iopoll(struct kiocb *kiocb, struct io_comp_batch *iob,
> unsigned int flags);
>
> -static inline struct request_queue *bdev_get_queue(struct block_device *bdev)
> -{
> - return bdev->bd_queue; /* this is never NULL */
> -}
> -
> /* Helper to convert BLK_ZONE_ZONE_XXX to its string format XXX */
> const char *blk_zone_cond_str(enum blk_zone_cond zone_cond);
>
> @@ -1295,25 +1350,6 @@ static inline enum blk_zoned_model bdev_zoned_model(struct block_device *bdev)
> return BLK_ZONED_NONE;
> }
>
> -static inline bool bdev_is_zoned(struct block_device *bdev)
> -{
> - struct request_queue *q = bdev_get_queue(bdev);
> -
> - if (q)
> - return blk_queue_is_zoned(q);
> -
> - return false;
> -}
> -
> -static inline sector_t bdev_zone_sectors(struct block_device *bdev)
> -{
> - struct request_queue *q = bdev_get_queue(bdev);
> -
> - if (!blk_queue_is_zoned(q))
> - return 0;
> - return q->limits.chunk_sectors;
> -}
> -
> static inline int queue_dma_alignment(const struct request_queue *q)
> {
> return q ? q->dma_alignment : 511;
--
Damien Le Moal
Western Digital Research
On 2022-07-28 01:16, Bart Van Assche wrote:
> On 7/27/22 09:22, Pankaj Raghav wrote:
>> Checking if a given sector is aligned to a zone is a common
>> operation that is performed for zoned devices. Add
>> bdev_is_zone_start helper to check for this instead of opencoding it
>> everywhere.
>
> I can't find the bdev_is_zone_start() function in this patch?
>
I made the name change from bdev_is_zone_start to bdev_is_zone_aligned
last moment and missed changing it in the commit log.
>> To make this work bdev_get_queue(), bdev_zone_sectors() and
>> bdev_is_zoned() are moved earlier without modifications.
>
> Can that change perhaps be isolated into a separate patch?
>
>> diff --git a/block/blk-core.c b/block/blk-core.c
>> index 3d286a256d3d..1f7e9a90e198 100644
>> --- a/block/blk-core.c
>> +++ b/block/blk-core.c
>> @@ -570,7 +570,7 @@ static inline blk_status_t
>> blk_check_zone_append(struct request_queue *q,
>> return BLK_STS_NOTSUPP;
>> /* The bio sector must point to the start of a sequential zone */
>> - if (bio->bi_iter.bi_sector & (bdev_zone_sectors(bio->bi_bdev) -
>> 1) ||
>> + if (!bdev_is_zone_aligned(bio->bi_bdev, bio->bi_iter.bi_sector) ||
>> !bio_zone_is_seq(bio))
>> return BLK_STS_IOERR;
>
> The bdev_is_zone_start() name seems more clear to me than
> bdev_is_zone_aligned(). Has there already been a discussion about which
> name to use for this function?
>
The reason I did s/bdev_is_zone_start/bdev_is_zone_aligned is that this
name makes more sense for also checking if a given size is a multiple of
zone sectors for e.g., used in PATCH 9:
- if (len & (zone_sectors - 1)) {
+ if (!bdev_is_zone_aligned(bdev, len)) {
I felt `bdev_is_zone_aligned` fits the use case of checking if the
sector starts at the start of a zone and also check if a given length of
sectors also align with the zone sectors. bdev_is_zone_start does not
make the intention clear for the latter use case IMO.
But I am fine with going back to bdev_is_zone_start if you and Damien
feel strongly otherwise.
>> + /*
>> + * Non power-of-2 zone size support was added to remove the
>> + * gap between zone capacity and zone size. Though it is
>> technically
>> + * possible to have gaps in a non power-of-2 device, Linux
>> requires
>> + * the zone size to be equal to zone capacity for non power-of-2
>> + * zoned devices.
>> + */
>> + if (!is_power_of_2(zone->len) && zone->capacity < zone->len) {
>> + pr_warn("%s: Invalid zone capacity for non power of 2
>> zone size",
>> + disk->disk_name);
>
> Given the severity of this error, shouldn't the zone capacity and length
> be reported in the error message?
>
Ok.
> Thanks,
>
> Bart.
On 2022-07-28 05:07, Damien Le Moal wrote:
> On 7/28/22 01:22, Pankaj Raghav wrote:
>> Checking if a given sector is aligned to a zone is a common
>> operation that is performed for zoned devices. Add
>> bdev_is_zone_start helper to check for this instead of opencoding it
>
> The patch actually introduces bdev_is_zone_aligned(). I agree with Bart
> that bdev_is_zone_start() is a better name.
I have posted my rationale behind this change in my reply to Bart. Let
me know what you think.
>
<snip>
>> 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);
>
> args->nr_zones = disk_zone_no(disk, capacity);
>
We are doing a round up with a division here mainly to take into account
the last unequal zone if present. disk_zone_no does just a division so
it won't account for the unequal last zone.
>> } 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",
>> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
>> index 85b832908f28..1be805223026 100644
>> --- a/include/linux/blkdev.h
>> +++ b/include/linux/blkdev.h
>> @@ -634,6 +634,11 @@ static inline bool queue_is_mq(struct request_queue *q)
>> return q->mq_ops;
>> }
>>
>> +static inline struct request_queue *bdev_get_queue(struct block_device *bdev)
>> +{
>> + return bdev->bd_queue; /* this is never NULL */
>> +}
>> +
>> #ifdef CONFIG_PM
>> static inline enum rpm_status queue_rpm_status(struct request_queue *q)
>> {
>> @@ -665,6 +670,25 @@ static inline bool blk_queue_is_zoned(struct request_queue *q)
>> }
>> }
>>
>> +static inline bool bdev_is_zoned(struct block_device *bdev)
>> +{
>> + struct request_queue *q = bdev_get_queue(bdev);
>> +
>> + if (q)
>> + return blk_queue_is_zoned(q);
>> +
>> + return false;
>> +}
>> +
>> +static inline sector_t bdev_zone_sectors(struct block_device *bdev)
>> +{
>> + struct request_queue *q = bdev_get_queue(bdev);
>> +
>> + if (!blk_queue_is_zoned(q))
>> + return 0;
>> + return q->limits.chunk_sectors;
>> +}
>> +
>> #ifdef CONFIG_BLK_DEV_ZONED
>> static inline unsigned int disk_nr_zones(struct gendisk *disk)
>> {
>> @@ -684,6 +708,30 @@ static inline unsigned int disk_zone_no(struct gendisk *disk, sector_t sector)
>> return div64_u64(sector, zone_sectors);
>> }
>>
>> +static inline sector_t bdev_offset_from_zone_start(struct block_device *bdev,
>> + sector_t sec)
>> +{
>> + sector_t zone_sectors = bdev_zone_sectors(bdev);
>> + u64 remainder = 0;
>> +
>> + if (!bdev_is_zoned(bdev))
>> + return 0;
>> +
>> + if (is_power_of_2(zone_sectors))
>> + return sec & (zone_sectors - 1);
>> +
>> + div64_u64_rem(sec, zone_sectors, &remainder);
>> + return remainder;
>> +}
>> +
>> +static inline bool bdev_is_zone_aligned(struct block_device *bdev, sector_t sec)
>> +{
>> + if (!bdev_is_zoned(bdev))
>> + return false;
>
> This is checked in bdev_offset_from_zone_start(). No need to add it again
> here.
>
bdev_offset_from_zone_start returns 0 if the device is not zoned, and
the below check will then return `true`. That is why I explicitly return
a false if the device is not zoned.
>> +
>> + return bdev_offset_from_zone_start(bdev, sec) == 0;
>> +}
>> +
>> static inline bool disk_zone_is_seq(struct gendisk *disk, sector_t sector)
>> {
>> if (!blk_queue_is_zoned(disk->queue))
>> @@ -728,6 +776,18 @@ static inline unsigned int disk_zone_no(struct gendisk *disk, sector_t sector)
>> {
>> return 0;
>> }
>> +
>> +static inline sector_t bdev_offset_from_zone_start(struct block_device *bdev,
>> + sector_t sec)
>> +{
>> + return 0;
>> +}
>
> This one is not used when CONFIG_BLK_DEV_ZONED is not set. No need to
> define it.
>
Ok. I will remove it if it is not required.
>> +
>> +static inline bool bdev_is_zone_aligned(struct block_device *bdev, sector_t sec)
>> +{
>
On 7/28/22 05:11, Pankaj Raghav wrote:
> On 2022-07-28 01:16, Bart Van Assche wrote:
>> The bdev_is_zone_start() name seems more clear to me than
>> bdev_is_zone_aligned(). Has there already been a discussion about which
>> name to use for this function?
>>
> The reason I did s/bdev_is_zone_start/bdev_is_zone_aligned is that this
> name makes more sense for also checking if a given size is a multiple of
> zone sectors for e.g., used in PATCH 9:
>
> - if (len & (zone_sectors - 1)) {
> + if (!bdev_is_zone_aligned(bdev, len)) {
>
> I felt `bdev_is_zone_aligned` fits the use case of checking if the
> sector starts at the start of a zone and also check if a given length of
> sectors also align with the zone sectors. bdev_is_zone_start does not
> make the intention clear for the latter use case IMO.
>
> But I am fine with going back to bdev_is_zone_start if you and Damien
> feel strongly otherwise.
The "zone start LBA" terminology occurs in ZBC-1, ZBC-2 and ZNS but
"zone aligned" not. I prefer "zone start" because it is clear,
unambiguous and because it has the same meaning as in the corresponding
standards documents. I propose to proceed as follows for checking
whether a number of LBAs is a multiple of the zone length:
* Either use bdev_is_zone_start() directly.
* Or introduce a synonym for bdev_is_zone_start() with an appropriate
name, e.g. bdev_is_zone_len_multiple().
Thanks,
Bart.
On 2022-07-28 15:29, Bart Van Assche wrote:
>> But I am fine with going back to bdev_is_zone_start if you and Damien
>> feel strongly otherwise.
> The "zone start LBA" terminology occurs in ZBC-1, ZBC-2 and ZNS but
> "zone aligned" not. I prefer "zone start" because it is clear,
> unambiguous and because it has the same meaning as in the corresponding
> standards documents. I propose to proceed as follows for checking
> whether a number of LBAs is a multiple of the zone length:
> * Either use bdev_is_zone_start() directly.
> * Or introduce a synonym for bdev_is_zone_start() with an appropriate
> name, e.g. bdev_is_zone_len_multiple().
>
Thanks for the clarification Bart. I will go with bdev_is_zone_start()
as it is also a commonly used terminology.
> Thanks,
>
> Bart.