[PATCH v2] common/rc: Modify _require_batched_discard to improve test coverage

[PATCH v2] common/rc: Modify _require_batched_discard to improve test coverage

[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 <ojaswin@linux.ibm.com>
Reviewed-by: Ritesh Harjani <riteshh@linux.ibm.com>
---

Changes since v1 [1] 

*  Changed $RET to a local variable
*  Fixed the grep command 

[1]
https://lore.kernel.org/all/20220401055713.634842-1-ojaswin@linux.ibm.com/

 common/rc | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/common/rc b/common/rc
index e2d3d72a..f366e409 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"
+
+	grep -q "not supported" <($FSTRIM_PROG $1 2>&1)
+	local ret=$?
+	if [ "$ret" = "0" ]
+	then
+		_notrun "FITRIM not supported on $1"
+	fi
 }
 
 _require_dumpe2fs()
-- 
2.27.0

Re: [PATCH v2] common/rc: Modify _require_batched_discard to improve test coverage

[Zorro Lang]

On Mon, May 16, 2022 at 12:09:51PM +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.
> 
> 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 <ojaswin@linux.ibm.com>
> Reviewed-by: Ritesh Harjani <riteshh@linux.ibm.com>
> ---
> 
> Changes since v1 [1] 
> 
> *  Changed $RET to a local variable
> *  Fixed the grep command 
> 
> [1]
> https://lore.kernel.org/all/20220401055713.634842-1-ojaswin@linux.ibm.com/
> 
>  common/rc | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/common/rc b/common/rc
> index e2d3d72a..f366e409 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"
> +
> +	grep -q "not supported" <($FSTRIM_PROG $1 2>&1)
> +	local ret=$?
> +	if [ "$ret" = "0" ]

Oh I forgot to ask why we need to add a variable (ret), to record the return
value at here. Why can't use "$?" directly? e.g.

  grep -q "not supported" <($FSTRIM_PROG $1 2>&1)
  if [ $? -eq 0 ]

Others look good to me.

Thanks,
Zorro

> +	then
> +		_notrun "FITRIM not supported on $1"
> +	fi
>  }
>  
>  _require_dumpe2fs()
> -- 
> 2.27.0
> 

Re: [PATCH v2] common/rc: Modify _require_batched_discard to improve test coverage

[Ojaswin Mujoo]

> 
> Oh I forgot to ask why we need to add a variable (ret), to record the return
> value at here. Why can't use "$?" directly? e.g.
> 
>   grep -q "not supported" <($FSTRIM_PROG $1 2>&1)
>   if [ $? -eq 0 ]
> 
> Others look good to me.
> 
> Thanks,
> Zorro

That's right, I did it out of habbit. Anyways, I have eliminated the
variable and sent in a new version here [1]. Let me know if it looks
good.

Thank you!
Ojaswin

[1]
https://lore.kernel.org/all/20220516084505.97655-1-ojaswin@linux.ibm.com/
> 
> > +	then
> > +		_notrun "FITRIM not supported on $1"
> > +	fi
> >  }
> >  
> >  _require_dumpe2fs()
> > -- 
> > 2.27.0
> > 
>