Re: [PATCH v2] btrfs: replace writeback inhibition xarray with a fixed inline buffer

On Thu, Jul 2, 2026 at 12:48 AM Leo Martins <[email protected]> wrote:
>
> Commit f9a48549a15a ("btrfs: inhibit extent buffer writeback to prevent
> COW amplification") tracks the extent buffers a transaction handle has
> inhibited in a per-handle xarray. Keying the tracking to the transaction
> handle is correct, but using an xarray for it causes two problems in
> production.
>
> First, a write_iops regression. Every COW calls
> btrfs_inhibit_eb_writeback() from btrfs_force_cow_block() and
> should_cow_block(), which does an xa_store() keyed by eb->start. The
> kernel test robot reported a 22.6% fio.write_iops regression on a
> single-task 4k randwrite workload (ftruncate ioengine, buffered IO) on
> btrfs. The cost is the per-COW xarray store done on every COW'd block.
> Replacing it with a non-allocating fixed buffer recovers the lost
> throughput, and that buffer does more per-COW bookkeeping yet still
> recovers, so the cost is the xarray operation itself rather than the
> extra tracking work.
>
> Second, an unbounded cleanup walk. btrfs_uninhibit_all_eb_writeback()
> iterates every eb the handle inhibited with xa_for_each(). A single
> handle that COWs a very large number of blocks (inode eviction, or
> truncate of a file with many extents, where btrfs_truncate_inode_items()
> loops over many search_again descents under one handle) makes that walk
> arbitrarily long. It runs in __btrfs_end_transaction() before
> num_writers is dropped, so it blocks the committing thread; this shows up
> as multi-second stalls and RCU stall reports.
>
> Replace the xarray with a fixed inline array on btrfs_trans_handle,
> managed with a CLOCK (second-chance) eviction policy. Inhibiting a buffer
> becomes an array append with no allocation and no tree walk, and the
> end-of-handle cleanup is bounded by the array size.
>
> The set that actually needs protection is the working set the handle
> revisits across search_again descents, the search path frontier, which is
> on the order of the tree height. It is not every block the handle ever
> COWs. should_cow_block() re-inhibiting an already tracked buffer marks it
> referenced, so revisited buffers survive eviction while write-once buffers
> are reclaimed first. A small fixed buffer is therefore enough where a
> non-evicting array would either overflow or have to grow without bound.
> BTRFS_INHIBITED_EBS_SLOTS is 8 and the reference bits pack into a u32.
>
> The CLOCK eviction is what justifies the extra complexity over a plain
> non-evicting array. The test workload stresses amplification: it removes
> 16 heavily fragmented 64 MiB files in one transaction while background
> writeback keeps writing out in-use metadata. A re-COW event is a buffer
> already COWed in the running transaction that was written back and then
> COWed again; the figure below is the ratio of re-COW events to first-COW
> events summed across the eviction (n=5, lower is better):
>
>   tracking                           re-COW per first-COW
>   no inhibition                      6.1
>   non-evicting array, 32 slots       3.8
>   CLOCK array, 8 slots (this patch)  1.6
>   unbounded xarray (reverted)        1.4
>
> The non-evicting array fills with write-once buffers and stops covering
> the buffers the handle keeps revisiting, so even at four times the slots
> it leaves most of the amplification. CLOCK evicts the cold buffers and
> keeps the revisited ones, recovering almost all of the unbounded benefit.
> The eviction policy, not the buffer size, is what closes the gap.
>
> eb->writeback_inhibitors and the WB_SYNC_ALL bypass in
> lock_extent_buffer_for_io() are unchanged, so fsync and commit behavior
> are unaffected. A reference is taken on each tracked buffer so it cannot
> be freed while the array points at it; eviction drops that reference and
> the inhibitor count.
>
> Fixes: f9a48549a15a ("btrfs: inhibit extent buffer writeback to prevent COW amplification")
> Reported-by: kernel test robot <[email protected]>
> Closes: https://lore.kernel.org/oe-lkp/[email protected]
> Signed-off-by: Leo Martins <[email protected]>
> Reviewed-by: Sun YangKai <[email protected]>

Reviewed-by: Filipe Manana <[email protected]>

Some minor things to fixup below.

First, the Reviewed-by tag is in the wrong location, see:

https://btrfs.readthedocs.io/en/latest/dev/Developer-s-FAQ.html#ordering

> ---
> v2:
> - Present the amplification numbers as a table instead of prose (David Sterba).
> - Use int for the loop indices and the slot local instead of u32 (David Sterba).
> - Replace the BTRFS_INHIBITED_EBS_SLOTS comment with static_assert checks for
>   the <= 32 bound and the power-of-two size (David Sterba).
> - Widen inhibited_ebs_hand from u8 to u32; the handle stays in the same slab
>   bucket and the u8 only left an alignment hole (David Sterba).
> - Factor slot selection and eviction into btrfs_inhibit_claim_slot()
>   (Sun YangKai).
> - Add Reviewed-by from Sun YangKai.
>
> v1: https://lore.kernel.org/linux-btrfs/e0d3d3ac7c585b023c2e14cec7e2b995b247b68e.1781728340.git.loemra.dev@gmail.com/
>
>  fs/btrfs/extent_io.c   | 93 +++++++++++++++++++++++++++---------------
>  fs/btrfs/transaction.c |  2 -
>  fs/btrfs/transaction.h | 21 ++++++++--
>  3 files changed, 78 insertions(+), 38 deletions(-)
>
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index 9d7ca80477fd..145b6cf37331 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -2982,47 +2982,75 @@ static inline void btrfs_release_extent_buffer(struct extent_buffer *eb)
>         kmem_cache_free(extent_buffer_cache, eb);
>  }
>
> +/*
> + * Claim a slot to track an extent buffer in, evicting the coldest tracked buffer
> + * when the array is full.
> + *
> + * Slots fill in order until the array is full. After that a CLOCK (second
> + * chance) scan advances the hand, clearing one reference bit per step, until it
> + * lands on an unreferenced slot whose buffer is evicted. Clearing a bit per step
> + * bounds the scan to BTRFS_INHIBITED_EBS_SLOTS iterations.
> + */
> +static int btrfs_inhibit_claim_slot(struct btrfs_trans_handle *trans)
> +{
> +       int slot;
> +
> +       if (trans->nr_inhibited_ebs < BTRFS_INHIBITED_EBS_SLOTS)
> +               return trans->nr_inhibited_ebs++;
> +
> +       while (trans->inhibited_ebs_referenced &
> +              (1U << trans->inhibited_ebs_hand)) {
> +               trans->inhibited_ebs_referenced &=
> +                       ~(1U << trans->inhibited_ebs_hand);
> +               trans->inhibited_ebs_hand =
> +                       (trans->inhibited_ebs_hand + 1) %
> +                       BTRFS_INHIBITED_EBS_SLOTS;
> +       }
> +       slot = trans->inhibited_ebs_hand;
> +       trans->inhibited_ebs_hand =
> +               (trans->inhibited_ebs_hand + 1) % BTRFS_INHIBITED_EBS_SLOTS;
> +
> +       atomic_dec(&trans->inhibited_ebs[slot]->writeback_inhibitors);
> +       free_extent_buffer(trans->inhibited_ebs[slot]);
> +       return slot;
> +}
> +
>  /*
>   * Inhibit writeback on buffer during transaction.
>   *
>   * @trans:  transaction handle that will own the inhibitor
>   * @eb:      extent buffer to inhibit writeback on
>   *
> - * Attempt to track this extent buffer in the transaction's inhibited set.  If
> - * memory allocation fails, the buffer is simply not tracked. It may be written
> - * back and need re-COW, which is the original behavior.  This is acceptable
> - * since inhibiting writeback is an optimization.
> + * Attempt to track this extent buffer in the transaction's inhibited set.  When
> + * the set is full the coldest tracked buffer is evicted instead.  An untracked
> + * buffer may be written back and need re-COW, which is the original behavior.
> + * This is acceptable since inhibiting writeback is an optimization.
>   */
> -void btrfs_inhibit_eb_writeback(struct btrfs_trans_handle *trans, struct extent_buffer *eb)
> +void btrfs_inhibit_eb_writeback(struct btrfs_trans_handle *trans,
> +                               struct extent_buffer *eb)

