On 2023/10/27 17:30, Qu Wenruo wrote:
On 2023/10/27 17:25, Holger Hoffstätte wrote:
On 2023-10-26 23:01, Qu Wenruo wrote:
On 2023/10/27 00:30, Holger Hoffstätte wrote:
On 2023-10-26 15:31, Sam James wrote:
'btrfs: scrub: fix grouping of read IO' seems to intorduce a -Wmaybe-uninitialized warning (which becomes fatal with the kernel's passed -Werror=...) with 6.5.9:
/var/tmp/portage/sys-kernel/gentoo-kernel-6.5.9/work/linux-6.5/fs/btrfs/scrub.c: In function ‘scrub_simple_mirror.isra’: /var/tmp/portage/sys-kernel/gentoo-kernel-6.5.9/work/linux-6.5/fs/btrfs/scrub.c:2075:29: error: ‘found_logical’ may be used uninitialized [-Werror=maybe-uninitialized[https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wmaybe-uninitialized]] 2075 | cur_logical = found_logical + BTRFS_STRIPE_LEN; /var/tmp/portage/sys-kernel/gentoo-kernel-6.5.9/work/linux-6.5/fs/btrfs/scrub.c:2040:21: note: ‘found_logical’ was declared here 2040 | u64 found_logical; | ^~~~~~~~~~~~~Good find! found_logical is passed by reference to queue_scrub_stripe(..) (inlined) where it is used without ever being set:
... /* Either >0 as no more extents or <0 for error. */ if (ret) return ret; if (found_logical_ret) *found_logical_ret = stripe->logical; sctx->cur_stripe++; ...
Something is missing here, and somehow I don't think it's just the top-level initialisation of found_logical.
This looks like a false alert for me.
@found_logical is intentionally uninitialized to catch any uninitialized usage by compiler.
It would be set by queue_scrub_stripe() when there is any stripe found.
Can you show me where the reference is set before the quoted if block?
Sure.
Firstly inside queue_scrub_stripe():
ret = scrub_find_fill_first_stripe(bg, &sctx->extent_path, &sctx->csum_path, dev, physical, mirror_num, logical, length, stripe); /* Either >0 as no more extents or <0 for error. */ if (ret) return ret; if (found_logical_ret) *found_logical_ret = stripe->logical;In this case, we would only set @found_logical_ret to the found stripe's logical.
Either we got ret > 0, meaning no more extent in the chunk, or we got some critical error.
Sorry, missing the latter part of the sentence:
In either case, the caller would handle it.
Then back to scrub_simple_mirrors():
ret = queue_scrub_stripe(sctx, bg, device, mirror_num, cur_logical, logical_end - cur_logical, cur_physical, &found_logical); if (ret > 0) { /* No more extent, just update the accounting */ sctx->stat.last_physical = physical + logical_length; ret = 0; break; } if (ret < 0) break; cur_logical = found_logical + BTRFS_STRIPE_LEN;We got to the if block I mentioned.
Either we got ret != 0, then we would break out the whole loop of scrub_simple_mirror(). Or we got ret == 0, which would initialize @found_logical.
Thanks, Qu
The warning happens because according to the compiler this is exactly not what's happening, and I did not see any initializing write either.
thanks, Holger