Re: [PATCH v4 1/3] btrfs: implement ref_tracker for delayed_nodes

[Leo Martins <loemra.dev@gmail.com>]

On Wed, 13 Aug 2025 14:50:52 +0200 David Sterba <dsterba@suse.cz> wrote:

> On Tue, Aug 12, 2025 at 04:04:39PM -0700, Leo Martins wrote:
> > This patch adds ref_tracker infrastructure for btrfs_delayed_node.
> > 
> > It is a response to the largest btrfs related crash in our fleet.
> > We're seeing softlockups in btrfs_kill_all_delayed_nodes that seem
> > to be a result of delayed_nodes not being released properly.
> > 
> > A ref_tracker object is allocated on reference count increases and
> > freed on reference count decreases. The ref_tracker object stores
> > a stack trace of where it is allocated. The ref_tracker_dir object
> > is embedded in btrfs_delayed_node and keeps track of all current
> > and some old/freed ref_tracker objects. When a leak is detected
> > we can print the stack traces for all ref_trackers that have not
> > yet been freed.
> > 
> > Here is a common example of taking a reference to a delayed_node
> > and freeing it with ref_tracker.
> > 
> > ```C
> > struct btrfs_ref_tracker tracker;
> > struct btrfs_delayed_node *node;
> > 
> > node = btrfs_get_delayed_node(inode, &tracker);
> > // use delayed_node...
> > btrfs_release_delayed_node(node, &tracker);
> > ```
> > 
> > There are two special cases where the delayed_node reference is "long
> > lived", meaning that the thread that takes the reference and the thread
> > that releases the reference are different. The `inode_cache_tracker`
> > tracks the delayed_node stored in btrfs_inode. The `node_list_tracker`
> > tracks the delayed_node stored in the btrfs_delayed_root
> > node_list/prepare_list. These trackers are embedded in the
> > btrfs_delayed_node.
> > 
> > btrfs_ref_tracker and btrfs_ref_tracker_dir are wrappers that either
> > compile to the corresponding ref_tracker structs or empty structs
> > depending on CONFIG_BTRFS_DEBUG. There are also btrfs wrappers for
> > the ref_tracker API.
> > 
> > Signed-off-by: Leo Martins <loemra.dev@gmail.com>
> 
> There's some witespace damage that fails when the patch is applied by
> 'git am', it can be fixed manually but please fix that next time.

Sorry, totally my fault, did not run checkpatch before sending out.
Will fix next time.

> 
> > ---
> >  fs/btrfs/Kconfig         |   3 +-
> >  fs/btrfs/delayed-inode.c | 192 ++++++++++++++++++++++++++++-----------
> >  fs/btrfs/delayed-inode.h |  70 ++++++++++++++
> >  3 files changed, 209 insertions(+), 56 deletions(-)
> > 
> > diff --git a/fs/btrfs/Kconfig b/fs/btrfs/Kconfig
> > index c352f3ae0385..2745de514196 100644
> > --- a/fs/btrfs/Kconfig
> > +++ b/fs/btrfs/Kconfig
> > @@ -61,7 +61,8 @@ config BTRFS_FS_RUN_SANITY_TESTS
> >  
> >  config BTRFS_DEBUG
> >  	bool "Btrfs debugging support"
> > -	depends on BTRFS_FS
> > +	depends on BTRFS_FS && STACKTRACE_SUPPORT
> 
> How does this work? If STACKTRACE_SUPPORT is not enabled then we can't
> enable BTRFS_DEBUG?

That's correct, my understanding is that STACKTRACE_SUPPORT is something
configured by different architectures based on whether or not they
support stacktraces. Maybe it would be better to do something like

select REF_TRACKER if STACKTRACE_SUPPORT

so we can still use DEBUG on architectures that don't support stacktraces,
though I can't imagine they would be very relevant.

> 
> > +	select REF_TRACKER
> >  	help
> >  	  Enable run-time debugging support for the btrfs filesystem.
> >  
> 
> > @@ -78,6 +95,12 @@ struct btrfs_delayed_node {
> >  	 * actual number of leaves we end up using. Protected by @mutex.
> >  	 */
> >  	u32 index_item_leaves;
> > +	/* Used to track all references to this delayed node. */
> > +	struct btrfs_ref_tracker_dir ref_dir;
> > + 	/* Used to track delayed node reference stored in node list. */
> > +	struct btrfs_ref_tracker node_list_tracker;
> > + 	/* Used to track delayed node reference stored in inode cache. */
> > +	struct btrfs_ref_tracker inode_cache_tracker;
> 
> Some of the comments have mixed space and tabs in the initial space.

Sorry, again.

> 
> >  };
> >  
> >  struct btrfs_delayed_item {
> > @@ -169,4 +192,51 @@ void __cold btrfs_delayed_inode_exit(void);
> >  /* for debugging */
> >  void btrfs_assert_delayed_root_empty(struct btrfs_fs_info *fs_info);
> >  
> > +#define BTRFS_DELAYED_NODE_REF_TRACKER_QUARANTINE_COUNT 16
> > +#define BTRFS_DELAYED_NODE_REF_TRACKER_DISPLAY_LIMIT 16
> > +
> > +#ifdef CONFIG_BTRFS_DEBUG
> > +static inline void btrfs_delayed_node_ref_tracker_dir_init(struct btrfs_delayed_node *node)
> > +{
> > +	ref_tracker_dir_init(&node->ref_dir.dir, 
> 
> Trailing space.
> 
> > +			     BTRFS_DELAYED_NODE_REF_TRACKER_QUARANTINE_COUNT,
> > +			     "delayed_node");
> > +}

Sent using hkml (https://github.com/sjp38/hackermail)