[PATCH] shared/005,7: add check whether debugfs succeeds to set a negative i_size or not

[PATCH] shared/005,7: add check whether debugfs succeeds to set a negative i_size or not

[Xiao Yang]

shared/005 and shared/007 work abnornamlly on RHEL6.8GA and
RHEL6.9Beta because debugfs fails to set i_size to -1 or -512
and reports "exceeds field size maximum".  When debugfs fails
to set a negative i_size, we can skip these cases which don't
trigger the kernel bug.

Signed-off-by: Xiao Yang <yangx.jy@cn.fujitsu.com>
---
 tests/shared/005 | 7 +++++++
 tests/shared/007 | 7 +++++++
 2 files changed, 14 insertions(+)

diff --git a/tests/shared/005 b/tests/shared/005
index 296de17..37912d6 100755
--- a/tests/shared/005
+++ b/tests/shared/005
@@ -67,6 +67,13 @@ $DEBUGFS_PROG -w -R "sif /a size -1" $SCRATCH_DEV >> $seqres.full 2>&1
 
 echo "Remount, try to append"
 _scratch_mount
+
+#check whether debugfs succeeds to set i_size to -1 or not
+i_size=$(stat -c "%s" $testdir/a)
+if [ $i_size -ne -1 ]; then
+	_notrun "Could not set i_size to -1 successfully, skip test."
+fi
+
 dd if=/dev/zero of=$testdir/a bs=512 count=1 oflag=append conv=notrunc >> $seqres.full 2>&1 || echo "Write did not succeed (ok)."
 sync
 
diff --git a/tests/shared/007 b/tests/shared/007
index 0729ab8..06055a0 100755
--- a/tests/shared/007
+++ b/tests/shared/007
@@ -69,6 +69,13 @@ $DEBUGFS_PROG -w -R "sif /a size 0xFFFFFFFFFFFFFE00" $SCRATCH_DEV >> $seqres.ful
 
 echo "Remount, try to append"
 _scratch_mount
+
+#check whether debugfs succeeds to set i_size to -512 or not
+i_size=$(stat -c "%s" $testdir/a)
+if [ $i_size -ne -512 ]; then
+	_notrun "Could not set i_size to -512 successfully, skip test."
+fi
+
 dd if=/dev/zero of=$testdir/a bs=512 count=1 oflag=direct,append conv=notrunc >> $seqres.full 2>&1 || echo "Write did not succeed (ok)."
 sync
 
-- 
1.8.3.1

Re: [PATCH] shared/005,7: add check whether debugfs succeeds to set a negative i_size or not

[Xiao Yang]

Hi Eryu

Sorry, The previous patch is defective. Please see the following two points:

1) If the kernel bug has been fixed, stat command fails to get i_size and
reports "Structure needs cleaning".

2) shared/007 passed unexpectedly because of invalid argument when debugfs
fails to set i_size to the highest multiple of 512 below -1. So we need
to add strict check.

Thanks for you review. I will rewrite this patch.

Best Regards,
Xiao Yang

On 2017/02/13 19:21, Xiao Yang Wrote:
> shared/005 and shared/007 work abnornamlly on RHEL6.8GA and
> RHEL6.9Beta because debugfs fails to set i_size to -1 or -512
> and reports "exceeds field size maximum".  When debugfs fails
> to set a negative i_size, we can skip these cases which don't
> trigger the kernel bug.
>
> Signed-off-by: Xiao Yang <yangx.jy@cn.fujitsu.com>
> ---
>  tests/shared/005 | 7 +++++++
>  tests/shared/007 | 7 +++++++
>  2 files changed, 14 insertions(+)
>
> diff --git a/tests/shared/005 b/tests/shared/005
> index 296de17..37912d6 100755
> --- a/tests/shared/005
> +++ b/tests/shared/005
> @@ -67,6 +67,13 @@ $DEBUGFS_PROG -w -R "sif /a size -1" $SCRATCH_DEV >> $seqres.full 2>&1
>  
>  echo "Remount, try to append"
>  _scratch_mount
> +
> +#check whether debugfs succeeds to set i_size to -1 or not
> +i_size=$(stat -c "%s" $testdir/a)
> +if [ $i_size -ne -1 ]; then
> +	_notrun "Could not set i_size to -1 successfully, skip test."
> +fi
> +
>  dd if=/dev/zero of=$testdir/a bs=512 count=1 oflag=append conv=notrunc >> $seqres.full 2>&1 || echo "Write did not succeed (ok)."
>  sync
>  
> diff --git a/tests/shared/007 b/tests/shared/007
> index 0729ab8..06055a0 100755
> --- a/tests/shared/007
> +++ b/tests/shared/007
> @@ -69,6 +69,13 @@ $DEBUGFS_PROG -w -R "sif /a size 0xFFFFFFFFFFFFFE00" $SCRATCH_DEV >> $seqres.ful
>  
>  echo "Remount, try to append"
>  _scratch_mount
> +
> +#check whether debugfs succeeds to set i_size to -512 or not
> +i_size=$(stat -c "%s" $testdir/a)
> +if [ $i_size -ne -512 ]; then
> +	_notrun "Could not set i_size to -512 successfully, skip test."
> +fi
> +
>  dd if=/dev/zero of=$testdir/a bs=512 count=1 oflag=direct,append conv=notrunc >> $seqres.full 2>&1 || echo "Write did not succeed (ok)."
>  sync
>  

