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

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

[Qu Wenruo]

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(-)

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

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

[David Sterba]

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.

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

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 2346 bytes --]



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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Anand Jain]

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

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

[David Sterba]

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.

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

[David Sterba]

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.

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

[Filipe Manana]

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

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

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 4924 bytes --]



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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 520 bytes --]