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

[Qu Wenruo <wqu@suse.com>]

On 2022/4/20 16:41, Johannes Thumshirn wrote:
> On 20/04/2022 10:38, Qu Wenruo wrote:
>>
>>
>> On 2022/4/20 16:25, Johannes Thumshirn wrote:
>>> On 20/04/2022 10:09, Qu Wenruo wrote:
>>>> In function btrfs_bg_flags_to_raid_index(), we use quite some if () to
>>>> convert the BTRFS_BLOCK_GROUP_* bits to a index number.
>>>>
>>>> But the truth is, there is really no such need for so many branches at
>>>> all.
>>>> Since all BTRFS_BLOCK_GROUP_* flags are just one single bit set inside
>>>> BTRFS_BLOCK_GROUP_PROFILES_MASK, we can easily use ilog2() to calculate
>>>> their values.
>>>>
>>>> This calculation has an anchor point, the lowest PROFILE bit, which is
>>>> RAID0.
>>>>
>>>> Even it's fixed on-disk format and should never change, here I added
>>>> extra compile time checks to make it super safe:
>>>>
>>>> 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.

The problem is, why you need to look the index up?

The index is pretty straight forward, just a enum for each profile, one 
should not really bother whatever the value is.

> 
> If you want to get rid of the branches (which I still don't see a reason for)

The new code is just 5 asm commands on x86_64.

> have you considered creating a lookup table?

The index is used for the lookup table of btrfs_raid_type.

Thanks,
Qu