mbox series

[00/27] Struct btrfs_path auto cleaning conversions

Message ID cover.1740562070.git.dsterba@suse.com (mailing list archive)
Headers show
Series Struct btrfs_path auto cleaning conversions | expand

Message

David Sterba Feb. 26, 2025, 9:50 a.m. UTC 
We have the scoped freeing for struct btrfs_path but it's not used as
much as it could. This patchset converts the easy cases and it's also a
preview if we really want to do that. It makes understanding the exit
paths a bit less obvious, but so far I think it's manageable.

The path is used in many functions and following a few simple patterns,
with the macro BTRFS_PATH_AUTO_FREE quite visible among the
declarations, so it's nothing hard to be aware of that when reading the
code.

The conversion has been done on half of the files, so if somebody wants
to continue, feel free. I've skipped functions with more complicated
branching where the auto freeing would make it worse.

David Sterba (27):
  btrfs: use BTRFS_PATH_AUTO_FREE in sample_block_group_extent_item()
  btrfs: use BTRFS_PATH_AUTO_FREE in insert_dev_extent()
  btrfs: use BTRFS_PATH_AUTO_FREE in btrfs_setup_space_cache()
  btrfs: use BTRFS_PATH_AUTO_FREE in btrfs_start_dirty_block_groups()
  btrfs: use BTRFS_PATH_AUTO_FREE in btrfs_write_dirty_block_groups()
  btrfs: use BTRFS_PATH_AUTO_FREE in btrfs_insert_item()
  btrfs: use BTRFS_PATH_AUTO_FREE in btrfs_commit_inode_delayed_items()
  btrfs: use BTRFS_PATH_AUTO_FREE in btrfs_init_dev_replace()
  btrfs: use BTRFS_PATH_AUTO_FREE in btrfs_run_dev_replace()
  btrfs: use BTRFS_PATH_AUTO_FREE in btrfs_check_dir_item_collision()
  btrfs: use BTRFS_PATH_AUTO_FREE in btrfs_read_tree_root()
  btrfs: use BTRFS_PATH_AUTO_FREE in load_global_roots()
  btrfs: use BTRFS_PATH_AUTO_FREE in btrfs_init_root_free_objectid()
  btrfs: use BTRFS_PATH_AUTO_FREE in btrfs_get_name()
  btrfs: use BTRFS_PATH_AUTO_FREE in btrfs_lookup_data_extent()
  btrfs: use BTRFS_PATH_AUTO_FREE in btrfs_lookup_extent_info()
  btrfs: use BTRFS_PATH_AUTO_FREE in __btrfs_inc_extent_ref()
  btrfs: use BTRFS_PATH_AUTO_FREE in run_delayed_extent_op()
  btrfs: use BTRFS_PATH_AUTO_FREE in check_ref_exists()
  btrfs: use BTRFS_PATH_AUTO_FREE in btrfs_drop_subtree()
  btrfs: use BTRFS_PATH_AUTO_FREE in btrfs_insert_hole_extent()
  btrfs: use BTRFS_PATH_AUTO_FREE in btrfs_lookup_bio_sums()
  btrfs: use BTRFS_PATH_AUTO_FREE in btrfs_del_csums()
  btrfs: use BTRFS_PATH_AUTO_FREE in btrfs_remove_free_space_inode()
  btrfs: use BTRFS_PATH_AUTO_FREE in populate_free_space_tree()
  btrfs: use BTRFS_PATH_AUTO_FREE in clear_free_space_tree()
  btrfs: use BTRFS_PATH_AUTO_FREE in load_free_space_tree()

 fs/btrfs/block-group.c      | 20 ++++++----------
 fs/btrfs/ctree.c            |  3 +--
 fs/btrfs/delayed-inode.c    |  3 +--
 fs/btrfs/dev-replace.c      | 32 ++++++++++---------------
 fs/btrfs/dir-item.c         | 24 +++++++------------
 fs/btrfs/disk-io.c          | 29 ++++++++++-------------
 fs/btrfs/export.c           | 10 +++-----
 fs/btrfs/extent-tree.c      | 47 ++++++++++++++-----------------------
 fs/btrfs/file-item.c        | 17 +++++---------
 fs/btrfs/free-space-cache.c | 13 ++++------
 fs/btrfs/free-space-tree.c  | 45 ++++++++++++++---------------------
 11 files changed, 91 insertions(+), 152 deletions(-)

Comments

Johannes Thumshirn Feb. 27, 2025, 7:09 a.m. UTC | #1 
On 26.02.25 10:51, David Sterba wrote:
> We have the scoped freeing for struct btrfs_path but it's not used as
> much as it could. This patchset converts the easy cases and it's also a
> preview if we really want to do that. It makes understanding the exit
> paths a bit less obvious, but so far I think it's manageable.
> 
> The path is used in many functions and following a few simple patterns,
> with the macro BTRFS_PATH_AUTO_FREE quite visible among the
> declarations, so it's nothing hard to be aware of that when reading the
> code.
> 
> The conversion has been done on half of the files, so if somebody wants
> to continue, feel free. I've skipped functions with more complicated
> branching where the auto freeing would make it worse.

