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

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

On 2023/12/4 20:41, l@damenly.org wrote:
> 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 ;)

That's a good idea.

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

I don't want to allow it to be skipped. Maybe I can add some logic to go
lowmem mode depending on the filesystem size.

Just don't want to risk any possible corruption.

Thanks,
Qu
>
> --
> 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;
>