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@xxxxxxxxxxxxxx>
> ---
> 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;
> }
>
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
