[PATCH] xfs/444: add check for xfs_db write bno array

[PATCH] xfs/444: add check for xfs_db write bno array

[yang xu]

On RHEL7.5Alpha, the test case can trigger an xfsprogs bug that xfs_db
writes bno array failed ,such as:
  $ xfs_db -x -c "agfl 0" -c "write bno[32] 78" /dev/sda9
    bno[32] = 32:null 33:null 34:null 35:null 36:null 37:null ...

the correct result as below:
    bno[32] = 78

On xfs/444 the following code triggers this bug:
    cat $tmp.remap | while read dest_pos bno junk; do
          _scratch_xfs_set_metadata_field "bno[$dest_pos]" "$bno" \
          "agfl 0" >> $seqres.full
       done

It is because xfs_db on RHEL7.5Alpha doesn't support write via array
indexing.  The problem has been fixed since xfprogs commit 4222d000ed
("db: write via array indexing doesn't work").  But the fix patch has
not been merged into RHEL7.5Alpha.

For users, they should know the reason for the failure of the test is
a filesystem bug or an xfsprogs bug.  So, we add check for xfs_db
write bno array.

Signed-off-by: yang xu <xuyang.jy@cn.fujitsu.com>
---
 tests/xfs/444 | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/tests/xfs/444 b/tests/xfs/444
index 141be52..fe80502 100755
--- a/tests/xfs/444
+++ b/tests/xfs/444
@@ -58,6 +58,22 @@ _require_test_program "punch-alternating"
 # This is only a v5 filesystem problem
 _require_scratch_xfs_crc
 
+check_xfs_db_write_bno_array() {
+	_scratch_xfs_set_metadata_field "bno[32]" "78" "agfl 0" >> $seqres.full
+
+	# Before xfsprogs commit 0ebbf1d58898 ("db: limit AGFL bno
+	# arrayi printing), When asking for a single agfl entry,
+	# the result outputs the entire remainder of the array
+	# starting at the given index.
+	# It is difficult to extract single entry values.
+	# So filter them.
+	bno2=$(_scratch_xfs_get_metadata_field "bno[32]" \
+		"agfl 0" |sed -e 's/ .*$//g' | sed -e 's/^.*://g')
+	echo "bno[32] set 78 get $bno2" >> $seqres.full
+
+	[ "${bno2}" != "78" ] && _fail "xfs_db write can't support bno array"
+}
+
 mount_loop() {
 	if ! _try_scratch_mount >> $seqres.full 2>&1; then
 		echo "scratch mount failed" >> $seqres.full
@@ -233,6 +249,10 @@ ENDL
 	diff -u $tmp.repair $tmp.remount >> $seqres.full
 }
 
+#Before xfsprogs commit 4222d000ed("db: write via array indexing doesn't work").
+#xfs_db command to write a specific AGFL index doesn't work. We should check it.
+check_xfs_db_write_bno_array
+
 runtest fix_end
 runtest fix_start
 runtest fix_wrap
-- 
1.8.3.1

Re: [PATCH] xfs/444: add check for xfs_db write bno array

[Dave Chinner]

On Thu, Apr 12, 2018 at 06:00:30PM +0800, yang xu wrote:
> On RHEL7.5Alpha, the test case can trigger an xfsprogs bug that xfs_db
> writes bno array failed ,such as:
>   $ xfs_db -x -c "agfl 0" -c "write bno[32] 78" /dev/sda9
>     bno[32] = 32:null 33:null 34:null 35:null 36:null 37:null ...
> 
> the correct result as below:
>     bno[32] = 78
> 
> On xfs/444 the following code triggers this bug:
>     cat $tmp.remap | while read dest_pos bno junk; do
>           _scratch_xfs_set_metadata_field "bno[$dest_pos]" "$bno" \
>           "agfl 0" >> $seqres.full
>        done
> 
> It is because xfs_db on RHEL7.5Alpha doesn't support write via array
> indexing.  The problem has been fixed since xfprogs commit 4222d000ed
> ("db: write via array indexing doesn't work").  But the fix patch has
> not been merged into RHEL7.5Alpha.
> 
> For users, they should know the reason for the failure of the test is
> a filesystem bug or an xfsprogs bug.  So, we add check for xfs_db
> write bno array.
> 
> Signed-off-by: yang xu <xuyang.jy@cn.fujitsu.com>
> ---
>  tests/xfs/444 | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/tests/xfs/444 b/tests/xfs/444
> index 141be52..fe80502 100755
> --- a/tests/xfs/444
> +++ b/tests/xfs/444
> @@ -58,6 +58,22 @@ _require_test_program "punch-alternating"
>  # This is only a v5 filesystem problem
>  _require_scratch_xfs_crc
>  
> +check_xfs_db_write_bno_array() {

_require_xfs_db_write_array(), and move to common/xfs.

> +	_scratch_xfs_set_metadata_field "bno[32]" "78" "agfl 0" >> $seqres.full
> +
> +	# Before xfsprogs commit 0ebbf1d58898 ("db: limit AGFL bno
> +	# arrayi printing), When asking for a single agfl entry,
> +	# the result outputs the entire remainder of the array
> +	# starting at the given index.
> +	# It is difficult to extract single entry values.
> +	# So filter them.
> +	bno2=$(_scratch_xfs_get_metadata_field "bno[32]" \
> +		"agfl 0" |sed -e 's/ .*$//g' | sed -e 's/^.*://g')
> +	echo "bno[32] set 78 get $bno2" >> $seqres.full
> +
> +	[ "${bno2}" != "78" ] && _fail "xfs_db write can't support bno array"

test should _notrun is db doesn't have therequired support, not
_fail.

> +#Before xfsprogs commit 4222d000ed("db: write via array indexing doesn't work").
> +#xfs_db command to write a specific AGFL index doesn't work. We should check it.
> +check_xfs_db_write_bno_array

This comment belongs with the function, not the caller.

Cheers,

Dave.

-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] xfs/444: add check for xfs_db write bno array

