Re: [PATCH RESEND v4 2/8] btrfs: Make btrfs_parse_options() parse mount option in a atomic way

[Miao Xie <miaoxie@huawei.com>]

On Thu, 29 Jan 2015 10:24:35 +0800, Qu Wenruo wrote:
> Current btrfs_parse_options() is not atomic, which can set and clear a
> bit, especially for nospace_cache case.
> 
> For example, if a fs is mounted with nospace_cache,
> btrfs_parse_options() will set SPACE_CACHE bit first(since
> cache_generation is non-zeo) and clear the SPACE_CACHE bit due to
> nospace_cache mount option.
> So under heavy operations and remount a nospace_cache btrfs, there is a
> windows for commit to create space cache.
> 
> This bug can be reproduced by fstest/btrfs/071 073 074 with
> nospace_cache mount option. It has about 50% chance to create space
> cache, and about 10% chance to create wrong space cache, which can't
> pass btrfsck.
> 
> This patch will do the mount option parse in a copy-and-update method.
> First copy the mount_opt from fs_info to new_opt, and only update
> options in new_opt. At last, copy the new_opt back to
> fs_info->mount_opt.
> 
> This patch is already good enough to fix the above nospace_cache +
> remount bug, but need later patch to make sure mount options does not
> change during transaction.
> 
> Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
> ---
>  fs/btrfs/ctree.h |  16 ++++----
>  fs/btrfs/super.c | 115 +++++++++++++++++++++++++++++--------------------------
>  2 files changed, 69 insertions(+), 62 deletions(-)
> 
> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> index 5f99743..26bb47b 100644
> --- a/fs/btrfs/ctree.h
> +++ b/fs/btrfs/ctree.h
> @@ -2119,18 +2119,18 @@ struct btrfs_ioctl_defrag_range_args {
>  #define btrfs_test_opt(root, opt)	((root)->fs_info->mount_opt & \
>  					 BTRFS_MOUNT_##opt)
>  
> -#define btrfs_set_and_info(root, opt, fmt, args...)			\
> +#define btrfs_set_and_info(fs_info, val, opt, fmt, args...)		\
>  {									\
> -	if (!btrfs_test_opt(root, opt))					\
> -		btrfs_info(root->fs_info, fmt, ##args);			\
> -	btrfs_set_opt(root->fs_info->mount_opt, opt);			\
> +	if (!btrfs_raw_test_opt(val, opt))				\
> +		btrfs_info(fs_info, fmt, ##args);			\
> +	btrfs_set_opt(val, opt);					\
>  }
>  
> -#define btrfs_clear_and_info(root, opt, fmt, args...)			\
> +#define btrfs_clear_and_info(fs_info, val, opt, fmt, args...)		\
>  {									\
> -	if (btrfs_test_opt(root, opt))					\
> -		btrfs_info(root->fs_info, fmt, ##args);			\
> -	btrfs_clear_opt(root->fs_info->mount_opt, opt);			\
> +	if (btrfs_raw_test_opt(val, opt))				\
> +		btrfs_info(fs_info, fmt, ##args);			\
> +	btrfs_clear_opt(val, opt);					\
>  }
>  
>  /*
> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
> index b0c45b2..490fe1f 100644
> --- a/fs/btrfs/super.c
> +++ b/fs/btrfs/super.c
> @@ -395,10 +395,13 @@ int btrfs_parse_options(struct btrfs_root *root, char *options)
>  	int ret = 0;
>  	char *compress_type;
>  	bool compress_force = false;
> +	unsigned long new_opt;
> +
> +	new_opt = info->mount_opt;

Here and

>  
>  	cache_gen = btrfs_super_cache_generation(root->fs_info->super_copy);
>  	if (cache_gen)
> -		btrfs_set_opt(info->mount_opt, SPACE_CACHE);
[SNIP]
>  out:
> -	if (!ret && btrfs_test_opt(root, SPACE_CACHE))
> -		btrfs_info(root->fs_info, "disk space caching is enabled");
> +	if (!ret) {
> +		if (btrfs_raw_test_opt(new_opt, SPACE_CACHE))
> +			btrfs_info(info, "disk space caching is enabled");
> +		info->mount_opt = new_opt;

Here need ACCESS_ONCE to wrap info->mount_opt, or the complier might use
info->mount_opt instead of new_opt.

But I worried that this is not key reason of the wrong space cache. Could
you explain the race condition which caused the wrong space cache?

Thanks
Miao

> +	}
>  	kfree(orig);
>  	return ret;
>  }
>