[PATCH 0/6] Cleanups and W=2 warning fixes

[PATCH 0/6] Cleanups and W=2 warning fixes

[David Sterba]

We have a clean run of 'make W='1 with gcc 13, there are some
interesting warnings to fix with level 2 and even 3. We can't enable the
warning flags by defualt due to reports from generic code.

This short series removes shadowed variables, adds const and removes
an unused macro. There are still some shadow variables to fix but the
remaining cases are with 'ret' variables so I skipped it for now.

David Sterba (6):
  btrfs: remove duplicate name variable declarations
  btrfs: rename macro local variables that clash with other variables
  btrfs: use for-local variabls that shadow function variables
  btrfs: remove unused define EXTENT_SIZE_PER_ITEM
  btrfs: keep const whene returnin value from get_unaligned_le8()
  btrfs: constify parameters of write_eb_member() and its users

 fs/btrfs/accessors.h   | 12 ++++++------
 fs/btrfs/extent_io.c   |  6 ++----
 fs/btrfs/inode.c       |  2 --
 fs/btrfs/qgroup.c      | 11 +++++------
 fs/btrfs/space-info.c  |  2 --
 fs/btrfs/subpage.c     |  8 ++++----
 fs/btrfs/transaction.h |  6 +++---
 fs/btrfs/volumes.c     |  9 +++------
 fs/btrfs/zoned.c       |  8 +++-----
 9 files changed, 26 insertions(+), 38 deletions(-)

-- 
2.45.0

[PATCH 1/6] btrfs: remove duplicate name variable declarations

[David Sterba]

