[PATCH] btrfs-progs: tests/misc/058: reduce the space requirement and speed up the test

[PATCH] btrfs-progs: tests/misc/058: reduce the space requirement and speed up the test

[Qu Wenruo]

[BUG]
When I was testing misc/058, the fs still has around 7GiB free space,
but during that test case, btrfs kernel module reports write failures
and even git commands failed inside that fs.

And obviously the test case failed.

[CAUSE]
It turns out that, the test case itself would require 6GiB (4 data
disks) + 1.5GiB x 2 (the two replace target), thus it requires 9 GiB
free space.

And obviously my partition is not that large and failed.

[FIX]
In fact, we really don't need that much space at all.

Our objective is to test "btrfs device replace --enqueue" functionality,
there is not much need to wait for 1 second, we can just do the enqueue
immediately.

So this patch would reduce the file size to a more sane (and rounded)
2GiB, and do the enqueue immediately.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 tests/misc-tests/058-replace-start-enqueue/test.sh | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/tests/misc-tests/058-replace-start-enqueue/test.sh b/tests/misc-tests/058-replace-start-enqueue/test.sh
index 1a24d5ec7ecb..bdbc87b4090d 100755
--- a/tests/misc-tests/058-replace-start-enqueue/test.sh
+++ b/tests/misc-tests/058-replace-start-enqueue/test.sh
@@ -21,16 +21,15 @@ run_check_mount_test_dev
 run_check $SUDO_HELPER "$TOP/btrfs" device remove "$REPLACE1" "$TEST_MNT"
 run_check $SUDO_HELPER "$TOP/btrfs" device remove "$REPLACE2" "$TEST_MNT"
 
-for i in `seq 48`; do
+for i in `seq 16`; do
 	run_check $SUDO_HELPER dd if=/dev/zero of="$TEST_MNT/file$i" bs=1M count=128 status=noxfer
 done
 # Sync so replace start does not block in unwritten IO
 run_check "$TOP/btrfs" filesystem sync "$TEST_MNT"
 run_check "$TOP/btrfs" filesystem usage -T "$TEST_MNT"
 
-# Go background, should not be that fast, estimated 10 seconds
+# Go background, should not be that fast.
 run_check $SUDO_HELPER "$TOP/btrfs" replace start 2 "$REPLACE1" "$TEST_MNT"
-run_check sleep 1
 # No background, should wait
 run_check $SUDO_HELPER "$TOP/btrfs" replace start --enqueue 3 "$REPLACE2" "$TEST_MNT"
 run_check $SUDO_HELPER "$TOP/btrfs" replace status "$TEST_MNT"
-- 
2.41.0

Re: [PATCH] btrfs-progs: tests/misc/058: reduce the space requirement and speed up the test

[David Sterba]

On Tue, Aug 08, 2023 at 02:55:21PM +0800, Qu Wenruo wrote:
> [BUG]
> When I was testing misc/058, the fs still has around 7GiB free space,
> but during that test case, btrfs kernel module reports write failures
> and even git commands failed inside that fs.
> 
> And obviously the test case failed.
> 
> [CAUSE]
> It turns out that, the test case itself would require 6GiB (4 data
> disks) + 1.5GiB x 2 (the two replace target), thus it requires 9 GiB
> free space.
> 
> And obviously my partition is not that large and failed.

The file sizes were picked so the replace is not too fast, this again
depends on the system. Please add more space for tests.

> [FIX]
> In fact, we really don't need that much space at all.
> 
> Our objective is to test "btrfs device replace --enqueue" functionality,
> there is not much need to wait for 1 second, we can just do the enqueue
> immediately.

This depends on the system and the sleep might be needed if the first
command does not start the first replace. The test is not testing just
the --enqueue, but that two replaces can be enqueued on top each other.
So we need the first one to start.

> So this patch would reduce the file size to a more sane (and rounded)
> 2GiB, and do the enqueue immediately.

I'm not sure that the test would actually work as intended after the
changes. The sleeps and dependency on system is fragile but we don't
have anything better than to over allocate and provide enough time for
the other commands to catch up.

Re: [PATCH] btrfs-progs: tests/misc/058: reduce the space requirement and speed up the test

[David Sterba]

