[PATCH 0/8] Fixup uninitialized warnings and enable extra checks

[PATCH 0/8] Fixup uninitialized warnings and enable extra checks

[Josef Bacik]

Hello,

We had been failing the raid56 related scrub tests on our overnight tests since
November.  Initially I asked Qu to look into these as I didn't have time to dig
in, and he was unable to reproduce.  I assumed it was some oddity in my setup,
so I ignored it.  However recently I got a report that I regressed some of these
tests with an unrelated change.  When debugging it I found it was because of an
uninitialized return value, which would have been caught by more modern gcc's
with -Wmaybe-uninitialized.

In order to avoid these sort of problems in the future lets fix up all the false
positivies that this warning brings, and then enable the option for btrfs so we
can avoid this style of failure in the future.  Thanks,

Josef

Josef Bacik (8):
  btrfs: fix uninit warning in run_one_async_start
  btrfs: fix uninit warning in btrfs_cleanup_ordered_extents
  btrfs: fix uninit warning from get_inode_gen usage
  btrfs: fix uninit warning in btrfs_update_block_group
  btrfs: fix uninit warning in __set_extent_bit and convert_extent_bit
  btrfs: extract out zone cache usage into it's own helper
  btrfs: fix uninit warning in btrfs_sb_log_location
  btrfs: turn on -Wmaybe-uninitialized

 fs/btrfs/Makefile         |  1 +
 fs/btrfs/block-group.c    |  2 +-
 fs/btrfs/disk-io.c        |  2 +-
 fs/btrfs/extent-io-tree.c |  8 ++---
 fs/btrfs/inode.c          |  2 +-
 fs/btrfs/send.c           |  8 ++---
 fs/btrfs/zoned.c          | 75 ++++++++++++++++++++++++---------------
 7 files changed, 57 insertions(+), 41 deletions(-)

-- 
2.26.3

[PATCH 1/8] btrfs: fix uninit warning in run_one_async_start

[Josef Bacik]