[Eryu Guan]

On Thu, Apr 12, 2018 at 10:09:52PM +1000, Dave Chinner wrote:
> On Thu, Apr 12, 2018 at 06:00:30PM +0800, yang xu wrote:
> > On RHEL7.5Alpha, the test case can trigger an xfsprogs bug that xfs_db
> > writes bno array failed ,such as:
> >   $ xfs_db -x -c "agfl 0" -c "write bno[32] 78" /dev/sda9
> >     bno[32] = 32:null 33:null 34:null 35:null 36:null 37:null ...
> > 
> > the correct result as below:
> >     bno[32] = 78
> > 
> > On xfs/444 the following code triggers this bug:
> >     cat $tmp.remap | while read dest_pos bno junk; do
> >           _scratch_xfs_set_metadata_field "bno[$dest_pos]" "$bno" \
> >           "agfl 0" >> $seqres.full
> >        done
> > 
> > It is because xfs_db on RHEL7.5Alpha doesn't support write via array
> > indexing.  The problem has been fixed since xfprogs commit 4222d000ed
> > ("db: write via array indexing doesn't work").  But the fix patch has
> > not been merged into RHEL7.5Alpha.
> > 
> > For users, they should know the reason for the failure of the test is
> > a filesystem bug or an xfsprogs bug.  So, we add check for xfs_db
> > write bno array.
> > 
> > Signed-off-by: yang xu <xuyang.jy@cn.fujitsu.com>

(Please cc xfs list too for xfs-specific test cases in future. I didn't
add xfs list in this thread as Dave already replied).

