On Thu, Feb 25, 2021 at 03:59:20PM +0100, David Sterba wrote: > On Tue, Feb 23, 2021 at 08:28:10PM +0200, Nikolay Borisov wrote: > > > > > > On 23.02.21 г. 20:22 ч., Boris Burkov wrote: > > > The intended logic of the check is to catch cases where the desired > > > free_space_tree setting doesn't match the mounted setting, and the > > > remount is anything but ro->rw. However, it makes the mistake of > > > checking equality on a masked integer (btrfs_test_opt) against a boolean > > > (btrfs_fs_compat_ro). > > > > > > If you run the reproducer: > > > mount -o space_cache=v2 dev mnt > > > mount -o remount,ro mnt > > > > > > you would expect no warning, because the remount is not attempting to > > > change the free space tree setting, but we do see the warning. > > > > > > To fix this, convert the option test to a boolean. > > > > > > I tested a variety of transitions: > > > sudo mount -o space_cache=v2 /dev/vg0/lv0 mnt/lol > > > (fst enabled) > > > mount -o remount,ro mnt/lol > > > (no warning, no fst change) > > > sudo mount -o remount,rw,space_cache=v1,clear_cache > > > (no warning, ro->rw) > > > sudo mount -o remount,rw,space_cache=v2 mnt > > > (warning, rw->rw with change) > > > sudo mount -o remount,ro mnt > > > (no warning, no fst change) > > > sudo mount -o remount,rw,space_cache=v2 mnt > > > (no warning, no fst change) > > > > > > Reported-by: Chris Murphy <li...@colorremedies.com> > > > Signed-off-by: Boris Burkov <bo...@bur.io> > > > --- > > > fs/btrfs/super.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c > > > index f8435641b912..d4992ceab5ea 100644 > > > --- a/fs/btrfs/super.c > > > +++ b/fs/btrfs/super.c > > > @@ -1918,7 +1918,7 @@ static int btrfs_remount(struct super_block *sb, > > > int *flags, char *data) > > > btrfs_resize_thread_pool(fs_info, > > > fs_info->thread_pool_size, old_thread_pool_size); > > > > > > - if (btrfs_test_opt(fs_info, FREE_SPACE_TREE) != > > > + if (!!btrfs_test_opt(fs_info, FREE_SPACE_TREE) != > > > > I'd rather thave the !! convert to bool magic in the macro definition i.e > > : > > > > #define btrfs_test_opt(fs_info, opt) !!((fs_info)->mount_opt & \ > > > > BTRFS_MOUNT_##opt) > > > > Yeah, that sounds safer and we should convert all predicate functions to > bool eg. __btrfs_fs_compat_ro. The whole value of the macro needs to be > in ( .. ) too.
For the minimal quick fix I'd add (bool) cast to both sides of == so it's clear and then we can do further cleanups in separate patches.
