Re: [PATCH 1/3] btrfs-progs: Add the single-dev feature (to both mkfs/tune)

[Josef Bacik <josef@toxicpanda.com>]

On Thu, Aug 03, 2023 at 12:43:39PM -0300, Guilherme G. Piccoli wrote:
> The single-dev feature allows a device to be mounted regardless of
> its fsid already being present in another device - in other words,
> this feature disables RAID modes / metadata_uuid, allowing a single
> device per filesystem. Its goal is mainly to allow mounting the
> same fsid at the same time in the system.
> 
> Introduce hereby the feature to both mkfs (-O single-dev) and
> btrfstune (-s), syncing the kernel-shared headers as well. The
> feature is a compat_ro, its kernel version was set to v6.5.
> 
> Suggested-by: Qu Wenruo <wqu@suse.com>
> Signed-off-by: Guilherme G. Piccoli <gpiccoli@igalia.com>
> ---
> 
> Hi folks, thanks in advance for reviews! Notice that I've added
> the feature to btrfstune as well, but I found docs online saying
> this tool is deprecated..so not sure if that was the proper approach.
> 
> Also, a design decision: I've skipped the btrfs_register_one_device()
> call when mkfs was just used with the single-dev tuning, or else
> it shows a (harmless) error and succeeds, since of course scanning
> fails for such devices, as per the feature implementation.
> So, I thought it was more straightforward to just skip the call itself.
> 

This is a reasonable approach.

