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

[l@damenly.org]

On 2023-12-04 09:44, Su Yue wrote:
> 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 ;)
> 
And maybe an option to skip check? People who want to convert to BG tree
usually have large filesystems, then the original check can be killed
because of OOM...

--
Su

> --
> 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;