[1/3] common/rc: test that xfs_io's falloc command supports specific flags

Commit Message

Filipe Manana April 4, 2017, 6:34 a.m. UTC

From: Filipe Manana <[e-mail]>

For example NFS 4.2 supports fallocate but it does not support its
KEEP_SIZE flag, so we want to skip tests that use fallocate with that
flag on filesystems that don't support it.

Suggested-by: Eryu Guan <[e-mail]>
Signed-off-by: Filipe Manana <[e-mail]>
---
 common/rc | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Eryu Guan April 6, 2017, 2:18 p.m. UTC | #1

On Tue, Apr 04, 2017 at 07:34:29AM +0100, [e-mail] wrote:
> From: Filipe Manana <[e-mail]>
> 
> For example NFS 4.2 supports fallocate but it does not support its
> KEEP_SIZE flag, so we want to skip tests that use fallocate with that
> flag on filesystems that don't support it.
> 
> Suggested-by: Eryu Guan <[e-mail]>
> Signed-off-by: Filipe Manana <[e-mail]>
> ---
>  common/rc | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/common/rc b/common/rc
> index e1ab2c6..3d0f089 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -2021,8 +2021,8 @@ _require_xfs_io_command()
>  	"chproj")
>  		testio=`$XFS_IO_PROG -F -f -c "chproj 0" $testfile 2>&1`
>  		;;
> -	"falloc" )
> -		testio=`$XFS_IO_PROG -F -f -c "falloc 0 1m" $testfile 2>&1`
> +	"falloc*" )

This doesn't work as expected with quotes. I can remove the quotes at
commit time though.

Thanks,
Eryu

> +		testio=`$XFS_IO_PROG -F -f -c "$command 0 1m" $testfile 2>&1`
>  		;;
>  	"fpunch" | "fcollapse" | "zero" | "fzero" | "finsert" | "funshare")
>  		testio=`$XFS_IO_PROG -F -f -c "pwrite 0 20k" -c "fsync" \
> -- 
> 2.7.0.rc3
> 
> --
> To unsubscribe from this list: send the line "unsubscribe fstests" in
> the body of a message to [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Filipe Manana April 6, 2017, 2:20 p.m. UTC | #2

On Thu, Apr 6, 2017 at 3:18 PM, Eryu Guan <[e-mail]> wrote:
> On Tue, Apr 04, 2017 at 07:34:29AM +0100, [e-mail] wrote:
>> From: Filipe Manana <[e-mail]>
>>
>> For example NFS 4.2 supports fallocate but it does not support its
>> KEEP_SIZE flag, so we want to skip tests that use fallocate with that
>> flag on filesystems that don't support it.
>>
>> Suggested-by: Eryu Guan <[e-mail]>
>> Signed-off-by: Filipe Manana <[e-mail]>
>> ---
>>  common/rc | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/common/rc b/common/rc
>> index e1ab2c6..3d0f089 100644
>> --- a/common/rc
>> +++ b/common/rc
>> @@ -2021,8 +2021,8 @@ _require_xfs_io_command()
>>       "chproj")
>>               testio=`$XFS_IO_PROG -F -f -c "chproj 0" $testfile 2>&1`
>>               ;;
>> -     "falloc" )
>> -             testio=`$XFS_IO_PROG -F -f -c "falloc 0 1m" $testfile 2>&1`
>> +     "falloc*" )
>
> This doesn't work as expected with quotes. I can remove the quotes at
> commit time though.

Hum, it did work for me, strange.

But please do, thanks.

>
> Thanks,
> Eryu
>
>> +             testio=`$XFS_IO_PROG -F -f -c "$command 0 1m" $testfile 2>&1`
>>               ;;
>>       "fpunch" | "fcollapse" | "zero" | "fzero" | "finsert" | "funshare")
>>               testio=`$XFS_IO_PROG -F -f -c "pwrite 0 20k" -c "fsync" \
>> --
>> 2.7.0.rc3
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe fstests" in
>> the body of a message to [e-mail]
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

David Sterba April 6, 2017, 2:28 p.m. UTC | #3

On Thu, Apr 06, 2017 at 03:20:43PM +0100, Filipe Manana wrote:
> On Thu, Apr 6, 2017 at 3:18 PM, Eryu Guan <[e-mail]> wrote:
> > On Tue, Apr 04, 2017 at 07:34:29AM +0100, [e-mail] wrote:
> >> From: Filipe Manana <[e-mail]>
> >>
> >> For example NFS 4.2 supports fallocate but it does not support its
> >> KEEP_SIZE flag, so we want to skip tests that use fallocate with that
> >> flag on filesystems that don't support it.
> >>
> >> Suggested-by: Eryu Guan <[e-mail]>
> >> Signed-off-by: Filipe Manana <[e-mail]>
> >> ---
> >>  common/rc | 4 ++--
> >>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/common/rc b/common/rc
> >> index e1ab2c6..3d0f089 100644
> >> --- a/common/rc
> >> +++ b/common/rc
> >> @@ -2021,8 +2021,8 @@ _require_xfs_io_command()
> >>       "chproj")
> >>               testio=`$XFS_IO_PROG -F -f -c "chproj 0" $testfile 2>&1`
> >>               ;;
> >> -     "falloc" )
> >> -             testio=`$XFS_IO_PROG -F -f -c "falloc 0 1m" $testfile 2>&1`
> >> +     "falloc*" )
> >
> > This doesn't work as expected with quotes. I can remove the quotes at
> > commit time though.
> 
> Hum, it did work for me, strange.

