On Mon, Nov 11, 2019 at 6:51 AM Qu Wenruo <wqu@xxxxxxxx> 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@xxxxxxxx> > --- > 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 > -- Filipe David Manana, “Whether you think you can, or you think you can't — you're right.”
