On Tue, Jan 26, 2021 at 03:32:36PM -0500, Josef Bacik wrote:
> On 1/26/21 1:36 PM, David Sterba wrote:
> > On Fri, Oct 09, 2020 at 09:28:18AM -0400, Josef Bacik wrote:
> >> I got a automated message from somebody who runs clang against our
> >> kernels and it's because I used the wrong enum type for what I passed
> >> into flush_space. Change the argument to be explicitly the enum we're
> >> expecting to make everything consistent. Maybe eventually gcc will
> >> catch errors like this.
> >
> > I can't find any such public report and none of the clang warnings seem
> > to be specific about that. Local run with clang 11 does not produce any
> > warning.
> >
>
> IDK, it was a private email just to me with the following output from clang
>
> s/btrfs/space-info.c:1115:12: warning: implicit conversion from
> enumeration type 'enum btrfs_flush_state' to different enumeration type
> 'enum btrfs_reserve_flush_enum' [-Wenum-conversion]
> flush = FLUSH_DELALLOC;
> ~ ^~~~~~~~~~~~~~
> fs/btrfs/space-info.c:1120:12: warning: implicit conversion from
> enumeration type 'enum btrfs_flush_state' to different enumeration type
> 'enum btrfs_reserve_flush_enum' [-Wenum-conversion]
> flush = FORCE_COMMIT_TRANS;
> ~ ^~~~~~~~~~~~~~~~~~
> fs/btrfs/space-info.c:1124:12: warning: implicit conversion from
> enumeration type 'enum btrfs_flush_state' to different enumeration type
> 'enum btrfs_reserve_flush_enum' [-Wenum-conversion]
> flush = FLUSH_DELAYED_ITEMS_NR;
> ~ ^~~~~~~~~~~~~~~~~~~~~~
> fs/btrfs/space-info.c:1127:12: warning: implicit conversion from
> enumeration type 'enum btrfs_flush_state' to different enumeration type
> 'enum btrfs_reserve_flush_enum' [-Wenum-conversion]
> flush = FLUSH_DELAYED_REFS_NR;
> ~ ^~~~~~~~~~~~~~~~~~~~~
>
> I figure it made sense, might as well fix it. Do we have that option for our
> shiny new -W build options? Thanks,
We can add -Wenum-conversion sure, but neither gcc (10.2.1) nor clang
(11.0.1) reproduces the warning for me so I wonder how useful it would
be. I've also tried adding -Wextra and -Wall and the build is clean.
Per gcc documentation, -Wenum-conversion is not exactly what the code
did:
"Warn when a value of enumerated type is implicitly converted to a
different enumerated type. This warning is enabled by -Wextra."
And now that I read the warning carefuly it does not apply to our code,
"btrfs_reserve_flush_enum = btrfs_flush_state" does not happen so it
must have been some older revision of the code.
Changing
@@ -1073,7 +1073,7 @@ static void btrfs_preempt_reclaim_metadata_space(struct
work_struct *work)
spin_lock(&space_info->lock);
while (need_preemptive_reclaim(fs_info, space_info)) {
- enum btrfs_flush_state flush;
+ enum btrfs_reserve_flush_enum flush;
---
gcc says (on misc-next without this patchset)
fs/btrfs/space-info.c: In function ‘btrfs_preempt_reclaim_metadata_space’:
fs/btrfs/space-info.c:1112:10: warning: implicit conversion from ‘enum
btrfs_flush_state’ to ‘enum btrfs_reserve_flush_enum’ [-Wenum-conversion]
1112 | flush = FLUSH_DELALLOC;
| ^
CC [M] fs/btrfs/block-group.o
fs/btrfs/space-info.c:1117:10: warning: implicit conversion from ‘enum
btrfs_flush_state’ to ‘enum btrfs_reserve_flush_enum’ [-Wenum-conversion]
1117 | flush = FORCE_COMMIT_TRANS;
| ^
fs/btrfs/space-info.c:1121:10: warning: implicit conversion from ‘enum
btrfs_flush_state’ to ‘enum btrfs_reserve_flush_enum’ [-Wenum-conversion]
1121 | flush = FLUSH_DELAYED_ITEMS_NR;
| ^
fs/btrfs/space-info.c:1124:10: warning: implicit conversion from ‘enum
btrfs_flush_state’ to ‘enum btrfs_reserve_flush_enum’ [-Wenum-conversion]
1124 | flush = FLUSH_DELAYED_REFS_NR;
| ^
fs/btrfs/space-info.c:1135:48: warning: implicit conversion from ‘enum
btrfs_reserve_flush_enum’ to ‘enum btrfs_flush_state’ [-Wenum-conversion]
1135 | flush_space(fs_info, space_info, to_reclaim, flush, true);
|
---
So you weren't fixing the reported problem by this patch. Assigning enum
to an int should be natural so thre's no reason to fix that but for
clarity I don't mind, the patch stays.
