| Message ID | 335b0d7cd8c0e7492332273a330807a8130e213e.1741596325.git.naohiro.aota@wdc.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | btrfs: zoned: skip reporting zone for new block group | expand |
On 3/12/25 10:31, Naohiro Aota wrote: > {bdev,disk}_zone_capacity() takes block_device or gendisk and sector position > and returns the zone capacity of the corresponding zone. > > Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com> > --- > include/linux/blkdev.h | 21 +++++++++++++++++++++ > 1 file changed, 21 insertions(+) > > diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h > index d37751789bf5..3c860a0cf339 100644 > --- a/include/linux/blkdev.h > +++ b/include/linux/blkdev.h > @@ -826,6 +826,27 @@ static inline u64 sb_bdev_nr_blocks(struct super_block *sb) > (sb->s_blocksize_bits - SECTOR_SHIFT); > } > > +#ifdef CONFIG_BLK_DEV_ZONED There is already an "#ifdef CONFIG_BLK_DEV_ZONED" in blkdev.h, see disk_nr_zones(). Please add this new helper under that ifdef to avoid adding a new one. > +static inline unsigned int disk_zone_capacity(struct gendisk *disk, sector_t pos) > +{ > + sector_t zone_sectors = disk->queue->limits.chunk_sectors; > + > + if (pos + zone_sectors >= get_capacity(disk)) > + return disk->last_zone_capacity; > + return disk->zone_capacity; > +} > +#else /* CONFIG_BLK_DEV_ZONED */ > +static inline unsigned int disk_zone_capacity(struct gendisk *disk, sector_t pos) Is this ever called for a non zoned drive ? It should not be... So do we really need this stub ? > +{ > + return 0; > +} > +#endif /* CONFIG_BLK_DEV_ZONED */ > + > +static inline unsigned int bdev_zone_capacity(struct block_device *bdev, sector_t pos) > +{ > + return disk_zone_capacity(bdev->bd_disk, pos); > +} > + > int bdev_disk_changed(struct gendisk *disk, bool invalidate); > > void put_disk(struct gendisk *disk);
On Wed Mar 12, 2025 at 10:39 AM JST, Damien Le Moal wrote: > On 3/12/25 10:31, Naohiro Aota wrote: >> {bdev,disk}_zone_capacity() takes block_device or gendisk and sector position >> and returns the zone capacity of the corresponding zone. >> >> Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com> >> --- >> include/linux/blkdev.h | 21 +++++++++++++++++++++ >> 1 file changed, 21 insertions(+) >> >> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h >> index d37751789bf5..3c860a0cf339 100644 >> --- a/include/linux/blkdev.h >> +++ b/include/linux/blkdev.h >> @@ -826,6 +826,27 @@ static inline u64 sb_bdev_nr_blocks(struct super_block *sb) >> (sb->s_blocksize_bits - SECTOR_SHIFT); >> } >> >> +#ifdef CONFIG_BLK_DEV_ZONED > > There is already an "#ifdef CONFIG_BLK_DEV_ZONED" in blkdev.h, see > disk_nr_zones(). Please add this new helper under that ifdef to avoid adding a > new one. Sure. But, since this patch uses get_capacity(), we need to move the existing ones here instead. > >> +static inline unsigned int disk_zone_capacity(struct gendisk *disk, sector_t pos) >> +{ >> + sector_t zone_sectors = disk->queue->limits.chunk_sectors; >> + >> + if (pos + zone_sectors >= get_capacity(disk)) >> + return disk->last_zone_capacity; >> + return disk->zone_capacity; >> +} >> +#else /* CONFIG_BLK_DEV_ZONED */ >> +static inline unsigned int disk_zone_capacity(struct gendisk *disk, sector_t pos) > > Is this ever called for a non zoned drive ? It should not be... So do we really > need this stub ? I added that just let it compile without CONFIG_BLK_DEV_ZONED. Well, the code querying the capacity should be under CONFIG_BLK_DEV_ZONED, so maybe it is not necessary. I'll drop that. > >> +{ >> + return 0; >> +} >> +#endif /* CONFIG_BLK_DEV_ZONED */ >> + >> +static inline unsigned int bdev_zone_capacity(struct block_device *bdev, sector_t pos) >> +{ >> + return disk_zone_capacity(bdev->bd_disk, pos); >> +} >> + >> int bdev_disk_changed(struct gendisk *disk, bool invalidate); >> >> void put_disk(struct gendisk *disk);
On Wed, Mar 12, 2025 at 10:31:03AM +0900, Naohiro Aota wrote: > +#ifdef CONFIG_BLK_DEV_ZONED > +static inline unsigned int disk_zone_capacity(struct gendisk *disk, sector_t pos) Overly long line. > +{ > + sector_t zone_sectors = disk->queue->limits.chunk_sectors; > + > + if (pos + zone_sectors >= get_capacity(disk)) > + return disk->last_zone_capacity; > + return disk->zone_capacity; But I also don't understand how pos plays in here. Maybe add a kerneldoc comment describing what the function is supposed to do?
On 3/12/25 14:27, Christoph Hellwig wrote: > On Wed, Mar 12, 2025 at 10:31:03AM +0900, Naohiro Aota wrote: >> +#ifdef CONFIG_BLK_DEV_ZONED >> +static inline unsigned int disk_zone_capacity(struct gendisk *disk, sector_t pos) > > Overly long line. > >> +{ >> + sector_t zone_sectors = disk->queue->limits.chunk_sectors; >> + >> + if (pos + zone_sectors >= get_capacity(disk)) >> + return disk->last_zone_capacity; >> + return disk->zone_capacity; > > But I also don't understand how pos plays in here. Maybe add a > kerneldoc comment describing what the function is supposed to do? The last zone can be smaller than all other zones, hence we have disk->zone_capacity and disk->last_zone_capacity. Pos is a sector used to indicate the zone for which you want the capacity. But yes, agreed, a kernel doc would be nice to clarify that.
On Wed, Mar 12, 2025 at 03:55:31PM +0900, Damien Le Moal wrote: > On 3/12/25 14:27, Christoph Hellwig wrote: > > On Wed, Mar 12, 2025 at 10:31:03AM +0900, Naohiro Aota wrote: > >> +#ifdef CONFIG_BLK_DEV_ZONED > >> +static inline unsigned int disk_zone_capacity(struct gendisk *disk, sector_t pos) > > > > Overly long line. > > > >> +{ > >> + sector_t zone_sectors = disk->queue->limits.chunk_sectors; > >> + > >> + if (pos + zone_sectors >= get_capacity(disk)) > >> + return disk->last_zone_capacity; > >> + return disk->zone_capacity; > > > > But I also don't understand how pos plays in here. Maybe add a > > kerneldoc comment describing what the function is supposed to do? > > The last zone can be smaller than all other zones, hence we have > disk->zone_capacity and disk->last_zone_capacity. Pos is a sector used to > indicate the zone for which you want the capacity. > > But yes, agreed, a kernel doc would be nice to clarify that. Should it be zoned_start then to make that obvious?
On 3/12/25 16:00, Christoph Hellwig wrote: > On Wed, Mar 12, 2025 at 03:55:31PM +0900, Damien Le Moal wrote: >> On 3/12/25 14:27, Christoph Hellwig wrote: >>> On Wed, Mar 12, 2025 at 10:31:03AM +0900, Naohiro Aota wrote: >>>> +#ifdef CONFIG_BLK_DEV_ZONED >>>> +static inline unsigned int disk_zone_capacity(struct gendisk *disk, sector_t pos) >>> >>> Overly long line. >>> >>>> +{ >>>> + sector_t zone_sectors = disk->queue->limits.chunk_sectors; >>>> + >>>> + if (pos + zone_sectors >= get_capacity(disk)) >>>> + return disk->last_zone_capacity; >>>> + return disk->zone_capacity; >>> >>> But I also don't understand how pos plays in here. Maybe add a >>> kerneldoc comment describing what the function is supposed to do? >> >> The last zone can be smaller than all other zones, hence we have >> disk->zone_capacity and disk->last_zone_capacity. Pos is a sector used to >> indicate the zone for which you want the capacity. >> >> But yes, agreed, a kernel doc would be nice to clarify that. > > Should it be zoned_start then to make that obvious? Well, it does not have to be the zone start. It can be any sector contained in the zone you are interested in. With a comment, that should be clear, and also the same as helpers such as disk_zone_no() which do not require the zone start sector.
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h index d37751789bf5..3c860a0cf339 100644 --- a/include/linux/blkdev.h +++ b/include/linux/blkdev.h @@ -826,6 +826,27 @@ static inline u64 sb_bdev_nr_blocks(struct super_block *sb) (sb->s_blocksize_bits - SECTOR_SHIFT); } +#ifdef CONFIG_BLK_DEV_ZONED +static inline unsigned int disk_zone_capacity(struct gendisk *disk, sector_t pos) +{ + sector_t zone_sectors = disk->queue->limits.chunk_sectors; + + if (pos + zone_sectors >= get_capacity(disk)) + return disk->last_zone_capacity; + return disk->zone_capacity; +} +#else /* CONFIG_BLK_DEV_ZONED */ +static inline unsigned int disk_zone_capacity(struct gendisk *disk, sector_t pos) +{ + return 0; +} +#endif /* CONFIG_BLK_DEV_ZONED */ + +static inline unsigned int bdev_zone_capacity(struct block_device *bdev, sector_t pos) +{ + return disk_zone_capacity(bdev->bd_disk, pos); +} + int bdev_disk_changed(struct gendisk *disk, bool invalidate); void put_disk(struct gendisk *disk);
{bdev,disk}_zone_capacity() takes block_device or gendisk and sector position and returns the zone capacity of the corresponding zone. Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com> --- include/linux/blkdev.h | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+)