[PATCH] btrfs: skip the extent map tree lock for inodes without extent maps

[PATCH] btrfs: skip the extent map tree lock for inodes without extent maps

[Breno Leitao]

The shrinker (find_first_inode_to_shrink()) takes inode->extent_tree.lock in write
mode on every inode it walks, only to find out whether that inode has
any extent maps.

Most have none, from what I understand, so, avoid the lock  by testing the
tree with a plain read before taking the lock. tree->root is only
modified with the tree lock held for write, so the unlocked read is
a benign race: a false negative just defers the inode to a later scan.

On my tests, find_first_inode_to_shrink() was a bit faster, so, if this
patch is correct, I think it is worth having to reduce lock contention.

Signed-off-by: Breno Leitao <leitao@debian.org>
---
 fs/btrfs/extent_map.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/btrfs/extent_map.c b/fs/btrfs/extent_map.c
index 6ad7b39ae358b..cdcd2b779050d 100644
--- a/fs/btrfs/extent_map.c
+++ b/fs/btrfs/extent_map.c
@@ -1219,6 +1219,9 @@ static struct btrfs_inode *find_first_inode_to_shrink(struct btrfs_root *root,
 
 		tree = &inode->extent_tree;
 
+		if (!READ_ONCE(tree->root.rb_node))
+			goto next;
+
 		/*
 		 * We want to be fast so if the lock is busy we don't want to
 		 * spend time waiting for it (some task is about to do IO for

---
base-commit: 6a746cd265aed59107ebdaa9ce039bb832922969
change-id: 20260820-b4-btrfs-em-shrinker-7382d7f0dd05

Best regards,
--  
Breno Leitao <leitao@debian.org>

Re: [PATCH] btrfs: skip the extent map tree lock for inodes without extent maps

[Qu Wenruo]

在 2026/8/21 20:08, Breno Leitao 写道:
> The shrinker (find_first_inode_to_shrink()) takes inode->extent_tree.lock in write
> mode on every inode it walks, only to find out whether that inode has
> any extent maps.
> 
> Most have none, from what I understand, so, avoid the lock  by testing the
> tree with a plain read before taking the lock. tree->root is only
> modified with the tree lock held for write, so the unlocked read is
> a benign race: a false negative just defers the inode to a later scan.
> 
> On my tests, find_first_inode_to_shrink() was a bit faster, so, if this
> patch is correct, I think it is worth having to reduce lock contention.

How much faster?

We are using write_trylock() already, meaning if it's not locked we 
should get the lock immediately, otherwise we skip the inode.

So the lock contention should be low already.

Furthermore, if there are some hidden concurrency bugs, it will be very 
hard to debug.

I strongly prefer to stick to the existing locking scheme, unless you 
have a very strong argument not to.

> 
> Signed-off-by: Breno Leitao <leitao@debian.org>
> ---
>   fs/btrfs/extent_map.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/fs/btrfs/extent_map.c b/fs/btrfs/extent_map.c
> index 6ad7b39ae358b..cdcd2b779050d 100644
> --- a/fs/btrfs/extent_map.c
> +++ b/fs/btrfs/extent_map.c
> @@ -1219,6 +1219,9 @@ static struct btrfs_inode *find_first_inode_to_shrink(struct btrfs_root *root,
>   
>   		tree = &inode->extent_tree;
>   
> +		if (!READ_ONCE(tree->root.rb_node))
> +			goto next;
> +
>   		/*
>   		 * We want to be fast so if the lock is busy we don't want to
>   		 * spend time waiting for it (some task is about to do IO for
> 
> ---
> base-commit: 6a746cd265aed59107ebdaa9ce039bb832922969
> change-id: 20260820-b4-btrfs-em-shrinker-7382d7f0dd05
> 
> Best regards,
> --
> Breno Leitao <leitao@debian.org>
> 

Re: [PATCH] btrfs: skip the extent map tree lock for inodes without extent maps

[Breno Leitao]

Hello Qu,

On Fri, Aug 21, 2026 at 08:43:00PM +0930, Qu Wenruo wrote:
> 在 2026/8/21 20:08, Breno Leitao 写道:
> > The shrinker (find_first_inode_to_shrink()) takes inode->extent_tree.lock in write
> > mode on every inode it walks, only to find out whether that inode has
> > any extent maps.
> > 
> > Most have none, from what I understand, so, avoid the lock  by testing the
> > tree with a plain read before taking the lock. tree->root is only
> > modified with the tree lock held for write, so the unlocked read is
> > a benign race: a false negative just defers the inode to a later scan.
> > 
> > On my tests, find_first_inode_to_shrink() was a bit faster, so, if this
> > patch is correct, I think it is worth having to reduce lock contention.
> 
> How much faster?

Filipe has asked the same question, and I used that thread to reply,
let's use that thread for performance discussions.

> We are using write_trylock() already, meaning if it's not locked we should
> get the lock immediately, otherwise we skip the inode.

You are right that the shrinker itself never waits. But there are two
costs left that the trylock does not avoid:

1) Even uncontended, the atomic is not free.  write_trylock() is a
   cmpxchg that has to pull the extent_map_tree cache line in
   exclusive.

2) The trylock only protects the shrinker from waiting.  When it
   succeeds we do hold the lock, briefly, on an inode we are about to
   skip anyway, and anyone arriving in that window block

While my microbenchmark results in Filipe's answer might be skewed
toward this case, the fleet profiler shows find_first_inode_to_shrink()
is far from negligible in production.

Re: [PATCH] btrfs: skip the extent map tree lock for inodes without extent maps

[Filipe Manana]

On Fri, Aug 21, 2026 at 11:39 AM Breno Leitao <leitao@debian.org> wrote:
>
> The shrinker (find_first_inode_to_shrink()) takes inode->extent_tree.lock in write
> mode on every inode it walks, only to find out whether that inode has
> any extent maps.
>
> Most have none, from what I understand, so, avoid the lock  by testing the
> tree with a plain read before taking the lock. tree->root is only
> modified with the tree lock held for write, so the unlocked read is
> a benign race: a false negative just defers the inode to a later scan.
>
> On my tests, find_first_inode_to_shrink() was a bit faster, so, if this
> patch is correct, I think it is worth having to reduce lock contention.

When claiming performance gains in a patch, please always mention what
those gains were....

>
> Signed-off-by: Breno Leitao <leitao@debian.org>
> ---
>  fs/btrfs/extent_map.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/fs/btrfs/extent_map.c b/fs/btrfs/extent_map.c
> index 6ad7b39ae358b..cdcd2b779050d 100644
> --- a/fs/btrfs/extent_map.c
> +++ b/fs/btrfs/extent_map.c
> @@ -1219,6 +1219,9 @@ static struct btrfs_inode *find_first_inode_to_shrink(struct btrfs_root *root,
>
>                 tree = &inode->extent_tree;
>
> +               if (!READ_ONCE(tree->root.rb_node))

Please use the proper rbtree api to check if a tree is empty:

RB_EMPTY_ROOT(&tree->root)

That's a lot more elegant and hides the use of READ_ONCE().

But really when there are harmless races, I like to see an explicit
data_race() annotation and a comment about why the race is harmless.

Nevertheless, first, I would like to know how much the performance
improvement was...

Thanks.



> +                       goto next;
> +
>                 /*
>                  * We want to be fast so if the lock is busy we don't want to
>                  * spend time waiting for it (some task is about to do IO for
>
> ---
> base-commit: 6a746cd265aed59107ebdaa9ce039bb832922969
> change-id: 20260820-b4-btrfs-em-shrinker-7382d7f0dd05
>
> Best regards,
> --
> Breno Leitao <leitao@debian.org>
>
>

Re: [PATCH] btrfs: skip the extent map tree lock for inodes without extent maps

[Breno Leitao]

Hello Filipe,

On Fri, Aug 21, 2026 at 12:17:26PM +0100, Filipe Manana wrote:
> On Fri, Aug 21, 2026 at 11:39 AM Breno Leitao <leitao@debian.org> wrote:
> > +               if (!READ_ONCE(tree->root.rb_node))
> 
> Please use the proper rbtree api to check if a tree is empty:
> 
> RB_EMPTY_ROOT(&tree->root)
> 
> That's a lot more elegant and hides the use of READ_ONCE().
> 
> But really when there are harmless races, I like to see an explicit
> data_race() annotation and a comment about why the race is harmless.
> 
> Nevertheless, first, I would like to know how much the performance
> improvement was...

Sorry for not posting it earlier.

How I got into this: I've found while profiling arm64 production hosts
that had high system utilization and : _raw_write_trylock and xa_find
dominated, and the callchain pointed at find_first_inode_to_shrink()
locking every inode it walks.

Looking at the whole the Meta production fleet, the extent map shrinker
is ~0.35% of non-idle kernel CPU.  Within it, and attributing callees to
their caller, find_first_inode_to_shrink() is ~65% and the
write_trylock() it does is ~30% of the whole shrinker. Two thirds of
what the shrinker costs is looking for inodes that have anything to
shrink, and half of that is a lock we do not need to take.


micro benchmarks
================

I've hacked up a 6 GiB btrfs on a loop device, 100000 empty files
kept open, plus 200 1 MiB files created last, so they have the highest
inode numbers and every scan has to walk all the empty ones first.

Each round drops the page cache, re-reads the data files to recreate the
extent maps, then triggers the shrinker with "echo
2 > /proc/sys/vm/drop_caches". 15 rounds per run, arm64 (Neoverse V2),
8 CPUs, no lock debugging, cost of find_first_inode_to_shrink() from the
 ftrace function profiler:

  ns per inode walked, median of runs

                          base   patched    delta
    idle                  46.4      40.1   -13.6%
    4 concurrent readers  47.8      38.4   -19.7%

A separate build with CONFIG_LOCK_STAT, same test, for the extent map
tree rwlock.  Note the shrinker is not the only user of that lock, every
extent map insert and lookup takes it too, which is why the acquisition
count drops by two thirds rather than to nothing:

                              base   patched    delta
    write acquisitions      628016    228000   -63.7%
    hold time total (us)     47512     22717   -52.2%
    acq cacheline bounces     1574      1288   -18.2%

Does it help?

Thanks for the quick reply,
--breno

Re: [PATCH] btrfs: skip the extent map tree lock for inodes without extent maps

[Filipe Manana]

On Fri, Aug 21, 2026 at 5:07 PM Breno Leitao <leitao@debian.org> wrote:
>
> Hello Filipe,
>
> On Fri, Aug 21, 2026 at 12:17:26PM +0100, Filipe Manana wrote:
> > On Fri, Aug 21, 2026 at 11:39 AM Breno Leitao <leitao@debian.org> wrote:
> > > +               if (!READ_ONCE(tree->root.rb_node))
> >
> > Please use the proper rbtree api to check if a tree is empty:
> >
> > RB_EMPTY_ROOT(&tree->root)
> >
> > That's a lot more elegant and hides the use of READ_ONCE().
> >
> > But really when there are harmless races, I like to see an explicit
> > data_race() annotation and a comment about why the race is harmless.
> >
> > Nevertheless, first, I would like to know how much the performance
> > improvement was...
>
> Sorry for not posting it earlier.
>
> How I got into this: I've found while profiling arm64 production hosts
> that had high system utilization and : _raw_write_trylock and xa_find
> dominated, and the callchain pointed at find_first_inode_to_shrink()
> locking every inode it walks.
>
> Looking at the whole the Meta production fleet, the extent map shrinker
> is ~0.35% of non-idle kernel CPU.  Within it, and attributing callees to
> their caller, find_first_inode_to_shrink() is ~65% and the
> write_trylock() it does is ~30% of the whole shrinker. Two thirds of
> what the shrinker costs is looking for inodes that have anything to
> shrink, and half of that is a lock we do not need to take.
>
>
> micro benchmarks
> ================
>
> I've hacked up a 6 GiB btrfs on a loop device, 100000 empty files
> kept open, plus 200 1 MiB files created last, so they have the highest
> inode numbers and every scan has to walk all the empty ones first.
>
> Each round drops the page cache, re-reads the data files to recreate the
> extent maps, then triggers the shrinker with "echo
> 2 > /proc/sys/vm/drop_caches". 15 rounds per run, arm64 (Neoverse V2),
> 8 CPUs, no lock debugging, cost of find_first_inode_to_shrink() from the
>  ftrace function profiler:
>
>   ns per inode walked, median of runs
>
>                           base   patched    delta
>     idle                  46.4      40.1   -13.6%
>     4 concurrent readers  47.8      38.4   -19.7%
>
> A separate build with CONFIG_LOCK_STAT, same test, for the extent map
> tree rwlock.  Note the shrinker is not the only user of that lock, every
> extent map insert and lookup takes it too, which is why the acquisition
> count drops by two thirds rather than to nothing:
>
>                               base   patched    delta
>     write acquisitions      628016    228000   -63.7%
>     hold time total (us)     47512     22717   -52.2%
>     acq cacheline bounces     1574      1288   -18.2%
>
> Does it help?

Yes, and all that should be in the changelog.

Thanks.
>
> Thanks for the quick reply,
> --breno
>