On Wed, Aug 09, 2023 at 02:26:49PM +0200, David Sterba wrote:
> On Tue, Aug 08, 2023 at 02:55:21PM +0800, Qu Wenruo wrote:
> > [BUG]
> > When I was testing misc/058, the fs still has around 7GiB free space,
> > but during that test case, btrfs kernel module reports write failures
> > and even git commands failed inside that fs.
> > 
> > And obviously the test case failed.
> > 
> > [CAUSE]
> > It turns out that, the test case itself would require 6GiB (4 data
> > disks) + 1.5GiB x 2 (the two replace target), thus it requires 9 GiB
> > free space.
> > 
> > And obviously my partition is not that large and failed.
> 
> The file sizes were picked so the replace is not too fast, this again
> depends on the system. Please add more space for tests.
> 
> > [FIX]
> > In fact, we really don't need that much space at all.
> > 
> > Our objective is to test "btrfs device replace --enqueue" functionality,
> > there is not much need to wait for 1 second, we can just do the enqueue
> > immediately.
> 
> This depends on the system and the sleep might be needed if the first
> command does not start the first replace. The test is not testing just
> the --enqueue, but that two replaces can be enqueued on top each other.
> So we need the first one to start.
> 
> > So this patch would reduce the file size to a more sane (and rounded)
> > 2GiB, and do the enqueue immediately.
> 
> I'm not sure that the test would actually work as intended after the
> changes. The sleeps and dependency on system is fragile but we don't
> have anything better than to over allocate and provide enough time for
> the other commands to catch up.

The reduced test still reliably verifies the fix so I'll apply it.
Thanks.

Re: [PATCH] btrfs-progs: tests/misc/058: reduce the space requirement and speed up the test

[Qu Wenruo]

On 2023/8/9 20:35, David Sterba wrote:
> On Wed, Aug 09, 2023 at 02:26:49PM +0200, David Sterba wrote:
>> On Tue, Aug 08, 2023 at 02:55:21PM +0800, Qu Wenruo wrote:
>>> [BUG]
>>> When I was testing misc/058, the fs still has around 7GiB free space,
>>> but during that test case, btrfs kernel module reports write failures
>>> and even git commands failed inside that fs.
>>>
>>> And obviously the test case failed.
>>>
>>> [CAUSE]
>>> It turns out that, the test case itself would require 6GiB (4 data
>>> disks) + 1.5GiB x 2 (the two replace target), thus it requires 9 GiB
>>> free space.
>>>
>>> And obviously my partition is not that large and failed.
>>
>> The file sizes were picked so the replace is not too fast, this again
>> depends on the system. Please add more space for tests.
>>
>>> [FIX]
>>> In fact, we really don't need that much space at all.
>>>
>>> Our objective is to test "btrfs device replace --enqueue" functionality,
>>> there is not much need to wait for 1 second, we can just do the enqueue
>>> immediately.
>>
>> This depends on the system and the sleep might be needed if the first
>> command does not start the first replace. The test is not testing just
>> the --enqueue, but that two replaces can be enqueued on top each other.
>> So we need the first one to start.
>>
>>> So this patch would reduce the file size to a more sane (and rounded)
>>> 2GiB, and do the enqueue immediately.
>>
>> I'm not sure that the test would actually work as intended after the
>> changes. The sleeps and dependency on system is fragile but we don't
>> have anything better than to over allocate and provide enough time for
>> the other commands to catch up.
>
> The reduced test still reliably verifies the fix so I'll apply it.
> Thanks.

Despite the merge, I still want to discuss the principle behind the test
cases.

Unlike fstests, we don't really have strong requirement on the disk
sizes, thus most tests only go a 2GiB sparse file.

This leads to very loose disk size requirement, just like this case, we
can easily go 6GiB (more accurate 9GiB) without any warning or checks in
advance.

I believe the proper way to go in the future would be either:

- Add a proper size requirement check
   Just like xfstests

- Put more explicit recommends on the file sizes
   We can recommend something like doing IO for 4sec, and only sleep for
   1sec.
   But unfortunate this is not future proof, as modern PCIE5 drives can
   already go beyond 10GiB/s sequential writes.

Although for this particular case, I'm wondering if it's possible to do
multiple enqueue calls? E.g:

   btrfs replace start --enqueue 2 $replace_dev1
   btrfs replace start --enqueue 2 $replace_dev2
   btrfs replace start --enqueue 2 $replace_dev1

If that's possible, I'd say it's better than any of the existing method.

Thanks,
Qu

Re: [PATCH] btrfs-progs: tests/misc/058: reduce the space requirement and speed up the test

[David Sterba]