Re: [PATCH] shared/005,7: add check whether debugfs succeeds to set a negative i_size or not

[Eryu Guan]

On Mon, Feb 20, 2017 at 10:13:46AM +0800, Xiao Yang wrote:
> Hi Eryu
> 
> Sorry, The previous patch is defective. Please see the following two points:
> 
> 1) If the kernel bug has been fixed, stat command fails to get i_size and
> reports "Structure needs cleaning".
> 
> 2) shared/007 passed unexpectedly because of invalid argument when debugfs
> fails to set i_size to the highest multiple of 512 below -1. So we need
> to add strict check.

Thanks for the update! I've pushed your patch to upstream yesterday (not
sure why I didn't see the announce email in the list.. I'll resend
shortly, but the upstream repo should have been updated). Please send a
fix based on new master.

Thanks,
Eryu

[PATCH] shared/005,7: fix making sure debugfs sets negative i_size && add strict check for dd

[Xiao Yang]

1) If the kernel bug has been fixed, stat command fails to get i_size
   and reports "Structure needs cleaning".  So we use debugfs -R "stat"
   instead of stat command to make sure debugfs sets negative i_size.
   These cases have been broken by commit 0e13e40b247a1 ("shared/005,7:
   make sure debugfs sets negative i_size").

2) shared/007 passes unexpectedly on RHEL6.8GA and RHEL6.9Beta
   because of invalid argument. When debugfs fails to set i_size
   to -512, dd with direct flag fails and reports "invalid argument".
   So we can add strict check.

Signed-off-by: Xiao Yang <yangx.jy@cn.fujitsu.com>
---
 tests/shared/005 | 15 ++++++++-------
 tests/shared/007 | 15 ++++++++-------
 2 files changed, 16 insertions(+), 14 deletions(-)

diff --git a/tests/shared/005 b/tests/shared/005
index 76c551c..23de873 100755
--- a/tests/shared/005
+++ b/tests/shared/005
@@ -65,16 +65,17 @@ echo "Corrupt filesystem"
 _scratch_unmount
 $DEBUGFS_PROG -w -R "sif /a size -1" $SCRATCH_DEV >> $seqres.full 2>&1
 
-echo "Remount, try to append"
-_scratch_mount
-
 # check whether debugfs succeeds to set i_size to -1 or not
-i_size=$(stat -c "%s" $testdir/a)
-if [ $i_size -ne -1 ]; then
+$DEBUGFS_PROG -R "stat /a" $SCRATCH_DEV 2>&1 | grep -q "Size: 18446744073709551615" || \
 	_notrun "Could not set i_size to -1 successfully, skip test."
-fi
 
-dd if=/dev/zero of=$testdir/a bs=512 count=1 oflag=append conv=notrunc >> $seqres.full 2>&1 || echo "Write did not succeed (ok)."
+echo "Remount, try to append"
+_scratch_mount
+
+# dd may fail because of unexpected cause(eg. invalid argument), so we add strict check.
+dd_out=$(dd if=/dev/zero of=$testdir/a bs=512 count=1 oflag=append conv=notrunc 2>&1)
+echo $dd_out | grep -q "Structure needs cleaning" && echo "Write did not succeed (ok)."
+echo $dd_out >> $seqres.full
 sync
 
 # success, all done
diff --git a/tests/shared/007 b/tests/shared/007
index 77636f4..fc7e3ac 100755
--- a/tests/shared/007
+++ b/tests/shared/007
@@ -67,16 +67,17 @@ _scratch_unmount
 # -1 so that we can perform a dio write.
 $DEBUGFS_PROG -w -R "sif /a size 0xFFFFFFFFFFFFFE00" $SCRATCH_DEV >> $seqres.full 2>&1
 
-echo "Remount, try to append"
-_scratch_mount
-
 # check whether debugfs succeeds to set i_size to -512 or not
-i_size=$(stat -c "%s" $testdir/a)
-if [ $i_size -ne -512 ]; then
+$DEBUGFS_PROG -R "stat /a" $SCRATCH_DEV 2>&1 | grep -q "Size: 18446744073709551104" || \
 	_notrun "Could not set i_size to -512 successfully, skip test."
