* [PATCH] common/rc: Modify _require_batched_discard to improve test coverage
@ 2022-04-01 5:57 Ojaswin Mujoo
2022-04-01 9:54 ` Ritesh Harjani
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Ojaswin Mujoo @ 2022-04-01 5:57 UTC (permalink / raw)
To: fstests; +Cc: riteshh, linux-ext4, linux-kernel
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
Signed-off-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>
---
common/rc | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/common/rc b/common/rc
index e2d3d72a..97386342 100644
--- a/common/rc
+++ b/common/rc
@@ -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()
--
2.27.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] common/rc: Modify _require_batched_discard to improve test coverage
2022-04-01 5:57 [PATCH] common/rc: Modify _require_batched_discard to improve test coverage Ojaswin Mujoo
@ 2022-04-01 9:54 ` Ritesh Harjani
2022-04-03 8:19 ` Ojaswin Mujoo
2022-05-09 6:22 ` Ojaswin Mujoo
2022-05-10 6:32 ` Zorro Lang
2 siblings, 1 reply; 6+ messages in thread
From: Ritesh Harjani @ 2022-04-01 9:54 UTC (permalink / raw)
To: Ojaswin Mujoo; +Cc: fstests, riteshh, linux-ext4, linux-kernel
On 22/04/01 11:27AM, Ojaswin Mujoo wrote:
> 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:
Reviewed-by: Ritesh Harjani <riteshh@linux.ibm.com>
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
>
> 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 <ojaswin@linux.ibm.com>
> ---
> common/rc | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] common/rc: Modify _require_batched_discard to improve test coverage
2022-04-01 9:54 ` Ritesh Harjani
@ 2022-04-03 8:19 ` Ojaswin Mujoo
0 siblings, 0 replies; 6+ messages in thread
From: Ojaswin Mujoo @ 2022-04-03 8:19 UTC (permalink / raw)
To: Ritesh Harjani; +Cc: fstests, riteshh, linux-ext4, linux-kernel
On Fri, Apr 01, 2022 at 03:24:36PM +0530, Ritesh Harjani wrote:
> On 22/04/01 11:27AM, Ojaswin Mujoo wrote:
> > 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:
>
> Reviewed-by: Ritesh Harjani <riteshh@linux.ibm.com>
Thank you for the review Ritesh.
>
>
> 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).
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
>
> -ritesh
>
> [1]: https://github.com/util-linux/util-linux/blob/master/sys-utils/fstrim.c
>
> >
> > 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 <ojaswin@linux.ibm.com>
> > ---
> > common/rc | 8 +++++++-
> > 1 file changed, 7 insertions(+), 1 deletion(-)
> >
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] common/rc: Modify _require_batched_discard to improve test coverage
2022-04-01 5:57 [PATCH] common/rc: Modify _require_batched_discard to improve test coverage Ojaswin Mujoo
2022-04-01 9:54 ` Ritesh Harjani
@ 2022-05-09 6:22 ` Ojaswin Mujoo
2022-05-10 6:32 ` Zorro Lang
2 siblings, 0 replies; 6+ messages in thread
From: Ojaswin Mujoo @ 2022-05-09 6:22 UTC (permalink / raw)
To: fstests; +Cc: riteshh, linux-ext4, linux-kernel
Greetings,
Please do let me know if there are any reviews or comments on this patch.
Thank you,
Ojaswin
On Fri, Apr 01, 2022 at 11:27:13AM +0530, Ojaswin Mujoo wrote:
> 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
>
> Signed-off-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>
> ---
> common/rc | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/common/rc b/common/rc
> index e2d3d72a..97386342 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -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()
> --
> 2.27.0
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] common/rc: Modify _require_batched_discard to improve test coverage
2022-04-01 5:57 [PATCH] common/rc: Modify _require_batched_discard to improve test coverage Ojaswin Mujoo
2022-04-01 9:54 ` Ritesh Harjani
2022-05-09 6:22 ` Ojaswin Mujoo
@ 2022-05-10 6:32 ` Zorro Lang
2022-05-10 11:49 ` Ojaswin Mujoo
2 siblings, 1 reply; 6+ messages in thread
From: Zorro Lang @ 2022-05-10 6:32 UTC (permalink / raw)
To: Ojaswin Mujoo; +Cc: fstests, riteshh, linux-ext4, linux-kernel
On Fri, Apr 01, 2022 at 11:27:13AM +0530, Ojaswin Mujoo wrote:
> 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
>
> Signed-off-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>
> ---
> common/rc | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/common/rc b/common/rc
> index e2d3d72a..97386342 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -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.
> + if [ "$RET" = "0" ]
> + then
> + _notrun "FITRIM not supported on $1"
> + fi
> }
>
> _require_dumpe2fs()
> --
> 2.27.0
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] common/rc: Modify _require_batched_discard to improve test coverage
2022-05-10 6:32 ` Zorro Lang
@ 2022-05-10 11:49 ` Ojaswin Mujoo
0 siblings, 0 replies; 6+ messages in thread
From: Ojaswin Mujoo @ 2022-05-10 11:49 UTC (permalink / raw)
To: fstests, riteshh, linux-ext4, linux-kernel
Hi Zorro,
Thanks for the review.
On Tue, May 10, 2022 at 02:32:23PM +0800, Zorro Lang wrote:
> On Fri, Apr 01, 2022 at 11:27:13AM +0530, Ojaswin Mujoo wrote:
> > 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
> >
> > Signed-off-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>
> > ---
> > common/rc | 8 +++++++-
> > 1 file changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/common/rc b/common/rc
> > index e2d3d72a..97386342 100644
> > --- a/common/rc
> > +++ b/common/rc
> > @@ -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.
>
> 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
>
> > + if [ "$RET" = "0" ]
> > + then
> > + _notrun "FITRIM not supported on $1"
> > + fi
> > }
> >
> > _require_dumpe2fs()
> > --
> > 2.27.0
> >
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-05-10 11:49 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-01 5:57 [PATCH] common/rc: Modify _require_batched_discard to improve test coverage Ojaswin Mujoo
2022-04-01 9:54 ` Ritesh Harjani
2022-04-03 8:19 ` Ojaswin Mujoo
2022-05-09 6:22 ` Ojaswin Mujoo
2022-05-10 6:32 ` Zorro Lang
2022-05-10 11:49 ` Ojaswin Mujoo
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.