[PATCH] btrfs: zoned: fix initial free space detection

[PATCH] btrfs: zoned: fix initial free space detection

[Naohiro Aota]

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;
 	u64 reclaimable_unusable;
 
 	WARN_ON(!initial && offset + size > block_group->zone_capacity);
-- 
2.45.2

Re: [PATCH] btrfs: zoned: fix initial free space detection

[Johannes Thumshirn]

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>

Re: [PATCH] btrfs: zoned: fix initial free space detection

[Naohiro Aota]

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>

Re: [PATCH] btrfs: zoned: fix initial free space detection

[David Sterba]

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>

Re: [PATCH] btrfs: zoned: fix initial free space detection

[Naohiro Aota]

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));

Re: [PATCH] btrfs: zoned: fix initial free space detection

[David Sterba]

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.