[PATCH 0/2] Fix some -Wmaybe-uninitialized errors

[PATCH 0/2] Fix some -Wmaybe-uninitialized errors

[Josef Bacik]

Hello,

Jens reported to me two warnings he was seeing with
CONFIG_CC_OPTIMIZE_FOR_SIZE=y set in his .config.  We don't see these errors
without this option, and they're not correct warnings.  However we've had issues
where -Wmaybe-uninitialized would have caught a real bug, so this warning is
generally valuable.  Fix these warnings so we have a clean build.  Thanks,

Josef

Josef Bacik (2):
  btrfs: make sure to initialize start and len in find_free_dev_extent
  btrfs: initialize start_slot in btrfs_log_prealloc_extents

 fs/btrfs/tree-log.c |  2 +-
 fs/btrfs/volumes.c  | 14 ++++++--------
 2 files changed, 7 insertions(+), 9 deletions(-)

-- 
2.41.0

[PATCH 1/2] btrfs: make sure to initialize start and len in find_free_dev_extent

[Josef Bacik]

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(-)

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)) {
-- 
2.41.0

Re: [PATCH 1/2] btrfs: make sure to initialize start and len in find_free_dev_extent

[Jens Axboe]

Tested-by: Jens Axboe <axboe@kernel.dk>

-- 
Jens Axboe

Re: [PATCH 1/2] btrfs: make sure to initialize start and len in find_free_dev_extent

[Qu Wenruo]

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)) {

Re: [PATCH 1/2] btrfs: make sure to initialize start and len in find_free_dev_extent

[David Sterba]

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.

[PATCH 2/2] btrfs: initialize start_slot in btrfs_log_prealloc_extents

[Josef Bacik]

Jens reported a compiler warning when using
CONFIG_CC_OPTIMIZE_FOR_SIZE=y that looks like this

fs/btrfs/tree-log.c: In function ‘btrfs_log_prealloc_extents’:
fs/btrfs/tree-log.c:4828:23: warning: ‘start_slot’ may be used
uninitialized [-Wmaybe-uninitialized]
 4828 |                 ret = copy_items(trans, inode, dst_path, path,
      |                       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 4829 |                                  start_slot, ins_nr, 1, 0);
      |                                  ~~~~~~~~~~~~~~~~~~~~~~~~~
fs/btrfs/tree-log.c:4725:13: note: ‘start_slot’ was declared here
 4725 |         int start_slot;
      |             ^~~~~~~~~~

The compiler is incorrect, as we only use this code when ins_len > 0,
and when ins_len > 0 we have start_slot properly initialized.  However
we generally find the -Wmaybe-uninitialized warnings valuable, so
initialize start_slot to get rid of the warning.

Reported-by: Jens Axboe <axboe@kernel.dk>
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/tree-log.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
index d1e46b839519..cbb17b542131 100644
--- a/fs/btrfs/tree-log.c
+++ b/fs/btrfs/tree-log.c
@@ -4722,7 +4722,7 @@ static int btrfs_log_prealloc_extents(struct btrfs_trans_handle *trans,
 	struct extent_buffer *leaf;
 	int slot;
 	int ins_nr = 0;
-	int start_slot;
+	int start_slot = 0;
 	int ret;
 
 	if (!(inode->flags & BTRFS_INODE_PREALLOC))
-- 
2.41.0

Re: [PATCH 2/2] btrfs: initialize start_slot in btrfs_log_prealloc_extents

[Jens Axboe]

Tested-by: Jens Axboe <axboe@kernel.dk>

-- 
Jens Axboe

Re: [PATCH 2/2] btrfs: initialize start_slot in btrfs_log_prealloc_extents

[Qu Wenruo]

On 2023/9/6 00:15, Josef Bacik wrote:
> Jens reported a compiler warning when using
> CONFIG_CC_OPTIMIZE_FOR_SIZE=y that looks like this
>
> fs/btrfs/tree-log.c: In function ‘btrfs_log_prealloc_extents’:
> fs/btrfs/tree-log.c:4828:23: warning: ‘start_slot’ may be used
> uninitialized [-Wmaybe-uninitialized]
>   4828 |                 ret = copy_items(trans, inode, dst_path, path,
>        |                       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>   4829 |                                  start_slot, ins_nr, 1, 0);
>        |                                  ~~~~~~~~~~~~~~~~~~~~~~~~~
> fs/btrfs/tree-log.c:4725:13: note: ‘start_slot’ was declared here
>   4725 |         int start_slot;
>        |             ^~~~~~~~~~
>
> The compiler is incorrect, as we only use this code when ins_len > 0,
> and when ins_len > 0 we have start_slot properly initialized.  However
> we generally find the -Wmaybe-uninitialized warnings valuable, so
> initialize start_slot to get rid of the warning.
>
> Reported-by: Jens Axboe <axboe@kernel.dk>
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>

I think we're in a dilemma, if we don't do the initialization, bad
compiler may warn.

But if we do the initialization, some static checker may also warn...

Thanks,
Qu
> ---
>   fs/btrfs/tree-log.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
> index d1e46b839519..cbb17b542131 100644
> --- a/fs/btrfs/tree-log.c
> +++ b/fs/btrfs/tree-log.c
> @@ -4722,7 +4722,7 @@ static int btrfs_log_prealloc_extents(struct btrfs_trans_handle *trans,
>   	struct extent_buffer *leaf;
>   	int slot;
>   	int ins_nr = 0;
> -	int start_slot;
> +	int start_slot = 0;
>   	int ret;
>
>   	if (!(inode->flags & BTRFS_INODE_PREALLOC))

Re: [PATCH 2/2] btrfs: initialize start_slot in btrfs_log_prealloc_extents

[David Sterba]

On Wed, Sep 06, 2023 at 06:51:26AM +0800, Qu Wenruo wrote:
> 
> 
> On 2023/9/6 00:15, Josef Bacik wrote:
> > Jens reported a compiler warning when using
> > CONFIG_CC_OPTIMIZE_FOR_SIZE=y that looks like this
> >
> > fs/btrfs/tree-log.c: In function ‘btrfs_log_prealloc_extents’:
> > fs/btrfs/tree-log.c:4828:23: warning: ‘start_slot’ may be used
> > uninitialized [-Wmaybe-uninitialized]
> >   4828 |                 ret = copy_items(trans, inode, dst_path, path,
> >        |                       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >   4829 |                                  start_slot, ins_nr, 1, 0);
> >        |                                  ~~~~~~~~~~~~~~~~~~~~~~~~~
> > fs/btrfs/tree-log.c:4725:13: note: ‘start_slot’ was declared here
> >   4725 |         int start_slot;
> >        |             ^~~~~~~~~~
> >
> > The compiler is incorrect, as we only use this code when ins_len > 0,
> > and when ins_len > 0 we have start_slot properly initialized.  However
> > we generally find the -Wmaybe-uninitialized warnings valuable, so
> > initialize start_slot to get rid of the warning.
> >
> > Reported-by: Jens Axboe <axboe@kernel.dk>
> > Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> 
> I think we're in a dilemma, if we don't do the initialization, bad
> compiler may warn.
> 
> But if we do the initialization, some static checker may also warn...

I've seen static checkers to accept variables initialized to 0 and
unused, there's a lot of code that does that as a matter of coding style
so there would be too many warnings for a generally good practice.
If we see a warning from something else than compiler then we can
reconsider the way it's fixed but I think we'd see more compiler reports
than static checker reports.

Re: [PATCH 0/2] Fix some -Wmaybe-uninitialized errors

[David Sterba]

On Tue, Sep 05, 2023 at 12:15:22PM -0400, Josef Bacik wrote:
> Hello,
> 
> Jens reported to me two warnings he was seeing with
> CONFIG_CC_OPTIMIZE_FOR_SIZE=y set in his .config.  We don't see these errors
> without this option, and they're not correct warnings.  However we've had issues
> where -Wmaybe-uninitialized would have caught a real bug, so this warning is
> generally valuable.  Fix these warnings so we have a clean build.  Thanks,
> 
> Josef
> 
> Josef Bacik (2):
>   btrfs: make sure to initialize start and len in find_free_dev_extent
>   btrfs: initialize start_slot in btrfs_log_prealloc_extents

Added to misc-next, thanks.