Re: [PATCH] btrfs: fix spurious free_space_tree remount warning

[David Sterba <dsterba@suse.cz>]

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 <lists@colorremedies.com>
> > > Signed-off-by: Boris Burkov <boris@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.