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

[David Sterba <dsterba@suse.cz>]

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.