| Message ID | 4da50284fed071fea6d629f09d318f70a4e42c47.1698461922.git.wqu@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: make found_logical_ret parameter mandatory for function queue_scrub_stripe() | expand |
On 10/28/23 10:58, Qu Wenruo wrote: > [BUG] > There is a compiling warning reported on commit ae76d8e3e135 ("btrfs: > scrub: fix grouping of read IO"), where gcc (14.0.0 20231022 experimental) > is reporting the following uninitialized variable: > > fs/btrfs/scrub.c: In function ‘scrub_simple_mirror.isra’: > 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; > fs/btrfs/scrub.c:2040:21: note: ‘found_logical’ was declared here > 2040 | u64 found_logical; > | ^~~~~~~~~~~~~ > > [CAUSE] > This is a false alert, as @found_logical is passed as parameter > @found_logical_ret of function queue_scrub_stripe(). > > As long as queue_scrub_stripe() returned 0, we would update > @found_logical_ret. > And if queue_scrub_stripe() returned >0 or <0, the caller would not > utilized @found_logical, thus there should be nothing wrong. > > Although the triggering gcc is still experimental, it looks like the > extra check on "if (found_logical_ret)" can sometimes confuse the > compiler. > > Meanwhile the only caller of queue_scrub_stripe() is always passing a > valid pointer, there is no need for such check at all. > > [FIX] > Although the report itself is a false alert, we can still make it more > explicit by: > > - Replace the check for @found_logical_ret with ASSERT() > > - Initialize @found_logical to U64_MAX > > - Add one extra ASSERT() to make sure @found_logical got updated > > Link: https://lore.kernel.org/linux-btrfs/87fs1x1p93.fsf@gentoo.org/ > Signed-off-by: Qu Wenruo <wqu@suse.com> Nit: Pls add. Fixes: ae76d8e3e135 ... Changes looks good. Reviewed-by: Anand Jain <anand.jain@oracle.com>
On Sat, Oct 28, 2023 at 01:28:45PM +1030, Qu Wenruo wrote: > [BUG] > There is a compiling warning reported on commit ae76d8e3e135 ("btrfs: > scrub: fix grouping of read IO"), where gcc (14.0.0 20231022 experimental) > is reporting the following uninitialized variable: > > fs/btrfs/scrub.c: In function ‘scrub_simple_mirror.isra’: > 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; > fs/btrfs/scrub.c:2040:21: note: ‘found_logical’ was declared here > 2040 | u64 found_logical; > | ^~~~~~~~~~~~~ > > [CAUSE] > This is a false alert, as @found_logical is passed as parameter > @found_logical_ret of function queue_scrub_stripe(). > > As long as queue_scrub_stripe() returned 0, we would update > @found_logical_ret. > And if queue_scrub_stripe() returned >0 or <0, the caller would not > utilized @found_logical, thus there should be nothing wrong. > > Although the triggering gcc is still experimental, it looks like the > extra check on "if (found_logical_ret)" can sometimes confuse the > compiler. > > Meanwhile the only caller of queue_scrub_stripe() is always passing a > valid pointer, there is no need for such check at all. > > [FIX] > Although the report itself is a false alert, we can still make it more > explicit by: > > - Replace the check for @found_logical_ret with ASSERT() > > - Initialize @found_logical to U64_MAX > > - Add one extra ASSERT() to make sure @found_logical got updated > > Link: https://lore.kernel.org/linux-btrfs/87fs1x1p93.fsf@gentoo.org/ > Signed-off-by: Qu Wenruo <wqu@suse.com> Added to misc-next, thanks.
diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c index 9ce5be21b036..e6160e8dc39d 100644 --- a/fs/btrfs/scrub.c +++ b/fs/btrfs/scrub.c @@ -1868,6 +1868,9 @@ static int queue_scrub_stripe(struct scrub_ctx *sctx, struct btrfs_block_group * */ ASSERT(sctx->cur_stripe < SCRUB_TOTAL_STRIPES); + /* @found_logical_ret must be specified. */ + ASSERT(found_logical_ret); + stripe = &sctx->stripes[sctx->cur_stripe]; scrub_reset_stripe(stripe); ret = scrub_find_fill_first_stripe(bg, &sctx->extent_path, @@ -1876,8 +1879,7 @@ static int queue_scrub_stripe(struct scrub_ctx *sctx, struct btrfs_block_group * /* Either >0 as no more extents or <0 for error. */ if (ret) return ret; - if (found_logical_ret) - *found_logical_ret = stripe->logical; + *found_logical_ret = stripe->logical; sctx->cur_stripe++; /* We filled one group, submit it. */ @@ -2080,7 +2082,7 @@ static int scrub_simple_mirror(struct scrub_ctx *sctx, /* Go through each extent items inside the logical range */ while (cur_logical < logical_end) { - u64 found_logical; + u64 found_logical = U64_MAX; u64 cur_physical = physical + cur_logical - logical_start; /* Canceled? */ @@ -2115,6 +2117,11 @@ static int scrub_simple_mirror(struct scrub_ctx *sctx, if (ret < 0) break; + /* + * queue_scrub_stripe() returned 0, @found_logical must be + * updated. + */ + ASSERT(found_logical != U64_MAX); cur_logical = found_logical + BTRFS_STRIPE_LEN; /* Don't hold CPU for too long time */
[BUG] There is a compiling warning reported on commit ae76d8e3e135 ("btrfs: scrub: fix grouping of read IO"), where gcc (14.0.0 20231022 experimental) is reporting the following uninitialized variable: fs/btrfs/scrub.c: In function ‘scrub_simple_mirror.isra’: 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; fs/btrfs/scrub.c:2040:21: note: ‘found_logical’ was declared here 2040 | u64 found_logical; | ^~~~~~~~~~~~~ [CAUSE] This is a false alert, as @found_logical is passed as parameter @found_logical_ret of function queue_scrub_stripe(). As long as queue_scrub_stripe() returned 0, we would update @found_logical_ret. And if queue_scrub_stripe() returned >0 or <0, the caller would not utilized @found_logical, thus there should be nothing wrong. Although the triggering gcc is still experimental, it looks like the extra check on "if (found_logical_ret)" can sometimes confuse the compiler. Meanwhile the only caller of queue_scrub_stripe() is always passing a valid pointer, there is no need for such check at all. [FIX] Although the report itself is a false alert, we can still make it more explicit by: - Replace the check for @found_logical_ret with ASSERT() - Initialize @found_logical to U64_MAX - Add one extra ASSERT() to make sure @found_logical got updated Link: https://lore.kernel.org/linux-btrfs/87fs1x1p93.fsf@gentoo.org/ Signed-off-by: Qu Wenruo <wqu@suse.com> --- fs/btrfs/scrub.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-)