* Re: [PATCH] btrfs: use unsigned types for constants defined as bit shifts
2025-04-22 15:55 [PATCH] btrfs: use unsigned types for constants defined as bit shifts David Sterba
@ 2025-05-05 17:38 ` Boris Burkov
2025-05-07 13:50 ` Daniel Vacek
2025-05-09 8:43 ` Daniel Vacek
2 siblings, 0 replies; 8+ messages in thread
From: Boris Burkov @ 2025-05-05 17:38 UTC (permalink / raw)
To: David Sterba; +Cc: linux-btrfs
On Tue, Apr 22, 2025 at 05:55:41PM +0200, David Sterba wrote:
> The unsigned type is a recommended practice (CWE-190, CWE-194) for bit
> shifts to avoid problems with potential unwanted sign extensions.
> Although there are no such cases in btrfs codebase, follow the
> recommendation.
>
> Signed-off-by: David Sterba <dsterba@suse.com>
Reviewed-by: Boris Burkov <boris@bur.io>
> ---
> fs/btrfs/backref.h | 4 ++--
> fs/btrfs/direct-io.c | 4 ++--
> fs/btrfs/extent_io.h | 2 +-
> fs/btrfs/inode.c | 12 ++++++------
> fs/btrfs/ordered-data.c | 4 ++--
> fs/btrfs/raid56.c | 5 ++---
> fs/btrfs/tests/extent-io-tests.c | 6 +++---
> fs/btrfs/zstd.c | 2 +-
> 8 files changed, 19 insertions(+), 20 deletions(-)
>
> diff --git a/fs/btrfs/backref.h b/fs/btrfs/backref.h
> index 74e6140312747f..953637115956b9 100644
> --- a/fs/btrfs/backref.h
> +++ b/fs/btrfs/backref.h
> @@ -423,8 +423,8 @@ struct btrfs_backref_node *btrfs_backref_alloc_node(
> struct btrfs_backref_edge *btrfs_backref_alloc_edge(
> struct btrfs_backref_cache *cache);
>
> -#define LINK_LOWER (1 << 0)
> -#define LINK_UPPER (1 << 1)
> +#define LINK_LOWER (1U << 0)
> +#define LINK_UPPER (1U << 1)
>
> void btrfs_backref_link_edge(struct btrfs_backref_edge *edge,
> struct btrfs_backref_node *lower,
> diff --git a/fs/btrfs/direct-io.c b/fs/btrfs/direct-io.c
> index 3a03142dee099b..fde612d9b077e3 100644
> --- a/fs/btrfs/direct-io.c
> +++ b/fs/btrfs/direct-io.c
> @@ -151,8 +151,8 @@ static struct extent_map *btrfs_create_dio_extent(struct btrfs_inode *inode,
> }
>
> ordered = btrfs_alloc_ordered_extent(inode, start, file_extent,
> - (1 << type) |
> - (1 << BTRFS_ORDERED_DIRECT));
> + (1U << type) |
> + (1U << BTRFS_ORDERED_DIRECT));
> if (IS_ERR(ordered)) {
> if (em) {
> btrfs_free_extent_map(em);
> diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h
> index bcdb067da06d1e..b677006ab14ee6 100644
> --- a/fs/btrfs/extent_io.h
> +++ b/fs/btrfs/extent_io.h
> @@ -67,7 +67,7 @@ enum {
> * single word in a bitmap may straddle two pages in the extent buffer.
> */
> #define BIT_BYTE(nr) ((nr) / BITS_PER_BYTE)
> -#define BYTE_MASK ((1 << BITS_PER_BYTE) - 1)
> +#define BYTE_MASK ((1U << BITS_PER_BYTE) - 1)
> #define BITMAP_FIRST_BYTE_MASK(start) \
> ((BYTE_MASK << ((start) & (BITS_PER_BYTE - 1))) & BYTE_MASK)
> #define BITMAP_LAST_BYTE_MASK(nbits) \
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 652db811e5cabd..73ef9b9b2b2cd3 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -1151,7 +1151,7 @@ static void submit_one_async_extent(struct async_chunk *async_chunk,
> btrfs_free_extent_map(em);
>
> ordered = btrfs_alloc_ordered_extent(inode, start, &file_extent,
> - 1 << BTRFS_ORDERED_COMPRESSED);
> + 1U << BTRFS_ORDERED_COMPRESSED);
> if (IS_ERR(ordered)) {
> btrfs_drop_extent_map_range(inode, start, end, false);
> ret = PTR_ERR(ordered);
> @@ -1396,7 +1396,7 @@ static noinline int cow_file_range(struct btrfs_inode *inode,
> btrfs_free_extent_map(em);
>
> ordered = btrfs_alloc_ordered_extent(inode, start, &file_extent,
> - 1 << BTRFS_ORDERED_REGULAR);
> + 1U << BTRFS_ORDERED_REGULAR);
> if (IS_ERR(ordered)) {
> btrfs_unlock_extent(&inode->io_tree, start,
> start + cur_alloc_size - 1, &cached);
> @@ -1976,8 +1976,8 @@ static int nocow_one_range(struct btrfs_inode *inode, struct folio *locked_folio
>
> ordered = btrfs_alloc_ordered_extent(inode, file_pos, &nocow_args->file_extent,
> is_prealloc
> - ? (1 << BTRFS_ORDERED_PREALLOC)
> - : (1 << BTRFS_ORDERED_NOCOW));
> + ? (1U << BTRFS_ORDERED_PREALLOC)
> + : (1U << BTRFS_ORDERED_NOCOW));
> if (IS_ERR(ordered)) {
> if (is_prealloc)
> btrfs_drop_extent_map_range(inode, file_pos, end, false);
> @@ -9688,8 +9688,8 @@ ssize_t btrfs_do_encoded_write(struct kiocb *iocb, struct iov_iter *from,
> btrfs_free_extent_map(em);
>
> ordered = btrfs_alloc_ordered_extent(inode, start, &file_extent,
> - (1 << BTRFS_ORDERED_ENCODED) |
> - (1 << BTRFS_ORDERED_COMPRESSED));
> + (1U << BTRFS_ORDERED_ENCODED) |
> + (1U << BTRFS_ORDERED_COMPRESSED));
> if (IS_ERR(ordered)) {
> btrfs_drop_extent_map_range(inode, start, end, false);
> ret = PTR_ERR(ordered);
> diff --git a/fs/btrfs/ordered-data.c b/fs/btrfs/ordered-data.c
> index b5b544712e93a3..6151d32704d2da 100644
> --- a/fs/btrfs/ordered-data.c
> +++ b/fs/btrfs/ordered-data.c
> @@ -155,7 +155,7 @@ static struct btrfs_ordered_extent *alloc_ordered_extent(
> u64 qgroup_rsv = 0;
>
> if (flags &
> - ((1 << BTRFS_ORDERED_NOCOW) | (1 << BTRFS_ORDERED_PREALLOC))) {
> + ((1U << BTRFS_ORDERED_NOCOW) | (1U << BTRFS_ORDERED_PREALLOC))) {
> /* For nocow write, we can release the qgroup rsv right now */
> ret = btrfs_qgroup_free_data(inode, NULL, file_offset, num_bytes, &qgroup_rsv);
> if (ret < 0)
> @@ -253,7 +253,7 @@ static void insert_ordered_extent(struct btrfs_ordered_extent *entry)
> * @disk_bytenr: Offset of extent on disk.
> * @disk_num_bytes: Size of extent on disk.
> * @offset: Offset into unencoded data where file data starts.
> - * @flags: Flags specifying type of extent (1 << BTRFS_ORDERED_*).
> + * @flags: Flags specifying type of extent (1U << BTRFS_ORDERED_*).
> * @compress_type: Compression algorithm used for data.
> *
> * Most of these parameters correspond to &struct btrfs_file_extent_item. The
> diff --git a/fs/btrfs/raid56.c b/fs/btrfs/raid56.c
> index 4657517f5480b7..06670e987f92f8 100644
> --- a/fs/btrfs/raid56.c
> +++ b/fs/btrfs/raid56.c
> @@ -203,8 +203,7 @@ int btrfs_alloc_stripe_hash_table(struct btrfs_fs_info *info)
> struct btrfs_stripe_hash_table *x;
> struct btrfs_stripe_hash *cur;
> struct btrfs_stripe_hash *h;
> - int num_entries = 1 << BTRFS_STRIPE_HASH_TABLE_BITS;
> - int i;
> + unsigned int num_entries = 1U << BTRFS_STRIPE_HASH_TABLE_BITS;
>
> if (info->stripe_hash_table)
> return 0;
> @@ -225,7 +224,7 @@ int btrfs_alloc_stripe_hash_table(struct btrfs_fs_info *info)
>
> h = table->table;
>
> - for (i = 0; i < num_entries; i++) {
> + for (unsigned int i = 0; i < num_entries; i++) {
> cur = h + i;
> INIT_LIST_HEAD(&cur->hash_list);
> spin_lock_init(&cur->lock);
> diff --git a/fs/btrfs/tests/extent-io-tests.c b/fs/btrfs/tests/extent-io-tests.c
> index b603563bd20986..d6aff41c38b165 100644
> --- a/fs/btrfs/tests/extent-io-tests.c
> +++ b/fs/btrfs/tests/extent-io-tests.c
> @@ -14,9 +14,9 @@
> #include "../disk-io.h"
> #include "../btrfs_inode.h"
>
> -#define PROCESS_UNLOCK (1 << 0)
> -#define PROCESS_RELEASE (1 << 1)
> -#define PROCESS_TEST_LOCKED (1 << 2)
> +#define PROCESS_UNLOCK (1U << 0)
> +#define PROCESS_RELEASE (1U << 1)
> +#define PROCESS_TEST_LOCKED (1U << 2)
>
> static noinline int process_page_range(struct inode *inode, u64 start, u64 end,
> unsigned long flags)
> diff --git a/fs/btrfs/zstd.c b/fs/btrfs/zstd.c
> index 75efca4da194c6..4a796a049b5a24 100644
> --- a/fs/btrfs/zstd.c
> +++ b/fs/btrfs/zstd.c
> @@ -24,7 +24,7 @@
> #include "super.h"
>
> #define ZSTD_BTRFS_MAX_WINDOWLOG 17
> -#define ZSTD_BTRFS_MAX_INPUT (1 << ZSTD_BTRFS_MAX_WINDOWLOG)
> +#define ZSTD_BTRFS_MAX_INPUT (1U << ZSTD_BTRFS_MAX_WINDOWLOG)
> #define ZSTD_BTRFS_DEFAULT_LEVEL 3
> #define ZSTD_BTRFS_MIN_LEVEL -15
> #define ZSTD_BTRFS_MAX_LEVEL 15
> --
> 2.49.0
>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] btrfs: use unsigned types for constants defined as bit shifts
2025-04-22 15:55 [PATCH] btrfs: use unsigned types for constants defined as bit shifts David Sterba
2025-05-05 17:38 ` Boris Burkov
@ 2025-05-07 13:50 ` Daniel Vacek
2025-05-07 17:43 ` David Sterba
2025-05-09 8:43 ` Daniel Vacek
2 siblings, 1 reply; 8+ messages in thread
From: Daniel Vacek @ 2025-05-07 13:50 UTC (permalink / raw)
To: David Sterba; +Cc: linux-btrfs
On Tue, 22 Apr 2025 at 17:55, David Sterba <dsterba@suse.com> wrote:
>
> The unsigned type is a recommended practice (CWE-190, CWE-194) for bit
> shifts to avoid problems with potential unwanted sign extensions.
> Although there are no such cases in btrfs codebase, follow the
> recommendation.
>
> Signed-off-by: David Sterba <dsterba@suse.com>
> ---
> fs/btrfs/backref.h | 4 ++--
> fs/btrfs/direct-io.c | 4 ++--
> fs/btrfs/extent_io.h | 2 +-
> fs/btrfs/inode.c | 12 ++++++------
> fs/btrfs/ordered-data.c | 4 ++--
> fs/btrfs/raid56.c | 5 ++---
> fs/btrfs/tests/extent-io-tests.c | 6 +++---
> fs/btrfs/zstd.c | 2 +-
> 8 files changed, 19 insertions(+), 20 deletions(-)
>
> diff --git a/fs/btrfs/backref.h b/fs/btrfs/backref.h
> index 74e6140312747f..953637115956b9 100644
> --- a/fs/btrfs/backref.h
> +++ b/fs/btrfs/backref.h
> @@ -423,8 +423,8 @@ struct btrfs_backref_node *btrfs_backref_alloc_node(
> struct btrfs_backref_edge *btrfs_backref_alloc_edge(
> struct btrfs_backref_cache *cache);
>
> -#define LINK_LOWER (1 << 0)
> -#define LINK_UPPER (1 << 1)
> +#define LINK_LOWER (1U << 0)
> +#define LINK_UPPER (1U << 1)
>
> void btrfs_backref_link_edge(struct btrfs_backref_edge *edge,
> struct btrfs_backref_node *lower,
> diff --git a/fs/btrfs/direct-io.c b/fs/btrfs/direct-io.c
> index 3a03142dee099b..fde612d9b077e3 100644
> --- a/fs/btrfs/direct-io.c
> +++ b/fs/btrfs/direct-io.c
> @@ -151,8 +151,8 @@ static struct extent_map *btrfs_create_dio_extent(struct btrfs_inode *inode,
> }
>
> ordered = btrfs_alloc_ordered_extent(inode, start, file_extent,
> - (1 << type) |
> - (1 << BTRFS_ORDERED_DIRECT));
> + (1U << type) |
> + (1U << BTRFS_ORDERED_DIRECT));
> if (IS_ERR(ordered)) {
> if (em) {
> btrfs_free_extent_map(em);
> diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h
> index bcdb067da06d1e..b677006ab14ee6 100644
> --- a/fs/btrfs/extent_io.h
> +++ b/fs/btrfs/extent_io.h
> @@ -67,7 +67,7 @@ enum {
> * single word in a bitmap may straddle two pages in the extent buffer.
> */
> #define BIT_BYTE(nr) ((nr) / BITS_PER_BYTE)
> -#define BYTE_MASK ((1 << BITS_PER_BYTE) - 1)
> +#define BYTE_MASK ((1U << BITS_PER_BYTE) - 1)
> #define BITMAP_FIRST_BYTE_MASK(start) \
> ((BYTE_MASK << ((start) & (BITS_PER_BYTE - 1))) & BYTE_MASK)
> #define BITMAP_LAST_BYTE_MASK(nbits) \
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 652db811e5cabd..73ef9b9b2b2cd3 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -1151,7 +1151,7 @@ static void submit_one_async_extent(struct async_chunk *async_chunk,
> btrfs_free_extent_map(em);
>
> ordered = btrfs_alloc_ordered_extent(inode, start, &file_extent,
> - 1 << BTRFS_ORDERED_COMPRESSED);
> + 1U << BTRFS_ORDERED_COMPRESSED);
> if (IS_ERR(ordered)) {
> btrfs_drop_extent_map_range(inode, start, end, false);
> ret = PTR_ERR(ordered);
> @@ -1396,7 +1396,7 @@ static noinline int cow_file_range(struct btrfs_inode *inode,
> btrfs_free_extent_map(em);
>
> ordered = btrfs_alloc_ordered_extent(inode, start, &file_extent,
> - 1 << BTRFS_ORDERED_REGULAR);
> + 1U << BTRFS_ORDERED_REGULAR);
> if (IS_ERR(ordered)) {
> btrfs_unlock_extent(&inode->io_tree, start,
> start + cur_alloc_size - 1, &cached);
> @@ -1976,8 +1976,8 @@ static int nocow_one_range(struct btrfs_inode *inode, struct folio *locked_folio
>
> ordered = btrfs_alloc_ordered_extent(inode, file_pos, &nocow_args->file_extent,
> is_prealloc
> - ? (1 << BTRFS_ORDERED_PREALLOC)
> - : (1 << BTRFS_ORDERED_NOCOW));
> + ? (1U << BTRFS_ORDERED_PREALLOC)
> + : (1U << BTRFS_ORDERED_NOCOW));
> if (IS_ERR(ordered)) {
> if (is_prealloc)
> btrfs_drop_extent_map_range(inode, file_pos, end, false);
> @@ -9688,8 +9688,8 @@ ssize_t btrfs_do_encoded_write(struct kiocb *iocb, struct iov_iter *from,
> btrfs_free_extent_map(em);
>
> ordered = btrfs_alloc_ordered_extent(inode, start, &file_extent,
> - (1 << BTRFS_ORDERED_ENCODED) |
> - (1 << BTRFS_ORDERED_COMPRESSED));
> + (1U << BTRFS_ORDERED_ENCODED) |
> + (1U << BTRFS_ORDERED_COMPRESSED));
> if (IS_ERR(ordered)) {
> btrfs_drop_extent_map_range(inode, start, end, false);
> ret = PTR_ERR(ordered);
> diff --git a/fs/btrfs/ordered-data.c b/fs/btrfs/ordered-data.c
> index b5b544712e93a3..6151d32704d2da 100644
> --- a/fs/btrfs/ordered-data.c
> +++ b/fs/btrfs/ordered-data.c
> @@ -155,7 +155,7 @@ static struct btrfs_ordered_extent *alloc_ordered_extent(
> u64 qgroup_rsv = 0;
>
> if (flags &
> - ((1 << BTRFS_ORDERED_NOCOW) | (1 << BTRFS_ORDERED_PREALLOC))) {
> + ((1U << BTRFS_ORDERED_NOCOW) | (1U << BTRFS_ORDERED_PREALLOC))) {
> /* For nocow write, we can release the qgroup rsv right now */
> ret = btrfs_qgroup_free_data(inode, NULL, file_offset, num_bytes, &qgroup_rsv);
> if (ret < 0)
> @@ -253,7 +253,7 @@ static void insert_ordered_extent(struct btrfs_ordered_extent *entry)
> * @disk_bytenr: Offset of extent on disk.
> * @disk_num_bytes: Size of extent on disk.
> * @offset: Offset into unencoded data where file data starts.
> - * @flags: Flags specifying type of extent (1 << BTRFS_ORDERED_*).
> + * @flags: Flags specifying type of extent (1U << BTRFS_ORDERED_*).
> * @compress_type: Compression algorithm used for data.
> *
> * Most of these parameters correspond to &struct btrfs_file_extent_item. The
> diff --git a/fs/btrfs/raid56.c b/fs/btrfs/raid56.c
> index 4657517f5480b7..06670e987f92f8 100644
> --- a/fs/btrfs/raid56.c
> +++ b/fs/btrfs/raid56.c
> @@ -203,8 +203,7 @@ int btrfs_alloc_stripe_hash_table(struct btrfs_fs_info *info)
> struct btrfs_stripe_hash_table *x;
> struct btrfs_stripe_hash *cur;
> struct btrfs_stripe_hash *h;
> - int num_entries = 1 << BTRFS_STRIPE_HASH_TABLE_BITS;
> - int i;
> + unsigned int num_entries = 1U << BTRFS_STRIPE_HASH_TABLE_BITS;
This one does not really make much sense. It is an isolated local thing.
> if (info->stripe_hash_table)
> return 0;
> @@ -225,7 +224,7 @@ int btrfs_alloc_stripe_hash_table(struct btrfs_fs_info *info)
>
> h = table->table;
>
> - for (i = 0; i < num_entries; i++) {
> + for (unsigned int i = 0; i < num_entries; i++) {
I'd just do:
for (int i = 0; i < 1 << BTRFS_STRIPE_HASH_TABLE_BITS; i++) {
The compiler will resolve the shift and the loop will compare to the
immediate constant value.
---
Quite honestly the whole patch is questionable. The recommendations
are about right shifts. Left shifts are not prone to any sinedness
issues.
What is more important is where the constants are being used. They
should honor the type they are compared with or assigned to. Like for
example 0x80ULL for flags as these are usually declared unsigned long
long, and so on...
For example the LINK_LOWER is converted to int when used as
btrfs_backref_link_edge(..., LINK_LOWER) parameter and then another
LINK_LOWER is and-ed to that int argument. I know, the logical
operations are not really influenced by the signedness, but still.
And btw, the LINK_UPPER is not used at all anywhere in the code, if I
see correctly.
---
In theory the situation could be even worse at some places as
incorrectly using an unsigned constant may force a signed variable to
get promoted to unsigned one to match. This may result in an int
variable with the value of -1 ending up as UINT_MAX instead. And now
imagine if(i < (1U << 4)) where int i = -1;
I did not check all the places where the constants you are changing
are being used, but it looks scary.
> cur = h + i;
> INIT_LIST_HEAD(&cur->hash_list);
> spin_lock_init(&cur->lock);
> diff --git a/fs/btrfs/tests/extent-io-tests.c b/fs/btrfs/tests/extent-io-tests.c
> index b603563bd20986..d6aff41c38b165 100644
> --- a/fs/btrfs/tests/extent-io-tests.c
> +++ b/fs/btrfs/tests/extent-io-tests.c
> @@ -14,9 +14,9 @@
> #include "../disk-io.h"
> #include "../btrfs_inode.h"
>
> -#define PROCESS_UNLOCK (1 << 0)
> -#define PROCESS_RELEASE (1 << 1)
> -#define PROCESS_TEST_LOCKED (1 << 2)
> +#define PROCESS_UNLOCK (1U << 0)
> +#define PROCESS_RELEASE (1U << 1)
> +#define PROCESS_TEST_LOCKED (1U << 2)
>
> static noinline int process_page_range(struct inode *inode, u64 start, u64 end,
> unsigned long flags)
> diff --git a/fs/btrfs/zstd.c b/fs/btrfs/zstd.c
> index 75efca4da194c6..4a796a049b5a24 100644
> --- a/fs/btrfs/zstd.c
> +++ b/fs/btrfs/zstd.c
> @@ -24,7 +24,7 @@
> #include "super.h"
>
> #define ZSTD_BTRFS_MAX_WINDOWLOG 17
> -#define ZSTD_BTRFS_MAX_INPUT (1 << ZSTD_BTRFS_MAX_WINDOWLOG)
> +#define ZSTD_BTRFS_MAX_INPUT (1U << ZSTD_BTRFS_MAX_WINDOWLOG)
> #define ZSTD_BTRFS_DEFAULT_LEVEL 3
> #define ZSTD_BTRFS_MIN_LEVEL -15
> #define ZSTD_BTRFS_MAX_LEVEL 15
> --
> 2.49.0
>
>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] btrfs: use unsigned types for constants defined as bit shifts
2025-05-07 13:50 ` Daniel Vacek
@ 2025-05-07 17:43 ` David Sterba
2025-05-07 21:53 ` Boris Burkov
2025-05-09 7:02 ` Daniel Vacek
0 siblings, 2 replies; 8+ messages in thread
From: David Sterba @ 2025-05-07 17:43 UTC (permalink / raw)
To: Daniel Vacek; +Cc: David Sterba, linux-btrfs
On Wed, May 07, 2025 at 03:50:51PM +0200, Daniel Vacek wrote:
> > --- a/fs/btrfs/raid56.c
> > +++ b/fs/btrfs/raid56.c
> > @@ -203,8 +203,7 @@ int btrfs_alloc_stripe_hash_table(struct btrfs_fs_info *info)
> > struct btrfs_stripe_hash_table *x;
> > struct btrfs_stripe_hash *cur;
> > struct btrfs_stripe_hash *h;
> > - int num_entries = 1 << BTRFS_STRIPE_HASH_TABLE_BITS;
> > - int i;
> > + unsigned int num_entries = 1U << BTRFS_STRIPE_HASH_TABLE_BITS;
>
> This one does not really make much sense. It is an isolated local thing.
>
> > if (info->stripe_hash_table)
> > return 0;
> > @@ -225,7 +224,7 @@ int btrfs_alloc_stripe_hash_table(struct btrfs_fs_info *info)
> >
> > h = table->table;
> >
> > - for (i = 0; i < num_entries; i++) {
> > + for (unsigned int i = 0; i < num_entries; i++) {
>
> I'd just do:
>
> for (int i = 0; i < 1 << BTRFS_STRIPE_HASH_TABLE_BITS; i++) {
>
> The compiler will resolve the shift and the loop will compare to the
> immediate constant value.
Yes, it's a compile time constant. It's in num_entries because it's used
twice in the function, for that we usually have a local variable so we
don't have to open code the value everywhere.
> ---
>
> Quite honestly the whole patch is questionable. The recommendations
> are about right shifts. Left shifts are not prone to any sinedness
> issues.
>
> What is more important is where the constants are being used. They
> should honor the type they are compared with or assigned to. Like for
> example 0x80ULL for flags as these are usually declared unsigned long
> long, and so on...
Agreed, flags and masks should be unsigned.
> For example the LINK_LOWER is converted to int when used as
> btrfs_backref_link_edge(..., LINK_LOWER) parameter and then another
> LINK_LOWER is and-ed to that int argument. I know, the logical
> operations are not really influenced by the signedness, but still.
Well, it's more a matter of consistency and coding style. We did have a
real bug with signed bit defined on 32bit int that got promoted to u64
not as a single bit but 0xffffffff80000000 and this even got propagated
to on-disk data. We were lucky it never had any real impact but since
then I'm on the side of making bit shifts unconditionally on unsigned
types. 77eea05e7851d910b7992c8c237a6b5d462050da
>
> And btw, the LINK_UPPER is not used at all anywhere in the code, if I
> see correctly.
$ git grep LINK_UPPER
backref.c: if (link_which & LINK_UPPER)
backref.h:#define LINK_UPPER (1U << 1)
> ---
>
> In theory the situation could be even worse at some places as
> incorrectly using an unsigned constant may force a signed variable to
> get promoted to unsigned one to match. This may result in an int
> variable with the value of -1 ending up as UINT_MAX instead. And now
> imagine if(i < (1U << 4)) where int i = -1;
>
> I did not check all the places where the constants you are changing
> are being used, but it looks scary.
This is touching the core problem, mixing the signed and unsigned types
in the wrong and very unobvious way. But we maybe disagree that it
should be int, while I'd rather unify that to unsigned.
If you find a scary and potentially wrong use please send a RFC patch so
we can see how much we can avoid that by changing that to a safer
pattern.
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] btrfs: use unsigned types for constants defined as bit shifts
2025-05-07 17:43 ` David Sterba
@ 2025-05-07 21:53 ` Boris Burkov
2025-05-09 7:02 ` Daniel Vacek
1 sibling, 0 replies; 8+ messages in thread
From: Boris Burkov @ 2025-05-07 21:53 UTC (permalink / raw)
To: David Sterba; +Cc: Daniel Vacek, David Sterba, linux-btrfs
On Wed, May 07, 2025 at 07:43:28PM +0200, David Sterba wrote:
> On Wed, May 07, 2025 at 03:50:51PM +0200, Daniel Vacek wrote:
> > > --- a/fs/btrfs/raid56.c
> > > +++ b/fs/btrfs/raid56.c
> > > @@ -203,8 +203,7 @@ int btrfs_alloc_stripe_hash_table(struct btrfs_fs_info *info)
> > > struct btrfs_stripe_hash_table *x;
> > > struct btrfs_stripe_hash *cur;
> > > struct btrfs_stripe_hash *h;
> > > - int num_entries = 1 << BTRFS_STRIPE_HASH_TABLE_BITS;
> > > - int i;
> > > + unsigned int num_entries = 1U << BTRFS_STRIPE_HASH_TABLE_BITS;
> >
> > This one does not really make much sense. It is an isolated local thing.
> >
> > > if (info->stripe_hash_table)
> > > return 0;
> > > @@ -225,7 +224,7 @@ int btrfs_alloc_stripe_hash_table(struct btrfs_fs_info *info)
> > >
> > > h = table->table;
> > >
> > > - for (i = 0; i < num_entries; i++) {
> > > + for (unsigned int i = 0; i < num_entries; i++) {
> >
> > I'd just do:
> >
> > for (int i = 0; i < 1 << BTRFS_STRIPE_HASH_TABLE_BITS; i++) {
> >
> > The compiler will resolve the shift and the loop will compare to the
> > immediate constant value.
>
> Yes, it's a compile time constant. It's in num_entries because it's used
> twice in the function, for that we usually have a local variable so we
> don't have to open code the value everywhere.
>
> > ---
> >
> > Quite honestly the whole patch is questionable. The recommendations
> > are about right shifts. Left shifts are not prone to any sinedness
> > issues.
> >
> > What is more important is where the constants are being used. They
> > should honor the type they are compared with or assigned to. Like for
> > example 0x80ULL for flags as these are usually declared unsigned long
> > long, and so on...
>
> Agreed, flags and masks should be unsigned.
>
> > For example the LINK_LOWER is converted to int when used as
> > btrfs_backref_link_edge(..., LINK_LOWER) parameter and then another
> > LINK_LOWER is and-ed to that int argument. I know, the logical
> > operations are not really influenced by the signedness, but still.
>
> Well, it's more a matter of consistency and coding style. We did have a
> real bug with signed bit defined on 32bit int that got promoted to u64
> not as a single bit but 0xffffffff80000000 and this even got propagated
> to on-disk data. We were lucky it never had any real impact but since
> then I'm on the side of making bit shifts unconditionally on unsigned
> types. 77eea05e7851d910b7992c8c237a6b5d462050da
>
For what it's worth, my support for moving to unsigned types for all
shifts is based on that same investigation.
I think a sign extension on s32->u64 is way more surprising and hard to
debug than a "whoops I thought -1 was less than 1 but actually it's
bigger", which is the first thing you would think of for weird
arithmetic errors.
> >
> > And btw, the LINK_UPPER is not used at all anywhere in the code, if I
> > see correctly.
>
> $ git grep LINK_UPPER
> backref.c: if (link_which & LINK_UPPER)
> backref.h:#define LINK_UPPER (1U << 1)
>
> > ---
> >
> > In theory the situation could be even worse at some places as
> > incorrectly using an unsigned constant may force a signed variable to
> > get promoted to unsigned one to match. This may result in an int
> > variable with the value of -1 ending up as UINT_MAX instead. And now
> > imagine if(i < (1U << 4)) where int i = -1;
> >
> > I did not check all the places where the constants you are changing
> > are being used, but it looks scary.
>
> This is touching the core problem, mixing the signed and unsigned types
> in the wrong and very unobvious way. But we maybe disagree that it
> should be int, while I'd rather unify that to unsigned.
>
> If you find a scary and potentially wrong use please send a RFC patch so
> we can see how much we can avoid that by changing that to a safer
> pattern.
I suppose the MOST convincing form of this patch would thoroughly audit
all the users/arithmetic involved to make sure they aren't doing
anything silly. I immediately noticed that LINK_UPPER/LINK_LOWER do get
passed as ints to btrfs_backref_link_edge() in a way that is fine to my
eye, but it wouldn't hurt to change that unsigned int too, the way you
did in a few other places?
I am still comfortable with this as-is.
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] btrfs: use unsigned types for constants defined as bit shifts
2025-05-07 17:43 ` David Sterba
2025-05-07 21:53 ` Boris Burkov
@ 2025-05-09 7:02 ` Daniel Vacek
2025-05-12 20:04 ` David Sterba
1 sibling, 1 reply; 8+ messages in thread
From: Daniel Vacek @ 2025-05-09 7:02 UTC (permalink / raw)
To: dsterba; +Cc: David Sterba, linux-btrfs
On Wed, 7 May 2025 at 19:43, David Sterba <dsterba@suse.cz> wrote:
>
> On Wed, May 07, 2025 at 03:50:51PM +0200, Daniel Vacek wrote:
> > > --- a/fs/btrfs/raid56.c
> > > +++ b/fs/btrfs/raid56.c
> > > @@ -203,8 +203,7 @@ int btrfs_alloc_stripe_hash_table(struct btrfs_fs_info *info)
> > > struct btrfs_stripe_hash_table *x;
> > > struct btrfs_stripe_hash *cur;
> > > struct btrfs_stripe_hash *h;
> > > - int num_entries = 1 << BTRFS_STRIPE_HASH_TABLE_BITS;
> > > - int i;
> > > + unsigned int num_entries = 1U << BTRFS_STRIPE_HASH_TABLE_BITS;
> >
> > This one does not really make much sense. It is an isolated local thing.
> >
> > > if (info->stripe_hash_table)
> > > return 0;
> > > @@ -225,7 +224,7 @@ int btrfs_alloc_stripe_hash_table(struct btrfs_fs_info *info)
> > >
> > > h = table->table;
> > >
> > > - for (i = 0; i < num_entries; i++) {
> > > + for (unsigned int i = 0; i < num_entries; i++) {
> >
> > I'd just do:
> >
> > for (int i = 0; i < 1 << BTRFS_STRIPE_HASH_TABLE_BITS; i++) {
> >
> > The compiler will resolve the shift and the loop will compare to the
> > immediate constant value.
>
> Yes, it's a compile time constant. It's in num_entries because it's used
> twice in the function, for that we usually have a local variable so we
> don't have to open code the value everywhere.
Right. I'm just blind, sorry. Could be const unsigned int.
I have noticed before, using `const` is also not consistent within btrfs
code. But that's OT here.
> > ---
> >
> > Quite honestly the whole patch is questionable. The recommendations
> > are about right shifts. Left shifts are not prone to any sinedness
> > issues.
> >
> > What is more important is where the constants are being used. They
> > should honor the type they are compared with or assigned to. Like for
> > example 0x80ULL for flags as these are usually declared unsigned long
> > long, and so on...
>
> Agreed, flags and masks should be unsigned.
>
> > For example the LINK_LOWER is converted to int when used as
> > btrfs_backref_link_edge(..., LINK_LOWER) parameter and then another
> > LINK_LOWER is and-ed to that int argument. I know, the logical
> > operations are not really influenced by the signedness, but still.
>
> Well, it's more a matter of consistency and coding style. We did have a
> real bug with signed bit defined on 32bit int that got promoted to u64
> not as a single bit but 0xffffffff80000000 and this even got propagated
> to on-disk data. We were lucky it never had any real impact but since
> then I'm on the side of making bit shifts unconditionally on unsigned
> types. 77eea05e7851d910b7992c8c237a6b5d462050da
Yeah, that's nasty. That's precisely what I meant when saying that
more important is the types actually do match. The implicit promotions
are the source of hidden bugs.
I'll check all the changes once again, to be sure.
> >
> > And btw, the LINK_UPPER is not used at all anywhere in the code, if I
> > see correctly.
>
> $ git grep LINK_UPPER
> backref.c: if (link_which & LINK_UPPER)
> backref.h:#define LINK_UPPER (1U << 1)
This is what I was referring to. The flag is never used. The whole
`if` can be removed as it'll always be false.
$ g g btrfs_backref_link_edge
fs/btrfs/backref.c:void btrfs_backref_link_edge(struct btrfs_backref_edge *edge,
fs/btrfs/backref.c: btrfs_backref_link_edge(edge, cur, upper, LINK_LOWER);
fs/btrfs/backref.c: btrfs_backref_link_edge(edge, lower, upper,
LINK_LOWER);
fs/btrfs/backref.h:void btrfs_backref_link_edge(struct btrfs_backref_edge *edge,
> > ---
> >
> > In theory the situation could be even worse at some places as
> > incorrectly using an unsigned constant may force a signed variable to
> > get promoted to unsigned one to match. This may result in an int
> > variable with the value of -1 ending up as UINT_MAX instead. And now
> > imagine if(i < (1U << 4)) where int i = -1;
> >
> > I did not check all the places where the constants you are changing
> > are being used, but it looks scary.
>
> This is touching the core problem, mixing the signed and unsigned types
> in the wrong and very unobvious way. But we maybe disagree that it
> should be int, while I'd rather unify that to unsigned.
>
> If you find a scary and potentially wrong use please send a RFC patch so
> we can see how much we can avoid that by changing that to a safer
> pattern.
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] btrfs: use unsigned types for constants defined as bit shifts
2025-05-09 7:02 ` Daniel Vacek
@ 2025-05-12 20:04 ` David Sterba
0 siblings, 0 replies; 8+ messages in thread
From: David Sterba @ 2025-05-12 20:04 UTC (permalink / raw)
To: Daniel Vacek; +Cc: dsterba, David Sterba, linux-btrfs
On Fri, May 09, 2025 at 09:02:56AM +0200, Daniel Vacek wrote:
> > Yes, it's a compile time constant. It's in num_entries because it's used
> > twice in the function, for that we usually have a local variable so we
> > don't have to open code the value everywhere.
>
> Right. I'm just blind, sorry. Could be const unsigned int.
>
> I have noticed before, using `const` is also not consistent within btrfs
> code. But that's OT here.
Yes it is inconsistent, it was not part of the coding style from the
beginning and the codebase is now like 18 years old. We've started
adding const to local variables some years ago.
I did some passes over the parameters, and add const in patches in
for-next unless it's there already. Further unification is probably a
good thing, though it may need some taste where to add them. Compiler
usually has enough information to see which variables change or not, so
some cases are implicit const anyway.
An optimization to reload a value of non-const variable in the middle of
other code is also possible but there are constraints that may prevent
it in most cases. From what I've seen adding const also prevents this
optimizations. But again this is so rare that we may not need to care
about that too much.
I did some random search and clear candidates for const are fscrypt_str
variables.
> > > ---
> > >
> > > Quite honestly the whole patch is questionable. The recommendations
> > > are about right shifts. Left shifts are not prone to any sinedness
> > > issues.
> > >
> > > What is more important is where the constants are being used. They
> > > should honor the type they are compared with or assigned to. Like for
> > > example 0x80ULL for flags as these are usually declared unsigned long
> > > long, and so on...
> >
> > Agreed, flags and masks should be unsigned.
> >
> > > For example the LINK_LOWER is converted to int when used as
> > > btrfs_backref_link_edge(..., LINK_LOWER) parameter and then another
> > > LINK_LOWER is and-ed to that int argument. I know, the logical
> > > operations are not really influenced by the signedness, but still.
> >
> > Well, it's more a matter of consistency and coding style. We did have a
> > real bug with signed bit defined on 32bit int that got promoted to u64
> > not as a single bit but 0xffffffff80000000 and this even got propagated
> > to on-disk data. We were lucky it never had any real impact but since
> > then I'm on the side of making bit shifts unconditionally on unsigned
> > types. 77eea05e7851d910b7992c8c237a6b5d462050da
>
> Yeah, that's nasty. That's precisely what I meant when saying that
> more important is the types actually do match. The implicit promotions
> are the source of hidden bugs.
>
> I'll check all the changes once again, to be sure.
>
> > >
> > > And btw, the LINK_UPPER is not used at all anywhere in the code, if I
> > > see correctly.
> >
> > $ git grep LINK_UPPER
> > backref.c: if (link_which & LINK_UPPER)
> > backref.h:#define LINK_UPPER (1U << 1)
>
> This is what I was referring to. The flag is never used. The whole
> `if` can be removed as it'll always be false.
Right. The last use was in 0097422c0dfe0a ("btrfs: remove
clone_backref_node() from relocation") so it's a leftover and can be
removed and there are probably some cascdaded changes from there.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] btrfs: use unsigned types for constants defined as bit shifts
2025-04-22 15:55 [PATCH] btrfs: use unsigned types for constants defined as bit shifts David Sterba
2025-05-05 17:38 ` Boris Burkov
2025-05-07 13:50 ` Daniel Vacek
@ 2025-05-09 8:43 ` Daniel Vacek
2 siblings, 0 replies; 8+ messages in thread
From: Daniel Vacek @ 2025-05-09 8:43 UTC (permalink / raw)
To: David Sterba; +Cc: linux-btrfs
On Tue, 22 Apr 2025 at 17:55, David Sterba <dsterba@suse.com> wrote:
>
> The unsigned type is a recommended practice (CWE-190, CWE-194) for bit
> shifts to avoid problems with potential unwanted sign extensions.
> Although there are no such cases in btrfs codebase, follow the
> recommendation.
>
> Signed-off-by: David Sterba <dsterba@suse.com>
> ---
> fs/btrfs/backref.h | 4 ++--
> fs/btrfs/direct-io.c | 4 ++--
> fs/btrfs/extent_io.h | 2 +-
> fs/btrfs/inode.c | 12 ++++++------
> fs/btrfs/ordered-data.c | 4 ++--
> fs/btrfs/raid56.c | 5 ++---
> fs/btrfs/tests/extent-io-tests.c | 6 +++---
> fs/btrfs/zstd.c | 2 +-
> 8 files changed, 19 insertions(+), 20 deletions(-)
>
> diff --git a/fs/btrfs/backref.h b/fs/btrfs/backref.h
> index 74e6140312747f..953637115956b9 100644
> --- a/fs/btrfs/backref.h
> +++ b/fs/btrfs/backref.h
> @@ -423,8 +423,8 @@ struct btrfs_backref_node *btrfs_backref_alloc_node(
> struct btrfs_backref_edge *btrfs_backref_alloc_edge(
> struct btrfs_backref_cache *cache);
>
> -#define LINK_LOWER (1 << 0)
> -#define LINK_UPPER (1 << 1)
> +#define LINK_LOWER (1U << 0)
> +#define LINK_UPPER (1U << 1)
With this maybe you also want
--- a/fs/btrfs/backref.c
+++ b/fs/btrfs/backref.c
@@ -3164,7 +3164,7 @@ void btrfs_backref_release_cache(struct
btrfs_backref_cache *cache)
void btrfs_backref_link_edge(struct btrfs_backref_edge *edge,
struct btrfs_backref_node *lower,
struct btrfs_backref_node *upper,
- int link_which)
+ unsigned int link_which)
{
ASSERT(upper && lower && upper->level == lower->level + 1);
edge->node[LOWER] = lower;
Otherwise you changed it to unsigned which gets converted back to
signed again. Ie., the change does not make much sense. The end result
is (int)(1U << 0).
And as mentioned before, the LINK_UPPER may be removed.
> void btrfs_backref_link_edge(struct btrfs_backref_edge *edge,
> struct btrfs_backref_node *lower,
> diff --git a/fs/btrfs/direct-io.c b/fs/btrfs/direct-io.c
> index 3a03142dee099b..fde612d9b077e3 100644
> --- a/fs/btrfs/direct-io.c
> +++ b/fs/btrfs/direct-io.c
> @@ -151,8 +151,8 @@ static struct extent_map *btrfs_create_dio_extent(struct btrfs_inode *inode,
> }
>
> ordered = btrfs_alloc_ordered_extent(inode, start, file_extent,
> - (1 << type) |
> - (1 << BTRFS_ORDERED_DIRECT));
> + (1U << type) |
> + (1U << BTRFS_ORDERED_DIRECT));
Technically these should be 1ul but this will get implicitly promoted.
Luckily we're using no more than 13 bits so far so no bit is lost.
> if (IS_ERR(ordered)) {
> if (em) {
> btrfs_free_extent_map(em);
> diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h
> index bcdb067da06d1e..b677006ab14ee6 100644
> --- a/fs/btrfs/extent_io.h
> +++ b/fs/btrfs/extent_io.h
> @@ -67,7 +67,7 @@ enum {
> * single word in a bitmap may straddle two pages in the extent buffer.
> */
> #define BIT_BYTE(nr) ((nr) / BITS_PER_BYTE)
> -#define BYTE_MASK ((1 << BITS_PER_BYTE) - 1)
> +#define BYTE_MASK ((1U << BITS_PER_BYTE) - 1)
> #define BITMAP_FIRST_BYTE_MASK(start) \
> ((BYTE_MASK << ((start) & (BITS_PER_BYTE - 1))) & BYTE_MASK)
> #define BITMAP_LAST_BYTE_MASK(nbits) \
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 652db811e5cabd..73ef9b9b2b2cd3 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -1151,7 +1151,7 @@ static void submit_one_async_extent(struct async_chunk *async_chunk,
> btrfs_free_extent_map(em);
>
> ordered = btrfs_alloc_ordered_extent(inode, start, &file_extent,
> - 1 << BTRFS_ORDERED_COMPRESSED);
> + 1U << BTRFS_ORDERED_COMPRESSED);
1UL
> if (IS_ERR(ordered)) {
> btrfs_drop_extent_map_range(inode, start, end, false);
> ret = PTR_ERR(ordered);
> @@ -1396,7 +1396,7 @@ static noinline int cow_file_range(struct btrfs_inode *inode,
> btrfs_free_extent_map(em);
>
> ordered = btrfs_alloc_ordered_extent(inode, start, &file_extent,
> - 1 << BTRFS_ORDERED_REGULAR);
> + 1U << BTRFS_ORDERED_REGULAR);
and again
> if (IS_ERR(ordered)) {
> btrfs_unlock_extent(&inode->io_tree, start,
> start + cur_alloc_size - 1, &cached);
> @@ -1976,8 +1976,8 @@ static int nocow_one_range(struct btrfs_inode *inode, struct folio *locked_folio
>
> ordered = btrfs_alloc_ordered_extent(inode, file_pos, &nocow_args->file_extent,
> is_prealloc
> - ? (1 << BTRFS_ORDERED_PREALLOC)
> - : (1 << BTRFS_ORDERED_NOCOW));
> + ? (1U << BTRFS_ORDERED_PREALLOC)
> + : (1U << BTRFS_ORDERED_NOCOW));
same
> if (IS_ERR(ordered)) {
> if (is_prealloc)
> btrfs_drop_extent_map_range(inode, file_pos, end, false);
> @@ -9688,8 +9688,8 @@ ssize_t btrfs_do_encoded_write(struct kiocb *iocb, struct iov_iter *from,
> btrfs_free_extent_map(em);
>
> ordered = btrfs_alloc_ordered_extent(inode, start, &file_extent,
> - (1 << BTRFS_ORDERED_ENCODED) |
> - (1 << BTRFS_ORDERED_COMPRESSED));
> + (1U << BTRFS_ORDERED_ENCODED) |
> + (1U << BTRFS_ORDERED_COMPRESSED));
same
> if (IS_ERR(ordered)) {
> btrfs_drop_extent_map_range(inode, start, end, false);
> ret = PTR_ERR(ordered);
> diff --git a/fs/btrfs/ordered-data.c b/fs/btrfs/ordered-data.c
> index b5b544712e93a3..6151d32704d2da 100644
> --- a/fs/btrfs/ordered-data.c
> +++ b/fs/btrfs/ordered-data.c
> @@ -155,7 +155,7 @@ static struct btrfs_ordered_extent *alloc_ordered_extent(
> u64 qgroup_rsv = 0;
>
> if (flags &
> - ((1 << BTRFS_ORDERED_NOCOW) | (1 << BTRFS_ORDERED_PREALLOC))) {
> + ((1U << BTRFS_ORDERED_NOCOW) | (1U << BTRFS_ORDERED_PREALLOC))) {
same
> /* For nocow write, we can release the qgroup rsv right now */
> ret = btrfs_qgroup_free_data(inode, NULL, file_offset, num_bytes, &qgroup_rsv);
> if (ret < 0)
> @@ -253,7 +253,7 @@ static void insert_ordered_extent(struct btrfs_ordered_extent *entry)
> * @disk_bytenr: Offset of extent on disk.
> * @disk_num_bytes: Size of extent on disk.
> * @offset: Offset into unencoded data where file data starts.
> - * @flags: Flags specifying type of extent (1 << BTRFS_ORDERED_*).
> + * @flags: Flags specifying type of extent (1U << BTRFS_ORDERED_*).
> * @compress_type: Compression algorithm used for data.
> *
> * Most of these parameters correspond to &struct btrfs_file_extent_item. The
> diff --git a/fs/btrfs/raid56.c b/fs/btrfs/raid56.c
> index 4657517f5480b7..06670e987f92f8 100644
> --- a/fs/btrfs/raid56.c
> +++ b/fs/btrfs/raid56.c
> @@ -203,8 +203,7 @@ int btrfs_alloc_stripe_hash_table(struct btrfs_fs_info *info)
> struct btrfs_stripe_hash_table *x;
> struct btrfs_stripe_hash *cur;
> struct btrfs_stripe_hash *h;
> - int num_entries = 1 << BTRFS_STRIPE_HASH_TABLE_BITS;
> - int i;
> + unsigned int num_entries = 1U << BTRFS_STRIPE_HASH_TABLE_BITS;
>
> if (info->stripe_hash_table)
> return 0;
> @@ -225,7 +224,7 @@ int btrfs_alloc_stripe_hash_table(struct btrfs_fs_info *info)
>
> h = table->table;
>
> - for (i = 0; i < num_entries; i++) {
> + for (unsigned int i = 0; i < num_entries; i++) {
> cur = h + i;
> INIT_LIST_HEAD(&cur->hash_list);
> spin_lock_init(&cur->lock);
> diff --git a/fs/btrfs/tests/extent-io-tests.c b/fs/btrfs/tests/extent-io-tests.c
> index b603563bd20986..d6aff41c38b165 100644
> --- a/fs/btrfs/tests/extent-io-tests.c
> +++ b/fs/btrfs/tests/extent-io-tests.c
> @@ -14,9 +14,9 @@
> #include "../disk-io.h"
> #include "../btrfs_inode.h"
>
> -#define PROCESS_UNLOCK (1 << 0)
> -#define PROCESS_RELEASE (1 << 1)
> -#define PROCESS_TEST_LOCKED (1 << 2)
> +#define PROCESS_UNLOCK (1U << 0)
> +#define PROCESS_RELEASE (1U << 1)
> +#define PROCESS_TEST_LOCKED (1U << 2)
same
> static noinline int process_page_range(struct inode *inode, u64 start, u64 end,
> unsigned long flags)
> diff --git a/fs/btrfs/zstd.c b/fs/btrfs/zstd.c
> index 75efca4da194c6..4a796a049b5a24 100644
> --- a/fs/btrfs/zstd.c
> +++ b/fs/btrfs/zstd.c
> @@ -24,7 +24,7 @@
> #include "super.h"
>
> #define ZSTD_BTRFS_MAX_WINDOWLOG 17
> -#define ZSTD_BTRFS_MAX_INPUT (1 << ZSTD_BTRFS_MAX_WINDOWLOG)
> +#define ZSTD_BTRFS_MAX_INPUT (1U << ZSTD_BTRFS_MAX_WINDOWLOG)
This one is being used in place of size_t, so technically it should
also be 1UL. But also, 17 bits happily fit into uint here.
Anyways, all these 1ULs are not a big deal. Having 1U is just fine. So
sorry about the noise.
To wrap it up, it's just the btrfs_backref_link_edge() prototype being
fishy and LINK_UPPER being useless. The rest is OK.
> #define ZSTD_BTRFS_DEFAULT_LEVEL 3
> #define ZSTD_BTRFS_MIN_LEVEL -15
> #define ZSTD_BTRFS_MAX_LEVEL 15
> --
> 2.49.0
>
>
^ permalink raw reply [flat|nested] 8+ messages in thread