| Message ID | 20220427160255.300418-5-p.raghav@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | support non power of 2 zoned devices | expand |
On 4/28/22 01:02, Pankaj Raghav wrote: > 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 the effects should be negligible as the > helper blk_queue_zone_aligned() optimizes the calculation for those > devices. Note that the append path cannot be accessed by direct raw access > to the block device but only through a filesystem abstraction. > > Finally, remove the check for power_of_2 zone size requirement in > blk-zoned.c > > Reviewed-by: Luis Chamberlain <mcgrof@kernel.org> > Signed-off-by: Pankaj Raghav <p.raghav@samsung.com> > --- > block/blk-core.c | 3 +-- > block/blk-zoned.c | 12 ++++++------ > 2 files changed, 7 insertions(+), 8 deletions(-) > > diff --git a/block/blk-core.c b/block/blk-core.c > index 937bb6b86331..850caf311064 100644 > --- a/block/blk-core.c > +++ b/block/blk-core.c > @@ -634,8 +634,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 (pos & (blk_queue_zone_sectors(q) - 1) || > - !blk_queue_zone_is_seq(q, pos)) > + if (!blk_queue_zone_aligned(q, pos) || !blk_queue_zone_is_seq(q, pos)) blk_queue_zone_aligned() is a little confusing since "aligned" is also used for write-pointer aligned. I would rename this helper blk_queue_is_zone_start() or something like that. > return BLK_STS_IOERR; > > /* > diff --git a/block/blk-zoned.c b/block/blk-zoned.c > index 1dff4a8bd51d..f7c7c3bd148d 100644 > --- a/block/blk-zoned.c > +++ b/block/blk-zoned.c > @@ -288,10 +288,10 @@ int blkdev_zone_mgmt(struct block_device *bdev, enum req_opf op, > return -EINVAL; > > /* Check alignment (handle eventual smaller last zone) */ > - if (sector & (zone_sectors - 1)) > + if (!blk_queue_zone_aligned(q, sector)) > return -EINVAL; > > - if ((nr_sectors & (zone_sectors - 1)) && end_sector != capacity) > + if (!blk_queue_zone_aligned(q, nr_sectors) && end_sector != capacity) > return -EINVAL; > > /* > @@ -489,14 +489,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 size", > + disk->disk_name); The message is weird now. Please change it to "Invalid zone size". Also, the entire premise of this patch series is that it is hard for people to support the unusable sectors between zone capacity and zone end for drives with a zone capacity smaller than the zone size. Yet, here you do not check that zone capacity == zone size for drives that do not have a zone size equal to a power of 2 number of sectors. This means that we can still have drives with ZC < ZS AND ZS not equal to a power of 2. So from the point of view of your arguments, no gains at all. Any thoughts on this ? > 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",
On Thu, Apr 28, 2022 at 08:37:27AM +0900, Damien Le Moal wrote: > Also, the entire premise of this patch series is that it is hard for > people to support the unusable sectors between zone capacity and zone end > for drives with a zone capacity smaller than the zone size. > > Yet, here you do not check that zone capacity == zone size for drives that > do not have a zone size equal to a power of 2 number of sectors. This > means that we can still have drives with ZC < ZS AND ZS not equal to a > power of 2. So from the point of view of your arguments, no gains at all. > Any thoughts on this ? You are right, a check should be added on bringup so that if npo2 is used, zone cap == zone size. That should be added on the next iteration of this patch. Luis
On 4/27/22 09:02, Pankaj Raghav wrote: > 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 the effects should be negligible as the > helper blk_queue_zone_aligned() optimizes the calculation for those > devices. Note that the append path cannot be accessed by direct raw access > to the block device but only through a filesystem abstraction. > > Finally, remove the check for power_of_2 zone size requirement in > blk-zoned.c > > Reviewed-by: Luis Chamberlain <mcgrof@kernel.org> > Signed-off-by: Pankaj Raghav <p.raghav@samsung.com> > --- > block/blk-core.c | 3 +-- > block/blk-zoned.c | 12 ++++++------ > 2 files changed, 7 insertions(+), 8 deletions(-) > > diff --git a/block/blk-core.c b/block/blk-core.c > index 937bb6b86331..850caf311064 100644 > --- a/block/blk-core.c > +++ b/block/blk-core.c > @@ -634,8 +634,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 (pos & (blk_queue_zone_sectors(q) - 1) || > - !blk_queue_zone_is_seq(q, pos)) > + if (!blk_queue_zone_aligned(q, pos) || !blk_queue_zone_is_seq(q, pos)) > return BLK_STS_IOERR; > > /* > diff --git a/block/blk-zoned.c b/block/blk-zoned.c > index 1dff4a8bd51d..f7c7c3bd148d 100644 > --- a/block/blk-zoned.c > +++ b/block/blk-zoned.c > @@ -288,10 +288,10 @@ int blkdev_zone_mgmt(struct block_device *bdev, enum req_opf op, > return -EINVAL; > > /* Check alignment (handle eventual smaller last zone) */ > - if (sector & (zone_sectors - 1)) > + if (!blk_queue_zone_aligned(q, sector)) > return -EINVAL; > > - if ((nr_sectors & (zone_sectors - 1)) && end_sector != capacity) > + if (!blk_queue_zone_aligned(q, nr_sectors) && end_sector != capacity) > return -EINVAL; > > /* > @@ -489,14 +489,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 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); This is a different calculation than the one you're using in the first patch. Can you please add a helper such that both are using the same calculation? Cheers, Hannes
diff --git a/block/blk-core.c b/block/blk-core.c index 937bb6b86331..850caf311064 100644 --- a/block/blk-core.c +++ b/block/blk-core.c @@ -634,8 +634,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 (pos & (blk_queue_zone_sectors(q) - 1) || - !blk_queue_zone_is_seq(q, pos)) + if (!blk_queue_zone_aligned(q, pos) || !blk_queue_zone_is_seq(q, pos)) return BLK_STS_IOERR; /* diff --git a/block/blk-zoned.c b/block/blk-zoned.c index 1dff4a8bd51d..f7c7c3bd148d 100644 --- a/block/blk-zoned.c +++ b/block/blk-zoned.c @@ -288,10 +288,10 @@ int blkdev_zone_mgmt(struct block_device *bdev, enum req_opf op, return -EINVAL; /* Check alignment (handle eventual smaller last zone) */ - if (sector & (zone_sectors - 1)) + if (!blk_queue_zone_aligned(q, sector)) return -EINVAL; - if ((nr_sectors & (zone_sectors - 1)) && end_sector != capacity) + if (!blk_queue_zone_aligned(q, nr_sectors) && end_sector != capacity) return -EINVAL; /* @@ -489,14 +489,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 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",