On Tue, Jan 26, 2021 at 03:32:36PM -0500, Josef Bacik wrote:

On 1/26/21 1:36 PM, David Sterba wrote:

quoted

On Fri, Oct 09, 2020 at 09:28:18AM -0400, Josef Bacik wrote:

quoted

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.