-fi
 
-dd if=/dev/zero of=$testdir/a bs=512 count=1 oflag=direct,append conv=notrunc >> $seqres.full 2>&1 || echo "Write did not succeed (ok)."
+echo "Remount, try to append"
+_scratch_mount
+
+# dd may fail because of unexpected cause (eg. invalid argument), so we add strict check.
+dd_out=$(dd if=/dev/zero of=$testdir/a bs=512 count=1 oflag=direct,append conv=notrunc 2>&1)
+echo $dd_out | grep -q "Structure needs cleaning" && echo "Write did not succeed (ok)."
+echo $dd_out >> $seqres.full
 sync
 
 # success, all done
-- 
1.8.3.1

Re: [PATCH] shared/005,7: fix making sure debugfs sets negative i_size && add strict check for dd

[Eryu Guan]

On Mon, Feb 20, 2017 at 01:02:59PM +0800, Xiao Yang wrote:
> 1) If the kernel bug has been fixed, stat command fails to get i_size
>    and reports "Structure needs cleaning".  So we use debugfs -R "stat"
>    instead of stat command to make sure debugfs sets negative i_size.
>    These cases have been broken by commit 0e13e40b247a1 ("shared/005,7:
>    make sure debugfs sets negative i_size").
> 
> 2) shared/007 passes unexpectedly on RHEL6.8GA and RHEL6.9Beta
>    because of invalid argument. When debugfs fails to set i_size
>    to -512, dd with direct flag fails and reports "invalid argument".
>    So we can add strict check.

But you've already added the check for i_size and _notrun if debugfs
failed to set i_size to -512, there's no chance for dd to hit EINVAL.
And I think any error dd is hitting is acceptable, as long as test runs
(which means i_size was set to -1/-512 successfully). So I don't think
we need the check on dd's output. Did I miss anything? 

Thanks,
Eryu

Re: [PATCH] shared/005,7: fix making sure debugfs sets negative i_size && add strict check for dd

[Xiao Yang]

On 2017/02/22 12:20, Eryu Guan wrote:
> On Mon, Feb 20, 2017 at 01:02:59PM +0800, Xiao Yang wrote:
>> 1) If the kernel bug has been fixed, stat command fails to get i_size
>>     and reports "Structure needs cleaning".  So we use debugfs -R "stat"
>>     instead of stat command to make sure debugfs sets negative i_size.
>>     These cases have been broken by commit 0e13e40b247a1 ("shared/005,7:
>>     make sure debugfs sets negative i_size").
>>
>> 2) shared/007 passes unexpectedly on RHEL6.8GA and RHEL6.9Beta
>>     because of invalid argument. When debugfs fails to set i_size
>>     to -512, dd with direct flag fails and reports "invalid argument".
>>     So we can add strict check.
> But you've already added the check for i_size and _notrun if debugfs
> failed to set i_size to -512, there's no chance for dd to hit EINVAL.
> And I think any error dd is hitting is acceptable, as long as test runs
> (which means i_size was set to -1/-512 successfully). So I don't think
> we need the check on dd's output. Did I miss anything?
Hi Eryu

Thanks for your comments.

xfs/134 passed unexpectedly on RHEL7.3GA because dd hits ENOSPC.  So I think
all of these cases should fail instead of pass when hitting  unexpected 
error.
Do you think whether we only check on dd's output of xfs/133,4 or 
remove  all? :-)

Best Regards,
XIao Yang
>
>
> Thanks,
> Eryu
>
>

Re: [PATCH] shared/005,7: fix making sure debugfs sets negative i_size && add strict check for dd

[Eryu Guan]

On Wed, Feb 22, 2017 at 12:56:54PM +0800, Xiao Yang wrote:
> On 2017/02/22 12:20, Eryu Guan wrote:
> > On Mon, Feb 20, 2017 at 01:02:59PM +0800, Xiao Yang wrote:
> > > 1) If the kernel bug has been fixed, stat command fails to get i_size
> > >     and reports "Structure needs cleaning".  So we use debugfs -R "stat"
> > >     instead of stat command to make sure debugfs sets negative i_size.
> > >     These cases have been broken by commit 0e13e40b247a1 ("shared/005,7:
> > >     make sure debugfs sets negative i_size").
> > > 
> > > 2) shared/007 passes unexpectedly on RHEL6.8GA and RHEL6.9Beta
> > >     because of invalid argument. When debugfs fails to set i_size
> > >     to -512, dd with direct flag fails and reports "invalid argument".
> > >     So we can add strict check.
> > But you've already added the check for i_size and _notrun if debugfs
> > failed to set i_size to -512, there's no chance for dd to hit EINVAL.
> > And I think any error dd is hitting is acceptable, as long as test runs
> > (which means i_size was set to -1/-512 successfully). So I don't think
> > we need the check on dd's output. Did I miss anything?
> Hi Eryu
> 
> Thanks for your comments.
> 
> xfs/134 passed unexpectedly on RHEL7.3GA because dd hits ENOSPC.  So I think

