| Message ID | 20191111065004.24705-1-wqu@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: mkfs: Make no-holes as default mkfs incompat features | expand |
On Mon, Nov 11, 2019 at 02:50:04PM +0800, Qu Wenruo wrote: > No-holes feature could save 53 bytes for each hole we have, and it > provides a pretty good workaround to prevent btrfs check from reporting > non-contiguous file extent holes for mixed direct/buffered IO. > > The latter feature is more helpful for developers for handle log-writes > based test cases. Thanks. The plan to make no-holes default has been there for some time already. What it needs is a full round of testing and validation before making it default. And as defaults change rarely, I'd like to add free-space-tree as mkfs default as well, there's enough demand for that and we want to start deprecating v1 in the future. I have in my near-top todo list to do that, with the following checklist: - run fstests with various features together + the new default - release build - debugging build with UBSAN, KASAN and additional useful debugging tools - run stress tests + the new feature - check that the documentation covers the change - mkfs.btrfs help string - manual page of mkfs.btrfs: benefits, pros/cons, conversion to/from the feature (if applicable), with example commands (if applicable) - wiki documentation update - verify that all commonly used tools work with it (image, check, tune) For free-space-tree specifically, there's https://github.com/kdave/drafts/blob/master/btrfs/progs-fst-default.txt I don't have objections to the patch, that's the easy part. The above is non-coding work and is namely making sure that the usecase and usability is good, or with known documented quirks. Making it default in progs release 5.4 is IMO doable, there are probably 2-3 weeks before the release, but this task needs one or more persons willing to do the above.
On 2019/11/12 上午2:02, David Sterba wrote: > On Mon, Nov 11, 2019 at 02:50:04PM +0800, Qu Wenruo wrote: >> No-holes feature could save 53 bytes for each hole we have, and it >> provides a pretty good workaround to prevent btrfs check from reporting >> non-contiguous file extent holes for mixed direct/buffered IO. >> >> The latter feature is more helpful for developers for handle log-writes >> based test cases. > > Thanks. The plan to make no-holes default has been there for some time > already. What it needs is a full round of testing and validation before > making it default. And as defaults change rarely, I'd like to add > free-space-tree as mkfs default as well, there's enough demand for that > and we want to start deprecating v1 in the future. > > I have in my near-top todo list to do that, with the following > checklist: > > - run fstests with various features together + the new default > - release build > - debugging build with UBSAN, KASAN and additional useful debugging > tools Already running with no_holes for several previous releases. Not to mention new btrfs specific log-writes test cases are all already using this feature to avoid btrfs check failure. So I think this part should be OK. > - run stress tests + the new feature Any extra suggestions for the stress test tool? Despite that, extra 24x7 host may be needed for this test. > - check that the documentation covers the change > - mkfs.btrfs help string > - manual page of mkfs.btrfs: benefits, pros/cons, conversion to/from > the feature (if applicable), with example commands (if applicable) > - wiki documentation update Forgot this part. I'll add this info in next update. Thanks, Qu > - verify that all commonly used tools work with it (image, check, tune) > > For free-space-tree specifically, there's > https://github.com/kdave/drafts/blob/master/btrfs/progs-fst-default.txt > > I don't have objections to the patch, that's the easy part. The above is > non-coding work and is namely making sure that the usecase and usability > is good, or with known documented quirks. > > Making it default in progs release 5.4 is IMO doable, there are probably > 2-3 weeks before the release, but this task needs one or more persons > willing to do the above. >
On 12/11/19 9:01 AM, Qu Wenruo wrote: > > > On 2019/11/12 上午2:02, David Sterba wrote: >> On Mon, Nov 11, 2019 at 02:50:04PM +0800, Qu Wenruo wrote: >>> No-holes feature could save 53 bytes for each hole we have, and it >>> provides a pretty good workaround to prevent btrfs check from reporting >>> non-contiguous file extent holes for mixed direct/buffered IO. >>> >>> The latter feature is more helpful for developers for handle log-writes >>> based test cases. >> >> Thanks. The plan to make no-holes default has been there for some time >> already. What it needs is a full round of testing and validation before >> making it default. And as defaults change rarely, I'd like to add >> free-space-tree as mkfs default as well, there's enough demand for that >> and we want to start deprecating v1 in the future. >> >> I have in my near-top todo list to do that, with the following >> checklist: >> >> - run fstests with various features together + the new default >> - release build >> - debugging build with UBSAN, KASAN and additional useful debugging >> tools > Already running with no_holes for several previous releases. > > Not to mention new btrfs specific log-writes test cases are all already > using this feature to avoid btrfs check failure. > > So I think this part should be OK. > >> - run stress tests + the new feature > > Any extra suggestions for the stress test tool? > > Despite that, extra 24x7 host may be needed for this test. > To keep the ball rolling I can run stress tests part here, yep need extra suggestions on the stress test tools. Thanks, Anand >> - check that the documentation covers the change >> - mkfs.btrfs help string >> - manual page of mkfs.btrfs: benefits, pros/cons, conversion to/from >> the feature (if applicable), with example commands (if applicable) >> - wiki documentation update > > Forgot this part. > I'll add this info in next update. > > Thanks, > Qu >> - verify that all commonly used tools work with it (image, check, tune) >> >> For free-space-tree specifically, there's >> https://github.com/kdave/drafts/blob/master/btrfs/progs-fst-default.txt >> >> I don't have objections to the patch, that's the easy part. The above is >> non-coding work and is namely making sure that the usecase and usability >> is good, or with known documented quirks. >> >> Making it default in progs release 5.4 is IMO doable, there are probably >> 2-3 weeks before the release, but this task needs one or more persons >> willing to do the above. >> >
On Mon, Nov 11, 2019 at 6:51 AM Qu Wenruo <wqu@suse.com> wrote: > > No-holes feature could save 53 bytes for each hole we have, and it > provides a pretty good workaround to prevent btrfs check from reporting > non-contiguous file extent holes for mixed direct/buffered IO. > > The latter feature is more helpful for developers for handle log-writes > based test cases. So it seems your motivation is to get rid of the false fsck alerts. The good part of no-holes is that it stops using 53 bytes of metadata (file extent items) to mark holes. That's great. The not so good, but necessary, part is that fsck (btrfs check) stops checking for missing extent items, assuming that any missing extent item is because a hole was punched or as consequence of writing beyond eof. So any bug that causes a file extent item to be dropped and not replaced will not be so easy to detect anymore. That can make bugs much harder to detect, such as [1] for example, which is the most recent I remember. I have been testing this feature regularly since it was introduced way back in 2013. Since then I've fixed many bugs with it, mostly corruptions happening with send/receive and fsync. Last one fixed was for send/receive earlier this year in May. And I have yet another one for hole punch + fsync that I found last week and I've just sent a fix for it [2]. As I don't recall anyone else consistently submitting fixes or bug reports for this feature, I don't know if I should assume people (users and developers) are testing it and not finding issues or simply not testing it enough (or at all). We started having a lot of false positives (fsck complaining about missing extent items) from fstests test cases that use fsstress since fsstress was fixed to use direct IO when the test filesystem is not xfs [3]. In an old thread with you [4], I remember pointing out that most of such fsck reports were due to mixing buffered and direct IO writes. So using the no-holes feature is a way to silence the tests. However, are we sure that at this point all such fsck alerts are because of that? When I looked at it after the fsstress change, I couldn't find any case that wasn't because of that, but since then I stopped looking into it, both due to other priorities and because it's very time consuming to check that. While I'm not against making it a default, I would like to know if someone has been doing that type of verification recently. I think that's the most important point. Just running fstests with MKFS_OPTIONS="-O no-holes" is not enough IMHO. Thanks. [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=609e804d771f59dc5d45a93e5ee0053c74bbe2bf [2] https://patchwork.kernel.org/patch/11239653/ [3] https://git.kernel.org/pub/scm/fs/xfs/xfstests-dev.git/commit/?id=b669b303d02e39a62a212b87f4bd1ce259f73d10 [4] https://www.spinics.net/lists/linux-btrfs/msg75350.html > > Signed-off-by: Qu Wenruo <wqu@suse.com> > --- > common/fsfeatures.c | 2 +- > common/fsfeatures.h | 5 +++-- > 2 files changed, 4 insertions(+), 3 deletions(-) > > diff --git a/common/fsfeatures.c b/common/fsfeatures.c > index 50934bd161b0..2028be9ad312 100644 > --- a/common/fsfeatures.c > +++ b/common/fsfeatures.c > @@ -84,7 +84,7 @@ static const struct btrfs_fs_feature { > "no_holes", > VERSION_TO_STRING2(3,14), > VERSION_TO_STRING2(4,0), > - NULL, 0, > + VERSION_TO_STRING2(5,4), > "no explicit hole extents for files" }, > /* Keep this one last */ > { "list-all", BTRFS_FEATURE_LIST_ALL, NULL } > diff --git a/common/fsfeatures.h b/common/fsfeatures.h > index 3cc9452a3327..544daeeedf30 100644 > --- a/common/fsfeatures.h > +++ b/common/fsfeatures.h > @@ -21,8 +21,9 @@ > > #define BTRFS_MKFS_DEFAULT_NODE_SIZE SZ_16K > #define BTRFS_MKFS_DEFAULT_FEATURES \ > - (BTRFS_FEATURE_INCOMPAT_EXTENDED_IREF \ > - | BTRFS_FEATURE_INCOMPAT_SKINNY_METADATA) > + (BTRFS_FEATURE_INCOMPAT_EXTENDED_IREF | \ > + BTRFS_FEATURE_INCOMPAT_SKINNY_METADATA | \ > + BTRFS_FEATURE_INCOMPAT_NO_HOLES) > > /* > * Avoid multi-device features (RAID56) and mixed block groups > -- > 2.24.0 >
On 2019/11/12 下午11:19, Filipe Manana wrote: > On Mon, Nov 11, 2019 at 6:51 AM Qu Wenruo <wqu@suse.com> wrote: >> >> No-holes feature could save 53 bytes for each hole we have, and it >> provides a pretty good workaround to prevent btrfs check from reporting >> non-contiguous file extent holes for mixed direct/buffered IO. >> >> The latter feature is more helpful for developers for handle log-writes >> based test cases. > > So it seems your motivation is to get rid of the false fsck alerts. > > The good part of no-holes is that it stops using 53 bytes of metadata > (file extent items) to mark holes. That's great. > > The not so good, but necessary, part is that fsck (btrfs check) stops > checking for missing extent items, assuming that any > missing extent item is because a hole was punched or as consequence of > writing beyond eof. > So any bug that causes a file extent item to be dropped and not > replaced will not be so easy to detect anymore. That can make > bugs much harder to detect, such as [1] for example, which is the most > recent I remember. Indeed, that would be much bigger problem than false alerts from btrfs check. > > I have been testing this feature regularly since it was introduced way > back in 2013. > Since then I've fixed many bugs with it, mostly corruptions happening > with send/receive and fsync. Last one fixed was for send/receive > earlier this year in May. > And I have yet another one for hole punch + fsync that I found last > week and I've just sent a fix for it [2]. > As I don't recall anyone else consistently submitting fixes or bug > reports for this feature, I don't know if I should assume people > (users and developers) are testing it and not finding issues or simply > not testing it enough (or at all). > > We started having a lot of false positives (fsck complaining about > missing extent items) from fstests test cases that use fsstress since > fsstress was fixed to > use direct IO when the test filesystem is not xfs [3]. In an old > thread with you [4], I remember pointing out that most of such fsck > reports were due to mixing buffered and direct IO writes. > So using the no-holes feature is a way to silence the tests. However, > are we sure that at this point all such fsck alerts are because of > that? That means we need extra tool to not only to verify the data, but also the extent layout, normally something done by btrfs check. Not something we can easily handle in current test suites. Thanks, Qu > When I looked at it after the fsstress change, I couldn't find any > case that wasn't because of that, but since then I stopped looking > into it, both due to other priorities and because it's very time > consuming to check that. > > While I'm not against making it a default, I would like to know if > someone has been doing that type of verification recently. I think > that's the most important point. > Just running fstests with MKFS_OPTIONS="-O no-holes" is not enough IMHO. > > Thanks. > > [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=609e804d771f59dc5d45a93e5ee0053c74bbe2bf > [2] https://patchwork.kernel.org/patch/11239653/ > [3] https://git.kernel.org/pub/scm/fs/xfs/xfstests-dev.git/commit/?id=b669b303d02e39a62a212b87f4bd1ce259f73d10 > [4] https://www.spinics.net/lists/linux-btrfs/msg75350.html > >> >> Signed-off-by: Qu Wenruo <wqu@suse.com> >> --- >> common/fsfeatures.c | 2 +- >> common/fsfeatures.h | 5 +++-- >> 2 files changed, 4 insertions(+), 3 deletions(-) >> >> diff --git a/common/fsfeatures.c b/common/fsfeatures.c >> index 50934bd161b0..2028be9ad312 100644 >> --- a/common/fsfeatures.c >> +++ b/common/fsfeatures.c >> @@ -84,7 +84,7 @@ static const struct btrfs_fs_feature { >> "no_holes", >> VERSION_TO_STRING2(3,14), >> VERSION_TO_STRING2(4,0), >> - NULL, 0, >> + VERSION_TO_STRING2(5,4), >> "no explicit hole extents for files" }, >> /* Keep this one last */ >> { "list-all", BTRFS_FEATURE_LIST_ALL, NULL } >> diff --git a/common/fsfeatures.h b/common/fsfeatures.h >> index 3cc9452a3327..544daeeedf30 100644 >> --- a/common/fsfeatures.h >> +++ b/common/fsfeatures.h >> @@ -21,8 +21,9 @@ >> >> #define BTRFS_MKFS_DEFAULT_NODE_SIZE SZ_16K >> #define BTRFS_MKFS_DEFAULT_FEATURES \ >> - (BTRFS_FEATURE_INCOMPAT_EXTENDED_IREF \ >> - | BTRFS_FEATURE_INCOMPAT_SKINNY_METADATA) >> + (BTRFS_FEATURE_INCOMPAT_EXTENDED_IREF | \ >> + BTRFS_FEATURE_INCOMPAT_SKINNY_METADATA | \ >> + BTRFS_FEATURE_INCOMPAT_NO_HOLES) >> >> /* >> * Avoid multi-device features (RAID56) and mixed block groups >> -- >> 2.24.0 >> > >
On Tue, Nov 12, 2019 at 11:10:57AM +0800, Anand Jain wrote: > On 12/11/19 9:01 AM, Qu Wenruo wrote: > > On 2019/11/12 上午2:02, David Sterba wrote: > >> On Mon, Nov 11, 2019 at 02:50:04PM +0800, Qu Wenruo wrote: > >>> No-holes feature could save 53 bytes for each hole we have, and it > >>> provides a pretty good workaround to prevent btrfs check from reporting > >>> non-contiguous file extent holes for mixed direct/buffered IO. > >>> > >>> The latter feature is more helpful for developers for handle log-writes > >>> based test cases. > >> > >> Thanks. The plan to make no-holes default has been there for some time > >> already. What it needs is a full round of testing and validation before > >> making it default. And as defaults change rarely, I'd like to add > >> free-space-tree as mkfs default as well, there's enough demand for that > >> and we want to start deprecating v1 in the future. > >> > >> I have in my near-top todo list to do that, with the following > >> checklist: > >> > >> - run fstests with various features together + the new default > >> - release build > >> - debugging build with UBSAN, KASAN and additional useful debugging > >> tools > > Already running with no_holes for several previous releases. > > > > Not to mention new btrfs specific log-writes test cases are all already > > using this feature to avoid btrfs check failure. > > > > So I think this part should be OK. > > > >> - run stress tests + the new feature > > > > Any extra suggestions for the stress test tool? > > > > Despite that, extra 24x7 host may be needed for this test. > > > > To keep the ball rolling I can run stress tests part here, > yep need extra suggestions on the stress test tools. There are synthetic workload generators like fio, fs-mark, etc. so you can try random options and do unplug tests, or sync/umount/fsck/mount/continue a few times. Otherwise you can simulate workloads that users do, like databases, VMs, copying large files around, git is a good stressing tool as well. The point here is to get a group focus on the newly enabled features, on various hardware (cpus, memory, disks). As has been mentioned, no-holes is part of testing environments of developers so this is not completely new thing.
On Mon, Nov 11, 2019 at 07:02:56PM +0100, David Sterba wrote: > Making it default in progs release 5.4 is IMO doable, there are probably > 2-3 weeks before the release, but this task needs one or more persons > willing to do the above. For the record, defaults for mkfs will not change in 5.4.
diff --git a/common/fsfeatures.c b/common/fsfeatures.c index 50934bd161b0..2028be9ad312 100644 --- a/common/fsfeatures.c +++ b/common/fsfeatures.c @@ -84,7 +84,7 @@ static const struct btrfs_fs_feature { "no_holes", VERSION_TO_STRING2(3,14), VERSION_TO_STRING2(4,0), - NULL, 0, + VERSION_TO_STRING2(5,4), "no explicit hole extents for files" }, /* Keep this one last */ { "list-all", BTRFS_FEATURE_LIST_ALL, NULL } diff --git a/common/fsfeatures.h b/common/fsfeatures.h index 3cc9452a3327..544daeeedf30 100644 --- a/common/fsfeatures.h +++ b/common/fsfeatures.h @@ -21,8 +21,9 @@ #define BTRFS_MKFS_DEFAULT_NODE_SIZE SZ_16K #define BTRFS_MKFS_DEFAULT_FEATURES \ - (BTRFS_FEATURE_INCOMPAT_EXTENDED_IREF \ - | BTRFS_FEATURE_INCOMPAT_SKINNY_METADATA) + (BTRFS_FEATURE_INCOMPAT_EXTENDED_IREF | \ + BTRFS_FEATURE_INCOMPAT_SKINNY_METADATA | \ + BTRFS_FEATURE_INCOMPAT_NO_HOLES) /* * Avoid multi-device features (RAID56) and mixed block groups
No-holes feature could save 53 bytes for each hole we have, and it provides a pretty good workaround to prevent btrfs check from reporting non-contiguous file extent holes for mixed direct/buffered IO. The latter feature is more helpful for developers for handle log-writes based test cases. Signed-off-by: Qu Wenruo <wqu@suse.com> --- common/fsfeatures.c | 2 +- common/fsfeatures.h | 5 +++-- 2 files changed, 4 insertions(+), 3 deletions(-)