When running 'make W=2' there are a few reports where a variable of the
same name is declared in a nested block. In all the cases we can use the
one declared in the parent block, no problematic cases were found.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/extent_io.c | 4 +---
 fs/btrfs/inode.c     | 2 --
 2 files changed, 1 insertion(+), 5 deletions(-)

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 597387e9f040..2d773c1cbaa7 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -3507,7 +3507,6 @@ struct extent_buffer *btrfs_clone_extent_buffer(const struct extent_buffer *src)
 
 	for (int i = 0; i < num_folios; i++) {
 		struct folio *folio = new->folios[i];
-		int ret;
 
 		ret = attach_extent_buffer_folio(new, folio, NULL);
 		if (ret < 0) {
@@ -4587,8 +4586,7 @@ static void assert_eb_folio_uptodate(const struct extent_buffer *eb, int i)
 		return;
 
 	if (fs_info->nodesize < PAGE_SIZE) {
-		struct folio *folio = eb->folios[0];
-
+		folio = eb->folios[0];
 		ASSERT(i == 0);
 		if (WARN_ON(!btrfs_subpage_test_uptodate(fs_info, folio,
 							 eb->start, eb->len)))
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 3cf32bc721d2..87278d2f8447 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -1617,10 +1617,8 @@ static noinline void submit_compressed_extents(struct btrfs_work *work, bool do_
 	u64 alloc_hint = 0;
 
 	if (do_free) {
-		struct async_chunk *async_chunk;
 		struct async_cow *async_cow;
 
-		async_chunk = container_of(work, struct async_chunk, work);
 		btrfs_add_delayed_iput(async_chunk->inode);
 		if (async_chunk->blkcg_css)
 			css_put(async_chunk->blkcg_css);
-- 
2.45.0

[PATCH 2/6] btrfs: rename macro local variables that clash with other variables

[David Sterba]

Fix variable names in two macros where there's a local function variable
of the same name.  In subpage_calc_start_bit() it's in several callers,
in btrfs_abort_transaction() it's only in replace_file_extents().
Found by 'make W=2'.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/subpage.c     | 8 ++++----
 fs/btrfs/transaction.h | 6 +++---
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/fs/btrfs/subpage.c b/fs/btrfs/subpage.c
index 54736f6238e6..9127704236ab 100644
--- a/fs/btrfs/subpage.c
+++ b/fs/btrfs/subpage.c
@@ -242,12 +242,12 @@ static void btrfs_subpage_assert(const struct btrfs_fs_info *fs_info,
 
 #define subpage_calc_start_bit(fs_info, folio, name, start, len)	\
 ({									\
-	unsigned int start_bit;						\
+	unsigned int __start_bit;						\
 									\
 	btrfs_subpage_assert(fs_info, folio, start, len);		\
-	start_bit = offset_in_page(start) >> fs_info->sectorsize_bits;	\
-	start_bit += fs_info->subpage_info->name##_offset;		\
-	start_bit;							\
+	__start_bit = offset_in_page(start) >> fs_info->sectorsize_bits;	\
+	__start_bit += fs_info->subpage_info->name##_offset;		\
+	__start_bit;							\
 })
 
 void btrfs_subpage_start_reader(const struct btrfs_fs_info *fs_info,
diff --git a/fs/btrfs/transaction.h b/fs/btrfs/transaction.h
index 4e451ab173b1..90b987941dd1 100644
--- a/fs/btrfs/transaction.h
+++ b/fs/btrfs/transaction.h
@@ -229,11 +229,11 @@ bool __cold abort_should_print_stack(int error);
  */
 #define btrfs_abort_transaction(trans, error)		\
 do {								\
-	bool first = false;					\
+	bool __first = false;					\
 	/* Report first abort since mount */			\
 	if (!test_and_set_bit(BTRFS_FS_STATE_TRANS_ABORTED,	\
 			&((trans)->fs_info->fs_state))) {	\
-		first = true;					\
+		__first = true;					\
 		if (WARN(abort_should_print_stack(error),	\
 			KERN_ERR				\
 			"BTRFS: Transaction aborted (error %d)\n",	\
@@ -246,7 +246,7 @@ do {								\
 		}						\
 	}							\
 	__btrfs_abort_transaction((trans), __func__,		\
-				  __LINE__, (error), first);	\
+				  __LINE__, (error), __first);	\
 } while (0)
 
 int btrfs_end_transaction(struct btrfs_trans_handle *trans);
-- 
2.45.0

[PATCH 3/6] btrfs: use for-local variabls that shadow function variables

[David Sterba]

We've started to use for-loop local variables and in a few places this
shadows a function variable. Convert a few cases reported by 'make W=2'.
If applicable also change the style to post-increment, that's the
preferred one.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/qgroup.c  | 11 +++++------
 fs/btrfs/volumes.c |  9 +++------
 fs/btrfs/zoned.c   |  8 +++-----
 3 files changed, 11 insertions(+), 17 deletions(-)

diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index fc2a7ea26354..a94a5b87b042 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -3216,7 +3216,6 @@ int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans, u64 srcid,
 			 struct btrfs_qgroup_inherit *inherit)
 {
 	int ret = 0;
-	int i;
 	u64 *i_qgroups;
 	bool committing = false;
 	struct btrfs_fs_info *fs_info = trans->fs_info;
@@ -3273,7 +3272,7 @@ int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans, u64 srcid,
 		i_qgroups = (u64 *)(inherit + 1);
 		nums = inherit->num_qgroups + 2 * inherit->num_ref_copies +
 		       2 * inherit->num_excl_copies;
-		for (i = 0; i < nums; ++i) {
+		for (int i = 0; i < nums; i++) {
 			srcgroup = find_qgroup_rb(fs_info, *i_qgroups);
 
 			/*
@@ -3300,7 +3299,7 @@ int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans, u64 srcid,
 	 */
 	if (inherit) {
 		i_qgroups = (u64 *)(inherit + 1);
-		for (i = 0; i < inherit->num_qgroups; ++i, ++i_qgroups) {
+		for (int i = 0; i < inherit->num_qgroups; i++, i_qgroups++) {
 			if (*i_qgroups == 0)
 				continue;
 			ret = add_qgroup_relation_item(trans, objectid,
@@ -3386,7 +3385,7 @@ int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans, u64 srcid,
 		goto unlock;
 
 	i_qgroups = (u64 *)(inherit + 1);
-	for (i = 0; i < inherit->num_qgroups; ++i) {
+	for (int i = 0; i < inherit->num_qgroups; i++) {
 		if (*i_qgroups) {
 			ret = add_relation_rb(fs_info, qlist_prealloc[i], objectid,
 					      *i_qgroups);
@@ -3406,7 +3405,7 @@ int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans, u64 srcid,
 		++i_qgroups;
 	}
 
-	for (i = 0; i <  inherit->num_ref_copies; ++i, i_qgroups += 2) {
+	for (int i = 0; i < inherit->num_ref_copies; i++, i_qgroups += 2) {
 		struct btrfs_qgroup *src;
 		struct btrfs_qgroup *dst;
 
@@ -3427,7 +3426,7 @@ int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans, u64 srcid,
 		/* Manually tweaking numbers certainly needs a rescan */
 		need_rescan = true;
 	}
-	for (i = 0; i <  inherit->num_excl_copies; ++i, i_qgroups += 2) {
+	for (int i = 0; i <  inherit->num_excl_copies; i++, i_qgroups += 2) {
 		struct btrfs_qgroup *src;
 		struct btrfs_qgroup *dst;
 
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 7c9d68b1ba69..3f70f727dacf 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -5623,8 +5623,6 @@ static struct btrfs_block_group *create_chunk(struct btrfs_trans_handle *trans,
 	u64 start = ctl->start;
 	u64 type = ctl->type;
 	int ret;
-	int i;
-	int j;
 
 	map = btrfs_alloc_chunk_map(ctl->num_stripes, GFP_NOFS);
 	if (!map)
@@ -5639,8 +5637,8 @@ static struct btrfs_block_group *create_chunk(struct btrfs_trans_handle *trans,
 	map->sub_stripes = ctl->sub_stripes;
 	map->num_stripes = ctl->num_stripes;
 
-	for (i = 0; i < ctl->ndevs; ++i) {
-		for (j = 0; j < ctl->dev_stripes; ++j) {
+	for (int i = 0; i < ctl->ndevs; i++) {
+		for (int j = 0; j < ctl->dev_stripes; j++) {
 			int s = i * ctl->dev_stripes + j;
 			map->stripes[s].dev = devices_info[i].dev;
 			map->stripes[s].physical = devices_info[i].dev_offset +
@@ -6618,7 +6616,6 @@ int btrfs_map_block(struct btrfs_fs_info *fs_info, enum btrfs_map_op op,
 	struct btrfs_chunk_map *map;
 	struct btrfs_io_geometry io_geom = { 0 };
 	u64 map_offset;
-	int i;
 	int ret = 0;
 	int num_copies;
 	struct btrfs_io_context *bioc = NULL;
@@ -6764,7 +6761,7 @@ int btrfs_map_block(struct btrfs_fs_info *fs_info, enum btrfs_map_op op,
 		 * For all other non-RAID56 profiles, just copy the target
 		 * stripe into the bioc.
 		 */
-		for (i = 0; i < io_geom.num_stripes; i++) {
+		for (int i = 0; i < io_geom.num_stripes; i++) {
 			ret = set_io_stripe(fs_info, logical, length,
 					    &bioc->stripes[i], map, &io_geom);
 			if (ret < 0)
diff --git a/fs/btrfs/zoned.c b/fs/btrfs/zoned.c
index dde4a0a34037..e9087264f3e3 100644
--- a/fs/btrfs/zoned.c
+++ b/fs/btrfs/zoned.c
@@ -87,9 +87,8 @@ static int sb_write_pointer(struct block_device *bdev, struct blk_zone *zones,
 	bool empty[BTRFS_NR_SB_LOG_ZONES];
 	bool full[BTRFS_NR_SB_LOG_ZONES];
 	sector_t sector;
-	int i;
 
-	for (i = 0; i < BTRFS_NR_SB_LOG_ZONES; i++) {
+	for (int i = 0; i < BTRFS_NR_SB_LOG_ZONES; i++) {
 		ASSERT(zones[i].type != BLK_ZONE_TYPE_CONVENTIONAL);
 		empty[i] = (zones[i].cond == BLK_ZONE_COND_EMPTY);
 		full[i] = sb_zone_is_full(&zones[i]);
@@ -121,9 +120,8 @@ static int sb_write_pointer(struct block_device *bdev, struct blk_zone *zones,
 		struct address_space *mapping = bdev->bd_inode->i_mapping;
 		struct page *page[BTRFS_NR_SB_LOG_ZONES];
 		struct btrfs_super_block *super[BTRFS_NR_SB_LOG_ZONES];
-		int i;
 
-		for (i = 0; i < BTRFS_NR_SB_LOG_ZONES; i++) {
+		for (int i = 0; i < BTRFS_NR_SB_LOG_ZONES; i++) {
 			u64 zone_end = (zones[i].start + zones[i].capacity) << SECTOR_SHIFT;
 			u64 bytenr = ALIGN_DOWN(zone_end, BTRFS_SUPER_INFO_SIZE) -
 						BTRFS_SUPER_INFO_SIZE;
@@ -144,7 +142,7 @@ static int sb_write_pointer(struct block_device *bdev, struct blk_zone *zones,
 		else
 			sector = zones[0].start;
 
-		for (i = 0; i < BTRFS_NR_SB_LOG_ZONES; i++)
+		for (int i = 0; i < BTRFS_NR_SB_LOG_ZONES; i++)
 			btrfs_release_disk_super(super[i]);
 	} else if (!full[0] && (empty[1] || full[1])) {
 		sector = zones[0].wp;
-- 
2.45.0

[PATCH 4/6] btrfs: remove unused define EXTENT_SIZE_PER_ITEM

[David Sterba]

This was added  in c61a16a701a126 ("Btrfs: fix the confusion between
delalloc bytes and metadata bytes") and removed in 03fe78cc2942c5
("btrfs: use delalloc_bytes to determine flush amount for
shrink_delalloc") where the calculation was reworked to use a
non-constant numbers. This was found by 'make W=2'.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/space-info.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/fs/btrfs/space-info.c b/fs/btrfs/space-info.c
index d620323d08ea..411b95601f18 100644
--- a/fs/btrfs/space-info.c
+++ b/fs/btrfs/space-info.c
@@ -587,8 +587,6 @@ static inline u64 calc_reclaim_items_nr(const struct btrfs_fs_info *fs_info,
 	return nr;
 }
 
-#define EXTENT_SIZE_PER_ITEM	SZ_256K
-
 /*
  * shrink metadata reservation for delalloc
  */
-- 
2.45.0

[PATCH 5/6] btrfs: keep const whene returnin value from get_unaligned_le8()

[David Sterba]

This was reported by -Wcast-qual, the get_unaligned_le8() simply returns
the arugment and there's no reason to drop the cast.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/accessors.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/btrfs/accessors.h b/fs/btrfs/accessors.h
index 6fce3e8d3dac..c60f0e7f768a 100644
--- a/fs/btrfs/accessors.h
+++ b/fs/btrfs/accessors.h
@@ -34,7 +34,7 @@ void btrfs_init_map_token(struct btrfs_map_token *token, struct extent_buffer *e
 
 static inline u8 get_unaligned_le8(const void *p)
 {
-       return *(u8 *)p;
+       return *(const u8 *)p;
 }
 
 static inline void put_unaligned_le8(u8 val, void *p)
-- 
2.45.0

[PATCH 6/6] btrfs: constify parameters of write_eb_member() and its users

[David Sterba]

Reported by '-Wcast-qual', the argument from which write_extent_buffer()
reads data to write to the eb should be const. In addition the const
needs to be also added to __write_extent_buffer() local buffers.

All callers of write_eb_member() can now be updated to use const for the
input buffer structure or type.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/accessors.h | 10 +++++-----
 fs/btrfs/extent_io.c |  2 +-
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/fs/btrfs/accessors.h b/fs/btrfs/accessors.h
index c60f0e7f768a..6c3deaa3e878 100644
--- a/fs/btrfs/accessors.h
+++ b/fs/btrfs/accessors.h
@@ -48,8 +48,8 @@ static inline void put_unaligned_le8(u8 val, void *p)
 			    offsetof(type, member),			\
 			    sizeof_field(type, member)))
 
-#define write_eb_member(eb, ptr, type, member, result) (\
-	write_extent_buffer(eb, (char *)(result),			\
+#define write_eb_member(eb, ptr, type, member, source) (		\
+	write_extent_buffer(eb, (const char *)(source),			\
 			   ((unsigned long)(ptr)) +			\
 			    offsetof(type, member),			\
 			    sizeof_field(type, member)))
@@ -353,7 +353,7 @@ static inline void btrfs_tree_block_key(const struct extent_buffer *eb,
 
 static inline void btrfs_set_tree_block_key(const struct extent_buffer *eb,
 					    struct btrfs_tree_block_info *item,
-					    struct btrfs_disk_key *key)
+					    const struct btrfs_disk_key *key)
 {
 	write_eb_member(eb, item, struct btrfs_tree_block_info, key, key);
 }
@@ -446,7 +446,7 @@ void btrfs_node_key(const struct extent_buffer *eb,
 		    struct btrfs_disk_key *disk_key, int nr);
 
 static inline void btrfs_set_node_key(const struct extent_buffer *eb,
-				      struct btrfs_disk_key *disk_key, int nr)
+				      const struct btrfs_disk_key *disk_key, int nr)
 {
 	unsigned long ptr;
 
@@ -512,7 +512,7 @@ static inline void btrfs_item_key(const struct extent_buffer *eb,
 }
 
 static inline void btrfs_set_item_key(struct extent_buffer *eb,
-				      struct btrfs_disk_key *disk_key, int nr)
+				      const struct btrfs_disk_key *disk_key, int nr)
 {
 	struct btrfs_item *item = btrfs_item_nr(eb, nr);
 
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 2d773c1cbaa7..bf50301ee528 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -4604,7 +4604,7 @@ static void __write_extent_buffer(const struct extent_buffer *eb,
 	size_t cur;
 	size_t offset;
 	char *kaddr;
-	char *src = (char *)srcv;
+	const char *src = (const char *)srcv;
 	unsigned long i = get_eb_folio_index(eb, start);
 	/* For unmapped (dummy) ebs, no need to check their uptodate status. */
 	const bool check_uptodate = !test_bit(EXTENT_BUFFER_UNMAPPED, &eb->bflags);
-- 
2.45.0

Re: [PATCH 0/6] Cleanups and W=2 warning fixes

[Boris Burkov]

On Mon, May 20, 2024 at 09:52:08PM +0200, David Sterba wrote:
> We have a clean run of 'make W='1 with gcc 13, there are some
> interesting warnings to fix with level 2 and even 3. We can't enable the
> warning flags by defualt due to reports from generic code.
> 
> This short series removes shadowed variables, adds const and removes
> an unused macro. There are still some shadow variables to fix but the
> remaining cases are with 'ret' variables so I skipped it for now.

These LGTM. I did notice a few typos in the patch titles, though.
Reviewed-by: Boris Burkov <boris@bur.io>

> 
> David Sterba (6):
>   btrfs: remove duplicate name variable declarations
>   btrfs: rename macro local variables that clash with other variables
>   btrfs: use for-local variabls that shadow function variables
btrfs: use for-local variables that shadow function variables
>   btrfs: remove unused define EXTENT_SIZE_PER_ITEM
>   btrfs: keep const whene returnin value from get_unaligned_le8()
btrfs: keep const when returning value from get_unaligned_le8()
>   btrfs: constify parameters of write_eb_member() and its users
> 
>  fs/btrfs/accessors.h   | 12 ++++++------
>  fs/btrfs/extent_io.c   |  6 ++----
>  fs/btrfs/inode.c       |  2 --
>  fs/btrfs/qgroup.c      | 11 +++++------
>  fs/btrfs/space-info.c  |  2 --
>  fs/btrfs/subpage.c     |  8 ++++----
>  fs/btrfs/transaction.h |  6 +++---
>  fs/btrfs/volumes.c     |  9 +++------
>  fs/btrfs/zoned.c       |  8 +++-----
>  9 files changed, 26 insertions(+), 38 deletions(-)
> 
> -- 
> 2.45.0
> 

Re: [PATCH 0/6] Cleanups and W=2 warning fixes

[Anand Jain]

On 5/21/24 03:52, David Sterba wrote:
> We have a clean run of 'make W='1 with gcc 13, there are some
> interesting warnings to fix with level 2 and even 3. We can't enable the
> warning flags by defualt due to reports from generic code.
> 
> This short series removes shadowed variables, adds const and removes
> an unused macro. There are still some shadow variables to fix but the
> remaining cases are with 'ret' variables so I skipped it for now.
> 
> David Sterba (6):
>    btrfs: remove duplicate name variable declarations
>    btrfs: rename macro local variables that clash with other variables
>    btrfs: use for-local variabls that shadow function variables
>    btrfs: remove unused define EXTENT_SIZE_PER_ITEM
>    btrfs: keep const whene returnin value from get_unaligned_le8()
>    btrfs: constify parameters of write_eb_member() and its users


Reviewed-by: Anand Jain <anand.jain@oracle.com>


Thanks, Anand

> 
>   fs/btrfs/accessors.h   | 12 ++++++------
>   fs/btrfs/extent_io.c   |  6 ++----
>   fs/btrfs/inode.c       |  2 --
>   fs/btrfs/qgroup.c      | 11 +++++------
>   fs/btrfs/space-info.c  |  2 --
>   fs/btrfs/subpage.c     |  8 ++++----
>   fs/btrfs/transaction.h |  6 +++---
>   fs/btrfs/volumes.c     |  9 +++------
>   fs/btrfs/zoned.c       |  8 +++-----
>   9 files changed, 26 insertions(+), 38 deletions(-)
> 

Re: [PATCH 3/6] btrfs: use for-local variabls that shadow function variables

[Naohiro Aota]

On Mon, May 20, 2024 at 09:52:26PM GMT, David Sterba wrote:
> We've started to use for-loop local variables and in a few places this
> shadows a function variable. Convert a few cases reported by 'make W=2'.
> If applicable also change the style to post-increment, that's the
> preferred one.
> 
> Signed-off-by: David Sterba <dsterba@suse.com>
> ---

LGTM asides from a small nit below.

Reviewed-by: Naohiro Aota <naohiro.aota@wdc.com>

>  fs/btrfs/qgroup.c  | 11 +++++------
>  fs/btrfs/volumes.c |  9 +++------
>  fs/btrfs/zoned.c   |  8 +++-----
>  3 files changed, 11 insertions(+), 17 deletions(-)
> 
> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
> index fc2a7ea26354..a94a5b87b042 100644
> --- a/fs/btrfs/qgroup.c
> +++ b/fs/btrfs/qgroup.c
> @@ -3216,7 +3216,6 @@ int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans, u64 srcid,
>  			 struct btrfs_qgroup_inherit *inherit)
>  {
>  	int ret = 0;
> -	int i;
>  	u64 *i_qgroups;
>  	bool committing = false;
>  	struct btrfs_fs_info *fs_info = trans->fs_info;
> @@ -3273,7 +3272,7 @@ int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans, u64 srcid,
>  		i_qgroups = (u64 *)(inherit + 1);
>  		nums = inherit->num_qgroups + 2 * inherit->num_ref_copies +
>  		       2 * inherit->num_excl_copies;
> -		for (i = 0; i < nums; ++i) {
> +		for (int i = 0; i < nums; i++) {
>  			srcgroup = find_qgroup_rb(fs_info, *i_qgroups);
>  
>  			/*
> @@ -3300,7 +3299,7 @@ int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans, u64 srcid,
>  	 */
>  	if (inherit) {
>  		i_qgroups = (u64 *)(inherit + 1);
> -		for (i = 0; i < inherit->num_qgroups; ++i, ++i_qgroups) {
> +		for (int i = 0; i < inherit->num_qgroups; i++, i_qgroups++) {
>  			if (*i_qgroups == 0)
>  				continue;
>  			ret = add_qgroup_relation_item(trans, objectid,
> @@ -3386,7 +3385,7 @@ int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans, u64 srcid,
>  		goto unlock;
>  
>  	i_qgroups = (u64 *)(inherit + 1);
> -	for (i = 0; i < inherit->num_qgroups; ++i) {
> +	for (int i = 0; i < inherit->num_qgroups; i++) {
>  		if (*i_qgroups) {
>  			ret = add_relation_rb(fs_info, qlist_prealloc[i], objectid,
>  					      *i_qgroups);
> @@ -3406,7 +3405,7 @@ int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans, u64 srcid,
>  		++i_qgroups;
>  	}
>  
> -	for (i = 0; i <  inherit->num_ref_copies; ++i, i_qgroups += 2) {
> +	for (int i = 0; i < inherit->num_ref_copies; i++, i_qgroups += 2) {
>  		struct btrfs_qgroup *src;
>  		struct btrfs_qgroup *dst;
>  
> @@ -3427,7 +3426,7 @@ int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans, u64 srcid,
>  		/* Manually tweaking numbers certainly needs a rescan */
>  		need_rescan = true;
>  	}
> -	for (i = 0; i <  inherit->num_excl_copies; ++i, i_qgroups += 2) {
> +	for (int i = 0; i <  inherit->num_excl_copies; i++, i_qgroups += 2) {
                           ^
nit:                       we have double space here for no reason.
Can we just dedup it as well?

>  		struct btrfs_qgroup *src;
>  		struct btrfs_qgroup *dst;
>  
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 7c9d68b1ba69..3f70f727dacf 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -5623,8 +5623,6 @@ static struct btrfs_block_group *create_chunk(struct btrfs_trans_handle *trans,
>  	u64 start = ctl->start;
>  	u64 type = ctl->type;
>  	int ret;
> -	int i;
> -	int j;
>  
>  	map = btrfs_alloc_chunk_map(ctl->num_stripes, GFP_NOFS);
>  	if (!map)
> @@ -5639,8 +5637,8 @@ static struct btrfs_block_group *create_chunk(struct btrfs_trans_handle *trans,
>  	map->sub_stripes = ctl->sub_stripes;
>  	map->num_stripes = ctl->num_stripes;
>  
> -	for (i = 0; i < ctl->ndevs; ++i) {
> -		for (j = 0; j < ctl->dev_stripes; ++j) {
> +	for (int i = 0; i < ctl->ndevs; i++) {
> +		for (int j = 0; j < ctl->dev_stripes; j++) {
>  			int s = i * ctl->dev_stripes + j;
>  			map->stripes[s].dev = devices_info[i].dev;
>  			map->stripes[s].physical = devices_info[i].dev_offset +
> @@ -6618,7 +6616,6 @@ int btrfs_map_block(struct btrfs_fs_info *fs_info, enum btrfs_map_op op,
>  	struct btrfs_chunk_map *map;
>  	struct btrfs_io_geometry io_geom = { 0 };
>  	u64 map_offset;
> -	int i;
>  	int ret = 0;
>  	int num_copies;
>  	struct btrfs_io_context *bioc = NULL;
> @@ -6764,7 +6761,7 @@ int btrfs_map_block(struct btrfs_fs_info *fs_info, enum btrfs_map_op op,
>  		 * For all other non-RAID56 profiles, just copy the target
>  		 * stripe into the bioc.
>  		 */
> -		for (i = 0; i < io_geom.num_stripes; i++) {
> +		for (int i = 0; i < io_geom.num_stripes; i++) {
>  			ret = set_io_stripe(fs_info, logical, length,
>  					    &bioc->stripes[i], map, &io_geom);
>  			if (ret < 0)
> diff --git a/fs/btrfs/zoned.c b/fs/btrfs/zoned.c
> index dde4a0a34037..e9087264f3e3 100644
> --- a/fs/btrfs/zoned.c
> +++ b/fs/btrfs/zoned.c
> @@ -87,9 +87,8 @@ static int sb_write_pointer(struct block_device *bdev, struct blk_zone *zones,
>  	bool empty[BTRFS_NR_SB_LOG_ZONES];
>  	bool full[BTRFS_NR_SB_LOG_ZONES];
>  	sector_t sector;
> -	int i;
>  
> -	for (i = 0; i < BTRFS_NR_SB_LOG_ZONES; i++) {
> +	for (int i = 0; i < BTRFS_NR_SB_LOG_ZONES; i++) {
>  		ASSERT(zones[i].type != BLK_ZONE_TYPE_CONVENTIONAL);
>  		empty[i] = (zones[i].cond == BLK_ZONE_COND_EMPTY);
>  		full[i] = sb_zone_is_full(&zones[i]);
> @@ -121,9 +120,8 @@ static int sb_write_pointer(struct block_device *bdev, struct blk_zone *zones,
>  		struct address_space *mapping = bdev->bd_inode->i_mapping;
>  		struct page *page[BTRFS_NR_SB_LOG_ZONES];
>  		struct btrfs_super_block *super[BTRFS_NR_SB_LOG_ZONES];
> -		int i;
>  
> -		for (i = 0; i < BTRFS_NR_SB_LOG_ZONES; i++) {
> +		for (int i = 0; i < BTRFS_NR_SB_LOG_ZONES; i++) {
>  			u64 zone_end = (zones[i].start + zones[i].capacity) << SECTOR_SHIFT;
>  			u64 bytenr = ALIGN_DOWN(zone_end, BTRFS_SUPER_INFO_SIZE) -
>  						BTRFS_SUPER_INFO_SIZE;
> @@ -144,7 +142,7 @@ static int sb_write_pointer(struct block_device *bdev, struct blk_zone *zones,
>  		else
>  			sector = zones[0].start;
>  
> -		for (i = 0; i < BTRFS_NR_SB_LOG_ZONES; i++)
> +		for (int i = 0; i < BTRFS_NR_SB_LOG_ZONES; i++)
>  			btrfs_release_disk_super(super[i]);
>  	} else if (!full[0] && (empty[1] || full[1])) {
>  		sector = zones[0].wp;
> -- 
> 2.45.0
> 

Re: [PATCH 0/6] Cleanups and W=2 warning fixes

[Naohiro Aota]

On Mon, May 20, 2024 at 09:52:08PM GMT, David Sterba wrote:
> We have a clean run of 'make W='1 with gcc 13, there are some
> interesting warnings to fix with level 2 and even 3. We can't enable the
> warning flags by defualt due to reports from generic code.
> 
> This short series removes shadowed variables, adds const and removes
> an unused macro. There are still some shadow variables to fix but the
> remaining cases are with 'ret' variables so I skipped it for now.
> 
> David Sterba (6):
>   btrfs: remove duplicate name variable declarations
>   btrfs: rename macro local variables that clash with other variables
>   btrfs: use for-local variabls that shadow function variables
>   btrfs: remove unused define EXTENT_SIZE_PER_ITEM
>   btrfs: keep const whene returnin value from get_unaligned_le8()
>   btrfs: constify parameters of write_eb_member() and its users

A small nit added to patch 3 but for whole the series

Reviewed-by: Naohiro Aota <naohiro.aota@wdc.com>

>  fs/btrfs/accessors.h   | 12 ++++++------
>  fs/btrfs/extent_io.c   |  6 ++----
>  fs/btrfs/inode.c       |  2 --
>  fs/btrfs/qgroup.c      | 11 +++++------
>  fs/btrfs/space-info.c  |  2 --
>  fs/btrfs/subpage.c     |  8 ++++----
>  fs/btrfs/transaction.h |  6 +++---
>  fs/btrfs/volumes.c     |  9 +++------
>  fs/btrfs/zoned.c       |  8 +++-----
>  9 files changed, 26 insertions(+), 38 deletions(-)
> 
> -- 
> 2.45.0
> 

Re: [PATCH 3/6] btrfs: use for-local variabls that shadow function variables

[David Sterba]

On Tue, May 21, 2024 at 04:13:53AM +0000, Naohiro Aota wrote:
> On Mon, May 20, 2024 at 09:52:26PM GMT, David Sterba wrote:
> > We've started to use for-loop local variables and in a few places this
> > shadows a function variable. Convert a few cases reported by 'make W=2'.
> > If applicable also change the style to post-increment, that's the
> > preferred one.
> > 
> > Signed-off-by: David Sterba <dsterba@suse.com>
> > ---
> 
> LGTM asides from a small nit below.
> 
> Reviewed-by: Naohiro Aota <naohiro.aota@wdc.com>
> 
> >  fs/btrfs/qgroup.c  | 11 +++++------
> >  fs/btrfs/volumes.c |  9 +++------
> >  fs/btrfs/zoned.c   |  8 +++-----
> >  3 files changed, 11 insertions(+), 17 deletions(-)
> > 
> > diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
> > index fc2a7ea26354..a94a5b87b042 100644
> > --- a/fs/btrfs/qgroup.c
> > +++ b/fs/btrfs/qgroup.c
> > @@ -3216,7 +3216,6 @@ int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans, u64 srcid,
> >  			 struct btrfs_qgroup_inherit *inherit)
> >  {
> >  	int ret = 0;
> > -	int i;
> >  	u64 *i_qgroups;
> >  	bool committing = false;
> >  	struct btrfs_fs_info *fs_info = trans->fs_info;
> > @@ -3273,7 +3272,7 @@ int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans, u64 srcid,
> >  		i_qgroups = (u64 *)(inherit + 1);
> >  		nums = inherit->num_qgroups + 2 * inherit->num_ref_copies +
> >  		       2 * inherit->num_excl_copies;
> > -		for (i = 0; i < nums; ++i) {
> > +		for (int i = 0; i < nums; i++) {
> >  			srcgroup = find_qgroup_rb(fs_info, *i_qgroups);
> >  
> >  			/*
> > @@ -3300,7 +3299,7 @@ int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans, u64 srcid,
> >  	 */
> >  	if (inherit) {
> >  		i_qgroups = (u64 *)(inherit + 1);
> > -		for (i = 0; i < inherit->num_qgroups; ++i, ++i_qgroups) {
> > +		for (int i = 0; i < inherit->num_qgroups; i++, i_qgroups++) {
> >  			if (*i_qgroups == 0)
> >  				continue;
> >  			ret = add_qgroup_relation_item(trans, objectid,
> > @@ -3386,7 +3385,7 @@ int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans, u64 srcid,
> >  		goto unlock;
> >  
> >  	i_qgroups = (u64 *)(inherit + 1);
> > -	for (i = 0; i < inherit->num_qgroups; ++i) {
> > +	for (int i = 0; i < inherit->num_qgroups; i++) {
> >  		if (*i_qgroups) {
> >  			ret = add_relation_rb(fs_info, qlist_prealloc[i], objectid,
> >  					      *i_qgroups);
> > @@ -3406,7 +3405,7 @@ int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans, u64 srcid,
> >  		++i_qgroups;
> >  	}
> >  
> > -	for (i = 0; i <  inherit->num_ref_copies; ++i, i_qgroups += 2) {
> > +	for (int i = 0; i < inherit->num_ref_copies; i++, i_qgroups += 2) {
> >  		struct btrfs_qgroup *src;
> >  		struct btrfs_qgroup *dst;
> >  
> > @@ -3427,7 +3426,7 @@ int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans, u64 srcid,
> >  		/* Manually tweaking numbers certainly needs a rescan */
> >  		need_rescan = true;
> >  	}
> > -	for (i = 0; i <  inherit->num_excl_copies; ++i, i_qgroups += 2) {
> > +	for (int i = 0; i <  inherit->num_excl_copies; i++, i_qgroups += 2) {
>                            ^
> nit:                       we have double space here for no reason.
> Can we just dedup it as well?

I remember removing it but probably forgot to refresh the patch before
sending.