| Message ID | 3223c8646dd74cc0252e3b619a7a2cd6b078d85a.1693930391.git.josef@toxicpanda.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix some -Wmaybe-uninitialized errors | expand |
Tested-by: Jens Axboe <axboe@kernel.dk>
On 2023/9/6 00:15, Josef Bacik wrote: > Jens reported a compiler error when using CONFIG_CC_OPTIMIZE_FOR_SIZE=y > that looks like this > > In function ‘gather_device_info’, > inlined from ‘btrfs_create_chunk’ at fs/btrfs/volumes.c:5507:8: > fs/btrfs/volumes.c:5245:48: warning: ‘dev_offset’ may be used uninitialized [-Wmaybe-uninitialized] > 5245 | devices_info[ndevs].dev_offset = dev_offset; > | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~ > fs/btrfs/volumes.c: In function ‘btrfs_create_chunk’: > fs/btrfs/volumes.c:5196:13: note: ‘dev_offset’ was declared here > 5196 | u64 dev_offset; > > This occurs because find_free_dev_extent is responsible for setting > dev_offset, however if we get an -ENOMEM at the top of the function > we'll return without setting the value. > > This isn't actually a problem because we will see the -ENOMEM in > gather_device_info() and return and not use the uninitialized value, > however we also just don't want the compiler warning so rework the code > slightly in find_free_dev_extent() to make sure it's always setting > *start and *len to avoid the compiler error. > > Reported-by: Jens Axboe <axboe@kernel.dk> > Signed-off-by: Josef Bacik <josef@toxicpanda.com> Reviewed-by: Qu Wenruo <wqu@suse.com> Just one nitpick below. > --- > fs/btrfs/volumes.c | 14 ++++++-------- > 1 file changed, 6 insertions(+), 8 deletions(-) > > diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c > index 4b0e441227b2..08dba911212c 100644 > --- a/fs/btrfs/volumes.c > +++ b/fs/btrfs/volumes.c > @@ -1594,25 +1594,23 @@ static int find_free_dev_extent(struct btrfs_device *device, u64 num_bytes, > u64 search_start; > u64 hole_size; > u64 max_hole_start; > - u64 max_hole_size; > + u64 max_hole_size = 0; > u64 extent_end; > u64 search_end = device->total_bytes; > int ret; > int slot; > struct extent_buffer *l; > > - search_start = dev_extent_search_start(device); > + max_hole_start = search_start = dev_extent_search_start(device); IIRC we don't recommend such assignment, it would be better to go two lines to do the assignment. Thanks, Qu > > WARN_ON(device->zone_info && > !IS_ALIGNED(num_bytes, device->zone_info->zone_size)); > > path = btrfs_alloc_path(); > - if (!path) > - return -ENOMEM; > - > - max_hole_start = search_start; > - max_hole_size = 0; > - > + if (!path) { > + ret = -ENOMEM; > + goto out; > + } > again: > if (search_start >= search_end || > test_bit(BTRFS_DEV_STATE_REPLACE_TGT, &device->dev_state)) {
On Wed, Sep 06, 2023 at 06:48:41AM +0800, Qu Wenruo wrote: > > - search_start = dev_extent_search_start(device); > > + max_hole_start = search_start = dev_extent_search_start(device); > > IIRC we don't recommend such assignment, it would be better to go two > lines to do the assignment. Right, fixed in the commit, thanks.
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c index 4b0e441227b2..08dba911212c 100644 --- a/fs/btrfs/volumes.c +++ b/fs/btrfs/volumes.c @@ -1594,25 +1594,23 @@ static int find_free_dev_extent(struct btrfs_device *device, u64 num_bytes, u64 search_start; u64 hole_size; u64 max_hole_start; - u64 max_hole_size; + u64 max_hole_size = 0; u64 extent_end; u64 search_end = device->total_bytes; int ret; int slot; struct extent_buffer *l; - search_start = dev_extent_search_start(device); + max_hole_start = search_start = dev_extent_search_start(device); WARN_ON(device->zone_info && !IS_ALIGNED(num_bytes, device->zone_info->zone_size)); path = btrfs_alloc_path(); - if (!path) - return -ENOMEM; - - max_hole_start = search_start; - max_hole_size = 0; - + if (!path) { + ret = -ENOMEM; + goto out; + } again: if (search_start >= search_end || test_bit(BTRFS_DEV_STATE_REPLACE_TGT, &device->dev_state)) {
Jens reported a compiler error when using CONFIG_CC_OPTIMIZE_FOR_SIZE=y that looks like this In function ‘gather_device_info’, inlined from ‘btrfs_create_chunk’ at fs/btrfs/volumes.c:5507:8: fs/btrfs/volumes.c:5245:48: warning: ‘dev_offset’ may be used uninitialized [-Wmaybe-uninitialized] 5245 | devices_info[ndevs].dev_offset = dev_offset; | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~ fs/btrfs/volumes.c: In function ‘btrfs_create_chunk’: fs/btrfs/volumes.c:5196:13: note: ‘dev_offset’ was declared here 5196 | u64 dev_offset; This occurs because find_free_dev_extent is responsible for setting dev_offset, however if we get an -ENOMEM at the top of the function we'll return without setting the value. This isn't actually a problem because we will see the -ENOMEM in gather_device_info() and return and not use the uninitialized value, however we also just don't want the compiler warning so rework the code slightly in find_free_dev_extent() to make sure it's always setting *start and *len to avoid the compiler error. Reported-by: Jens Axboe <axboe@kernel.dk> Signed-off-by: Josef Bacik <josef@toxicpanda.com> --- fs/btrfs/volumes.c | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-)