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

From: Nikolay Borisov <nborisov@suse.com>
To: Boris Burkov <boris@bur.io>,
	linux-btrfs@vger.kernel.org, kernel-team@fb.com,
	'Chris Murphy ' <lists@colorremedies.com>
Subject: Re: [PATCH] btrfs: fix spurious free_space_tree remount warning
Date: Tue, 23 Feb 2021 20:28:10 +0200	[thread overview]
Message-ID: <8769846e-68b2-766f-5fdd-43ffb79f4586@suse.com> (raw)
In-Reply-To: <4a019f01584f1d818203b7c2ed65204583a11592.1614104082.git.boris@bur.io>

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)                     

>  	    btrfs_fs_compat_ro(fs_info, FREE_SPACE_TREE) &&
>  	    (!sb_rdonly(sb) || (*flags & SB_RDONLY))) {
>  		btrfs_warn(fs_info,
> 