We avoid style/indentation only changes.

>  {
> -       unsigned long index = eb->start >> trans->fs_info->nodesize_bits;
> -       void *old;
> +       int slot;
>
>         lockdep_assert_held(&eb->lock);
> -       /* Check if already inhibited by this handle. */
> -       old = xa_load(&trans->writeback_inhibited_ebs, index);
> -       if (old == eb)
> -               return;
>
> -       /* Take reference for the xarray entry. */
> -       refcount_inc(&eb->refs);
> -
> -       old = xa_store(&trans->writeback_inhibited_ebs, index, eb, GFP_NOFS);
> -       if (xa_is_err(old)) {
> -               /* Allocation failed, just skip inhibiting this buffer. */
> -               free_extent_buffer(eb);
> -               return;
> +       /* Already tracked: set its reference bit (second chance) and return. */
> +       for (int i = 0; i < trans->nr_inhibited_ebs; i++) {
> +               if (trans->inhibited_ebs[i] == eb) {
> +                       trans->inhibited_ebs_referenced |= 1U << i;
> +                       return;
> +               }
>         }
>
> -       /* Handle replacement of different eb at same index. */
> -       if (old && old != eb) {
> -               struct extent_buffer *old_eb = old;
> -
> -               atomic_dec(&old_eb->writeback_inhibitors);
> -               free_extent_buffer(old_eb);
> -       }
> +       slot = btrfs_inhibit_claim_slot(trans);
>
> +       /*
> +        * Pin the eb while the array holds a raw pointer to it; the counter is
> +        * what lock_extent_buffer_for_io() checks.
> +        */
> +       refcount_inc(&eb->refs);
>         atomic_inc(&eb->writeback_inhibitors);
> +       trans->inhibited_ebs[slot] = eb;
> +       trans->inhibited_ebs_referenced |= 1U << slot;
>  }
>
>  /*
> @@ -3030,14 +3058,13 @@ void btrfs_inhibit_eb_writeback(struct btrfs_trans_handle *trans, struct extent_
>   */
>  void btrfs_uninhibit_all_eb_writeback(struct btrfs_trans_handle *trans)
>  {
> -       struct extent_buffer *eb;
> -       unsigned long index;
> -
> -       xa_for_each(&trans->writeback_inhibited_ebs, index, eb) {
> -               atomic_dec(&eb->writeback_inhibitors);
> -               free_extent_buffer(eb);
> +       for (int i = 0; i < trans->nr_inhibited_ebs; i++) {
> +               atomic_dec(&trans->inhibited_ebs[i]->writeback_inhibitors);
> +               free_extent_buffer(trans->inhibited_ebs[i]);
>         }
> -       xa_destroy(&trans->writeback_inhibited_ebs);
> +       trans->nr_inhibited_ebs = 0;
> +       trans->inhibited_ebs_referenced = 0;
> +       trans->inhibited_ebs_hand = 0;
>  }
>
>  static struct extent_buffer *__alloc_extent_buffer(struct btrfs_fs_info *fs_info,
> diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
> index 4358f4b63057..b4b8d587effb 100644
> --- a/fs/btrfs/transaction.c
> +++ b/fs/btrfs/transaction.c
> @@ -697,8 +697,6 @@ start_transaction(struct btrfs_root *root, unsigned int num_items,
>                 goto alloc_fail;
>         }
>
> -       xa_init(&h->writeback_inhibited_ebs);
> -
>         /*
>          * If we are JOIN_NOLOCK we're already committing a transaction and
>          * waiting on this guy, so we don't need to do the sb_start_intwrite
> diff --git a/fs/btrfs/transaction.h b/fs/btrfs/transaction.h
> index 7d70fe486758..e918dde76920 100644
> --- a/fs/btrfs/transaction.h
> +++ b/fs/btrfs/transaction.h
> @@ -7,12 +7,12 @@
>  #define BTRFS_TRANSACTION_H
>
>  #include <linux/atomic.h>
> +#include <linux/build_bug.h>
>  #include <linux/refcount.h>
>  #include <linux/list.h>
>  #include <linux/time64.h>
>  #include <linux/mutex.h>
>  #include <linux/wait.h>
> -#include <linux/xarray.h>
>  #include "btrfs_inode.h"
>  #include "delayed-ref.h"
>
> @@ -23,6 +23,7 @@ struct btrfs_fs_info;
>  struct btrfs_root_item;
>  struct btrfs_root;
>  struct btrfs_path;
> +struct extent_buffer;
>
>  /*
>   * Signal that a direct IO write is in progress, to avoid deadlock for sync
> @@ -136,6 +137,17 @@ enum {
>
>  #define TRANS_EXTWRITERS       (__TRANS_START | __TRANS_ATTACH)
>
> +/*
> + * Number of extent buffers a transaction handle tracks for writeback
> + * inhibition. The CLOCK reference bits pack into a u32 so this must not exceed
> + * 32, and keeping it a power of two lets the compiler reduce the CLOCK hand
> + * modulo to a mask.
> + */
> +#define BTRFS_INHIBITED_EBS_SLOTS      8
> +static_assert(BTRFS_INHIBITED_EBS_SLOTS <= 32);
> +static_assert(BTRFS_INHIBITED_EBS_SLOTS != 0 &&
> +             (BTRFS_INHIBITED_EBS_SLOTS & (BTRFS_INHIBITED_EBS_SLOTS - 1)) == 0);
> +
>  struct btrfs_trans_handle {
>         u64 transid;
>         u64 bytes_reserved;
> @@ -163,8 +175,11 @@ struct btrfs_trans_handle {
>         struct btrfs_fs_info *fs_info;
>         struct list_head new_bgs;
>         struct btrfs_block_rsv delayed_rsv;
> -       /* Extent buffers with writeback inhibited by this handle. */
> -       struct xarray writeback_inhibited_ebs;
> +       /* Extent buffers this handle has inhibited writeback on. */
> +       struct extent_buffer *inhibited_ebs[BTRFS_INHIBITED_EBS_SLOTS];
> +       u32 inhibited_ebs_referenced;   /* CLOCK reference bit per slot */
> +       u32 nr_inhibited_ebs;
> +       u32 inhibited_ebs_hand;         /* CLOCK hand */

Comments about structure members should be always be in a line above
the member, and the first word should be capitalized, ending the
sentence with punctuation.
You did that well for inhibited_eb, but not the the other 2 members.

See:

https://btrfs.readthedocs.io/en/latest/dev/Development-notes.html#comments

Thanks.

>  };
>
>  /*
>
> base-commit: 12cdfd13afd5e929f359f0f9c0443e0cbd7f43fa
> --
> 2.53.0-Meta
>
>