| Message ID | 20220427160255.300418-13-p.raghav@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/16] block: make blkdev_nr_zones and blk_queue_zone_no generic for npo2 zsze | expand |
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 <mcgrof@kernel.org> > Signed-off-by: Pankaj Raghav <p.raghav@samsung.com> > --- > 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]; >
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.
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. > >
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];