Re: [PATCH] btrfs: fix u32 to s64 type conversion in dirty_metadata_bytes accounting

On Mon, Jun 29, 2026 at 01:08:05PM +0100, Filipe Manana wrote:
> On Mon, Jun 29, 2026 at 8:09 AM Dave Chen <[email protected]> wrote:
> >
> > The percpu_counter dirty_metadata_bytes is updated by negating eb->len
> > and passing it to percpu_counter_add_batch(), whose amount parameter is
> > s64.  Since commit 84cda1a6087d ("btrfs: cache folio size and shift in
> > extent_buffer"), eb->len is u32.  The u32 result of -eb->len, when
> > widened to the s64 parameter, becomes a large positive value instead of
> > the intended negative value.  For eb->len == 16384 the counter adds
> > +4294950912 instead of subtracting 16384.
> >
> > The counter therefore grows on every metadata writeback instead of
> > shrinking by the extent buffer size, permanently exceeding
> > BTRFS_DIRTY_METADATA_THRESH and causing __btrfs_btree_balance_dirty()
> > to trigger balance_dirty_pages_ratelimited() unconditionally, adding
> > unnecessary writeback pressure.
> >
> > Cast eb->len to s64 before negation at both call sites so the
> > subtraction is performed in signed 64-bit arithmetic.
> >
> > Signed-off-by: Dave Chen <[email protected]>
> 
> Curious, do you have any performance data to give an idea of the impact?
> 
> I'm asking not only for clarity but also to determine if adding a
> Fixes tag for stable backports makes sense.
> The commit that caused this issue is 84cda1a6087d ("btrfs: cache folio
> size and shift in extent_buffer"), which changed struct
> extent_buffer::len from unsigned long (64-bit on 64 bits platforms) to
> u32.

Oh my that's very well hidden signedness bug. I'd rather not rely on the
correct implicit conversions and add a helper like
percpu_counter_sub_batch() that takes the positive value to subtract and
correctly casts it to s64 inside.

We have more places with negative unsigned value passed to
percpu_counter_add_batch(), e.g. in btrfs_remove_ordered_extent().  The
entry->num_bytes is u64 so it's correct.

In the rest of kernel the negative values are not used frequently, in
xfs, networking, bpf.

This fix can go in as-is as it's minimal, the API update can follow so
we can be sure gems like

lib/flex_proportions.c:fprop_reflect_period_percpu()

		percpu_counter_add_batch(&pl->events,
                       -val + (val >> (period-pl->period)), PROP_BATCH);

can be made more understandable.