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

[David Sterba <dsterba@suse.cz>]

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.