| Message ID | a7f717432896b5f12847435727838b32bd6e2b35.1624905177.git.johannes.thumshirn@wdc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] btrfs: zoned: remove fs_info->max_zone_append_size | expand |
On Tue, Jun 29, 2021 at 03:36:45AM +0900, Johannes Thumshirn wrote: > Remove fs_info->max_zone_append_size, it doesn't serve any purpose. Why was it added then? Commit 862931c76327 ("btrfs: introduce max_zone_append_size") states some reasons so you should explain why it's not needed now. It's a partial revert of the commit. > Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com> > > --- > Changes to v1: > - also remove the local max_zone_append_size variable in > btrfs_check_zoned_mode() (Anand) > --- > fs/btrfs/ctree.h | 2 -- > fs/btrfs/extent_io.c | 1 - > fs/btrfs/zoned.c | 10 ---------- > 3 files changed, 13 deletions(-) > > diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h > index d7ef4d7d2c1a..7a9cf4d12157 100644 > --- a/fs/btrfs/ctree.h > +++ b/fs/btrfs/ctree.h > @@ -1014,8 +1014,6 @@ struct btrfs_fs_info { > u64 zoned; > }; > > - /* Max size to emit ZONE_APPEND write command */ > - u64 max_zone_append_size; > struct mutex zoned_meta_io_lock; > spinlock_t treelog_bg_lock; > u64 treelog_bg; > diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c > index 9e81d25dea70..1f947e24091a 100644 > --- a/fs/btrfs/extent_io.c > +++ b/fs/btrfs/extent_io.c > @@ -3266,7 +3266,6 @@ static int calc_bio_boundaries(struct btrfs_bio_ctrl *bio_ctrl, > return 0; > } > > - ASSERT(fs_info->max_zone_append_size > 0); > /* Ordered extent not yet created, so we're good */ > ordered = btrfs_lookup_ordered_extent(inode, logical); > if (!ordered) { > diff --git a/fs/btrfs/zoned.c b/fs/btrfs/zoned.c > index 297c0b1c0634..76754e441e20 100644 > --- a/fs/btrfs/zoned.c > +++ b/fs/btrfs/zoned.c > @@ -529,7 +529,6 @@ int btrfs_check_zoned_mode(struct btrfs_fs_info *fs_info) > u64 zoned_devices = 0; > u64 nr_devices = 0; > u64 zone_size = 0; > - u64 max_zone_append_size = 0; > const bool incompat_zoned = btrfs_fs_incompat(fs_info, ZONED); > int ret = 0; > > @@ -565,11 +564,6 @@ int btrfs_check_zoned_mode(struct btrfs_fs_info *fs_info) > ret = -EINVAL; > goto out; > } > - if (!max_zone_append_size || > - (zone_info->max_zone_append_size && > - zone_info->max_zone_append_size < max_zone_append_size)) > - max_zone_append_size = > - zone_info->max_zone_append_size; > } > nr_devices++; > } > @@ -619,7 +613,6 @@ int btrfs_check_zoned_mode(struct btrfs_fs_info *fs_info) > } > > fs_info->zone_size = zone_size; > - fs_info->max_zone_append_size = max_zone_append_size; > fs_info->fs_devices->chunk_alloc_policy = BTRFS_CHUNK_ALLOC_ZONED; > > /* > @@ -1318,9 +1311,6 @@ bool btrfs_use_zone_append(struct btrfs_inode *inode, u64 start) > if (!btrfs_is_zoned(fs_info)) > return false; > > - if (!fs_info->max_zone_append_size) > - return false; > - > if (!is_data_inode(&inode->vfs_inode)) > return false; > > -- > 2.31.1
On 28/06/2021 22:22, David Sterba wrote: > On Tue, Jun 29, 2021 at 03:36:45AM +0900, Johannes Thumshirn wrote: >> Remove fs_info->max_zone_append_size, it doesn't serve any purpose. > Why was it added then? Commit 862931c76327 ("btrfs: introduce > max_zone_append_size") states some reasons so you should explain why > it's not needed now. It's a partial revert of the commit. > There used to be a patch in the original series for zoned support which limited the extent size to max_zone_append_size, but this patch has been dropped from the series somewhere around v9 IIRC. We've decided to go the opposite round, instead of limiting extents in the first place we split them before submission to comply with the device's limits.
On 29/06/2021 02:36, Johannes Thumshirn wrote: > Remove fs_info->max_zone_append_size, it doesn't serve any purpose. > > Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com> I hope the reason to remove max_zone_append_size shall go into the commit log. With that, Reviewed-by: Anand Jain <anand.jain@oracle.com> Thanks, Anand > > --- > Changes to v1: > - also remove the local max_zone_append_size variable in > btrfs_check_zoned_mode() (Anand) > --- > fs/btrfs/ctree.h | 2 -- > fs/btrfs/extent_io.c | 1 - > fs/btrfs/zoned.c | 10 ---------- > 3 files changed, 13 deletions(-) > > diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h > index d7ef4d7d2c1a..7a9cf4d12157 100644 > --- a/fs/btrfs/ctree.h > +++ b/fs/btrfs/ctree.h > @@ -1014,8 +1014,6 @@ struct btrfs_fs_info { > u64 zoned; > }; > > - /* Max size to emit ZONE_APPEND write command */ > - u64 max_zone_append_size; > struct mutex zoned_meta_io_lock; > spinlock_t treelog_bg_lock; > u64 treelog_bg; > diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c > index 9e81d25dea70..1f947e24091a 100644 > --- a/fs/btrfs/extent_io.c > +++ b/fs/btrfs/extent_io.c > @@ -3266,7 +3266,6 @@ static int calc_bio_boundaries(struct btrfs_bio_ctrl *bio_ctrl, > return 0; > } > > - ASSERT(fs_info->max_zone_append_size > 0); > /* Ordered extent not yet created, so we're good */ > ordered = btrfs_lookup_ordered_extent(inode, logical); > if (!ordered) { > diff --git a/fs/btrfs/zoned.c b/fs/btrfs/zoned.c > index 297c0b1c0634..76754e441e20 100644 > --- a/fs/btrfs/zoned.c > +++ b/fs/btrfs/zoned.c > @@ -529,7 +529,6 @@ int btrfs_check_zoned_mode(struct btrfs_fs_info *fs_info) > u64 zoned_devices = 0; > u64 nr_devices = 0; > u64 zone_size = 0; > - u64 max_zone_append_size = 0; > const bool incompat_zoned = btrfs_fs_incompat(fs_info, ZONED); > int ret = 0; > > @@ -565,11 +564,6 @@ int btrfs_check_zoned_mode(struct btrfs_fs_info *fs_info) > ret = -EINVAL; > goto out; > } > - if (!max_zone_append_size || > - (zone_info->max_zone_append_size && > - zone_info->max_zone_append_size < max_zone_append_size)) > - max_zone_append_size = > - zone_info->max_zone_append_size; > } > nr_devices++; > } > @@ -619,7 +613,6 @@ int btrfs_check_zoned_mode(struct btrfs_fs_info *fs_info) > } > > fs_info->zone_size = zone_size; > - fs_info->max_zone_append_size = max_zone_append_size; > fs_info->fs_devices->chunk_alloc_policy = BTRFS_CHUNK_ALLOC_ZONED; > > /* > @@ -1318,9 +1311,6 @@ bool btrfs_use_zone_append(struct btrfs_inode *inode, u64 start) > if (!btrfs_is_zoned(fs_info)) > return false; > > - if (!fs_info->max_zone_append_size) > - return false; > - > if (!is_data_inode(&inode->vfs_inode)) > return false; > >
On Tue, Jun 29, 2021 at 03:36:45AM +0900, Johannes Thumshirn wrote: > Remove fs_info->max_zone_append_size, it doesn't serve any purpose. > > Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com> > > --- > Changes to v1: > - also remove the local max_zone_append_size variable in > btrfs_check_zoned_mode() (Anand) And what about btrfs_zoned_device_info::max_zone_append_size, should it also be removed? In case you don't have plans with it I'll remove it, no need to resend, it's just a few more lines but want to know if it's accidental or intentional.
On 30/06/2021 20:51, David Sterba wrote: > On Tue, Jun 29, 2021 at 03:36:45AM +0900, Johannes Thumshirn wrote: >> Remove fs_info->max_zone_append_size, it doesn't serve any purpose. >> >> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com> >> >> --- >> Changes to v1: >> - also remove the local max_zone_append_size variable in >> btrfs_check_zoned_mode() (Anand) > > And what about btrfs_zoned_device_info::max_zone_append_size, should it > also be removed? In case you don't have plans with it I'll remove it, no > need to resend, it's just a few more lines but want to know if it's > accidental or intentional. > I /think/ this one can stay until we work on multi-device/RAID support. We could though also remove it and bring it back in case we need it again. @Naohiro what's your take on that?
On 2021/07/01 17:02, Johannes Thumshirn wrote: > On 30/06/2021 20:51, David Sterba wrote: >> On Tue, Jun 29, 2021 at 03:36:45AM +0900, Johannes Thumshirn wrote: >>> Remove fs_info->max_zone_append_size, it doesn't serve any purpose. >>> >>> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com> >>> >>> --- >>> Changes to v1: >>> - also remove the local max_zone_append_size variable in >>> btrfs_check_zoned_mode() (Anand) >> >> And what about btrfs_zoned_device_info::max_zone_append_size, should it >> also be removed? In case you don't have plans with it I'll remove it, no >> need to resend, it's just a few more lines but want to know if it's >> accidental or intentional. >> > > I /think/ this one can stay until we work on multi-device/RAID support. We are nowhere near close to this for now, so I am all for removing it. > We could though also remove it and bring it back in case we need it again. > > @Naohiro what's your take on that? >
On Thu, Jul 01, 2021 at 10:06:22AM +0000, Damien Le Moal wrote: > On 2021/07/01 17:02, Johannes Thumshirn wrote: > > On 30/06/2021 20:51, David Sterba wrote: > >> On Tue, Jun 29, 2021 at 03:36:45AM +0900, Johannes Thumshirn wrote: > >>> Remove fs_info->max_zone_append_size, it doesn't serve any purpose. > >>> > >>> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com> > >>> > >>> --- > >>> Changes to v1: > >>> - also remove the local max_zone_append_size variable in > >>> btrfs_check_zoned_mode() (Anand) > >> > >> And what about btrfs_zoned_device_info::max_zone_append_size, should it > >> also be removed? In case you don't have plans with it I'll remove it, no > >> need to resend, it's just a few more lines but want to know if it's > >> accidental or intentional. > >> > > > > I /think/ this one can stay until we work on multi-device/RAID support. > > We are nowhere near close to this for now, so I am all for removing it. Ok then, I'll update the patch.
On 01/07/2021 12:06, Damien Le Moal wrote: > On 2021/07/01 17:02, Johannes Thumshirn wrote: >> On 30/06/2021 20:51, David Sterba wrote: >>> On Tue, Jun 29, 2021 at 03:36:45AM +0900, Johannes Thumshirn wrote: >>>> Remove fs_info->max_zone_append_size, it doesn't serve any purpose. >>>> >>>> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com> >>>> >>>> --- >>>> Changes to v1: >>>> - also remove the local max_zone_append_size variable in >>>> btrfs_check_zoned_mode() (Anand) >>> >>> And what about btrfs_zoned_device_info::max_zone_append_size, should it >>> also be removed? In case you don't have plans with it I'll remove it, no >>> need to resend, it's just a few more lines but want to know if it's >>> accidental or intentional. >>> >> >> I /think/ this one can stay until we work on multi-device/RAID support. > > We are nowhere near close to this for now, so I am all for removing it. OK then let's get rid of it all. The block layer knows the limits when we're constructing a bio.
diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h index d7ef4d7d2c1a..7a9cf4d12157 100644 --- a/fs/btrfs/ctree.h +++ b/fs/btrfs/ctree.h @@ -1014,8 +1014,6 @@ struct btrfs_fs_info { u64 zoned; }; - /* Max size to emit ZONE_APPEND write command */ - u64 max_zone_append_size; struct mutex zoned_meta_io_lock; spinlock_t treelog_bg_lock; u64 treelog_bg; diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c index 9e81d25dea70..1f947e24091a 100644 --- a/fs/btrfs/extent_io.c +++ b/fs/btrfs/extent_io.c @@ -3266,7 +3266,6 @@ static int calc_bio_boundaries(struct btrfs_bio_ctrl *bio_ctrl, return 0; } - ASSERT(fs_info->max_zone_append_size > 0); /* Ordered extent not yet created, so we're good */ ordered = btrfs_lookup_ordered_extent(inode, logical); if (!ordered) { diff --git a/fs/btrfs/zoned.c b/fs/btrfs/zoned.c index 297c0b1c0634..76754e441e20 100644 --- a/fs/btrfs/zoned.c +++ b/fs/btrfs/zoned.c @@ -529,7 +529,6 @@ int btrfs_check_zoned_mode(struct btrfs_fs_info *fs_info) u64 zoned_devices = 0; u64 nr_devices = 0; u64 zone_size = 0; - u64 max_zone_append_size = 0; const bool incompat_zoned = btrfs_fs_incompat(fs_info, ZONED); int ret = 0; @@ -565,11 +564,6 @@ int btrfs_check_zoned_mode(struct btrfs_fs_info *fs_info) ret = -EINVAL; goto out; } - if (!max_zone_append_size || - (zone_info->max_zone_append_size && - zone_info->max_zone_append_size < max_zone_append_size)) - max_zone_append_size = - zone_info->max_zone_append_size; } nr_devices++; } @@ -619,7 +613,6 @@ int btrfs_check_zoned_mode(struct btrfs_fs_info *fs_info) } fs_info->zone_size = zone_size; - fs_info->max_zone_append_size = max_zone_append_size; fs_info->fs_devices->chunk_alloc_policy = BTRFS_CHUNK_ALLOC_ZONED; /* @@ -1318,9 +1311,6 @@ bool btrfs_use_zone_append(struct btrfs_inode *inode, u64 start) if (!btrfs_is_zoned(fs_info)) return false; - if (!fs_info->max_zone_append_size) - return false; - if (!is_data_inode(&inode->vfs_inode)) return false;
Remove fs_info->max_zone_append_size, it doesn't serve any purpose. Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com> --- Changes to v1: - also remove the local max_zone_append_size variable in btrfs_check_zoned_mode() (Anand) --- fs/btrfs/ctree.h | 2 -- fs/btrfs/extent_io.c | 1 - fs/btrfs/zoned.c | 10 ---------- 3 files changed, 13 deletions(-)