* [PATCH v2 0/2] btrfs: re-define btrfs_raid_types
@ 2021-10-27 5:28 Qu Wenruo
2021-10-27 5:28 ` [PATCH v2 1/2] btrfs: move definition of btrfs_raid_types to volumes.h Qu Wenruo
2021-10-27 5:28 ` [PATCH v2 2/2] btrfs: use ilog2() to replace if () branches for btrfs_bg_flags_to_raid_index() Qu Wenruo
0 siblings, 2 replies; 13+ messages in thread
From: Qu Wenruo @ 2021-10-27 5:28 UTC (permalink / raw)
To: linux-btrfs
By the nature of BTRFS_BLOCK_GROUP_* profiles, converting the flag into
an index should only need one bits AND, one if () check for SINGLE
profile, one right shift to align the values, one ilog2() call which is
normally converted into ffs() assembly code.
But we're using a lot of if () branches to do the convert.
This patch will re-define btrfs_raid_types by:
- Move it to volumes.h
btrfs_raid_types are only used internally, no need to be exposed
through UAPI.
- Re-order btrfs_raid_types
To make them match their value order
- Use ilog2() to convert them into index
- Inline btrfs_bg_flags_to_raid_index()
It's just 5 assembly commands now.
Changelog:
v2:
- Fix the start value of BTRFS_RAID_RAID0
Qu Wenruo (2):
btrfs: move definition of btrfs_raid_types to volumes.h
btrfs: use ilog2() to replace if () branches for
btrfs_bg_flags_to_raid_index()
fs/btrfs/space-info.h | 2 ++
fs/btrfs/volumes.c | 26 -----------------------
fs/btrfs/volumes.h | 37 ++++++++++++++++++++++++++++++++-
include/uapi/linux/btrfs_tree.h | 13 ------------
4 files changed, 38 insertions(+), 40 deletions(-)
--
2.33.1
^ permalink raw reply [flat|nested] 13+ messages in thread* [PATCH v2 1/2] btrfs: move definition of btrfs_raid_types to volumes.h 2021-10-27 5:28 [PATCH v2 0/2] btrfs: re-define btrfs_raid_types Qu Wenruo @ 2021-10-27 5:28 ` Qu Wenruo 2021-10-27 5:28 ` [PATCH v2 2/2] btrfs: use ilog2() to replace if () branches for btrfs_bg_flags_to_raid_index() Qu Wenruo 1 sibling, 0 replies; 13+ messages in thread From: Qu Wenruo @ 2021-10-27 5:28 UTC (permalink / raw) To: linux-btrfs It's only internally used as another way to represent btrfs profiles, it's not exposed through any on-disk format. Furthermore, since it's internal structure, its definition can change in the future. Signed-off-by: Qu Wenruo <wqu@suse.com> --- fs/btrfs/volumes.h | 13 +++++++++++++ include/uapi/linux/btrfs_tree.h | 13 ------------- 2 files changed, 13 insertions(+), 13 deletions(-) diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h index 3b8130680749..e0c374a7c30b 100644 --- a/fs/btrfs/volumes.h +++ b/fs/btrfs/volumes.h @@ -17,6 +17,19 @@ extern struct mutex uuid_mutex; #define BTRFS_STRIPE_LEN SZ_64K +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_NR_RAID_TYPES +}; + struct btrfs_io_geometry { /* remaining bytes before crossing a stripe */ u64 len; diff --git a/include/uapi/linux/btrfs_tree.h b/include/uapi/linux/btrfs_tree.h index e1c4c732aaba..819dec72f232 100644 --- a/include/uapi/linux/btrfs_tree.h +++ b/include/uapi/linux/btrfs_tree.h @@ -875,19 +875,6 @@ struct btrfs_dev_replace_item { #define BTRFS_BLOCK_GROUP_RESERVED (BTRFS_AVAIL_ALLOC_BIT_SINGLE | \ BTRFS_SPACE_INFO_GLOBAL_RSV) -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_NR_RAID_TYPES -}; - #define BTRFS_BLOCK_GROUP_TYPE_MASK (BTRFS_BLOCK_GROUP_DATA | \ BTRFS_BLOCK_GROUP_SYSTEM | \ BTRFS_BLOCK_GROUP_METADATA) -- 2.33.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v2 2/2] btrfs: use ilog2() to replace if () branches for btrfs_bg_flags_to_raid_index() 2021-10-27 5:28 [PATCH v2 0/2] btrfs: re-define btrfs_raid_types Qu Wenruo 2021-10-27 5:28 ` [PATCH v2 1/2] btrfs: move definition of btrfs_raid_types to volumes.h Qu Wenruo @ 2021-10-27 5:28 ` Qu Wenruo 2021-10-27 6:37 ` Nikolay Borisov ` (2 more replies) 1 sibling, 3 replies; 13+ messages in thread From: Qu Wenruo @ 2021-10-27 5:28 UTC (permalink / raw) To: linux-btrfs 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. Only one fixed offset is needed to make the index sequential (the lowest profile bit starts at ilog2(1 << 3) while we have 0 reserved for SINGLE). Even with that calculation involved (one if(), one ilog2(), one minus), it should still be way faster than the if () branches, and now it is definitely small enough to be inlined. Signed-off-by: Qu Wenruo <wqu@suse.com> --- fs/btrfs/space-info.h | 2 ++ fs/btrfs/volumes.c | 26 -------------------------- fs/btrfs/volumes.h | 42 ++++++++++++++++++++++++++++++++---------- 3 files changed, 34 insertions(+), 36 deletions(-) diff --git a/fs/btrfs/space-info.h b/fs/btrfs/space-info.h index cb5056472e79..5a0686ab9679 100644 --- a/fs/btrfs/space-info.h +++ b/fs/btrfs/space-info.h @@ -3,6 +3,8 @@ #ifndef BTRFS_SPACE_INFO_H #define BTRFS_SPACE_INFO_H +#include "volumes.h" + struct btrfs_space_info { spinlock_t lock; diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c index a8ea3f88c4db..94a3dfe709e8 100644 --- a/fs/btrfs/volumes.c +++ b/fs/btrfs/volumes.c @@ -154,32 +154,6 @@ const struct btrfs_raid_attr btrfs_raid_array[BTRFS_NR_RAID_TYPES] = { }, }; -/* - * Convert block group flags (BTRFS_BLOCK_GROUP_*) to btrfs_raid_types, which - * can be used as index to access btrfs_raid_array[]. - */ -enum btrfs_raid_types __attribute_const__ btrfs_bg_flags_to_raid_index(u64 flags) -{ - if (flags & BTRFS_BLOCK_GROUP_RAID10) - return BTRFS_RAID_RAID10; - else if (flags & BTRFS_BLOCK_GROUP_RAID1) - return BTRFS_RAID_RAID1; - else if (flags & BTRFS_BLOCK_GROUP_RAID1C3) - return BTRFS_RAID_RAID1C3; - else if (flags & BTRFS_BLOCK_GROUP_RAID1C4) - return BTRFS_RAID_RAID1C4; - else if (flags & BTRFS_BLOCK_GROUP_DUP) - return BTRFS_RAID_DUP; - else if (flags & BTRFS_BLOCK_GROUP_RAID0) - return BTRFS_RAID_RAID0; - else if (flags & BTRFS_BLOCK_GROUP_RAID5) - return BTRFS_RAID_RAID5; - else if (flags & BTRFS_BLOCK_GROUP_RAID6) - return BTRFS_RAID_RAID6; - - return BTRFS_RAID_SINGLE; /* BTRFS_BLOCK_GROUP_SINGLE */ -} - const char *btrfs_bg_type_to_raid_name(u64 flags) { const int index = btrfs_bg_flags_to_raid_index(flags); diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h index e0c374a7c30b..7038c6cee39a 100644 --- a/fs/btrfs/volumes.h +++ b/fs/btrfs/volumes.h @@ -17,19 +17,42 @@ extern struct mutex uuid_mutex; #define BTRFS_STRIPE_LEN SZ_64K +/* + * Here we use ilog2(BTRFS_BLOCK_GROUP_*) to convert the profile bits to + * an index. + * We reserve 0 for BTRFS_RAID_SINGLE, while the lowest profile, ilog2(RAID0), + * is 3, thus we need this shift to make all index numbers sequential. + */ +#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), BTRFS_NR_RAID_TYPES }; +/* + * Convert block group flags (BTRFS_BLOCK_GROUP_*) to btrfs_raid_types, which + * can be used as index to access btrfs_raid_array[]. + */ +static inline enum btrfs_raid_types __attribute_const__ +btrfs_bg_flags_to_raid_index(u64 flags) +{ + u64 profile = flags & BTRFS_BLOCK_GROUP_PROFILE_MASK; + + if (!profile) + return BTRFS_RAID_SINGLE; + + return ilog2(profile >> BTRFS_RAID_SHIFT); +} + struct btrfs_io_geometry { /* remaining bytes before crossing a stripe */ u64 len; @@ -646,7 +669,6 @@ void btrfs_scratch_superblocks(struct btrfs_fs_info *fs_info, struct block_device *bdev, const char *device_path); -enum btrfs_raid_types __attribute_const__ btrfs_bg_flags_to_raid_index(u64 flags); int btrfs_bg_type_to_factor(u64 flags); const char *btrfs_bg_type_to_raid_name(u64 flags); int btrfs_verify_dev_extents(struct btrfs_fs_info *fs_info); -- 2.33.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v2 2/2] btrfs: use ilog2() to replace if () branches for btrfs_bg_flags_to_raid_index() 2021-10-27 5:28 ` [PATCH v2 2/2] btrfs: use ilog2() to replace if () branches for btrfs_bg_flags_to_raid_index() Qu Wenruo @ 2021-10-27 6:37 ` Nikolay Borisov 2021-10-27 7:41 ` Qu Wenruo 2021-10-27 9:23 ` Anand Jain 2021-10-29 14:11 ` David Sterba 2 siblings, 1 reply; 13+ messages in thread From: Nikolay Borisov @ 2021-10-27 6:37 UTC (permalink / raw) To: Qu Wenruo, linux-btrfs On 27.10.21 г. 8:28, 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. > > Only one fixed offset is needed to make the index sequential (the > lowest profile bit starts at ilog2(1 << 3) while we have 0 reserved for > SINGLE). > > Even with that calculation involved (one if(), one ilog2(), one minus), > it should still be way faster than the if () branches, and now it is > definitely small enough to be inlined. > Is this used in a performance critical path, are there any numbers which prove that it's indeed faster? ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 2/2] btrfs: use ilog2() to replace if () branches for btrfs_bg_flags_to_raid_index() 2021-10-27 6:37 ` Nikolay Borisov @ 2021-10-27 7:41 ` Qu Wenruo 0 siblings, 0 replies; 13+ messages in thread From: Qu Wenruo @ 2021-10-27 7:41 UTC (permalink / raw) To: Nikolay Borisov, Qu Wenruo, linux-btrfs On 2021/10/27 14:37, Nikolay Borisov wrote: > > > On 27.10.21 г. 8:28, 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. >> >> Only one fixed offset is needed to make the index sequential (the >> lowest profile bit starts at ilog2(1 << 3) while we have 0 reserved for >> SINGLE). >> >> Even with that calculation involved (one if(), one ilog2(), one minus), >> it should still be way faster than the if () branches, and now it is >> definitely small enough to be inlined. >> > > Is this used in a performance critical path, Not really in a hot path. Most of them are called in a per block group/chunk base. The only hotter path is in __btrfs_map_block() where if we need full stripe, we will call btrfs_chunk_max_errors() which in turn call the function. That's the hottest path I can find, and even for that case it's just per-bio base. > are there any numbers which prove that it's indeed faster? No real world bench for it. But from x86_75 asm code, it's definitely smaller, with only one branch. New: btrfs_bg_flags_to_raid_index: xorl %eax, %eax andl $2040, %edi je .L2499 shrq $2, %rdi movl $-1, %eax bsrq %rdi,%rax .L2499: ret Old: btrfs_bg_flags_to_raid_index: xorl %eax, %eax testb $64, %dil jne .L429 movl $1, %eax testb $16, %dil jne .L429 movl $7, %eax testl $512, %edi jne .L429 movl $8, %eax testl $1024, %edi jne .L429 movl $2, %eax testb $32, %dil jne .L429 movl $3, %eax testb $8, %dil jne .L429 movl $5, %eax testb $-128, %dil jne .L429 andl $256, %edi cmpq $1, %rdi sbbl %eax, %eax andl $-2, %eax addl $6, %eax .L429: ret Which I don't really believe the older code can be any faster, considering so many branches, and pure lines of asm. Thanks, Qu ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 2/2] btrfs: use ilog2() to replace if () branches for btrfs_bg_flags_to_raid_index() 2021-10-27 5:28 ` [PATCH v2 2/2] btrfs: use ilog2() to replace if () branches for btrfs_bg_flags_to_raid_index() Qu Wenruo 2021-10-27 6:37 ` Nikolay Borisov @ 2021-10-27 9:23 ` Anand Jain 2021-10-27 10:41 ` Qu Wenruo 2021-10-29 14:11 ` David Sterba 2 siblings, 1 reply; 13+ messages in thread From: Anand Jain @ 2021-10-27 9:23 UTC (permalink / raw) To: Qu Wenruo, linux-btrfs On 27/10/2021 13:28, 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. > > Only one fixed offset is needed to make the index sequential (the > lowest profile bit starts at ilog2(1 << 3) while we have 0 reserved for > SINGLE). > > Even with that calculation involved (one if(), one ilog2(), one minus), > it should still be way faster than the if () branches, and now it is > definitely small enough to be inlined. > Why not just use reverse static index similar to const struct btrfs_raid_attr btrfs_raid_array[BTRFS_NR_RAID_TYPES] = { <snip> } Thanks, Anand > Signed-off-by: Qu Wenruo <wqu@suse.com> > --- > fs/btrfs/space-info.h | 2 ++ > fs/btrfs/volumes.c | 26 -------------------------- > fs/btrfs/volumes.h | 42 ++++++++++++++++++++++++++++++++---------- > 3 files changed, 34 insertions(+), 36 deletions(-) > > diff --git a/fs/btrfs/space-info.h b/fs/btrfs/space-info.h > index cb5056472e79..5a0686ab9679 100644 > --- a/fs/btrfs/space-info.h > +++ b/fs/btrfs/space-info.h > @@ -3,6 +3,8 @@ > #ifndef BTRFS_SPACE_INFO_H > #define BTRFS_SPACE_INFO_H > > +#include "volumes.h" > + > struct btrfs_space_info { > spinlock_t lock; > > diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c > index a8ea3f88c4db..94a3dfe709e8 100644 > --- a/fs/btrfs/volumes.c > +++ b/fs/btrfs/volumes.c > @@ -154,32 +154,6 @@ const struct btrfs_raid_attr btrfs_raid_array[BTRFS_NR_RAID_TYPES] = { > }, > }; > > -/* > - * Convert block group flags (BTRFS_BLOCK_GROUP_*) to btrfs_raid_types, which > - * can be used as index to access btrfs_raid_array[]. > - */ > -enum btrfs_raid_types __attribute_const__ btrfs_bg_flags_to_raid_index(u64 flags) > -{ > - if (flags & BTRFS_BLOCK_GROUP_RAID10) > - return BTRFS_RAID_RAID10; > - else if (flags & BTRFS_BLOCK_GROUP_RAID1) > - return BTRFS_RAID_RAID1; > - else if (flags & BTRFS_BLOCK_GROUP_RAID1C3) > - return BTRFS_RAID_RAID1C3; > - else if (flags & BTRFS_BLOCK_GROUP_RAID1C4) > - return BTRFS_RAID_RAID1C4; > - else if (flags & BTRFS_BLOCK_GROUP_DUP) > - return BTRFS_RAID_DUP; > - else if (flags & BTRFS_BLOCK_GROUP_RAID0) > - return BTRFS_RAID_RAID0; > - else if (flags & BTRFS_BLOCK_GROUP_RAID5) > - return BTRFS_RAID_RAID5; > - else if (flags & BTRFS_BLOCK_GROUP_RAID6) > - return BTRFS_RAID_RAID6; > - > - return BTRFS_RAID_SINGLE; /* BTRFS_BLOCK_GROUP_SINGLE */ > -} > - > const char *btrfs_bg_type_to_raid_name(u64 flags) > { > const int index = btrfs_bg_flags_to_raid_index(flags); > diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h > index e0c374a7c30b..7038c6cee39a 100644 > --- a/fs/btrfs/volumes.h > +++ b/fs/btrfs/volumes.h > @@ -17,19 +17,42 @@ extern struct mutex uuid_mutex; > > #define BTRFS_STRIPE_LEN SZ_64K > > +/* > + * Here we use ilog2(BTRFS_BLOCK_GROUP_*) to convert the profile bits to > + * an index. > + * We reserve 0 for BTRFS_RAID_SINGLE, while the lowest profile, ilog2(RAID0), > + * is 3, thus we need this shift to make all index numbers sequential. > + */ > +#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), > BTRFS_NR_RAID_TYPES > }; > > +/* > + * Convert block group flags (BTRFS_BLOCK_GROUP_*) to btrfs_raid_types, which > + * can be used as index to access btrfs_raid_array[]. > + */ > +static inline enum btrfs_raid_types __attribute_const__ > +btrfs_bg_flags_to_raid_index(u64 flags) > +{ > + u64 profile = flags & BTRFS_BLOCK_GROUP_PROFILE_MASK; > + > + if (!profile) > + return BTRFS_RAID_SINGLE; > + > + return ilog2(profile >> BTRFS_RAID_SHIFT); > +} > + > struct btrfs_io_geometry { > /* remaining bytes before crossing a stripe */ > u64 len; > @@ -646,7 +669,6 @@ void btrfs_scratch_superblocks(struct btrfs_fs_info *fs_info, > struct block_device *bdev, > const char *device_path); > > -enum btrfs_raid_types __attribute_const__ btrfs_bg_flags_to_raid_index(u64 flags); > int btrfs_bg_type_to_factor(u64 flags); > const char *btrfs_bg_type_to_raid_name(u64 flags); > int btrfs_verify_dev_extents(struct btrfs_fs_info *fs_info); > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 2/2] btrfs: use ilog2() to replace if () branches for btrfs_bg_flags_to_raid_index() 2021-10-27 9:23 ` Anand Jain @ 2021-10-27 10:41 ` Qu Wenruo 2021-10-28 1:04 ` Anand Jain 0 siblings, 1 reply; 13+ messages in thread From: Qu Wenruo @ 2021-10-27 10:41 UTC (permalink / raw) To: Anand Jain, Qu Wenruo, linux-btrfs On 2021/10/27 17:23, Anand Jain wrote: > On 27/10/2021 13:28, 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. >> >> Only one fixed offset is needed to make the index sequential (the >> lowest profile bit starts at ilog2(1 << 3) while we have 0 reserved for >> SINGLE). >> >> Even with that calculation involved (one if(), one ilog2(), one minus), >> it should still be way faster than the if () branches, and now it is >> definitely small enough to be inlined. >> > > Why not just use reverse static index similar to > > const struct btrfs_raid_attr btrfs_raid_array[BTRFS_NR_RAID_TYPES] = { > <snip> > } Sorry, I didn't get the point. Mind to share more details? Thanks, Qu > > Thanks, Anand > >> Signed-off-by: Qu Wenruo <wqu@suse.com> >> --- >> fs/btrfs/space-info.h | 2 ++ >> fs/btrfs/volumes.c | 26 -------------------------- >> fs/btrfs/volumes.h | 42 ++++++++++++++++++++++++++++++++---------- >> 3 files changed, 34 insertions(+), 36 deletions(-) >> >> diff --git a/fs/btrfs/space-info.h b/fs/btrfs/space-info.h >> index cb5056472e79..5a0686ab9679 100644 >> --- a/fs/btrfs/space-info.h >> +++ b/fs/btrfs/space-info.h >> @@ -3,6 +3,8 @@ >> #ifndef BTRFS_SPACE_INFO_H >> #define BTRFS_SPACE_INFO_H >> +#include "volumes.h" >> + >> struct btrfs_space_info { >> spinlock_t lock; >> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c >> index a8ea3f88c4db..94a3dfe709e8 100644 >> --- a/fs/btrfs/volumes.c >> +++ b/fs/btrfs/volumes.c >> @@ -154,32 +154,6 @@ const struct btrfs_raid_attr >> btrfs_raid_array[BTRFS_NR_RAID_TYPES] = { >> }, >> }; >> -/* >> - * Convert block group flags (BTRFS_BLOCK_GROUP_*) to >> btrfs_raid_types, which >> - * can be used as index to access btrfs_raid_array[]. >> - */ >> -enum btrfs_raid_types __attribute_const__ >> btrfs_bg_flags_to_raid_index(u64 flags) >> -{ >> - if (flags & BTRFS_BLOCK_GROUP_RAID10) >> - return BTRFS_RAID_RAID10; >> - else if (flags & BTRFS_BLOCK_GROUP_RAID1) >> - return BTRFS_RAID_RAID1; >> - else if (flags & BTRFS_BLOCK_GROUP_RAID1C3) >> - return BTRFS_RAID_RAID1C3; >> - else if (flags & BTRFS_BLOCK_GROUP_RAID1C4) >> - return BTRFS_RAID_RAID1C4; >> - else if (flags & BTRFS_BLOCK_GROUP_DUP) >> - return BTRFS_RAID_DUP; >> - else if (flags & BTRFS_BLOCK_GROUP_RAID0) >> - return BTRFS_RAID_RAID0; >> - else if (flags & BTRFS_BLOCK_GROUP_RAID5) >> - return BTRFS_RAID_RAID5; >> - else if (flags & BTRFS_BLOCK_GROUP_RAID6) >> - return BTRFS_RAID_RAID6; >> - >> - return BTRFS_RAID_SINGLE; /* BTRFS_BLOCK_GROUP_SINGLE */ >> -} >> - >> const char *btrfs_bg_type_to_raid_name(u64 flags) >> { >> const int index = btrfs_bg_flags_to_raid_index(flags); >> diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h >> index e0c374a7c30b..7038c6cee39a 100644 >> --- a/fs/btrfs/volumes.h >> +++ b/fs/btrfs/volumes.h >> @@ -17,19 +17,42 @@ extern struct mutex uuid_mutex; >> #define BTRFS_STRIPE_LEN SZ_64K >> +/* >> + * Here we use ilog2(BTRFS_BLOCK_GROUP_*) to convert the profile bits to >> + * an index. >> + * We reserve 0 for BTRFS_RAID_SINGLE, while the lowest profile, >> ilog2(RAID0), >> + * is 3, thus we need this shift to make all index numbers sequential. >> + */ >> +#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), >> BTRFS_NR_RAID_TYPES >> }; > > >> +/* >> + * Convert block group flags (BTRFS_BLOCK_GROUP_*) to >> btrfs_raid_types, which >> + * can be used as index to access btrfs_raid_array[]. >> + */ >> +static inline enum btrfs_raid_types __attribute_const__ >> +btrfs_bg_flags_to_raid_index(u64 flags) >> +{ >> + u64 profile = flags & BTRFS_BLOCK_GROUP_PROFILE_MASK; >> + >> + if (!profile) >> + return BTRFS_RAID_SINGLE; >> + >> + return ilog2(profile >> BTRFS_RAID_SHIFT); >> +} >> + >> struct btrfs_io_geometry { >> /* remaining bytes before crossing a stripe */ >> u64 len; >> @@ -646,7 +669,6 @@ void btrfs_scratch_superblocks(struct >> btrfs_fs_info *fs_info, >> struct block_device *bdev, >> const char *device_path); >> -enum btrfs_raid_types __attribute_const__ >> btrfs_bg_flags_to_raid_index(u64 flags); >> int btrfs_bg_type_to_factor(u64 flags); >> const char *btrfs_bg_type_to_raid_name(u64 flags); >> int btrfs_verify_dev_extents(struct btrfs_fs_info *fs_info); >> > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 2/2] btrfs: use ilog2() to replace if () branches for btrfs_bg_flags_to_raid_index() 2021-10-27 10:41 ` Qu Wenruo @ 2021-10-28 1:04 ` Anand Jain 2021-10-28 7:10 ` Qu Wenruo 0 siblings, 1 reply; 13+ messages in thread From: Anand Jain @ 2021-10-28 1:04 UTC (permalink / raw) To: Qu Wenruo, Qu Wenruo, linux-btrfs On 27/10/2021 18:41, Qu Wenruo wrote: > > > On 2021/10/27 17:23, Anand Jain wrote: >> On 27/10/2021 13:28, 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. >>> >>> Only one fixed offset is needed to make the index sequential (the >>> lowest profile bit starts at ilog2(1 << 3) while we have 0 reserved for >>> SINGLE). >>> >>> Even with that calculation involved (one if(), one ilog2(), one minus), >>> it should still be way faster than the if () branches, and now it is >>> definitely small enough to be inlined. >>> >> >> Why not just use reverse static index similar to >> >> const struct btrfs_raid_attr btrfs_raid_array[BTRFS_NR_RAID_TYPES] = { >> <snip> >> } > > Sorry, I didn't get the point. > > Mind to share more details? > Something like /* Must match with the order of BTRFS_BLOCK_GROUP_XYZ */ +enum btrfs_raid_types { + BTRFS_RAID_RAID0, + BTRFS_RAID_RAID1, + BTRFS_RAID_DUP, + BTRFS_RAID_RAID10, + BTRFS_RAID_RAID5, + BTRFS_RAID_RAID6, + BTRFS_RAID_RAID1C3, + BTRFS_RAID_RAID1C4, + BTRFS_RAID_SINGLE, + BTRFS_NR_RAID_TYPES +}; btrfs_bg_flags_to_raid_index(u64 flags) { int ret; flags = flags & BTRFS_BLOCK_GROUP_PROFILE_MASK; ret = ilog2(flags); if (ret == 0) return BTRFS_RAID_SINGLE; return ret - 3; } Should work. No? Thanks, Anand > Thanks, > Qu > >> >> Thanks, Anand >> >>> Signed-off-by: Qu Wenruo <wqu@suse.com> >>> --- >>> fs/btrfs/space-info.h | 2 ++ >>> fs/btrfs/volumes.c | 26 -------------------------- >>> fs/btrfs/volumes.h | 42 ++++++++++++++++++++++++++++++++---------- >>> 3 files changed, 34 insertions(+), 36 deletions(-) >>> >>> diff --git a/fs/btrfs/space-info.h b/fs/btrfs/space-info.h >>> index cb5056472e79..5a0686ab9679 100644 >>> --- a/fs/btrfs/space-info.h >>> +++ b/fs/btrfs/space-info.h >>> @@ -3,6 +3,8 @@ >>> #ifndef BTRFS_SPACE_INFO_H >>> #define BTRFS_SPACE_INFO_H >>> +#include "volumes.h" >>> + >>> struct btrfs_space_info { >>> spinlock_t lock; >>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c >>> index a8ea3f88c4db..94a3dfe709e8 100644 >>> --- a/fs/btrfs/volumes.c >>> +++ b/fs/btrfs/volumes.c >>> @@ -154,32 +154,6 @@ const struct btrfs_raid_attr >>> btrfs_raid_array[BTRFS_NR_RAID_TYPES] = { >>> }, >>> }; >>> -/* >>> - * Convert block group flags (BTRFS_BLOCK_GROUP_*) to >>> btrfs_raid_types, which >>> - * can be used as index to access btrfs_raid_array[]. >>> - */ >>> -enum btrfs_raid_types __attribute_const__ >>> btrfs_bg_flags_to_raid_index(u64 flags) >>> -{ >>> - if (flags & BTRFS_BLOCK_GROUP_RAID10) >>> - return BTRFS_RAID_RAID10; >>> - else if (flags & BTRFS_BLOCK_GROUP_RAID1) >>> - return BTRFS_RAID_RAID1; >>> - else if (flags & BTRFS_BLOCK_GROUP_RAID1C3) >>> - return BTRFS_RAID_RAID1C3; >>> - else if (flags & BTRFS_BLOCK_GROUP_RAID1C4) >>> - return BTRFS_RAID_RAID1C4; >>> - else if (flags & BTRFS_BLOCK_GROUP_DUP) >>> - return BTRFS_RAID_DUP; >>> - else if (flags & BTRFS_BLOCK_GROUP_RAID0) >>> - return BTRFS_RAID_RAID0; >>> - else if (flags & BTRFS_BLOCK_GROUP_RAID5) >>> - return BTRFS_RAID_RAID5; >>> - else if (flags & BTRFS_BLOCK_GROUP_RAID6) >>> - return BTRFS_RAID_RAID6; >>> - >>> - return BTRFS_RAID_SINGLE; /* BTRFS_BLOCK_GROUP_SINGLE */ >>> -} >>> - >>> const char *btrfs_bg_type_to_raid_name(u64 flags) >>> { >>> const int index = btrfs_bg_flags_to_raid_index(flags); >>> diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h >>> index e0c374a7c30b..7038c6cee39a 100644 >>> --- a/fs/btrfs/volumes.h >>> +++ b/fs/btrfs/volumes.h >>> @@ -17,19 +17,42 @@ extern struct mutex uuid_mutex; >>> #define BTRFS_STRIPE_LEN SZ_64K >>> +/* >>> + * Here we use ilog2(BTRFS_BLOCK_GROUP_*) to convert the profile >>> bits to >>> + * an index. >>> + * We reserve 0 for BTRFS_RAID_SINGLE, while the lowest profile, >>> ilog2(RAID0), >>> + * is 3, thus we need this shift to make all index numbers sequential. >>> + */ >>> +#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), >>> BTRFS_NR_RAID_TYPES >>> }; >> >> >>> +/* >>> + * Convert block group flags (BTRFS_BLOCK_GROUP_*) to >>> btrfs_raid_types, which >>> + * can be used as index to access btrfs_raid_array[]. >>> + */ >>> +static inline enum btrfs_raid_types __attribute_const__ >>> +btrfs_bg_flags_to_raid_index(u64 flags) >>> +{ >>> + u64 profile = flags & BTRFS_BLOCK_GROUP_PROFILE_MASK; >>> + >>> + if (!profile) >>> + return BTRFS_RAID_SINGLE; >>> + >>> + return ilog2(profile >> BTRFS_RAID_SHIFT); >>> +} >>> + >>> struct btrfs_io_geometry { >>> /* remaining bytes before crossing a stripe */ >>> u64 len; >>> @@ -646,7 +669,6 @@ void btrfs_scratch_superblocks(struct >>> btrfs_fs_info *fs_info, >>> struct block_device *bdev, >>> const char *device_path); >>> -enum btrfs_raid_types __attribute_const__ >>> btrfs_bg_flags_to_raid_index(u64 flags); >>> int btrfs_bg_type_to_factor(u64 flags); >>> const char *btrfs_bg_type_to_raid_name(u64 flags); >>> int btrfs_verify_dev_extents(struct btrfs_fs_info *fs_info); >>> >> ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 2/2] btrfs: use ilog2() to replace if () branches for btrfs_bg_flags_to_raid_index() 2021-10-28 1:04 ` Anand Jain @ 2021-10-28 7:10 ` Qu Wenruo 2021-10-28 21:53 ` Anand Jain 0 siblings, 1 reply; 13+ messages in thread From: Qu Wenruo @ 2021-10-28 7:10 UTC (permalink / raw) To: Anand Jain, Qu Wenruo, linux-btrfs On 2021/10/28 09:04, Anand Jain wrote: > > > On 27/10/2021 18:41, Qu Wenruo wrote: >> >> >> On 2021/10/27 17:23, Anand Jain wrote: >>> On 27/10/2021 13:28, 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. >>>> >>>> Only one fixed offset is needed to make the index sequential (the >>>> lowest profile bit starts at ilog2(1 << 3) while we have 0 reserved for >>>> SINGLE). >>>> >>>> Even with that calculation involved (one if(), one ilog2(), one minus), >>>> it should still be way faster than the if () branches, and now it is >>>> definitely small enough to be inlined. >>>> >>> >>> Why not just use reverse static index similar to >>> >>> const struct btrfs_raid_attr btrfs_raid_array[BTRFS_NR_RAID_TYPES] = { >>> <snip> >>> } >> >> Sorry, I didn't get the point. >> >> Mind to share more details? >> > > Something like > > /* Must match with the order of BTRFS_BLOCK_GROUP_XYZ */ > +enum btrfs_raid_types { > + BTRFS_RAID_RAID0, > + BTRFS_RAID_RAID1, > + BTRFS_RAID_DUP, > + BTRFS_RAID_RAID10, > + BTRFS_RAID_RAID5, > + BTRFS_RAID_RAID6, > + BTRFS_RAID_RAID1C3, > + BTRFS_RAID_RAID1C4, > + BTRFS_RAID_SINGLE, > + BTRFS_NR_RAID_TYPES OK I got your point. But the point here is, I don't want to enum btrfs_raid_types to hide the value and rely on us to make it in proper order. Thus that's why my version have all the ilog2() values. And in that case, it's just a difference between the shift value and where we put RAID_SINGLE. > +}; > > btrfs_bg_flags_to_raid_index(u64 flags) > { > int ret; > flags = flags & BTRFS_BLOCK_GROUP_PROFILE_MASK; > > ret = ilog2(flags); > if (ret == 0) > return BTRFS_RAID_SINGLE; In fact, ilog2(0) behavior is undefined, and should be avoided. In this case, we still need to check if (flags == 0), and just change the shift value. Thanks, Qu > > return ret - 3; > } > > Should work. No? > > Thanks, Anand > >> Thanks, >> Qu >> >>> >>> Thanks, Anand >>> >>>> Signed-off-by: Qu Wenruo <wqu@suse.com> >>>> --- >>>> fs/btrfs/space-info.h | 2 ++ >>>> fs/btrfs/volumes.c | 26 -------------------------- >>>> fs/btrfs/volumes.h | 42 ++++++++++++++++++++++++++++++++---------- >>>> 3 files changed, 34 insertions(+), 36 deletions(-) >>>> >>>> diff --git a/fs/btrfs/space-info.h b/fs/btrfs/space-info.h >>>> index cb5056472e79..5a0686ab9679 100644 >>>> --- a/fs/btrfs/space-info.h >>>> +++ b/fs/btrfs/space-info.h >>>> @@ -3,6 +3,8 @@ >>>> #ifndef BTRFS_SPACE_INFO_H >>>> #define BTRFS_SPACE_INFO_H >>>> +#include "volumes.h" >>>> + >>>> struct btrfs_space_info { >>>> spinlock_t lock; >>>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c >>>> index a8ea3f88c4db..94a3dfe709e8 100644 >>>> --- a/fs/btrfs/volumes.c >>>> +++ b/fs/btrfs/volumes.c >>>> @@ -154,32 +154,6 @@ const struct btrfs_raid_attr >>>> btrfs_raid_array[BTRFS_NR_RAID_TYPES] = { >>>> }, >>>> }; >>>> -/* >>>> - * Convert block group flags (BTRFS_BLOCK_GROUP_*) to >>>> btrfs_raid_types, which >>>> - * can be used as index to access btrfs_raid_array[]. >>>> - */ >>>> -enum btrfs_raid_types __attribute_const__ >>>> btrfs_bg_flags_to_raid_index(u64 flags) >>>> -{ >>>> - if (flags & BTRFS_BLOCK_GROUP_RAID10) >>>> - return BTRFS_RAID_RAID10; >>>> - else if (flags & BTRFS_BLOCK_GROUP_RAID1) >>>> - return BTRFS_RAID_RAID1; >>>> - else if (flags & BTRFS_BLOCK_GROUP_RAID1C3) >>>> - return BTRFS_RAID_RAID1C3; >>>> - else if (flags & BTRFS_BLOCK_GROUP_RAID1C4) >>>> - return BTRFS_RAID_RAID1C4; >>>> - else if (flags & BTRFS_BLOCK_GROUP_DUP) >>>> - return BTRFS_RAID_DUP; >>>> - else if (flags & BTRFS_BLOCK_GROUP_RAID0) >>>> - return BTRFS_RAID_RAID0; >>>> - else if (flags & BTRFS_BLOCK_GROUP_RAID5) >>>> - return BTRFS_RAID_RAID5; >>>> - else if (flags & BTRFS_BLOCK_GROUP_RAID6) >>>> - return BTRFS_RAID_RAID6; >>>> - >>>> - return BTRFS_RAID_SINGLE; /* BTRFS_BLOCK_GROUP_SINGLE */ >>>> -} >>>> - >>>> const char *btrfs_bg_type_to_raid_name(u64 flags) >>>> { >>>> const int index = btrfs_bg_flags_to_raid_index(flags); >>>> diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h >>>> index e0c374a7c30b..7038c6cee39a 100644 >>>> --- a/fs/btrfs/volumes.h >>>> +++ b/fs/btrfs/volumes.h >>>> @@ -17,19 +17,42 @@ extern struct mutex uuid_mutex; >>>> #define BTRFS_STRIPE_LEN SZ_64K >>>> +/* >>>> + * Here we use ilog2(BTRFS_BLOCK_GROUP_*) to convert the profile >>>> bits to >>>> + * an index. >>>> + * We reserve 0 for BTRFS_RAID_SINGLE, while the lowest profile, >>>> ilog2(RAID0), >>>> + * is 3, thus we need this shift to make all index numbers sequential. >>>> + */ >>>> +#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), >>>> BTRFS_NR_RAID_TYPES >>>> }; >>> >>> >>>> +/* >>>> + * Convert block group flags (BTRFS_BLOCK_GROUP_*) to >>>> btrfs_raid_types, which >>>> + * can be used as index to access btrfs_raid_array[]. >>>> + */ >>>> +static inline enum btrfs_raid_types __attribute_const__ >>>> +btrfs_bg_flags_to_raid_index(u64 flags) >>>> +{ >>>> + u64 profile = flags & BTRFS_BLOCK_GROUP_PROFILE_MASK; >>>> + >>>> + if (!profile) >>>> + return BTRFS_RAID_SINGLE; >>>> + >>>> + return ilog2(profile >> BTRFS_RAID_SHIFT); >>>> +} >>>> + >>>> struct btrfs_io_geometry { >>>> /* remaining bytes before crossing a stripe */ >>>> u64 len; >>>> @@ -646,7 +669,6 @@ void btrfs_scratch_superblocks(struct >>>> btrfs_fs_info *fs_info, >>>> struct block_device *bdev, >>>> const char *device_path); >>>> -enum btrfs_raid_types __attribute_const__ >>>> btrfs_bg_flags_to_raid_index(u64 flags); >>>> int btrfs_bg_type_to_factor(u64 flags); >>>> const char *btrfs_bg_type_to_raid_name(u64 flags); >>>> int btrfs_verify_dev_extents(struct btrfs_fs_info *fs_info); >>>> >>> ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 2/2] btrfs: use ilog2() to replace if () branches for btrfs_bg_flags_to_raid_index() 2021-10-28 7:10 ` Qu Wenruo @ 2021-10-28 21:53 ` Anand Jain 0 siblings, 0 replies; 13+ messages in thread From: Anand Jain @ 2021-10-28 21:53 UTC (permalink / raw) To: Qu Wenruo, Qu Wenruo, linux-btrfs On 28/10/2021 15:10, Qu Wenruo wrote: > > > On 2021/10/28 09:04, Anand Jain wrote: >> >> >> On 27/10/2021 18:41, Qu Wenruo wrote: >>> >>> >>> On 2021/10/27 17:23, Anand Jain wrote: >>>> On 27/10/2021 13:28, 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. >>>>> >>>>> Only one fixed offset is needed to make the index sequential (the >>>>> lowest profile bit starts at ilog2(1 << 3) while we have 0 reserved >>>>> for >>>>> SINGLE). >>>>> >>>>> Even with that calculation involved (one if(), one ilog2(), one >>>>> minus), >>>>> it should still be way faster than the if () branches, and now it is >>>>> definitely small enough to be inlined. >>>>> >>>> >>>> Why not just use reverse static index similar to >>>> >>>> const struct btrfs_raid_attr btrfs_raid_array[BTRFS_NR_RAID_TYPES] = { >>>> <snip> >>>> } >>> >>> Sorry, I didn't get the point. >>> >>> Mind to share more details? >>> >> >> Something like >> >> /* Must match with the order of BTRFS_BLOCK_GROUP_XYZ */ >> +enum btrfs_raid_types { >> + BTRFS_RAID_RAID0, >> + BTRFS_RAID_RAID1, >> + BTRFS_RAID_DUP, >> + BTRFS_RAID_RAID10, >> + BTRFS_RAID_RAID5, >> + BTRFS_RAID_RAID6, >> + BTRFS_RAID_RAID1C3, >> + BTRFS_RAID_RAID1C4, >> + BTRFS_RAID_SINGLE, >> + BTRFS_NR_RAID_TYPES > > OK I got your point. > > But the point here is, I don't want to enum btrfs_raid_types to hide the > value and rely on us to make it in proper order. > > Thus that's why my version have all the ilog2() values. > > And in that case, it's just a difference between the shift value and > where we put RAID_SINGLE. Agree. If some commit misses the order, we are in the soup. More comments below. > >> +}; >> >> btrfs_bg_flags_to_raid_index(u64 flags) >> { >> int ret; >> flags = flags & BTRFS_BLOCK_GROUP_PROFILE_MASK; >> >> ret = ilog2(flags); >> if (ret == 0) >> return BTRFS_RAID_SINGLE; > > In fact, ilog2(0) behavior is undefined, and should be avoided. > > In this case, we still need to check if (flags == 0), and just change > the shift value. > > Thanks, > Qu > >> >> return ret - 3; >> } >> >> Should work. No? >> >> Thanks, Anand >> >>> Thanks, >>> Qu >>> >>>> >>>> Thanks, Anand >>>> >>>>> Signed-off-by: Qu Wenruo <wqu@suse.com> >>>>> --- >>>>> fs/btrfs/space-info.h | 2 ++ >>>>> fs/btrfs/volumes.c | 26 -------------------------- >>>>> fs/btrfs/volumes.h | 42 >>>>> ++++++++++++++++++++++++++++++++---------- >>>>> 3 files changed, 34 insertions(+), 36 deletions(-) >>>>> >>>>> diff --git a/fs/btrfs/space-info.h b/fs/btrfs/space-info.h >>>>> index cb5056472e79..5a0686ab9679 100644 >>>>> --- a/fs/btrfs/space-info.h >>>>> +++ b/fs/btrfs/space-info.h >>>>> @@ -3,6 +3,8 @@ >>>>> #ifndef BTRFS_SPACE_INFO_H >>>>> #define BTRFS_SPACE_INFO_H >>>>> +#include "volumes.h" >>>>> + >>>>> struct btrfs_space_info { >>>>> spinlock_t lock; >>>>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c >>>>> index a8ea3f88c4db..94a3dfe709e8 100644 >>>>> --- a/fs/btrfs/volumes.c >>>>> +++ b/fs/btrfs/volumes.c >>>>> @@ -154,32 +154,6 @@ const struct btrfs_raid_attr >>>>> btrfs_raid_array[BTRFS_NR_RAID_TYPES] = { >>>>> }, >>>>> }; >>>>> -/* >>>>> - * Convert block group flags (BTRFS_BLOCK_GROUP_*) to >>>>> btrfs_raid_types, which >>>>> - * can be used as index to access btrfs_raid_array[]. >>>>> - */ >>>>> -enum btrfs_raid_types __attribute_const__ >>>>> btrfs_bg_flags_to_raid_index(u64 flags) >>>>> -{ >>>>> - if (flags & BTRFS_BLOCK_GROUP_RAID10) >>>>> - return BTRFS_RAID_RAID10; >>>>> - else if (flags & BTRFS_BLOCK_GROUP_RAID1) >>>>> - return BTRFS_RAID_RAID1; >>>>> - else if (flags & BTRFS_BLOCK_GROUP_RAID1C3) >>>>> - return BTRFS_RAID_RAID1C3; >>>>> - else if (flags & BTRFS_BLOCK_GROUP_RAID1C4) >>>>> - return BTRFS_RAID_RAID1C4; >>>>> - else if (flags & BTRFS_BLOCK_GROUP_DUP) >>>>> - return BTRFS_RAID_DUP; >>>>> - else if (flags & BTRFS_BLOCK_GROUP_RAID0) >>>>> - return BTRFS_RAID_RAID0; >>>>> - else if (flags & BTRFS_BLOCK_GROUP_RAID5) >>>>> - return BTRFS_RAID_RAID5; >>>>> - else if (flags & BTRFS_BLOCK_GROUP_RAID6) >>>>> - return BTRFS_RAID_RAID6; >>>>> - >>>>> - return BTRFS_RAID_SINGLE; /* BTRFS_BLOCK_GROUP_SINGLE */ >>>>> -} >>>>> - >>>>> const char *btrfs_bg_type_to_raid_name(u64 flags) >>>>> { >>>>> const int index = btrfs_bg_flags_to_raid_index(flags); >>>>> diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h >>>>> index e0c374a7c30b..7038c6cee39a 100644 >>>>> --- a/fs/btrfs/volumes.h >>>>> +++ b/fs/btrfs/volumes.h >>>>> @@ -17,19 +17,42 @@ extern struct mutex uuid_mutex; >>>>> #define BTRFS_STRIPE_LEN SZ_64K >>>>> +/* >>>>> + * Here we use ilog2(BTRFS_BLOCK_GROUP_*) to convert the profile >>>>> bits to >>>>> + * an index. >>>>> + * We reserve 0 for BTRFS_RAID_SINGLE, while the lowest profile, >>>>> ilog2(RAID0), >>>>> + * is 3, thus we need this shift to make all index numbers >>>>> sequential. >>>>> + */ >>>>> +#define BTRFS_RAID_SHIFT (ilog2(BTRFS_BLOCK_GROUP_RAID0) - 1) >>>>> + David may need this to be. #define BTRFS_BG_FLAG_TO_INDEX(f) \ ilog2(f >> (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_RAID0 = BTRFS_BG_FLAG_TO_INDEX(BTRFS_BLOCK_GROUP_RAID0), :: etc. >>>>> + 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), >>>>> BTRFS_NR_RAID_TYPES >>>>> }; >>>> >>>> >>>>> +/* >>>>> + * Convert block group flags (BTRFS_BLOCK_GROUP_*) to >>>>> btrfs_raid_types, which >>>>> + * can be used as index to access btrfs_raid_array[]. >>>>> + */ >>>>> +static inline enum btrfs_raid_types __attribute_const__ >>>>> +btrfs_bg_flags_to_raid_index(u64 flags) >>>>> +{ >>>>> + u64 profile = flags & BTRFS_BLOCK_GROUP_PROFILE_MASK; >>>>> + >>>>> + if (!profile) >>>>> + return BTRFS_RAID_SINGLE; >>>>> + >>>>> + return ilog2(profile >> BTRFS_RAID_SHIFT); return BTRFS_BG_FLAG_TO_INDEX(profile); Otherwise looks good. Thanks, Anand >>>>> +} >>>>> + >>>>> struct btrfs_io_geometry { >>>>> /* remaining bytes before crossing a stripe */ >>>>> u64 len; >>>>> @@ -646,7 +669,6 @@ void btrfs_scratch_superblocks(struct >>>>> btrfs_fs_info *fs_info, >>>>> struct block_device *bdev, >>>>> const char *device_path); >>>>> -enum btrfs_raid_types __attribute_const__ >>>>> btrfs_bg_flags_to_raid_index(u64 flags); >>>>> int btrfs_bg_type_to_factor(u64 flags); >>>>> const char *btrfs_bg_type_to_raid_name(u64 flags); >>>>> int btrfs_verify_dev_extents(struct btrfs_fs_info *fs_info); >>>>> >>>> ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 2/2] btrfs: use ilog2() to replace if () branches for btrfs_bg_flags_to_raid_index() 2021-10-27 5:28 ` [PATCH v2 2/2] btrfs: use ilog2() to replace if () branches for btrfs_bg_flags_to_raid_index() Qu Wenruo 2021-10-27 6:37 ` Nikolay Borisov 2021-10-27 9:23 ` Anand Jain @ 2021-10-29 14:11 ` David Sterba 2021-10-29 23:38 ` Qu Wenruo 2 siblings, 1 reply; 13+ messages in thread From: David Sterba @ 2021-10-29 14:11 UTC (permalink / raw) To: Qu Wenruo; +Cc: linux-btrfs On Wed, Oct 27, 2021 at 01:28:59PM +0800, Qu Wenruo wrote: > +#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. I agree that deriving the indexes from the flags is safe but all the magic around that scares me a bit. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 2/2] btrfs: use ilog2() to replace if () branches for btrfs_bg_flags_to_raid_index() 2021-10-29 14:11 ` David Sterba @ 2021-10-29 23:38 ` Qu Wenruo 2021-11-02 17:16 ` David Sterba 0 siblings, 1 reply; 13+ messages in thread From: Qu Wenruo @ 2021-10-29 23:38 UTC (permalink / raw) To: dsterba, Qu Wenruo, linux-btrfs On 2021/10/29 22:11, David Sterba wrote: > On Wed, Oct 27, 2021 at 01:28:59PM +0800, Qu Wenruo wrote: >> +#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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 2/2] btrfs: use ilog2() to replace if () branches for btrfs_bg_flags_to_raid_index() 2021-10-29 23:38 ` Qu Wenruo @ 2021-11-02 17:16 ` David Sterba 0 siblings, 0 replies; 13+ messages in thread From: David Sterba @ 2021-11-02 17:16 UTC (permalink / raw) To: Qu Wenruo; +Cc: dsterba, Qu Wenruo, linux-btrfs On Sat, Oct 30, 2021 at 07:38:49AM +0800, Qu Wenruo wrote: > On 2021/10/29 22:11, David Sterba wrote: > > On Wed, Oct 27, 2021 at 01:28:59PM +0800, Qu Wenruo wrote: > >> +#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. > > 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2021-11-02 17:17 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-10-27 5:28 [PATCH v2 0/2] btrfs: re-define btrfs_raid_types Qu Wenruo 2021-10-27 5:28 ` [PATCH v2 1/2] btrfs: move definition of btrfs_raid_types to volumes.h Qu Wenruo 2021-10-27 5:28 ` [PATCH v2 2/2] btrfs: use ilog2() to replace if () branches for btrfs_bg_flags_to_raid_index() Qu Wenruo 2021-10-27 6:37 ` Nikolay Borisov 2021-10-27 7:41 ` Qu Wenruo 2021-10-27 9:23 ` Anand Jain 2021-10-27 10:41 ` Qu Wenruo 2021-10-28 1:04 ` Anand Jain 2021-10-28 7:10 ` Qu Wenruo 2021-10-28 21:53 ` Anand Jain 2021-10-29 14:11 ` David Sterba 2021-10-29 23:38 ` Qu Wenruo 2021-11-02 17:16 ` David Sterba
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.