fstests: generic/260: Make it handle btrfs more gracefully

PATCH Superseded Qu Wenruo 3 participants 5 messages May 28, 2019 Btrfs file system

Share & tools

Qu Wenruo

author

2019-05-28 08:19 UTC #0 lore raw

PATCH 1/1 fstests: generic/260: Make it handle btrfs more gracefully

If a filesystem doesn't map its logical address space (normally the bytenr/blocknr returned by fiemap) directly to its devices(s), the following assumptions used in the test case is no longer true:
- trim range start beyond the end of fs should fail
- trim range start beyond the end of fs with len set should fail

Under the following example, even with just one device, btrfs can still trim the fs correctly while breaking above assumption:

0 1G 1.25G

|---------------|///////////////|-----------------| <- btrfs logical
		   |				       address space
        ------------  mapped as SINGLE
        |

0 V 256M
|///////////////| <- device address space

Thus trim range start=1G len=256M will cause btrfs to trim the 256M block group, thus return correct result.

Furthermore, there is no definitely behavior for whether a fs should trim the unmapped space.
Btrfs currently will always trim the unmapped space, but the behavior can change as large trim can be very expensive.

Despite the change to skip certain tests for btrfs, still run the following tests for btrfs:
- trim start=U64_MAX with lenght set
This will expose a bug that btrfs doesn't check overflow of the range. This bug will be fixed soon.

- trim beyond the end of the fs
This will expose a bug where btrfs could send trim command beyond the end of its device.
This bug is a regression, can be fixed by reverting c2d1b3aae336 ("btrfs:
Honour FITRIM range constraints during free space trim")

With proper fixes for btrfs, this test case should pass on btrfs, ext4, xfs.

Signed-off-by Qu Wenruo <[email protected]>

---
 common/rc             | 11 +++++++
 tests/generic/260     | 75 ++++++++++++++++++++++++++++---------------
 tests/generic/260.out |  9 +-----
 3 files changed, 62 insertions(+), 33 deletions(-)

common/rc +11 -0

index 17b89d5d..d7a5898f 100644
@@ -4005,6 +4005,17 @@ _require_fibmap()
 	rm -f $file
 }
 
