generic: Add some tests around journal replay/recoveryloop
v3 cover generic: Add some tests around journal replay/recoveryloop
Hello,
The ext4 fast_commit kernel fix has landed into mainline tree [1]. In this v3, I have addressed review comments from Darrick. Does this looks good to be picked up?
I have tested ext4 1k, 4k (w & w/o fast_commit). Also tested other FS with default configs (like xfs, btrfs, f2fs). No surprises were seen.
-ritesh
Changelogs:
===========
v2 => v3
=========
1. Addressed review comments from Darrick.
2. Rebased to latest master.
v1 => v2
=========
Sending v2 with tests/ext4/ converted to tests/generic/
(although I had not received any review comments on v1).
It seems all of the tests which I had sent in v1 are not ext4 specific anyways. So in v2, I have made those into tests/generic/.
Summary
=========
These are some of the tests which when tested with ext4 fast_commit feature w/o kernel fixes, could cause tests failures and/or KASAN bug (generic/486).
I gave these tests a run with default xfs, btrfs and f2fs configs (along with ext4). No surprises observed.
[v2]: https://lore.kernel.org/all/[email protected]/
[v1]: https://lore.kernel.org/all/[email protected]/
Ritesh Harjani (4):
generic/468: Add another falloc test entry common/punch: Add block_size argument to _filter_fiemap_** generic/678: Add a new shutdown recovery test generic/679: Add a test to check unwritten extents tracking
common/punch | 9 +++--- tests/generic/468 | 8 +++++ tests/generic/468.out | 2 ++ tests/generic/678 | 72 +++++++++++++++++++++++++++++++++++++++++++ tests/generic/678.out | 7 +++++ tests/generic/679 | 65 ++++++++++++++++++++++++++++++++++++++ tests/generic/679.out | 6 ++++ 7 files changed, 165 insertions(+), 4 deletions(-) create mode 100755 tests/generic/678 create mode 100644 tests/generic/678.out create mode 100755 tests/generic/679 create mode 100644 tests/generic/679.out
--
2.31.1
PATCH v3 1/4 generic/468: Add another falloc test entry
From: Ritesh Harjani <[email protected]>
Add another falloc test entry which could hit a kernel bug with ext4 fast_commit feature w/o below kernel commit [1].
<log>
[ 410.888496][ T2743] BUG: KASAN: use-after-free in ext4_mb_mark_bb+0x26a/0x6c0 [ 410.890432][ T2743] Read of size 8 at addr ffff888171886000 by task mount/2743
This happens when falloc -k size is huge which spans across more than 1 flex block group in ext4. This causes a bug in fast_commit replay code which is fixed by kernel commit at [1].
--- tests/generic/468 | 8 ++++++++ tests/generic/468.out | 2 ++ 2 files changed, 10 insertions(+)
tests/generic/468 +8 -0
@@ -34,6 +34,13 @@ _scratch_mkfs >/dev/null 2>&1 _require_metadata_journaling $SCRATCH_DEV _scratch_mount +# blocksize and fact are used in the last case of the fsync/fdatasync test. +# This is mainly trying to test recovery operation in case where the data +# blocks written, exceeds the default flex group size (32768*4096*16) in ext4. +blocks=32768 +blocksize=4096 +fact=18 + testfile=$SCRATCH_MNT/testfile # check inode metadata after shutdown
@@ -85,6 +92,7 @@ for i in fsync fdatasync; do test_falloc $i "-k " 1024 test_falloc $i "-k " 4096 test_falloc $i "-k " 104857600 + test_falloc $i "-k " $(($blocks*$blocksize*$fact)) done status=0
tests/generic/468.out +2 -0
@@ -5,9 +5,11 @@ QA output created by 468 ==== falloc -k 1024 test with fsync ==== ==== falloc -k 4096 test with fsync ==== ==== falloc -k 104857600 test with fsync ==== +==== falloc -k 2415919104 test with fsync ==== ==== falloc 1024 test with fdatasync ==== ==== falloc 4096 test with fdatasync ==== ==== falloc 104857600 test with fdatasync ==== ==== falloc -k 1024 test with fdatasync ==== ==== falloc -k 4096 test with fdatasync ==== ==== falloc -k 104857600 test with fdatasync ==== +==== falloc -k 2415919104 test with fdatasync ====
Signature · 1 line
2.31.1
PATCH v3 2/4 common/punch: Add block_size argument to _filter_fiemap_**
In reply to Ritesh HarjaniFrom: Ritesh Harjani <[email protected]>
Add block_size paramter to _filter_fiemap_flags() and _filter_hole_fiemap(). This is used in next patches
Also this fixes some of the end of line whitespace issues while we are at it.
--- common/punch | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)
common/punch +5 -4
@@ -109,6 +109,7 @@ _filter_fiemap() _filter_fiemap_flags() { + block_size=$1 $AWK_PROG ' $3 ~ /hole/ { print $1, $2, $3;
@@ -135,19 +136,19 @@ _filter_fiemap_flags() } print $1, $2, flag_str }' | - _coalesce_extents + _coalesce_extents $block_size } -# Filters fiemap output to only print the +# Filters fiemap output to only print the # file offset column and whether or not # it is an extent or a hole _filter_hole_fiemap() { $AWK_PROG ' $3 ~ /hole/ { - print $1, $2, $3; + print $1, $2, $3; next; - } + } $5 ~ /0x[[:xdigit:]]+/ { print $1, $2, "extent"; }' |
Signature · 1 line
2.31.1
PATCH v3 3/4 generic/678: Add a new shutdown recovery test
In reply to Ritesh HarjaniFrom: Ritesh Harjani <[email protected]>
In certain cases (it is noted with ext4 fast_commit feature) that, replay phase may not delete the right range of blocks (after sudden FS shutdown) due to some operations which depends on inode->i_size (which during replay of an inode with fast_commit could be 0 for sometime).
This fstest is added to test for such scenarios for all generic fs.
This test case is based on the test case shared via Xin Yin.
--- tests/generic/678 | 72 +++++++++++++++++++++++++++++++++++++++++++ tests/generic/678.out | 7 +++++ 2 files changed, 79 insertions(+) create mode 100755 tests/generic/678 create mode 100644 tests/generic/678.out
tests/generic/678 new file +72 -0
@@ -0,0 +1,72 @@ +#! /bin/bash +# SPDX-License-Identifier: GPL-2.0 +# Copyright (c) 2022 IBM Corporation. All Rights Reserved. +# +# FS QA Test 678 +# +# This test with ext4 fast_commit feature w/o below patch missed to delete the right +# range during replay phase, since it depends upon inode->i_size (which might not be +# stable during replay phase, at least for ext4). +# 0b5b5a62b945a141: ext4: use ext4_ext_remove_space() for fast commit replay delete range +# (Based on test case shared by Xin Yin <[email protected]>) +# + +. ./common/preamble +_begin_fstest auto shutdown quick log recoveryloop + +# Override the default cleanup function. +_cleanup() +{ + cd / + rm -r -f $tmp.* +} + +# Import common functions. +. ./common/filter +. ./common/punch + +# real QA test starts here + +# Modify as appropriate. +_supported_fs generic +_require_scratch +_require_xfs_io_command "fpunch" +_require_xfs_io_command "fzero" +_require_xfs_io_command "fiemap" +_require_scratch_shutdown + +t1=$SCRATCH_MNT/foo +t2=$SCRATCH_MNT/bar + +_scratch_mkfs > $seqres.full 2>&1 + +_scratch_mount >> $seqres.full 2>&1 + +bs=$(_get_file_block_size $SCRATCH_MNT) + +# create and write data to t1 +$XFS_IO_PROG -f -c "pwrite 0 $((100*$bs))" $t1 | _filter_xfs_io_numbers + +# fzero certain range in between with -k +$XFS_IO_PROG -c "fzero -k $((40*$bs)) $((20*$bs))" $t1 + +# create and fsync a new file t2 +$XFS_IO_PROG -f -c "fsync" $t2 + +# fpunch within the i_size of a file +$XFS_IO_PROG -c "fpunch $((30*$bs)) $((20*$bs))" $t1 + +# fsync t1 to trigger journal operation +$XFS_IO_PROG -c "fsync" $t1 + +# shutdown FS now for replay journal to kick in next mount +_scratch_shutdown -v >> $seqres.full 2>&1 + +_scratch_cycle_mount + +# check fiemap reported is valid or not +$XFS_IO_PROG -c "fiemap -v" $t1 | _filter_fiemap_flags $bs + +# success, all done +status=0 +exit
tests/generic/678.out new file +7 -0
@@ -0,0 +1,7 @@ +QA output created by 678 +wrote XXXX/XXXX bytes at offset XXXX +XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +0: [0..29]: none +1: [30..49]: hole +2: [50..59]: unwritten +3: [60..99]: nonelast
Signature · 1 line
2.31.1
PATCH v3 4/4 generic/679: Add a test to check unwritten extents tracking
In reply to Ritesh HarjaniFrom: Ritesh Harjani <[email protected]>
With these sequence of operation (in certain cases like with ext4 fast_commit) could miss to track unwritten extents during replay phase
(after sudden FS shutdown).
This fstest adds a test case to test this.
5e4d0eba1ccaf19f
ext4: fix fast commit may miss tracking range for FALLOC_FL_ZERO_RANGE
--- tests/generic/679 | 65 +++++++++++++++++++++++++++++++++++++++++++ tests/generic/679.out | 6 ++++ 2 files changed, 71 insertions(+) create mode 100755 tests/generic/679 create mode 100644 tests/generic/679.out
tests/generic/679 new file +65 -0
@@ -0,0 +1,65 @@ +#! /bin/bash +# SPDX-License-Identifier: GPL-2.0 +# Copyright (c) 2022 IBM Corporation. All Rights Reserved. +# +# FS QA Test 679 +# +# Test below sequence of operation which (w/o below kernel patch) in case of +# ext4 with fast_commit may misss to track unwritten extents. +# commit 5e4d0eba1ccaf19f +# ext4: fix fast commit may miss tracking range for FALLOC_FL_ZERO_RANGE +# +. ./common/preamble +_begin_fstest auto quick log shutdown recoveryloop + +# Override the default cleanup function. +_cleanup() +{ + cd / + rm -r -f $tmp.* +} + +# Import common functions. +. ./common/filter +. ./common/punch + +# real QA test starts here + +# Modify as appropriate. +_supported_fs generic +_require_scratch +_require_xfs_io_command "fzero" +_require_xfs_io_command "fiemap" +_require_scratch_shutdown + +t1=$SCRATCH_MNT/t1 + +_scratch_mkfs > $seqres.full 2>&1 + +_scratch_mount >> $seqres.full 2>&1 + +bs=$(_get_file_block_size $SCRATCH_MNT) + +# create and write data to t1 +$XFS_IO_PROG -f -c "pwrite 0 $((100*$bs))" $t1 | _filter_xfs_io_numbers + +# fsync t1 +$XFS_IO_PROG -c "fsync" $t1 + +# fzero certain range in between +$XFS_IO_PROG -c "fzero -k $((40*$bs)) $((20*$bs))" $t1 + +# fsync t1 +$XFS_IO_PROG -c "fsync" $t1 + +# shutdown FS now for replay of journal to kick during next mount +_scratch_shutdown -v >> $seqres.full 2>&1 + +_scratch_cycle_mount + +# check fiemap reported is valid or not +$XFS_IO_PROG -c "fiemap -v" $t1 | _filter_fiemap_flags $bs + +# success, all done +status=0 +exit
tests/generic/679.out new file +6 -0
@@ -0,0 +1,6 @@ +QA output created by 679 +wrote XXXX/XXXX bytes at offset XXXX +XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +0: [0..39]: none +1: [40..59]: unwritten +2: [60..99]: nonelast
Signature · 1 line
2.31.1
Quoted 1 line · Ritesh Harjani
Hello,
Hi,
Your below patches looks like not pure text format, they might contain binary character or some special characers, looks like the "^M" [1].
So please check how you generate/edit/send these patches, make sure they're pure text for Linux then send again.
Thanks,
Zorro
[1]
# cat -A /path/to/your_patch index 95752d3b..5e73cff9 100755^M$ --- a/tests/generic/468^M$ +++ b/tests/generic/468^M$ @@ -34,6 +34,13 @@ _scratch_mkfs >/dev/null 2>&1^M$ _require_metadata_journaling $SCRATCH_DEV^M$ _scratch_mount^M$ ^M$ +# blocksize and fact are used in the last case of the fsync/fdatasync test.^M$ +# This is mainly trying to test recovery operation in case where the data^M$ +# blocks written, exceeds the default flex group size (32768*4096*16) in ext4.^M$ +blocks=32768^M$ +blocksize=4096^M$ +fact=18^M$ +^M$ ... ...
Quoted 61 lines
The ext4
fast_commitkernel fix has landed into mainline tree [1]. In this v3, I have addressed review comments from Darrick. Does this looks good to be picked up?I have tested ext4 1k, 4k (w & w/o
fast_commit). Also tested other FS with default configs (like xfs, btrfs, f2fs). No surprises were seen.-ritesh
Changelogs:
===========v2 => v3
=========
1. Addressed review comments from Darrick.
2. Rebased to latest master.v1 => v2
=========
Sending v2 with tests/ext4/ converted to tests/generic/
(although I had not received any review comments on v1).
It seems all of the tests which I had sent in v1 are not ext4 specific anyways. So in v2, I have made those into tests/generic/.Summary
=========
These are some of the tests which when tested with ext4fast_commitfeature w/o kernel fixes, could cause tests failures and/or KASAN bug (generic/486).I gave these tests a run with default xfs, btrfs and f2fs configs (along with ext4). No surprises observed.
[v2]: https://lore.kernel.org/all/[email protected]/
[v1]: https://lore.kernel.org/all/[email protected]/Ritesh Harjani (4):
generic/468: Add another falloc test entry common/punch: Addblock_sizeargument to _filter_fiemap_** generic/678: Add a new shutdown recovery test generic/679: Add a test to check unwritten extents trackingcommon/punch | 9 +++--- tests/generic/468 | 8 +++++ tests/generic/468.out | 2 ++ tests/generic/678 | 72 +++++++++++++++++++++++++++++++++++++++++++ tests/generic/678.out | 7 +++++ tests/generic/679 | 65 ++++++++++++++++++++++++++++++++++++++ tests/generic/679.out | 6 ++++ 7 files changed, 165 insertions(+), 4 deletions(-) create mode 100755 tests/generic/678 create mode 100644 tests/generic/678.out create mode 100755 tests/generic/679 create mode 100644 tests/generic/679.out--
2.31.1
Quoted 7 lines · Zorro Lang
Quoted 1 line · Ritesh Harjani
Hello,
Hi,
Your below patches looks like not pure text format, they might contain binary character or some special characers, looks like the "^M" [1].
Ohh, is it. Might be some settings needed to be tweaked for using gmail for community patches.
Quoted 3 lines
So please check how you generate/edit/send these patches, make sure they're pure text for Linux then send again.
Sure Zorro, I will check it again at my end. However, when I tried using b4 am -o - [email protected] | git am
It worked fine for me and applied all 4 patches to my local tree.
I also didn't notice any of such characters in my other inbox ("[email protected]"), which was cc'd too. hmm, strange.
Sure, I will check it once again at my end and resend this.
-ritesh
Quoted 86 lines
Thanks,
Zorro[1]
# cat -A /path/to/your_patch index 95752d3b..5e73cff9 100755^M$tests/generic/468^M$ +7
-0@@ -34,6 +34,13 @@ _scratch_mkfs >/dev/null 2>&1^M$ _require_metadata_journaling $SCRATCH_DEV^M$ _scratch_mount^M$ ^M$ +# blocksize and fact are used in the last case of the fsync/fdatasync test.^M$ +# This is mainly trying to test recovery operation in case where the data^M$ +# blocks written, exceeds the default flex group size (32768*4096*16) in ext4.^M$ +blocks=32768^M$ +blocksize=4096^M$ +fact=18^M$ +^M$Quoted 62 lines
The ext4
fast_commitkernel fix has landed into mainline tree [1]. In this v3, I have addressed review comments from Darrick. Does this looks good to be picked up?I have tested ext4 1k, 4k (w & w/o
fast_commit). Also tested other FS with default configs (like xfs, btrfs, f2fs). No surprises were seen.-ritesh
Changelogs:
===========v2 => v3
=========
1. Addressed review comments from Darrick.
2. Rebased to latest master.v1 => v2
=========
Sending v2 with tests/ext4/ converted to tests/generic/
(although I had not received any review comments on v1).
It seems all of the tests which I had sent in v1 are not ext4 specific anyways. So in v2, I have made those into tests/generic/.Summary
=========
These are some of the tests which when tested with ext4fast_commitfeature w/o kernel fixes, could cause tests failures and/or KASAN bug (generic/486).I gave these tests a run with default xfs, btrfs and f2fs configs (along with ext4). No surprises observed.
[v2]: https://lore.kernel.org/all/[email protected]/
[v1]: https://lore.kernel.org/all/[email protected]/Ritesh Harjani (4):
generic/468: Add another falloc test entry common/punch: Addblock_sizeargument to _filter_fiemap_** generic/678: Add a new shutdown recovery test generic/679: Add a test to check unwritten extents trackingcommon/punch | 9 +++--- tests/generic/468 | 8 +++++ tests/generic/468.out | 2 ++ tests/generic/678 | 72 +++++++++++++++++++++++++++++++++++++++++++ tests/generic/678.out | 7 +++++ tests/generic/679 | 65 ++++++++++++++++++++++++++++++++++++++ tests/generic/679.out | 6 ++++ 7 files changed, 165 insertions(+), 4 deletions(-) create mode 100755 tests/generic/678 create mode 100644 tests/generic/678.out create mode 100755 tests/generic/679 create mode 100644 tests/generic/679.out--
2.31.1
Quoted 8 lines · Ritesh Harjani
Quoted 7 lines · Zorro Lang
Quoted 1 line · Ritesh Harjani
Hello,
Hi,
Your below patches looks like not pure text format, they might contain binary character or some special characers, looks like the "^M" [1].
Sorry to bother you. But here is what I tried.
1. Download the mbx file using b4 am. I didn't see any such character ("^M") in the patches.
2. Saved the patch using mutt. Again didn't see such character while doing cat -A /patch/to/patch
3. Downloaded the mail using eml format from webmail. Here I do see this character appended. But that happens not just for my patch, but for all other patches too.
So could this be related to the way you are downloading these patches. Please let me know, if I need to resend these patches again? Because, I don't see this behavior at my end. But I would happy to correct it, if that's not the case.
-ritesh
Quoted 19 lines · Ritesh Harjani
Quoted 8 lines · Ritesh Harjani
Quoted 7 lines · Zorro Lang
Quoted 1 line · Ritesh Harjani
Hello,
Hi,
Your below patches looks like not pure text format, they might contain binary character or some special characers, looks like the "^M" [1].
Sorry to bother you. But here is what I tried.
1. Download the mbx file using b4 am. I didn't see any such character ("^M") in the patches.
2. Saved the patch using mutt. Again didn't see such character while doing cat -A /patch/to/patch
3. Downloaded the mail using eml format from webmail. Here I do see this character appended. But that happens not just for my patch, but for all other patches too.
Quoted 4 lines
So could this be related to the way you are downloading these patches. Please let me know, if I need to resend these patches again? Because, I don't see this behavior at my end. But I would happy to correct it, if that's not the case.
Hmm... weird, When I tried to open your patch emails, my mutt show me:
[-- application/octet-stream is unsupported (use 'v' to view this part) --]
Then I have to input 'v' to see the patch content. I'm not sure what's wrong, this's the 2nd time I hit this "octet-stream is unsupported" issue yesterday.
Hi Darrick, or any other forks, can you open above 4 patches normally? If that's only my personal issue, I'll check my side.
Thanks,
Zorro
Quoted 3 lines
-ritesh
Quoted 27 lines · Zorro Lang
Quoted 19 lines · Ritesh Harjani
Quoted 8 lines · Ritesh Harjani
Quoted 7 lines · Zorro Lang
Quoted 1 line · Ritesh Harjani
Hello,
Hi,
Your below patches looks like not pure text format, they might contain binary character or some special characers, looks like the "^M" [1].
Sorry to bother you. But here is what I tried.
1. Download the mbx file using b4 am. I didn't see any such character ("^M") in the patches.
2. Saved the patch using mutt. Again didn't see such character while doing cat -A /patch/to/patch
3. Downloaded the mail using eml format from webmail. Here I do see this character appended. But that happens not just for my patch, but for all other patches too.Quoted 4 lines
So could this be related to the way you are downloading these patches. Please let me know, if I need to resend these patches again? Because, I don't see this behavior at my end. But I would happy to correct it, if that's not the case.
Hmm... weird, When I tried to open your patch emails, my mutt show me:
Quoted 7 lines
[-- application/octet-stream is unsupported (use 'v' to view this part) --]
Then I have to input 'v' to see the patch content. I'm not sure what's wrong, this's the 2nd time I hit this "octet-stream is unsupported" issue yesterday.
Hi Darrick, or any other forks, can you open above 4 patches normally? If that's only my personal issue, I'll check my side.
Hi Zorro,
The patchset seems to open normally at my end.
Thanks,
Ojaswin
Quoted 8 lines
Thanks,
ZorroQuoted 3 lines
-ritesh
Quoted 27 lines · Zorro Lang
Quoted 19 lines · Ritesh Harjani
Quoted 8 lines · Ritesh Harjani
Quoted 7 lines · Zorro Lang
Quoted 1 line · Ritesh Harjani
Hello,
Hi,
Your below patches looks like not pure text format, they might contain binary character or some special characers, looks like the "^M" [1].
Sorry to bother you. But here is what I tried.
1. Download the mbx file using b4 am. I didn't see any such character ("^M") in the patches.
2. Saved the patch using mutt. Again didn't see such character while doing cat -A /patch/to/patch
3. Downloaded the mail using eml format from webmail. Here I do see this character appended. But that happens not just for my patch, but for all other patches too.Quoted 4 lines
So could this be related to the way you are downloading these patches. Please let me know, if I need to resend these patches again? Because, I don't see this behavior at my end. But I would happy to correct it, if that's not the case.
Hmm... weird, When I tried to open your patch emails, my mutt show me:
Quoted 7 lines
[-- application/octet-stream is unsupported (use 'v' to view this part) --]
Then I have to input 'v' to see the patch content. I'm not sure what's wrong, this's the 2nd time I hit this "octet-stream is unsupported" issue yesterday.
Hi Darrick, or any other forks, can you open above 4 patches normally? If that's only my personal issue, I'll check my side.
There's no application/octet anywhere in the email that I received. Has your IT department gone rogue^W^Wincreased value-add again?
--D
Quoted 7 lines
Thanks,
ZorroQuoted 3 lines
-ritesh
Quoted 30 lines · Darrick J. Wong
Quoted 29 lines · Zorro Lang
Quoted 19 lines · Ritesh Harjani
Quoted 8 lines · Ritesh Harjani
Quoted 7 lines · Zorro Lang
Quoted 1 line · Ritesh Harjani
Hello,
Hi,
Your below patches looks like not pure text format, they might contain binary character or some special characers, looks like the "^M" [1].
Sorry to bother you. But here is what I tried.
1. Download the mbx file using b4 am. I didn't see any such character ("^M") in the patches.
2. Saved the patch using mutt. Again didn't see such character while doing cat -A /patch/to/patch
3. Downloaded the mail using eml format from webmail. Here I do see this character appended. But that happens not just for my patch, but for all other patches too.Quoted 4 lines
So could this be related to the way you are downloading these patches. Please let me know, if I need to resend these patches again? Because, I don't see this behavior at my end. But I would happy to correct it, if that's not the case.
Hmm... weird, When I tried to open your patch emails, my mutt show me:
[-- application/octet-stream is unsupported (use 'v' to view this part) --]
Quoted 8 lines
Quoted 5 lines
Then I have to input 'v' to see the patch content. I'm not sure what's wrong, this's the 2nd time I hit this "octet-stream is unsupported" issue yesterday.
Hi Darrick, or any other forks, can you open above 4 patches normally? If that's only my personal issue, I'll check my side.
There's no application/octet anywhere in the email that I received. Has your IT department gone rogue^W^Wincreased value-add again?
Thanks Darrick and Ojaswin! I'll check with our IT department. But it's weird, only this patchset and 2 of another patch[1](it's sent 3 times) has this problem, I see all other patches normally. And Ritesh, sorry for taking your time to check it :-D
Thanks,
Zorro
[1]
[PATCH v2] generic/674: replace _require_scratch_reflink with _require_scratch_dedupe
Quoted 11 lines
--D
Quoted 7 lines
Thanks,
ZorroQuoted 3 lines
-ritesh
Re: [PATCHv3 1/4] generic/468: Add another falloc test entry
In reply to Ritesh HarjaniQuoted 26 lines · Ritesh Harjani
From: Ritesh Harjani <[email protected]>
Add another falloc test entry which could hit a kernel bug with ext4
fast_commitfeature w/o below kernel commit [1].<log>
[ 410.888496][ T2743] BUG: KASAN: use-after-free in ext4_mb_mark_bb+0x26a/0x6c0 [ 410.890432][ T2743] Read of size 8 at addr ffff888171886000 by task mount/2743This happens when falloc -k size is huge which spans across more than 1 flex block group in ext4. This causes a bug in
fast_commitreplay code which is fixed by kernel commit at [1].Signed-off-by Ritesh Harjani <[email protected]>--- tests/generic/468 | 8 ++++++++ tests/generic/468.out | 2 ++ 2 files changed, 10 insertions(+)tests/generic/468 +0
-0
tests/generic/468 _scratch_mkfs >/dev/null 2>&1 · line 41
Quoted 8 lines
tests/generic/468 +5
-0@@ -34,6 +34,13 @@ _scratch_mkfs >/dev/null 2>&1 _require_metadata_journaling $SCRATCH_DEV _scratch_mount +# blocksize and fact are used in the last case of the fsync/fdatasync test. +# This is mainly trying to test recovery operation in case where the data +# blocks written, exceeds the default flex group size (32768*4096*16) in ext4. +blocks=32768 +blocksize=4096
Block size can change based on mkfs parameters. You should extract this dynamically from the filesystem the test is being run on.
tests/generic/468 _scratch_mkfs >/dev/null 2>&1 · line 42
Quoted 1 line
tests/generic/468 +1
-0+fact=18
What is "fact" supposed to mean?
Indeed, wouldn't this simply be better as something like:
larger_than_ext4_fg_size=$((32768 * $blksize * 18))
And then
tests/generic/468 for i in fsync fdatasync; do · line 95
Quoted 8 lines
tests/generic/468 +1
-0testfile=$SCRATCH_MNT/testfile # check inode metadata after shutdown@@ -85,6 +92,7 @@ for i in fsync fdatasync; do test_falloc $i "-k " 1024 test_falloc $i "-k " 4096 test_falloc $i "-k " 104857600 + test_falloc $i "-k " $(($blocks*$blocksize*$fact))
test_falloc $i "-k " $larger_than_ext4_fg_size
And just scrub all the sizes from the golden output?
Cheers,
Dave.
Signature · 2 lines
Dave Chinner [email protected]
Re: [PATCHv3 3/4] generic/678: Add a new shutdown recovery test
In reply to Ritesh HarjaniQuoted 36 lines · Ritesh Harjani
From: Ritesh Harjani <[email protected]>
In certain cases (it is noted with ext4
fast_commitfeature) that, replay phase may not delete the right range of blocks (after sudden FS shutdown) due to some operations which depends oninode->i_size(which during replay of an inode withfast_commitcould be 0 for sometime).
This fstest is added to test for such scenarios for all generic fs.This test case is based on the test case shared via Xin Yin.
Signed-off-by Ritesh Harjani <[email protected]>--- tests/generic/678 | 72 +++++++++++++++++++++++++++++++++++++++++++ tests/generic/678.out | 7 +++++ 2 files changed, 79 insertions(+) create mode 100755 tests/generic/678 create mode 100644 tests/generic/678.outtests/generic/678 new file +12
-0@@ -0,0 +1,72 @@ +#! /bin/bash +# SPDX-License-Identifier: GPL-2.0 +# Copyright (c) 2022 IBM Corporation. All Rights Reserved. +# +# FS QA Test 678 +# +# This test with ext4 fast_commit feature w/o below patch missed to delete the right +# range during replay phase, since it depends upon inode->i_size (which might not be +# stable during replay phase, at least for ext4). +# 0b5b5a62b945a141: ext4: use ext4_ext_remove_space() for fast commit replay delete range +# (Based on test case shared by Xin Yin <[email protected]>) +#
tests/generic/678 line 22
Quoted 10 lines
tests/generic/678 new file +10
-0+ +. ./common/preamble +_begin_fstest auto shutdown quick log recoveryloop + +# Override the default cleanup function. +_cleanup() +{ + cd / + rm -r -f $tmp.* +}
It's the same as the default cleanup function.
Cheers,
Dave.
Signature · 2 lines
Dave Chinner [email protected]
Re: [PATCHv3 4/4] generic/679: Add a test to check unwritten extents tracking
In reply to Ritesh HarjaniQuoted 35 lines · Ritesh Harjani
From: Ritesh Harjani <[email protected]>
With these sequence of operation (in certain cases like with ext4
fast_commit) could miss to track unwritten extents during replay phase
(after sudden FS shutdown).This fstest adds a test case to test this.
5e4d0eba1ccaf19f
ext4: fix fast commit may miss tracking range forFALLOC_FL_ZERO_RANGESigned-off-by Ritesh Harjani <[email protected]>--- tests/generic/679 | 65 +++++++++++++++++++++++++++++++++++++++++++ tests/generic/679.out | 6 ++++ 2 files changed, 71 insertions(+) create mode 100755 tests/generic/679 create mode 100644 tests/generic/679.outtests/generic/679 new file +10
-0@@ -0,0 +1,65 @@ +#! /bin/bash +# SPDX-License-Identifier: GPL-2.0 +# Copyright (c) 2022 IBM Corporation. All Rights Reserved. +# +# FS QA Test 679 +# +# Test below sequence of operation which (w/o below kernel patch) in case of +# ext4 with fast_commit may misss to track unwritten extents. +# commit 5e4d0eba1ccaf19f +# ext4: fix fast commit may miss tracking range for FALLOC_FL_ZERO_RANGE
tests/generic/679 line 20
Quoted 10 lines
tests/generic/679 new file +10
-0+# +. ./common/preamble +_begin_fstest auto quick log shutdown recoveryloop + +# Override the default cleanup function. +_cleanup() +{ + cd / + rm -r -f $tmp.* +}
Same as default.
Quoted 51 lines
tests/generic/679 new file +45
-0+ +# Import common functions. +. ./common/filter +. ./common/punch + +# real QA test starts here + +# Modify as appropriate. +_supported_fs generic +_require_scratch +_require_xfs_io_command "fzero" +_require_xfs_io_command "fiemap" +_require_scratch_shutdown + +t1=$SCRATCH_MNT/t1 + +_scratch_mkfs > $seqres.full 2>&1 + +_scratch_mount >> $seqres.full 2>&1 + +bs=$(_get_file_block_size $SCRATCH_MNT) + +# create and write data to t1 +$XFS_IO_PROG -f -c "pwrite 0 $((100*$bs))" $t1 | _filter_xfs_io_numbers + +# fsync t1 +$XFS_IO_PROG -c "fsync" $t1 + +# fzero certain range in between +$XFS_IO_PROG -c "fzero -k $((40*$bs)) $((20*$bs))" $t1 + +# fsync t1 +$XFS_IO_PROG -c "fsync" $t1 + +# shutdown FS now for replay of journal to kick during next mount +_scratch_shutdown -v >> $seqres.full 2>&1 + +_scratch_cycle_mount + +# check fiemap reported is valid or not +$XFS_IO_PROG -c "fiemap -v" $t1 | _filter_fiemap_flags $bs + +# success, all done +status=0 +exittests/generic/679.out new file +0
-0
tests/generic/679.out line 6
Quoted 6 lines
tests/generic/679.out new file +6
-0@@ -0,0 +1,6 @@ +QA output created by 679 +wrote XXXX/XXXX bytes at offset XXXX +XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +0: [0..39]: none +1: [40..59]: unwritten +2: [60..99]: nonelast
This is a subset of the the previous test, and looks like it should be tested first before adding the second file and punch operation the previous test adds to this write/zero operations. IOWs, they look like they could easily be combined into a single test without losing anything except having an extra test that has to be run...
Cheers,
Dave.
Signature · 2 lines
Dave Chinner [email protected]
Re: [PATCHv3 1/4] generic/468: Add another falloc test entry
In reply to Dave ChinnerQuoted 30 lines · Dave Chinner
Quoted 29 lines · Ritesh Harjani
From: Ritesh Harjani <[email protected]>
Add another falloc test entry which could hit a kernel bug with ext4
fast_commitfeature w/o below kernel commit [1].<log>
[ 410.888496][ T2743] BUG: KASAN: use-after-free in ext4_mb_mark_bb+0x26a/0x6c0 [ 410.890432][ T2743] Read of size 8 at addr ffff888171886000 by task mount/2743This happens when falloc -k size is huge which spans across more than 1 flex block group in ext4. This causes a bug in
fast_commitreplay code which is fixed by kernel commit at [1].Signed-off-by Ritesh Harjani <[email protected]>--- tests/generic/468 | 8 ++++++++ tests/generic/468.out | 2 ++ 2 files changed, 10 insertions(+)tests/generic/468 +0
-0@@ -34,6 +34,13 @@ _scratch_mkfs >/dev/null 2>&1 _require_metadata_journaling $SCRATCH_DEV _scratch_mount
Quoted 9 lines
tests/generic/468 _scratch_mkfs >/dev/null 2>&1 · line 41
Quoted 5 lines
tests/generic/468 +5
-0+# blocksize and fact are used in the last case of the fsync/fdatasync test. +# This is mainly trying to test recovery operation in case where the data +# blocks written, exceeds the default flex group size (32768*4096*16) in ext4. +blocks=32768 +blocksize=4096Block size can change based on mkfs parameters. You should extract this dynamically from the filesystem the test is being run on.
Yes, but we still have kept just 4096 because, anything bigger than that like 65536 might require a bigger disk size itself to test. The overall size requirement of the disk will then become ~36G (32768 * 65536 * 18) Hence I went ahead with 4096 which is good enough for testing.
But sure, I will add a comment explaining why we have hardcoded it to 4096 so that others don't get confused. Larger than this size disk anyway doesn't get tested much right?
Quoted 12 lines
tests/generic/468 _scratch_mkfs >/dev/null 2>&1 · line 42
Quoted 1 line
tests/generic/468 +1
-0+fact=18What is "fact" supposed to mean?
Indeed, wouldn't this simply be better as something like:
larger_than_ext4_fg_size=$((32768 * $blksize * 18))
And then
Quoted 2 lines
tests/generic/468 +0
-0testfile=$SCRATCH_MNT/testfile
Quoted 9 lines
tests/generic/468 for i in fsync fdatasync; do · line 95
Quoted 6 lines
tests/generic/468 +1
-0# check inode metadata after shutdown@@ -85,6 +92,7 @@ for i in fsync fdatasync; do test_falloc $i "-k " 1024 test_falloc $i "-k " 4096 test_falloc $i "-k " 104857600 + test_falloc $i "-k " $(($blocks*$blocksize*$fact))
test_falloc$i "-k " $larger_than_ext4_fg_size
Yes, looks good to me. Thanks for suggestion.
Quoted 2 lines
And just scrub all the sizes from the golden output?
This won't be needed since I still would like to go with 4096 blocksize, to avoid a large disk size requirement which anyway won't be tested much.
If this sounds good to you, I will fix rest of the changes as discussed in the next revision.
-ritesh
Re: [PATCHv3 3/4] generic/678: Add a new shutdown recovery test
In reply to Dave ChinnerQuoted 41 lines · Dave Chinner
Quoted 40 lines · Ritesh Harjani
From: Ritesh Harjani <[email protected]>
In certain cases (it is noted with ext4
fast_commitfeature) that, replay phase may not delete the right range of blocks (after sudden FS shutdown) due to some operations which depends oninode->i_size(which during replay of an inode withfast_commitcould be 0 for sometime).
This fstest is added to test for such scenarios for all generic fs.This test case is based on the test case shared via Xin Yin.
Signed-off-by Ritesh Harjani <[email protected]>--- tests/generic/678 | 72 +++++++++++++++++++++++++++++++++++++++++++ tests/generic/678.out | 7 +++++ 2 files changed, 79 insertions(+) create mode 100755 tests/generic/678 create mode 100644 tests/generic/678.outtests/generic/678 new file +16
-0@@ -0,0 +1,72 @@ +#! /bin/bash +# SPDX-License-Identifier: GPL-2.0 +# Copyright (c) 2022 IBM Corporation. All Rights Reserved. +# +# FS QA Test 678 +# +# This test with ext4 fast_commit feature w/o below patch missed to delete the right +# range during replay phase, since it depends upon inode->i_size (which might not be +# stable during replay phase, at least for ext4). +# 0b5b5a62b945a141: ext4: use ext4_ext_remove_space() for fast commit replay delete range +# (Based on test case shared by Xin Yin <[email protected]>) +# + +. ./common/preamble +_begin_fstest auto shutdown quick log recoveryloop +
Quoted 9 lines
tests/generic/678 line 22
Quoted 6 lines
tests/generic/678 new file +6
-0+# Override the default cleanup function. +_cleanup() +{ + cd / + rm -r -f $tmp.* +}It's the same as the default cleanup function.
Sure, will remove this definition from here in next revision. Since it is available in common/preamble now.
-ritesh
Quoted 7 lines
Cheers,
Dave.
--
Dave Chinner
[email protected]
Re: [PATCHv3 4/4] generic/679: Add a test to check unwritten extents tracking
In reply to Dave ChinnerQuoted 40 lines · Dave Chinner
Quoted 39 lines · Ritesh Harjani
From: Ritesh Harjani <[email protected]>
With these sequence of operation (in certain cases like with ext4
fast_commit) could miss to track unwritten extents during replay phase
(after sudden FS shutdown).This fstest adds a test case to test this.
5e4d0eba1ccaf19f
ext4: fix fast commit may miss tracking range forFALLOC_FL_ZERO_RANGESigned-off-by Ritesh Harjani <[email protected]>--- tests/generic/679 | 65 +++++++++++++++++++++++++++++++++++++++++++ tests/generic/679.out | 6 ++++ 2 files changed, 71 insertions(+) create mode 100755 tests/generic/679 create mode 100644 tests/generic/679.outtests/generic/679 new file +14
-0@@ -0,0 +1,65 @@ +#! /bin/bash +# SPDX-License-Identifier: GPL-2.0 +# Copyright (c) 2022 IBM Corporation. All Rights Reserved. +# +# FS QA Test 679 +# +# Test below sequence of operation which (w/o below kernel patch) in case of +# ext4 with fast_commit may misss to track unwritten extents. +# commit 5e4d0eba1ccaf19f +# ext4: fix fast commit may miss tracking range for FALLOC_FL_ZERO_RANGE +# +. ./common/preamble +_begin_fstest auto quick log shutdown recoveryloop +
Quoted 8 lines
tests/generic/679 line 20
Quoted 6 lines
tests/generic/679 new file +6
-0+# Override the default cleanup function. +_cleanup() +{ + cd / + rm -r -f $tmp.* +}Same as default.
ohk. So the same _cleanup() function definition is available in common/preamble now. So I guess, we can just remove this definition from here completely.
Got it. Thanks for pointing out.
Quoted 56 lines
Quoted 56 lines
tests/generic/679 new file +45
-0+ +# Import common functions. +. ./common/filter +. ./common/punch + +# real QA test starts here + +# Modify as appropriate. +_supported_fs generic +_require_scratch +_require_xfs_io_command "fzero" +_require_xfs_io_command "fiemap" +_require_scratch_shutdown + +t1=$SCRATCH_MNT/t1 + +_scratch_mkfs > $seqres.full 2>&1 + +_scratch_mount >> $seqres.full 2>&1 + +bs=$(_get_file_block_size $SCRATCH_MNT) + +# create and write data to t1 +$XFS_IO_PROG -f -c "pwrite 0 $((100*$bs))" $t1 | _filter_xfs_io_numbers + +# fsync t1 +$XFS_IO_PROG -c "fsync" $t1 + +# fzero certain range in between +$XFS_IO_PROG -c "fzero -k $((40*$bs)) $((20*$bs))" $t1 + +# fsync t1 +$XFS_IO_PROG -c "fsync" $t1 + +# shutdown FS now for replay of journal to kick during next mount +_scratch_shutdown -v >> $seqres.full 2>&1 + +_scratch_cycle_mount + +# check fiemap reported is valid or not +$XFS_IO_PROG -c "fiemap -v" $t1 | _filter_fiemap_flags $bs + +# success, all done +status=0 +exittests/generic/679.out new file +4
-0@@ -0,0 +1,6 @@ +QA output created by 679 +wrote XXXX/XXXX bytes at offset XXXX +XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +0: [0..39]: none
Quoted 8 lines
tests/generic/679.out line 6
Quoted 2 lines
tests/generic/679.out new file +2
-0+1: [40..59]: unwritten +2: [60..99]: nonelastThis is a subset of the the previous test, and looks like it should be tested first before adding the second file and punch operation the previous test adds to this write/zero operations. IOWs, they look like they could easily be combined into a single test without losing anything except having an extra test that has to be run...
Sure, will look into combining the two in one common generic fstest.
Thanks for the review
-ritesh
Re: [PATCHv3 1/4] generic/468: Add another falloc test entry
In reply to Ritesh HarjaniQuoted 10 lines · Ritesh Harjani
Quoted 9 lines
tests/generic/468 _scratch_mkfs >/dev/null 2>&1 · line 41
Quoted 5 lines
tests/generic/468 +5
-0+# blocksize and fact are used in the last case of the fsync/fdatasync test. +# This is mainly trying to test recovery operation in case where the data +# blocks written, exceeds the default flex group size (32768*4096*16) in ext4. +blocks=32768 +blocksize=4096Block size can change based on mkfs parameters. You should extract this dynamically from the filesystem the test is being run on.
Quoted 4 lines
Yes, but we still have kept just 4096 because, anything bigger than that like 65536 might require a bigger disk size itself to test. The overall size requirement of the disk will then become ~36G (32768 * 65536 * 18) Hence I went ahead with 4096 which is good enough for testing.
What if the block size is *smaller*? For example, I run an ext4/1k configuration (which is how I test block size > page size on x86 VM's :-).
Quoted 3 lines
But sure, I will add a comment explaining why we have hardcoded it to 4096 so that others don't get confused. Larger than this size disk anyway doesn't get tested much right?
At $WORK we use a 100GB disk by default when running xfstests, and I wouldn't be surprised if theree are other folks who might use larger disk sizes.
Maybe test to see whether the scratch disk is too small for the given parameters and if so skip the test using _notrun?
- Ted
Re: [PATCHv3 1/4] generic/468: Add another falloc test entry
In reply to Ritesh HarjaniQuoted 41 lines · Ritesh Harjani
Quoted 30 lines · Dave Chinner
Quoted 29 lines · Ritesh Harjani
From: Ritesh Harjani <[email protected]>
Add another falloc test entry which could hit a kernel bug with ext4
fast_commitfeature w/o below kernel commit [1].<log>
[ 410.888496][ T2743] BUG: KASAN: use-after-free in ext4_mb_mark_bb+0x26a/0x6c0 [ 410.890432][ T2743] Read of size 8 at addr ffff888171886000 by task mount/2743This happens when falloc -k size is huge which spans across more than 1 flex block group in ext4. This causes a bug in
fast_commitreplay code which is fixed by kernel commit at [1].Signed-off-by Ritesh Harjani <[email protected]>--- tests/generic/468 | 8 ++++++++ tests/generic/468.out | 2 ++ 2 files changed, 10 insertions(+)tests/generic/468 +0
-0@@ -34,6 +34,13 @@ _scratch_mkfs >/dev/null 2>&1 _require_metadata_journaling $SCRATCH_DEV _scratch_mountQuoted 9 lines
tests/generic/468 _scratch_mkfs >/dev/null 2>&1 · line 41
Quoted 5 lines
tests/generic/468 +5
-0+# blocksize and fact are used in the last case of the fsync/fdatasync test. +# This is mainly trying to test recovery operation in case where the data +# blocks written, exceeds the default flex group size (32768*4096*16) in ext4. +blocks=32768 +blocksize=4096Block size can change based on mkfs parameters. You should extract this dynamically from the filesystem the test is being run on.
Quoted 4 lines
Yes, but we still have kept just 4096 because, anything bigger than that like 65536 might require a bigger disk size itself to test. The overall size requirement of the disk will then become ~36G (32768 * 65536 * 18) Hence I went ahead with 4096 which is good enough for testing.
If the test setup doesn't have a disk large enough, then the test should be skipped. That's what '_require_scratch_size' is for.
i.e. _require_scratch_size $larger_than_ext4_fg_size
Will do that check once we've calculated the size needed.
Quoted 3 lines
But sure, I will add a comment explaining why we have hardcoded it to 4096 so that others don't get confused. Larger than this size disk anyway doesn't get tested much right?
You shouldn't be constricting the test based on assumptions about test configurations. If someone decides to test 64k block size, then they can size their devices appropriately for the configuration they want to test. If a 64kB block size filesystem can overrun the on-disk structure and fail, then the test should exercise that and fail appropriately.
Cheers,
Dave.
Signature · 2 lines
Dave Chinner [email protected]
Re: [PATCHv3 1/4] generic/468: Add another falloc test entry
In reply to Theodore Ts'oQuoted 11 lines · Theodore Ts'o
Quoted 10 lines · Ritesh Harjani
Quoted 9 lines
tests/generic/468 _scratch_mkfs >/dev/null 2>&1 · line 41
Quoted 5 lines
tests/generic/468 +5
-0+# blocksize and fact are used in the last case of the fsync/fdatasync test. +# This is mainly trying to test recovery operation in case where the data +# blocks written, exceeds the default flex group size (32768*4096*16) in ext4. +blocks=32768 +blocksize=4096Block size can change based on mkfs parameters. You should extract this dynamically from the filesystem the test is being run on.
Quoted 7 lines
Quoted 4 lines
Yes, but we still have kept just 4096 because, anything bigger than that like 65536 might require a bigger disk size itself to test. The overall size requirement of the disk will then become ~36G (32768 * 65536 * 18) Hence I went ahead with 4096 which is good enough for testing.
What if the block size is *smaller*? For example, I run an ext4/1k configuration (which is how I test block size > page size on x86 VM's :-).
For 1k bs, this test can still reproduce the problem. Because the given size will easily overflow the required number of blocks in 1K case.
Quoted 8 lines
Quoted 4 lines
But sure, I will add a comment explaining why we have hardcoded it to 4096 so that others don't get confused. Larger than this size disk anyway doesn't get tested much right?
At $WORK we use a 100GB disk by default when running xfstests, and I wouldn't be surprised if theree are other folks who might use larger disk sizes.
Ohk, sure. Thanks for the info.
Quoted 4 lines
Maybe test to see whether the scratch disk is too small for the given parameters and if so skip the test using
_notrun?
Yes, I think I got the point. I will make the changes accordingly.
-ritesh
Re: [PATCHv3 1/4] generic/468: Add another falloc test entry
In reply to Dave ChinnerQuoted 47 lines · Dave Chinner
Quoted 41 lines · Ritesh Harjani
Quoted 30 lines · Dave Chinner
Quoted 29 lines · Ritesh Harjani
From: Ritesh Harjani <[email protected]>
Add another falloc test entry which could hit a kernel bug with ext4
fast_commitfeature w/o below kernel commit [1].<log>
[ 410.888496][ T2743] BUG: KASAN: use-after-free in ext4_mb_mark_bb+0x26a/0x6c0 [ 410.890432][ T2743] Read of size 8 at addr ffff888171886000 by task mount/2743This happens when falloc -k size is huge which spans across more than 1 flex block group in ext4. This causes a bug in
fast_commitreplay code which is fixed by kernel commit at [1].Signed-off-by Ritesh Harjani <[email protected]>--- tests/generic/468 | 8 ++++++++ tests/generic/468.out | 2 ++ 2 files changed, 10 insertions(+)tests/generic/468 +0
-0@@ -34,6 +34,13 @@ _scratch_mkfs >/dev/null 2>&1 _require_metadata_journaling $SCRATCH_DEV _scratch_mountQuoted 9 lines
tests/generic/468 _scratch_mkfs >/dev/null 2>&1 · line 41
Quoted 5 lines
tests/generic/468 +5
-0+# blocksize and fact are used in the last case of the fsync/fdatasync test. +# This is mainly trying to test recovery operation in case where the data +# blocks written, exceeds the default flex group size (32768*4096*16) in ext4. +blocks=32768 +blocksize=4096Block size can change based on mkfs parameters. You should extract this dynamically from the filesystem the test is being run on.
Quoted 4 lines
Yes, but we still have kept just 4096 because, anything bigger than that like 65536 might require a bigger disk size itself to test. The overall size requirement of the disk will then become ~36G (32768 * 65536 * 18) Hence I went ahead with 4096 which is good enough for testing.
Quoted 6 lines
If the test setup doesn't have a disk large enough, then the test should be skipped. That's what '
_require_scratch_size' is for.i.e.
_require_scratch_size$larger_than_ext4_fg_sizeWill do that check once we've calculated the size needed.
Sure.
Quoted 11 lines
Quoted 4 lines
But sure, I will add a comment explaining why we have hardcoded it to 4096 so that others don't get confused. Larger than this size disk anyway doesn't get tested much right?
You shouldn't be constricting the test based on assumptions about test configurations. If someone decides to test 64k block size, then they can size their devices appropriately for the configuration they want to test. If a 64kB block size filesystem can overrun the on-disk structure and fail, then the test should exercise that and fail appropriately.
Sure Dave. Got the point. I will try and make the changes, such that test doesn't assume any particular user test configuration. And be generic as much as possible so that we could hit the issue we are aiming via this test.
-ritesh