On Thu, Aug 10, 2023 at 09:06:32AM +0800, Qu Wenruo wrote:
> On 2023/8/9 20:35, David Sterba wrote:
> > On Wed, Aug 09, 2023 at 02:26:49PM +0200, David Sterba wrote:
> >> On Tue, Aug 08, 2023 at 02:55:21PM +0800, Qu Wenruo wrote:
> >>> [BUG]
> >>> When I was testing misc/058, the fs still has around 7GiB free space,
> >>> but during that test case, btrfs kernel module reports write failures
> >>> and even git commands failed inside that fs.
> >>>
> >>> And obviously the test case failed.
> >>>
> >>> [CAUSE]
> >>> It turns out that, the test case itself would require 6GiB (4 data
> >>> disks) + 1.5GiB x 2 (the two replace target), thus it requires 9 GiB
> >>> free space.
> >>>
> >>> And obviously my partition is not that large and failed.
> >>
> >> The file sizes were picked so the replace is not too fast, this again
> >> depends on the system. Please add more space for tests.
> >>
> >>> [FIX]
> >>> In fact, we really don't need that much space at all.
> >>>
> >>> Our objective is to test "btrfs device replace --enqueue" functionality,
> >>> there is not much need to wait for 1 second, we can just do the enqueue
> >>> immediately.
> >>
> >> This depends on the system and the sleep might be needed if the first
> >> command does not start the first replace. The test is not testing just
> >> the --enqueue, but that two replaces can be enqueued on top each other.
> >> So we need the first one to start.
> >>
> >>> So this patch would reduce the file size to a more sane (and rounded)
> >>> 2GiB, and do the enqueue immediately.
> >>
> >> I'm not sure that the test would actually work as intended after the
> >> changes. The sleeps and dependency on system is fragile but we don't
> >> have anything better than to over allocate and provide enough time for
> >> the other commands to catch up.
> >
> > The reduced test still reliably verifies the fix so I'll apply it.
> > Thanks.
> 
> Despite the merge, I still want to discuss the principle behind the test
> cases.
> 
> Unlike fstests, we don't really have strong requirement on the disk
> sizes, thus most tests only go a 2GiB sparse file.
> 
> This leads to very loose disk size requirement, just like this case, we
> can easily go 6GiB (more accurate 9GiB) without any warning or checks in
> advance.
> 
> I believe the proper way to go in the future would be either:
> 
> - Add a proper size requirement check
>    Just like xfstests

For tests that require some minimum size eg. to create files the minimum
size would make sense and we can make them explicit in the tests but for
vast majority I think we can let it 2G.

Tests that work on multiple devices could make sure all the device sizes
will fit on the partition in advance. This can be done by the wrappers
so we would not need to change the tests.

> - Put more explicit recommends on the file sizes
>    We can recommend something like doing IO for 4sec, and only sleep for
>    1sec.

For tests that depend on timing, yes.

>    But unfortunate this is not future proof, as modern PCIE5 drives can
>    already go beyond 10GiB/s sequential writes.

Yeah, we'd need the tests scale to the the device throughput but either
it would have to be measured before the test (and depens on the system
load) or we'd have to guess the speed class by reading the device info,
which might not be obvious due to layering.

We can assume at least HDD based devices and SSD and scale the tests for
them. NVMe devices are too fast for some tests so we won't have the
coverage on them.

> Although for this particular case, I'm wondering if it's possible to do
> multiple enqueue calls? E.g:
> 
>    btrfs replace start --enqueue 2 $replace_dev1
>    btrfs replace start --enqueue 2 $replace_dev2
>    btrfs replace start --enqueue 2 $replace_dev1
> 
> If that's possible, I'd say it's better than any of the existing method.

Right this seems to be more reliable. I designed the test by following
the steps from the report, but the bug would be reproducibe by the above
sequence too.

Re: [PATCH] btrfs-progs: tests/misc/058: reduce the space requirement and speed up the test

[Josef Bacik]

On Tue, Aug 08, 2023 at 02:55:21PM +0800, Qu Wenruo wrote:
> [BUG]
> When I was testing misc/058, the fs still has around 7GiB free space,
> but during that test case, btrfs kernel module reports write failures
> and even git commands failed inside that fs.
> 
> And obviously the test case failed.
> 
> [CAUSE]
> It turns out that, the test case itself would require 6GiB (4 data
> disks) + 1.5GiB x 2 (the two replace target), thus it requires 9 GiB
> free space.
> 
> And obviously my partition is not that large and failed.
> 
> [FIX]
> In fact, we really don't need that much space at all.
> 
> Our objective is to test "btrfs device replace --enqueue" functionality,
> there is not much need to wait for 1 second, we can just do the enqueue
> immediately.
> 
> So this patch would reduce the file size to a more sane (and rounded)
> 2GiB, and do the enqueue immediately.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>

Reviewed-by: Josef Bacik <josef@toxicpanda.com>

Thanks,

Josef