Re: [PATCH v4 1/2] btrfs: fix periodic reclaim condition

[Chris Mason <clm@meta.com>]

Sun YangKai <sunk67188@gmail.com> wrote:
> Problems with current implementation:
> 1. reclaimable_bytes is signed while chunk_sz is unsigned, causing
>    negative reclaimable_bytes to trigger reclaim unexpectedly
> 2. The "space must be freed between scans" assumption breaks the
>    two-scan requirement: first scan marks block groups, second scan
>    reclaims them. Without the second scan, no reclamation occurs.
> 
> Instead, track actual reclaim progress: pause reclaim when block groups
> will be reclaimed, and resume only when progress is made. This ensures
> reclaim continues until no further progress can be made. And resume
> perioidc reclaim when there's enough free space.
> 
Hi everyone,

I'm running the btrfs for-next branch, along with the encryption patch set
through my AI review prompts:

https://github.com/masoncl/review-prompts

I'm working on some btrfs specific instructions, but hopefully these reviews
are already useful.  I've kept the headers to make it easier to double check
that I'm actually replying to the right email, but please let me know if any
parts of this are incorrect:

commit 19eff93dc738e8afaa59cb374b44bb5a162e6c2d
Author: Sun YangKai <sunk67188@gmail.com>

btrfs: fix periodic reclaim condition

This commit fixes issues with periodic reclaim by tracking actual reclaim
progress rather than relying on the assumption that space must be freed
between scans. The patch addresses problems with signed/unsigned comparison
and ensures reclaim continues until no further progress can be made.

> diff --git a/fs/btrfs/space-info.c b/fs/btrfs/space-info.c
> index ebe97d6d67d3a..bc493243f777d 100644
> --- a/fs/btrfs/space-info.c
> +++ b/fs/btrfs/space-info.c

[ ... ]

> @@ -2098,11 +2098,11 @@ static bool is_reclaim_urgent(struct btrfs_space_info *space_info)
>  	return unalloc < data_chunk_size;
>  }
>
> -static void do_reclaim_sweep(struct btrfs_space_info *space_info, int raid)
> +static bool do_reclaim_sweep(struct btrfs_space_info *space_info, int raid)
>  {
>  	struct btrfs_block_group *bg;
>  	int thresh_pct;
> -	bool try_again = true;
> +	bool will_reclaim = false;
>  	bool urgent;
>
>  	spin_lock(&space_info->lock);

[ ... ]

> @@ -2137,12 +2137,13 @@ static void do_reclaim_sweep(struct btrfs_space_info *space_info, int raid)
>  	 * If we have any staler groups, we don't touch the fresher ones, but if we
>  	 * really need a block group, do take a fresh one.
>  	 */
> -	if (try_again && urgent) {
> -		try_again = false;
> +	if (!will_reclaim && urgent) {
> +		urgent = false;
>  		goto again;
>  	}
>
>  	up_read(&space_info->groups_sem);
> +	return will_reclaim;
>  }

[ ... ]

> @@ -2179,7 +2181,6 @@ static bool btrfs_should_periodic_reclaim(struct btrfs_space_info *space_info)
>
>  	spin_lock(&space_info->lock);
>  	ret = space_info->periodic_reclaim_ready;
> -	btrfs_set_periodic_reclaim_ready(space_info, false);
>  	spin_unlock(&space_info->lock);
>
>  	return ret;
> @@ -2193,8 +2194,10 @@ void btrfs_reclaim_sweep(const struct btrfs_fs_info *fs_info)
>  	list_for_each_entry(space_info, &fs_info->space_info, list) {
>  		if (!btrfs_should_periodic_reclaim(space_info))
>  			continue;
> -		for (raid = 0; raid < BTRFS_NR_RAID_TYPES; raid++)
> -			do_reclaim_sweep(space_info, raid);
> +		for (raid = 0; raid < BTRFS_NR_RAID_TYPES; raid++) {
> +			if (do_reclaim_sweep(space_info, raid))
> +				btrfs_set_periodic_reclaim_ready(space_info, false);
> +		}
>  	}
>  }

Does btrfs_reclaim_sweep()->btrfs_set_periodic_reclaim_ready() violate
locking requirements? The function btrfs_set_periodic_reclaim_ready() has
lockdep_assert_held(&space_info->lock) at line 2163, but at the call site
above (line 2199), no lock is held.

Looking at do_reclaim_sweep(), it releases space_info->lock via
up_read(&space_info->groups_sem) at line 2145 before returning. Similarly,
the old code had btrfs_should_periodic_reclaim() release the lock with
spin_unlock(&space_info->lock) at line 2184 before returning.

In the original code, btrfs_set_periodic_reclaim_ready() was called from
btrfs_should_periodic_reclaim() while holding space_info->lock (acquired
at line 2182, call to btrfs_set_periodic_reclaim_ready, then release at
line 2184). This patch moves the call to btrfs_reclaim_sweep() but doesn't
acquire the lock at the new call site.

Should space_info->lock be acquired before calling
btrfs_set_periodic_reclaim_ready() at line 2199?