[bug report] btrfs: periodic block_group reclaim

[bug report] btrfs: periodic block_group reclaim

[Dan Carpenter]

Hello Boris Burkov,

Commit e4ca3932ae90 ("btrfs: periodic block_group reclaim") from Feb
2, 2024 (linux-next), leads to the following Smatch static checker
warning:

	fs/btrfs/space-info.c:2139 do_reclaim_sweep()
	warn: iterator 'bg' changed during iteration

fs/btrfs/space-info.c
    2125 static bool do_reclaim_sweep(struct btrfs_space_info *space_info, int raid)
    2126 {
    2127         struct btrfs_block_group *bg;
    2128         int thresh_pct;
    2129         bool will_reclaim = false;
    2130         bool urgent;
    2131 
    2132         spin_lock(&space_info->lock);
    2133         urgent = is_reclaim_urgent(space_info);
    2134         thresh_pct = btrfs_calc_reclaim_threshold(space_info);
    2135         spin_unlock(&space_info->lock);
    2136 
    2137         down_read(&space_info->groups_sem);
    2138 again:
--> 2139         list_for_each_entry(bg, &space_info->block_groups[raid], list) {
    2140                 u64 thresh;
    2141                 bool reclaim = false;
    2142 
    2143                 btrfs_get_block_group(bg);

We bump the refcount here.

    2144                 spin_lock(&bg->lock);
    2145                 thresh = mult_perc(bg->length, thresh_pct);
    2146                 if (bg->used < thresh && bg->reclaim_mark) {
    2147                         will_reclaim = true;
    2148                         reclaim = true;
    2149                 }
    2150                 bg->reclaim_mark++;
    2151                 spin_unlock(&bg->lock);
    2152                 if (reclaim)
    2153                         btrfs_mark_bg_to_reclaim(bg);
    2154                 btrfs_put_block_group(bg);
                                               ^^

This decrements the "bg" so now presumably a different thread could free
it.  The race window between this btrfs_put_block_group() and the
btrfs_get_block_group() on the next iteration is pretty small.
Most likely this get/put pair could be removed.  I can't see a reason
for it.

    2155         }
    2156 
    2157         /*
    2158          * In situations where we are very motivated to reclaim (low unalloc)
    2159          * use two passes to make the reclaim mark check best effort.
    2160          *
    2161          * If we have any staler groups, we don't touch the fresher ones, but if we
    2162          * really need a block group, do take a fresh one.
    2163          */
    2164         if (!will_reclaim && urgent) {
    2165                 urgent = false;
    2166                 goto again;
    2167         }
    2168 
    2169         up_read(&space_info->groups_sem);
    2170         return will_reclaim;
    2171 }

This email is a free service from the Smatch-CI project [smatch.sf.net].

regards,
dan carpenter

Re: [bug report] btrfs: periodic block_group reclaim

[Boris Burkov]

On Fri, May 08, 2026 at 10:49:26AM +0300, Dan Carpenter wrote:
> Hello Boris Burkov,
> 
> Commit e4ca3932ae90 ("btrfs: periodic block_group reclaim") from Feb
> 2, 2024 (linux-next), leads to the following Smatch static checker
> warning:
> 
> 	fs/btrfs/space-info.c:2139 do_reclaim_sweep()
> 	warn: iterator 'bg' changed during iteration
> 
> fs/btrfs/space-info.c
>     2125 static bool do_reclaim_sweep(struct btrfs_space_info *space_info, int raid)
>     2126 {
>     2127         struct btrfs_block_group *bg;
>     2128         int thresh_pct;
>     2129         bool will_reclaim = false;
>     2130         bool urgent;
>     2131 
>     2132         spin_lock(&space_info->lock);
>     2133         urgent = is_reclaim_urgent(space_info);
>     2134         thresh_pct = btrfs_calc_reclaim_threshold(space_info);
>     2135         spin_unlock(&space_info->lock);
>     2136 
>     2137         down_read(&space_info->groups_sem);
>     2138 again:
> --> 2139         list_for_each_entry(bg, &space_info->block_groups[raid], list) {
>     2140                 u64 thresh;
>     2141                 bool reclaim = false;
>     2142 
>     2143                 btrfs_get_block_group(bg);
> 
> We bump the refcount here.
> 
>     2144                 spin_lock(&bg->lock);
>     2145                 thresh = mult_perc(bg->length, thresh_pct);
>     2146                 if (bg->used < thresh && bg->reclaim_mark) {
>     2147                         will_reclaim = true;
>     2148                         reclaim = true;
>     2149                 }
>     2150                 bg->reclaim_mark++;
>     2151                 spin_unlock(&bg->lock);
>     2152                 if (reclaim)
>     2153                         btrfs_mark_bg_to_reclaim(bg);
>     2154                 btrfs_put_block_group(bg);
>                                                ^^
> 
> This decrements the "bg" so now presumably a different thread could free
> it.  The race window between this btrfs_put_block_group() and the
> btrfs_get_block_group() on the next iteration is pretty small.
> Most likely this get/put pair could be removed.  I can't see a reason
> for it.
> 

Hi,

This should be protected by read locking the groups_sem for the duration
of the loop. The code which unlinks block_group->list write locks
groups_sem and has a reference to the bg during that time.

There is at least one other loop that uses this same pattern
(find_free_extent() in fs/btrfs/extent-tree.c). I agree that it doesn't
make the most sense to get/put the refcount if it doesn't really protect
against anything and the rwsem is the actual protection.

There was a patch on the list a few months back that switched to a
per-cpu refcount for block_group->refs, which suggests that getting rid
of some of these logically redundant refcounts could matter for
performance too. I'll take a look soon.

Thanks,
Boris

>     2155         }
>     2156 
>     2157         /*
>     2158          * In situations where we are very motivated to reclaim (low unalloc)
>     2159          * use two passes to make the reclaim mark check best effort.
>     2160          *
>     2161          * If we have any staler groups, we don't touch the fresher ones, but if we
>     2162          * really need a block group, do take a fresh one.
>     2163          */
>     2164         if (!will_reclaim && urgent) {
>     2165                 urgent = false;
>     2166                 goto again;
>     2167         }
>     2168 
>     2169         up_read(&space_info->groups_sem);
>     2170         return will_reclaim;
>     2171 }
> 
> This email is a free service from the Smatch-CI project [smatch.sf.net].
> 
> regards,
> dan carpenter