> > ---
> >  tests/xfs/444 | 20 ++++++++++++++++++++
> >  1 file changed, 20 insertions(+)
> > 
> > diff --git a/tests/xfs/444 b/tests/xfs/444
> > index 141be52..fe80502 100755
> > --- a/tests/xfs/444
> > +++ b/tests/xfs/444
> > @@ -58,6 +58,22 @@ _require_test_program "punch-alternating"
> >  # This is only a v5 filesystem problem
> >  _require_scratch_xfs_crc
> >  
> > +check_xfs_db_write_bno_array() {
> 
> _require_xfs_db_write_array(), and move to common/xfs.
> 
> > +	_scratch_xfs_set_metadata_field "bno[32]" "78" "agfl 0" >> $seqres.full
> > +
> > +	# Before xfsprogs commit 0ebbf1d58898 ("db: limit AGFL bno
> > +	# arrayi printing), When asking for a single agfl entry,
> > +	# the result outputs the entire remainder of the array
> > +	# starting at the given index.
> > +	# It is difficult to extract single entry values.
> > +	# So filter them.
> > +	bno2=$(_scratch_xfs_get_metadata_field "bno[32]" \
> > +		"agfl 0" |sed -e 's/ .*$//g' | sed -e 's/^.*://g')
> > +	echo "bno[32] set 78 get $bno2" >> $seqres.full
> > +
> > +	[ "${bno2}" != "78" ] && _fail "xfs_db write can't support bno array"
> 
> test should _notrun is db doesn't have therequired support, not
> _fail.

If xfsprogs commit 4222d000ed ("db: write via array indexing doesn't
work") introduced a new feature, I agreed that we need the _require rule
and _notrun the test.

But this issue looks like a bug in xfsprogs to me, not a missing feature
in xfsprogs on RHEL7, so I tend to fail the test instead of adding a new
_require rule & _notrun the test. And in this case, IMHO, I don't think
it's necessary to do any update to the test, just leave the test as it
is and file a new bug in Red Hat bugzilla.

> 
> > +#Before xfsprogs commit 4222d000ed("db: write via array indexing doesn't work").
> > +#xfs_db command to write a specific AGFL index doesn't work. We should check it.
> > +check_xfs_db_write_bno_array
> 
> This comment belongs with the function, not the caller.

Agreed.

Thanks,
Eryu

> 
> Cheers,
> 
> Dave.
> 
> -- 
> Dave Chinner
> david@fromorbit.com
> --
> To unsubscribe from this list: send the line "unsubscribe fstests" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] xfs/444: add check for xfs_db write bno array

[Dave Chinner]

On Thu, Apr 12, 2018 at 09:00:38PM +0800, Eryu Guan wrote:
> On Thu, Apr 12, 2018 at 10:09:52PM +1000, Dave Chinner wrote:
> > On Thu, Apr 12, 2018 at 06:00:30PM +0800, yang xu wrote:
> > > ---
> > >  tests/xfs/444 | 20 ++++++++++++++++++++
> > >  1 file changed, 20 insertions(+)
> > > 
> > > diff --git a/tests/xfs/444 b/tests/xfs/444
> > > index 141be52..fe80502 100755
> > > --- a/tests/xfs/444
> > > +++ b/tests/xfs/444
> > > @@ -58,6 +58,22 @@ _require_test_program "punch-alternating"
> > >  # This is only a v5 filesystem problem
> > >  _require_scratch_xfs_crc
> > >  
> > > +check_xfs_db_write_bno_array() {
> > 
> > _require_xfs_db_write_array(), and move to common/xfs.
> > 
> > > +	_scratch_xfs_set_metadata_field "bno[32]" "78" "agfl 0" >> $seqres.full
> > > +
> > > +	# Before xfsprogs commit 0ebbf1d58898 ("db: limit AGFL bno
> > > +	# arrayi printing), When asking for a single agfl entry,
> > > +	# the result outputs the entire remainder of the array
> > > +	# starting at the given index.
> > > +	# It is difficult to extract single entry values.
> > > +	# So filter them.
> > > +	bno2=$(_scratch_xfs_get_metadata_field "bno[32]" \
> > > +		"agfl 0" |sed -e 's/ .*$//g' | sed -e 's/^.*://g')
> > > +	echo "bno[32] set 78 get $bno2" >> $seqres.full
> > > +
> > > +	[ "${bno2}" != "78" ] && _fail "xfs_db write can't support bno array"
> > 
> > test should _notrun is db doesn't have therequired support, not
> > _fail.
> 
> If xfsprogs commit 4222d000ed ("db: write via array indexing doesn't
> work") introduced a new feature, I agreed that we need the _require rule
> and _notrun the test.

_require* rules are not for "new features" - they are for defining
the support needed to run the test correctly. This is no different
to detecting the mkfs binary version we are running to determine
if a certain fix is present or not:

# Skip the test if all calls passed - mkfs accepts invalid input
_require_xfs_mkfs_validation()
{
        _xfs_mkfs_validation_check
        if [ "$?" -eq 0 ]; then
                _notrun "Requires newer mkfs with stricter input checks: the oldest supported version of xfsprogs is 4.7."
        fi
}

This is exactly the same sort of situation, because....

> But this issue looks like a bug in xfsprogs to me, not a missing feature
> in xfsprogs on RHEL7, so I tend to fail the test instead of adding a new
> _require rule & _notrun the test. And in this case, IMHO, I don't think
> it's necessary to do any update to the test, just leave the test as it
> is and file a new bug in Red Hat bugzilla.

... this isn't a RHEL specific issue - it's an xfsprogs version
issue.  i.e.  any older distro that has a binary with a broken
"write array" command will fail this test. None of them are going to
get updated xfsprogs packages, so like having an old mkfs.xfs
binary, this test should run conditionally on having a version of
xfs_db that actually works correctly....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] xfs/444: add check for xfs_db write bno array

[Eryu Guan]

On Fri, Apr 13, 2018 at 09:00:35AM +1000, Dave Chinner wrote:
> On Thu, Apr 12, 2018 at 09:00:38PM +0800, Eryu Guan wrote:
> > On Thu, Apr 12, 2018 at 10:09:52PM +1000, Dave Chinner wrote:
> > > On Thu, Apr 12, 2018 at 06:00:30PM +0800, yang xu wrote:
> > > > ---
> > > >  tests/xfs/444 | 20 ++++++++++++++++++++
> > > >  1 file changed, 20 insertions(+)
> > > > 
> > > > diff --git a/tests/xfs/444 b/tests/xfs/444
> > > > index 141be52..fe80502 100755
> > > > --- a/tests/xfs/444
> > > > +++ b/tests/xfs/444
> > > > @@ -58,6 +58,22 @@ _require_test_program "punch-alternating"
> > > >  # This is only a v5 filesystem problem
> > > >  _require_scratch_xfs_crc
> > > >  
> > > > +check_xfs_db_write_bno_array() {
> > > 
> > > _require_xfs_db_write_array(), and move to common/xfs.
> > > 
> > > > +	_scratch_xfs_set_metadata_field "bno[32]" "78" "agfl 0" >> $seqres.full
> > > > +
> > > > +	# Before xfsprogs commit 0ebbf1d58898 ("db: limit AGFL bno
> > > > +	# arrayi printing), When asking for a single agfl entry,
> > > > +	# the result outputs the entire remainder of the array
> > > > +	# starting at the given index.
> > > > +	# It is difficult to extract single entry values.
> > > > +	# So filter them.
> > > > +	bno2=$(_scratch_xfs_get_metadata_field "bno[32]" \
> > > > +		"agfl 0" |sed -e 's/ .*$//g' | sed -e 's/^.*://g')
> > > > +	echo "bno[32] set 78 get $bno2" >> $seqres.full
> > > > +
> > > > +	[ "${bno2}" != "78" ] && _fail "xfs_db write can't support bno array"
> > > 
> > > test should _notrun is db doesn't have therequired support, not
> > > _fail.
> > 
> > If xfsprogs commit 4222d000ed ("db: write via array indexing doesn't
> > work") introduced a new feature, I agreed that we need the _require rule
> > and _notrun the test.
> 
> _require* rules are not for "new features" - they are for defining
> the support needed to run the test correctly. This is no different

Agreed, I was not clear previously, by "new features" I mean changes
like new features that change the behavior.

> to detecting the mkfs binary version we are running to determine
> if a certain fix is present or not:
> 
> # Skip the test if all calls passed - mkfs accepts invalid input
> _require_xfs_mkfs_validation()
> {
>         _xfs_mkfs_validation_check
>         if [ "$?" -eq 0 ]; then
>                 _notrun "Requires newer mkfs with stricter input checks: the oldest supported version of xfsprogs is 4.7."
>         fi
> }

Just like this _require rule, it's more like a behavior change that
added stricter input checks.

> 
> This is exactly the same sort of situation, because....
> 
> > But this issue looks like a bug in xfsprogs to me, not a missing feature
> > in xfsprogs on RHEL7, so I tend to fail the test instead of adding a new
> > _require rule & _notrun the test. And in this case, IMHO, I don't think
> > it's necessary to do any update to the test, just leave the test as it
> > is and file a new bug in Red Hat bugzilla.
> 
> ... this isn't a RHEL specific issue - it's an xfsprogs version
> issue.  i.e.  any older distro that has a binary with a broken
> "write array" command will fail this test. None of them are going to
> get updated xfsprogs packages, so like having an old mkfs.xfs
> binary, this test should run conditionally on having a version of
> xfs_db that actually works correctly....

But I still think it's a pure bug in xfsprogs, not xfsprogs version
issue nor a behavior change in xfsprogs, as we did support "write via
array indexing", just that it was broken in a certain case, and commit
4222d000ed3b fixed that bug. We should expose bugs by letting the test
fail, not paper over it by _notrun the test. 

Thanks,
Eryu

Re: [PATCH] xfs/444: add check for xfs_db write bno array

[Dave Chinner]

On Fri, Apr 13, 2018 at 12:36:12PM +0800, Eryu Guan wrote:
> > > But this issue looks like a bug in xfsprogs to me, not a missing feature
> > > in xfsprogs on RHEL7, so I tend to fail the test instead of adding a new
> > > _require rule & _notrun the test. And in this case, IMHO, I don't think
> > > it's necessary to do any update to the test, just leave the test as it
> > > is and file a new bug in Red Hat bugzilla.
> > 
> > ... this isn't a RHEL specific issue - it's an xfsprogs version
> > issue.  i.e.  any older distro that has a binary with a broken
> > "write array" command will fail this test. None of them are going to
> > get updated xfsprogs packages, so like having an old mkfs.xfs
> > binary, this test should run conditionally on having a version of
> > xfs_db that actually works correctly....
> 
> But I still think it's a pure bug in xfsprogs, not xfsprogs version
> issue nor a behavior change in xfsprogs, as we did support "write via
> array indexing", just that it was broken in a certain case, and commit
> 4222d000ed3b fixed that bug. We should expose bugs by letting the test
> fail, not paper over it by _notrun the test. 

Yes, it's a bug in xfsprogs. But it's a bug in a diagnostic
utility that is only used by test infrastructure and XFS
developers.

Yes, it's ialso fixed in recent version of xfsprogs, but you know
very well that we test distros that have ancient xfsprogs and will
never have this issue fixed in them. We use detectiona nd notrun to
avoid tests they should not run all the time, and I don't see how
this is any different.

I really don't understand why you are pushing back on this - why
should this specific infrastructure deficiency cause test failures,
when all the existing infrastructure support checks cause tests to
notrun rather than fail?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] xfs/444: add check for xfs_db write bno array

[Eryu Guan]

On Fri, Apr 13, 2018 at 11:42:10PM +1000, Dave Chinner wrote:
> On Fri, Apr 13, 2018 at 12:36:12PM +0800, Eryu Guan wrote:
> > > > But this issue looks like a bug in xfsprogs to me, not a missing feature
> > > > in xfsprogs on RHEL7, so I tend to fail the test instead of adding a new
> > > > _require rule & _notrun the test. And in this case, IMHO, I don't think
> > > > it's necessary to do any update to the test, just leave the test as it
> > > > is and file a new bug in Red Hat bugzilla.
> > > 
> > > ... this isn't a RHEL specific issue - it's an xfsprogs version
> > > issue.  i.e.  any older distro that has a binary with a broken
> > > "write array" command will fail this test. None of them are going to
> > > get updated xfsprogs packages, so like having an old mkfs.xfs
> > > binary, this test should run conditionally on having a version of
> > > xfs_db that actually works correctly....
> > 
> > But I still think it's a pure bug in xfsprogs, not xfsprogs version
> > issue nor a behavior change in xfsprogs, as we did support "write via
> > array indexing", just that it was broken in a certain case, and commit
> > 4222d000ed3b fixed that bug. We should expose bugs by letting the test
> > fail, not paper over it by _notrun the test. 
> 
> Yes, it's a bug in xfsprogs. But it's a bug in a diagnostic
> utility that is only used by test infrastructure and XFS
> developers.

OK, I got your point now, it's a bug in a diagnostic tool that is only
used by XFS developers as a test infrastructure, so we can treat it as a
infrastructure dependency as all other _require rules. I think that
makes sense in this case. Thanks for the explanation!

yang xu, would you mind sending a v2 patch as Dave suggested? Thanks!

Eryu

> 
> Yes, it's ialso fixed in recent version of xfsprogs, but you know
> very well that we test distros that have ancient xfsprogs and will
> never have this issue fixed in them. We use detectiona nd notrun to
> avoid tests they should not run all the time, and I don't see how
> this is any different.
> 
> I really don't understand why you are pushing back on this - why
> should this specific infrastructure deficiency cause test failures,
> when all the existing infrastructure support checks cause tests to
> notrun rather than fail?
> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com

[PATCH v2] common/xfs: Add require_xfs_db_write_array function

[yang xu]

xfsprogs commit 4222d000ed("db: write via array indexing doesn't
work") fixes a bug that xfs_db write can't support array indexing.
This function will check whether the bug is fixed on the current
xfsprogs.

xfs/444 applies the function, and skips if this bug exists.

Signed-off-by: yang xu <xuyang.jy@cn.fujitsu.com>
---
 common/xfs    | 18 ++++++++++++++++++
 tests/xfs/444 |  1 +
 2 files changed, 19 insertions(+)

diff --git a/common/xfs b/common/xfs
index 3169f87..c4611a1 100644
--- a/common/xfs
+++ b/common/xfs
@@ -701,3 +701,21 @@ _scratch_xfs_set_sb_field()
 {
 	_scratch_xfs_set_metadata_field "$1" "$2" "sb 0"
 }
+
+#Before xfsprogs commit 4222d000ed("db: write via array indexing doesn't work"),
+#xfs_db command to write a specific AGFL index doesn't work.
+_require_xfs_db_write_array()
+{
+	_scratch_xfs_set_metadata_field "bno[32]" "78" "agfl 0" >/dev/null 2>&1
+
+	# Before xfsprogs commit 0ebbf1d58898 ("db: limit AGFL bno
+	# arrayi printing), When asking for a single agfl entry,
+	# the result outputs the entire remainder of the array
+	# starting at the given index.
+	# It is difficult to extract single entry values.
+	# So filter useless information.
+	bno_value=$(_scratch_xfs_get_metadata_field "bno[32]" \
+		"agfl 0" |sed -e 's/ .*$//g' | sed -e 's/^.*://g')
+
+	[ "${bno_value}" != "78" ] && _notrun "xfs_db write can't support array"
+}
diff --git a/tests/xfs/444 b/tests/xfs/444
index 141be52..9700422 100755
--- a/tests/xfs/444
+++ b/tests/xfs/444
@@ -54,6 +54,7 @@ _supported_os Linux
 _require_check_dmesg
 _require_scratch
 _require_test_program "punch-alternating"
+_require_xfs_db_write_array
 
 # This is only a v5 filesystem problem
 _require_scratch_xfs_crc
-- 
1.8.3.1

Re: [PATCH v2] common/xfs: Add require_xfs_db_write_array function

[Darrick J. Wong]

On Mon, Apr 16, 2018 at 05:07:35PM +0800, yang xu wrote:
> xfsprogs commit 4222d000ed("db: write via array indexing doesn't
> work") fixes a bug that xfs_db write can't support array indexing.
> This function will check whether the bug is fixed on the current
> xfsprogs.
> 
> xfs/444 applies the function, and skips if this bug exists.
> 
> Signed-off-by: yang xu <xuyang.jy@cn.fujitsu.com>
> ---
>  common/xfs    | 18 ++++++++++++++++++
>  tests/xfs/444 |  1 +
>  2 files changed, 19 insertions(+)
> 
> diff --git a/common/xfs b/common/xfs
> index 3169f87..c4611a1 100644
> --- a/common/xfs
> +++ b/common/xfs
> @@ -701,3 +701,21 @@ _scratch_xfs_set_sb_field()
>  {
>  	_scratch_xfs_set_metadata_field "$1" "$2" "sb 0"
>  }
> +
> +#Before xfsprogs commit 4222d000ed("db: write via array indexing doesn't work"),
> +#xfs_db command to write a specific AGFL index doesn't work.
> +_require_xfs_db_write_array()
> +{
> +	_scratch_xfs_set_metadata_field "bno[32]" "78" "agfl 0" >/dev/null 2>&1

This is going to fail randomly unless the caller previously called
_require_scratch to make sure that there's some kind of xfs fs image
sitting on the scratch device (and that there even is a scratch device).
Please document that requirement.  Alternately, consider formatting a xfs
image file in $TEST_DIR and poking that with xfs_db so you don't have to
deal with formatting and cleaning up the scratch device.

> +
> +	# Before xfsprogs commit 0ebbf1d58898 ("db: limit AGFL bno
> +	# arrayi printing), When asking for a single agfl entry,
> +	# the result outputs the entire remainder of the array
> +	# starting at the given index.
> +	# It is difficult to extract single entry values.
> +	# So filter useless information.
> +	bno_value=$(_scratch_xfs_get_metadata_field "bno[32]" \
> +		"agfl 0" |sed -e 's/ .*$//g' | sed -e 's/^.*://g')
> +
> +	[ "${bno_value}" != "78" ] && _notrun "xfs_db write can't support array"

It seems like a bad idea to leave a potentially corrupt scratch fs lying
around after this helper exits.

--D

> +}
> diff --git a/tests/xfs/444 b/tests/xfs/444
> index 141be52..9700422 100755
> --- a/tests/xfs/444
> +++ b/tests/xfs/444
> @@ -54,6 +54,7 @@ _supported_os Linux
>  _require_check_dmesg
>  _require_scratch
>  _require_test_program "punch-alternating"
> +_require_xfs_db_write_array
>  
>  # This is only a v5 filesystem problem
>  _require_scratch_xfs_crc
> -- 
> 1.8.3.1
> 
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe fstests" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] common/xfs: Add require_xfs_db_write_array function

[Darrick J. Wong]

On Tue, Apr 17, 2018 at 12:03:40PM +0800, xuyang.jy wrote:
> on 2018/4/17 1:22, Darrick J. Wong write:
> 
> >On Mon, Apr 16, 2018 at 05:07:35PM +0800, yang xu wrote:
> >>xfsprogs commit 4222d000ed("db: write via array indexing doesn't
> >>work") fixes a bug that xfs_db write can't support array indexing.
> >>This function will check whether the bug is fixed on the current
> >>xfsprogs.
> >>
> >>xfs/444 applies the function, and skips if this bug exists.
> >>
> >>Signed-off-by: yang xu<xuyang.jy@cn.fujitsu.com>
> >>---
> >>  common/xfs    | 18 ++++++++++++++++++
> >>  tests/xfs/444 |  1 +
> >>  2 files changed, 19 insertions(+)
> >>
> >>diff --git a/common/xfs b/common/xfs
> >>index 3169f87..c4611a1 100644
> >>--- a/common/xfs
> >>+++ b/common/xfs
> >>@@ -701,3 +701,21 @@ _scratch_xfs_set_sb_field()
> >>  {
> >>  	_scratch_xfs_set_metadata_field "$1" "$2" "sb 0"
> >>  }
> >>+
> >>+#Before xfsprogs commit 4222d000ed("db: write via array indexing doesn't work"),
> >>+#xfs_db command to write a specific AGFL index doesn't work.
> >>+_require_xfs_db_write_array()
> >>+{
> >>+	_scratch_xfs_set_metadata_field "bno[32]" "78" "agfl 0">/dev/null 2>&1
> >This is going to fail randomly unless the caller previously called
> >_require_scratch to make sure that there's some kind of xfs fs image
> >sitting on the scratch device (and that there even is a scratch device).
> >Please document that requirement.  Alternately, consider formatting a xfs
> >image file in $TEST_DIR and poking that with xfs_db so you don't have to
> >deal with formatting and cleaning up the scratch device.
> Hi Darrick
> 
> We want to create a xfs image file in /tmp/ and use it, as below:
> 
> [root@RHEL7U5Alpha_CLIENT xfstests-dev]# git diff common/xfs
> diff --git a/common/xfs b/common/xfs
> index 3169f87..39702f7 100644
> --- a/common/xfs
> +++ b/common/xfs
> @@ -701,3 +701,19 @@ _scratch_xfs_set_sb_field()
>  {      	_scratch_xfs_set_metadata_field "$1" "$2" "sb 0"
>  }
> +
> +#Before xfsprogs commit 4222d000ed("db: write via array indexing doesn't work"),
> +##xfs_db command to write a specific AGFL index doesn't work.
> +_require_xfs_db_write_array()
> +{
> +       local supported=0
> +
> +       touch /tmp/$$.img
> +       $MKFS_XFS_PROG -d file,name=/tmp/$$.img,size=512m>/dev/null 2>&1
> +       $XFS_DB_PROG -x -c "agfl 0" -c "write bno[32] 78" /tmp/$$.img \
> +>/dev/null 2>&1
> +       $XFS_DB_PROG -x -c "agfl 0" -c "print bno[32]" /tmp/$$.img \
> +               | grep -q "bno\[32\] = 78"&&  supported=1
> +       rm -f /tmp/$$.img
> +       [ $supported -eq 0 ]&&  _notrun "xfs_db write can't support array"
> +}
> [root@RHEL7U5Alpha_CLIENT xfstests-dev]#
> 
> How about this modification?

$TEST_DIR/$seqres.img, not /tmp/$$.img

Don't assume anything's writable (and big enough for fs images) if it
isn't under $TEST_DIR or $SCRATCH_MNT.

--D

> 
> >>+
> >>+	# Before xfsprogs commit 0ebbf1d58898 ("db: limit AGFL bno
> >>+	# arrayi printing), When asking for a single agfl entry,
> >>+	# the result outputs the entire remainder of the array
> >>+	# starting at the given index.
> >>+	# It is difficult to extract single entry values.
> >>+	# So filter useless information.
> >>+	bno_value=$(_scratch_xfs_get_metadata_field "bno[32]" \
> >>+		"agfl 0" |sed -e 's/ .*$//g' | sed -e 's/^.*://g')
> >>+
> >>+	[ "${bno_value}" != "78" ]&&  _notrun "xfs_db write can't support array"
> >It seems like a bad idea to leave a potentially corrupt scratch fs lying
> >around after this helper exits.
> Agreed.  I will use a temp xfs image file.
> 
> Thanks
> Yang Xu
> 
> >--D
> >
> >>+}
> >>diff --git a/tests/xfs/444 b/tests/xfs/444
> >>index 141be52..9700422 100755
> >>--- a/tests/xfs/444
> >>+++ b/tests/xfs/444
> >>@@ -54,6 +54,7 @@ _supported_os Linux
> >>  _require_check_dmesg
> >>  _require_scratch
> >>  _require_test_program "punch-alternating"
> >>+_require_xfs_db_write_array
> >>
> >>  # This is only a v5 filesystem problem
> >>  _require_scratch_xfs_crc
> >>-- 
> >>1.8.3.1
> >>
> >>
> >>
> >>--
> >>To unsubscribe from this list: send the line "unsubscribe fstests" in
> >>the body of a message tomajordomo@vger.kernel.org
> >>More majordomo info athttp://vger.kernel.org/majordomo-info.html
> >.
> >
> 
> 
> 

[PATCH v3] common/xfs: Add require_xfs_db_write_array function

[yang xu]

xfsprogs commit 4222d00("db: write via array indexing doesn't
work") fixes a bug that xfs_db write can't support array indexing.
This function will check whether the bug is fixed on the current
xfsprogs.

xfs/444 applies the function, and skips if this bug exists.

Signed-off-by: yang xu <xuyang.jy@cn.fujitsu.com>
---
 common/xfs    | 17 +++++++++++++++++
 tests/xfs/444 |  1 +
 2 files changed, 18 insertions(+)

diff --git a/common/xfs b/common/xfs
index 3169f87..0b59994 100644
--- a/common/xfs
+++ b/common/xfs
@@ -701,3 +701,20 @@ _scratch_xfs_set_sb_field()
 {
 	_scratch_xfs_set_metadata_field "$1" "$2" "sb 0"
 }
+
+#Before xfsprogs commit 4222d000ed("db: write via array indexing doesn't work"),
+##xfs_db command to write a specific AGFL index doesn't work.
+_require_xfs_db_write_array()
+{
+	local supported=0
+
+	_require_test
+	touch $TEST_DIR/$seq.img
+	$MKFS_XFS_PROG -d file,name=$TEST_DIR/$seq.img,size=512m >/dev/null 2>&1
+	$XFS_DB_PROG -x -c "agfl 0" -c "write bno[32] 78" $TEST_DIR/$seq.img \
+		>/dev/null 2>&1
+	$XFS_DB_PROG -x -c "agfl 0" -c "print bno[32]" $TEST_DIR/$seq.img \
+		| grep -q "bno\[32\] = 78" && supported=1
+	rm -f $TEST_DIR/$seq.img
+	[ $supported -eq 0 ] && _notrun "xfs_db write can't support array"
+}
diff --git a/tests/xfs/444 b/tests/xfs/444
index 141be52..9700422 100755
--- a/tests/xfs/444
+++ b/tests/xfs/444
@@ -54,6 +54,7 @@ _supported_os Linux
 _require_check_dmesg
 _require_scratch
 _require_test_program "punch-alternating"
+_require_xfs_db_write_array
 
 # This is only a v5 filesystem problem
 _require_scratch_xfs_crc
-- 
1.8.3.1

Re: [PATCH v3] common/xfs: Add require_xfs_db_write_array function

[Darrick J. Wong]

On Tue, Apr 17, 2018 at 02:11:40PM +0800, yang xu wrote:
> xfsprogs commit 4222d00("db: write via array indexing doesn't
> work") fixes a bug that xfs_db write can't support array indexing.
> This function will check whether the bug is fixed on the current
> xfsprogs.
> 
> xfs/444 applies the function, and skips if this bug exists.
> 
> Signed-off-by: yang xu <xuyang.jy@cn.fujitsu.com>
> ---
>  common/xfs    | 17 +++++++++++++++++
>  tests/xfs/444 |  1 +
>  2 files changed, 18 insertions(+)
> 
> diff --git a/common/xfs b/common/xfs
> index 3169f87..0b59994 100644
> --- a/common/xfs
> +++ b/common/xfs
> @@ -701,3 +701,20 @@ _scratch_xfs_set_sb_field()
>  {
>  	_scratch_xfs_set_metadata_field "$1" "$2" "sb 0"
>  }
> +
> +#Before xfsprogs commit 4222d000ed("db: write via array indexing doesn't work"),
> +##xfs_db command to write a specific AGFL index doesn't work.
> +_require_xfs_db_write_array()
> +{
> +	local supported=0
> +
> +	_require_test
> +	touch $TEST_DIR/$seq.img
> +	$MKFS_XFS_PROG -d file,name=$TEST_DIR/$seq.img,size=512m >/dev/null 2>&1
> +	$XFS_DB_PROG -x -c "agfl 0" -c "write bno[32] 78" $TEST_DIR/$seq.img \
> +		>/dev/null 2>&1
> +	$XFS_DB_PROG -x -c "agfl 0" -c "print bno[32]" $TEST_DIR/$seq.img \
> +		| grep -q "bno\[32\] = 78" && supported=1
> +	rm -f $TEST_DIR/$seq.img
> +	[ $supported -eq 0 ] && _notrun "xfs_db write can't support array"
> +}
> diff --git a/tests/xfs/444 b/tests/xfs/444
> index 141be52..9700422 100755
> --- a/tests/xfs/444
> +++ b/tests/xfs/444
> @@ -54,6 +54,7 @@ _supported_os Linux
>  _require_check_dmesg
>  _require_scratch
>  _require_test_program "punch-alternating"
> +_require_xfs_db_write_array

Looks ok,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

>  
>  # This is only a v5 filesystem problem
>  _require_scratch_xfs_crc
> -- 
> 1.8.3.1
> 
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe fstests" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3] common/xfs: Add require_xfs_db_write_array function

[Eryu Guan]

On Tue, Apr 17, 2018 at 10:59:17AM -0700, Darrick J. Wong wrote:
> On Tue, Apr 17, 2018 at 02:11:40PM +0800, yang xu wrote:
> > xfsprogs commit 4222d00("db: write via array indexing doesn't
> > work") fixes a bug that xfs_db write can't support array indexing.
> > This function will check whether the bug is fixed on the current
> > xfsprogs.
> > 
> > xfs/444 applies the function, and skips if this bug exists.
> > 
> > Signed-off-by: yang xu <xuyang.jy@cn.fujitsu.com>
> > ---
> >  common/xfs    | 17 +++++++++++++++++
> >  tests/xfs/444 |  1 +
> >  2 files changed, 18 insertions(+)
> > 
> > diff --git a/common/xfs b/common/xfs
> > index 3169f87..0b59994 100644
> > --- a/common/xfs
> > +++ b/common/xfs
> > @@ -701,3 +701,20 @@ _scratch_xfs_set_sb_field()
> >  {
> >  	_scratch_xfs_set_metadata_field "$1" "$2" "sb 0"
> >  }
> > +
> > +#Before xfsprogs commit 4222d000ed("db: write via array indexing doesn't work"),
> > +##xfs_db command to write a specific AGFL index doesn't work.

I'll add more comments here to explain why we require a bug fix instead
of exposing the bug to users, e.g.

"It's a bug in a diagnostic tool that is only used by XFS developers as
a test infrastructure, so we can treat it as a infrastructure dependency
as all other _require rules."

> > +_require_xfs_db_write_array()
> > +{
> > +	local supported=0
> > +
> > +	_require_test
> > +	touch $TEST_DIR/$seq.img
> > +	$MKFS_XFS_PROG -d file,name=$TEST_DIR/$seq.img,size=512m >/dev/null 2>&1
> > +	$XFS_DB_PROG -x -c "agfl 0" -c "write bno[32] 78" $TEST_DIR/$seq.img \
> > +		>/dev/null 2>&1
> > +	$XFS_DB_PROG -x -c "agfl 0" -c "print bno[32]" $TEST_DIR/$seq.img \
> > +		| grep -q "bno\[32\] = 78" && supported=1
> > +	rm -f $TEST_DIR/$seq.img
> > +	[ $supported -eq 0 ] && _notrun "xfs_db write can't support array"
> > +}
> > diff --git a/tests/xfs/444 b/tests/xfs/444
> > index 141be52..9700422 100755
> > --- a/tests/xfs/444
> > +++ b/tests/xfs/444
> > @@ -54,6 +54,7 @@ _supported_os Linux
> >  _require_check_dmesg
> >  _require_scratch
> >  _require_test_program "punch-alternating"
> > +_require_xfs_db_write_array
> 
> Looks ok,
> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

Thanks for the review!

Eryu

> 
> --D
> 
> >  
> >  # This is only a v5 filesystem problem
> >  _require_scratch_xfs_crc
> > -- 
> > 1.8.3.1
> > 
> > 
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe fstests" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] xfs/444: add check for xfs_db write bno array

[Xu, Yang]

On Fri, Apr 13, 2018 at 11:42:10PM +1000, Dave Chinner wrote:
> On Fri, Apr 13, 2018 at 12:36:12PM +0800, Eryu Guan wrote:
> OK, I got your point now, it's a bug in a diagnostic tool that is only used by XFS developers as a test infrastructure, so we can
> treat it as a infrastructure dependency as all other _require rules. I think that makes sense in this case. Thanks for 
> the explanation!

> yang xu, would you mind sending a v2 patch as Dave suggested? Thanks!

> Eryu
Hi Eryu
I will send a v2 patch as Dave suggested.

Thanks 
Yang Xu

> 
> Yes, it's ialso fixed in recent version of xfsprogs, but you know very 
> well that we test distros that have ancient xfsprogs and will never 
> have this issue fixed in them. We use detectiona nd notrun to avoid 
> tests they should not run all the time, and I don't see how this is 
> any different.
> 
> I really don't understand why you are pushing back on this - why 
> should this specific infrastructure deficiency cause test failures, 
> when all the existing infrastructure support checks cause tests to 
> notrun rather than fail?
> 
> Cheers,
> 
> Dave.
> --
> Dave Chinner
> david@fromorbit.com