Re: [PATCH v3 2/2] btrfs: use ilog2() to replace if () branches for btrfs_bg_flags_to_raid_index()

[David Sterba <dsterba@suse.cz>]

On Wed, Apr 20, 2022 at 08:41:13AM +0000, Johannes Thumshirn wrote:
> On 20/04/2022 10:38, Qu Wenruo wrote:
> >>> 1. Make sure RAID0 is always the lowest bit in PROFILE_MASK
> >>>     This is done by finding the first (least significant) bit set of
> >>>     RAID0 and PROFILE_MASK & ~RAID0.
> >>>
> >>> 2. Make sure RAID0 bit set beyond the highest bit of TYPE_MASK
> >>
> >> TBH I think this change obscures the code more than it improves it.
> >>
> > Right, that kinda makes sense.
> > 
> > Will update the patchset to remove that line if needed.
> 
> I think the whole patch makes the code harder to follow. As of now you can
> just look it up, now you have to look how the calculation is done etc.

I think the index is the least useful information about the profiles,
it's there just that we have a sequence representing the profile flags
that's usable for arrays, like the space infos. The on-disk definition
is the bit and that's the source, how exactly it's converted to the
index is IMHO just a detail.

> If you want to get rid of the branches (which I still don't see a reason for)
> have you considered creating a lookup table?

It's yet another place that keeps the mapping of the values open coded.

Possibly if we would want to take it farther, a single definition of the
index enums could be like

#define	DEFINE_RAID_INDEX(profile)				\
	BTRFS_RAID_##profile   = BTRFS_BG_FLAG_TO_INDEX(BTRFS_BLOCK_GROUP_##profile)

and then used like

enum raid_types {
	DEFINE_RAID_INDEX(RAID0),
	DEFINE_RAID_INDEX(RAID10),
	...
};

But that's obscuring what exactly does it define and we do use the plain
indexes like BTRFS_RAID_DUP in several places. It's sometimes annoying
that ctags don't locate all the setget helpers because of all the
BTRFS_SETGET_FUNCS macros.