common/rc: Modify _require_batched_discard to improve test coverage
PATCH 1/1 common/rc: Modify _require_batched_discard to improve test coverage
A recent ext4 patch discussed [1] that some devices (eg LVMs) can have a discard granularity as big as 42MB which makes it larger than the group size of ext4 FS with 1k BS. This causes the FITRIM IOCTL to fail on filesystems like ext4.
This case was not correctly handle by "_require_batched_discard" as it incorrectly interpreted the FITRIM failure as fs not supporting the IOCTL. This caused the tests like generic/260 to incorectly report "not run" instead of "failed" in case of large discard granularity.
Fix "_require_batched_discard" to use a more accurate method to determine if discard is supported.
[1] commit 173b6e383d2
ext4: avoid trim error on fs with small groups
--- common/rc | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)
common/rc +7 -1
@@ -3858,7 +3858,13 @@ _require_batched_discard() exit 1 fi _require_fstrim - $FSTRIM_PROG $1 > /dev/null 2>&1 || _notrun "FITRIM not supported on $1" + + $FSTRIM_PROG $1 2>&1 | grep -q "not supported" + RET=$? + if [ "$RET" = "0" ] + then + _notrun "FITRIM not supported on $1" + fi } _require_dumpe2fs()
Signature · 1 line
2.27.0
Quoted 10 lines · Ojaswin Mujoo
A recent ext4 patch discussed [1] that some devices (eg LVMs) can have a discard granularity as big as 42MB which makes it larger than the group size of ext4 FS with 1k BS. This causes the FITRIM IOCTL to fail on filesystems like ext4.
This case was not correctly handle by "
_require_batched_discard" as it incorrectly interpreted the FITRIM failure as fs not supporting the IOCTL. This caused the tests like generic/260 to incorectly report "not run" instead of "failed" in case of large discard granularity.
Ok, I looked at fstrim code and it does print [1] "the discard operation is not supported" in case of rc == 1. And if rc != 0 it will always returns EXIT_FAILURE.
So this patch looks good to me. Feel free to add:
Although it will be good to check if we can add a generic test case using maybe lvm or dm device, where this device could report large discard_granularity for actually excercising this code path (rather then changing kernel code to test it).
-ritesh
[1]: https://github.com/util-linux/util-linux/blob/master/sys-utils/fstrim.c
Quoted 12 lines
Fix "
_require_batched_discard" to use a more accurate method to determine if discard is supported.[1] commit
173b6e383d2
ext4: avoid trim error on fs with small groupsSigned-off-by Ojaswin Mujoo <[email protected]>--- common/rc | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)
Quoted 12 lines · Ritesh Harjani
Quoted 10 lines · Ojaswin Mujoo
A recent ext4 patch discussed [1] that some devices (eg LVMs) can have a discard granularity as big as 42MB which makes it larger than the group size of ext4 FS with 1k BS. This causes the FITRIM IOCTL to fail on filesystems like ext4.
This case was not correctly handle by "
_require_batched_discard" as it incorrectly interpreted the FITRIM failure as fs not supporting the IOCTL. This caused the tests like generic/260 to incorectly report "not run" instead of "failed" in case of large discard granularity.
Quoted 7 lines
Ok, I looked at fstrim code and it does print [1] "the discard operation is not supported" in case of rc == 1. And if rc != 0 it will always returns
EXIT_FAILURE.So this patch looks good to me. Feel free to add:
Reviewed-by Ritesh Harjani <[email protected]>
Thank you for the review Ritesh.
Quoted 6 lines
Although it will be good to check if we can add a generic test case using maybe lvm or dm device, where this device could report large
discard_granularityfor actually excercising this code path (rather then changing kernel code to test it).
You are correct, as I was not able to simulate a device with disc gran of 40MB+ I tested this by hard coding the granularity in the kernel.
That being said, I would appreciate if anyone has any insights on using LVM/DM to get that high a granularity so I can test it more accurately.
Regards,
Ojaswin
Quoted 17 lines
-ritesh
[1]: https://github.com/util-linux/util-linux/blob/master/sys-utils/fstrim.c
Quoted 12 lines
Fix "
_require_batched_discard" to use a more accurate method to determine if discard is supported.[1] commit
173b6e383d2
ext4: avoid trim error on fs with small groupsSigned-off-by Ojaswin Mujoo <[email protected]>--- common/rc | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)
Greetings,
Please do let me know if there are any reviews or comments on this patch.
Thank you,
Ojaswin
Quoted 44 lines · Ojaswin Mujoo
A recent ext4 patch discussed [1] that some devices (eg LVMs) can have a discard granularity as big as 42MB which makes it larger than the group size of ext4 FS with 1k BS. This causes the FITRIM IOCTL to fail on filesystems like ext4.
This case was not correctly handle by "
_require_batched_discard" as it incorrectly interpreted the FITRIM failure as fs not supporting the IOCTL. This caused the tests like generic/260 to incorectly report "not run" instead of "failed" in case of large discard granularity.Fix "
_require_batched_discard" to use a more accurate method to determine if discard is supported.[1] commit
173b6e383d2
ext4: avoid trim error on fs with small groupsSigned-off-by Ojaswin Mujoo <[email protected]>--- common/rc | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)common/rc +7
-1@@ -3858,7 +3858,13 @@ _require_batched_discard() exit 1 fi _require_fstrim - $FSTRIM_PROG $1 > /dev/null 2>&1 || _notrun "FITRIM not supported on $1" + + $FSTRIM_PROG $1 2>&1 | grep -q "not supported" + RET=$? + if [ "$RET" = "0" ] + then + _notrun "FITRIM not supported on $1" + fi } _require_dumpe2fs()Signature · 1 line
2.27.0
Quoted 27 lines · Ojaswin Mujoo
A recent ext4 patch discussed [1] that some devices (eg LVMs) can have a discard granularity as big as 42MB which makes it larger than the group size of ext4 FS with 1k BS. This causes the FITRIM IOCTL to fail on filesystems like ext4.
This case was not correctly handle by "
_require_batched_discard" as it incorrectly interpreted the FITRIM failure as fs not supporting the IOCTL. This caused the tests like generic/260 to incorectly report "not run" instead of "failed" in case of large discard granularity.Fix "
_require_batched_discard" to use a more accurate method to determine if discard is supported.[1] commit
173b6e383d2
ext4: avoid trim error on fs with small groupsSigned-off-by Ojaswin Mujoo <[email protected]>--- common/rc | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)common/rc +0
-0
common/rc _require_batched_discard() · line 3863
Quoted 7 lines
common/rc +3
-1@@ -3858,7 +3858,13 @@ _require_batched_discard() exit 1 fi _require_fstrim - $FSTRIM_PROG $1 > /dev/null 2>&1 || _notrun "FITRIM not supported on $1" + + $FSTRIM_PROG $1 2>&1 | grep -q "not supported" + RET=$?
Better to use global variable carefully in common functions, if it's not necessary, I'd recommend using "local ret" at here.
From my experience, the *quiet (-q)* grep does "exit_on_match" directly, it won't wait the write process, if the write process is still writing but the grep has exited, then it'll cause broken pipe, and the write process exit with failure.
It doesn't always happend, it depends. So I'd like to use "${PIPESTATUS[1]}" or write it as 'grep -q "not supported" <($FSTRIM_PROG $1 2>&1)', to make sure we just care about the "grep" result.
Quoted 10 lines
common/rc +4
-0+ if [ "$RET" = "0" ] + then + _notrun "FITRIM not supported on $1" + fi } _require_dumpe2fs()Signature · 1 line
2.27.0
Hi Zorro,
Thanks for the review.
Quoted 23 lines · Zorro Lang
Quoted 22 lines · Ojaswin Mujoo
A recent ext4 patch discussed [1] that some devices (eg LVMs) can have a discard granularity as big as 42MB which makes it larger than the group size of ext4 FS with 1k BS. This causes the FITRIM IOCTL to fail on filesystems like ext4.
This case was not correctly handle by "
_require_batched_discard" as it incorrectly interpreted the FITRIM failure as fs not supporting the IOCTL. This caused the tests like generic/260 to incorectly report "not run" instead of "failed" in case of large discard granularity.Fix "
_require_batched_discard" to use a more accurate method to determine if discard is supported.[1] commit
173b6e383d2
ext4: avoid trim error on fs with small groupsSigned-off-by Ojaswin Mujoo <[email protected]>--- common/rc | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)
Quoted 15 lines
common/rc _require_batched_discard() · line 3863
Quoted 12 lines
common/rc +3
-1@@ -3858,7 +3858,13 @@ _require_batched_discard() exit 1 fi _require_fstrim - $FSTRIM_PROG $1 > /dev/null 2>&1 || _notrun "FITRIM not supported on $1" + + $FSTRIM_PROG $1 2>&1 | grep -q "not supported" + RET=$?Better to use global variable carefully in common functions, if it's not necessary, I'd recommend using "local ret" at here.
Sure, I'll make the change.
Quoted 9 lines
From my experience, the *quiet (-q)* grep does "
exit_on_match" directly, it won't wait the write process, if the write process is still writing but the grep has exited, then it'll cause broken pipe, and the write process exit with failure.It doesn't always happend, it depends. So I'd like to use "${PIPESTATUS[1]}" or write it as 'grep -q "not supported" <($FSTRIM_PROG $1 2>&1)', to make sure we just care about the "grep" result.
Ah makes sense, will make this change as well.
Regards,
Ojaswin
Quoted 12 lines
Quoted 11 lines
common/rc +4
-0+ if [ "$RET" = "0" ] + then + _notrun "FITRIM not supported on $1" + fi } _require_dumpe2fs()Signature · 1 line
2.27.0