> Cheers,
> 
> Guilherme
> 
>  common/fsfeatures.c        |  7 ++++
>  kernel-shared/ctree.h      |  3 +-
>  kernel-shared/uapi/btrfs.h |  7 ++++
>  mkfs/main.c                |  4 ++-
>  tune/main.c                | 72 +++++++++++++++++++++++---------------
>  5 files changed, 63 insertions(+), 30 deletions(-)
> 
> diff --git a/common/fsfeatures.c b/common/fsfeatures.c
> index 00658fa5159f..a320b7062b8c 100644
> --- a/common/fsfeatures.c
> +++ b/common/fsfeatures.c
> @@ -160,6 +160,13 @@ static const struct btrfs_feature mkfs_features[] = {
>  		VERSION_NULL(default),
>  		.desc		= "RAID1 with 3 or 4 copies"
>  	},
> +	{
> +		.name		= "single-dev",
> +		.compat_ro_flag	= BTRFS_FEATURE_COMPAT_RO_SINGLE_DEV,
> +		.sysfs_name	= "single_dev",
> +		VERSION_TO_STRING2(compat, 6,5),
> +		.desc		= "single device (allows same fsid mounting)"
> +	},
>  #ifdef BTRFS_ZONED
>  	{
>  		.name		= "zoned",
> diff --git a/kernel-shared/ctree.h b/kernel-shared/ctree.h
> index 59533879b939..e3fd834aa6dd 100644
> --- a/kernel-shared/ctree.h
> +++ b/kernel-shared/ctree.h
> @@ -86,7 +86,8 @@ static inline u32 __BTRFS_LEAF_DATA_SIZE(u32 nodesize)
>  	(BTRFS_FEATURE_COMPAT_RO_FREE_SPACE_TREE |	\
>  	 BTRFS_FEATURE_COMPAT_RO_FREE_SPACE_TREE_VALID | \
>  	 BTRFS_FEATURE_COMPAT_RO_VERITY |		\
> -	 BTRFS_FEATURE_COMPAT_RO_BLOCK_GROUP_TREE)
> +	 BTRFS_FEATURE_COMPAT_RO_BLOCK_GROUP_TREE |	\
> +	 BTRFS_FEATURE_COMPAT_RO_SINGLE_DEV)
>  
>  #if EXPERIMENTAL
>  #define BTRFS_FEATURE_INCOMPAT_SUPP			\
> diff --git a/kernel-shared/uapi/btrfs.h b/kernel-shared/uapi/btrfs.h
> index 85b04f89a2a9..2e0ee6ef6446 100644
> --- a/kernel-shared/uapi/btrfs.h
> +++ b/kernel-shared/uapi/btrfs.h
> @@ -336,6 +336,13 @@ _static_assert(sizeof(struct btrfs_ioctl_fs_info_args) == 1024);
>   */
>  #define BTRFS_FEATURE_COMPAT_RO_BLOCK_GROUP_TREE	(1ULL << 3)
>  
> +/*
> + * Single devices (as flagged by the corresponding compat_ro flag) only
> + * gets scanned during mount time; also, a random fsid is generated for
> + * them, in order to cope with same-fsid filesystem mounts.
> + */
> +#define BTRFS_FEATURE_COMPAT_RO_SINGLE_DEV		(1ULL << 4)
> +
>  #define BTRFS_FEATURE_INCOMPAT_MIXED_BACKREF	(1ULL << 0)
>  #define BTRFS_FEATURE_INCOMPAT_DEFAULT_SUBVOL	(1ULL << 1)
>  #define BTRFS_FEATURE_INCOMPAT_MIXED_GROUPS	(1ULL << 2)
> diff --git a/mkfs/main.c b/mkfs/main.c
> index 972ed1112ea6..429799932224 100644
> --- a/mkfs/main.c
> +++ b/mkfs/main.c
> @@ -1025,6 +1025,7 @@ int BOX_MAIN(mkfs)(int argc, char **argv)
>  	char *label = NULL;
>  	int nr_global_roots = sysconf(_SC_NPROCESSORS_ONLN);
>  	char *source_dir = NULL;
> +	bool single_dev;
>  
>  	cpu_detect_flags();
>  	hash_init_accel();
> @@ -1218,6 +1219,7 @@ int BOX_MAIN(mkfs)(int argc, char **argv)
>  		usage(&mkfs_cmd, 1);
>  
>  	opt_zoned = !!(features.incompat_flags & BTRFS_FEATURE_INCOMPAT_ZONED);
> +	single_dev = !!(features.compat_ro_flags & BTRFS_FEATURE_COMPAT_RO_SINGLE_DEV);
>  
>  	if (source_dir && device_count > 1) {
>  		error("the option -r is limited to a single device");
> @@ -1815,7 +1817,7 @@ out:
>  		device_count = argc - optind;
>  		while (device_count-- > 0) {
>  			file = argv[optind++];
> -			if (path_is_block_device(file) == 1)
> +			if (path_is_block_device(file) == 1 && !single_dev)
>  				btrfs_register_one_device(file);
>  		}
>  	}
> diff --git a/tune/main.c b/tune/main.c
> index 0ca1e01282c9..95e55fcda44f 100644
> --- a/tune/main.c
> +++ b/tune/main.c
> @@ -42,27 +42,31 @@
>  #include "tune/tune.h"
>  #include "check/clear-cache.h"
>  
> +#define SET_SUPER_FLAGS(type) \
> +static int set_super_##type##_flags(struct btrfs_root *root, u64 flags) \
> +{									\
> +	struct btrfs_trans_handle *trans;				\
> +	struct btrfs_super_block *disk_super;				\
> +	u64 super_flags;						\
> +	int ret;							\
> +									\
> +	disk_super = root->fs_info->super_copy;				\
> +	super_flags = btrfs_super_##type##_flags(disk_super);		\
> +	super_flags |= flags;						\
> +	trans = btrfs_start_transaction(root, 1);			\
> +	BUG_ON(IS_ERR(trans));						\
> +	btrfs_set_super_##type##_flags(disk_super, super_flags);	\
> +	ret = btrfs_commit_transaction(trans, root);			\
> +									\
> +	return ret;							\
> +}
> +
> +SET_SUPER_FLAGS(incompat)
> +SET_SUPER_FLAGS(compat_ro)
> +
>  static char *device;
>  static int force = 0;
>  
> -static int set_super_incompat_flags(struct btrfs_root *root, u64 flags)
> -{
> -	struct btrfs_trans_handle *trans;
> -	struct btrfs_super_block *disk_super;
> -	u64 super_flags;
> -	int ret;
> -
> -	disk_super = root->fs_info->super_copy;
> -	super_flags = btrfs_super_incompat_flags(disk_super);
> -	super_flags |= flags;
> -	trans = btrfs_start_transaction(root, 1);
> -	BUG_ON(IS_ERR(trans));
> -	btrfs_set_super_incompat_flags(disk_super, super_flags);
> -	ret = btrfs_commit_transaction(trans, root);
> -
> -	return ret;
> -}
> -
>  static int convert_to_fst(struct btrfs_fs_info *fs_info)
>  {
>  	int ret;
> @@ -102,6 +106,7 @@ static const char * const tune_usage[] = {
>  	OPTLINE("-r", "enable extended inode refs (mkfs: extref, for hardlink limits)"),
>  	OPTLINE("-x", "enable skinny metadata extent refs (mkfs: skinny-metadata)"),
>  	OPTLINE("-n", "enable no-holes feature (mkfs: no-holes, more efficient sparse file representation)"),
> +	OPTLINE("-s", "enable single device feature (mkfs: single-dev, allows same fsid mounting)"),

btrfstune is going to be integrated into an actual btrfs command, so we're no
longer using short options for new btrfstune commands.  Figure out a long name
instead and use that only.  Something like

--convert-to-single-device

as you would be using this on an existing file system.  The rest of the code is
generally fine.  Thanks,

Josef