The zone size shift variable is useful only if the zone sizes are known
to be power of 2. Remove that variable and use generic helpers from
block layer to calculate zone index in zonefs
Reviewed-by: Luis Chamberlain <[email protected]>
Signed-off-by: Pankaj Raghav <[email protected]>
---
fs/zonefs/super.c | 6 ++----
fs/zonefs/zonefs.h | 1 -
2 files changed, 2 insertions(+), 5 deletions(-)
diff --git a/fs/zonefs/super.c b/fs/zonefs/super.c
index 3614c7834007..5422be2ca570 100644
--- a/fs/zonefs/super.c
+++ b/fs/zonefs/super.c
@@ -401,10 +401,9 @@ static void __zonefs_io_error(struct inode *inode, bool write)
{
struct zonefs_inode_info *zi = ZONEFS_I(inode);
struct super_block *sb = inode->i_sb;
- struct zonefs_sb_info *sbi = ZONEFS_SB(sb);
unsigned int noio_flag;
unsigned int nr_zones =
- zi->i_zone_size >> (sbi->s_zone_sectors_shift + SECTOR_SHIFT);
+ bdev_zone_no(sb->s_bdev, zi->i_zone_size >> SECTOR_SHIFT);
struct zonefs_ioerr_data err = {
.inode = inode,
.write = write,
@@ -1300,7 +1299,7 @@ static void zonefs_init_file_inode(struct inode *inode, struct blk_zone *zone,
struct zonefs_sb_info *sbi = ZONEFS_SB(sb);
struct zonefs_inode_info *zi = ZONEFS_I(inode);
- inode->i_ino = zone->start >> sbi->s_zone_sectors_shift;
+ inode->i_ino = bdev_zone_no(sb->s_bdev, zone->start);
inode->i_mode = S_IFREG | sbi->s_perm;
zi->i_ztype = type;
@@ -1647,7 +1646,6 @@ static int zonefs_fill_super(struct super_block *sb, void *data, int silent)
* interface constraints.
*/
sb_set_blocksize(sb, bdev_zone_write_granularity(sb->s_bdev));
- sbi->s_zone_sectors_shift = ilog2(bdev_zone_sectors(sb->s_bdev));
sbi->s_uid = GLOBAL_ROOT_UID;
sbi->s_gid = GLOBAL_ROOT_GID;
sbi->s_perm = 0640;
diff --git a/fs/zonefs/zonefs.h b/fs/zonefs/zonefs.h
index 7b147907c328..2d5ea3be3a8e 100644
--- a/fs/zonefs/zonefs.h
+++ b/fs/zonefs/zonefs.h
@@ -175,7 +175,6 @@ struct zonefs_sb_info {
kgid_t s_gid;
umode_t s_perm;
uuid_t s_uuid;
- unsigned int s_zone_sectors_shift;
unsigned int s_nr_files[ZONEFS_ZTYPE_MAX];
--
2.25.1
On 4/28/22 01:02, Pankaj Raghav wrote:
> The zone size shift variable is useful only if the zone sizes are known
> to be power of 2. Remove that variable and use generic helpers from
> block layer to calculate zone index in zonefs
Period missing at the end of the sentence.
What about zonefs-tools and its test suite ? Is everything still OK on
that front ? I suspect not...
>
> Reviewed-by: Luis Chamberlain <[email protected]>
> Signed-off-by: Pankaj Raghav <[email protected]>
> ---
> fs/zonefs/super.c | 6 ++----
> fs/zonefs/zonefs.h | 1 -
> 2 files changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/fs/zonefs/super.c b/fs/zonefs/super.c
> index 3614c7834007..5422be2ca570 100644
> --- a/fs/zonefs/super.c
> +++ b/fs/zonefs/super.c
> @@ -401,10 +401,9 @@ static void __zonefs_io_error(struct inode *inode, bool write)
> {
> struct zonefs_inode_info *zi = ZONEFS_I(inode);
> struct super_block *sb = inode->i_sb;
> - struct zonefs_sb_info *sbi = ZONEFS_SB(sb);
> unsigned int noio_flag;
> unsigned int nr_zones =
> - zi->i_zone_size >> (sbi->s_zone_sectors_shift + SECTOR_SHIFT);
> + bdev_zone_no(sb->s_bdev, zi->i_zone_size >> SECTOR_SHIFT);
> struct zonefs_ioerr_data err = {
> .inode = inode,
> .write = write,
> @@ -1300,7 +1299,7 @@ static void zonefs_init_file_inode(struct inode *inode, struct blk_zone *zone,
> struct zonefs_sb_info *sbi = ZONEFS_SB(sb);
> struct zonefs_inode_info *zi = ZONEFS_I(inode);
>
> - inode->i_ino = zone->start >> sbi->s_zone_sectors_shift;
> + inode->i_ino = bdev_zone_no(sb->s_bdev, zone->start);
> inode->i_mode = S_IFREG | sbi->s_perm;
>
> zi->i_ztype = type;
> @@ -1647,7 +1646,6 @@ static int zonefs_fill_super(struct super_block *sb, void *data, int silent)
> * interface constraints.
> */
> sb_set_blocksize(sb, bdev_zone_write_granularity(sb->s_bdev));
> - sbi->s_zone_sectors_shift = ilog2(bdev_zone_sectors(sb->s_bdev));
> sbi->s_uid = GLOBAL_ROOT_UID;
> sbi->s_gid = GLOBAL_ROOT_GID;
> sbi->s_perm = 0640;
> diff --git a/fs/zonefs/zonefs.h b/fs/zonefs/zonefs.h
> index 7b147907c328..2d5ea3be3a8e 100644
> --- a/fs/zonefs/zonefs.h
> +++ b/fs/zonefs/zonefs.h
> @@ -175,7 +175,6 @@ struct zonefs_sb_info {
> kgid_t s_gid;
> umode_t s_perm;
> uuid_t s_uuid;
> - unsigned int s_zone_sectors_shift;
>
> unsigned int s_nr_files[ZONEFS_ZTYPE_MAX];
>
--
Damien Le Moal
Western Digital Research
On 2022-04-28 01:39, Damien Le Moal wrote:
> On 4/28/22 01:02, Pankaj Raghav wrote:
>> The zone size shift variable is useful only if the zone sizes are known
>> to be power of 2. Remove that variable and use generic helpers from
>> block layer to calculate zone index in zonefs
>
> Period missing at the end of the sentence.
>
Ack
> What about zonefs-tools and its test suite ? Is everything still OK on
> that front ? I suspect not...
>
I don't know why you assume that :). Zonefs had only one place that had
the assumption of po2 zsze sectors:
if (nr_zones < dev.nr_zones) {
size_t runt_sectors = dev.capacity & (dev.zone_nr_sectors - 1);
In my local tree I changed it and I was able to run zonefs tests for non
po2 zone device. I have also mentioned it in my cover letter:
```
ZoneFS:
zonefs-tests.sh from zonefs-tools were run with no failures.
```
I will make sure to add my private tree for zonefs in my cover letter in
the next rev. But even without that change, a typical emulated npo2
device should work fine because the changes are applicable only for
"runt" zones.
On 4/29/22 00:54, Pankaj Raghav wrote:
> On 2022-04-28 01:39, Damien Le Moal wrote:
>> On 4/28/22 01:02, Pankaj Raghav wrote:
>>> The zone size shift variable is useful only if the zone sizes are known
>>> to be power of 2. Remove that variable and use generic helpers from
>>> block layer to calculate zone index in zonefs
>>
>> Period missing at the end of the sentence.
>>
> Ack
>> What about zonefs-tools and its test suite ? Is everything still OK on
>> that front ? I suspect not...
>>
> I don't know why you assume that :). Zonefs had only one place that had
> the assumption of po2 zsze sectors:
> if (nr_zones < dev.nr_zones) {
> size_t runt_sectors = dev.capacity & (dev.zone_nr_sectors - 1);
>
> In my local tree I changed it and I was able to run zonefs tests for non
> po2 zone device. I have also mentioned it in my cover letter:
> ```
> ZoneFS:
> zonefs-tests.sh from zonefs-tools were run with no failures.
> ```
This is still not convincing given the code I saw. Additional test cases
need to be added with data verification & concurrent regular writes also
sent while doing copy to verify locking.
Which also reminds me that I have not seen any change to mq-deadline zone
write locking for this series. What is the assumption ? That users should
not be issuing writes when a copy is on-going ? What a bout the reverse
case ? at the very least, it seems that blk_issue_copy() should be taking
the zone write lock.
> I will make sure to add my private tree for zonefs in my cover letter in
> the next rev. But even without that change, a typical emulated npo2
> device should work fine because the changes are applicable only for
> "runt" zones.
--
Damien Le Moal
Western Digital Research
Hi Damien,
On 2022-04-28 23:49, Damien Le Moal wrote:
> This is still not convincing given the code I saw. Additional test cases
> need to be added with data verification & concurrent regular writes also
> sent while doing copy to verify locking.
>
> Which also reminds me that I have not seen any change to mq-deadline zone
> write locking for this series. What is the assumption ? That users should
> not be issuing writes when a copy is on-going ? What a bout the reverse
> case ? at the very least, it seems that blk_issue_copy() should be taking
> the zone write lock.
>
I think you posted this comment in this thread instead of posting it in
the copy offload thread.
>> I will make sure to add my private tree for zonefs in my cover letter in
>> the next rev. But even without that change, a typical emulated npo2
>> device should work fine because the changes are applicable only for
>> "runt" zones.
>
>