]> git.ipfire.org Git - thirdparty/linux.git/commitdiff
btrfs: make found_logical_ret parameter mandatory for function queue_scrub_stripe()
authorQu Wenruo <wqu@suse.com>
Sat, 28 Oct 2023 02:58:45 +0000 (13:28 +1030)
committerDavid Sterba <dsterba@suse.com>
Fri, 3 Nov 2023 15:40:15 +0000 (16:40 +0100)
[BUG]
There is a compilation 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/
Fixes: ae76d8e3e135 ("btrfs: scrub: fix grouping of read IO")
Reviewed-by: Anand Jain <anand.jain@oracle.com>
Signed-off-by: Qu Wenruo <wqu@suse.com>
Reviewed-by: David Sterba <dsterba@suse.com>
Signed-off-by: David Sterba <dsterba@suse.com>
fs/btrfs/scrub.c

index 9ce5be21b0360107d1a2416f06fa0cbfd1710030..f62a408671cbc3ba06bf244bb12de96bdfb4334a 100644 (file)
@@ -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,8 @@ 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 */