Re: [PATCH 4/6] btrfs: defrag: use auto kfree in defrag_one_range() for folios array

On Tue, Jun 30, 2026 at 9:05 PM David Sterba <[email protected]> wrote:
>
> On Tue, Jun 30, 2026 at 10:53:54AM +0100, Filipe Manana wrote:
> > On Mon, Jun 29, 2026 at 11:55 PM David Sterba <[email protected]> wrote:
> > > > @@ -1251,7 +1249,6 @@ static int defrag_one_range(struct btrfs_inode *inode, u64 start, u32 len,
> > > >               folio_unlock(folios[i]);
> > > >               folio_put(folios[i]);
> > > >       }
> > > > -     kfree(folios);
> > >
> > > There's non-trivial work following this freeing, down from
> >
> > I'd rather disagree it's non-trivial work.
> > Everything below kfree() is trivial, and it was placed there in patch
> > 1 by chance.
>
> See a rough call graph of functions below the removed kfree, that's far
> beyond trivial. We follow the principle of not holding resources for
> longer than necessary, this applies to memory too.

It's still fast stuff. We hold the inode's vfs lock higher up the call
chain, so the inode's block reserve spinlocks are very unlikely to be
busy.

Also, you are concerned about slightly delayed memory release, but why
is that more important than releasing excess space first? Holding
excess space for longer can have an heavy impact - blocking tasks and
triggering async reclaim (running delayed refs, delayed items, commit
transactions, etc).

If it were a path-freeing operation, I could agree, since we would be
holding locks on a btree and impacting concurrency.
But in this case, deciding between releasing memory first or releasing
excess space (and most of the time we won't have excess space to
release, as we checked for holes before locking the extent range,
etc), it's not clear what is most beneficial.

>
> What I count as trivial is:
>
> - several asignments of local variables, ponter dererences are ok
> - one shallow function call
> - called functions do simple expressions only
>
> Indicators of what's not trivial:
>
> - spin locks
> - structure traversal
> - unbounded calls like waiting
> - more allocations
> - nesting deeper than 2 calls
>
>
> defrag_one_range:
>
>   btrfs_delalloc_release_extents
>     spin_lock inode
>       btrfs_mod_outstanding_extents - minor things, trace point call
>       btrfs_calculate_inode_block_rsv_size
>         - several clculations
>         - block rsv spin lock
>     spin_unlock
>     btrfs_inode_rsv_release (1)
>       btrfs_block_rsv_release
>         block_rsv_release_bytes
>           spin lock block group
>           spin lock space info
>           - lots of assignments, pointer derefs, calculations
>       btrfs_qgroup_free_meta_prealloc (branched on qgroups)
>         - qgroup calculations, spin locks taken
>         btrfs_qgroup_free_refroot (2)
>           - spin locks for qgroups
>           find_qgroup_rb
>           - qgroup list traversal
>       btrfs_qgroup_convert_reserved_meta
>         - spin locks for qgroups
>         qgroup_convert_meta
>           - spin locks for qgroups
>           - qgroup iteration
>           - resource releases
>         add_root_meta_rsv
>           - spin lock for reservations
>
>   btrfs_delalloc_release_space
>     btrfs_delalloc_release_metadata
>       - spin lock of inode
>       btrfs_calculate_inode_block_rsv_size
>         - calculations, spin locks
>       btrfs_inode_rsv_release
>         - see (1)
>     btrfs_free_reserved_data_space
>       btrfs_free_reserved_data_space_noquota
>         btrfs_space_info_free_bytes_may_use
>           - spin lock for space info
>           btrfs_try_granting_tickets
>             - reservation processing
>       btrfs_qgroup_free_data
>         __btrfs_qgroup_release_data
>           btrfs_clear_record_extent_bits (no qgroups) (3)
>             btrfs_clear_extent_bit_changeset
>               - tree traversal
>           qgroup_free_reserved_data
>             - ulist traversal
>             btrfs_clear_record_extent_bits
>               - see (3)
>             btrfs_qgroup_free_refroot
>               - see (2)
>           btrfs_clear_record_extent_bits (qgroups)
>             - see (3)
>             btrfs_clear_extent_bit_changeset
>               - tree traversal
>           btrfs_qgroup_free_refroot
>             - see (2)
>
>   extent_changeset_free
>     ulist_release
>       - enumerate nodes, kfree
>     kfree
>