Quoted globs don't work the same way as unquoted inside case, this

#!/bin/sh
i=abc
case $i in
	abc*) echo notquoted;;
	"abc*") echo quoted;;
esac
---

will return 'notquoted'
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Eryu Guan April 7, 2017, 8:51 a.m. UTC | #4

On Tue, Apr 04, 2017 at 07:34:29AM +0100, [e-mail] wrote:
> From: Filipe Manana <[e-mail]>
> 
> For example NFS 4.2 supports fallocate but it does not support its
> KEEP_SIZE flag, so we want to skip tests that use fallocate with that
> flag on filesystems that don't support it.
> 
> Suggested-by: Eryu Guan <[e-mail]>
> Signed-off-by: Filipe Manana <[e-mail]>
> ---
>  common/rc | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/common/rc b/common/rc
> index e1ab2c6..3d0f089 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -2021,8 +2021,8 @@ _require_xfs_io_command()
>  	"chproj")
>  		testio=`$XFS_IO_PROG -F -f -c "chproj 0" $testfile 2>&1`
>  		;;
> -	"falloc" )
> -		testio=`$XFS_IO_PROG -F -f -c "falloc 0 1m" $testfile 2>&1`
> +	"falloc*" )
> +		testio=`$XFS_IO_PROG -F -f -c "$command 0 1m" $testfile 2>&1`

Sorry, I was wrong about this. It would break the subsequent
$XFS_IO_PROG -c "help $command" | grep ... command if another $param is
specified. Seems adding $param to falloc command is the right way, as
what Darrick did to fiemap in his new test.

-               testio=`$XFS_IO_PROG -F -f -c "falloc 0 1m" $testfile 2>&1`
+               testio=`$XFS_IO_PROG -F -f -c "falloc $param 0 1m" $testfile 2>&1`

Do you mind me updating these three patches accordingly? Or can you send
out new version if you like?

Thanks! And sorry again!

Eryu

P.S. I'm thinking of converting all the case switches (except the
default one) in _require_xfs_io_command() to actually run the $command
with $param, and doing other cleanups, but that won't block this patch
and I can do it in another patch.

>  		;;
>  	"fpunch" | "fcollapse" | "zero" | "fzero" | "finsert" | "funshare")
>  		testio=`$XFS_IO_PROG -F -f -c "pwrite 0 20k" -c "fsync" \
> -- 
> 2.7.0.rc3
> 
> --
> To unsubscribe from this list: send the line "unsubscribe fstests" in
> the body of a message to [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Filipe Manana April 7, 2017, 9:13 a.m. UTC | #5

On Fri, Apr 7, 2017 at 9:51 AM, Eryu Guan <[e-mail]> wrote:
> On Tue, Apr 04, 2017 at 07:34:29AM +0100, [e-mail] wrote:
>> From: Filipe Manana <[e-mail]>
>>
>> For example NFS 4.2 supports fallocate but it does not support its
>> KEEP_SIZE flag, so we want to skip tests that use fallocate with that
>> flag on filesystems that don't support it.
>>
>> Suggested-by: Eryu Guan <[e-mail]>
>> Signed-off-by: Filipe Manana <[e-mail]>
>> ---
>>  common/rc | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/common/rc b/common/rc
>> index e1ab2c6..3d0f089 100644
>> --- a/common/rc
>> +++ b/common/rc
>> @@ -2021,8 +2021,8 @@ _require_xfs_io_command()
>>       "chproj")
>>               testio=`$XFS_IO_PROG -F -f -c "chproj 0" $testfile 2>&1`
>>               ;;
>> -     "falloc" )
>> -             testio=`$XFS_IO_PROG -F -f -c "falloc 0 1m" $testfile 2>&1`
>> +     "falloc*" )
>> +             testio=`$XFS_IO_PROG -F -f -c "$command 0 1m" $testfile 2>&1`
>
> Sorry, I was wrong about this. It would break the subsequent
> $XFS_IO_PROG -c "help $command" | grep ... command if another $param is
> specified.

Yeah I had noticed that because the following won't cause the return anymore:

test -z "$param" && return

> Seems adding $param to falloc command is the right way, as
> what Darrick did to fiemap in his new test.
>
> -               testio=`$XFS_IO_PROG -F -f -c "falloc 0 1m" $testfile 2>&1`
> +               testio=`$XFS_IO_PROG -F -f -c "falloc $param 0 1m" $testfile 2>&1`

But in that case grepping the help output, at the very end of the
function, will fail for falloc since its help output fails to match
the grep pattern (as highlighted in the thread you pointed before).
So that grep pattern would have to change as well.

>
> Do you mind me updating these three patches accordingly? Or can you send
> out new version if you like?

Sure, fell free to update it as you feel it's the best way.

Thanks!

>
> Thanks! And sorry again!
>
> Eryu
>
> P.S. I'm thinking of converting all the case switches (except the
> default one) in _require_xfs_io_command() to actually run the $command
> with $param, and doing other cleanups, but that won't block this patch
> and I can do it in another patch.
>
>>               ;;
>>       "fpunch" | "fcollapse" | "zero" | "fzero" | "finsert" | "funshare")
>>               testio=`$XFS_IO_PROG -F -f -c "pwrite 0 20k" -c "fsync" \
>> --
>> 2.7.0.rc3
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe fstests" in
>> the body of a message to [e-mail]
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Eryu Guan April 7, 2017, 9:45 a.m. UTC | #6

