Re: [PATCH 3/3] btrfs-progs: tune: add fsck runs before and after a full conversion

[Su Yue <l@damenly.org>]

On Mon 04 Dec 2023 at 17:26, Qu Wenruo <wqu@suse.com> wrote:

> We have two bug reports in the mailing list around block group 
> tree
> conversion.
>
> Although currently there is still no strong evidence showing 
> btrfstune
> is the culprit yet, I still want to be extra cautious.
>
> This patch would add an extra safenet for the end users, that a 
> full
> readonly btrfs check would be executed on the filesystem before 
> doing
> any full fs conversion (including bg/extent tree and csum 
> conversion).
>
> This can catch any existing health problem of the filesystem.
>
> Furthermore, after the conversion is done, there would also be a 
> "btrfs
> check" run, to make sure the conversion itself is fine, to make 
> sure we
> have done everything to make sure the problem is not from our 
> side.
>
> One thing to note is, the initial check would only happen on a 
> source
> filesystem which is not under any existing conversion.
> As a fs already under conversion can easily trigger btrfs check 
> false
> alerts.
>
Better to mention above behavior in documentation too? Or some
impatient people may give up then complain btrfs is slow ;)

--
Su
>
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  Makefile    |  3 ++-
>  tune/main.c | 55 
>  +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 57 insertions(+), 1 deletion(-)
>
> diff --git a/Makefile b/Makefile
> index 374f59b99150..47c6421781f3 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -267,7 +267,8 @@ mkfs_objects = mkfs/main.o mkfs/common.o 
> mkfs/rootdir.o
>  image_objects = image/main.o image/sanitize.o 
>  image/image-create.o image/common.o \
>  		image/image-restore.o
>  tune_objects = tune/main.o tune/seeding.o tune/change-uuid.o 
>  tune/change-metadata-uuid.o \
> -	       tune/convert-bgt.o tune/change-csum.o 
> common/clear-cache.o tune/quota.o
> +	       tune/convert-bgt.o tune/change-csum.o 
> common/clear-cache.o tune/quota.o \
> +	       check/main.o check/mode-common.o check/mode-lowmem.o 
> mkfs/common.o
>  all_objects = $(objects) $(cmds_objects) $(libbtrfs_objects) 
>  $(convert_objects) \
>  	      $(mkfs_objects) $(image_objects) $(tune_objects) 
>  $(libbtrfsutil_objects)
>
> diff --git a/tune/main.c b/tune/main.c
> index 9dcb55952b59..f05ab7c3b36e 100644
> --- a/tune/main.c
> +++ b/tune/main.c
> @@ -29,6 +29,7 @@
>  #include "kernel-shared/transaction.h"
>  #include "kernel-shared/volumes.h"
>  #include "kernel-shared/free-space-tree.h"
> +#include "kernel-shared/zoned.h"
>  #include "common/utils.h"
>  #include "common/open-utils.h"
>  #include "common/device-scan.h"
> @@ -367,6 +368,47 @@ int BOX_MAIN(btrfstune)(int argc, char 
> *argv[])
>  	if (change_metadata_uuid || random_fsid || new_fsid_str)
>  		ctree_flags |= OPEN_CTREE_USE_LATEST_BDEV;
>
> +	/*
> +	 * For fs rewrites operations, we need to verify the initial
> +	 * filesystem to make sure they are healthy.
> +	 * Otherwise the transaction protection is not going to save 
> us.
> +	 */
> +	if (to_bg_tree || to_extent_tree || csum_type != -1) {
> +		struct btrfs_super_block sb = { 0 };
> +		u64 sb_flags;
> +
> +		/*
> +		 * Here we just read out the superblock without any extra 
> check,
> +		 * as later btrfs_check() would do more comprehensive 
> check on
> +		 * it.
> +		 */
> +		ret = sbread(fd, &sb, 0);
> +		if (ret < 0) {
> +			errno = -ret;
> +			error("failed to read super block from \"%s\"", 
> device);
> +			goto free_out;
> +		}
> +		sb_flags = btrfs_super_flags(&sb);
> +		/*
> +		 * If we're not resuming the conversion, we should check 
> the fs
> +		 * first.
> +		 */
> +		if (!(sb_flags & (BTRFS_SUPER_FLAG_CHANGING_BG_TREE |
> +				  BTRFS_SUPER_FLAG_CHANGING_DATA_CSUM |
> +				  BTRFS_SUPER_FLAG_CHANGING_META_CSUM))) {
> +			bool check_ret;
> +			struct btrfs_check_options options =
> +				btrfs_default_check_options;
> +
> +			check_ret = btrfs_check(device, &options);
> +			if (check_ret) {
> +				error(
> +		"\"btrfs check\" found some errors, will not touch the 
> filesystem");
> +				ret = -EUCLEAN;
> +				goto free_out;
> +			}
> +		}
> +	}
>  	root = open_ctree_fd(fd, device, 0, ctree_flags);
>
>  	if (!root) {
> @@ -535,6 +577,19 @@ out:
>  	}
>  	close_ctree(root);
>  	btrfs_close_all_devices();
> +	/* A sucessful run finished, let's verify the fs again. */
> +	if (!ret && (to_bg_tree || to_extent_tree || csum_type)) {
> +		bool check_ret;
> +		struct btrfs_check_options options = 
> btrfs_default_check_options;
> +
> +		check_ret = btrfs_check(device, &options);
> +		if (check_ret) {
> +			error(
> +	"\"btrfs check\" found some errors after the conversion, 
> please contact the developers");
> +			ret = -EUCLEAN;
> +		}
> +
> +	}
>
>  free_out:
>  	return ret;