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.
