Re: [PATCH 6.5 211/285] btrfs: scrub: fix grouping of read IO

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





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.

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




[Index of Archives]     [Linux Kernel]     [Kernel Development Newbies]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Hiking]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux