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

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

On 2022/4/20 23:16, David Sterba wrote:
> 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.

Ctags are really out-of-date now.
LSP is the future, just try clangd, handling macros is just a piece of cake.

Thanks,
Qu