| Message ID | f919ead47c266bbb4c24ba873e565e4c36b9377a.1701672971.git.wqu@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs-progs: tune: run "btrfs check" before and after full fs conversion | expand |
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;
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;
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; >
On Mon, Dec 04, 2023 at 08:52:15PM +1030, Qu Wenruo wrote: > > 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. We have power users who know what they're doing and no way to skip the check is considered a usability bug. The check can fail for reasons that may not be due to detected problems but due to lack of memory. The heuristic regarding lowmem or original mode does not sound like a good idea to me, it seems too unreliable. > Just don't want to risk any possible corruption. Instead of forcing the check in all cases, by default it can skip the conversion and print a warning with and recommendation to run check. With an option to override it it would start right away.
On 2023/12/6 03:16, David Sterba wrote: > On Mon, Dec 04, 2023 at 08:52:15PM +1030, Qu Wenruo wrote: >>> 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. > > We have power users who know what they're doing and no way to skip the > check is considered a usability bug. The check can fail for reasons that > may not be due to detected problems but due to lack of memory. The > heuristic regarding lowmem or original mode does not sound like a good > idea to me, it seems too unreliable. That's true. > >> Just don't want to risk any possible corruption. > > Instead of forcing the check in all cases, by default it can skip the > conversion and print a warning with and recommendation to run check. > With an option to override it it would start right away. > So you mean without any special option, the btrfstune for bgt/csum change would be no-op but only outputting a warning? This would be stronger than the 10s delay of btrfs check, and require less work (no need to export btrfs check). But I'm not that confident if most users would even follow the recommendation... Thanks, Qu
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;
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. Signed-off-by: Qu Wenruo <wqu@suse.com> --- Makefile | 3 ++- tune/main.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 57 insertions(+), 1 deletion(-)