Skip to contentSkip to conversation

generic: Add some tests around journal replay/recoveryloop

PATCH In review Ritesh Harjani 6 participants 22 messages Mar 31, 2022 – Apr 6, 2022 VFS

lore
Share & tools
lore
#0 lore raw

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.

[1]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=bfdc502a4a4c058bf4cbb1df0c297761d528f54d

-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

#1 lore raw

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

[1]: https://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git/commit/?h=dev&id=bfdc502a4a4c058bf4cbb1df0c297761d528f54d

---
 tests/generic/468     | 8 ++++++++
 tests/generic/468.out | 2 ++
 2 files changed, 10 insertions(+)
tests/generic/468 +8 -0
index 95752d3b..5e73cff9 100755
@@ -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
index b3a28d5e..a09cedb8 100644
@@ -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
#2 lore raw

Re: [PATCHv3 1/4] generic/468: Add another falloc test entry

Quoted 26 lines · Ritesh Harjani

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

[1]: https://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git/commit/?h=dev&id=bfdc502a4a4c058bf4cbb1df0c297761d528f54d

---
 tests/generic/468     | 8 ++++++++
 tests/generic/468.out | 2 ++
 2 files changed, 10 insertions(+)
tests/generic/468 +0 -0
index 95752d3b..5e73cff9 100755

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

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]
#3 lore raw

Re: [PATCHv3 1/4] generic/468: Add another falloc test entry

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

[1]: https://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git/commit/?h=dev&id=bfdc502a4a4c058bf4cbb1df0c297761d528f54d

---
 tests/generic/468     | 8 ++++++++
 tests/generic/468.out | 2 ++
 2 files changed, 10 insertions(+)
tests/generic/468 +0 -0
index 95752d3b..5e73cff9 100755
@@ -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=4096

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

Quoted 2 lines
tests/generic/468 +0 -0
 testfile=$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

#4 lore raw

Re: [PATCHv3 1/4] generic/468: Add another falloc test entry

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

Block 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

#5 lore raw

Re: [PATCHv3 1/4] generic/468: Add another falloc test entry

Quoted 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=4096

Block 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

#6 lore raw

Re: [PATCHv3 1/4] generic/468: Add another falloc test entry

In reply to Ritesh Harjani
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_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].

[1]: https://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git/commit/?h=dev&id=bfdc502a4a4c058bf4cbb1df0c297761d528f54d

---
 tests/generic/468     | 8 ++++++++
 tests/generic/468.out | 2 ++
 2 files changed, 10 insertions(+)
tests/generic/468 +0 -0
index 95752d3b..5e73cff9 100755
@@ -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=4096

Block 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]
#7 lore raw

Re: [PATCHv3 1/4] generic/468: Add another falloc test entry

Quoted 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_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].

[1]: https://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git/commit/?h=dev&id=bfdc502a4a4c058bf4cbb1df0c297761d528f54d

---
 tests/generic/468     | 8 ++++++++
 tests/generic/468.out | 2 ++
 2 files changed, 10 insertions(+)
tests/generic/468 +0 -0
index 95752d3b..5e73cff9 100755
@@ -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=4096

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

Will 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

#8 lore raw

PATCH v3 2/4 common/punch: Add block_size argument to _filter_fiemap_**

In reply to Ritesh Harjani

From: 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
index b6b8a0b9..706e7c11 100644
@@ -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
#9 lore raw

PATCH v3 3/4 generic/678: Add a new shutdown recovery test

In reply to Ritesh Harjani

From: 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
new file mode 100755
index 00000000..46a7be6c
@@ -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
new file mode 100644
index 00000000..e0992edd
@@ -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
#10 lore raw

Re: [PATCHv3 3/4] generic/678: Add a new shutdown recovery test

Quoted 36 lines · Ritesh Harjani

From: 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 +12 -0
new file mode 100755
index 00000000..46a7be6c
@@ -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]
#11 lore raw

Re: [PATCHv3 3/4] generic/678: Add a new shutdown recovery test

Quoted 41 lines · Dave Chinner
Quoted 40 lines · Ritesh Harjani

From: 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 +16 -0
new file mode 100755
index 00000000..46a7be6c
@@ -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]

#12 lore raw

PATCH v3 4/4 generic/679: Add a test to check unwritten extents tracking

In reply to 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 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
new file mode 100755
index 00000000..4f35a9cd
@@ -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
new file mode 100644
index 00000000..4d3c3377
@@ -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
#13 lore raw

Re: [PATCHv3 4/4] generic/679: Add a test to check unwritten extents tracking

Quoted 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 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 +10 -0
new file mode 100755
index 00000000..4f35a9cd
@@ -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
+exit
tests/generic/679.out new file +0 -0
new file mode 100644
index 00000000..4d3c3377

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]
#14 lore raw

Re: [PATCHv3 4/4] generic/679: Add a test to check unwritten extents tracking

Quoted 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 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 +14 -0
new file mode 100755
index 00000000..4f35a9cd
@@ -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
+exit
tests/generic/679.out new file +4 -0
new file mode 100644
index 00000000..4d3c3377
@@ -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]: 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...

Sure, will look into combining the two in one common generic fstest.

Thanks for the review
-ritesh

#15 lore raw
In reply to Ritesh Harjani
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_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.

[1]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=bfdc502a4a4c058bf4cbb1df0c297761d528f54d

-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

#16 lore raw
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_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.

[1]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=bfdc502a4a4c058bf4cbb1df0c297761d528f54d

-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

#17 lore raw
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

#18 lore raw
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

#19 lore raw
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,
Zorro

Quoted 3 lines

-ritesh

#20 lore raw
In reply to Zorro Lang
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,
Zorro

Quoted 3 lines

-ritesh

#21 lore raw
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,
Zorro

Quoted 3 lines

-ritesh