[PATCH 1/3] common/attr: require xfs_spaceman for xfs acl_get_max

[PATCH 1/3] common/attr: require xfs_spaceman for xfs acl_get_max

[David Disseldorp]

For xfs, _acl_get_max() calls xfs_info which in turn calls
"xfs_spaceman -c info ...".

Without this change, xfs_spaceman absence results in:
--- tests/generic/026.out
+++ /home/david/xfstests/results//generic/026.out.bad
@@ -1,9 +1,11 @@
 QA output created by 026
+/usr/sbin/xfs_info: line 44: xfs_spaceman: command not found

 === Test out large ACLs  ===
+/usr/sbin/xfs_info: line 44: xfs_spaceman: command not found
 1 below acl max
 acl max
...

Signed-off-by: David Disseldorp <ddiss@suse.de>
---
 common/attr | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/common/attr b/common/attr
index cce4d1b2..a94c5e4b 100644
--- a/common/attr
+++ b/common/attr
@@ -57,6 +57,9 @@ _acl_get_max()
 
 _require_acl_get_max()
 {
+	# _acl_get_max -> xfs_info -> xfs_spaceman
+	[ "$FSTYP" == "xfs" ] && _require_xfs_spaceman_command "info"
+
 	if [ $(_acl_get_max) -eq 0 ]; then
 		_notrun "$FSTYP does not define maximum ACL count"
 	fi
-- 
2.35.3

[PATCH 2/3] tests/generic/027: use c-style for loops

[David Disseldorp]

Signed-off-by: David Disseldorp <ddiss@suse.de>
---
 tests/generic/027 | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/tests/generic/027 b/tests/generic/027
index 47f1981d..002be120 100755
--- a/tests/generic/027
+++ b/tests/generic/027
@@ -53,22 +53,18 @@ fi
 
 dir=$SCRATCH_MNT/testdir
 echo -n "iteration" >>$seqres.full
-i=1
-while [ $i -le $loop ]; do
+for ((i = 1; i < loop; i++)); do
 	echo -n " $i" >>$seqres.full
-	nr_worker=8
-	while [ $nr_worker -gt 0 ]; do
+	for ((nr_worker = 8; nr_worker > 0; nr_worker--)); do
 		# half buffered I/O half direct I/O
 		if [ `expr $nr_worker % 2` -eq 0 ]; then
 			create_file $dir/$nr_worker -d >>$seqres.full &
 		else
 			create_file $dir/$nr_worker >>$seqres.full &
 		fi
-		let nr_worker=$nr_worker-1
 	done
 	wait
 	rm -rf $dir
-	let i=$i+1
 done
 _scratch_unmount
 
-- 
2.35.3

[PATCH 3/3] check: ensure sect_stop is initialized if interrupted

[David Disseldorp]

sect_stop is normally set immediately prior to calling _wrapup() via
run_section(). However, when called via a trap signal handler,
sect_stop may be uninitialized, leading to a negative section time
(sect_stop - sect_start) in the xunit report. E.g.
  Interrupted!
  Passed all 1 tests
  Xunit report: /home/david/xfstests/results//result.xml
  rapido1:/# head /home/david/xfstests/results//result.xml
  <?xml version="1.0" encoding="UTF-8"?>
  <testsuite name="xfstests" failures="0" skipped="0" tests="1"
   time="-1670885797" ... >

This commit uses the existing $interrupt flag to determine when
sect_stop needs to be initialised.

Signed-off-by: David Disseldorp <ddiss@suse.de>
---
 check | 1 +
 1 file changed, 1 insertion(+)

diff --git a/check b/check
index d2e51296..cb3ea155 100755
--- a/check
+++ b/check
@@ -433,6 +433,7 @@ _wrapup()
 {
 	seq="check"
 	check="$RESULT_BASE/check"
+	$interrupt && sect_stop=`_wallclock`
 
 	if $showme && $needwrap; then
 		if $do_report; then
-- 
2.35.3

Re: [PATCH 2/3] tests/generic/027: use c-style for loops

[Eric Biggers]

On Tue, Dec 13, 2022 at 12:08:19AM +0100, David Disseldorp wrote:
> Signed-off-by: David Disseldorp <ddiss@suse.de>
> ---
>  tests/generic/027 | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/tests/generic/027 b/tests/generic/027
> index 47f1981d..002be120 100755
> --- a/tests/generic/027
> +++ b/tests/generic/027
> @@ -53,22 +53,18 @@ fi
>  
>  dir=$SCRATCH_MNT/testdir
>  echo -n "iteration" >>$seqres.full
> -i=1
> -while [ $i -le $loop ]; do
> +for ((i = 1; i < loop; i++)); do

This does one fewer iteration than before.

- Eric

Re: [PATCH 2/3] tests/generic/027: use c-style for loops

[David Disseldorp]

On Mon, 12 Dec 2022 21:51:09 -0800, Eric Biggers wrote:

> > -while [ $i -le $loop ]; do
> > +for ((i = 1; i < loop; i++)); do  
> 
> This does one fewer iteration than before.

Gah you're right. I'll drop this patch as it was only intended as a
readability clean-up. Thanks for the review.

Cheers, David

Re: [PATCH 1/3] common/attr: require xfs_spaceman for xfs acl_get_max

[Zorro Lang]

On Tue, Dec 13, 2022 at 12:08:18AM +0100, David Disseldorp wrote:
> For xfs, _acl_get_max() calls xfs_info which in turn calls
> "xfs_spaceman -c info ...".
> 
> Without this change, xfs_spaceman absence results in:
> --- tests/generic/026.out
> +++ /home/david/xfstests/results//generic/026.out.bad
> @@ -1,9 +1,11 @@
>  QA output created by 026
> +/usr/sbin/xfs_info: line 44: xfs_spaceman: command not found

Hmm.. That's weird, both xfs_info and xfs_spaceman are from xfsprogs
package. If someone xfsprogs version doesn't hasve provide xfs_spaceman,
why its xfs_info would like to call that? What xfsprogs do you use?

Thanks,
Zorro

> 
>  === Test out large ACLs  ===
> +/usr/sbin/xfs_info: line 44: xfs_spaceman: command not found
>  1 below acl max
>  acl max
> ...
> 
> Signed-off-by: David Disseldorp <ddiss@suse.de>
> ---
>  common/attr | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/common/attr b/common/attr
> index cce4d1b2..a94c5e4b 100644
> --- a/common/attr
> +++ b/common/attr
> @@ -57,6 +57,9 @@ _acl_get_max()
>  
>  _require_acl_get_max()
>  {
> +	# _acl_get_max -> xfs_info -> xfs_spaceman
> +	[ "$FSTYP" == "xfs" ] && _require_xfs_spaceman_command "info"
> +
>  	if [ $(_acl_get_max) -eq 0 ]; then
>  		_notrun "$FSTYP does not define maximum ACL count"
>  	fi
> -- 
> 2.35.3
> 

Re: [PATCH 3/3] check: ensure sect_stop is initialized if interrupted

[Zorro Lang]

On Tue, Dec 13, 2022 at 12:08:20AM +0100, David Disseldorp wrote:
> sect_stop is normally set immediately prior to calling _wrapup() via
> run_section(). However, when called via a trap signal handler,
> sect_stop may be uninitialized, leading to a negative section time
> (sect_stop - sect_start) in the xunit report. E.g.
>   Interrupted!
>   Passed all 1 tests
>   Xunit report: /home/david/xfstests/results//result.xml
>   rapido1:/# head /home/david/xfstests/results//result.xml
>   <?xml version="1.0" encoding="UTF-8"?>
>   <testsuite name="xfstests" failures="0" skipped="0" tests="1"
>    time="-1670885797" ... >
> 
> This commit uses the existing $interrupt flag to determine when
> sect_stop needs to be initialised.
> 
> Signed-off-by: David Disseldorp <ddiss@suse.de>
> ---

I don't use Xunit report generally, but this change makes sense to me.

Reviewed-by: Zorro Lang <zlang@redhat.com>

>  check | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/check b/check
> index d2e51296..cb3ea155 100755
> --- a/check
> +++ b/check
> @@ -433,6 +433,7 @@ _wrapup()
>  {
>  	seq="check"
>  	check="$RESULT_BASE/check"
> +	$interrupt && sect_stop=`_wallclock`
>  
>  	if $showme && $needwrap; then
>  		if $do_report; then
> -- 
> 2.35.3
> 

Re: [PATCH 1/3] common/attr: require xfs_spaceman for xfs acl_get_max

[David Disseldorp]

On Tue, 13 Dec 2022 22:41:52 +0800, Zorro Lang wrote:

> Hmm.. That's weird, both xfs_info and xfs_spaceman are from xfsprogs
> package. If someone xfsprogs version doesn't hasve provide xfs_spaceman,
> why its xfs_info would like to call that? What xfsprogs do you use?

I have a hacky bunch of scripts that use Dracut to generate an
initramfs image with minimal dependencies for xfstests alongside the
test kernel: https://github.com/rapido-linux/rapido

The rapido dependency list is now fixed, but I figured that this change
makes sense given the other scattered _require_X checks for xfsprogs
binaries. Feel free to drop this change if you don't agree.

Cheers, David

Re: [PATCH 1/3] common/attr: require xfs_spaceman for xfs acl_get_max

[Zorro Lang]

On Tue, Dec 13, 2022 at 04:32:34PM +0100, David Disseldorp wrote:
> On Tue, 13 Dec 2022 22:41:52 +0800, Zorro Lang wrote:
> 
> > Hmm.. That's weird, both xfs_info and xfs_spaceman are from xfsprogs
> > package. If someone xfsprogs version doesn't hasve provide xfs_spaceman,
> > why its xfs_info would like to call that? What xfsprogs do you use?
> 
> I have a hacky bunch of scripts that use Dracut to generate an
> initramfs image with minimal dependencies for xfstests alongside the
> test kernel: https://github.com/rapido-linux/rapido
> 
> The rapido dependency list is now fixed, but I figured that this change
> makes sense given the other scattered _require_X checks for xfsprogs
> binaries. Feel free to drop this change if you don't agree.

We should let xfsprogs decide how xfs_info works. xfs_info doesn't always
depends on xfs_spaceman, old xfs_info might use xfs_db or other things to
get xfs info, and I don't know if it'll be changed to use other command in
the future again.

So this change will cause all cases which call _require_acl_get_max will notrun
on old (maybe future) xfsprogs which doesn't has xfs_spaceman "info" command.
I think that's not what we want, so I'd like to drop this patch. Thanks for
your understanding.

Thanks,
Zorro

> 
> Cheers, David
> 

Re: [PATCH 1/3] common/attr: require xfs_spaceman for xfs acl_get_max

[David Disseldorp]

On Wed, 14 Dec 2022 02:25:31 +0800, Zorro Lang wrote:

> We should let xfsprogs decide how xfs_info works. xfs_info doesn't always
> depends on xfs_spaceman, old xfs_info might use xfs_db or other things to
> get xfs info, and I don't know if it'll be changed to use other command in
> the future again.
> 
> So this change will cause all cases which call _require_acl_get_max will notrun
> on old (maybe future) xfsprogs which doesn't has xfs_spaceman "info" command.
> I think that's not what we want, so I'd like to drop this patch. Thanks for
> your understanding.

I wasn't aware that the xfs_info->xfs_spaceman call path was a new
change. With that in mind, I agree it makes sense to drop this patch.

Cheers, David

Re: [PATCH 1/3] common/attr: require xfs_spaceman for xfs acl_get_max

[Darrick J. Wong]

On Tue, Dec 13, 2022 at 08:08:31PM +0100, David Disseldorp wrote:
> On Wed, 14 Dec 2022 02:25:31 +0800, Zorro Lang wrote:
> 
> > We should let xfsprogs decide how xfs_info works. xfs_info doesn't always
> > depends on xfs_spaceman, old xfs_info might use xfs_db or other things to
> > get xfs info, and I don't know if it'll be changed to use other command in
> > the future again.
> > 
> > So this change will cause all cases which call _require_acl_get_max will notrun
> > on old (maybe future) xfsprogs which doesn't has xfs_spaceman "info" command.
> > I think that's not what we want, so I'd like to drop this patch. Thanks for
> > your understanding.
> 
> I wasn't aware that the xfs_info->xfs_spaceman call path was a new
> change. With that in mind, I agree it makes sense to drop this patch.

Since 4.17, xfs_info requires xfs_spaceman if the fs is mounted, or
xfs_db if unmounted.  Before that, it required xfs_growfs and only
worked on mounted filesystems.

That said, perhaps _acl_get_max should be doing:

	$XFS_INFO_PROG $TEST_DIR | _filter_mkfs > /dev/null 2> $tmp.info
	test "${PIPESTATUS[0]}" -ne 0 && \
		_fail "cannot get maximum acl count for xfs"

Rather than spraying whatever nonsense it does if spaceman is missing?

--D

> Cheers, David

Re: [PATCH 1/3] common/attr: require xfs_spaceman for xfs acl_get_max

[Zorro Lang]

On Tue, Dec 13, 2022 at 11:27:25AM -0800, Darrick J. Wong wrote:
> On Tue, Dec 13, 2022 at 08:08:31PM +0100, David Disseldorp wrote:
> > On Wed, 14 Dec 2022 02:25:31 +0800, Zorro Lang wrote:
> > 
> > > We should let xfsprogs decide how xfs_info works. xfs_info doesn't always
> > > depends on xfs_spaceman, old xfs_info might use xfs_db or other things to
> > > get xfs info, and I don't know if it'll be changed to use other command in
> > > the future again.
> > > 
> > > So this change will cause all cases which call _require_acl_get_max will notrun
> > > on old (maybe future) xfsprogs which doesn't has xfs_spaceman "info" command.
> > > I think that's not what we want, so I'd like to drop this patch. Thanks for
> > > your understanding.
> > 
> > I wasn't aware that the xfs_info->xfs_spaceman call path was a new
> > change. With that in mind, I agree it makes sense to drop this patch.
> 
> Since 4.17, xfs_info requires xfs_spaceman if the fs is mounted, or
> xfs_db if unmounted.  Before that, it required xfs_growfs and only
> worked on mounted filesystems.
> 
> That said, perhaps _acl_get_max should be doing:
> 
> 	$XFS_INFO_PROG $TEST_DIR | _filter_mkfs > /dev/null 2> $tmp.info
> 	test "${PIPESTATUS[0]}" -ne 0 && \
> 		_fail "cannot get maximum acl count for xfs"
> 
> Rather than spraying whatever nonsense it does if spaceman is missing?

Make more sense, but I don't think it's necessary to check return status
of each command line. xfs_info generally works well, we can do this change
when we have a real requirement in one day :)

Thanks,
Zorro

> 
> --D
> 
> > Cheers, David
>