All of lore.kernel.org
 help / color / mirror / Atom feed
From: Johannes Thumshirn <Johannes.Thumshirn@wdc.com>
To: Qu Wenruo <quwenruo.btrfs@gmx.com>, Qu Wenruo <wqu@suse.com>,
	"linux-btrfs@vger.kernel.org" <linux-btrfs@vger.kernel.org>
Subject: Re: [PATCH v3 2/2] btrfs: use ilog2() to replace if () branches for btrfs_bg_flags_to_raid_index()
Date: Wed, 20 Apr 2022 08:41:13 +0000	[thread overview]
Message-ID: <PH0PR04MB741682A87F86554F81F5AE839BF59@PH0PR04MB7416.namprd04.prod.outlook.com> (raw)
In-Reply-To: e57d9b3e-94fc-a20f-ff92-ccf19c0b035b@gmx.com

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.

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?


  reply	other threads:[~2022-04-20  8:41 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-04-20  8:08 [PATCH v3 0/2] btrfs: re-define btrfs_raid_types Qu Wenruo
2022-04-20  8:08 ` [PATCH v3 1/2] btrfs: move definition of btrfs_raid_types to volumes.h Qu Wenruo
2022-04-20  8:13   ` Johannes Thumshirn
2022-04-20  8:08 ` [PATCH v3 2/2] btrfs: use ilog2() to replace if () branches for btrfs_bg_flags_to_raid_index() Qu Wenruo
2022-04-20  8:25   ` Johannes Thumshirn
2022-04-20  8:38     ` Qu Wenruo
2022-04-20  8:41       ` Johannes Thumshirn [this message]
2022-04-20  9:45         ` Qu Wenruo
2022-04-20 15:16         ` David Sterba
2022-04-20 23:01           ` Qu Wenruo
2022-04-22 20:30 ` [PATCH v3 0/2] btrfs: re-define btrfs_raid_types David Sterba

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=PH0PR04MB741682A87F86554F81F5AE839BF59@PH0PR04MB7416.namprd04.prod.outlook.com \
    --to=johannes.thumshirn@wdc.com \
    --cc=linux-btrfs@vger.kernel.org \
    --cc=quwenruo.btrfs@gmx.com \
    --cc=wqu@suse.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.