linux-kernel - Re: [PATCH v2] btrfs: reduce chunk_map lookups in btrfs_map

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20240813124752.GN25962@twin.jikos.cz>
Date: Tue, 13 Aug 2024 14:47:52 +0200
From: David Sterba <dsterba@...e.cz>
To: Johannes Thumshirn <Johannes.Thumshirn@....com>
Cc: Filipe Manana <fdmanana@...nel.org>,
	Johannes Thumshirn <jth@...nel.org>, Chris Mason <clm@...com>,
	Josef Bacik <josef@...icpanda.com>, David Sterba <dsterba@...e.com>,
	"open list:BTRFS FILE SYSTEM" <linux-btrfs@...r.kernel.org>,
	open list <linux-kernel@...r.kernel.org>, Qu Wenruo <wqu@...e.com>
Subject: Re: [PATCH v2] btrfs: reduce chunk_map lookups in btrfs_map_block

On Tue, Aug 13, 2024 at 11:04:52AM +0000, Johannes Thumshirn wrote:
> On 13.08.24 12:57, Filipe Manana wrote:
> > On Tue, Aug 13, 2024 at 11:49 AM Johannes Thumshirn <jth@...nel.org> wrote:
> >>
> >> From: Johannes Thumshirn <johannes.thumshirn@....com>
> >>
> >> Currently we're calling btrfs_num_copies() before btrfs_get_chunk_map() in
> >> btrfs_map_block(). But btrfs_num_copies() itself does a chunk map lookup
> >> to be able to calculate the number of copies.
> >>
> >> So split out the code getting the number of copies from btrfs_num_copies()
> >> into a helper called btrfs_chunk_map_num_copies() and directly call it
> >> from btrfs_map_block() and btrfs_num_copies().
> >>
> >> This saves us one rbtree lookup per btrfs_map_block() invocation.
> >>
> >> Reviewed-by: Qu Wenruo <wqu@...e.com>
> >> Reviewed-by: Filipe Manana <fdmanana@...e.com>
> >> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@....com>
> >> ---
> >> Changes in v2:
> >> - Added Reviewed-bys
> >> - Reflowed comments
> >> - Moved non RAID56 cases to the end without an if
> >> Link to v1:
> >> https://lore.kernel.org/all/20240812165931.9106-1-jth@kernel.org/
> >>
> >>   fs/btrfs/volumes.c | 58 +++++++++++++++++++++++++---------------------
> >>   1 file changed, 32 insertions(+), 26 deletions(-)
> >>
> >> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> >> index e07452207426..796f6350a017 100644
> >> --- a/fs/btrfs/volumes.c
> >> +++ b/fs/btrfs/volumes.c
> >> @@ -5781,38 +5781,44 @@ void btrfs_mapping_tree_free(struct btrfs_fs_info *fs_info)
> >>          write_unlock(&fs_info->mapping_tree_lock);
> >>   }
> >>
> >> +static int btrfs_chunk_map_num_copies(struct btrfs_chunk_map *map)
> > 
> > Same as commented before, can be const.
> 
> No it can't:
> fs/btrfs/volumes.c:5784:8: warning: type qualifiers ignored on function 
> return type [-Wignored-qualifiers]
>   5784 | static const int btrfs_chunk_map_num_copies(struct 
> btrfs_chunk_map *map)
> 
> Or did you mean the const struct btrfs_chunk_map? That could be done but 
> I don't see a reason.

The reason is to improve coding standards. Compiler verifies accidental
changes, internal APIs are cleaner.

It's much easier to add the const when the code written first, otherwise
it's tedious to identify the functions later on. I've been doing
cleanups like that eg. in 2917f74102cf23afe or other patches with
'constify' in the subject.

There are some marginal effects on generated asm code too but it
requires sufficient amount of "constification" so there are no cases
that would otherwise ruin a potential optimization.