Hmm, xfs/134 hits ENOSPC even on 4.9 kernel (where xfs/133 triggers
kernel BUG_ON), so I think the buffer I/O tests are targeded regression
tests and the dio tests are nice-to-have tests.

> all of these cases should fail instead of pass when hitting  unexpected
> error.
> Do you think whether we only check on dd's output of xfs/133,4 or remove
> all? :-)

IMO, what we're going to test is to make sure kernel doesn't hang or
crash when dealing with negative i_size, any error is acceptable as long
as kernel could detect this corruption without crashing or hanging.

Thanks,
Eryu

[PATCH v2] shared/005,7: fix making sure debugfs sets negative i_size

[Xiao Yang]

If the kernel bug has been fixed, stat command fails to get i_size
and reports "Structure needs cleaning".  So we use debugfs -R "stat"
instead of stat command to make sure debugfs sets negative i_size.
These cases have been broken by commit 0e13e40b247a1 ("shared/005,7:
make sure debugfs sets negative i_size").

Signed-off-by: Xiao Yang <yangx.jy@cn.fujitsu.com>
---
 tests/shared/005 | 9 +++------
 tests/shared/007 | 9 +++------
 2 files changed, 6 insertions(+), 12 deletions(-)

diff --git a/tests/shared/005 b/tests/shared/005
index 76c551c..de7b84f 100755
--- a/tests/shared/005
+++ b/tests/shared/005
@@ -65,15 +65,12 @@ echo "Corrupt filesystem"
 _scratch_unmount
 $DEBUGFS_PROG -w -R "sif /a size -1" $SCRATCH_DEV >> $seqres.full 2>&1
 
-echo "Remount, try to append"
-_scratch_mount
-
 # check whether debugfs succeeds to set i_size to -1 or not
-i_size=$(stat -c "%s" $testdir/a)
-if [ $i_size -ne -1 ]; then
+$DEBUGFS_PROG -R "stat /a" $SCRATCH_DEV 2>&1 | grep -q "Size: 18446744073709551615" || \
 	_notrun "Could not set i_size to -1 successfully, skip test."
-fi
 
+echo "Remount, try to append"
+_scratch_mount
 dd if=/dev/zero of=$testdir/a bs=512 count=1 oflag=append conv=notrunc >> $seqres.full 2>&1 || echo "Write did not succeed (ok)."
 sync
 
diff --git a/tests/shared/007 b/tests/shared/007
index 77636f4..785f19f 100755
--- a/tests/shared/007
+++ b/tests/shared/007
@@ -67,15 +67,12 @@ _scratch_unmount
 # -1 so that we can perform a dio write.
 $DEBUGFS_PROG -w -R "sif /a size 0xFFFFFFFFFFFFFE00" $SCRATCH_DEV >> $seqres.full 2>&1
 
-echo "Remount, try to append"
-_scratch_mount
-
 # check whether debugfs succeeds to set i_size to -512 or not
-i_size=$(stat -c "%s" $testdir/a)
-if [ $i_size -ne -512 ]; then
+$DEBUGFS_PROG -R "stat /a" $SCRATCH_DEV 2>&1 | grep -q "Size: 18446744073709551104" || \
 	_notrun "Could not set i_size to -512 successfully, skip test."
-fi
 
+echo "Remount, try to append"
+_scratch_mount
 dd if=/dev/zero of=$testdir/a bs=512 count=1 oflag=direct,append conv=notrunc >> $seqres.full 2>&1 || echo "Write did not succeed (ok)."
 sync
 
-- 
1.8.3.1

[PATCH] xfs/133,4: make sure xfs_db sets negative i_size && add strict check for dd

[Xiao Yang]

1) xfs/133 and xfs/134 work abnornamlly on RHEL6.8GA and RHEL6.9Beta
   because xfs_db fails to set i_size to -1 or -512 and reports "usage:
   write fieldname value".  The special argument "--" is only used to
   end option-scanning in getopt().  So we should add "--" for write
   command in xfs_db when -c option is supported by write command.
   getopt() has been produced by 'commit c9f5e3db22098 ("xfs_db: Allow
   writes of corrupted data")'.

2) When xfs_db fails to set a negative i_size due to unknown error, we can
   skip these cases which don't trigger the kernel bug.

3) xfs/134 passes unexpectedly on RHEL7.3GA due to no space left on device,
   So we can add strict check.

