On 2022/11/7 14:13, Zorro Lang wrote:
On Mon, Nov 07, 2022 at 12:41:34PM +0800, Qu Wenruo wrote:
[...]
btrfs/278 has been taken, please rebase on the latest for-next branch, or
use a big enough number.
I'm not sure if some one else has already complained about this, the idea of
a for-next branch of a test suite is stupid (nor the weekly tags).
Fstests is a test suite, it is only for fs developers, I doubt why we need
tags/for-next at all.
1)
I think fstests isn't only for developers, there're many people, they even
never contributed to fstests or linux, but they still need fstests to do
sanity/regression test daily/weekly for some downstream things.
2)
I don't doubt the tags are useless for most users, but as I know some people
enjoy it. And the tags don't bring in troubles to others don't use it, so why
can't I give it tags?
Tags are providing a stable anchor for downsteam QAers to rely on
certain tags.
Which is a bad behavior and should be discouraged.
The best way to discourage this thing is, just no tags, no branches,
forcing them to go the latest master.
Even for downstream QA, they should use the latest master.
They should have their own systems to detect new tests, and if new tests
fails, they really need to understand why, and better to submit a patch
to fix fstests.
3)
Although fstests is a test tool, but it still might bring in big/small
regression issues. Someone might submit lots of testing jobs after push some
changes, and happy to sleep or enjoy their weekend (wait the test done).
But when they come back, they find all test jobs are broken due to fstests
regression, they get nothing valid testing results. How despondent are they?
This is a bad behavior already, passing tests means nothing.
Only failed tests are good ones, even if it's a false alert.
Such failures drives us to improve the project.
Especially this already means they may miss some high priority regressions.
Some testing jobs are just automatical testing, they don't want to deal with
fstests issues manually. So the master branch trys to keep stable. The patches
will be in for-next branch at first, then merge onto master branch if no one
complains these patches.
That's a bad argument, convenience should not have a higher priority
than possible regressions/bugs.
And if QA guys want a stable basis for their downstream tests, they
should contribute to fstests, not relying on old tags to give them a
false joy of passing tests.
At least from my limited understanding, more commits to upstream
projects would always be a plus for QA people.
The for-next is for fstests developers, and other developers who always want
to use lastest update, and don't mind (even enjoy) dealing with fstests issues
(if there's) manually. The master branch is for the ones who just hope to get
a stabler (relative) for their automatical testing frame.
It's fine to not update fstests hourly, but relying on an old
branch/tags for months just to make the existing ones to pass, no, this
is definitely not the way to go.
Remember, before the whole for-next/tags things, developers just checkout
the master branch and go ./new, no need to bother the tags/branches at all.
Now you can treat for-next branch as master branch you used before. As your
using habits, you can always base on for-next branch, master branch isn't for
developers as you now.
Another point is, if you don't plan to backport tests for older tags,
just don't keep those tags, it makes no help.
Without tags, for-next doesn't make much sense either.
If some users don't want cutting edges, they always have the choice to
not update their local fstests that frequently (but hopefully not for
too long time).
But still, just one master branch is a very strong gesture to discourage
incorrect QA behavior.
2 files changed, 68 insertions(+)
create mode 100755 tests/btrfs/278
create mode 100644 tests/btrfs/278.out
diff --git a/tests/btrfs/278 b/tests/btrfs/278
new file mode 100755
index 00000000..ebbf207a
--- /dev/null
+++ b/tests/btrfs/278
@@ -0,0 +1,66 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (C) 2022 SUSE Linux Products GmbH. All Rights Reserved.
+#
+# FS QA Test 278
+#
+# A regression test for offending commit 786672e9e1a3 ("btrfs: scrub: use
+# larger block size for data extent scrub"), which makes btrfs scrub unable
+# to detect corruption if it's not the first sector of an data extent.
So 786672e9e1a3 is the commit which brought in this regression issue? Is there
a fix patch or commit by reviewed?
The fix (by reverting it) is send to btrfs mailing list, titled "Revert
\"btrfs: scrub: use larger block size for data extent scrub\"".
OK, thanks.
+#
+. ./common/preamble
+_begin_fstest auto quick scrub
+
+# Import common functions.
+. ./common/filter
+. ./common/btrfs
The common/btrfs is imported automatically, so don't need to import it at here.
And I think this case doesn't use any filter, if so, the common/filter isn't
needed either.
+
+# real QA test starts here
+
+# Modify as appropriate.
This comment can be removed.
If you really believe removing those boilerplate makes much sense, then I'd
say, the template should be updated to remove those completely.
Hmm... there're many comments in template should be removed when we start to
write a new case. Most of them are always removed properly, only this line
always be missed.
I don't doubt the template can be improved. But it's a tiny problem, so I
planned to improve that next time when we have chance to improve the template.
Before that, if a patch is all good, only this single line can be removed, I'll
ignore it. If a patch need to change, I'll point out this line incidentally :)
Another thing is, such nitpicking is a little too common.
From my limited experience of previous maintainer Guan, he would either
just fix it when merging.
Which is mostly inline with the fs maintainers, and makes the whole
process so smooth.
I really appreciate the maintenance effort and valuable comments (like
the one for "-s 4k"), but these comments on something from the template?
No, those makes no sense.
Thanks,
Qu
+_supported_fs btrfs
+
+# Need to use 4K as sector size
+_require_btrfs_support_sectorsize 4096
+_require_scratch
+
+_scratch_mkfs >> $seqres.full
Just check with you, do you need "-s 4k" so make sure sector size is 4k (even
if 4k is supported)?
I'll add "-s 4k" to make it more explicit for systems with larger page
sizes.
Thanks,
Zorro
Thanks,
Qu
Thanks,
Zorro
+_scratch_mount
+
+# Create a data extent with 2 sectors
+$XFS_IO_PROG -fc "pwrite -S 0xff 0 8k" $SCRATCH_MNT/foobar >> $seqres.full
+sync
+
+first_logical=$(_btrfs_get_first_logical $SCRATCH_MNT/foobar)
+echo "logical of the first sector: $first_logical" >> $seqres.full
+
+second_logical=$(( $first_logical + 4096 ))
+echo "logical of the second sector: $second_logical" >> $seqres.full
+
+second_physical=$(_btrfs_get_physical $second_logical 1)
+echo "physical of the second sector: $second_physical" >> $seqres.full
+
+second_dev=$(_btrfs_get_device_path $second_logical 1)
+echo "device of the second sector: $second_dev" >> $seqres.full
+
+_scratch_unmount
+
+# Corrupt the second sector of the data extent.
+$XFS_IO_PROG -c "pwrite -S 0x00 $second_physical 4k" $second_dev >> $seqres.full
+_scratch_mount
+
+# Redirect stderr and stdout, as if btrfs detected the unrepairable corruption,
+# it will output an error message.
+$BTRFS_UTIL_PROG scrub start -B $SCRATCH_MNT &> $tmp.output
+cat $tmp.output >> $seqres.full
+_scratch_unmount
+
+if ! grep -q "csum=1" $tmp.output; then
+ echo "Scrub failed to detect corruption"
+fi
+
+echo "Silence is golden"
+
+# success, all done
+status=0
+exit
diff --git a/tests/btrfs/278.out b/tests/btrfs/278.out
new file mode 100644
index 00000000..b4c4a95d
--- /dev/null
+++ b/tests/btrfs/278.out
@@ -0,0 +1,2 @@
+QA output created by 278
+Silence is golden
--
2.38.0