With -Wmaybe-uninitialized complains about ret being possibly
uninitialized, which isn't possible, however we can init the value to
get rid of the warning.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/disk-io.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 0888d484df80..c25b444027d6 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -693,7 +693,7 @@ int btrfs_validate_metadata_buffer(struct btrfs_bio *bbio,
 static void run_one_async_start(struct btrfs_work *work)
 {
 	struct async_submit_bio *async;
-	blk_status_t ret;
+	blk_status_t ret = BLK_STS_OK;
 
 	async = container_of(work, struct  async_submit_bio, work);
 	switch (async->submit_cmd) {
-- 
2.26.3

[PATCH 2/8] btrfs: fix uninit warning in btrfs_cleanup_ordered_extents

[Josef Bacik]

We can conditionally pass in a locked page, and then we'll use that page
range to skip marking errors as that will happen in another layer.
However this causes the compiler to complain because it doesn't
understand we only use these values when we have the page.  Make the
compiler stop complaining by setting these values to 0.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/inode.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 905ea19df125..dfceaf79d5d4 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -228,7 +228,7 @@ static inline void btrfs_cleanup_ordered_extents(struct btrfs_inode *inode,
 {
 	unsigned long index = offset >> PAGE_SHIFT;
 	unsigned long end_index = (offset + bytes - 1) >> PAGE_SHIFT;
-	u64 page_start, page_end;
+	u64 page_start = 0, page_end = 0;
 	struct page *page;
 
 	if (locked_page) {
-- 
2.26.3

[PATCH 3/8] btrfs: fix uninit warning from get_inode_gen usage

[Josef Bacik]

Anybody that calls get_inode_gen() can have an uninitialized gen if
there's an error.  This isn't a big deal because all the users just exit
if they get an error, however it makes -Wmaybe-uninitialized complain,
so fix this up to always init the passed in gen, this quiets all of the
uninitialized warnings in send.c.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/send.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c
index 67f7c698ade3..25a235179edb 100644
--- a/fs/btrfs/send.c
+++ b/fs/btrfs/send.c
@@ -955,14 +955,12 @@ static int get_inode_info(struct btrfs_root *root, u64 ino,
 static int get_inode_gen(struct btrfs_root *root, u64 ino, u64 *gen)
 {
 	int ret;
-	struct btrfs_inode_info info;
+	struct btrfs_inode_info info = {};
 
-	if (!gen)
-		return -EPERM;
+	ASSERT(gen);
 
 	ret = get_inode_info(root, ino, &info);
-	if (!ret)
-		*gen = info.gen;
+	*gen = info.gen;
 	return ret;
 }
 
-- 
2.26.3

[PATCH 4/8] btrfs: fix uninit warning in btrfs_update_block_group

[Josef Bacik]

reclaim isn't set in the alloc case, however we only care about reclaim
in the !alloc case.  This isn't an actual problem, however
-Wmaybe-uninitialized will complain, so initialize reclaim to quiet the
compiler.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/block-group.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c
index 708d843daa72..e90800388a41 100644
--- a/fs/btrfs/block-group.c
+++ b/fs/btrfs/block-group.c
@@ -3330,7 +3330,7 @@ int btrfs_update_block_group(struct btrfs_trans_handle *trans,
 	spin_unlock(&info->delalloc_root_lock);
 
 	while (total) {
-		bool reclaim;
+		bool reclaim = false;
 
 		cache = btrfs_lookup_block_group(info, bytenr);
 		if (!cache) {
-- 
2.26.3

[PATCH 5/8] btrfs: fix uninit warning in __set_extent_bit and convert_extent_bit

[Josef Bacik]

We will pass in the parent and p pointer into our tree_search function
to avoid doing a second search when inserting a new extent state into
the tree.  However because this is conditional upon passing in these
pointers the compiler seems to think these values can be uninitialized
if we're using -Wmaybe-uninitialized.  Fix this by initializing these
values.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/extent-io-tree.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/fs/btrfs/extent-io-tree.c b/fs/btrfs/extent-io-tree.c
index 9ae9cd1e7035..9e1f18706a02 100644
--- a/fs/btrfs/extent-io-tree.c
+++ b/fs/btrfs/extent-io-tree.c
@@ -972,8 +972,8 @@ static int __set_extent_bit(struct extent_io_tree *tree, u64 start, u64 end,
 {
 	struct extent_state *state;
 	struct extent_state *prealloc = NULL;
-	struct rb_node **p;
-	struct rb_node *parent;
+	struct rb_node **p = NULL;
+	struct rb_node *parent = NULL;
 	int err = 0;
 	u64 last_start;
 	u64 last_end;
@@ -1218,8 +1218,8 @@ int convert_extent_bit(struct extent_io_tree *tree, u64 start, u64 end,
 {
 	struct extent_state *state;
 	struct extent_state *prealloc = NULL;
-	struct rb_node **p;
-	struct rb_node *parent;
+	struct rb_node **p = NULL;
+	struct rb_node *parent = NULL;
 	int err = 0;
 	u64 last_start;
 	u64 last_end;
-- 
2.26.3

[PATCH 6/8] btrfs: extract out zone cache usage into it's own helper

[Josef Bacik]

There's a special case for loading the device zone info if we have the
zone cache which is a fair bit of code.  Extract this out into it's own
helper to clean up the code a little bit, and as a side effect it fixes
an uninitialized error we get with -Wmaybe-uninitialized where it
thought zno may have been uninitialized.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/zoned.c | 73 +++++++++++++++++++++++++++++-------------------
 1 file changed, 45 insertions(+), 28 deletions(-)

diff --git a/fs/btrfs/zoned.c b/fs/btrfs/zoned.c
index a759668477bb..f3640ab95e5e 100644
--- a/fs/btrfs/zoned.c
+++ b/fs/btrfs/zoned.c
@@ -216,11 +216,46 @@ static int emulate_report_zones(struct btrfs_device *device, u64 pos,
 	return i;
 }
 
+static int load_zones_from_cache(struct btrfs_zoned_device_info *zinfo, u64 pos,
+				 struct blk_zone *zones, unsigned int *nr_zones)
+{
+	unsigned int i;
+	u32 zno;
+
+	if (!zinfo->zone_cache)
+		return -ENOENT;
+
+	ASSERT(IS_ALIGNED(pos, zinfo->zone_size));
+	zno = pos >> zinfo->zone_size_shift;
+
+	/*
+	 * We cannot report zones beyond the zone end. So, it is OK to
+	 * cap *nr_zones to at the end.
+	 */
+	*nr_zones = min_t(u32, *nr_zones, zinfo->nr_zones - zno);
+
+	for (i = 0; i < *nr_zones; i++) {
+		struct blk_zone *zone_info;
+
+		zone_info = &zinfo->zone_cache[zno + i];
+		if (!zone_info->len)
+			break;
+	}
+
+	if (i == *nr_zones) {
+		/* Cache hit on all the zones */
+		memcpy(zones, zinfo->zone_cache + zno,
+		       sizeof(*zinfo->zone_cache) * *nr_zones);
+		return 0;
+	}
+
+	return -ENOENT;
+}
+
 static int btrfs_get_dev_zones(struct btrfs_device *device, u64 pos,
 			       struct blk_zone *zones, unsigned int *nr_zones)
 {
 	struct btrfs_zoned_device_info *zinfo = device->zone_info;
-	u32 zno;
 	int ret;
 
 	if (!*nr_zones)
@@ -233,32 +268,8 @@ static int btrfs_get_dev_zones(struct btrfs_device *device, u64 pos,
 	}
 
 	/* Check cache */
-	if (zinfo->zone_cache) {
-		unsigned int i;
-
-		ASSERT(IS_ALIGNED(pos, zinfo->zone_size));
-		zno = pos >> zinfo->zone_size_shift;
-		/*
-		 * We cannot report zones beyond the zone end. So, it is OK to
-		 * cap *nr_zones to at the end.
-		 */
-		*nr_zones = min_t(u32, *nr_zones, zinfo->nr_zones - zno);
-
-		for (i = 0; i < *nr_zones; i++) {
-			struct blk_zone *zone_info;
-
-			zone_info = &zinfo->zone_cache[zno + i];
-			if (!zone_info->len)
-				break;
-		}
-
-		if (i == *nr_zones) {
-			/* Cache hit on all the zones */
-			memcpy(zones, zinfo->zone_cache + zno,
-			       sizeof(*zinfo->zone_cache) * *nr_zones);
-			return 0;
-		}
-	}
+	if (!load_zones_from_cache(zinfo, pos, zones, nr_zones))
+		return 0;
 
 	ret = blkdev_report_zones(device->bdev, pos >> SECTOR_SHIFT, *nr_zones,
 				  copy_zone_info_cb, zones);
@@ -274,9 +285,15 @@ static int btrfs_get_dev_zones(struct btrfs_device *device, u64 pos,
 		return -EIO;
 
 	/* Populate cache */
-	if (zinfo->zone_cache)
+	if (zinfo->zone_cache) {
+		u32 zno;
+
+		ASSERT(IS_ALIGNED(pos, zinfo->zone_size));
+		zno = pos >> zinfo->zone_size_shift;
+
 		memcpy(zinfo->zone_cache + zno, zones,
 		       sizeof(*zinfo->zone_cache) * *nr_zones);
+	}
 
 	return 0;
 }
-- 
2.26.3

[PATCH 7/8] btrfs: fix uninit warning in btrfs_sb_log_location

[Josef Bacik]

We only have 3 possible mirrors, and we have ASSERT()'s to make sure
we're not passing in an invalid super mirror into this function, so
technically this value isn't uninitialized.  However
-Wmaybe-uninitialized will complain, so set it to U64_MAX so if we don't
have ASSERT()'s turned on it'll error out later on when it see's the
zone is beyond our maximum zones.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/zoned.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/btrfs/zoned.c b/fs/btrfs/zoned.c
index f3640ab95e5e..54568735415d 100644
--- a/fs/btrfs/zoned.c
+++ b/fs/btrfs/zoned.c
@@ -160,7 +160,7 @@ static int sb_write_pointer(struct block_device *bdev, struct blk_zone *zones,
  */
 static inline u32 sb_zone_number(int shift, int mirror)
 {
-	u64 zone;
+	u64 zone = U64_MAX;
 
 	ASSERT(mirror < BTRFS_SUPER_MIRROR_MAX);
 	switch (mirror) {
-- 
2.26.3

[PATCH 8/8] btrfs: turn on -Wmaybe-uninitialized

[Josef Bacik]

We had a recent bug that would have been caught by a newer compiler with
-Wmaybe-uninitialized and would have saved us a month of failing tests
that I didn't have time to investigate.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/Makefile | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/btrfs/Makefile b/fs/btrfs/Makefile
index 555c962fdad6..eca995abccdf 100644
--- a/fs/btrfs/Makefile
+++ b/fs/btrfs/Makefile
@@ -7,6 +7,7 @@ subdir-ccflags-y += -Wmissing-format-attribute
 subdir-ccflags-y += -Wmissing-prototypes
 subdir-ccflags-y += -Wold-style-definition
 subdir-ccflags-y += -Wmissing-include-dirs
+subdir-ccflags-y += -Wmaybe-uninitialized
 condflags := \
 	$(call cc-option, -Wunused-but-set-variable)		\
 	$(call cc-option, -Wunused-const-variable)		\
-- 
2.26.3

Re: [PATCH 0/8] Fixup uninitialized warnings and enable extra checks

[Qu Wenruo]

On 2022/12/17 04:15, Josef Bacik wrote:
> Hello,
> 
> We had been failing the raid56 related scrub tests on our overnight tests since
> November.  Initially I asked Qu to look into these as I didn't have time to dig
> in, and he was unable to reproduce.  I assumed it was some oddity in my setup,
> so I ignored it.  However recently I got a report that I regressed some of these
> tests with an unrelated change.  When debugging it I found it was because of an
> uninitialized return value, which would have been caught by more modern gcc's
> with -Wmaybe-uninitialized.

Any clue which patch is fixing the raid0/raid1 scrub failures?

As locally, I found my aarch64/x86_64 VMs are all reporting scrub errors 
for all profiles, including RAID0/RAID1.
(The failure happens after patch "btrfs: do not check header generation 
in btrfs_clean_tree_block").

I didn't notice any of the patches touching the scrub path, or is there 
some hidden paths involved?

Thanks,
Qu

> 
> In order to avoid these sort of problems in the future lets fix up all the false
> positivies that this warning brings, and then enable the option for btrfs so we
> can avoid this style of failure in the future.  Thanks,
> 
> Josef
> 
> Josef Bacik (8):
>    btrfs: fix uninit warning in run_one_async_start
>    btrfs: fix uninit warning in btrfs_cleanup_ordered_extents
>    btrfs: fix uninit warning from get_inode_gen usage
>    btrfs: fix uninit warning in btrfs_update_block_group
>    btrfs: fix uninit warning in __set_extent_bit and convert_extent_bit
>    btrfs: extract out zone cache usage into it's own helper
>    btrfs: fix uninit warning in btrfs_sb_log_location
>    btrfs: turn on -Wmaybe-uninitialized
> 
>   fs/btrfs/Makefile         |  1 +
>   fs/btrfs/block-group.c    |  2 +-
>   fs/btrfs/disk-io.c        |  2 +-
>   fs/btrfs/extent-io-tree.c |  8 ++---
>   fs/btrfs/inode.c          |  2 +-
>   fs/btrfs/send.c           |  8 ++---
>   fs/btrfs/zoned.c          | 75 ++++++++++++++++++++++++---------------
>   7 files changed, 57 insertions(+), 41 deletions(-)
> 

Re: [PATCH 0/8] Fixup uninitialized warnings and enable extra checks

[Qu Wenruo]

On 2022/12/17 07:55, Qu Wenruo wrote:
> 
> 
> On 2022/12/17 04:15, Josef Bacik wrote:
>> Hello,
>>
>> We had been failing the raid56 related scrub tests on our overnight 
>> tests since
>> November.  Initially I asked Qu to look into these as I didn't have 
>> time to dig
>> in, and he was unable to reproduce.  I assumed it was some oddity in 
>> my setup,
>> so I ignored it.  However recently I got a report that I regressed 
>> some of these
>> tests with an unrelated change.  When debugging it I found it was 
>> because of an
>> uninitialized return value, which would have been caught by more 
>> modern gcc's
>> with -Wmaybe-uninitialized.
> 
> Any clue which patch is fixing the raid0/raid1 scrub failures?
> 
> As locally, I found my aarch64/x86_64 VMs are all reporting scrub errors 
> for all profiles, including RAID0/RAID1.
> (The failure happens after patch "btrfs: do not check header generation 
> in btrfs_clean_tree_block").
> 
> I didn't notice any of the patches touching the scrub path, or is there 
> some hidden paths involved?

In fact, it's still reproducible reliably here, with all uninitialized 
fixes applied upon "btrfs: do not check header generation in 
btrfs_clean_tree_block".


btrfs/072 30s ... - output mismatch (see 
/home/adam/xfstests/results//btrfs/072.out.bad)
     --- tests/btrfs/072.out	2022-05-11 09:55:30.736666664 +0800
     +++ /home/adam/xfstests/results//btrfs/072.out.bad	2022-12-17 
08:05:26.750000015 +0800
     @@ -1,2 +1,9 @@
      QA output created by 072
      Silence is golden
     +Scrub find errors in "-m dup -d single" test
     +Scrub find errors in "-m raid0 -d raid0" test
     +Scrub find errors in "-m raid0 -d raid0" test
     +Scrub find errors in "-m raid1 -d raid0" test
     +Scrub find errors in "-m raid10 -d raid10" test
     ...
     (Run 'diff -u /home/adam/xfstests/tests/btrfs/072.out 
/home/adam/xfstests/results//btrfs/072.out.bad'  to see the entire diff)
btrfs/074 32s ... - output mismatch (see 
/home/adam/xfstests/results//btrfs/074.out.bad)
     --- tests/btrfs/074.out	2022-05-11 09:55:30.736666664 +0800
     +++ /home/adam/xfstests/results//btrfs/074.out.bad	2022-12-17 
08:06:00.036666681 +0800
     @@ -1,2 +1,3 @@
      QA output created by 074
      Silence is golden
     +Scrub find errors in "-m raid1 -d raid1" test
     ...
     (Run 'diff -u /home/adam/xfstests/tests/btrfs/074.out 
/home/adam/xfstests/results//btrfs/074.out.bad'  to see the entire diff)
Ran: btrfs/072 btrfs/074
Failures: btrfs/072 btrfs/074

Thanks,
Qu

> 
> Thanks,
> Qu
> 
>>
>> In order to avoid these sort of problems in the future lets fix up all 
>> the false
>> positivies that this warning brings, and then enable the option for 
>> btrfs so we
>> can avoid this style of failure in the future.  Thanks,
>>
>> Josef
>>
>> Josef Bacik (8):
>>    btrfs: fix uninit warning in run_one_async_start
>>    btrfs: fix uninit warning in btrfs_cleanup_ordered_extents
>>    btrfs: fix uninit warning from get_inode_gen usage
>>    btrfs: fix uninit warning in btrfs_update_block_group
>>    btrfs: fix uninit warning in __set_extent_bit and convert_extent_bit
>>    btrfs: extract out zone cache usage into it's own helper
>>    btrfs: fix uninit warning in btrfs_sb_log_location
>>    btrfs: turn on -Wmaybe-uninitialized
>>
>>   fs/btrfs/Makefile         |  1 +
>>   fs/btrfs/block-group.c    |  2 +-
>>   fs/btrfs/disk-io.c        |  2 +-
>>   fs/btrfs/extent-io-tree.c |  8 ++---
>>   fs/btrfs/inode.c          |  2 +-
>>   fs/btrfs/send.c           |  8 ++---
>>   fs/btrfs/zoned.c          | 75 ++++++++++++++++++++++++---------------
>>   7 files changed, 57 insertions(+), 41 deletions(-)
>>

Re: [PATCH 1/8] btrfs: fix uninit warning in run_one_async_start

[Qu Wenruo]

On 2022/12/17 04:15, Josef Bacik wrote:
> With -Wmaybe-uninitialized complains about ret being possibly
> uninitialized, which isn't possible, however we can init the value to
> get rid of the warning.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>

Reviewed-by: Qu Wenruo <wqu@suse.com>

Thanks,
Qu
> ---
>   fs/btrfs/disk-io.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index 0888d484df80..c25b444027d6 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -693,7 +693,7 @@ int btrfs_validate_metadata_buffer(struct btrfs_bio *bbio,
>   static void run_one_async_start(struct btrfs_work *work)
>   {
>   	struct async_submit_bio *async;
> -	blk_status_t ret;
> +	blk_status_t ret = BLK_STS_OK;
>   
>   	async = container_of(work, struct  async_submit_bio, work);
>   	switch (async->submit_cmd) {

Re: [PATCH 2/8] btrfs: fix uninit warning in btrfs_cleanup_ordered_extents

[Qu Wenruo]

On 2022/12/17 04:15, Josef Bacik wrote:
> We can conditionally pass in a locked page, and then we'll use that page
> range to skip marking errors as that will happen in another layer.
> However this causes the compiler to complain because it doesn't
> understand we only use these values when we have the page.  Make the
> compiler stop complaining by setting these values to 0.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>

Reviewed-by: Qu Wenruo <wqu@suse.com>

Thanks,
Qu
> ---
>   fs/btrfs/inode.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 905ea19df125..dfceaf79d5d4 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -228,7 +228,7 @@ static inline void btrfs_cleanup_ordered_extents(struct btrfs_inode *inode,
>   {
>   	unsigned long index = offset >> PAGE_SHIFT;
>   	unsigned long end_index = (offset + bytes - 1) >> PAGE_SHIFT;
> -	u64 page_start, page_end;
> +	u64 page_start = 0, page_end = 0;
>   	struct page *page;
>   
>   	if (locked_page) {

Re: [PATCH 3/8] btrfs: fix uninit warning from get_inode_gen usage

[Qu Wenruo]

On 2022/12/17 04:15, Josef Bacik wrote:
> Anybody that calls get_inode_gen() can have an uninitialized gen if
> there's an error.  This isn't a big deal because all the users just exit
> if they get an error, however it makes -Wmaybe-uninitialized complain,
> so fix this up to always init the passed in gen, this quiets all of the
> uninitialized warnings in send.c.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
Reviewed-by: Qu Wenruo <wqu@suse.com>

Thanks,
Qu
> ---
>   fs/btrfs/send.c | 8 +++-----
>   1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c
> index 67f7c698ade3..25a235179edb 100644
> --- a/fs/btrfs/send.c
> +++ b/fs/btrfs/send.c
> @@ -955,14 +955,12 @@ static int get_inode_info(struct btrfs_root *root, u64 ino,
>   static int get_inode_gen(struct btrfs_root *root, u64 ino, u64 *gen)
>   {
>   	int ret;
> -	struct btrfs_inode_info info;
> +	struct btrfs_inode_info info = {};
>   
> -	if (!gen)
> -		return -EPERM;
> +	ASSERT(gen);
>   
>   	ret = get_inode_info(root, ino, &info);
> -	if (!ret)
> -		*gen = info.gen;
> +	*gen = info.gen;
>   	return ret;
>   }
>   

Re: [PATCH 4/8] btrfs: fix uninit warning in btrfs_update_block_group

[Qu Wenruo]

On 2022/12/17 04:15, Josef Bacik wrote:
> reclaim isn't set in the alloc case, however we only care about reclaim
> in the !alloc case.  This isn't an actual problem, however
> -Wmaybe-uninitialized will complain, so initialize reclaim to quiet the
> compiler.

Reviewed-by: Qu Wenruo <wqu@suse.com>

Thanks,
Qu

> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> ---
>   fs/btrfs/block-group.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c
> index 708d843daa72..e90800388a41 100644
> --- a/fs/btrfs/block-group.c
> +++ b/fs/btrfs/block-group.c
> @@ -3330,7 +3330,7 @@ int btrfs_update_block_group(struct btrfs_trans_handle *trans,
>   	spin_unlock(&info->delalloc_root_lock);
>   
>   	while (total) {
> -		bool reclaim;
> +		bool reclaim = false;
>   
>   		cache = btrfs_lookup_block_group(info, bytenr);
>   		if (!cache) {

Re: [PATCH 5/8] btrfs: fix uninit warning in __set_extent_bit and convert_extent_bit

[Qu Wenruo]

On 2022/12/17 04:15, Josef Bacik wrote:
> We will pass in the parent and p pointer into our tree_search function
> to avoid doing a second search when inserting a new extent state into
> the tree.  However because this is conditional upon passing in these
> pointers the compiler seems to think these values can be uninitialized
> if we're using -Wmaybe-uninitialized.  Fix this by initializing these
> values.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>

Reviewed-by: Qu Wenruo <wqu@suse.com>

Thanks,
Qu
> ---
>   fs/btrfs/extent-io-tree.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/btrfs/extent-io-tree.c b/fs/btrfs/extent-io-tree.c
> index 9ae9cd1e7035..9e1f18706a02 100644
> --- a/fs/btrfs/extent-io-tree.c
> +++ b/fs/btrfs/extent-io-tree.c
> @@ -972,8 +972,8 @@ static int __set_extent_bit(struct extent_io_tree *tree, u64 start, u64 end,
>   {
>   	struct extent_state *state;
>   	struct extent_state *prealloc = NULL;
> -	struct rb_node **p;
> -	struct rb_node *parent;
> +	struct rb_node **p = NULL;
> +	struct rb_node *parent = NULL;
>   	int err = 0;
>   	u64 last_start;
>   	u64 last_end;
> @@ -1218,8 +1218,8 @@ int convert_extent_bit(struct extent_io_tree *tree, u64 start, u64 end,
>   {
>   	struct extent_state *state;
>   	struct extent_state *prealloc = NULL;
> -	struct rb_node **p;
> -	struct rb_node *parent;
> +	struct rb_node **p = NULL;
> +	struct rb_node *parent = NULL;
>   	int err = 0;
>   	u64 last_start;
>   	u64 last_end;

Re: [PATCH 8/8] btrfs: turn on -Wmaybe-uninitialized

[Qu Wenruo]

On 2022/12/17 04:15, Josef Bacik wrote:
> We had a recent bug that would have been caught by a newer compiler with
> -Wmaybe-uninitialized and would have saved us a month of failing tests
> that I didn't have time to investigate.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>

Reviewed-by: Qu Wenruo <wqu@suse.com>

Tested with gcc 12.2.0, no new warnings.

Thanks,
Qu

> ---
>   fs/btrfs/Makefile | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/fs/btrfs/Makefile b/fs/btrfs/Makefile
> index 555c962fdad6..eca995abccdf 100644
> --- a/fs/btrfs/Makefile
> +++ b/fs/btrfs/Makefile
> @@ -7,6 +7,7 @@ subdir-ccflags-y += -Wmissing-format-attribute
>   subdir-ccflags-y += -Wmissing-prototypes
>   subdir-ccflags-y += -Wold-style-definition
>   subdir-ccflags-y += -Wmissing-include-dirs
> +subdir-ccflags-y += -Wmaybe-uninitialized
>   condflags := \
>   	$(call cc-option, -Wunused-but-set-variable)		\
>   	$(call cc-option, -Wunused-const-variable)		\

Re: [PATCH 7/8] btrfs: fix uninit warning in btrfs_sb_log_location

[Naohiro Aota]

On Fri, Dec 16, 2022 at 03:15:57PM -0500, Josef Bacik wrote:
> We only have 3 possible mirrors, and we have ASSERT()'s to make sure
> we're not passing in an invalid super mirror into this function, so
> technically this value isn't uninitialized.  However
> -Wmaybe-uninitialized will complain, so set it to U64_MAX so if we don't
> have ASSERT()'s turned on it'll error out later on when it see's the
> zone is beyond our maximum zones.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>

Looks good.

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

Re: [PATCH 6/8] btrfs: extract out zone cache usage into it's own helper

[Naohiro Aota]

On Fri, Dec 16, 2022 at 03:15:56PM -0500, Josef Bacik wrote:
> There's a special case for loading the device zone info if we have the
> zone cache which is a fair bit of code.  Extract this out into it's own
> helper to clean up the code a little bit, and as a side effect it fixes
> an uninitialized error we get with -Wmaybe-uninitialized where it
> thought zno may have been uninitialized.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>

I'm going to rewrite the code around here with the following WIP branch, to
improve the zone caching.

https://github.com/naota/linux/commits/feature/zone-cache

Specifically, this commit removes the for-loop and the "if (i ==
*nr_zones)" block you moved in this patch. So, the resulting code will be
small enough to keep it there.

https://github.com/naota/linux/commit/8d592ac744111bb2f51595a1608beecadb2c5d03

Could you wait for a while for me to clean-up and send the series? I'll
also check the series with -Wmaybe-uninitialized.

> ---
>  fs/btrfs/zoned.c | 73 +++++++++++++++++++++++++++++-------------------
>  1 file changed, 45 insertions(+), 28 deletions(-)
> 
> diff --git a/fs/btrfs/zoned.c b/fs/btrfs/zoned.c
> index a759668477bb..f3640ab95e5e 100644
> --- a/fs/btrfs/zoned.c
> +++ b/fs/btrfs/zoned.c
> @@ -216,11 +216,46 @@ static int emulate_report_zones(struct btrfs_device *device, u64 pos,
>  	return i;
>  }
>  
> +static int load_zones_from_cache(struct btrfs_zoned_device_info *zinfo, u64 pos,
> +				 struct blk_zone *zones, unsigned int *nr_zones)
> +{
> +	unsigned int i;
> +	u32 zno;
> +
> +	if (!zinfo->zone_cache)
> +		return -ENOENT;
> +
> +	ASSERT(IS_ALIGNED(pos, zinfo->zone_size));
> +	zno = pos >> zinfo->zone_size_shift;
> +
> +	/*
> +	 * We cannot report zones beyond the zone end. So, it is OK to
> +	 * cap *nr_zones to at the end.
> +	 */
> +	*nr_zones = min_t(u32, *nr_zones, zinfo->nr_zones - zno);
> +
> +	for (i = 0; i < *nr_zones; i++) {
> +		struct blk_zone *zone_info;
> +
> +		zone_info = &zinfo->zone_cache[zno + i];
> +		if (!zone_info->len)
> +			break;
> +	}
> +
> +	if (i == *nr_zones) {
> +		/* Cache hit on all the zones */
> +		memcpy(zones, zinfo->zone_cache + zno,
> +		       sizeof(*zinfo->zone_cache) * *nr_zones);
> +		return 0;
> +	}
> +
> +	return -ENOENT;
> +}
> +
>  static int btrfs_get_dev_zones(struct btrfs_device *device, u64 pos,
>  			       struct blk_zone *zones, unsigned int *nr_zones)
>  {
>  	struct btrfs_zoned_device_info *zinfo = device->zone_info;
> -	u32 zno;
>  	int ret;
>  
>  	if (!*nr_zones)
> @@ -233,32 +268,8 @@ static int btrfs_get_dev_zones(struct btrfs_device *device, u64 pos,
>  	}
>  
>  	/* Check cache */
> -	if (zinfo->zone_cache) {
> -		unsigned int i;
> -
> -		ASSERT(IS_ALIGNED(pos, zinfo->zone_size));
> -		zno = pos >> zinfo->zone_size_shift;
> -		/*
> -		 * We cannot report zones beyond the zone end. So, it is OK to
> -		 * cap *nr_zones to at the end.
> -		 */
> -		*nr_zones = min_t(u32, *nr_zones, zinfo->nr_zones - zno);
> -
> -		for (i = 0; i < *nr_zones; i++) {
> -			struct blk_zone *zone_info;
> -
> -			zone_info = &zinfo->zone_cache[zno + i];
> -			if (!zone_info->len)
> -				break;
> -		}
> -
> -		if (i == *nr_zones) {
> -			/* Cache hit on all the zones */
> -			memcpy(zones, zinfo->zone_cache + zno,
> -			       sizeof(*zinfo->zone_cache) * *nr_zones);
> -			return 0;
> -		}
> -	}
> +	if (!load_zones_from_cache(zinfo, pos, zones, nr_zones))
> +		return 0;
>  
>  	ret = blkdev_report_zones(device->bdev, pos >> SECTOR_SHIFT, *nr_zones,
>  				  copy_zone_info_cb, zones);
> @@ -274,9 +285,15 @@ static int btrfs_get_dev_zones(struct btrfs_device *device, u64 pos,
>  		return -EIO;
>  
>  	/* Populate cache */
> -	if (zinfo->zone_cache)
> +	if (zinfo->zone_cache) {
> +		u32 zno;
> +
> +		ASSERT(IS_ALIGNED(pos, zinfo->zone_size));
> +		zno = pos >> zinfo->zone_size_shift;
> +
>  		memcpy(zinfo->zone_cache + zno, zones,
>  		       sizeof(*zinfo->zone_cache) * *nr_zones);
> +	}
>  
>  	return 0;
>  }
> -- 
> 2.26.3
> 

Re: [PATCH 1/8] btrfs: fix uninit warning in run_one_async_start

[Johannes Thumshirn]

On 16.12.22 21:17, Josef Bacik wrote:
> With -Wmaybe-uninitialized complains about ret being possibly
> uninitialized, which isn't possible, however we can init the value to
> get rid of the warning.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> ---
>  fs/btrfs/disk-io.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index 0888d484df80..c25b444027d6 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -693,7 +693,7 @@ int btrfs_validate_metadata_buffer(struct btrfs_bio *bbio,
>  static void run_one_async_start(struct btrfs_work *work)
>  {
>  	struct async_submit_bio *async;
> -	blk_status_t ret;
> +	blk_status_t ret = BLK_STS_OK;
>  
>  	async = container_of(work, struct  async_submit_bio, work);
>  	switch (async->submit_cmd) {

Wouldn't that be more future proof:

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 40f9c99aa44a..6ad5e5c6c858 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -707,6 +707,9 @@ static void run_one_async_start(struct btrfs_work *work)
                ret = btrfs_submit_bio_start_direct_io(async->inode,
                                async->bio, async->dio_file_offset);
                break;
+       default:
+               ret = BLK_STS_NOTSUPP;
+               ASSERT(0);
        }
        if (ret)
                async->status = ret;

Re: [PATCH 2/8] btrfs: fix uninit warning in btrfs_cleanup_ordered_extents

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

Re: [PATCH 3/8] btrfs: fix uninit warning from get_inode_gen usage

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

Re: [PATCH 4/8] btrfs: fix uninit warning in btrfs_update_block_group

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

Re: [PATCH 7/8] btrfs: fix uninit warning in btrfs_sb_log_location

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

Re: [PATCH 1/8] btrfs: fix uninit warning in run_one_async_start

[David Sterba]

On Mon, Dec 19, 2022 at 07:51:31AM +0000, Johannes Thumshirn wrote:
> On 16.12.22 21:17, Josef Bacik wrote:
> > With -Wmaybe-uninitialized complains about ret being possibly
> > uninitialized, which isn't possible, however we can init the value to
> > get rid of the warning.
> > 
> > Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> > ---
> >  fs/btrfs/disk-io.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> > index 0888d484df80..c25b444027d6 100644
> > --- a/fs/btrfs/disk-io.c
> > +++ b/fs/btrfs/disk-io.c
> > @@ -693,7 +693,7 @@ int btrfs_validate_metadata_buffer(struct btrfs_bio *bbio,
> >  static void run_one_async_start(struct btrfs_work *work)
> >  {
> >  	struct async_submit_bio *async;
> > -	blk_status_t ret;
> > +	blk_status_t ret = BLK_STS_OK;
> >  
> >  	async = container_of(work, struct  async_submit_bio, work);
> >  	switch (async->submit_cmd) {
> 
> Wouldn't that be more future proof:
> 
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index 40f9c99aa44a..6ad5e5c6c858 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -707,6 +707,9 @@ static void run_one_async_start(struct btrfs_work *work)
>                 ret = btrfs_submit_bio_start_direct_io(async->inode,
>                                 async->bio, async->dio_file_offset);
>                 break;
> +       default:
> +               ret = BLK_STS_NOTSUPP;
> +               ASSERT(0);

The assert and default: would probably make sense to catch any
accidental misuse of the async API. All the submit_cmd values are from a
fixed set and all are in the source code so any problem should be caught
at development time.

I'm not sure if there should be any fallback for the case where asserts
are complied out and what's the expected outcome of the BLK_STS_NOTSUPP.
This is a case that should never happen.

Re: [PATCH 3/8] btrfs: fix uninit warning from get_inode_gen usage

[David Sterba]

On Fri, Dec 16, 2022 at 03:15:53PM -0500, Josef Bacik wrote:
> Anybody that calls get_inode_gen() can have an uninitialized gen if
> there's an error.  This isn't a big deal because all the users just exit
> if they get an error, however it makes -Wmaybe-uninitialized complain,
> so fix this up to always init the passed in gen, this quiets all of the
> uninitialized warnings in send.c.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> ---
>  fs/btrfs/send.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c
> index 67f7c698ade3..25a235179edb 100644
> --- a/fs/btrfs/send.c
> +++ b/fs/btrfs/send.c
> @@ -955,14 +955,12 @@ static int get_inode_info(struct btrfs_root *root, u64 ino,
>  static int get_inode_gen(struct btrfs_root *root, u64 ino, u64 *gen)
>  {
>  	int ret;
> -	struct btrfs_inode_info info;
> +	struct btrfs_inode_info info = {};

The { 0 } initializer should be used.

Re: [PATCH 6/8] btrfs: extract out zone cache usage into it's own helper

[David Sterba]

On Mon, Dec 19, 2022 at 07:05:15AM +0000, Naohiro Aota wrote:
> On Fri, Dec 16, 2022 at 03:15:56PM -0500, Josef Bacik wrote:
> > There's a special case for loading the device zone info if we have the
> > zone cache which is a fair bit of code.  Extract this out into it's own
> > helper to clean up the code a little bit, and as a side effect it fixes
> > an uninitialized error we get with -Wmaybe-uninitialized where it
> > thought zno may have been uninitialized.
> > 
> > Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> 
> I'm going to rewrite the code around here with the following WIP branch, to
> improve the zone caching.
> 
> https://github.com/naota/linux/commits/feature/zone-cache
> 
> Specifically, this commit removes the for-loop and the "if (i ==
> *nr_zones)" block you moved in this patch. So, the resulting code will be
> small enough to keep it there.
> 
> https://github.com/naota/linux/commit/8d592ac744111bb2f51595a1608beecadb2c5d03
> 
> Could you wait for a while for me to clean-up and send the series? I'll
> also check the series with -Wmaybe-uninitialized.

I'd like to have the warnigs fixes first but if there is a shorter fix
that would not move the code then it can go in now and you wouldn't have
to redo your changes.

Re: [PATCH 0/8] Fixup uninitialized warnings and enable extra checks

[David Sterba]

On Fri, Dec 16, 2022 at 03:15:50PM -0500, Josef Bacik wrote:
> Hello,
> 
> We had been failing the raid56 related scrub tests on our overnight tests since
> November.  Initially I asked Qu to look into these as I didn't have time to dig
> in, and he was unable to reproduce.  I assumed it was some oddity in my setup,
> so I ignored it.  However recently I got a report that I regressed some of these
> tests with an unrelated change.  When debugging it I found it was because of an
> uninitialized return value, which would have been caught by more modern gcc's
> with -Wmaybe-uninitialized.
> 
> In order to avoid these sort of problems in the future lets fix up all the false
> positivies that this warning brings, and then enable the option for btrfs so we
> can avoid this style of failure in the future.  Thanks,
> 
> Josef
> 
> Josef Bacik (8):
>   btrfs: fix uninit warning in run_one_async_start
>   btrfs: fix uninit warning in btrfs_cleanup_ordered_extents
>   btrfs: fix uninit warning from get_inode_gen usage
>   btrfs: fix uninit warning in btrfs_update_block_group
>   btrfs: fix uninit warning in __set_extent_bit and convert_extent_bit
>   btrfs: extract out zone cache usage into it's own helper
>   btrfs: fix uninit warning in btrfs_sb_log_location
>   btrfs: turn on -Wmaybe-uninitialized

I've applied all except 1, 6 and 8, so there are still 2 reported
warnings with -Wmaybe-uninitialized that could be fixed in a slightly
different way.

Re: [PATCH 6/8] btrfs: extract out zone cache usage into it's own helper

[Naohiro Aota]

On Tue, Dec 20, 2022 at 08:24:56PM +0100, David Sterba wrote:
> On Mon, Dec 19, 2022 at 07:05:15AM +0000, Naohiro Aota wrote:
> > On Fri, Dec 16, 2022 at 03:15:56PM -0500, Josef Bacik wrote:
> > > There's a special case for loading the device zone info if we have the
> > > zone cache which is a fair bit of code.  Extract this out into it's own
> > > helper to clean up the code a little bit, and as a side effect it fixes
> > > an uninitialized error we get with -Wmaybe-uninitialized where it
> > > thought zno may have been uninitialized.
> > > 
> > > Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> > 
> > I'm going to rewrite the code around here with the following WIP branch, to
> > improve the zone caching.
> > 
> > https://github.com/naota/linux/commits/feature/zone-cache
> > 
> > Specifically, this commit removes the for-loop and the "if (i ==
> > *nr_zones)" block you moved in this patch. So, the resulting code will be
> > small enough to keep it there.
> > 
> > https://github.com/naota/linux/commit/8d592ac744111bb2f51595a1608beecadb2c5d03
> > 
> > Could you wait for a while for me to clean-up and send the series? I'll
> > also check the series with -Wmaybe-uninitialized.
> 
> I'd like to have the warnigs fixes first but if there is a shorter fix
> that would not move the code then it can go in now and you wouldn't have
> to redo your changes.

Sure, how about this then?

From 50bc2858dc58301ca1b7d61bb72ca2566e59032c Mon Sep 17 00:00:00 2001
From: Naohiro Aota <naohiro.aota@wdc.com>
Date: Thu, 22 Dec 2022 01:17:59 +0900
Subject: [PATCH] btrfs: zoned: fix uninit warning in btrfs_get_dev_zones

Fix an uninitialized error we get with -Wmaybe-uninitialized where it
thought zno may have been uninitialized.

Since there will be a rewrite of the code, avoid moving the code around and
do the small enough fix.

Reported-by: Josef Bacik <josef@toxicpanda.com>
Link: https://lore.kernel.org/linux-btrfs/af6c527cbd8bdc782e50bd33996ee83acc3a16fb.1671221596.git.josef@toxicpanda.com/
Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com>
---
 fs/btrfs/zoned.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/zoned.c b/fs/btrfs/zoned.c
index a759668477bb..ab63f8443e0a 100644
--- a/fs/btrfs/zoned.c
+++ b/fs/btrfs/zoned.c
@@ -220,7 +220,6 @@ static int btrfs_get_dev_zones(struct btrfs_device *device, u64 pos,
 			       struct blk_zone *zones, unsigned int *nr_zones)
 {
 	struct btrfs_zoned_device_info *zinfo = device->zone_info;
-	u32 zno;
 	int ret;
 
 	if (!*nr_zones)
@@ -235,6 +234,7 @@ static int btrfs_get_dev_zones(struct btrfs_device *device, u64 pos,
 	/* Check cache */
 	if (zinfo->zone_cache) {
 		unsigned int i;
+		u32 zno;
 
 		ASSERT(IS_ALIGNED(pos, zinfo->zone_size));
 		zno = pos >> zinfo->zone_size_shift;
@@ -274,9 +274,12 @@ static int btrfs_get_dev_zones(struct btrfs_device *device, u64 pos,
 		return -EIO;
 
 	/* Populate cache */
-	if (zinfo->zone_cache)
+	if (zinfo->zone_cache) {
+		u32 zno = pos >> zinfo->zone_size_shift;
+
 		memcpy(zinfo->zone_cache + zno, zones,
 		       sizeof(*zinfo->zone_cache) * *nr_zones);
+	}
 
 	return 0;
 }
-- 
2.39.0

Re: [PATCH 6/8] btrfs: extract out zone cache usage into it's own helper

[David Sterba]

On Wed, Dec 21, 2022 at 04:47:45PM +0000, Naohiro Aota wrote:
> Sure, how about this then?

That looks good, added to misc-next, thanks.

Re: [PATCH 1/8] btrfs: fix uninit warning in run_one_async_start

[David Sterba]

On Mon, Dec 19, 2022 at 07:51:31AM +0000, Johannes Thumshirn wrote:
> On 16.12.22 21:17, Josef Bacik wrote:
> > With -Wmaybe-uninitialized complains about ret being possibly
> > uninitialized, which isn't possible, however we can init the value to
> > get rid of the warning.
> > 
> > Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> > ---
> >  fs/btrfs/disk-io.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> > index 0888d484df80..c25b444027d6 100644
> > --- a/fs/btrfs/disk-io.c
> > +++ b/fs/btrfs/disk-io.c
> > @@ -693,7 +693,7 @@ int btrfs_validate_metadata_buffer(struct btrfs_bio *bbio,
> >  static void run_one_async_start(struct btrfs_work *work)
> >  {
> >  	struct async_submit_bio *async;
> > -	blk_status_t ret;
> > +	blk_status_t ret = BLK_STS_OK;
> >  
> >  	async = container_of(work, struct  async_submit_bio, work);
> >  	switch (async->submit_cmd) {
> 
> Wouldn't that be more future proof:
> 
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index 40f9c99aa44a..6ad5e5c6c858 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -707,6 +707,9 @@ static void run_one_async_start(struct btrfs_work *work)
>                 ret = btrfs_submit_bio_start_direct_io(async->inode,
>                                 async->bio, async->dio_file_offset);
>                 break;
> +       default:
> +               ret = BLK_STS_NOTSUPP;

I've committed version with BLK_STS_IOERR as it's a noisier error, I'm
not sure how much the op-not-supported would be noticed by any userspace
application if we ever reach this code but this is in the 'cannot
happen' realm.

Naohiro sent an update to the zoned fix so now we have all the warnings
fixed and I'll add the last patch to enable the warning.

Re: [PATCH 0/8] Fixup uninitialized warnings and enable extra checks

[David Sterba]

On Tue, Dec 20, 2022 at 08:37:06PM +0100, David Sterba wrote:
> On Fri, Dec 16, 2022 at 03:15:50PM -0500, Josef Bacik wrote:
> > Hello,
> > 
> > We had been failing the raid56 related scrub tests on our overnight tests since
> > November.  Initially I asked Qu to look into these as I didn't have time to dig
> > in, and he was unable to reproduce.  I assumed it was some oddity in my setup,
> > so I ignored it.  However recently I got a report that I regressed some of these
> > tests with an unrelated change.  When debugging it I found it was because of an
> > uninitialized return value, which would have been caught by more modern gcc's
> > with -Wmaybe-uninitialized.
> > 
> > In order to avoid these sort of problems in the future lets fix up all the false
> > positivies that this warning brings, and then enable the option for btrfs so we
> > can avoid this style of failure in the future.  Thanks,
> > 
> > Josef
> > 
> > Josef Bacik (8):
> >   btrfs: fix uninit warning in run_one_async_start
> >   btrfs: fix uninit warning in btrfs_cleanup_ordered_extents
> >   btrfs: fix uninit warning from get_inode_gen usage
> >   btrfs: fix uninit warning in btrfs_update_block_group
> >   btrfs: fix uninit warning in __set_extent_bit and convert_extent_bit
> >   btrfs: extract out zone cache usage into it's own helper
> >   btrfs: fix uninit warning in btrfs_sb_log_location
> >   btrfs: turn on -Wmaybe-uninitialized
> 
> I've applied all except 1, 6 and 8, so there are still 2 reported
> warnings with -Wmaybe-uninitialized that could be fixed in a slightly
> different way.

Fixed versions of patches 1 and 6 have been applied so 8 has been
applied too.

Re: [PATCH 8/8] btrfs: turn on -Wmaybe-uninitialized

[Nathan Chancellor]

On Fri, Dec 16, 2022 at 03:15:58PM -0500, Josef Bacik wrote:
> We had a recent bug that would have been caught by a newer compiler with
> -Wmaybe-uninitialized and would have saved us a month of failing tests
> that I didn't have time to investigate.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>

This needs to be moved to the condflags section, as
-Wmaybe-uninitialized is a GCC only flag, so this breaks our builds with
clang on next-20221226:

  error: unknown warning option '-Wmaybe-uninitialized'; did you mean '-Wuninitialized'? [-Werror,-Wunknown-warning-option]

I can send a patch on Tuesday unless the original commit can be amended.

> ---
>  fs/btrfs/Makefile | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/fs/btrfs/Makefile b/fs/btrfs/Makefile
> index 555c962fdad6..eca995abccdf 100644
> --- a/fs/btrfs/Makefile
> +++ b/fs/btrfs/Makefile
> @@ -7,6 +7,7 @@ subdir-ccflags-y += -Wmissing-format-attribute
>  subdir-ccflags-y += -Wmissing-prototypes
>  subdir-ccflags-y += -Wold-style-definition
>  subdir-ccflags-y += -Wmissing-include-dirs
> +subdir-ccflags-y += -Wmaybe-uninitialized
>  condflags := \
>  	$(call cc-option, -Wunused-but-set-variable)		\
>  	$(call cc-option, -Wunused-const-variable)		\
> -- 
> 2.26.3
> 
> 

Re: [PATCH 8/8] btrfs: turn on -Wmaybe-uninitialized

[Naresh Kamboju]

On Mon, 26 Dec 2022 at 09:47, Nathan Chancellor <nathan@kernel.org> wrote:
>
> On Fri, Dec 16, 2022 at 03:15:58PM -0500, Josef Bacik wrote:
> > We had a recent bug that would have been caught by a newer compiler with
> > -Wmaybe-uninitialized and would have saved us a month of failing tests
> > that I didn't have time to investigate.
> >
> > Signed-off-by: Josef Bacik <josef@toxicpanda.com>
>
> This needs to be moved to the condflags section, as
> -Wmaybe-uninitialized is a GCC only flag, so this breaks our builds with
> clang on next-20221226:
>
>   error: unknown warning option '-Wmaybe-uninitialized'; did you mean '-Wuninitialized'? [-Werror,-Wunknown-warning-option]

LKFT test farm also noticed these build breaks with clang on next-20221226.

Regressions found on arm64, riscv, s390 and powerpc

    - build/clang-lkftconfig
    - build/clang-15-lkftconfig
    - build/clang-15-defconfig-40bc7ee5
    - build/clang-15-defconfig
    - build/clang-nightly-defconfig
    - build/clang-15-lkftconfig-compat
    - build/clang-nightly-defconfig-40bc7ee5
    - build/clang-nightly-lkftconfig

make --silent --keep-going --jobs=8
O=/home/tuxbuild/.cache/tuxmake/builds/1/build LLVM=1 LLVM_IAS=1
ARCH=arm64 CROSS_COMPILE=aarch64-linux-gnu- 'HOSTCC=sccache clang'
'CC=sccache clang'

error: unknown warning option '-Wmaybe-uninitialized'; did you mean
'-Wuninitialized'? [-Werror,-Wunknown-warning-option]
make[4]: *** [/builds/linux/scripts/Makefile.build:252:
fs/btrfs/super.o] Error 1


> I can send a patch on Tuesday unless the original commit can be amended.


ref:
https://qa-reports.linaro.org/lkft/linux-next-master/build/next-20221226/testrun/13818773/suite/build/test/clang-15-defconfig/details/

- Naresh

Re: [PATCH 8/8] btrfs: turn on -Wmaybe-uninitialized

[David Sterba]

On Mon, Dec 26, 2022 at 07:34:51PM +0530, Naresh Kamboju wrote:
> On Mon, 26 Dec 2022 at 09:47, Nathan Chancellor <nathan@kernel.org> wrote:
> >
> > On Fri, Dec 16, 2022 at 03:15:58PM -0500, Josef Bacik wrote:
> > > We had a recent bug that would have been caught by a newer compiler with
> > > -Wmaybe-uninitialized and would have saved us a month of failing tests
> > > that I didn't have time to investigate.
> > >
> > > Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> >
> > This needs to be moved to the condflags section, as
> > -Wmaybe-uninitialized is a GCC only flag, so this breaks our builds with
> > clang on next-20221226:
> >
> >   error: unknown warning option '-Wmaybe-uninitialized'; did you mean '-Wuninitialized'? [-Werror,-Wunknown-warning-option]
> 
> LKFT test farm also noticed these build breaks with clang on next-20221226.

Fixed in our branch and for-next snapshot updated so the warnings will
be fixed once linux-next resumes.

Re: [PATCH 8/8] btrfs: turn on -Wmaybe-uninitialized

[Guenter Roeck]

On Fri, Dec 16, 2022 at 03:15:58PM -0500, Josef Bacik wrote:
> We had a recent bug that would have been caught by a newer compiler with
> -Wmaybe-uninitialized and would have saved us a month of failing tests
> that I didn't have time to investigate.
> 

Thanks to this patch, sparc64:allmodconfig and parisc:allmodconfig now
fail to commpile with the following error when trying to build images
with gcc 11.3.

Building sparc64:allmodconfig ... failed
--------------
Error log:
<stdin>:1517:2: warning: #warning syscall clone3 not implemented [-Wcpp]
fs/btrfs/inode.c: In function 'btrfs_lookup_dentry':
fs/btrfs/inode.c:5730:21: error: 'location.type' may be used uninitialized [-Werror=maybe-uninitialized]
 5730 |         if (location.type == BTRFS_INODE_ITEM_KEY) {
      |             ~~~~~~~~^~~~~
fs/btrfs/inode.c:5719:26: note: 'location' declared here
 5719 |         struct btrfs_key location;

Bisect log attached.

Guenter

---
# bad: [8bf1a529cd664c8e5268381f1e24fe67aa611dd3] Merge tag 'arm64-upstream' of git://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux
# good: [c9c3395d5e3dcc6daee66c6908354d47bf98cb0c] Linux 6.2
git bisect start 'HEAD' 'v6.2'
# good: [e43efb6d713bca3855909a2f9caec280a2b0a503] dt-bindings: riscv: correct starfive visionfive 2 compatibles
git bisect good e43efb6d713bca3855909a2f9caec280a2b0a503
# bad: [1f2d9ffc7a5f916935749ffc6e93fb33bfe94d2f] Merge tag 'sched-core-2023-02-20' of git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip
git bisect bad 1f2d9ffc7a5f916935749ffc6e93fb33bfe94d2f
# bad: [553637f73c314c742243b8dc5ef072e9dadbe581] Merge tag 'for-6.3/dio-2023-02-16' of git://git.kernel.dk/linux
git bisect bad 553637f73c314c742243b8dc5ef072e9dadbe581
# good: [274978f173276c5720a3cd8d0b6047d2c0d3a684] Merge tag 'fixes_for_v6.3-rc1' of git://git.kernel.org/pub/scm/linux/kernel/git/jack/linux-fs
git bisect good 274978f173276c5720a3cd8d0b6047d2c0d3a684
# bad: [801fcfc5d790f4a9be2897713bd6dd08bed253f1] btrfs: raid56: add a bio_list_put helper
git bisect bad 801fcfc5d790f4a9be2897713bd6dd08bed253f1
# bad: [e2fd83064a9bae368ce1c88a0cb9aee64ad4e124] btrfs: skip backref walking during fiemap if we know the leaf is shared
git bisect bad e2fd83064a9bae368ce1c88a0cb9aee64ad4e124
# bad: [cb0922f264643f03b04352f7a04abb913c609369] btrfs: don't use size classes for zoned file systems
git bisect bad cb0922f264643f03b04352f7a04abb913c609369
# good: [cd30d3bc78d9acbd505d0d6a4cff3b87e40a8187] btrfs: zoned: fix uninitialized variable warning in btrfs_get_dev_zones
git bisect good cd30d3bc78d9acbd505d0d6a4cff3b87e40a8187
# bad: [235e1c7b872f9cf16e8a3e6050a05774b8763c58] btrfs: use a single variable to track return value for log_dir_items()
git bisect bad 235e1c7b872f9cf16e8a3e6050a05774b8763c58
# bad: [d31de3785047a24959eda835b0bafb1f8629f8a9] btrfs: go to matching label when cleaning em in btrfs_submit_direct
git bisect bad d31de3785047a24959eda835b0bafb1f8629f8a9
# bad: [1ec49744ba83f0429c5c706708610f7821a7b6f4] btrfs: turn on -Wmaybe-uninitialized
git bisect bad 1ec49744ba83f0429c5c706708610f7821a7b6f4
# good: [a6ca692ec22bdac5ae700e82ff575a1b5141fa40] btrfs: fix uninitialized variable warning in run_one_async_start
git bisect good a6ca692ec22bdac5ae700e82ff575a1b5141fa40
# first bad commit: [1ec49744ba83f0429c5c706708610f7821a7b6f4] btrfs: turn on -Wmaybe-uninitialized

Re: [PATCH 8/8] btrfs: turn on -Wmaybe-uninitialized

[David Sterba]

On Tue, Feb 21, 2023 at 06:59:18PM -0800, Guenter Roeck wrote:
> On Fri, Dec 16, 2022 at 03:15:58PM -0500, Josef Bacik wrote:
> > We had a recent bug that would have been caught by a newer compiler with
> > -Wmaybe-uninitialized and would have saved us a month of failing tests
> > that I didn't have time to investigate.
> > 
> 
> Thanks to this patch, sparc64:allmodconfig and parisc:allmodconfig now
> fail to commpile with the following error when trying to build images
> with gcc 11.3.
> 
> Building sparc64:allmodconfig ... failed
> --------------
> Error log:
> <stdin>:1517:2: warning: #warning syscall clone3 not implemented [-Wcpp]
> fs/btrfs/inode.c: In function 'btrfs_lookup_dentry':
> fs/btrfs/inode.c:5730:21: error: 'location.type' may be used uninitialized [-Werror=maybe-uninitialized]
>  5730 |         if (location.type == BTRFS_INODE_ITEM_KEY) {
>       |             ~~~~~~~~^~~~~
> fs/btrfs/inode.c:5719:26: note: 'location' declared here
>  5719 |         struct btrfs_key location;

Thanks for the report, Linus warned me that there might be some fallouts
and that the warning flag might need reverted. But before I do that I'd
like to try to understand why the warnings happen in a code where is no
reason for it.

I did a quick test on gcc 11.3 (on x86_64, not on sparc64 unlike you
report), and there is no warning

gcc (SUSE Linux) 11.3.1 20220721 [revision a55184ada8e2887ca94c0ab07027617885beafc9]
Copyright (C) 2021 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

  DESCEND objtool
  CALL    scripts/checksyscalls.sh
  CC [M]  fs/btrfs/inode.o

I.e. it's the same version, different arch and likely not the same
config. In the function itself thre's a local variable passed by address
to a static function in the same file.

	struct btrfs_key location;
	...
	ret = btrfs_inode_by_name(BTRFS_I(dir), dentry, &location, &di_type);

and there it's

	btrfs_dir_item_key_to_cpu(path->nodes[0], di, location);

which is a series of helpers to read some data and store that to the
strucutre. At some point there's a call to read_extent_buffer() that's
in a different file.

A local variable passed by address to external function is quite common
so I'd expect more warnings and I don't see what's different in this
case.

Re: [PATCH 8/8] btrfs: turn on -Wmaybe-uninitialized

[Guenter Roeck]

On 2/22/23 08:38, David Sterba wrote:
> On Tue, Feb 21, 2023 at 06:59:18PM -0800, Guenter Roeck wrote:
>> On Fri, Dec 16, 2022 at 03:15:58PM -0500, Josef Bacik wrote:
>>> We had a recent bug that would have been caught by a newer compiler with
>>> -Wmaybe-uninitialized and would have saved us a month of failing tests
>>> that I didn't have time to investigate.
>>>
>>
>> Thanks to this patch, sparc64:allmodconfig and parisc:allmodconfig now
>> fail to commpile with the following error when trying to build images
>> with gcc 11.3.
>>
>> Building sparc64:allmodconfig ... failed
>> --------------
>> Error log:
>> <stdin>:1517:2: warning: #warning syscall clone3 not implemented [-Wcpp]
>> fs/btrfs/inode.c: In function 'btrfs_lookup_dentry':
>> fs/btrfs/inode.c:5730:21: error: 'location.type' may be used uninitialized [-Werror=maybe-uninitialized]
>>   5730 |         if (location.type == BTRFS_INODE_ITEM_KEY) {
>>        |             ~~~~~~~~^~~~~
>> fs/btrfs/inode.c:5719:26: note: 'location' declared here
>>   5719 |         struct btrfs_key location;
> 
> Thanks for the report, Linus warned me that there might be some fallouts
> and that the warning flag might need reverted. But before I do that I'd
> like to try to understand why the warnings happen in a code where is no
> reason for it.
> 
> I did a quick test on gcc 11.3 (on x86_64, not on sparc64 unlike you
> report), and there is no warning
> 
> gcc (SUSE Linux) 11.3.1 20220721 [revision a55184ada8e2887ca94c0ab07027617885beafc9]
> Copyright (C) 2021 Free Software Foundation, Inc.
> This is free software; see the source for copying conditions.  There is NO
> warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
> 
>    DESCEND objtool
>    CALL    scripts/checksyscalls.sh
>    CC [M]  fs/btrfs/inode.o
> 
> I.e. it's the same version, different arch and likely not the same
> config. In the function itself thre's a local variable passed by address
> to a static function in the same file.
> 
> 	struct btrfs_key location;
> 	...
> 	ret = btrfs_inode_by_name(BTRFS_I(dir), dentry, &location, &di_type);
> 
> and there it's
> 
> 	btrfs_dir_item_key_to_cpu(path->nodes[0], di, location);
> 
> which is a series of helpers to read some data and store that to the
> strucutre. At some point there's a call to read_extent_buffer() that's
> in a different file.
> 
> A local variable passed by address to external function is quite common
> so I'd expect more warnings and I don't see what's different in this
> case.

Me not either. I also don't see the problem with other architectures, only
with sparc and parisc. It doesn't have to be gcc 11.3, though; it also happens
with gcc 11.1, 11.2, 12.1, and 12.2 (tested on sparc).

Too bad that gcc doesn't tell why exactly it believes that the object
may be uninitialized. Anyway, the following change would fix the problem.

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 6c18dc9a1831..4bab8ab39948 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -5421,7 +5421,7 @@ static int btrfs_inode_by_name(struct btrfs_inode *dir, struct dentry *dentry,
                 return -ENOMEM;

         ret = fscrypt_setup_filename(&dir->vfs_inode, &dentry->d_name, 1, &fname);
-       if (ret)
+       if (ret < 0)
                 goto out;

Presumably gcc assumes that fscrypt_setup_filename() could return
a positive value.

Guenter

Re: [PATCH 8/8] btrfs: turn on -Wmaybe-uninitialized

[Guenter Roeck]

Copying regzbot.

#regzbot ^introduced 1ec49744ba83
#regzbot title Build failures for sparc64:allmodconfig and parisc:allmodconfig with gcc 11.x+

On Tue, Feb 21, 2023 at 06:59:21PM -0800, Guenter Roeck wrote:
> On Fri, Dec 16, 2022 at 03:15:58PM -0500, Josef Bacik wrote:
> > We had a recent bug that would have been caught by a newer compiler with
> > -Wmaybe-uninitialized and would have saved us a month of failing tests
> > that I didn't have time to investigate.
> > 
> 
> Thanks to this patch, sparc64:allmodconfig and parisc:allmodconfig now
> fail to commpile with the following error when trying to build images
> with gcc 11.3.
> 
> Building sparc64:allmodconfig ... failed
> --------------
> Error log:
> <stdin>:1517:2: warning: #warning syscall clone3 not implemented [-Wcpp]
> fs/btrfs/inode.c: In function 'btrfs_lookup_dentry':
> fs/btrfs/inode.c:5730:21: error: 'location.type' may be used uninitialized [-Werror=maybe-uninitialized]
>  5730 |         if (location.type == BTRFS_INODE_ITEM_KEY) {
>       |             ~~~~~~~~^~~~~
> fs/btrfs/inode.c:5719:26: note: 'location' declared here
>  5719 |         struct btrfs_key location;
> 
> Bisect log attached.
> 
> Guenter
> 
> ---
> # bad: [8bf1a529cd664c8e5268381f1e24fe67aa611dd3] Merge tag 'arm64-upstream' of git://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux
> # good: [c9c3395d5e3dcc6daee66c6908354d47bf98cb0c] Linux 6.2
> git bisect start 'HEAD' 'v6.2'
> # good: [e43efb6d713bca3855909a2f9caec280a2b0a503] dt-bindings: riscv: correct starfive visionfive 2 compatibles
> git bisect good e43efb6d713bca3855909a2f9caec280a2b0a503
> # bad: [1f2d9ffc7a5f916935749ffc6e93fb33bfe94d2f] Merge tag 'sched-core-2023-02-20' of git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip
> git bisect bad 1f2d9ffc7a5f916935749ffc6e93fb33bfe94d2f
> # bad: [553637f73c314c742243b8dc5ef072e9dadbe581] Merge tag 'for-6.3/dio-2023-02-16' of git://git.kernel.dk/linux
> git bisect bad 553637f73c314c742243b8dc5ef072e9dadbe581
> # good: [274978f173276c5720a3cd8d0b6047d2c0d3a684] Merge tag 'fixes_for_v6.3-rc1' of git://git.kernel.org/pub/scm/linux/kernel/git/jack/linux-fs
> git bisect good 274978f173276c5720a3cd8d0b6047d2c0d3a684
> # bad: [801fcfc5d790f4a9be2897713bd6dd08bed253f1] btrfs: raid56: add a bio_list_put helper
> git bisect bad 801fcfc5d790f4a9be2897713bd6dd08bed253f1
> # bad: [e2fd83064a9bae368ce1c88a0cb9aee64ad4e124] btrfs: skip backref walking during fiemap if we know the leaf is shared
> git bisect bad e2fd83064a9bae368ce1c88a0cb9aee64ad4e124
> # bad: [cb0922f264643f03b04352f7a04abb913c609369] btrfs: don't use size classes for zoned file systems
> git bisect bad cb0922f264643f03b04352f7a04abb913c609369
> # good: [cd30d3bc78d9acbd505d0d6a4cff3b87e40a8187] btrfs: zoned: fix uninitialized variable warning in btrfs_get_dev_zones
> git bisect good cd30d3bc78d9acbd505d0d6a4cff3b87e40a8187
> # bad: [235e1c7b872f9cf16e8a3e6050a05774b8763c58] btrfs: use a single variable to track return value for log_dir_items()
> git bisect bad 235e1c7b872f9cf16e8a3e6050a05774b8763c58
> # bad: [d31de3785047a24959eda835b0bafb1f8629f8a9] btrfs: go to matching label when cleaning em in btrfs_submit_direct
> git bisect bad d31de3785047a24959eda835b0bafb1f8629f8a9
> # bad: [1ec49744ba83f0429c5c706708610f7821a7b6f4] btrfs: turn on -Wmaybe-uninitialized
> git bisect bad 1ec49744ba83f0429c5c706708610f7821a7b6f4
> # good: [a6ca692ec22bdac5ae700e82ff575a1b5141fa40] btrfs: fix uninitialized variable warning in run_one_async_start
> git bisect good a6ca692ec22bdac5ae700e82ff575a1b5141fa40
> # first bad commit: [1ec49744ba83f0429c5c706708610f7821a7b6f4] btrfs: turn on -Wmaybe-uninitialized

Re: [PATCH 8/8] btrfs: turn on -Wmaybe-uninitialized

[Linux regression tracking (Thorsten Leemhuis)]

On 22.02.23 18:18, Guenter Roeck wrote:
> On 2/22/23 08:38, David Sterba wrote:
>> On Tue, Feb 21, 2023 at 06:59:18PM -0800, Guenter Roeck wrote:
>>> On Fri, Dec 16, 2022 at 03:15:58PM -0500, Josef Bacik wrote:
>>>> We had a recent bug that would have been caught by a newer compiler
>>>> with
>>>> -Wmaybe-uninitialized and would have saved us a month of failing tests
>>>> that I didn't have time to investigate.
>>>>
>>>
>>> Thanks to this patch, sparc64:allmodconfig and parisc:allmodconfig now
>>> fail to commpile with the following error when trying to build images
>>> with gcc 11.3.
>>>
>>> Building sparc64:allmodconfig ... failed
>>> --------------
>>> Error log:
>>> <stdin>:1517:2: warning: #warning syscall clone3 not implemented [-Wcpp]
>>> fs/btrfs/inode.c: In function 'btrfs_lookup_dentry':
>>> fs/btrfs/inode.c:5730:21: error: 'location.type' may be used
>>> uninitialized [-Werror=maybe-uninitialized]
>>>   5730 |         if (location.type == BTRFS_INODE_ITEM_KEY) {
>>>        |             ~~~~~~~~^~~~~
>>> fs/btrfs/inode.c:5719:26: note: 'location' declared here
>>>   5719 |         struct btrfs_key location;
>>
>> Thanks for the report, Linus warned me that there might be some fallouts
>> and that the warning flag might need reverted. But before I do that I'd
>> like to try to understand why the warnings happen in a code where is no
>> reason for it.
>>
>> I did a quick test on gcc 11.3 (on x86_64, not on sparc64 unlike you
>> report), and there is no warning
>>
>> gcc (SUSE Linux) 11.3.1 20220721 [revision
>> a55184ada8e2887ca94c0ab07027617885beafc9]
>> Copyright (C) 2021 Free Software Foundation, Inc.
>> This is free software; see the source for copying conditions.  There
>> is NO
>> warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR
>> PURPOSE.
>>
>>    DESCEND objtool
>>    CALL    scripts/checksyscalls.sh
>>    CC [M]  fs/btrfs/inode.o
>>
>> I.e. it's the same version, different arch and likely not the same
>> config. In the function itself thre's a local variable passed by address
>> to a static function in the same file.
>>
>>     struct btrfs_key location;
>>     ...
>>     ret = btrfs_inode_by_name(BTRFS_I(dir), dentry, &location, &di_type);
>>
>> and there it's
>>
>>     btrfs_dir_item_key_to_cpu(path->nodes[0], di, location);
>>
>> which is a series of helpers to read some data and store that to the
>> strucutre. At some point there's a call to read_extent_buffer() that's
>> in a different file.
>>
>> A local variable passed by address to external function is quite common
>> so I'd expect more warnings and I don't see what's different in this
>> case.
> 
> Me not either. I also don't see the problem with other architectures, only
> with sparc and parisc. It doesn't have to be gcc 11.3, though; it also
> happens
> with gcc 11.1, 11.2, 12.1, and 12.2 (tested on sparc).
> 
> Too bad that gcc doesn't tell why exactly it believes that the object
> may be uninitialized. Anyway, the following change would fix the problem.
> 
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 6c18dc9a1831..4bab8ab39948 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -5421,7 +5421,7 @@ static int btrfs_inode_by_name(struct btrfs_inode
> *dir, struct dentry *dentry,
>                 return -ENOMEM;
> 
>         ret = fscrypt_setup_filename(&dir->vfs_inode, &dentry->d_name,
> 1, &fname);
> -       if (ret)
> +       if (ret < 0)
>                 goto out;
> 
> Presumably gcc assumes that fscrypt_setup_filename() could return
> a positive value.

This discussion seems to have stalled, but from a kernelci report it
looks like above warning still happens:
https://lore.kernel.org/all/640bceb7.a70a0220.af8cd.146b@mx.google.com/

@btrfs developers, do you still have it on your radar?

Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
--
Everything you wanna know about Linux kernel regression tracking:
https://linux-regtracking.leemhuis.info/about/#tldr
If I did something stupid, please tell me, as explained on that page.

#regzbot poke

Re: [PATCH 8/8] btrfs: turn on -Wmaybe-uninitialized

[Guenter Roeck]

On 3/12/23 06:06, Linux regression tracking (Thorsten Leemhuis) wrote:
> On 22.02.23 18:18, Guenter Roeck wrote:
>> On 2/22/23 08:38, David Sterba wrote:
>>> On Tue, Feb 21, 2023 at 06:59:18PM -0800, Guenter Roeck wrote:
>>>> On Fri, Dec 16, 2022 at 03:15:58PM -0500, Josef Bacik wrote:
>>>>> We had a recent bug that would have been caught by a newer compiler
>>>>> with
>>>>> -Wmaybe-uninitialized and would have saved us a month of failing tests
>>>>> that I didn't have time to investigate.
>>>>>
>>>>
>>>> Thanks to this patch, sparc64:allmodconfig and parisc:allmodconfig now
>>>> fail to commpile with the following error when trying to build images
>>>> with gcc 11.3.
>>>>
>>>> Building sparc64:allmodconfig ... failed
>>>> --------------
>>>> Error log:
>>>> <stdin>:1517:2: warning: #warning syscall clone3 not implemented [-Wcpp]
>>>> fs/btrfs/inode.c: In function 'btrfs_lookup_dentry':
>>>> fs/btrfs/inode.c:5730:21: error: 'location.type' may be used
>>>> uninitialized [-Werror=maybe-uninitialized]
>>>>    5730 |         if (location.type == BTRFS_INODE_ITEM_KEY) {
>>>>         |             ~~~~~~~~^~~~~
>>>> fs/btrfs/inode.c:5719:26: note: 'location' declared here
>>>>    5719 |         struct btrfs_key location;
>>>
>>> Thanks for the report, Linus warned me that there might be some fallouts
>>> and that the warning flag might need reverted. But before I do that I'd
>>> like to try to understand why the warnings happen in a code where is no
>>> reason for it.
>>>
>>> I did a quick test on gcc 11.3 (on x86_64, not on sparc64 unlike you
>>> report), and there is no warning
>>>
>>> gcc (SUSE Linux) 11.3.1 20220721 [revision
>>> a55184ada8e2887ca94c0ab07027617885beafc9]
>>> Copyright (C) 2021 Free Software Foundation, Inc.
>>> This is free software; see the source for copying conditions.  There
>>> is NO
>>> warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR
>>> PURPOSE.
>>>
>>>     DESCEND objtool
>>>     CALL    scripts/checksyscalls.sh
>>>     CC [M]  fs/btrfs/inode.o
>>>
>>> I.e. it's the same version, different arch and likely not the same
>>> config. In the function itself thre's a local variable passed by address
>>> to a static function in the same file.
>>>
>>>      struct btrfs_key location;
>>>      ...
>>>      ret = btrfs_inode_by_name(BTRFS_I(dir), dentry, &location, &di_type);
>>>
>>> and there it's
>>>
>>>      btrfs_dir_item_key_to_cpu(path->nodes[0], di, location);
>>>
>>> which is a series of helpers to read some data and store that to the
>>> strucutre. At some point there's a call to read_extent_buffer() that's
>>> in a different file.
>>>
>>> A local variable passed by address to external function is quite common
>>> so I'd expect more warnings and I don't see what's different in this
>>> case.
>>
>> Me not either. I also don't see the problem with other architectures, only
>> with sparc and parisc. It doesn't have to be gcc 11.3, though; it also
>> happens
>> with gcc 11.1, 11.2, 12.1, and 12.2 (tested on sparc).
>>
>> Too bad that gcc doesn't tell why exactly it believes that the object
>> may be uninitialized. Anyway, the following change would fix the problem.
>>
>> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
>> index 6c18dc9a1831..4bab8ab39948 100644
>> --- a/fs/btrfs/inode.c
>> +++ b/fs/btrfs/inode.c
>> @@ -5421,7 +5421,7 @@ static int btrfs_inode_by_name(struct btrfs_inode
>> *dir, struct dentry *dentry,
>>                  return -ENOMEM;
>>
>>          ret = fscrypt_setup_filename(&dir->vfs_inode, &dentry->d_name,
>> 1, &fname);
>> -       if (ret)
>> +       if (ret < 0)
>>                  goto out;
>>
>> Presumably gcc assumes that fscrypt_setup_filename() could return
>> a positive value.
> 
> This discussion seems to have stalled, but from a kernelci report it
> looks like above warning still happens:
> https://lore.kernel.org/all/640bceb7.a70a0220.af8cd.146b@mx.google.com/
> 
> @btrfs developers, do you still have it on your radar?
> 
> Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
> --
> Everything you wanna know about Linux kernel regression tracking:
> https://linux-regtracking.leemhuis.info/about/#tldr
> If I did something stupid, please tell me, as explained on that page.
> 
> #regzbot poke

There was a patch:

#regzbot monitor: https://patchwork.kernel.org/project/linux-btrfs/patch/dc091588d770923c3afe779e1eb512724662db71.1678290988.git.sweettea-kernel@dorminy.me/
#regzbot fix: btrfs: fix compilation error on sparc/parisc
#regzbot ignore-activity

Guenter

Re: [PATCH 8/8] btrfs: turn on -Wmaybe-uninitialized

[Linux regression tracking (Thorsten Leemhuis)]

On 12.03.23 15:37, Guenter Roeck wrote:
> On 3/12/23 06:06, Linux regression tracking (Thorsten Leemhuis) wrote:
>> On 22.02.23 18:18, Guenter Roeck wrote:
>>> On 2/22/23 08:38, David Sterba wrote:
>>>> On Tue, Feb 21, 2023 at 06:59:18PM -0800, Guenter Roeck wrote:
>>>>> On Fri, Dec 16, 2022 at 03:15:58PM -0500, Josef Bacik wrote:
>>>>>> We had a recent bug that would have been caught by a newer compiler
>>>>>> with
>>>>>> -Wmaybe-uninitialized and would have saved us a month of failing
>>>>>> tests
>>>>>> that I didn't have time to investigate.
>>>>>>
>>>>>
>>>>> Thanks to this patch, sparc64:allmodconfig and parisc:allmodconfig now
>>>>> fail to commpile with the following error when trying to build images
>>>>> with gcc 11.3.
>>>>>
>>>>> Building sparc64:allmodconfig ... failed
>>>>> --------------
>>>>> Error log:
>>>>> <stdin>:1517:2: warning: #warning syscall clone3 not implemented
>>>>> [-Wcpp]
>>>>> fs/btrfs/inode.c: In function 'btrfs_lookup_dentry':
>>>>> fs/btrfs/inode.c:5730:21: error: 'location.type' may be used
>>>>> uninitialized [-Werror=maybe-uninitialized]
>>>>>    5730 |         if (location.type == BTRFS_INODE_ITEM_KEY) {
>>>>>         |             ~~~~~~~~^~~~~
>>>>> fs/btrfs/inode.c:5719:26: note: 'location' declared here
>>>>>    5719 |         struct btrfs_key location;
>>>>
>>>> Thanks for the report, Linus warned me that there might be some
>>>> fallouts
>>>> and that the warning flag might need reverted. But before I do that I'd
>>>> like to try to understand why the warnings happen in a code where is no
>>>> reason for it.
>>>>
>>>> I did a quick test on gcc 11.3 (on x86_64, not on sparc64 unlike you
>>>> report), and there is no warning
>>>>
>>>> gcc (SUSE Linux) 11.3.1 20220721 [revision
>>>> a55184ada8e2887ca94c0ab07027617885beafc9]
>>>> Copyright (C) 2021 Free Software Foundation, Inc.
>>>> This is free software; see the source for copying conditions.  There
>>>> is NO
>>>> warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR
>>>> PURPOSE.
>>>>
>>>>     DESCEND objtool
>>>>     CALL    scripts/checksyscalls.sh
>>>>     CC [M]  fs/btrfs/inode.o
>>>>
>>>> I.e. it's the same version, different arch and likely not the same
>>>> config. In the function itself thre's a local variable passed by
>>>> address
>>>> to a static function in the same file.
>>>>
>>>>      struct btrfs_key location;
>>>>      ...
>>>>      ret = btrfs_inode_by_name(BTRFS_I(dir), dentry, &location,
>>>> &di_type);
>>>>
>>>> and there it's
>>>>
>>>>      btrfs_dir_item_key_to_cpu(path->nodes[0], di, location);
>>>>
>>>> which is a series of helpers to read some data and store that to the
>>>> strucutre. At some point there's a call to read_extent_buffer() that's
>>>> in a different file.
>>>>
>>>> A local variable passed by address to external function is quite common
>>>> so I'd expect more warnings and I don't see what's different in this
>>>> case.
>>>
>>> Me not either. I also don't see the problem with other architectures,
>>> only
>>> with sparc and parisc. It doesn't have to be gcc 11.3, though; it also
>>> happens
>>> with gcc 11.1, 11.2, 12.1, and 12.2 (tested on sparc).
>>>
>>> Too bad that gcc doesn't tell why exactly it believes that the object
>>> may be uninitialized. Anyway, the following change would fix the
>>> problem.
>>>
>>> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
>>> index 6c18dc9a1831..4bab8ab39948 100644
>>> --- a/fs/btrfs/inode.c
>>> +++ b/fs/btrfs/inode.c
>>> @@ -5421,7 +5421,7 @@ static int btrfs_inode_by_name(struct btrfs_inode
>>> *dir, struct dentry *dentry,
>>>                  return -ENOMEM;
>>>
>>>          ret = fscrypt_setup_filename(&dir->vfs_inode, &dentry->d_name,
>>> 1, &fname);
>>> -       if (ret)
>>> +       if (ret < 0)
>>>                  goto out;
>>>
>>> Presumably gcc assumes that fscrypt_setup_filename() could return
>>> a positive value.
>>
>> This discussion seems to have stalled, but from a kernelci report it
>> looks like above warning still happens:
>> https://lore.kernel.org/all/640bceb7.a70a0220.af8cd.146b@mx.google.com/
>>
>> @btrfs developers, do you still have it on your radar?
>>
>> Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
>> -- 
>> Everything you wanna know about Linux kernel regression tracking:
>> https://linux-regtracking.leemhuis.info/about/#tldr
>> If I did something stupid, please tell me, as explained on that page.
>>
>> #regzbot poke
> 
> There was a patch:
> 
> #regzbot monitor:
> https://patchwork.kernel.org/project/linux-btrfs/patch/dc091588d770923c3afe779e1eb512724662db71.1678290988.git.sweettea-kernel@dorminy.me/
> #regzbot fix: btrfs: fix compilation error on sparc/parisc
> #regzbot ignore-activity

Ahh, great, thx.

Ciao, Thorsten

Re: [PATCH 8/8] btrfs: turn on -Wmaybe-uninitialized

[David Sterba]

On Sun, Mar 12, 2023 at 02:06:40PM +0100, Linux regression tracking (Thorsten Leemhuis) wrote:
> On 22.02.23 18:18, Guenter Roeck wrote:
> > On 2/22/23 08:38, David Sterba wrote:
> >> On Tue, Feb 21, 2023 at 06:59:18PM -0800, Guenter Roeck wrote:
> >>> On Fri, Dec 16, 2022 at 03:15:58PM -0500, Josef Bacik wrote:
> This discussion seems to have stalled, but from a kernelci report it
> looks like above warning still happens:
> https://lore.kernel.org/all/640bceb7.a70a0220.af8cd.146b@mx.google.com/
> 
> @btrfs developers, do you still have it on your radar?

I'm aware of the warnings and that it's caused by enabling the
-Wmaybe-uninitialized warning. One has a patch, IIRC there are 2-3 more,
so either there's a fix or the commit enabling the warning will be
reverted before 6.3 final.

Re: [PATCH 8/8] btrfs: turn on -Wmaybe-uninitialized

[Linux regression tracking #update (Thorsten Leemhuis)]

[TLDR: This mail in primarily relevant for Linux kernel regression
tracking. See link in footer if these mails annoy you.]

On 12.03.23 15:37, Guenter Roeck wrote:
> On 3/12/23 06:06, Linux regression tracking (Thorsten Leemhuis) wrote:
>> On 22.02.23 18:18, Guenter Roeck wrote:
>>> On 2/22/23 08:38, David Sterba wrote:
>>>> On Tue, Feb 21, 2023 at 06:59:18PM -0800, Guenter Roeck wrote:
>>>>> On Fri, Dec 16, 2022 at 03:15:58PM -0500, Josef Bacik wrote:
>>>>>> We had a recent bug that would have been caught by a newer compiler
>>>>>> with
>>>>>> -Wmaybe-uninitialized and would have saved us a month of failing
>>>>>> tests
>>>>>> that I didn't have time to investigate.
>>>>>>
>>>>>
>>>>> Thanks to this patch, sparc64:allmodconfig and parisc:allmodconfig now
>>>>> fail to commpile with the following error when trying to build images
>>>>> with gcc 11.3.
>>>>>
>>>>> Building sparc64:allmodconfig ... failed


> There was a patch:
> 
> #regzbot monitor:
> https://patchwork.kernel.org/project/linux-btrfs/patch/dc091588d770923c3afe779e1eb512724662db71.1678290988.git.sweettea-kernel@dorminy.me/
> #regzbot fix: btrfs: fix compilation error on sparc/parisc
> #regzbot ignore-activity

Thx for this, Guenter, unfortunately the fix got a different subject
when it was applied. Happens, no worries, but regzbot thus needs to get
told manually:

#regzbot fix: 10a8857a1beaa015efba7d56e06243d484549fb6
#regzbot ignore-activity

Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
--
Everything you wanna know about Linux kernel regression tracking:
https://linux-regtracking.leemhuis.info/about/#tldr
That page also explains what to do if mails like this annoy you.