On Fri, Apr 07, 2017 at 10:13:09AM +0100, Filipe Manana wrote:
> On Fri, Apr 7, 2017 at 9:51 AM, Eryu Guan <[e-mail]> wrote:
> > On Tue, Apr 04, 2017 at 07:34:29AM +0100, [e-mail] wrote:
> >> From: Filipe Manana <[e-mail]>
> >>
> >> For example NFS 4.2 supports fallocate but it does not support its
> >> KEEP_SIZE flag, so we want to skip tests that use fallocate with that
> >> flag on filesystems that don't support it.
> >>
> >> Suggested-by: Eryu Guan <[e-mail]>
> >> Signed-off-by: Filipe Manana <[e-mail]>
> >> ---
> >>  common/rc | 4 ++--
> >>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/common/rc b/common/rc
> >> index e1ab2c6..3d0f089 100644
> >> --- a/common/rc
> >> +++ b/common/rc
> >> @@ -2021,8 +2021,8 @@ _require_xfs_io_command()
> >>       "chproj")
> >>               testio=`$XFS_IO_PROG -F -f -c "chproj 0" $testfile 2>&1`
> >>               ;;
> >> -     "falloc" )
> >> -             testio=`$XFS_IO_PROG -F -f -c "falloc 0 1m" $testfile 2>&1`
> >> +     "falloc*" )
> >> +             testio=`$XFS_IO_PROG -F -f -c "$command 0 1m" $testfile 2>&1`
> >
> > Sorry, I was wrong about this. It would break the subsequent
> > $XFS_IO_PROG -c "help $command" | grep ... command if another $param is
> > specified.
> 
> Yeah I had noticed that because the following won't cause the return anymore:
> 
> test -z "$param" && return
> 
> > Seems adding $param to falloc command is the right way, as
> > what Darrick did to fiemap in his new test.
> >
> > -               testio=`$XFS_IO_PROG -F -f -c "falloc 0 1m" $testfile 2>&1`
> > +               testio=`$XFS_IO_PROG -F -f -c "falloc $param 0 1m" $testfile 2>&1`
> 
> But in that case grepping the help output, at the very end of the
> function, will fail for falloc since its help output fails to match
> the grep pattern (as highlighted in the thread you pointed before).
> So that grep pattern would have to change as well.

This should be fixed by xfsprogs commit 0c2ed80a3590 ("xfs_io: provide
long-format help for falloc"). But I agreed searching the help message
again for it would be redundant. But perhaps that needs wider review and
doesn't belong to this patch. I'll try to do that in my cleanup patch
for _require_xfs_io_command().

Thanks,
Eryu

> 
> >
> > Do you mind me updating these three patches accordingly? Or can you send
> > out new version if you like?
> 
> Sure, fell free to update it as you feel it's the best way.
> 
> Thanks!
> 
> >
> > Thanks! And sorry again!
> >
> > Eryu
> >
> > P.S. I'm thinking of converting all the case switches (except the
> > default one) in _require_xfs_io_command() to actually run the $command
> > with $param, and doing other cleanups, but that won't block this patch
> > and I can do it in another patch.
> >
> >>               ;;
> >>       "fpunch" | "fcollapse" | "zero" | "fzero" | "finsert" | "funshare")
> >>               testio=`$XFS_IO_PROG -F -f -c "pwrite 0 20k" -c "fsync" \
> >> --
> >> 2.7.0.rc3
> >>
> >> --
> >> To unsubscribe from this list: send the line "unsubscribe fstests" in
> >> the body of a message to [e-mail]
> >> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/common/rc b/common/rc
index e1ab2c6..3d0f089 100644
--- a/common/rc
+++ b/common/rc
@@ -2021,8 +2021,8 @@  _require_xfs_io_command()
 	"chproj")
 		testio=`$XFS_IO_PROG -F -f -c "chproj 0" $testfile 2>&1`
 		;;
-	"falloc" )
-		testio=`$XFS_IO_PROG -F -f -c "falloc 0 1m" $testfile 2>&1`
+	"falloc*" )
+		testio=`$XFS_IO_PROG -F -f -c "$command 0 1m" $testfile 2>&1`
 		;;
 	"fpunch" | "fcollapse" | "zero" | "fzero" | "finsert" | "funshare")
 		testio=`$XFS_IO_PROG -F -f -c "pwrite 0 20k" -c "fsync" \