+# Check if the logical address (returned by fiemap) of a fs is 1:1 mapped to
+# its underlying fs
+_is_fs_direct_mapped()
+{
+	if [ "$FSTYP" == "btrfs" ]; then
+		echo "0"
+	else
+		echo "1"
+	fi
+}
+
 _try_wipe_scratch_devs()
 {
 	test -x "$WIPEFS_PROG" || return 0

tests/generic/260 +50 ~~-25~~

129 lines not on this page · raw message

tests/generic/260.out +1 -8

index a16c4f74..f4ee2f72 100644
@@ -1,12 +1,5 @@
 QA output created by 260
-[+] Start beyond the end of fs (should fail)
-fstrim: SCRATCH_MNT: FITRIM ioctl failed: Invalid argument
-[+] Start beyond the end of fs with len set (should fail)
-fstrim: SCRATCH_MNT: FITRIM ioctl failed: Invalid argument
-[+] Start = 2^64-1 (should fail)
-fstrim: SCRATCH_MNT: FITRIM ioctl failed: Invalid argument
-[+] Start = 2^64-1 and len is set (should fail)
-fstrim: SCRATCH_MNT: FITRIM ioctl failed: Invalid argument
+[+] Optional trim range test (fs dependent)
 [+] Default length (should succeed)
 [+] Default length with start set (should succeed)
 [+] Length beyond the end of fs (should succeed)

Signature · 1 line

2.21.0

Nikolay Borisov

2019-05-28 12:48 UTC #1 lore raw

Quoted 52 lines · Qu Wenruo

If a filesystem doesn't map its logical address space (normally the bytenr/blocknr returned by fiemap) directly to its devices(s), the following assumptions used in the test case is no longer true:
- trim range start beyond the end of fs should fail
- trim range start beyond the end of fs with len set should fail
Under the following example, even with just one device, btrfs can still trim the fs correctly while breaking above assumption:
0 1G 1.25G
|---------------|///////////////|-----------------| <- btrfs logical
		   |				       address space
        ------------  mapped as SINGLE
        |
0 V 256M
|///////////////| <- device address space
Thus trim range start=1G len=256M will cause btrfs to trim the 256M block group, thus return correct result.
Furthermore, there is no definitely behavior for whether a fs should trim the unmapped space.
Btrfs currently will always trim the unmapped space, but the behavior can change as large trim can be very expensive.
Despite the change to skip certain tests for btrfs, still run the following tests for btrfs:
- trim start=U64_MAX with lenght set
This will expose a bug that btrfs doesn't check overflow of the range. This bug will be fixed soon.
- trim beyond the end of the fs
This will expose a bug where btrfs could send trim command beyond the end of its device.
This bug is a regression, can be fixed by reverting c2d1b3aae336 ("btrfs:
Honour FITRIM range constraints during free space trim")
With proper fixes for btrfs, this test case should pass on btrfs, ext4, xfs.
Signed-off-by Qu Wenruo <[email protected]>
---
 common/rc             | 11 +++++++
 tests/generic/260     | 75 ++++++++++++++++++++++++++++---------------
 tests/generic/260.out |  9 +-----
 3 files changed, 62 insertions(+), 33 deletions(-)
common/rc +0 -0
index 17b89d5d..d7a5898f 100644

common/rc _require_fibmap() · line 4009

Quoted 5 lines

common/rc +2 -0

@@ -4005,6 +4005,17 @@ _require_fibmap()
 	rm -f $file
 }
 
+# Check if the logical address (returned by fiemap) of a fs is 1:1 mapped to
+# its underlying fs

"underlying device" ?

common/rc _require_fibmap() · line 4017

Quoted 8 lines

common/rc +8 -0

+_is_fs_direct_mapped()
+{
+	if [ "$FSTYP" == "btrfs" ]; then
+		echo "0"
+	else
+		echo "1"
+	fi
+}

Instead of using echo just return 0 or 1 then see below

Quoted 46 lines

common/rc +1 -0

+
 _try_wipe_scratch_devs()
 {
 	test -x "$WIPEFS_PROG" || return 0

tests/generic/260 +5 ~~-18~~

index 9e652dee..452f88c1 100755
@@ -27,40 +27,51 @@ _supported_fs generic
 _supported_os Linux
 _require_math
 
+rm -f $seqres.full
+
 _require_scratch
 _scratch_mkfs >/dev/null 2>&1
 _scratch_mount
 
 _require_batched_discard $SCRATCH_MNT
 
+
 fssize=$($DF_PROG -k | grep "$SCRATCH_MNT" | grep "$SCRATCH_DEV"  | awk '{print $3}')
 
 beyond_eofs=$(_math "$fssize*2048")
 max_64bit=$(_math "2^64 - 1")
 
-# All these tests should return EINVAL
-# since the start is beyond the end of
-# the file system
-
-echo "[+] Start beyond the end of fs (should fail)"
-out=$($FSTRIM_PROG -o $beyond_eofs $SCRATCH_MNT 2>&1)
-[ $? -eq 0 ] && status=1
-echo $out | _filter_scratch
-
-echo "[+] Start beyond the end of fs with len set (should fail)"
-out=$($FSTRIM_PROG -o $beyond_eofs -l1M $SCRATCH_MNT 2>&1)
-[ $? -eq 0 ] && status=1
-echo $out | _filter_scratch
-
-echo "[+] Start = 2^64-1 (should fail)"
-out=$($FSTRIM_PROG -o $max_64bit $SCRATCH_MNT 2>&1)
-[ $? -eq 0 ] && status=1
-echo $out | _filter_scratch
+# For filesystem with direct mapping, all these tests should return EINVAL
+# since the start is beyond the end of the file system

tests/generic/260 _supported_fs generic · line 55

Quoted 10 lines

tests/generic/260 +10 -0

+#
+# Skip these tests if the filesystem has its own address space mapping,
+# as it's implementation dependent.
+# E.g btrfs can map its physical address of (devid=1, physical=1M, len=1M)
+# and (devid=2, physical=2M, len=1M) combined as RAID1, and mapped to its
+# address space (logical=1G, len=1M), thus making trim beyond the end of
+# fs (device) meaningless.
+
+echo "[+] Optional trim range test (fs dependent)"
+if [ $(_is_fs_direct_mapped) -eq 1 ]; then

this check is rather ugly, instead if you return 0 on success (i.e it's directly mapped) it can be rewritten simply as:

if is_fs_direct_mapped; then

which is a lot cleaner than the $() fuckery

Quoted 26 lines

tests/generic/260 +17 -3

+	echo "[+] Start beyond the end of fs (should fail)" >> $seqres.full
+	$FSTRIM_PROG -o $beyond_eofs $SCRATCH_MNT >> $seqres.full 2>&1
+	[ $? -eq 0 ] && status=1
+
+	echo "[+] Start beyond the end of fs with len set (should fail)" >> $seqres.full
+	$FSTRIM_PROG -o $beyond_eofs -l1M $SCRATCH_MNT >> $seqres.full 2>&1
+	[ $? -eq 0 ] && status=1
+
+	# indirectly mapped fs may use this special value to trim their
+	# unmapped space, so don't do this for indirectly mapped fs.
+	echo "[+] Start = 2^64-1 (should fail)" >> $seqres.full
+	$FSTRIM_PROG -o $max_64bit $SCRATCH_MNT 2>&1 >> $seqres.full 2>&1
+	[ $? -eq 0 ] && status=1
+fi
 
-echo "[+] Start = 2^64-1 and len is set (should fail)"
-out=$($FSTRIM_PROG -o $max_64bit -l1M $SCRATCH_MNT 2>&1)
+# This should fail due to overflow no matter how the fs is implemented
+echo "[+] Start = 2^64-1 and len is set (should fail)" >> $seqres.full
+$FSTRIM_PROG -o $max_64bit -l1M $SCRATCH_MNT >> $seqres.full 2>&1
 [ $? -eq 0 ] && status=1
-echo $out | _filter_scratch
 
 _scratch_unmount
 _scratch_mkfs >/dev/null 2>&1

tests/generic/260 _scratch_unmount · line 105

Quoted 9 lines

tests/generic/260 +2 -0

@@ -86,10 +97,12 @@ _scratch_unmount
 _scratch_mkfs >/dev/null 2>&1
 _scratch_mount
 
+echo "[+] Trim an empty fs" >> $seqres.full
 # This is a bit fuzzy, but since the file system is fresh
 # there should be at least (fssize/2) free space to trim.
 # This is supposed to catch wrong FITRIM argument handling
 bytes=$($FSTRIM_PROG -v -o10M $SCRATCH_MNT | _filter_fstrim)
+echo "$bytes trimed" >> $seqres.full

Does it bring any value printing those strings in seqres.full given the output of the executed command won't be there. I think not.

tests/generic/260 fi · line 117

Quoted 9 lines

tests/generic/260 +1 -1

 if [ $bytes -gt $(_math "$fssize*1024") ]; then
 	status=1

@@ -101,7 +114,7 @@ fi
 # It is because btrfs does not have not-yet-used parts of the device
 # mapped and since we got here right after the mkfs, there is not
 # enough free extents in the root tree.
-if [ $bytes -le $(_math "$fssize*512") ] && [ $FSTYP != "btrfs" ]; then
+if [ $bytes -le $(_math "$fssize*512") ] && [ $(_is_fs_direct_mapped) -eq 1 ]; then

same thing here - make is_fs_direct_mapped plain 'return 0/1' and check its ret val directly.

Quoted 61 lines

tests/generic/260 +15 -3

 	status=1
 	echo "After the full fs discard $bytes bytes were discarded"\
 	     "however the file system is $(_math "$fssize*1024") bytes long."

@@ -141,14 +154,24 @@ esac
 _scratch_unmount
 _scratch_mkfs >/dev/null 2>&1
 _scratch_mount
+
+echo "[+] Try to trim beyond the end of the fs" >> $seqres.full
 # It should fail since $start is beyond the end of file system
-$FSTRIM_PROG -o$start -l10M $SCRATCH_MNT &> /dev/null
-if [ $? -eq 0 ]; then
+$FSTRIM_PROG -o$start -l10M $SCRATCH_MNT >> $seqres.full 2>&1
+ret=$?
+if [ $ret -eq 0 ] && [ $(_is_fs_direct_mapped) -eq 1 ]; then
 	status=1
 	echo "It seems that fs logic handling start"\
 	     "argument overflows"
 fi
 
+# For indirectly mapped fs, it shouldn't fail.
+# Btrfs will fail due to a bug in boundary check
+if [ $ret -ne 0 ] && [ $(_is_fs_direct_mapped) -eq 0 ]; then
+	status=1
+	echo "Unexpected error happened during trim"
+fi
+
 _scratch_unmount
 _scratch_mkfs >/dev/null 2>&1
 _scratch_mount

@@ -160,8 +183,10 @@ _scratch_mount
 # It is because btrfs does not have not-yet-used parts of the device
 # mapped and since we got here right after the mkfs, there is not
 # enough free extents in the root tree.
+echo "[+] Try to trim the fs with large enough len" >> $seqres.full
 bytes=$($FSTRIM_PROG -v -l$len $SCRATCH_MNT | _filter_fstrim)
-if [ $bytes -le $(_math "$fssize*512") ] && [ $FSTYP != "btrfs" ]; then
+echo "$bytes trimed" >> $seqres.full
+if [ $bytes -le $(_math "$fssize*512") ] && [ $(_is_fs_direct_mapped) == 1 ]; then
 	status=1
 	echo "It seems that fs logic handling len argument overflows"
 fi

tests/generic/260.out +1 -8

index a16c4f74..f4ee2f72 100644
@@ -1,12 +1,5 @@
 QA output created by 260
-[+] Start beyond the end of fs (should fail)
-fstrim: SCRATCH_MNT: FITRIM ioctl failed: Invalid argument
-[+] Start beyond the end of fs with len set (should fail)
-fstrim: SCRATCH_MNT: FITRIM ioctl failed: Invalid argument
-[+] Start = 2^64-1 (should fail)
-fstrim: SCRATCH_MNT: FITRIM ioctl failed: Invalid argument
-[+] Start = 2^64-1 and len is set (should fail)
-fstrim: SCRATCH_MNT: FITRIM ioctl failed: Invalid argument
+[+] Optional trim range test (fs dependent)
 [+] Default length (should succeed)
 [+] Default length with start set (should succeed)
 [+] Length beyond the end of fs (should succeed)

Qu Wenruo

2019-05-28 13:24 UTC #2 lore raw

Quoted 58 lines · Nikolay Borisov

Quoted 55 lines · Qu Wenruo
If a filesystem doesn't map its logical address space (normally the bytenr/blocknr returned by fiemap) directly to its devices(s), the following assumptions used in the test case is no longer true:
- trim range start beyond the end of fs should fail
- trim range start beyond the end of fs with len set should fail
Under the following example, even with just one device, btrfs can still trim the fs correctly while breaking above assumption:
0 1G 1.25G
|---------------|///////////////|-----------------| <- btrfs logical
		   |				       address space
        ------------  mapped as SINGLE
        |
0 V 256M
|///////////////| <- device address space
Thus trim range start=1G len=256M will cause btrfs to trim the 256M block group, thus return correct result.
Furthermore, there is no definitely behavior for whether a fs should trim the unmapped space.
Btrfs currently will always trim the unmapped space, but the behavior can change as large trim can be very expensive.
Despite the change to skip certain tests for btrfs, still run the following tests for btrfs:
- trim start=U64_MAX with lenght set
This will expose a bug that btrfs doesn't check overflow of the range. This bug will be fixed soon.
- trim beyond the end of the fs
This will expose a bug where btrfs could send trim command beyond the end of its device.
This bug is a regression, can be fixed by reverting c2d1b3aae336 ("btrfs:
Honour FITRIM range constraints during free space trim")
With proper fixes for btrfs, this test case should pass on btrfs, ext4, xfs.
Signed-off-by Qu Wenruo <[email protected]>
---
 common/rc             | 11 +++++++
 tests/generic/260     | 75 ++++++++++++++++++++++++++++---------------
 tests/generic/260.out |  9 +-----
 3 files changed, 62 insertions(+), 33 deletions(-)
common/rc +0 -0
index 17b89d5d..d7a5898f 100644
@@ -4005,6 +4005,17 @@ _require_fibmap()
 	rm -f $file
 }
 

Quoted 4 lines

common/rc _require_fibmap() · line 4009
Quoted 2 lines
common/rc +2 -0
+# Check if the logical address (returned by fiemap) of a fs is 1:1 mapped to
+# its underlying fs
"underlying device" ?

What's the proper expression?

I mean the block device on which the fs lies above.

Quoted 72 lines

common/rc _require_fibmap() · line 4017

Quoted 9 lines

common/rc +8 -0

+_is_fs_direct_mapped()
+{
+	if [ "$FSTYP" == "btrfs" ]; then
+		echo "0"
+	else
+		echo "1"
+	fi
+}

Instead of using echo just return 0 or 1 then see below

Quoted 46 lines

common/rc +1 -0

+
 _try_wipe_scratch_devs()
 {
 	test -x "$WIPEFS_PROG" || return 0

tests/generic/260 +5 ~~-18~~

index 9e652dee..452f88c1 100755
@@ -27,40 +27,51 @@ _supported_fs generic
 _supported_os Linux
 _require_math
 
+rm -f $seqres.full
+
 _require_scratch
 _scratch_mkfs >/dev/null 2>&1
 _scratch_mount
 
 _require_batched_discard $SCRATCH_MNT
 
+
 fssize=$($DF_PROG -k | grep "$SCRATCH_MNT" | grep "$SCRATCH_DEV"  | awk '{print $3}')
 
 beyond_eofs=$(_math "$fssize*2048")
 max_64bit=$(_math "2^64 - 1")
 
-# All these tests should return EINVAL
-# since the start is beyond the end of
-# the file system
-
-echo "[+] Start beyond the end of fs (should fail)"
-out=$($FSTRIM_PROG -o $beyond_eofs $SCRATCH_MNT 2>&1)
-[ $? -eq 0 ] && status=1
-echo $out | _filter_scratch
-
-echo "[+] Start beyond the end of fs with len set (should fail)"
-out=$($FSTRIM_PROG -o $beyond_eofs -l1M $SCRATCH_MNT 2>&1)
-[ $? -eq 0 ] && status=1
-echo $out | _filter_scratch
-
-echo "[+] Start = 2^64-1 (should fail)"
-out=$($FSTRIM_PROG -o $max_64bit $SCRATCH_MNT 2>&1)
-[ $? -eq 0 ] && status=1
-echo $out | _filter_scratch
+# For filesystem with direct mapping, all these tests should return EINVAL
+# since the start is beyond the end of the file system

tests/generic/260 _supported_fs generic · line 55

Quoted 10 lines

tests/generic/260 +10 -0

+#
+# Skip these tests if the filesystem has its own address space mapping,
+# as it's implementation dependent.
+# E.g btrfs can map its physical address of (devid=1, physical=1M, len=1M)
+# and (devid=2, physical=2M, len=1M) combined as RAID1, and mapped to its
+# address space (logical=1G, len=1M), thus making trim beyond the end of
+# fs (device) meaningless.
+
+echo "[+] Optional trim range test (fs dependent)"
+if [ $(_is_fs_direct_mapped) -eq 1 ]; then

this check is rather ugly, instead if you return 0 on success (i.e it's directly mapped) it can be rewritten simply as:

Quoted 3 lines

if is_fs_direct_mapped; then
which is a lot cleaner than the $() fuckery

I tried that before reverting back to the echo one.

The biggest concern is, "return 0" is completely OK for regular functions which does some work.
But for bool return, especially for case like this _is_xxx function, return 0 for true is really confusing.

Or this is just the preferred way in bash?

Quoted 30 lines

tests/generic/260 +17 -3

+	echo "[+] Start beyond the end of fs (should fail)" >> $seqres.full
+	$FSTRIM_PROG -o $beyond_eofs $SCRATCH_MNT >> $seqres.full 2>&1
+	[ $? -eq 0 ] && status=1
+
+	echo "[+] Start beyond the end of fs with len set (should fail)" >> $seqres.full
+	$FSTRIM_PROG -o $beyond_eofs -l1M $SCRATCH_MNT >> $seqres.full 2>&1
+	[ $? -eq 0 ] && status=1
+
+	# indirectly mapped fs may use this special value to trim their
+	# unmapped space, so don't do this for indirectly mapped fs.
+	echo "[+] Start = 2^64-1 (should fail)" >> $seqres.full
+	$FSTRIM_PROG -o $max_64bit $SCRATCH_MNT 2>&1 >> $seqres.full 2>&1
+	[ $? -eq 0 ] && status=1
+fi
 
-echo "[+] Start = 2^64-1 and len is set (should fail)"
-out=$($FSTRIM_PROG -o $max_64bit -l1M $SCRATCH_MNT 2>&1)
+# This should fail due to overflow no matter how the fs is implemented
+echo "[+] Start = 2^64-1 and len is set (should fail)" >> $seqres.full
+$FSTRIM_PROG -o $max_64bit -l1M $SCRATCH_MNT >> $seqres.full 2>&1
 [ $? -eq 0 ] && status=1
-echo $out | _filter_scratch
 
 _scratch_unmount
 _scratch_mkfs >/dev/null 2>&1

@@ -86,10 +97,12 @@ _scratch_unmount
 _scratch_mkfs >/dev/null 2>&1
 _scratch_mount

Quoted 9 lines

tests/generic/260 _scratch_unmount · line 105
Quoted 6 lines
tests/generic/260 +2 -0
+echo "[+] Trim an empty fs" >> $seqres.full
 # This is a bit fuzzy, but since the file system is fresh
 # there should be at least (fssize/2) free space to trim.
 # This is supposed to catch wrong FITRIM argument handling
 bytes=$($FSTRIM_PROG -v -o10M $SCRATCH_MNT | _filter_fstrim)
+echo "$bytes trimed" >> $seqres.full
Does it bring any value printing those strings in seqres.full given the output of the executed command won't be there. I think not.

When something went wrong, like no bytes get trimmed but still return 0, then this is definitely valuable.

BTW, there are cases btrfs trimmed just 0 bytes. And it's the same command. To me, this can't be more valuable.

In fact, I even considered to check $bytes to make the btrfs failure more explicit.

Thanks,
Qu

Quoted 75 lines

tests/generic/260 fi · line 117

Quoted 10 lines

tests/generic/260 +1 -1

 if [ $bytes -gt $(_math "$fssize*1024") ]; then
 	status=1

@@ -101,7 +114,7 @@ fi
 # It is because btrfs does not have not-yet-used parts of the device
 # mapped and since we got here right after the mkfs, there is not
 # enough free extents in the root tree.
-if [ $bytes -le $(_math "$fssize*512") ] && [ $FSTYP != "btrfs" ]; then
+if [ $bytes -le $(_math "$fssize*512") ] && [ $(_is_fs_direct_mapped) -eq 1 ]; then

same thing here - make is_fs_direct_mapped plain 'return 0/1' and check its ret val directly.

Quoted 61 lines

tests/generic/260 +15 -3

 	status=1
 	echo "After the full fs discard $bytes bytes were discarded"\
 	     "however the file system is $(_math "$fssize*1024") bytes long."

@@ -141,14 +154,24 @@ esac
 _scratch_unmount
 _scratch_mkfs >/dev/null 2>&1
 _scratch_mount
+
+echo "[+] Try to trim beyond the end of the fs" >> $seqres.full
 # It should fail since $start is beyond the end of file system
-$FSTRIM_PROG -o$start -l10M $SCRATCH_MNT &> /dev/null
-if [ $? -eq 0 ]; then
+$FSTRIM_PROG -o$start -l10M $SCRATCH_MNT >> $seqres.full 2>&1
+ret=$?
+if [ $ret -eq 0 ] && [ $(_is_fs_direct_mapped) -eq 1 ]; then
 	status=1
 	echo "It seems that fs logic handling start"\
 	     "argument overflows"
 fi
 
+# For indirectly mapped fs, it shouldn't fail.
+# Btrfs will fail due to a bug in boundary check
+if [ $ret -ne 0 ] && [ $(_is_fs_direct_mapped) -eq 0 ]; then
+	status=1
+	echo "Unexpected error happened during trim"
+fi
+
 _scratch_unmount
 _scratch_mkfs >/dev/null 2>&1
 _scratch_mount

@@ -160,8 +183,10 @@ _scratch_mount
 # It is because btrfs does not have not-yet-used parts of the device
 # mapped and since we got here right after the mkfs, there is not
 # enough free extents in the root tree.
+echo "[+] Try to trim the fs with large enough len" >> $seqres.full
 bytes=$($FSTRIM_PROG -v -l$len $SCRATCH_MNT | _filter_fstrim)
-if [ $bytes -le $(_math "$fssize*512") ] && [ $FSTYP != "btrfs" ]; then
+echo "$bytes trimed" >> $seqres.full
+if [ $bytes -le $(_math "$fssize*512") ] && [ $(_is_fs_direct_mapped) == 1 ]; then
 	status=1
 	echo "It seems that fs logic handling len argument overflows"
 fi

tests/generic/260.out +1 -8

index a16c4f74..f4ee2f72 100644
@@ -1,12 +1,5 @@
 QA output created by 260
-[+] Start beyond the end of fs (should fail)
-fstrim: SCRATCH_MNT: FITRIM ioctl failed: Invalid argument
-[+] Start beyond the end of fs with len set (should fail)
-fstrim: SCRATCH_MNT: FITRIM ioctl failed: Invalid argument
-[+] Start = 2^64-1 (should fail)
-fstrim: SCRATCH_MNT: FITRIM ioctl failed: Invalid argument
-[+] Start = 2^64-1 and len is set (should fail)
-fstrim: SCRATCH_MNT: FITRIM ioctl failed: Invalid argument
+[+] Optional trim range test (fs dependent)
 [+] Default length (should succeed)
 [+] Default length with start set (should succeed)
 [+] Length beyond the end of fs (should succeed)

Nikolay Borisov

2019-05-28 13:45 UTC #3 lore raw

Quoted 61 lines · Qu Wenruo

Quoted 58 lines · Nikolay Borisov
Quoted 55 lines · Qu Wenruo
If a filesystem doesn't map its logical address space (normally the bytenr/blocknr returned by fiemap) directly to its devices(s), the following assumptions used in the test case is no longer true:
- trim range start beyond the end of fs should fail
- trim range start beyond the end of fs with len set should fail
Under the following example, even with just one device, btrfs can still trim the fs correctly while breaking above assumption:
0 1G 1.25G
|---------------|///////////////|-----------------| <- btrfs logical
		   |				       address space
        ------------  mapped as SINGLE
        |
0 V 256M
|///////////////| <- device address space
Thus trim range start=1G len=256M will cause btrfs to trim the 256M block group, thus return correct result.
Furthermore, there is no definitely behavior for whether a fs should trim the unmapped space.
Btrfs currently will always trim the unmapped space, but the behavior can change as large trim can be very expensive.
Despite the change to skip certain tests for btrfs, still run the following tests for btrfs:
- trim start=U64_MAX with lenght set
This will expose a bug that btrfs doesn't check overflow of the range. This bug will be fixed soon.
- trim beyond the end of the fs
This will expose a bug where btrfs could send trim command beyond the end of its device.
This bug is a regression, can be fixed by reverting c2d1b3aae336 ("btrfs:
Honour FITRIM range constraints during free space trim")
With proper fixes for btrfs, this test case should pass on btrfs, ext4, xfs.
Signed-off-by Qu Wenruo <[email protected]>
---
 common/rc             | 11 +++++++
 tests/generic/260     | 75 ++++++++++++++++++++++++++++---------------
 tests/generic/260.out |  9 +-----
 3 files changed, 62 insertions(+), 33 deletions(-)
common/rc +0 -0
index 17b89d5d..d7a5898f 100644
@@ -4005,6 +4005,17 @@ _require_fibmap()
 	rm -f $file
 }
 

Quoted 8 lines

Quoted 4 lines
common/rc _require_fibmap() · line 4009
Quoted 2 lines
common/rc +2 -0
+# Check if the logical address (returned by fiemap) of a fs is 1:1 mapped to
+# its underlying fs
"underlying device" ?
What's the proper expression?
I mean the block device on which the fs lies above.

I got confused by the "of a fs is 1:1 mapped to its underlying fs" this seems to have tautology and I gathered you meant if the extents as returned by fiemap of a fs are 1:1 mapped to the underlying device that the fs sits on ?

<snip>

Quoted 8 lines

Quoted 7 lines
this check is rather ugly, instead if you return 0 on success (i.e it's directly mapped) it can be rewritten simply as:
if is_fs_direct_mapped; then
which is a lot cleaner than the $() fuckery

Quoted 8 lines

I tried that before reverting back to the echo one.
The biggest concern is, "return 0" is completely OK for regular functions which does some work.
But for bool return, especially for case like this _is_xxx function, return 0 for true is really confusing.
Or this is just the preferred way in bash?

This is the the unix convention - 0 indicates success,
http://tldp.org/LDP/Bash-Beginners-Guide/html/sect_07_01.html

And https://www.tldp.org/LDP/abs/html/exit-status.html re. exit statuses.

Quoted 41 lines

Quoted 31 lines

common/rc +17 -3

+	echo "[+] Start beyond the end of fs (should fail)" >> $seqres.full
+	$FSTRIM_PROG -o $beyond_eofs $SCRATCH_MNT >> $seqres.full 2>&1
+	[ $? -eq 0 ] && status=1
+
+	echo "[+] Start beyond the end of fs with len set (should fail)" >> $seqres.full
+	$FSTRIM_PROG -o $beyond_eofs -l1M $SCRATCH_MNT >> $seqres.full 2>&1
+	[ $? -eq 0 ] && status=1
+
+	# indirectly mapped fs may use this special value to trim their
+	# unmapped space, so don't do this for indirectly mapped fs.
+	echo "[+] Start = 2^64-1 (should fail)" >> $seqres.full
+	$FSTRIM_PROG -o $max_64bit $SCRATCH_MNT 2>&1 >> $seqres.full 2>&1
+	[ $? -eq 0 ] && status=1
+fi
 
-echo "[+] Start = 2^64-1 and len is set (should fail)"
-out=$($FSTRIM_PROG -o $max_64bit -l1M $SCRATCH_MNT 2>&1)
+# This should fail due to overflow no matter how the fs is implemented
+echo "[+] Start = 2^64-1 and len is set (should fail)" >> $seqres.full
+$FSTRIM_PROG -o $max_64bit -l1M $SCRATCH_MNT >> $seqres.full 2>&1
 [ $? -eq 0 ] && status=1
-echo $out | _filter_scratch
 
 _scratch_unmount
 _scratch_mkfs >/dev/null 2>&1

@@ -86,10 +97,12 @@ _scratch_unmount
 _scratch_mkfs >/dev/null 2>&1
 _scratch_mount

Quoted 9 lines

common/rc _scratch_unmount · line 105
Quoted 6 lines
common/rc +2 -0
+echo "[+] Trim an empty fs" >> $seqres.full
 # This is a bit fuzzy, but since the file system is fresh
 # there should be at least (fssize/2) free space to trim.
 # This is supposed to catch wrong FITRIM argument handling
 bytes=$($FSTRIM_PROG -v -o10M $SCRATCH_MNT | _filter_fstrim)
+echo "$bytes trimed" >> $seqres.full
Does it bring any value printing those strings in seqres.full given the output of the executed command won't be there. I think not.

Quoted 8 lines

When something went wrong, like no bytes get trimmed but still return 0, then this is definitely valuable.
BTW, there are cases btrfs trimmed just 0 bytes. And it's the same command. To me, this can't be more valuable.
In fact, I even considered to check $bytes to make the btrfs failure more explicit.

Fair enough but perhaps you can make

echo -n "Trim an empty fs" >> $seqres.full

exec commands

echo "$bytes trimmed". That way there is going to be a single line only. Anyway this is a nit so feel free to ignore.

Quoted 79 lines

Thanks,
Qu

Quoted 75 lines

common/rc fi · line 117

Quoted 10 lines

common/rc +1 -1

 if [ $bytes -gt $(_math "$fssize*1024") ]; then
 	status=1

@@ -101,7 +114,7 @@ fi
 # It is because btrfs does not have not-yet-used parts of the device
 # mapped and since we got here right after the mkfs, there is not
 # enough free extents in the root tree.
-if [ $bytes -le $(_math "$fssize*512") ] && [ $FSTYP != "btrfs" ]; then
+if [ $bytes -le $(_math "$fssize*512") ] && [ $(_is_fs_direct_mapped) -eq 1 ]; then

same thing here - make is_fs_direct_mapped plain 'return 0/1' and check its ret val directly.

Quoted 61 lines

common/rc +15 -3

 	status=1
 	echo "After the full fs discard $bytes bytes were discarded"\
 	     "however the file system is $(_math "$fssize*1024") bytes long."

@@ -141,14 +154,24 @@ esac
 _scratch_unmount
 _scratch_mkfs >/dev/null 2>&1
 _scratch_mount
+
+echo "[+] Try to trim beyond the end of the fs" >> $seqres.full
 # It should fail since $start is beyond the end of file system
-$FSTRIM_PROG -o$start -l10M $SCRATCH_MNT &> /dev/null
-if [ $? -eq 0 ]; then
+$FSTRIM_PROG -o$start -l10M $SCRATCH_MNT >> $seqres.full 2>&1
+ret=$?
+if [ $ret -eq 0 ] && [ $(_is_fs_direct_mapped) -eq 1 ]; then
 	status=1
 	echo "It seems that fs logic handling start"\
 	     "argument overflows"
 fi
 
+# For indirectly mapped fs, it shouldn't fail.
+# Btrfs will fail due to a bug in boundary check
+if [ $ret -ne 0 ] && [ $(_is_fs_direct_mapped) -eq 0 ]; then
+	status=1
+	echo "Unexpected error happened during trim"
+fi
+
 _scratch_unmount
 _scratch_mkfs >/dev/null 2>&1
 _scratch_mount

@@ -160,8 +183,10 @@ _scratch_mount
 # It is because btrfs does not have not-yet-used parts of the device
 # mapped and since we got here right after the mkfs, there is not
 # enough free extents in the root tree.
+echo "[+] Try to trim the fs with large enough len" >> $seqres.full
 bytes=$($FSTRIM_PROG -v -l$len $SCRATCH_MNT | _filter_fstrim)
-if [ $bytes -le $(_math "$fssize*512") ] && [ $FSTYP != "btrfs" ]; then
+echo "$bytes trimed" >> $seqres.full
+if [ $bytes -le $(_math "$fssize*512") ] && [ $(_is_fs_direct_mapped) == 1 ]; then
 	status=1
 	echo "It seems that fs logic handling len argument overflows"
 fi

tests/generic/260.out +1 -8

index a16c4f74..f4ee2f72 100644
@@ -1,12 +1,5 @@
 QA output created by 260
-[+] Start beyond the end of fs (should fail)
-fstrim: SCRATCH_MNT: FITRIM ioctl failed: Invalid argument
-[+] Start beyond the end of fs with len set (should fail)
-fstrim: SCRATCH_MNT: FITRIM ioctl failed: Invalid argument
-[+] Start = 2^64-1 (should fail)
-fstrim: SCRATCH_MNT: FITRIM ioctl failed: Invalid argument
-[+] Start = 2^64-1 and len is set (should fail)
-fstrim: SCRATCH_MNT: FITRIM ioctl failed: Invalid argument
+[+] Optional trim range test (fs dependent)
 [+] Default length (should succeed)
 [+] Default length with start set (should succeed)
 [+] Length beyond the end of fs (should succeed)

Qu Wenruo

2019-05-28 13:56 UTC #4 lore raw

Quoted 69 lines · Nikolay Borisov

Quoted 66 lines · Qu Wenruo
Quoted 62 lines · Nikolay Borisov
Quoted 52 lines · Qu Wenruo
If a filesystem doesn't map its logical address space (normally the bytenr/blocknr returned by fiemap) directly to its devices(s), the following assumptions used in the test case is no longer true:
- trim range start beyond the end of fs should fail
- trim range start beyond the end of fs with len set should fail
Under the following example, even with just one device, btrfs can still trim the fs correctly while breaking above assumption:
0 1G 1.25G
|---------------|///////////////|-----------------| <- btrfs logical
		   |				       address space
        ------------  mapped as SINGLE
        |
0 V 256M
|///////////////| <- device address space
Thus trim range start=1G len=256M will cause btrfs to trim the 256M block group, thus return correct result.
Furthermore, there is no definitely behavior for whether a fs should trim the unmapped space.
Btrfs currently will always trim the unmapped space, but the behavior can change as large trim can be very expensive.
Despite the change to skip certain tests for btrfs, still run the following tests for btrfs:
- trim start=U64_MAX with lenght set
This will expose a bug that btrfs doesn't check overflow of the range. This bug will be fixed soon.
- trim beyond the end of the fs
This will expose a bug where btrfs could send trim command beyond the end of its device.
This bug is a regression, can be fixed by reverting c2d1b3aae336 ("btrfs:
Honour FITRIM range constraints during free space trim")
With proper fixes for btrfs, this test case should pass on btrfs, ext4, xfs.
Signed-off-by Qu Wenruo <[email protected]>
---
 common/rc             | 11 +++++++
 tests/generic/260     | 75 ++++++++++++++++++++++++++++---------------
 tests/generic/260.out |  9 +-----
 3 files changed, 62 insertions(+), 33 deletions(-)
common/rc +0 -0
index 17b89d5d..d7a5898f 100644
common/rc _require_fibmap() · line 4009
Quoted 5 lines
common/rc +2 -0
@@ -4005,6 +4005,17 @@ _require_fibmap()
 	rm -f $file
 }
 
+# Check if the logical address (returned by fiemap) of a fs is 1:1 mapped to
+# its underlying fs
"underlying device" ?

Quoted 8 lines

Quoted 3 lines
What's the proper expression?
I mean the block device on which the fs lies above.
I got confused by the "of a fs is 1:1 mapped to its underlying fs" this seems to have tautology and I gathered you meant if the extents as returned by fiemap of a fs are 1:1 mapped to the underlying device that the fs sits on ?

Yes.

For directly mapped fs, if fiemap returns something like offset=1M, len=1M. Then we should be able to get the content (or at least the encrypted content) from the offset 1M from the device.

For indirectly mapped fs, the fiemap just returns something can't be directly used. (e.g. the value can even be beyond the end of any of the device).

I just don't have a good expression to show the above lines.

Quoted 76 lines

<snip>
Quoted 8 lines
Quoted 7 lines
this check is rather ugly, instead if you return 0 on success (i.e it's directly mapped) it can be rewritten simply as:
if is_fs_direct_mapped; then
which is a lot cleaner than the $() fuckery
Quoted 8 lines
I tried that before reverting back to the echo one.
The biggest concern is, "return 0" is completely OK for regular functions which does some work.
But for bool return, especially for case like this _is_xxx function, return 0 for true is really confusing.
Or this is just the preferred way in bash?
This is the the unix convention - 0 indicates success,
http://tldp.org/LDP/Bash-Beginners-Guide/html/sect_07_01.html
And https://www.tldp.org/LDP/abs/html/exit-status.html re. exit statuses.
Quoted 41 lines
Quoted 31 lines
Quoted 31 lines
common/rc +17 -3
+	echo "[+] Start beyond the end of fs (should fail)" >> $seqres.full
+	$FSTRIM_PROG -o $beyond_eofs $SCRATCH_MNT >> $seqres.full 2>&1
+	[ $? -eq 0 ] && status=1
+
+	echo "[+] Start beyond the end of fs with len set (should fail)" >> $seqres.full
+	$FSTRIM_PROG -o $beyond_eofs -l1M $SCRATCH_MNT >> $seqres.full 2>&1
+	[ $? -eq 0 ] && status=1
+
+	# indirectly mapped fs may use this special value to trim their
+	# unmapped space, so don't do this for indirectly mapped fs.
+	echo "[+] Start = 2^64-1 (should fail)" >> $seqres.full
+	$FSTRIM_PROG -o $max_64bit $SCRATCH_MNT 2>&1 >> $seqres.full 2>&1
+	[ $? -eq 0 ] && status=1
+fi
 
-echo "[+] Start = 2^64-1 and len is set (should fail)"
-out=$($FSTRIM_PROG -o $max_64bit -l1M $SCRATCH_MNT 2>&1)
+# This should fail due to overflow no matter how the fs is implemented
+echo "[+] Start = 2^64-1 and len is set (should fail)" >> $seqres.full
+$FSTRIM_PROG -o $max_64bit -l1M $SCRATCH_MNT >> $seqres.full 2>&1
 [ $? -eq 0 ] && status=1
-echo $out | _filter_scratch
 
 _scratch_unmount
 _scratch_mkfs >/dev/null 2>&1
@@ -86,10 +97,12 @@ _scratch_unmount
 _scratch_mkfs >/dev/null 2>&1
 _scratch_mount
 
Quoted 9 lines
common/rc _scratch_unmount · line 105
Quoted 6 lines
common/rc +2 -0
+echo "[+] Trim an empty fs" >> $seqres.full
 # This is a bit fuzzy, but since the file system is fresh
 # there should be at least (fssize/2) free space to trim.
 # This is supposed to catch wrong FITRIM argument handling
 bytes=$($FSTRIM_PROG -v -o10M $SCRATCH_MNT | _filter_fstrim)
+echo "$bytes trimed" >> $seqres.full
Does it bring any value printing those strings in seqres.full given the output of the executed command won't be there. I think not.
Quoted 8 lines
When something went wrong, like no bytes get trimmed but still return 0, then this is definitely valuable.
BTW, there are cases btrfs trimmed just 0 bytes. And it's the same command. To me, this can't be more valuable.
In fact, I even considered to check $bytes to make the btrfs failure more explicit.

Quoted 8 lines

Fair enough but perhaps you can make
echo -n "Trim an empty fs" >> $seqres.full
exec commands
echo "$bytes trimmed". That way there is going to be a single line only. Anyway this is a nit so feel free to ignore.

Isn't there already a line printing doing exactly as you mentioned?

Although the long long comment makes it less obvious.

tests/generic/260 _scratch_unmount · line 105

Quoted 6 lines

tests/generic/260 +2 -0

+echo "[+] Trim an empty fs" >> $seqres.full
 # This is a bit fuzzy, but since the file system is fresh
 # there should be at least (fssize/2) free space to trim.
 # This is supposed to catch wrong FITRIM argument handling
 bytes=$($FSTRIM_PROG -v -o10M $SCRATCH_MNT | _filter_fstrim)
+echo "$bytes trimed" >> $seqres.full

Thanks,
Qu

Quoted 79 lines

Thanks,
Qu

Quoted 75 lines

common/rc fi · line 117

Quoted 10 lines

common/rc +1 -1

 if [ $bytes -gt $(_math "$fssize*1024") ]; then
 	status=1

@@ -101,7 +114,7 @@ fi
 # It is because btrfs does not have not-yet-used parts of the device
 # mapped and since we got here right after the mkfs, there is not
 # enough free extents in the root tree.
-if [ $bytes -le $(_math "$fssize*512") ] && [ $FSTYP != "btrfs" ]; then
+if [ $bytes -le $(_math "$fssize*512") ] && [ $(_is_fs_direct_mapped) -eq 1 ]; then

same thing here - make is_fs_direct_mapped plain 'return 0/1' and check its ret val directly.

Quoted 61 lines

common/rc +15 -3

 	status=1
 	echo "After the full fs discard $bytes bytes were discarded"\
 	     "however the file system is $(_math "$fssize*1024") bytes long."

@@ -141,14 +154,24 @@ esac
 _scratch_unmount
 _scratch_mkfs >/dev/null 2>&1
 _scratch_mount
+
+echo "[+] Try to trim beyond the end of the fs" >> $seqres.full
 # It should fail since $start is beyond the end of file system
-$FSTRIM_PROG -o$start -l10M $SCRATCH_MNT &> /dev/null
-if [ $? -eq 0 ]; then
+$FSTRIM_PROG -o$start -l10M $SCRATCH_MNT >> $seqres.full 2>&1
+ret=$?
+if [ $ret -eq 0 ] && [ $(_is_fs_direct_mapped) -eq 1 ]; then
 	status=1
 	echo "It seems that fs logic handling start"\
 	     "argument overflows"
 fi
 
+# For indirectly mapped fs, it shouldn't fail.
+# Btrfs will fail due to a bug in boundary check
+if [ $ret -ne 0 ] && [ $(_is_fs_direct_mapped) -eq 0 ]; then
+	status=1
+	echo "Unexpected error happened during trim"
+fi
+
 _scratch_unmount
 _scratch_mkfs >/dev/null 2>&1
 _scratch_mount

@@ -160,8 +183,10 @@ _scratch_mount
 # It is because btrfs does not have not-yet-used parts of the device
 # mapped and since we got here right after the mkfs, there is not
 # enough free extents in the root tree.
+echo "[+] Try to trim the fs with large enough len" >> $seqres.full
 bytes=$($FSTRIM_PROG -v -l$len $SCRATCH_MNT | _filter_fstrim)
-if [ $bytes -le $(_math "$fssize*512") ] && [ $FSTYP != "btrfs" ]; then
+echo "$bytes trimed" >> $seqres.full
+if [ $bytes -le $(_math "$fssize*512") ] && [ $(_is_fs_direct_mapped) == 1 ]; then
 	status=1
 	echo "It seems that fs logic handling len argument overflows"
 fi

tests/generic/260.out +1 -8

index a16c4f74..f4ee2f72 100644
@@ -1,12 +1,5 @@
 QA output created by 260
-[+] Start beyond the end of fs (should fail)
-fstrim: SCRATCH_MNT: FITRIM ioctl failed: Invalid argument
-[+] Start beyond the end of fs with len set (should fail)
-fstrim: SCRATCH_MNT: FITRIM ioctl failed: Invalid argument
-[+] Start = 2^64-1 (should fail)
-fstrim: SCRATCH_MNT: FITRIM ioctl failed: Invalid argument
-[+] Start = 2^64-1 and len is set (should fail)
-fstrim: SCRATCH_MNT: FITRIM ioctl failed: Invalid argument
+[+] Optional trim range test (fs dependent)
 [+] Default length (should succeed)
 [+] Default length with start set (should succeed)
 [+] Length beyond the end of fs (should succeed)