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

[Josef Bacik <josef@toxicpanda.com>]

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