Signed-off-by: Xiao Yang <yangx.jy@cn.fujitsu.com>
---
 tests/xfs/133 | 20 ++++++++++++++++++--
 tests/xfs/134 | 23 +++++++++++++++++++----
 2 files changed, 37 insertions(+), 6 deletions(-)

diff --git a/tests/xfs/133 b/tests/xfs/133
index d756d2e..0d7587e 100755
--- a/tests/xfs/133
+++ b/tests/xfs/133
@@ -63,11 +63,27 @@ inum=$(stat -c "%i" $testdir/a)
 
 echo "Corrupt filesystem"
 _scratch_unmount
-_scratch_xfs_db -x -c "inode ${inum}" -c 'write core.size -- -1' >> $seqres.full
+
+# 1) we should add "--" for write command in xfs_db when -c option is
+#    supported by write command.
+# 2) check whether xfs_db succeeds to set i_size to -1 or not
+_scratch_xfs_db -x -c "help" | grep -q "write \[-c\]"
+if [ $? -eq 0 ]; then
+	_scratch_xfs_db -x -c "inode ${inum}" -c 'write core.size -- -1' >> $seqres.full 2>&1 || \
+	_notrun "Could not set i_size to -1 successfully, skip test."
+else
+	_scratch_xfs_db -x -c "inode ${inum}" -c 'write core.size -1' >> $seqres.full 2>&1 || \
+	_notrun "Could not set i_size to -1 successfully, skip test."
+fi
 
 echo "Remount, try to append"
 _scratch_mount
-dd if=/dev/zero of=$testdir/a bs=512 count=1 oflag=append conv=notrunc >> $seqres.full 2>&1 || echo "Write did not succeed (ok)."
+
+# dd may fail unexpectedly(eg. no space left on device), So we add strict check.
+dd_out=$(dd if=/dev/zero of=$testdir/a bs=512 count=1 oflag=append conv=notrunc 2>&1)
+echo $dd_out | grep -q "Structure needs cleaning" && echo "Write did not succeed (ok)."
+echo $dd_out >> $seqres.full
+
 sync
 
 # success, all done
diff --git a/tests/xfs/134 b/tests/xfs/134
index 09466cd..d6d2dec 100755
--- a/tests/xfs/134
+++ b/tests/xfs/134
@@ -63,13 +63,28 @@ inum=$(stat -c "%i" $testdir/a)
 
 echo "Corrupt filesystem"
 _scratch_unmount
-# Set the file size to the highest multiple of 512 below
-# -1 so that we can perform a dio write.
-_scratch_xfs_db -x -c "inode ${inum}" -c 'write core.size -- -512' >> $seqres.full
+
+# 1) Set the file size to the highest multiple of 512 below -1 so
+#    that we can perform a dio write.
+# 2) we should add "--" for write command in xfs_db when -c option is
+#    supported by write command.
+# 3) check whether xfs_db succeeds to set i_size to -512 or not
+_scratch_xfs_db -x -c "help" | grep -q "write \[-c\]"
+if [ $? -eq 0 ]; then
+	_scratch_xfs_db -x -c "inode ${inum}" -c 'write core.size -- -512' >> $seqres.full 2>&1 || \
+	_notrun "Could not set i_size to -512 successfully, skip test."
+else
+	_scratch_xfs_db -x -c "inode ${inum}" -c 'write core.size -512' >> $seqres.full 2>&1 || \
+	_notrun "Could not set i_size to -512 successfully, skip test."
+fi
 
 echo "Remount, try to append"
 _scratch_mount
-dd if=/dev/zero of=$testdir/a bs=512 count=1 oflag=direct,append conv=notrunc >> $seqres.full 2>&1 || echo "Write did not succeed (ok)."
+
+# dd may fail unexpectedly(eg. no space left on device), So we add strict check.
+dd_out=$(dd if=/dev/zero of=$testdir/a bs=512 count=1 oflag=direct,append conv=notrunc 2>&1)
+echo $dd_out | grep -q "Structure needs cleaning" && echo "Write did not succeed (ok)."
+echo $dd_out >> $seqres.full
 sync
 
 # success, all done
-- 
1.8.3.1

[PATCH v2] xfs/133,4: make sure xfs_db sets negative i_size

[Xiao Yang]

1) xfs/133 and xfs/134 work abnornamlly on RHEL6.8GA and RHEL6.9Beta
   because xfs_db fails to set i_size to -1 or -512 and reports "usage:
   write fieldname value".  The special argument "--" is only used to
   end option-scanning in getopt().  So we should add "--" for write
   command in xfs_db when -c option is supported by write command.
   getopt() has been produced by 'commit c9f5e3db22098 ("xfs_db: Allow
   writes of corrupted data")'.

2) When xfs_db fails to set a negative i_size due to unknown error, we
   can skip these cases which don't trigger the kernel bug.

