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

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)
  */

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.

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.

But all BTRFS_RAID_* number should be pretty safe.

Thanks,
Qu