Skip to contentSkip to conversation

common/rc: Modify _require_batched_discard to improve test coverage

PATCH In review Ojaswin Mujoo 2 participants 4 messages May 16, 2022 – Jun 20, 2022

lore
Share & tools
lore

Ojaswin Mujoo

author
#0 lore raw

PATCH v3 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.

This case was not correctly handled by this test since "_require_batched_discard" incorrectly interpreted the FITRIM failure as SCRATCH_DEV not supporting the IOCTL. This caused the test to 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

Signed-off-by Ojaswin Mujoo <[email protected]>
Reviewed-by Ritesh Harjani <[email protected]>

---

Changes since v2 [1]

* Eliminated redundant $ret variable

[1]
https://lore.kernel.org/all/[email protected]/

 common/rc | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)
common/rc +6 -1
index e2d3d72a..94344248 100644
@@ -3858,7 +3858,12 @@ _require_batched_discard()
 		exit 1
 	fi
 	_require_fstrim
-	$FSTRIM_PROG $1 > /dev/null 2>&1 || _notrun "FITRIM not supported on $1"
+
+	grep -q "not supported" <($FSTRIM_PROG $1 2>&1)
+	if [ "$?" = "0" ]
+	then
+		_notrun "FITRIM not supported on $1"
+	fi
 }
 
 _require_dumpe2fs()
Signature · 1 line
2.27.0
author
#1 lore raw

Greetings,

Please let me know if there are any reviews/suggestions on this patch.

Thank you!

Quoted 26 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.

This case was not correctly handled by this test since "_require_batched_discard" incorrectly interpreted the FITRIM failure as SCRATCH_DEV not supporting the IOCTL. This caused the test to 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

Signed-off-by Ojaswin Mujoo <[email protected]>
Reviewed-by Ritesh Harjani <[email protected]>

---

Changes since v2 [1]

* Eliminated redundant $ret variable

[1]
https://lore.kernel.org/all/[email protected]/

#2 lore raw
Quoted 4 lines · Ojaswin Mujoo

Greetings,

Please let me know if there are any reviews/suggestions on this patch.

This patch has been merged in fstests v2022.05.22, as below

ee264b3f common/rc: Modify _require_batched_discard to improve test coverage

Is there anything wrong with that?

Thanks,
Zorro

Quoted 31 lines

Thank you!

Quoted 26 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.

This case was not correctly handled by this test since "_require_batched_discard" incorrectly interpreted the FITRIM failure as SCRATCH_DEV not supporting the IOCTL. This caused the test to 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

Signed-off-by Ojaswin Mujoo <[email protected]>
Reviewed-by Ritesh Harjani <[email protected]>

---

Changes since v2 [1]

* Eliminated redundant $ret variable

[1]
https://lore.kernel.org/all/[email protected]/

author
#3 lore raw
Quoted 6 lines · Zorro Lang
Quoted 4 lines · Ojaswin Mujoo

Greetings,

Please let me know if there are any reviews/suggestions on this patch.

Quoted 9 lines

This patch has been merged in fstests v2022.05.22, as below

ee264b3f common/rc: Modify _require_batched_discard to improve test coverage

Is there anything wrong with that?

Thanks,
Zorro

Ahh I wasn't aware of that, thanks for picking this up :)

Regards,
Ojaswin