Signed-off-by: Xiao Yang <yangx.jy@cn.fujitsu.com>
---
 tests/xfs/133 | 13 ++++++++++++-
 tests/xfs/134 | 17 ++++++++++++++---
 2 files changed, 26 insertions(+), 4 deletions(-)

diff --git a/tests/xfs/133 b/tests/xfs/133
index d756d2e..91b3b9b 100755
--- a/tests/xfs/133
+++ b/tests/xfs/133
@@ -63,7 +63,18 @@ inum=$(stat -c "%i" $testdir/a)
 
 echo "Corrupt filesystem"
 _scratch_unmount
-_scratch_xfs_db -x -c "inode ${inum}" -c 'write core.size -- -1' >> $seqres.full
+
+# 1) we should add "--" for write command in xfs_db when -c option is
+#    supported by write command.
+# 2) check whether xfs_db succeeds to set i_size to -1 or not
+_scratch_xfs_db -x -c "help" | grep -q "write \[-c\]"
+if [ $? -eq 0 ]; then
+	_scratch_xfs_db -x -c "inode ${inum}" -c 'write core.size -- -1' >> $seqres.full 2>&1 || \
+	_notrun "Could not set i_size to -1 successfully, skip test."
+else
+	_scratch_xfs_db -x -c "inode ${inum}" -c 'write core.size -1' >> $seqres.full 2>&1 || \
+	_notrun "Could not set i_size to -1 successfully, skip test."
+fi
 
 echo "Remount, try to append"
 _scratch_mount
diff --git a/tests/xfs/134 b/tests/xfs/134
index 09466cd..788f8e9 100755
--- a/tests/xfs/134
+++ b/tests/xfs/134
@@ -63,9 +63,20 @@ inum=$(stat -c "%i" $testdir/a)
 
 echo "Corrupt filesystem"
 _scratch_unmount
-# Set the file size to the highest multiple of 512 below
-# -1 so that we can perform a dio write.
-_scratch_xfs_db -x -c "inode ${inum}" -c 'write core.size -- -512' >> $seqres.full
+
+# 1) Set the file size to the highest multiple of 512 below -1 so
+#    that we can perform a dio write.
+# 2) we should add "--" for write command in xfs_db when -c option is
+#    supported by write command.
+# 3) check whether xfs_db succeeds to set i_size to -512 or not
+_scratch_xfs_db -x -c "help" | grep -q "write \[-c\]"
+if [ $? -eq 0 ]; then
+	_scratch_xfs_db -x -c "inode ${inum}" -c 'write core.size -- -512' >> $seqres.full 2>&1 || \
+	_notrun "Could not set i_size to -512 successfully, skip test."
+else
+	_scratch_xfs_db -x -c "inode ${inum}" -c 'write core.size -512' >> $seqres.full 2>&1 || \
+	_notrun "Could not set i_size to -512 successfully, skip test."
+fi
 
 echo "Remount, try to append"
 _scratch_mount
-- 
1.8.3.1

Re: [PATCH v2] xfs/133,4: make sure xfs_db sets negative i_size

[Eryu Guan]

On Wed, Feb 22, 2017 at 05:31:40PM +0800, Xiao Yang wrote:
> 1) xfs/133 and xfs/134 work abnornamlly on RHEL6.8GA and RHEL6.9Beta
>    because xfs_db fails to set i_size to -1 or -512 and reports "usage:
>    write fieldname value".  The special argument "--" is only used to
>    end option-scanning in getopt().  So we should add "--" for write
>    command in xfs_db when -c option is supported by write command.
>    getopt() has been produced by 'commit c9f5e3db22098 ("xfs_db: Allow
>    writes of corrupted data")'.
> 
> 2) When xfs_db fails to set a negative i_size due to unknown error, we
>    can skip these cases which don't trigger the kernel bug.
> 
> Signed-off-by: Xiao Yang <yangx.jy@cn.fujitsu.com>

Sorry for the late review on this patch. I just got some time to look at
xfs/133,4 a bit closely.

It turns out that the EINVAL error you saw is because of

echo m > $testdir/a

and direct append failed because the write offset is not 512-aligned.

Just "touch $testdir/a" should work (you can see test failure on RHEL6.8
with current master).

> ---
>  tests/xfs/133 | 13 ++++++++++++-
>  tests/xfs/134 | 17 ++++++++++++++---
>  2 files changed, 26 insertions(+), 4 deletions(-)
> 
> diff --git a/tests/xfs/133 b/tests/xfs/133
> index d756d2e..91b3b9b 100755
> --- a/tests/xfs/133
> +++ b/tests/xfs/133
> @@ -63,7 +63,18 @@ inum=$(stat -c "%i" $testdir/a)
>  
>  echo "Corrupt filesystem"
>  _scratch_unmount
> -_scratch_xfs_db -x -c "inode ${inum}" -c 'write core.size -- -1' >> $seqres.full
> +
> +# 1) we should add "--" for write command in xfs_db when -c option is
> +#    supported by write command.
> +# 2) check whether xfs_db succeeds to set i_size to -1 or not
> +_scratch_xfs_db -x -c "help" | grep -q "write \[-c\]"
> +if [ $? -eq 0 ]; then
> +	_scratch_xfs_db -x -c "inode ${inum}" -c 'write core.size -- -1' >> $seqres.full 2>&1 || \
> +	_notrun "Could not set i_size to -1 successfully, skip test."
> +else
> +	_scratch_xfs_db -x -c "inode ${inum}" -c 'write core.size -1' >> $seqres.full 2>&1 || \
> +	_notrun "Could not set i_size to -1 successfully, skip test."
> +fi

This check looks tedious, and xfs_db doesn't return non-zero on error,
so this will never _notrun.

I'd suggest running the two xfs_db commands unconditionally (with clear
comments), there has to be one suitable call for the xfs_db version in
use, and check core.size is -1/-512 by printing it with xfs_db. e.g.

# <comments about these two calls>
_scratch_xfs_db -x -c "inode ${inum}" -c "write core.size -1" >>$seqres.full 2>&1
_scratch_xfs_db -x -c "inode ${inum}" -c "write core.size -- -1" >>$seqres.full 2>&1

# check core.size and _notrun if it's not set correctly
isize=`_scratch_xfs_db -c "inode ${inum}" -c "print core.size" | $AWK_PROG '{print $3}'`
if [ $isize -ne -1 ]; then
	_notrun "..."
fi

Thanks,
Eryu

Re: [PATCH v2] xfs/133,4: make sure xfs_db sets negative i_size

[Xiao Yang]

On 2017/02/22 18:49, Eryu Guan wrote:
> On Wed, Feb 22, 2017 at 05:31:40PM +0800, Xiao Yang wrote:
>> 1) xfs/133 and xfs/134 work abnornamlly on RHEL6.8GA and RHEL6.9Beta
>>     because xfs_db fails to set i_size to -1 or -512 and reports "usage:
>>     write fieldname value".  The special argument "--" is only used to
>>     end option-scanning in getopt().  So we should add "--" for write
>>     command in xfs_db when -c option is supported by write command.
>>     getopt() has been produced by 'commit c9f5e3db22098 ("xfs_db: Allow
>>     writes of corrupted data")'.
>>
>> 2) When xfs_db fails to set a negative i_size due to unknown error, we
>>     can skip these cases which don't trigger the kernel bug.
>>
>> Signed-off-by: Xiao Yang<yangx.jy@cn.fujitsu.com>
> Sorry for the late review on this patch. I just got some time to look at
> xfs/133,4 a bit closely.
>
> It turns out that the EINVAL error you saw is because of
>
> echo m>  $testdir/a
>
> and direct append failed because the write offset is not 512-aligned.
>
> Just "touch $testdir/a" should work (you can see test failure on RHEL6.8
> with current master).
>
Hi Eryu

I saw test failure on RHEL6.8 occasionally when using touch command to 
create file. :-)
I will rewrite this patch as you suggestions.

Best Regards,
Xiao Yang
>> ---
>>   tests/xfs/133 | 13 ++++++++++++-
>>   tests/xfs/134 | 17 ++++++++++++++---
>>   2 files changed, 26 insertions(+), 4 deletions(-)
>>
>> diff --git a/tests/xfs/133 b/tests/xfs/133
>> index d756d2e..91b3b9b 100755
>> --- a/tests/xfs/133
>> +++ b/tests/xfs/133
>> @@ -63,7 +63,18 @@ inum=$(stat -c "%i" $testdir/a)
>>
>>   echo "Corrupt filesystem"
>>   _scratch_unmount
>> -_scratch_xfs_db -x -c "inode ${inum}" -c 'write core.size -- -1'>>  $seqres.full
>> +
>> +# 1) we should add "--" for write command in xfs_db when -c option is
>> +#    supported by write command.
>> +# 2) check whether xfs_db succeeds to set i_size to -1 or not
>> +_scratch_xfs_db -x -c "help" | grep -q "write \[-c\]"
>> +if [ $? -eq 0 ]; then
>> +	_scratch_xfs_db -x -c "inode ${inum}" -c 'write core.size -- -1'>>  $seqres.full 2>&1 || \
>> +	_notrun "Could not set i_size to -1 successfully, skip test."
>> +else
>> +	_scratch_xfs_db -x -c "inode ${inum}" -c 'write core.size -1'>>  $seqres.full 2>&1 || \
>> +	_notrun "Could not set i_size to -1 successfully, skip test."
>> +fi
> This check looks tedious, and xfs_db doesn't return non-zero on error,
> so this will never _notrun.
>
> I'd suggest running the two xfs_db commands unconditionally (with clear
> comments), there has to be one suitable call for the xfs_db version in
> use, and check core.size is -1/-512 by printing it with xfs_db. e.g.
>
> #<comments about these two calls>
> _scratch_xfs_db -x -c "inode ${inum}" -c "write core.size -1">>$seqres.full 2>&1
> _scratch_xfs_db -x -c "inode ${inum}" -c "write core.size -- -1">>$seqres.full 2>&1
>
> # check core.size and _notrun if it's not set correctly
> isize=`_scratch_xfs_db -c "inode ${inum}" -c "print core.size" | $AWK_PROG '{print $3}'`
> if [ $isize -ne -1 ]; then
> 	_notrun "..."
> fi
> Thanks,
> Eryu
> --
> 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
>
>
> .
>

[PATCH v3] xfs/133,4: make sure xfs_db sets negative i_size

[Xiao Yang]

1) xfs/133 and xfs/134 work abnornamlly on RHEL6.8GA and RHEL6.9Beta
   because xfs_db fails to set negative i_size and reports "usage:
   write fieldname value".  The special argument "--" is only used to
   end option-scanning in getopt().  So we can run two xfs_db commands
   to set negative i_size regardless of the special argument "--" is
   needed or not.  getopt() has been produced by 'commit c9f5e3db22098
   ("xfs_db: Allow writes of corrupted data")'.

2) xfs/134 passes unexpectedly on RHEL6.8GA due to EINVAL, so we use
   touch command to create 512-aligned test file.

Signed-off-by: Xiao Yang <yangx.jy@cn.fujitsu.com>
---
 tests/xfs/133 | 12 +++++++++++-
 tests/xfs/134 | 19 +++++++++++++++----
 2 files changed, 26 insertions(+), 5 deletions(-)

diff --git a/tests/xfs/133 b/tests/xfs/133
index d756d2e..ff73a41 100755
--- a/tests/xfs/133
+++ b/tests/xfs/133
@@ -63,7 +63,17 @@ inum=$(stat -c "%i" $testdir/a)
 
 echo "Corrupt filesystem"
 _scratch_unmount
-_scratch_xfs_db -x -c "inode ${inum}" -c 'write core.size -- -1' >> $seqres.full
+
+# run two xfs_db commands to set core.size regardless of the special argument "--"
+# is needed or not.
+_scratch_xfs_db -x -c "inode ${inum}" -c 'write core.size -- -1' >> $seqres.full 2>&1
+_scratch_xfs_db -x -c "inode ${inum}" -c 'write core.size -1' >> $seqres.full 2>&1
+
+# check core.size and _notrun if it's not set correctly
+i_size=`_scratch_xfs_db -c "inode ${inum}" -c "print core.size" | $AWK_PROG '{print $3}'`
+if [ $i_size -ne -1 ]; then
+	_notrun "Could not set i_size to -1 successfully, skip test."
+fi
 
 echo "Remount, try to append"
 _scratch_mount
diff --git a/tests/xfs/134 b/tests/xfs/134
index 09466cd..f064476 100755
--- a/tests/xfs/134
+++ b/tests/xfs/134
@@ -58,14 +58,25 @@ _scratch_mkfs  >> $seqres.full 2>&1
 _scratch_mount
 
 testdir=$SCRATCH_MNT
-echo m > $testdir/a
+# make sure the write offset is 512-aligned
+touch $testdir/a
 inum=$(stat -c "%i" $testdir/a)
 
 echo "Corrupt filesystem"
 _scratch_unmount
-# Set the file size to the highest multiple of 512 below
-# -1 so that we can perform a dio write.
-_scratch_xfs_db -x -c "inode ${inum}" -c 'write core.size -- -512' >> $seqres.full
+
+# 1) Set the file size to the highest multiple of 512 below -1 so
+#    that we can perform a dio write.
+# 2) run two xfs_db commands to set core.size regardless of the special
+#    argument "--" is needed or not.
+_scratch_xfs_db -x -c "inode ${inum}" -c 'write core.size -- -512' >> $seqres.full 2>&1
+_scratch_xfs_db -x -c "inode ${inum}" -c 'write core.size -512' >> $seqres.full 2>&1
+
+# check core.size and _notrun if it's not set correctly
+i_size=`_scratch_xfs_db -c "inode ${inum}" -c "print core.size" | $AWK_PROG '{print $3}'`
+if [ $i_size -ne -512 ]; then
+	_notrun "Could not set i_size to -512 successfully, skip test."
+fi
 
 echo "Remount, try to append"
 _scratch_mount
-- 
1.8.3.1