Re: [PATCH v6 05/28] btrfs: disallow space_cache in HMZONED mode

Josef Bacik
2019-12-13T20:39:17Z

On 12/12/19 11:08 PM, Naohiro Aota wrote:
> As updates to the space cache v1 are in-place, the space cache cannot be
> located over sequential zones and there is no guarantees that the device
> will have enough conventional zones to store this cache. Resolve this
> problem by disabling completely the space cache v1. This does not
> introduces any problems with sequential block groups: all the free space is
> located after the allocation pointer and no free space before the pointer.
> There is no need to have such cache.
>
> Note: we can technically use free-space-tree (space cache v2) on HMZONED
> mode. But, since HMZONED mode now always allocate extents in a block group
> sequentially regardless of underlying device zone type, it's no use to
> enable and maintain the tree.
>
> Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com>
> ---
> fs/btrfs/hmzoned.c | 18 ++++++++++++++++++
> fs/btrfs/hmzoned.h | 5 +++++
> fs/btrfs/super.c | 11 +++++++++--
> 3 files changed, 32 insertions(+), 2 deletions(-)
>
> diff --git a/fs/btrfs/hmzoned.c b/fs/btrfs/hmzoned.c
> index 1b24facd46b8..d62f11652973 100644
> --- a/fs/btrfs/hmzoned.c
> +++ b/fs/btrfs/hmzoned.c
> @@ -250,3 +250,21 @@ int btrfs_check_hmzoned_mode(struct btrfs_fs_info *fs_info)
> out:
> return ret;
> }
> +
> +int btrfs_check_mountopts_hmzoned(struct btrfs_fs_info *info)
> +{
> + if (!btrfs_fs_incompat(info, HMZONED))
> + return 0;
> +
> + /*
> + * SPACE CACHE writing is not CoWed. Disable that to avoid write
> + * errors in sequential zones.
> + */
> + if (btrfs_test_opt(info, SPACE_CACHE)) {
> + btrfs_err(info,
> + "space cache v1 not supportted in HMZONED mode");
> + return -EOPNOTSUPP;
> + }
> +
> + return 0;
> +}
> diff --git a/fs/btrfs/hmzoned.h b/fs/btrfs/hmzoned.h
> index 8e17f64ff986..d9ebe11afdf5 100644
> --- a/fs/btrfs/hmzoned.h
> +++ b/fs/btrfs/hmzoned.h
> @@ -29,6 +29,7 @@ int btrfs_get_dev_zone(struct btrfs_device *device, u64 pos,
> int btrfs_get_dev_zone_info(struct btrfs_device *device);
> void btrfs_destroy_dev_zone_info(struct btrfs_device *device);
> int btrfs_check_hmzoned_mode(struct btrfs_fs_info *fs_info);
> +int btrfs_check_mountopts_hmzoned(struct btrfs_fs_info *info);
> #else /* CONFIG_BLK_DEV_ZONED */
> static inline int btrfs_get_dev_zone(struct btrfs_device *device, u64 pos,
> struct blk_zone *zone)
> @@ -48,6 +49,10 @@ static inline int btrfs_check_hmzoned_mode(struct btrfs_fs_info *fs_info)
> btrfs_err(fs_info, "Zoned block devices support is not enabled");
> return -EOPNOTSUPP;
> }
> +static inline int btrfs_check_mountopts_hmzoned(struct btrfs_fs_info *info)
> +{
> + return 0;
> +}
> #endif
>
> static inline bool btrfs_dev_is_sequential(struct btrfs_device *device, u64 pos)
> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
> index 616f5abec267..1424c3c6e3cf 100644
> --- a/fs/btrfs/super.c
> +++ b/fs/btrfs/super.c
> @@ -442,8 +442,13 @@ int btrfs_parse_options(struct btrfs_fs_info *info, char *options,
> cache_gen = btrfs_super_cache_generation(info->super_copy);
> if (btrfs_fs_compat_ro(info, FREE_SPACE_TREE))
> btrfs_set_opt(info->mount_opt, FREE_SPACE_TREE);
> - else if (cache_gen)
> - btrfs_set_opt(info->mount_opt, SPACE_CACHE);
> + else if (cache_gen) {
> + if (btrfs_fs_incompat(info, HMZONED))
> + btrfs_info(info,
> + "ignoring existing space cache in HMZONED mode");

It would be good to clear the cache gen in this case. I assume this can happen
if we add a hmzoned device to an existing fs with space cache already? I'd hate
for weird corner cases to pop up if we removed it later and still had a valid
cache gen in place. Thanks,

Josef