Re: [PATCH] Make btrfs_prepare_device parallel during mkfs.btrfs

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

On 2022/8/25 00:05, Li Zhang wrote:
> [enhancement]
> When a disk is formatted as btrfs, it calls
> btrfs_prepare_device for each device, which takes too much time.

The idea is awesome.

>
> [implementation]
> Put each btrfs_prepare_device into a thread,
> wait for the first thread to complete to mkfs.btrfs,
> and wait for other threads to complete before adding
> other devices to the file system.
>
> [test]
> Using the btrfs-progs test case mkfs-tests, mkfs.btrfs works fine.
>
> But I don't have an actual zoed device,
> so I don't know how much time it saves, If you guys
> have a way to test it, please let me know.
>
> Signed-off-by: Li Zhang <zhanglikernel@gmail.com>
> ---
> Issue: 496
>
>   mkfs/main.c | 113 +++++++++++++++++++++++++++++++++++++++++++++---------------
>   1 file changed, 86 insertions(+), 27 deletions(-)
>
> diff --git a/mkfs/main.c b/mkfs/main.c
> index ce096d3..35fefe2 100644
> --- a/mkfs/main.c
> +++ b/mkfs/main.c
> @@ -31,6 +31,7 @@
>   #include <uuid/uuid.h>
>   #include <ctype.h>
>   #include <blkid/blkid.h>
> +#include <pthread.h>
>   #include "kernel-shared/ctree.h"
>   #include "kernel-shared/disk-io.h"
>   #include "kernel-shared/free-space-tree.h"
> @@ -60,6 +61,18 @@ struct mkfs_allocation {
>   	u64 system;
>   };
>
> +
> +struct prepare_device_progress {
> +	char *file;
> +	u64 dev_block_count;
> +	u64 block_count;

> +	bool zero_end;
> +	bool discard;
> +	bool zoned;
> +	int oflags;

A small nitpick.

Aren't those 4 values the same shared by all devices?
Thus I'm not sure if they need to be put into prepare_device_progress at
all.

IIRC, we may want some shared memory between all the threads:

- A pthread_mutex
   Will be explained later

- All the other shared infos like above flags/oflags

It can be global or passed by some pointers.

> +	int ret;
> +};
> +
>   static int create_metadata_block_groups(struct btrfs_root *root, bool mixed,
>   				struct mkfs_allocation *allocation)
>   {
> @@ -969,6 +982,28 @@ fail:
>   	return ret;
>   }
>
> +static void *prepare_one_dev(void *ctx)
> +{
> +	struct prepare_device_progress *prepare_ctx = ctx;
> +	int fd;
> +
> +	fd = open(prepare_ctx->file, prepare_ctx->oflags);
> +	if (fd < 0) {
> +		error("unable to open %s: %m", prepare_ctx->file);

If we have no permission for all devices (pretty common in fact, e.g.
forgot to use sudo), we will have multiple threads printing out the same
time.

Without a lock, the output will be a mess.

Thus we may want a mutex, even it's just for synchronizing the output.

> +		prepare_ctx->ret = fd;
> +		return NULL;
> +	}
> +	prepare_ctx->ret = btrfs_prepare_device(fd,
> +		prepare_ctx->file, &prepare_ctx->dev_block_count,
> +		prepare_ctx->block_count,
> +		(bconf.verbose ? PREP_DEVICE_VERBOSE : 0) |
> +		(prepare_ctx->zero_end ? PREP_DEVICE_ZERO_END : 0) |
> +		(prepare_ctx->discard ? PREP_DEVICE_DISCARD : 0) |
> +		(prepare_ctx->zoned ? PREP_DEVICE_ZONED : 0));
> +	close(fd);
> +	return NULL;
> +}
> +
>   int BOX_MAIN(mkfs)(int argc, char **argv)
>   {
>   	char *file;
> @@ -997,7 +1032,6 @@ int BOX_MAIN(mkfs)(int argc, char **argv)
>   	bool ssd = false;
>   	bool zoned = false;
>   	bool force_overwrite = false;
> -	int oflags;
>   	char *source_dir = NULL;
>   	bool source_dir_set = false;
>   	bool shrink_rootdir = false;
> @@ -1006,6 +1040,8 @@ int BOX_MAIN(mkfs)(int argc, char **argv)
>   	u64 shrink_size;
>   	int dev_cnt = 0;
>   	int saved_optind;
> +	pthread_t *t_prepare = NULL;
> +	struct prepare_device_progress *prepare_ctx = NULL;
>   	char fs_uuid[BTRFS_UUID_UNPARSED_SIZE] = { 0 };
>   	u64 features = BTRFS_MKFS_DEFAULT_FEATURES;
>   	u64 runtime_features = BTRFS_MKFS_DEFAULT_RUNTIME_FEATURES;
> @@ -1428,29 +1464,45 @@ int BOX_MAIN(mkfs)(int argc, char **argv)
>   		goto error;
>   	}
>
> -	dev_cnt--;
> -
> -	oflags = O_RDWR;
> -	if (zoned && zoned_model(file) == ZONED_HOST_MANAGED)
> -		oflags |= O_DIRECT;
> +	t_prepare = malloc(dev_cnt * sizeof(*t_prepare));
> +	prepare_ctx = malloc(dev_cnt * sizeof(*prepare_ctx));
>
> -	/*
> -	 * Open without O_EXCL so that the problem should not occur by the
> -	 * following operation in kernel:
> -	 * (btrfs_register_one_device() fails if O_EXCL is on)
> -	 */
> -	fd = open(file, oflags);
> -	if (fd < 0) {
> -		error("unable to open %s: %m", file);
> +	if (!t_prepare || !prepare_ctx) {
> +		error("unable to prepare dev");

Isn't this ENOMEM? The message doesn't seem to match the situation.

>   		goto error;
>   	}
> -	ret = btrfs_prepare_device(fd, file, &dev_block_count, block_count,
> -			(zero_end ? PREP_DEVICE_ZERO_END : 0) |
> -			(discard ? PREP_DEVICE_DISCARD : 0) |
> -			(bconf.verbose ? PREP_DEVICE_VERBOSE : 0) |
> -			(zoned ? PREP_DEVICE_ZONED : 0));
> +
> +	for (i = 0; i < dev_cnt; i++) {
> +		prepare_ctx[i].file = argv[optind + i - 1];
> +		prepare_ctx[i].block_count = block_count;
> +		prepare_ctx[i].dev_block_count = block_count;
> +		prepare_ctx[i].zero_end = zero_end;
> +		prepare_ctx[i].discard = discard;
> +		prepare_ctx[i].zoned = zoned;
> +		if (i == 0) {
> +			prepare_ctx[i].oflags = O_RDWR;
> +			/*
> +			 * Open without O_EXCL so that the problem should
> +			 * not occur by the following operation in kernel:
> +			 * (btrfs_register_one_device() fails if O_EXCL is on)
> +			 */

The comment seems out-dated, no O_EXCL involved anywhere.

> +			if (zoned && zoned_model(file) == ZONED_HOST_MANAGED)
> +				prepare_ctx[i].oflags = O_RDWR | O_DIRECT;

Do we need to treat the initial and other devices differently?

Can't we use the same flags for all devices?


> +		} else {
> +			prepare_ctx[i].oflags = O_RDWR;
> +		}
> +		ret = pthread_create(&t_prepare[i], NULL,
> +			prepare_one_dev, &prepare_ctx[i]);
> +	}
> +	pthread_join(t_prepare[0], NULL);
> +	ret = prepare_ctx[0].ret; > +

Can't we just wait for all devices?

I don't think treating them different could have much benefit.

Yes, we can have multiple-devices with different performance
characteristics, thus if the first device is the fastest one, it may
finish before all the others.

But this also means, the first one can be the slowest.

To me, parallel initialization is already a big enough improvement, and
for the most common case, all the devices should have the same or
similar performance characteristics, thus waiting for them all shouldn't
cause much difference.

>   	if (ret)
>   		goto error;
> +
> +	dev_cnt--;
> +	fd = open(file, prepare_ctx[0].oflags);
> +	dev_block_count = prepare_ctx[0].dev_block_count;
>   	if (block_count && block_count > dev_block_count) {
>   		error("%s is smaller than requested size, expected %llu, found %llu",
>   		      file, (unsigned long long)block_count,
> @@ -1459,7 +1511,7 @@ int BOX_MAIN(mkfs)(int argc, char **argv)
>   	}
>
>   	/* To create the first block group and chunk 0 in make_btrfs */
> -	system_group_size = zoned ?  zone_size(file) : BTRFS_MKFS_SYSTEM_GROUP_SIZE;
> +	system_group_size = zoned ? zone_size(file) : BTRFS_MKFS_SYSTEM_GROUP_SIZE;
>   	if (dev_block_count < system_group_size) {
>   		error("device is too small to make filesystem, must be at least %llu",
>   				(unsigned long long)system_group_size);
> @@ -1557,6 +1609,12 @@ int BOX_MAIN(mkfs)(int argc, char **argv)
>   	if (dev_cnt == 0)
>   		goto raid_groups;
>
> +	for (i = 0 ; i < dev_cnt; i++) {
> +		pthread_join(t_prepare[i+1], NULL);
> +		if (prepare_ctx[i+1].ret) {
> +			goto error;
> +		}
> +	}
>   	while (dev_cnt-- > 0) {
>   		file = argv[optind++];
>
> @@ -1578,12 +1636,9 @@ int BOX_MAIN(mkfs)(int argc, char **argv)
>   			close(fd);
>   			continue;
>   		}
> -		ret = btrfs_prepare_device(fd, file, &dev_block_count,
> -				block_count,
> -				(bconf.verbose ? PREP_DEVICE_VERBOSE : 0) |
> -				(zero_end ? PREP_DEVICE_ZERO_END : 0) |
> -				(discard ? PREP_DEVICE_DISCARD : 0) |
> -				(zoned ? PREP_DEVICE_ZONED : 0));
> +		dev_block_count = prepare_ctx[argc - saved_optind - dev_cnt - 1]
> +			.dev_block_count;
> +
>   		if (ret) {
>   			goto error;
>   		}

This goto error is a dead code now.

Thanks for the great idea on reducing the preparation time!
Qu

> @@ -1763,12 +1818,16 @@ out:
>
>   	btrfs_close_all_devices();
>   	free(label);
> -
> +	free(t_prepare);
> +	free(prepare_ctx);
>   	return !!ret;
> +
>   error:
>   	if (fd > 0)
>   		close(fd);
>
> +	free(t_prepare);
> +	free(prepare_ctx);
>   	free(label);
>   	exit(1);
>   success: