[PATCH 0/4] misc. fstests changes

[PATCH 0/4] misc. fstests changes

[Darrick J. Wong]

Hi all,

Here's a pile of tests to go with some of the stuff merged in 4.1[56].  We
start by removing the -y parameter from xfs_scrub since it will be removed in
the final xfsprogs 4.15 release.  Then we remove the automatic xfs_check run
from xfstests (individual tests can still request one) since it's been long
obsolete.  Next are three regression tests to check xfs' handling of xfs
quotas and free space fragmentation problems that were fixed in 4.16.
Finally, we update xfs/348 to reflect improved metadata verification of
corrupt symlinks.

--D

[PATCH 1/4] xfs_scrub: remove -y parameter

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Remove the -y parameter from scrub runs since we're removing that
option from xfs_scrub.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 common/fuzzy  |    2 +-
 tests/xfs/286 |    4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)


diff --git a/common/fuzzy b/common/fuzzy
index 8c5417a..f78fc17 100644
--- a/common/fuzzy
+++ b/common/fuzzy
@@ -217,7 +217,7 @@ __scratch_xfs_fuzz_field_test() {
 		# Try fixing the filesystem online?!
 		if [ "${repair}" = "online" ] || [ "${repair}" = "both" ]; then
 			__fuzz_notify "++ Try to repair filesystem online"
-			_scratch_scrub -y 2>&1
+			_scratch_scrub 2>&1
 			res=$?
 			test $res -ne 0 && \
 				(>&2 echo "online repair failed ($res) with ${field} = ${fuzzverb}.")
diff --git a/tests/xfs/286 b/tests/xfs/286
index 67128b2..e71b933 100755
--- a/tests/xfs/286
+++ b/tests/xfs/286
@@ -1,7 +1,7 @@
 #! /bin/bash
 # FS QA Test No. 286
 #
-# Race fio and xfs_scrub -y for a while to see if we crash or livelock.
+# Race fio and xfs_scrub for a while to see if we crash or livelock.
 #
 #-----------------------------------------------------------------------
 # Copyright (c) 2017 Oracle, Inc.  All Rights Reserved.
@@ -75,7 +75,7 @@ end=$((start + (60 * TIME_FACTOR) ))
 killstress &
 echo "Repair started at $(date --date="@${start}"), ending at $(date --date="@${end}")" >> $seqres.full
 while [ "$(date +%s)" -lt "$end" ]; do
-	XFS_SCRUB_FORCE_REPAIR=1 $TIMEOUT_PROG -s TERM $(( end - $(date +%s) + 2 )) $XFS_SCRUB_PROG -d -T -v -y $SCRATCH_MNT >> $seqres.full
+	XFS_SCRUB_FORCE_REPAIR=1 $TIMEOUT_PROG -s TERM $(( end - $(date +%s) + 2 )) $XFS_SCRUB_PROG -d -T -v $SCRATCH_MNT >> $seqres.full
 done
 
 echo "Test done"

[PATCH 2/4] xfs: skip xfs_check in _check_xfs_filesystem

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

xfs_check has been long obsolete, so stop running it automatically
after every test.  Tests that explicitly want xfs_check can call it
via _scratch_xfs_check or _xfs_check; that part doesn't go away.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 common/xfs |   17 -----------------
 1 file changed, 17 deletions(-)


diff --git a/common/xfs b/common/xfs
index 3dba40d..c63e5dc 100644
--- a/common/xfs
+++ b/common/xfs
@@ -386,23 +386,6 @@ _check_xfs_filesystem()
 		ok=0
 	fi
 
-	# xfs_check runs out of memory on large files, so even providing the test
-	# option (-t) to avoid indexing the free space trees doesn't make it pass on
-	# large filesystems. Avoid it.
-	if [ "$LARGE_SCRATCH_DEV" != yes ]; then
-		_xfs_check $extra_log_options $device 2>&1 |\
-			_fix_malloc >$tmp.fs_check
-	fi
-	if [ -s $tmp.fs_check ]; then
-		_log_err "_check_xfs_filesystem: filesystem on $device is inconsistent (c)"
-		echo "*** xfs_check output ***"		>>$seqres.full
-		cat $tmp.fs_check			>>$seqres.full
-		echo "*** end xfs_check output"		>>$seqres.full
-
-		xfs_metadump $device $seqres.check	>>$seqres.full 2>&1
-		ok=0
-	fi
-
 	$XFS_REPAIR_PROG -n $extra_options $extra_log_options $extra_rt_options $device >$tmp.repair 2>&1
 	if [ $? -ne 0 ]; then
 		_log_err "_check_xfs_filesystem: filesystem on $device is inconsistent (r)"

[PATCH 3/4] xfs: regression tests for reflink quota bugs

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Add three tests to look for quota bugs in xfs reflink.  The first test
looks for problems when we have speculative cow reservations in memory,
we chown the file, but the reservations don't move to the new owner.
The second test checks that we remembered to dqattach the inodes before
performing reflink operations.  The third is a stress test for reflink
quota handling near enospc and helped us to find a directio cow write
corruption bug when free space is fragmented.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 tests/xfs/904     |   93 +++++++++++++++++++++++++++++++++++++++++
 tests/xfs/904.out |   17 ++++++++
 tests/xfs/905     |   88 +++++++++++++++++++++++++++++++++++++++
 tests/xfs/905.out |   13 ++++++
 tests/xfs/906     |  119 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 tests/xfs/906.out |    6 +++
 tests/xfs/group   |    3 +
 7 files changed, 339 insertions(+)
 create mode 100755 tests/xfs/904
 create mode 100644 tests/xfs/904.out
 create mode 100755 tests/xfs/905
 create mode 100644 tests/xfs/905.out
 create mode 100755 tests/xfs/906
 create mode 100644 tests/xfs/906.out


diff --git a/tests/xfs/904 b/tests/xfs/904
new file mode 100755
index 0000000..ef78f1a
--- /dev/null
+++ b/tests/xfs/904
@@ -0,0 +1,93 @@
+#! /bin/bash
+# FS QA Test No. 904
+#
+# Regression test for a quota accounting bug when changing the owner of
+# a file that has CoW reservations and no dirty pages.  The reservations
+# should shift over to the new owner, but they do not.
+#
+#-----------------------------------------------------------------------
+# Copyright (c) 2018 Oracle, Inc.  All Rights Reserved.
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License as
+# published by the Free Software Foundation.
+#
+# This program is distributed in the hope that it would be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write the Free Software Foundation,
+# Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+#-----------------------------------------------------------------------
+#
+
+seq=`basename $0`
+seqres=$RESULT_DIR/$seq
+echo "QA output created by $seq"
+
+here=`pwd`
+tmp=/tmp/$$
+status=1	# failure is the default!
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+_cleanup()
+{
+	cd /
+	rm -f $tmp.*
+}
+
+# get standard environment, filters and checks
+. ./common/rc
+. ./common/reflink
+. ./common/quota
+. ./common/filter
+
+# Modify as appropriate.
+_supported_fs generic
+_supported_os Linux
+
+_require_quota
+_require_scratch_reflink
+_require_cp_reflink
+_require_user
+
+rm -f $seqres.full
+
+echo "Format and mount"
+_scratch_mkfs > "$seqres.full" 2>&1
+_scratch_mount "-o usrquota,grpquota" >> "$seqres.full" 2>&1
+
+echo "Create files"
+$XFS_IO_PROG -c "cowextsize 1m" $SCRATCH_MNT
+touch $SCRATCH_MNT/a $SCRATCH_MNT/force_fsgqa
+chown $qa_user $SCRATCH_MNT/a $SCRATCH_MNT/force_fsgqa
+_pwrite_byte 0x58 0 64k $SCRATCH_MNT/a >> $seqres.full
+$XFS_IO_PROG -c 'stat -r' $SCRATCH_MNT/a | grep stat.size >> $seqres.full
+_report_quota_blocks "-u $SCRATCH_MNT"
+
+echo "Reflink and CoW"
+_cp_reflink $SCRATCH_MNT/a $SCRATCH_MNT/b
+_pwrite_byte 0x59 0 64k $SCRATCH_MNT/a >> $seqres.full
+$XFS_IO_PROG -c 'stat -r' $SCRATCH_MNT/a | grep stat.size >> $seqres.full
+_report_quota_blocks "-u $SCRATCH_MNT"
+
+echo "Sync"
+sync
+_report_quota_blocks "-u $SCRATCH_MNT"
+
+echo "Chown and check quota"
+chown root $SCRATCH_MNT/a
+$XFS_IO_PROG -c 'stat -r' $SCRATCH_MNT/a | grep stat.size >> $seqres.full
+_report_quota_blocks "-u $SCRATCH_MNT"
+
+echo "Remount"
+_scratch_unmount
+_scratch_mount "-o usrquota,grpquota" >> "$seqres.full" 2>&1
+$XFS_IO_PROG -c 'stat -r' $SCRATCH_MNT/a | grep stat.size >> $seqres.full
+_report_quota_blocks "-u $SCRATCH_MNT"
+
+# success, all done
+status=0
+exit
diff --git a/tests/xfs/904.out b/tests/xfs/904.out
new file mode 100644
index 0000000..4c2f508
--- /dev/null
+++ b/tests/xfs/904.out
@@ -0,0 +1,17 @@
+QA output created by 904
+Format and mount
+Create files
+root 0 0 0
+fsgqa 64 0 0
+Reflink and CoW
+root 0 0 0
+fsgqa 1152 0 0
+Sync
+root 0 0 0
+fsgqa 1088 0 0
+Chown and check quota
+root 1024 0 0
+fsgqa 64 0 0
+Remount
+root 64 0 0
+fsgqa 64 0 0
diff --git a/tests/xfs/905 b/tests/xfs/905
new file mode 100755
index 0000000..a457c84
--- /dev/null
+++ b/tests/xfs/905
@@ -0,0 +1,88 @@
+#! /bin/bash
+# FS QA Test No. 905
+#
+# Regression test for a quota accounting bug when reflinking across EOF
+# of a file in which we forgot dq_attach.
+#
+#-----------------------------------------------------------------------
+# Copyright (c) 2018 Oracle, Inc.  All Rights Reserved.
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License as
+# published by the Free Software Foundation.
+#
+# This program is distributed in the hope that it would be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write the Free Software Foundation,
+# Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+#-----------------------------------------------------------------------
+#
+
+seq=`basename $0`
+seqres=$RESULT_DIR/$seq
+echo "QA output created by $seq"
+
+here=`pwd`
+tmp=/tmp/$$
+status=1	# failure is the default!
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+_cleanup()
+{
+	cd /
+	rm -f $tmp.*
+}
+
+# get standard environment, filters and checks
+. ./common/rc
+. ./common/reflink
+. ./common/quota
+. ./common/filter
+
+# Modify as appropriate.
+_supported_fs generic
+_supported_os Linux
+
+_require_quota
+_require_scratch_reflink
+_require_cp_reflink
+_require_user
+
+rm -f $seqres.full
+
+check_quota() {
+	du_total="$(du -ksc $SCRATCH_MNT/a $SCRATCH_MNT/b | tail -n 1 | awk '{print $1}')"
+	qu_total="$(_report_quota_blocks "-u $SCRATCH_MNT" | grep $qa_user | awk '{print $2}')"
+	echo "du: $du_total; quota: $qu_total"
+}
+
+echo "Format and mount (noquota)"
+_scratch_mkfs > "$seqres.full" 2>&1
+_scratch_mount "-o noquota" >> $seqres.full 2>&1
+
+echo "Create files"
+_pwrite_byte 0x58 0 1m $SCRATCH_MNT/a >> $seqres.full
+_pwrite_byte 0x58 0 1m $SCRATCH_MNT/b >> $seqres.full
+chown $qa_user $SCRATCH_MNT/a $SCRATCH_MNT/b
+check_quota 2>&1 | _filter_scratch
+
+echo "Mount (usrquota) and check quota"
+_scratch_unmount
+_scratch_mount "-o usrquota,grpquota" >> "$seqres.full" 2>&1
+check_quota
+
+echo "Reflink and check quota again"
+_reflink_range $SCRATCH_MNT/a 256k $SCRATCH_MNT/b 384k 128k >> $seqres.full
+_reflink_range $SCRATCH_MNT/a 256k $SCRATCH_MNT/b 960k 128k >> $seqres.full
+check_quota
+
+echo "Force quotacheck"
+_check_quota_usage
+
+# success, all done
+status=0
+exit
diff --git a/tests/xfs/905.out b/tests/xfs/905.out
new file mode 100644
index 0000000..fa95624
--- /dev/null
+++ b/tests/xfs/905.out
@@ -0,0 +1,13 @@
+QA output created by 905
+Format and mount (noquota)
+Create files
+repquota: Mountpoint (or device) SCRATCH_MNT not found or has no quota enabled.
+repquota: Not all specified mountpoints are using quota.
+du: 2048; quota: 
+Mount (usrquota) and check quota
+du: 2048; quota: 2048
+Reflink and check quota again
+du: 2112; quota: 2112
+Force quotacheck
+Comparing user usage
+Comparing group usage
diff --git a/tests/xfs/906 b/tests/xfs/906
new file mode 100755
index 0000000..b2e715d
--- /dev/null
+++ b/tests/xfs/906
@@ -0,0 +1,119 @@
+#! /bin/bash
+# FS QA Test No. 906
+#
+# Force enable all XFS quotas, run fsstress until the fs runs out of
+# space, and make sure the quotas are still correct when we're done.
+# This is a general regression/stress test for numerous quota bugs with
+# reflink and copy on write.
+#
+#-----------------------------------------------------------------------
+# Copyright (c) 2018 Oracle, Inc.  All Rights Reserved.
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License as
+# published by the Free Software Foundation.
+#
+# This program is distributed in the hope that it would be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write the Free Software Foundation,
+# Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+#-----------------------------------------------------------------------
+#
+
+seq=`basename $0`
+seqres=$RESULT_DIR/$seq
+echo "QA output created by $seq"
+
+here=`pwd`
+tmp=/tmp/$$
+status=1	# failure is the default!
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+_cleanup()
+{
+	cd /
+	rm -f $tmp.*
+	$KILLALL_PROG -9 fsstress > /dev/null 2>&1
+}
+
+# get standard environment, filters and checks
+. ./common/rc
+. ./common/quota
+. ./common/filter
+. ./common/reflink
+
+# Modify as appropriate.
+_supported_fs generic
+_supported_os Linux
+
+_require_scratch_reflink
+_require_quota
+_require_command "$KILLALL_PROG" "killall"
+
+rm -f $seqres.full
+
+report_quota_blocks() {
+	$XFS_QUOTA_PROG -x -c "report $1" $SCRATCH_MNT | \
+			awk '{x += $2;} END { print(x); }'
+}
+
+compare_quota_to_du() {
+	test $1 -eq $2 || echo "$3 quota $2 blocks does not match du $1 blocks?"
+}
+
+# Make sure the user/group/project quota block counts match the du output.
+# This ensures that we did the quota accounting correctly and that we're
+# accurately reporting cow preallocation blocks in stat.
+check_quota_du_blocks() {
+	sync
+	#$XFS_QUOTA_PROG -x -c 'report' $SCRATCH_MNT >> $seqres.full
+	du_rep=$(du -ks $SCRATCH_MNT | awk '{print $1}')
+	u_rep=$(report_quota_blocks -u)
+	g_rep=$(report_quota_blocks -g)
+	p_rep=$(report_quota_blocks -p)
+
+	compare_quota_to_du $du_rep $u_rep "user"
+	compare_quota_to_du $du_rep $g_rep "group"
+	compare_quota_to_du $du_rep $p_rep "project"
+}
+
+echo "Format and fsstress"
+
+_qmount_option "usrquota,grpquota,prjquota"
+# We use a small volume so that we hit ENOSPC.  This is critical for
+# regression testing a bug in the directio write code that could result in fs
+# corruption ("xfs: check reflink allocation mappings").
+#
+# This started as a test for quota accounting problems ("xfs: treat CoW fork
+# operations as delalloc for quota accounting") and ("xfs: call
+# xfs_qm_dqattach before performing reflink operations") though each of those
+# tests now have separate faster-running regression tests.
+_scratch_mkfs_sized $((1600 * 1048576)) > $seqres.full 2>&1
+_scratch_mount >> $seqres.full 2>&1 || _fail "mount failed..."
+
+nr_cpus=$((LOAD_FACTOR * 4))
+nr_ops=$((25000 * nr_cpus * TIME_FACTOR))
+$FSSTRESS_PROG $FSSTRESS_AVOID -w -d $SCRATCH_MNT -n $nr_ops -p $nr_cpus >> $seqres.full
+
+echo "Check quota before remount"
+check_quota_du_blocks
+
+# Clear out all the preallocations before we quotacheck.
+# The count comparison in _check_quota_usage will be unhappy if we don't
+# manage to clean out all the cow preallocations before the remount.
+_scratch_unmount
+_scratch_mount
+
+# Make sure the usage doesn't change after quotacheck.
+echo "Check quota after remount"
+_check_quota_usage
+
+check_quota_du_blocks
+
+# success, all done
+status=0
+exit
diff --git a/tests/xfs/906.out b/tests/xfs/906.out
new file mode 100644
index 0000000..201daf3
--- /dev/null
+++ b/tests/xfs/906.out
@@ -0,0 +1,6 @@
+QA output created by 906
+Format and fsstress
+Check quota before remount
+Check quota after remount
+Comparing user usage
+Comparing group usage
diff --git a/tests/xfs/group b/tests/xfs/group
index cf81451..fa68824 100644
--- a/tests/xfs/group
+++ b/tests/xfs/group
@@ -437,3 +437,6 @@
 437 auto quick other
 438 auto quick quota dangerous
 439 auto quick fuzzers log
+904 auto quick clone quota
+905 auto quick clone quota
+906 auto stress clone quota

[PATCH 4/4] xfs/348: dir->symlink corruption must not be allowed

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

A directory corrupted into a symlink will be caught by the upcoming
local format ifork verifiers.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 tests/xfs/348.out |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)


diff --git a/tests/xfs/348.out b/tests/xfs/348.out
index f4a7a71..17d9be2 100644
--- a/tests/xfs/348.out
+++ b/tests/xfs/348.out
@@ -239,7 +239,7 @@ would have junked entry "DATA" in directory PARENT_INO
 would have junked entry "DIR" in directory PARENT_INO
 would have junked entry "EMPTY" in directory PARENT_INO
 would have junked entry "FIFO" in directory PARENT_INO
-stat: 'SCRATCH_MNT/test/DIR' is a symbolic link
+stat: cannot stat 'SCRATCH_MNT/test/DIR': Structure needs cleaning
 stat: 'SCRATCH_MNT/test/DATA' is a symbolic link
 stat: cannot stat 'SCRATCH_MNT/test/EMPTY': Structure needs cleaning
 stat: 'SCRATCH_MNT/test/SYMLINK' is a symbolic link

Re: [PATCH 4/4] xfs/348: dir->symlink corruption must not be allowed

[Eryu Guan]

On Wed, Feb 07, 2018 at 01:19:45PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> A directory corrupted into a symlink will be caught by the upcoming

Still "upcoming" or already landed upstream?

> local format ifork verifiers.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

I was about to ping on this patch yesterday :)

Thanks,
Eryu

> ---
>  tests/xfs/348.out |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> 
> diff --git a/tests/xfs/348.out b/tests/xfs/348.out
> index f4a7a71..17d9be2 100644
> --- a/tests/xfs/348.out
> +++ b/tests/xfs/348.out
> @@ -239,7 +239,7 @@ would have junked entry "DATA" in directory PARENT_INO
>  would have junked entry "DIR" in directory PARENT_INO
>  would have junked entry "EMPTY" in directory PARENT_INO
>  would have junked entry "FIFO" in directory PARENT_INO
> -stat: 'SCRATCH_MNT/test/DIR' is a symbolic link
> +stat: cannot stat 'SCRATCH_MNT/test/DIR': Structure needs cleaning
>  stat: 'SCRATCH_MNT/test/DATA' is a symbolic link
>  stat: cannot stat 'SCRATCH_MNT/test/EMPTY': Structure needs cleaning
>  stat: 'SCRATCH_MNT/test/SYMLINK' is a symbolic link
> 

Re: [PATCH 2/4] xfs: skip xfs_check in _check_xfs_filesystem

[Eryu Guan]

On Wed, Feb 07, 2018 at 01:19:31PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> xfs_check has been long obsolete, so stop running it automatically
> after every test.  Tests that explicitly want xfs_check can call it
> via _scratch_xfs_check or _xfs_check; that part doesn't go away.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

I'd like to see an ACK on this from XFS community.

Thanks,
Eryu

> ---
>  common/xfs |   17 -----------------
>  1 file changed, 17 deletions(-)
> 
> 
> diff --git a/common/xfs b/common/xfs
> index 3dba40d..c63e5dc 100644
> --- a/common/xfs
> +++ b/common/xfs
> @@ -386,23 +386,6 @@ _check_xfs_filesystem()
>  		ok=0
>  	fi
>  
> -	# xfs_check runs out of memory on large files, so even providing the test
> -	# option (-t) to avoid indexing the free space trees doesn't make it pass on
> -	# large filesystems. Avoid it.
> -	if [ "$LARGE_SCRATCH_DEV" != yes ]; then
> -		_xfs_check $extra_log_options $device 2>&1 |\
> -			_fix_malloc >$tmp.fs_check
> -	fi
> -	if [ -s $tmp.fs_check ]; then
> -		_log_err "_check_xfs_filesystem: filesystem on $device is inconsistent (c)"
> -		echo "*** xfs_check output ***"		>>$seqres.full
> -		cat $tmp.fs_check			>>$seqres.full
> -		echo "*** end xfs_check output"		>>$seqres.full
> -
> -		xfs_metadump $device $seqres.check	>>$seqres.full 2>&1
> -		ok=0
> -	fi
> -
>  	$XFS_REPAIR_PROG -n $extra_options $extra_log_options $extra_rt_options $device >$tmp.repair 2>&1
>  	if [ $? -ne 0 ]; then
>  		_log_err "_check_xfs_filesystem: filesystem on $device is inconsistent (r)"
> 

Re: [PATCH 4/4] xfs/348: dir->symlink corruption must not be allowed

[Darrick J. Wong]

On Thu, Feb 08, 2018 at 07:11:17PM +0800, Eryu Guan wrote:
> On Wed, Feb 07, 2018 at 01:19:45PM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > A directory corrupted into a symlink will be caught by the upcoming
> 
> Still "upcoming" or already landed upstream?

Landed upstream finally. :)

--D

> > local format ifork verifiers.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> 
> I was about to ping on this patch yesterday :)
> 
> Thanks,
> Eryu
> 
> > ---
> >  tests/xfs/348.out |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > 
> > diff --git a/tests/xfs/348.out b/tests/xfs/348.out
> > index f4a7a71..17d9be2 100644
> > --- a/tests/xfs/348.out
> > +++ b/tests/xfs/348.out
> > @@ -239,7 +239,7 @@ would have junked entry "DATA" in directory PARENT_INO
> >  would have junked entry "DIR" in directory PARENT_INO
> >  would have junked entry "EMPTY" in directory PARENT_INO
> >  would have junked entry "FIFO" in directory PARENT_INO
> > -stat: 'SCRATCH_MNT/test/DIR' is a symbolic link
> > +stat: cannot stat 'SCRATCH_MNT/test/DIR': Structure needs cleaning
> >  stat: 'SCRATCH_MNT/test/DATA' is a symbolic link
> >  stat: cannot stat 'SCRATCH_MNT/test/EMPTY': Structure needs cleaning
> >  stat: 'SCRATCH_MNT/test/SYMLINK' is a symbolic link
> > 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/4] xfs: skip xfs_check in _check_xfs_filesystem

[Eryu Guan]

On Thu, Feb 08, 2018 at 08:51:52PM +0800, Eryu Guan wrote:
> On Wed, Feb 07, 2018 at 01:19:31PM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > xfs_check has been long obsolete, so stop running it automatically
> > after every test.  Tests that explicitly want xfs_check can call it
> > via _scratch_xfs_check or _xfs_check; that part doesn't go away.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> 
> I'd like to see an ACK on this from XFS community.

ping on this patch for review. Personally I'm happy to see skipping
xfs_check, it's deprecated and I can avoid test failures caused by
xfs_db crash due to out-of-memory in xfs_check run.

Thanks,
Eryu

> 
> > ---
> >  common/xfs |   17 -----------------
> >  1 file changed, 17 deletions(-)
> > 
> > 
> > diff --git a/common/xfs b/common/xfs
> > index 3dba40d..c63e5dc 100644
> > --- a/common/xfs
> > +++ b/common/xfs
> > @@ -386,23 +386,6 @@ _check_xfs_filesystem()
> >  		ok=0
> >  	fi
> >  
> > -	# xfs_check runs out of memory on large files, so even providing the test
> > -	# option (-t) to avoid indexing the free space trees doesn't make it pass on
> > -	# large filesystems. Avoid it.
> > -	if [ "$LARGE_SCRATCH_DEV" != yes ]; then
> > -		_xfs_check $extra_log_options $device 2>&1 |\
> > -			_fix_malloc >$tmp.fs_check
> > -	fi
> > -	if [ -s $tmp.fs_check ]; then
> > -		_log_err "_check_xfs_filesystem: filesystem on $device is inconsistent (c)"
> > -		echo "*** xfs_check output ***"		>>$seqres.full
> > -		cat $tmp.fs_check			>>$seqres.full
> > -		echo "*** end xfs_check output"		>>$seqres.full
> > -
> > -		xfs_metadump $device $seqres.check	>>$seqres.full 2>&1
> > -		ok=0
> > -	fi
> > -
> >  	$XFS_REPAIR_PROG -n $extra_options $extra_log_options $extra_rt_options $device >$tmp.repair 2>&1
> >  	if [ $? -ne 0 ]; then
> >  		_log_err "_check_xfs_filesystem: filesystem on $device is inconsistent (r)"
> > 
> --
> 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 2/4] xfs: skip xfs_check in _check_xfs_filesystem

[Eric Sandeen]

On 2/8/18 6:51 AM, Eryu Guan wrote:
> On Wed, Feb 07, 2018 at 01:19:31PM -0800, Darrick J. Wong wrote:
>> From: Darrick J. Wong <darrick.wong@oracle.com>
>>
>> xfs_check has been long obsolete, so stop running it automatically
>> after every test.  Tests that explicitly want xfs_check can call it
>> via _scratch_xfs_check or _xfs_check; that part doesn't go away.
>>
>> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> 
> I'd like to see an ACK on this from XFS community.

So, I thought we still had a local implementation of xfs_check-via-xfs_db
in xfstests as an intentional validation against xfs_repair:

+# xfs_check script is planned to be deprecated. But, we want to
+# be able to invoke "xfs_check" behavior in xfstests in order to
+# maintain the current verification levels.
+_xfs_check()

IOWS, "xfs_check is obsolete" is true for users/admins, but I thought
we were keeping it alive as a developer tool for now, and that it had
a place in xfstests.  No?

(But if it /is/ removed, wouldn't you kill off the _xfs_check() function as
well, to truly nuke it from orbit?)

Anyway, I don't think I understand the justification for this change;
it was explicitly kept alive in xfstests since commit 

	187bccd xfstests: Remove dependence of xfs_check script

and "xfs_check is obsolete for admins" doesn't seem to be sufficient rationale.

What do we gain or lose by removing this from _check_xfs_filesystem?

-Eric

> Thanks,
> Eryu
> 
>> ---
>>  common/xfs |   17 -----------------
>>  1 file changed, 17 deletions(-)
>>
>>
>> diff --git a/common/xfs b/common/xfs
>> index 3dba40d..c63e5dc 100644
>> --- a/common/xfs
>> +++ b/common/xfs
>> @@ -386,23 +386,6 @@ _check_xfs_filesystem()
>>  		ok=0
>>  	fi
>>  
>> -	# xfs_check runs out of memory on large files, so even providing the test
>> -	# option (-t) to avoid indexing the free space trees doesn't make it pass on
>> -	# large filesystems. Avoid it.


>> -	if [ "$LARGE_SCRATCH_DEV" != yes ]; then
>> -		_xfs_check $extra_log_options $device 2>&1 |\
>> -			_fix_malloc >$tmp.fs_check
>> -	fi
>> -	if [ -s $tmp.fs_check ]; then
>> -		_log_err "_check_xfs_filesystem: filesystem on $device is inconsistent (c)"
>> -		echo "*** xfs_check output ***"		>>$seqres.full
>> -		cat $tmp.fs_check			>>$seqres.full
>> -		echo "*** end xfs_check output"		>>$seqres.full
>> -
>> -		xfs_metadump $device $seqres.check	>>$seqres.full 2>&1
>> -		ok=0
>> -	fi
>> -
>>  	$XFS_REPAIR_PROG -n $extra_options $extra_log_options $extra_rt_options $device >$tmp.repair 2>&1
>>  	if [ $? -ne 0 ]; then
>>  		_log_err "_check_xfs_filesystem: filesystem on $device is inconsistent (r)"
>>
> --
> 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 2/4] xfs: skip xfs_check in _check_xfs_filesystem

[Darrick J. Wong]

On Wed, Feb 14, 2018 at 10:34:09AM -0600, Eric Sandeen wrote:
> On 2/8/18 6:51 AM, Eryu Guan wrote:
> > On Wed, Feb 07, 2018 at 01:19:31PM -0800, Darrick J. Wong wrote:
> >> From: Darrick J. Wong <darrick.wong@oracle.com>
> >>
> >> xfs_check has been long obsolete, so stop running it automatically
> >> after every test.  Tests that explicitly want xfs_check can call it
> >> via _scratch_xfs_check or _xfs_check; that part doesn't go away.
> >>
> >> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > I'd like to see an ACK on this from XFS community.
> 
> So, I thought we still had a local implementation of xfs_check-via-xfs_db
> in xfstests as an intentional validation against xfs_repair:
> 
> +# xfs_check script is planned to be deprecated. But, we want to
> +# be able to invoke "xfs_check" behavior in xfstests in order to
> +# maintain the current verification levels.
> +_xfs_check()
> 
> IOWS, "xfs_check is obsolete" is true for users/admins, but I thought
> we were keeping it alive as a developer tool for now, and that it had
> a place in xfstests.  No?
> 
> (But if it /is/ removed, wouldn't you kill off the _xfs_check() function as
> well, to truly nuke it from orbit?)
> 
> Anyway, I don't think I understand the justification for this change;
> it was explicitly kept alive in xfstests since commit 
> 
> 	187bccd xfstests: Remove dependence of xfs_check script
> 
> and "xfs_check is obsolete for admins" doesn't seem to be sufficient rationale.
> 
> What do we gain or lose by removing this from _check_xfs_filesystem?

xfs_check likes to consume memory, which means that it consistently runs
out and segfaults when I run xfstests on a 1k block configuration.  It's
also slow if the fs is really fragmented (which some of the reflink
tests set up), so I figured I'd kill both birds with one patch by
removing auto-xfscheck.

We /could/ leave it as a check against xfs_repair, but at this point we
have three different fsck(ish) tools and maybe it's just time to get rid
of check.  Consider that blockget didn't even support v5 filesystems
until October 2015[1], which was ~2 years after v5 support landed in
repair -- that convinced me (at the time) that nobody really cared about
xfs_check anymore.  I only added support so that I could work on
blocktrash fuzz testing. :)

--D

[1] e96864ff4d40, "xfs_db: enable blockget for v5 filesystems"

> 
> -Eric
> 
> > Thanks,
> > Eryu
> > 
> >> ---
> >>  common/xfs |   17 -----------------
> >>  1 file changed, 17 deletions(-)
> >>
> >>
> >> diff --git a/common/xfs b/common/xfs
> >> index 3dba40d..c63e5dc 100644
> >> --- a/common/xfs
> >> +++ b/common/xfs
> >> @@ -386,23 +386,6 @@ _check_xfs_filesystem()
> >>  		ok=0
> >>  	fi
> >>  
> >> -	# xfs_check runs out of memory on large files, so even providing the test
> >> -	# option (-t) to avoid indexing the free space trees doesn't make it pass on
> >> -	# large filesystems. Avoid it.
> 
> 
> >> -	if [ "$LARGE_SCRATCH_DEV" != yes ]; then
> >> -		_xfs_check $extra_log_options $device 2>&1 |\
> >> -			_fix_malloc >$tmp.fs_check
> >> -	fi
> >> -	if [ -s $tmp.fs_check ]; then
> >> -		_log_err "_check_xfs_filesystem: filesystem on $device is inconsistent (c)"
> >> -		echo "*** xfs_check output ***"		>>$seqres.full
> >> -		cat $tmp.fs_check			>>$seqres.full
> >> -		echo "*** end xfs_check output"		>>$seqres.full
> >> -
> >> -		xfs_metadump $device $seqres.check	>>$seqres.full 2>&1
> >> -		ok=0
> >> -	fi
> >> -
> >>  	$XFS_REPAIR_PROG -n $extra_options $extra_log_options $extra_rt_options $device >$tmp.repair 2>&1
> >>  	if [ $? -ne 0 ]; then
> >>  		_log_err "_check_xfs_filesystem: filesystem on $device is inconsistent (r)"
> >>
> > --
> > 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
> > 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/4] xfs: skip xfs_check in _check_xfs_filesystem

[Dave Chinner]

On Wed, Feb 14, 2018 at 09:02:54AM -0800, Darrick J. Wong wrote:
> On Wed, Feb 14, 2018 at 10:34:09AM -0600, Eric Sandeen wrote:
> > On 2/8/18 6:51 AM, Eryu Guan wrote:
> > > On Wed, Feb 07, 2018 at 01:19:31PM -0800, Darrick J. Wong wrote:
> > >> From: Darrick J. Wong <darrick.wong@oracle.com>
> > >>
> > >> xfs_check has been long obsolete, so stop running it automatically
> > >> after every test.  Tests that explicitly want xfs_check can call it
> > >> via _scratch_xfs_check or _xfs_check; that part doesn't go away.
> > >>
> > >> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > > 
> > > I'd like to see an ACK on this from XFS community.
> > 
> > So, I thought we still had a local implementation of xfs_check-via-xfs_db
> > in xfstests as an intentional validation against xfs_repair:
> > 
> > +# xfs_check script is planned to be deprecated. But, we want to
> > +# be able to invoke "xfs_check" behavior in xfstests in order to
> > +# maintain the current verification levels.
> > +_xfs_check()
> > 
> > IOWS, "xfs_check is obsolete" is true for users/admins, but I thought
> > we were keeping it alive as a developer tool for now, and that it had
> > a place in xfstests.  No?
> > 
> > (But if it /is/ removed, wouldn't you kill off the _xfs_check() function as
> > well, to truly nuke it from orbit?)
> > 
> > Anyway, I don't think I understand the justification for this change;
> > it was explicitly kept alive in xfstests since commit 
> > 
> > 	187bccd xfstests: Remove dependence of xfs_check script
> > 
> > and "xfs_check is obsolete for admins" doesn't seem to be sufficient rationale.
> > 
> > What do we gain or lose by removing this from _check_xfs_filesystem?
> 
> xfs_check likes to consume memory, which means that it consistently runs
> out and segfaults when I run xfstests on a 1k block configuration.  It's
> also slow if the fs is really fragmented (which some of the reflink
> tests set up), so I figured I'd kill both birds with one patch by
> removing auto-xfscheck.

I don't see it run out of memory on my 1p, 1GB RAM, 1k block size
test VM, running on 10GB test, 20GB scratch devices....

> We /could/ leave it as a check against xfs_repair, but at this point we
> have three different fsck(ish) tools and maybe it's just time to get rid
> of check.  Consider that blockget didn't even support v5 filesystems
> until October 2015[1], which was ~2 years after v5 support landed in
> repair -- that convinced me (at the time) that nobody really cared about
> xfs_check anymore.

Actually, that was more a result of the developer who was doing all
the v5 work being made the as maintainer half way thru it's
experimental stage and that meant time to work on the non-critical
pieces of v5 support were pretty severely curtailed. i.e. it wasn't
because we never intended to support this, just resources and
priorities changed drastically around that time.

Once we get scrub doing everything check does and have some
confidence that it's working correctly, then we can remove the db
based check code. Right now we still rely on it to cross check
repair and scrub is about to drive a bunch of new changes to th
erepair code to fix up stuff it doesn't detect.  Once we've got to
the point that repair and scrub to the same level and have a bit
more confidence in the scrub code, then we can think about retiring
the db-based check code...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 2/4] xfs: skip xfs_check in _check_xfs_filesystem

[Darrick J. Wong]

On Thu, Feb 15, 2018 at 08:22:28AM +1100, Dave Chinner wrote:
> On Wed, Feb 14, 2018 at 09:02:54AM -0800, Darrick J. Wong wrote:
> > On Wed, Feb 14, 2018 at 10:34:09AM -0600, Eric Sandeen wrote:
> > > On 2/8/18 6:51 AM, Eryu Guan wrote:
> > > > On Wed, Feb 07, 2018 at 01:19:31PM -0800, Darrick J. Wong wrote:
> > > >> From: Darrick J. Wong <darrick.wong@oracle.com>
> > > >>
> > > >> xfs_check has been long obsolete, so stop running it automatically
> > > >> after every test.  Tests that explicitly want xfs_check can call it
> > > >> via _scratch_xfs_check or _xfs_check; that part doesn't go away.
> > > >>
> > > >> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > > > 
> > > > I'd like to see an ACK on this from XFS community.
> > > 
> > > So, I thought we still had a local implementation of xfs_check-via-xfs_db
> > > in xfstests as an intentional validation against xfs_repair:
> > > 
> > > +# xfs_check script is planned to be deprecated. But, we want to
> > > +# be able to invoke "xfs_check" behavior in xfstests in order to
> > > +# maintain the current verification levels.
> > > +_xfs_check()
> > > 
> > > IOWS, "xfs_check is obsolete" is true for users/admins, but I thought
> > > we were keeping it alive as a developer tool for now, and that it had
> > > a place in xfstests.  No?
> > > 
> > > (But if it /is/ removed, wouldn't you kill off the _xfs_check() function as
> > > well, to truly nuke it from orbit?)
> > > 
> > > Anyway, I don't think I understand the justification for this change;
> > > it was explicitly kept alive in xfstests since commit 
> > > 
> > > 	187bccd xfstests: Remove dependence of xfs_check script
> > > 
> > > and "xfs_check is obsolete for admins" doesn't seem to be sufficient rationale.
> > > 
> > > What do we gain or lose by removing this from _check_xfs_filesystem?
> > 
> > xfs_check likes to consume memory, which means that it consistently runs
> > out and segfaults when I run xfstests on a 1k block configuration.  It's
> > also slow if the fs is really fragmented (which some of the reflink
> > tests set up), so I figured I'd kill both birds with one patch by
> > removing auto-xfscheck.
> 
> I don't see it run out of memory on my 1p, 1GB RAM, 1k block size
> test VM, running on 10GB test, 20GB scratch devices....

Maybe you're not crazy like me, who doesn't allow memory overcommit on
his test VMs. :P

> > We /could/ leave it as a check against xfs_repair, but at this point we
> > have three different fsck(ish) tools and maybe it's just time to get rid
> > of check.  Consider that blockget didn't even support v5 filesystems
> > until October 2015[1], which was ~2 years after v5 support landed in
> > repair -- that convinced me (at the time) that nobody really cared about
> > xfs_check anymore.
> 
> Actually, that was more a result of the developer who was doing all
> the v5 work being made the as maintainer half way thru it's
> experimental stage and that meant time to work on the non-critical
> pieces of v5 support were pretty severely curtailed. i.e. it wasn't
> because we never intended to support this, just resources and
> priorities changed drastically around that time.

Aha :)

> Once we get scrub doing everything check does and have some
> confidence that it's working correctly, then we can remove the db
> based check code. Right now we still rely on it to cross check
> repair and scrub is about to drive a bunch of new changes to th
> erepair code to fix up stuff it doesn't detect.  Once we've got to
> the point that repair and scrub to the same level and have a bit
> more confidence in the scrub code, then we can think about retiring
> the db-based check code...

TBH I've wondered off and on whether it would be worth it to duplicate
the existing fuzz tests so that we could assess whether or not
xfs_repair actually catches everything that xfs_check can... but first I
want to get scrub upstreamed so that I can fix all the things that
either of /those/ tools should catch but does not.

I withdraw this patch for now.

--D

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