Let's see where this will lead us to.

Anyways, apart from the 2 small nits,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
David Sterba Feb. 27, 2025, 9:55 a.m. UTC | #2 
On Wed, Feb 26, 2025 at 10:50:40AM +0100, David Sterba wrote:
> We have the scoped freeing for struct btrfs_path but it's not used as
> much as it could. This patchset converts the easy cases and it's also a
> preview if we really want to do that. It makes understanding the exit
> paths a bit less obvious, but so far I think it's manageable.
> 
> The path is used in many functions and following a few simple patterns,
> with the macro BTRFS_PATH_AUTO_FREE quite visible among the
> declarations, so it's nothing hard to be aware of that when reading the
> code.
> 
> The conversion has been done on half of the files, so if somebody wants
> to continue, feel free. I've skipped functions with more complicated
> branching where the auto freeing would make it worse.
> 
> David Sterba (27):
>   btrfs: use BTRFS_PATH_AUTO_FREE in sample_block_group_extent_item()
>   btrfs: use BTRFS_PATH_AUTO_FREE in insert_dev_extent()
>   btrfs: use BTRFS_PATH_AUTO_FREE in btrfs_setup_space_cache()
>   btrfs: use BTRFS_PATH_AUTO_FREE in btrfs_start_dirty_block_groups()
>   btrfs: use BTRFS_PATH_AUTO_FREE in btrfs_write_dirty_block_groups()
>   btrfs: use BTRFS_PATH_AUTO_FREE in btrfs_insert_item()
>   btrfs: use BTRFS_PATH_AUTO_FREE in btrfs_commit_inode_delayed_items()
>   btrfs: use BTRFS_PATH_AUTO_FREE in btrfs_init_dev_replace()
>   btrfs: use BTRFS_PATH_AUTO_FREE in btrfs_run_dev_replace()
>   btrfs: use BTRFS_PATH_AUTO_FREE in btrfs_check_dir_item_collision()
>   btrfs: use BTRFS_PATH_AUTO_FREE in btrfs_read_tree_root()
>   btrfs: use BTRFS_PATH_AUTO_FREE in load_global_roots()
>   btrfs: use BTRFS_PATH_AUTO_FREE in btrfs_init_root_free_objectid()
>   btrfs: use BTRFS_PATH_AUTO_FREE in btrfs_get_name()
>   btrfs: use BTRFS_PATH_AUTO_FREE in btrfs_lookup_data_extent()
>   btrfs: use BTRFS_PATH_AUTO_FREE in btrfs_lookup_extent_info()
>   btrfs: use BTRFS_PATH_AUTO_FREE in __btrfs_inc_extent_ref()
>   btrfs: use BTRFS_PATH_AUTO_FREE in run_delayed_extent_op()
>   btrfs: use BTRFS_PATH_AUTO_FREE in check_ref_exists()
>   btrfs: use BTRFS_PATH_AUTO_FREE in btrfs_drop_subtree()
>   btrfs: use BTRFS_PATH_AUTO_FREE in btrfs_insert_hole_extent()
>   btrfs: use BTRFS_PATH_AUTO_FREE in btrfs_lookup_bio_sums()
>   btrfs: use BTRFS_PATH_AUTO_FREE in btrfs_del_csums()
>   btrfs: use BTRFS_PATH_AUTO_FREE in btrfs_remove_free_space_inode()
>   btrfs: use BTRFS_PATH_AUTO_FREE in populate_free_space_tree()
>   btrfs: use BTRFS_PATH_AUTO_FREE in clear_free_space_tree()
>   btrfs: use BTRFS_PATH_AUTO_FREE in load_free_space_tree()

Daniel noted that the trivial patches are maybe too trivial and should
be grouped into fewer patches. I agree after looking at the series now,
so I'll rework it
Johannes Thumshirn Feb. 27, 2025, 10:56 a.m. UTC | #3 
On 27.02.25 10:55, David Sterba wrote:
> Daniel noted that the trivial patches are maybe too trivial and should
> be grouped into fewer patches. I agree after looking at the series now,
> so I'll rework it

Agreed, though I must say it was super nice for reviewing them, one tiny 
little change at a time.
David Sterba Feb. 27, 2025, 1:53 p.m. UTC | #4 
On Thu, Feb 27, 2025 at 10:56:57AM +0000, Johannes Thumshirn wrote:
> On 27.02.25 10:55, David Sterba wrote:
> > Daniel noted that the trivial patches are maybe too trivial and should
> > be grouped into fewer patches. I agree after looking at the series now,
> > so I'll rework it
> 
> Agreed, though I must say it was super nice for reviewing them, one tiny 
> little change at a time.

Yeah, I think grouping the simple declaration/free removal patches to
one would still be easy to review as the scope is always just one
function and nothing is needed to be kept in memory. Which roughly cuts
the size of the patchset to half.