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

[Sun YangKai <sunk67188@gmail.com>]

On 2026/2/9 02:24, Chris Mason wrote:
> 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?

Yes! Thanks a lot for the report and I've sent the fix:
https://lore.kernel.org/linux-btrfs/20260209130248.29418-1-sunk67188@gmail.com/