On Sat, Oct 30, 2021 at 07:38:49AM +0800, Qu Wenruo wrote:

On 2021/10/29 22:11, David Sterba wrote:

quoted

On Wed, Oct 27, 2021 at 01:28:59PM +0800, Qu Wenruo wrote:

quoted

+#define BTRFS_RAID_SHIFT	(ilog2(BTRFS_BLOCK_GROUP_RAID0) - 1)
+
  enum btrfs_raid_types {
-	BTRFS_RAID_RAID10,
-	BTRFS_RAID_RAID1,
-	BTRFS_RAID_DUP,
-	BTRFS_RAID_RAID0,
-	BTRFS_RAID_SINGLE,
-	BTRFS_RAID_RAID5,
-	BTRFS_RAID_RAID6,
-	BTRFS_RAID_RAID1C3,
-	BTRFS_RAID_RAID1C4,
+	BTRFS_RAID_SINGLE  = 0,
+	BTRFS_RAID_RAID0   = ilog2(BTRFS_BLOCK_GROUP_RAID0 >> BTRFS_RAID_SHIFT),
+	BTRFS_RAID_RAID1   = ilog2(BTRFS_BLOCK_GROUP_RAID1 >> BTRFS_RAID_SHIFT),
+	BTRFS_RAID_DUP     = ilog2(BTRFS_BLOCK_GROUP_DUP >> BTRFS_RAID_SHIFT),
+	BTRFS_RAID_RAID10  = ilog2(BTRFS_BLOCK_GROUP_RAID10 >> BTRFS_RAID_SHIFT),
+	BTRFS_RAID_RAID5   = ilog2(BTRFS_BLOCK_GROUP_RAID5 >> BTRFS_RAID_SHIFT),
+	BTRFS_RAID_RAID6   = ilog2(BTRFS_BLOCK_GROUP_RAID6 >> BTRFS_RAID_SHIFT),
+	BTRFS_RAID_RAID1C3 = ilog2(BTRFS_BLOCK_GROUP_RAID1C3 >> BTRFS_RAID_SHIFT),
+	BTRFS_RAID_RAID1C4 = ilog2(BTRFS_BLOCK_GROUP_RAID1C4 >> BTRFS_RAID_SHIFT),

Please use const_ilog2 in case all the terms in the expression are
constants that can be evaluated at the enum definition time.

Why? Kernel ilog2() is already handling the constants.

That's the biggest difference between kernel ilog2() and btrfs-progs
ilog2().

Only in btrfs-progs we need const_ilog2().

In fact, the comment of kernel ilog2() already shows this:

/**
  * ilog2 - log base 2 of 32-bit or a 64-bit unsigned value
  * @n: parameter
  *
  * constant-capable log of base 2 calculation
  * - this can be used to initialise global variables from constant
data, hence
  * the massive ternary operator construction
  *
  * selects the appropriately-sized optimised version depending on sizeof(n)
  */

Right, ilog2 is fine.

quoted

I agree that deriving the indexes from the flags is safe but all the
magic around that scares me a bit.

That's indeed a little concerning.

That's why I spare no code to make it as hard as possible to break, like
all the explicit definition of each BTRFS_RAID_* number.

Yeah, as somebody else commented adding a wrapper for the definitions
may be more explicit, ie. the part

	ilog2(BTRFS_BLOCK_GROUP_RAID1C4 >> BTRFS_RAID_SHIFT)

hiding that it's ilog2 and doing the shift

	DEFINE_IT(BTRFS_BLOCK_GROUP_RAID1C4)

using a macro should be all fine for the enum so it's all compile-time
constant.

To me the only possible problem in the future is someone adding two or
more profiles in one go, and possibly corrupt the BTRFS_NR_RAID_TYPES.

I may have two more profiles to add, I'll add them one by one anyway as
it's the cleaner.