Re: [PATCH] btrfs: mkfs: Make no-holes as default mkfs incompat features

[Filipe Manana <fdmanana@gmail.com>]

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
>


-- 
Filipe David Manana,

“Whether you think you can, or you think you can't — you're right.”