| Message ID | ec2aafb75c235d9c37aef52068992dca0d060d5f.1718096605.git.naohiro.aota@wdc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: zoned: fix initial free space detection | expand |
On 11.06.24 11:06, Naohiro Aota wrote: > When creating a new block group, it calls btrfs_fadd_new_free_space() to Nit: btrfs_add_new_free_space() Otherwise, Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
On Tue, Jun 11, 2024 at 06:05:52PM +0900, Naohiro Aota wrote: > When creating a new block group, it calls btrfs_fadd_new_free_space() to > add the entire block group range into the free space > accounting. __btrfs_add_free_space_zoned() checks if size == > block_group->length to detect the initial free space adding, and proceed > that case properly. > > However, if the zone_capacity == zone_size and the over-write speed is fast > enough, the entire zone can be over-written within one transaction. That > confuses __btrfs_add_free_space_zoned() to handle it as an initial free > space accounting. As a result, that block group becomes a strange state: 0 > used bytes, 0 zone_unusable bytes, but alloc_offset == zone_capacity (no > allocation anymore). > > The initial free space accounting can properly be checked by checking > alloc_offset too. > > Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com> > Fixes: 98173255bddd ("btrfs: zoned: calculate free space from zone capacity") > CC: stable@vger.kernel.org # 6.1+ > --- > fs/btrfs/free-space-cache.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/btrfs/free-space-cache.c b/fs/btrfs/free-space-cache.c > index fcfc1b62e762..72e60764d1ea 100644 > --- a/fs/btrfs/free-space-cache.c > +++ b/fs/btrfs/free-space-cache.c > @@ -2697,7 +2697,7 @@ static int __btrfs_add_free_space_zoned(struct btrfs_block_group *block_group, > u64 offset = bytenr - block_group->start; > u64 to_free, to_unusable; > int bg_reclaim_threshold = 0; > - bool initial = (size == block_group->length); > + bool initial = (size == block_group->length) && block_group->alloc_offset == 0; I'm not recommending to use compound conditions in the initializer block as it can hide some important detail, but in this case it's both related to the block group and still related to the variable name. Please put the pair of ( ) to the whole expression. Reviewed-by: David Sterba <dsterba@suse.com>
On Tue, Jun 11, 2024 at 10:45:09AM GMT, Johannes Thumshirn wrote: > On 11.06.24 11:06, Naohiro Aota wrote: > > When creating a new block group, it calls btrfs_fadd_new_free_space() to > Nit: btrfs_add_new_free_space() oh. I'll fix that. Thanks, > > > Otherwise, > Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
On Wed, Jun 12, 2024 at 09:51:29PM GMT, David Sterba wrote: > On Tue, Jun 11, 2024 at 06:05:52PM +0900, Naohiro Aota wrote: > > When creating a new block group, it calls btrfs_fadd_new_free_space() to > > add the entire block group range into the free space > > accounting. __btrfs_add_free_space_zoned() checks if size == > > block_group->length to detect the initial free space adding, and proceed > > that case properly. > > > > However, if the zone_capacity == zone_size and the over-write speed is fast > > enough, the entire zone can be over-written within one transaction. That > > confuses __btrfs_add_free_space_zoned() to handle it as an initial free > > space accounting. As a result, that block group becomes a strange state: 0 > > used bytes, 0 zone_unusable bytes, but alloc_offset == zone_capacity (no > > allocation anymore). > > > > The initial free space accounting can properly be checked by checking > > alloc_offset too. > > > > Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com> > > Fixes: 98173255bddd ("btrfs: zoned: calculate free space from zone capacity") > > CC: stable@vger.kernel.org # 6.1+ > > --- > > fs/btrfs/free-space-cache.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/fs/btrfs/free-space-cache.c b/fs/btrfs/free-space-cache.c > > index fcfc1b62e762..72e60764d1ea 100644 > > --- a/fs/btrfs/free-space-cache.c > > +++ b/fs/btrfs/free-space-cache.c > > @@ -2697,7 +2697,7 @@ static int __btrfs_add_free_space_zoned(struct btrfs_block_group *block_group, > > u64 offset = bytenr - block_group->start; > > u64 to_free, to_unusable; > > int bg_reclaim_threshold = 0; > > - bool initial = (size == block_group->length); > > + bool initial = (size == block_group->length) && block_group->alloc_offset == 0; > > I'm not recommending to use compound conditions in the initializer block > as it can hide some important detail, but in this case it's both related > to the block group and still related to the variable name. Please put > the pair of ( ) to the whole expression. > > Reviewed-by: David Sterba <dsterba@suse.com> Sure. You mean like this? bool initial = ((size == block_group->length) && (block_group->alloc_offset == 0));
On Thu, Jun 13, 2024 at 12:18:48AM +0000, Naohiro Aota wrote: > On Wed, Jun 12, 2024 at 09:51:29PM GMT, David Sterba wrote: > > On Tue, Jun 11, 2024 at 06:05:52PM +0900, Naohiro Aota wrote: > > > When creating a new block group, it calls btrfs_fadd_new_free_space() to > > > add the entire block group range into the free space > > > accounting. __btrfs_add_free_space_zoned() checks if size == > > > block_group->length to detect the initial free space adding, and proceed > > > that case properly. > > > > > > However, if the zone_capacity == zone_size and the over-write speed is fast > > > enough, the entire zone can be over-written within one transaction. That > > > confuses __btrfs_add_free_space_zoned() to handle it as an initial free > > > space accounting. As a result, that block group becomes a strange state: 0 > > > used bytes, 0 zone_unusable bytes, but alloc_offset == zone_capacity (no > > > allocation anymore). > > > > > > The initial free space accounting can properly be checked by checking > > > alloc_offset too. > > > > > > Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com> > > > Fixes: 98173255bddd ("btrfs: zoned: calculate free space from zone capacity") > > > CC: stable@vger.kernel.org # 6.1+ > > > --- > > > fs/btrfs/free-space-cache.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/fs/btrfs/free-space-cache.c b/fs/btrfs/free-space-cache.c > > > index fcfc1b62e762..72e60764d1ea 100644 > > > --- a/fs/btrfs/free-space-cache.c > > > +++ b/fs/btrfs/free-space-cache.c > > > @@ -2697,7 +2697,7 @@ static int __btrfs_add_free_space_zoned(struct btrfs_block_group *block_group, > > > u64 offset = bytenr - block_group->start; > > > u64 to_free, to_unusable; > > > int bg_reclaim_threshold = 0; > > > - bool initial = (size == block_group->length); > > > + bool initial = (size == block_group->length) && block_group->alloc_offset == 0; > > > > I'm not recommending to use compound conditions in the initializer block > > as it can hide some important detail, but in this case it's both related > > to the block group and still related to the variable name. Please put > > the pair of ( ) to the whole expression. > > > > Reviewed-by: David Sterba <dsterba@suse.com> > > Sure. You mean like this? > > bool initial = ((size == block_group->length) && (block_group->alloc_offset == 0)); Yes.
diff --git a/fs/btrfs/free-space-cache.c b/fs/btrfs/free-space-cache.c index fcfc1b62e762..72e60764d1ea 100644 --- a/fs/btrfs/free-space-cache.c +++ b/fs/btrfs/free-space-cache.c @@ -2697,7 +2697,7 @@ static int __btrfs_add_free_space_zoned(struct btrfs_block_group *block_group, u64 offset = bytenr - block_group->start; u64 to_free, to_unusable; int bg_reclaim_threshold = 0; - bool initial = (size == block_group->length); + bool initial = (size == block_group->length) && block_group->alloc_offset == 0; u64 reclaimable_unusable; WARN_ON(!initial && offset + size > block_group->zone_capacity);
When creating a new block group, it calls btrfs_fadd_new_free_space() to add the entire block group range into the free space accounting. __btrfs_add_free_space_zoned() checks if size == block_group->length to detect the initial free space adding, and proceed that case properly. However, if the zone_capacity == zone_size and the over-write speed is fast enough, the entire zone can be over-written within one transaction. That confuses __btrfs_add_free_space_zoned() to handle it as an initial free space accounting. As a result, that block group becomes a strange state: 0 used bytes, 0 zone_unusable bytes, but alloc_offset == zone_capacity (no allocation anymore). The initial free space accounting can properly be checked by checking alloc_offset too. Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com> Fixes: 98173255bddd ("btrfs: zoned: calculate free space from zone capacity") CC: stable@vger.kernel.org # 6.1+ --- fs/btrfs/free-space-cache.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)