[PATCHSET v2 0/4] fstests: filesystem population fixes

[PATCHSET v2 0/4] fstests: filesystem population fixes

[Darrick J. Wong]

Hi all,

[original patchset cover letter from Dave]

common/populate operations are slow. They are not coded for
performance, and do things in very slow ways. Mainly doing loops to
create/remove files and forcing a task to be created and destroy for
every individual operation. We can only fork a few thousand
processes a second, whilst we can create or remove tens of thousands
of files a second. Hence population speed is limited by fork/exit
overhead, not filesystem speed. I also changed it to run all the
creation steps in parallel, which means they run as fast as the
filesystem can handle them rather than as fast as a single CPU can
handle them.

patch 1 and patch 3 address these issues for common/populate and
xfs/294.  I may update a bunch of other tests that use loop { touch
file } to create thousands of files to speed them up as well.

The other patch in this series (patch 2) fixes the problem with
populating an Xfs btree format directory, which currently fails
because the removal step that creates sparse directory data also
causes the dabtree index to get smaller and free blocks, taking the
inode from btree to extent format and hence failing the populate
checks.

More details are in the commit messages for change.

[further changes from Darrick]

This series moves the FMT_BTREE creation bugfix to be first in line,
since it's a bug fix.  Next, I convert the directory and xattr creation
loops into separate programs to reduce the execve overhead.  This alone
is sufficient for a 10x reduction in runtime without substantially
altering what gets written to disk and comes out in the xfs fsck fuzz
tests.

The last patch in this series starts parallelizing things, but I've left
most of that out since the parallelization patches make it harder to
reliably generate a filesystem image where we can fuzz a two-level inobt
and still mount the fs to run online fsck.

If you're going to start using this mess, you probably ought to just
pull from my git trees, which are linked below.

This is an extraordinary way to destroy everything.  Enjoy!
Comments and questions are, as always, welcome.

--D

fstests git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfstests-dev.git/log/?h=fix-populate-slowness
---
 common/populate |   89 ++++++++++++++++++++++++++++++-------------------------
 src/popattr.py  |   62 ++++++++++++++++++++++++++++++++++++++
 src/popdir.pl   |   72 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 182 insertions(+), 41 deletions(-)
 create mode 100755 src/popattr.py
 create mode 100755 src/popdir.pl

[PATCH 1/4] populate: ensure btree directories are created reliably

[Darrick J. Wong]

From: Dave Chinner <dchinner@redhat.com>

The population function creates an XFS btree format directory by
polling the extent count of the inode and creating new dirents until
the extent count goes over the limit that pushes it into btree
format.

It then removes every second dirent to create empty space in the
directory data to ensure that operations like metadump with
obfuscation can check that they don't leak stale data from deleted
dirents.

Whilst this does not result in directory data blocks being freed, it
does not take into account the fact that the dabtree index has half
the entries removed from it and that can result in btree nodes
merging and extents being freed. This causes the extent count to go
down, and the inode is converted back into extent form. The
population checks then fail because it should be in btree form.

Fix this by counting the number of directory data extents rather than
the total number of extents in the data fork. We can do this simply
by using xfs_bmap and counting the number of extents returned as it
does not report extents beyond EOF (which is where the dabtree is
located). As the number of data blocks does not change with the
dirent removal algorithm used, this will ensure that the inode data
fork remains in btree format.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
Reviewed-by: Darrick J. Wong <djwong@kernel.org>
[djwong: make this patch first in line]
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 common/populate |    7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)


diff --git a/common/populate b/common/populate
index 44b4af1667..84f4b8e374 100644
--- a/common/populate
+++ b/common/populate
@@ -89,9 +89,12 @@ __populate_xfs_create_btree_dir() {
 		local creat=mkdir
 		test "$((nr % 20))" -eq 0 && creat=touch
 		$creat "${name}/$(printf "%.08d" "$nr")"
+		# Extent count checks use data blocks only to avoid the removal
+		# step from removing dabtree index blocks and reducing the
+		# number of extents below the required threshold.
 		if [ "$((nr % 40))" -eq 0 ]; then
-			local nextents="$(_xfs_get_fsxattr nextents $name)"
-			[ $nextents -gt $max_nextents ] && break
+			local nextents="$(xfs_bmap ${name} | grep -v hole | wc -l)"
+			[ "$((nextents - 1))" -gt $max_nextents ] && break
 		fi
 		nr=$((nr+1))
 	done

[PATCH 2/4] populate: remove file creation loops that take forever

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

Replace the file creation loops with a perl script that does everything
we want from a single process.  This reduces the runtime of
_scratch_xfs_populate substantially by avoiding thousands of execve
overhead.  On my system, this reduces the runtime of xfs/349 (with scrub
enabled) from ~140s to ~45s.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 common/populate |   61 ++++++++++++++++++-----------------------------
 src/popdir.pl   |   72 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 96 insertions(+), 37 deletions(-)
 create mode 100755 src/popdir.pl


diff --git a/common/populate b/common/populate
index 84f4b8e374..180540aedd 100644
--- a/common/populate
+++ b/common/populate
@@ -11,6 +11,7 @@ _require_populate_commands() {
 	_require_xfs_io_command "falloc"
 	_require_xfs_io_command "fpunch"
 	_require_test_program "punch-alternating"
+	_require_test_program "popdir.pl"
 	case "${FSTYP}" in
 	"xfs")
 		_require_command "$XFS_DB_PROG" "xfs_db"
@@ -54,55 +55,50 @@ __populate_fragment_file() {
 
 # Create a large directory
 __populate_create_dir() {
-	name="$1"
-	nr="$2"
-	missing="$3"
+	local name="$1"
+	local nr="$2"
+	local missing="$3"
+	shift; shift; shift
 
 	mkdir -p "${name}"
-	seq 0 "${nr}" | while read d; do
-		creat=mkdir
-		test "$((d % 20))" -eq 0 && creat=touch
-		$creat "${name}/$(printf "%.08d" "$d")"
-	done
+	$here/src/popdir.pl --dir "${name}" --end "${nr}" "$@"
 
 	test -z "${missing}" && return
-	seq 1 2 "${nr}" | while read d; do
-		rm -rf "${name}/$(printf "%.08d" "$d")"
-	done
+	$here/src/popdir.pl --dir "${name}" --start 1 --incr 2 --end "${nr}" --remove "$@"
 }
 
 # Create a large directory and ensure that it's a btree format
 __populate_xfs_create_btree_dir() {
 	local name="$1"
 	local isize="$2"
-	local missing="$3"
+	local dblksz="$3"
+	local missing="$4"
 	local icore_size="$(_xfs_get_inode_core_bytes $SCRATCH_MNT)"
 	# We need enough extents to guarantee that the data fork is in
 	# btree format.  Cycling the mount to use xfs_db is too slow, so
 	# watch for when the extent count exceeds the space after the
 	# inode core.
 	local max_nextents="$(((isize - icore_size) / 16))"
-	local nr=0
+	local nr
+	local incr
+
+	# Add about one block's worth of dirents before we check the data fork
+	# format.
+	incr=$(( (dblksz / 8) / 100 * 100 ))
 
 	mkdir -p "${name}"
-	while true; do
-		local creat=mkdir
-		test "$((nr % 20))" -eq 0 && creat=touch
-		$creat "${name}/$(printf "%.08d" "$nr")"
+	for ((nr = 0; ; nr += incr)); do
+		$here/src/popdir.pl --dir "${name}" --start "${nr}" --end "$((nr + incr - 1))"
+
 		# Extent count checks use data blocks only to avoid the removal
 		# step from removing dabtree index blocks and reducing the
 		# number of extents below the required threshold.
-		if [ "$((nr % 40))" -eq 0 ]; then
-			local nextents="$(xfs_bmap ${name} | grep -v hole | wc -l)"
-			[ "$((nextents - 1))" -gt $max_nextents ] && break
-		fi
-		nr=$((nr+1))
+		local nextents="$(xfs_bmap ${name} | grep -v hole | wc -l)"
+		[ "$((nextents - 1))" -gt $max_nextents ] && break
 	done
 
 	test -z "${missing}" && return
-	seq 1 2 "${nr}" | while read d; do
-		rm -rf "${name}/$(printf "%.08d" "$d")"
-	done
+	$here/src/popdir.pl --dir "${name}" --start 1 --incr 2 --end "${nr}" --remove
 }
 
 # Add a bunch of attrs to a file
@@ -224,9 +220,7 @@ _scratch_xfs_populate() {
 
 	# Fill up the root inode chunk
 	echo "+ fill root ino chunk"
-	seq 1 64 | while read f; do
-		$XFS_IO_PROG -f -c "truncate 0" "${SCRATCH_MNT}/dummy${f}"
-	done
+	$here/src/popdir.pl --dir "${SCRATCH_MNT}" --start 1 --end 64 --format "dummy%u" --file-mult 1
 
 	# Regular files
 	# - FMT_EXTENTS
@@ -261,7 +255,7 @@ _scratch_xfs_populate() {
 
 	# - BTREE
 	echo "+ btree dir"
-	__populate_xfs_create_btree_dir "${SCRATCH_MNT}/S_IFDIR.FMT_BTREE" "$isize" true
+	__populate_xfs_create_btree_dir "${SCRATCH_MNT}/S_IFDIR.FMT_BTREE" "$isize" "$dblksz" true
 
 	# Symlinks
 	# - FMT_LOCAL
@@ -340,14 +334,7 @@ _scratch_xfs_populate() {
 	local rec_per_btblock=16
 	local nr="$(( 2 * (blksz / rec_per_btblock) * ino_per_rec ))"
 	local dir="${SCRATCH_MNT}/INOBT"
-	mkdir -p "${dir}"
-	seq 0 "${nr}" | while read f; do
-		touch "${dir}/${f}"
-	done
-
-	seq 0 2 "${nr}" | while read f; do
-		rm -f "${dir}/${f}"
-	done
+	__populate_create_dir "${dir}" "${nr}" true --file-mult 1
 
 	# Reverse-mapping btree
 	is_rmapbt="$(_xfs_has_feature "$SCRATCH_MNT" rmapbt -v)"
diff --git a/src/popdir.pl b/src/popdir.pl
new file mode 100755
index 0000000000..dc0c046b7d
--- /dev/null
+++ b/src/popdir.pl
@@ -0,0 +1,72 @@
+#!/usr/bin/perl -w
+
+# Copyright (c) 2023 Oracle.  All rights reserved.
+# SPDX-License-Identifier: GPL-2.0
+#
+# Create a bunch of files and subdirs in a directory.
+
+use Getopt::Long;
+use File::Basename;
+
+$progname=$0;
+GetOptions("start=i" => \$start,
+	   "end=i" => \$end,
+	   "file-mult=i" => \$file_mult,
+	   "incr=i" => \$incr,
+	   "format=s" => \$format,
+	   "dir=s" => \$dir,
+	   "remove!" => \$remove,
+	   "help!" => \$help,
+	   "verbose!" => \$verbose);
+
+
+# check/remove output directory, get filesystem info
+if (defined $help) {
+  # newline at end of die message suppresses line number
+  print STDERR <<"EOF";
+Usage: $progname [options]
+Options:
+  --dir             chdir here before starting
+  --start=num       create names starting with this number (0)
+  --incr=num        increment file number by this much (1)
+  --end=num         stop at this file number (100)
+  --file-mult       create a regular file when file number is a multiple
+                    of this quantity (20)
+  --remove          remove instead of creating
+  --format=str      printf formatting string for file name ("%08d")
+  --verbose         verbose output
+  --help            this help screen
+EOF
+  exit(1) unless defined $help;
+  # otherwise...
+  exit(0);
+}
+
+if (defined $dir) {
+	chdir($dir) or die("chdir $dir");
+}
+$start = 0 if (!defined $start);
+$end = 100 if (!defined $end);
+$file_mult = 20 if (!defined $file_mult);
+$format = "%08d" if (!defined $format);
+$incr = 1 if (!defined $incr);
+
+for ($i = $start; $i <= $end; $i += $incr) {
+	$fname = sprintf($format, $i);
+
+	if ($remove) {
+		$verbose && print "rm $fname\n";
+		unlink($fname) or rmdir($fname) or die("unlink $fname");
+	} elsif ($file_mult == 0 or ($i % $file_mult) == 0) {
+		# create a file
+		$verbose && print "touch $fname\n";
+		open(DONTCARE, ">$fname") or die("touch $fname");
+		close(DONTCARE);
+	} else {
+		# create a subdir
+		$verbose && print "mkdir $fname\n";
+		mkdir($fname, 0755) or die("mkdir $fname");
+	}
+}
+
+exit(0);

[PATCH 3/4] populate: improve attr creation runtime

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

Replace the file creation loops with a python script that does
everything we want from a single process.  This reduces the runtime of
_scratch_xfs_populate substantially by avoiding thousands of execve
overhead.  This patch builds on the previous one by reducing the runtime
of xfs/349 from ~45s to ~15s.

For people who don't have python3, use setfattr's "restore" mode to bulk
create xattrs.  This reduces runtime to about ~25s.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 common/populate |   22 +++++++++++++++++---
 src/popattr.py  |   62 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 81 insertions(+), 3 deletions(-)
 create mode 100755 src/popattr.py


diff --git a/common/populate b/common/populate
index 180540aedd..f34551d272 100644
--- a/common/populate
+++ b/common/populate
@@ -12,6 +12,10 @@ _require_populate_commands() {
 	_require_xfs_io_command "fpunch"
 	_require_test_program "punch-alternating"
 	_require_test_program "popdir.pl"
+	if [ -n "${PYTHON3_PROG}" ]; then
+		_require_command $PYTHON3_PROG python3
+		_require_test_program "popattr.py"
+	fi
 	case "${FSTYP}" in
 	"xfs")
 		_require_command "$XFS_DB_PROG" "xfs_db"
@@ -108,9 +112,21 @@ __populate_create_attr() {
 	missing="$3"
 
 	touch "${name}"
-	seq 0 "${nr}" | while read d; do
-		setfattr -n "user.$(printf "%.08d" "$d")" -v "$(printf "%.08d" "$d")" "${name}"
-	done
+
+	if [ -n "${PYTHON3_PROG}" ]; then
+		${PYTHON3_PROG} $here/src/popattr.py --file "${name}" --end "${nr}"
+
+		test -z "${missing}" && return
+		${PYTHON3_PROG} $here/src/popattr.py --file "${name}" --start 1 --incr 2 --end "${nr}" --remove
+		return
+	fi
+
+	# Simulate a getfattr dump file so we can bulk-add attrs.
+	(
+		echo "# file: ${name}";
+		seq --format "user.%08g=\"abcdefgh\"" 0 "${nr}"
+		echo
+	) | setfattr --restore -
 
 	test -z "${missing}" && return
 	seq 1 2 "${nr}" | while read d; do
diff --git a/src/popattr.py b/src/popattr.py
new file mode 100755
index 0000000000..397ced9d33
--- /dev/null
+++ b/src/popattr.py
@@ -0,0 +1,62 @@
+#!/usr/bin/python3
+
+# Copyright (c) 2023 Oracle.  All rights reserved.
+# SPDX-License-Identifier: GPL-2.0
+#
+# Create a bunch of xattrs in a file.
+
+import argparse
+import sys
+import os
+
+parser = argparse.ArgumentParser(description = 'Mass create xattrs in a file')
+parser.add_argument(
+	'--file', required = True, type = str, help = 'manipulate this file')
+parser.add_argument(
+	'--start', type = int, default = 0,
+	help = 'create xattrs starting with this number')
+parser.add_argument(
+	'--incr', type = int, default = 1,
+	help = 'increment attr number by this much')
+parser.add_argument(
+	'--end', type = int, default = 1000,
+	help = 'stop at this attr number')
+parser.add_argument(
+	'--remove', dest = 'remove', action = 'store_true',
+	help = 'remove instead of creating')
+parser.add_argument(
+	'--format', type = str, default = '%08d',
+	help = 'printf formatting string for attr name')
+parser.add_argument(
+	'--verbose', dest = 'verbose', action = 'store_true',
+	help = 'verbose output')
+
+args = parser.parse_args()
+
+fmtstring = "user.%s" % args.format
+
+# If we are passed a regular file, open it as a proper file descriptor and
+# pass that around for speed.  Otherwise, we pass the path.
+fp = None
+try:
+	fp = open(args.file, 'r')
+	fd = fp.fileno()
+	os.listxattr(fd)
+	if args.verbose:
+		print("using fd calls")
+except:
+	if args.verbose:
+		print("using path calls")
+	fd = args.file
+
+for i in range(args.start, args.end + 1, args.incr):
+	fname = fmtstring % i
+
+	if args.remove:
+		if args.verbose:
+			print("removexattr %s" % fname)
+		os.removexattr(fd, fname)
+	else:
+		if args.verbose:
+			print("setxattr %s" % fname)
+		os.setxattr(fd, fname, b'abcdefgh')

[PATCH 4/4] populate: improve runtime of __populate_fill_fs

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

Run the copy loop in parallel to reduce runtime.  If filling the
populated fs is selected (which it isn't by default in xfs/349), this
reduces the runtime from ~18s to ~15s, since it's only making enough
copies to reduce the free space by 5%.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 common/populate |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)


diff --git a/common/populate b/common/populate
index f34551d272..1c3c28463f 100644
--- a/common/populate
+++ b/common/populate
@@ -151,8 +151,9 @@ __populate_fill_fs() {
 	echo "FILL FS"
 	echo "src_sz $SRC_SZ fs_sz $FS_SZ nr $NR"
 	seq 2 "${NR}" | while read nr; do
-		cp -pRdu "${dir}/test/1" "${dir}/test/${nr}"
+		cp -pRdu "${dir}/test/1" "${dir}/test/${nr}" &
 	done
+	wait
 }
 
 # For XFS, force on all the quota options if quota is enabled

Re: [PATCHSET v2 0/4] fstests: filesystem population fixes

[Andrey Albershteyn]

On Tue, Jan 17, 2023 at 04:42:02PM -0800, Darrick J. Wong wrote:
> Hi all,
> 
> [original patchset cover letter from Dave]
> 
> common/populate operations are slow. They are not coded for
> performance, and do things in very slow ways. Mainly doing loops to
> create/remove files and forcing a task to be created and destroy for
> every individual operation. We can only fork a few thousand
> processes a second, whilst we can create or remove tens of thousands
> of files a second. Hence population speed is limited by fork/exit
> overhead, not filesystem speed. I also changed it to run all the
> creation steps in parallel, which means they run as fast as the
> filesystem can handle them rather than as fast as a single CPU can
> handle them.
> 
> patch 1 and patch 3 address these issues for common/populate and
> xfs/294.  I may update a bunch of other tests that use loop { touch
> file } to create thousands of files to speed them up as well.
> 
> The other patch in this series (patch 2) fixes the problem with
> populating an Xfs btree format directory, which currently fails
> because the removal step that creates sparse directory data also
> causes the dabtree index to get smaller and free blocks, taking the
> inode from btree to extent format and hence failing the populate
> checks.
> 
> More details are in the commit messages for change.
> 
> [further changes from Darrick]
> 
> This series moves the FMT_BTREE creation bugfix to be first in line,
> since it's a bug fix.  Next, I convert the directory and xattr creation
> loops into separate programs to reduce the execve overhead.  This alone
> is sufficient for a 10x reduction in runtime without substantially
> altering what gets written to disk and comes out in the xfs fsck fuzz
> tests.
> 
> The last patch in this series starts parallelizing things, but I've left
> most of that out since the parallelization patches make it harder to
> reliably generate a filesystem image where we can fuzz a two-level inobt
> and still mount the fs to run online fsck.
> 
> If you're going to start using this mess, you probably ought to just
> pull from my git trees, which are linked below.
> 
> This is an extraordinary way to destroy everything.  Enjoy!
> Comments and questions are, as always, welcome.
> 
> --D
> 
> fstests git tree:
> https://git.kernel.org/cgit/linux/kernel/git/djwong/xfstests-dev.git/log/?h=fix-populate-slowness
> ---
>  common/populate |   89 ++++++++++++++++++++++++++++++-------------------------
>  src/popattr.py  |   62 ++++++++++++++++++++++++++++++++++++++
>  src/popdir.pl   |   72 ++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 182 insertions(+), 41 deletions(-)
>  create mode 100755 src/popattr.py
>  create mode 100755 src/popdir.pl
> 

The patchset looks good to me:

Reviewed-by: Andrey Albershteyn <aalbersh@redhat.com>

-- 
- Andrey

Re: [PATCH 2/4] populate: remove file creation loops that take forever

[Zorro Lang]

On Tue, Jan 17, 2023 at 04:44:02PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Replace the file creation loops with a perl script that does everything
> we want from a single process.  This reduces the runtime of
> _scratch_xfs_populate substantially by avoiding thousands of execve
> overhead.  On my system, this reduces the runtime of xfs/349 (with scrub
> enabled) from ~140s to ~45s.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---

Looks good to me,

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

>  common/populate |   61 ++++++++++++++++++-----------------------------
>  src/popdir.pl   |   72 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 96 insertions(+), 37 deletions(-)
>  create mode 100755 src/popdir.pl
> 
> 
> diff --git a/common/populate b/common/populate
> index 84f4b8e374..180540aedd 100644
> --- a/common/populate
> +++ b/common/populate
> @@ -11,6 +11,7 @@ _require_populate_commands() {
>  	_require_xfs_io_command "falloc"
>  	_require_xfs_io_command "fpunch"
>  	_require_test_program "punch-alternating"
> +	_require_test_program "popdir.pl"
>  	case "${FSTYP}" in
>  	"xfs")
>  		_require_command "$XFS_DB_PROG" "xfs_db"
> @@ -54,55 +55,50 @@ __populate_fragment_file() {
>  
>  # Create a large directory
>  __populate_create_dir() {
> -	name="$1"
> -	nr="$2"
> -	missing="$3"
> +	local name="$1"
> +	local nr="$2"
> +	local missing="$3"
> +	shift; shift; shift
>  
>  	mkdir -p "${name}"
> -	seq 0 "${nr}" | while read d; do
> -		creat=mkdir
> -		test "$((d % 20))" -eq 0 && creat=touch
> -		$creat "${name}/$(printf "%.08d" "$d")"
> -	done
> +	$here/src/popdir.pl --dir "${name}" --end "${nr}" "$@"
>  
>  	test -z "${missing}" && return
> -	seq 1 2 "${nr}" | while read d; do
> -		rm -rf "${name}/$(printf "%.08d" "$d")"
> -	done
> +	$here/src/popdir.pl --dir "${name}" --start 1 --incr 2 --end "${nr}" --remove "$@"
>  }
>  
>  # Create a large directory and ensure that it's a btree format
>  __populate_xfs_create_btree_dir() {
>  	local name="$1"
>  	local isize="$2"
> -	local missing="$3"
> +	local dblksz="$3"
> +	local missing="$4"
>  	local icore_size="$(_xfs_get_inode_core_bytes $SCRATCH_MNT)"
>  	# We need enough extents to guarantee that the data fork is in
>  	# btree format.  Cycling the mount to use xfs_db is too slow, so
>  	# watch for when the extent count exceeds the space after the
>  	# inode core.
>  	local max_nextents="$(((isize - icore_size) / 16))"
> -	local nr=0
> +	local nr
> +	local incr
> +
> +	# Add about one block's worth of dirents before we check the data fork
> +	# format.
> +	incr=$(( (dblksz / 8) / 100 * 100 ))
>  
>  	mkdir -p "${name}"
> -	while true; do
> -		local creat=mkdir
> -		test "$((nr % 20))" -eq 0 && creat=touch
> -		$creat "${name}/$(printf "%.08d" "$nr")"
> +	for ((nr = 0; ; nr += incr)); do
> +		$here/src/popdir.pl --dir "${name}" --start "${nr}" --end "$((nr + incr - 1))"
> +
>  		# Extent count checks use data blocks only to avoid the removal
>  		# step from removing dabtree index blocks and reducing the
>  		# number of extents below the required threshold.
> -		if [ "$((nr % 40))" -eq 0 ]; then
> -			local nextents="$(xfs_bmap ${name} | grep -v hole | wc -l)"
> -			[ "$((nextents - 1))" -gt $max_nextents ] && break
> -		fi
> -		nr=$((nr+1))
> +		local nextents="$(xfs_bmap ${name} | grep -v hole | wc -l)"
> +		[ "$((nextents - 1))" -gt $max_nextents ] && break
>  	done
>  
>  	test -z "${missing}" && return
> -	seq 1 2 "${nr}" | while read d; do
> -		rm -rf "${name}/$(printf "%.08d" "$d")"
> -	done
> +	$here/src/popdir.pl --dir "${name}" --start 1 --incr 2 --end "${nr}" --remove
>  }
>  
>  # Add a bunch of attrs to a file
> @@ -224,9 +220,7 @@ _scratch_xfs_populate() {
>  
>  	# Fill up the root inode chunk
>  	echo "+ fill root ino chunk"
> -	seq 1 64 | while read f; do
> -		$XFS_IO_PROG -f -c "truncate 0" "${SCRATCH_MNT}/dummy${f}"
> -	done
> +	$here/src/popdir.pl --dir "${SCRATCH_MNT}" --start 1 --end 64 --format "dummy%u" --file-mult 1
>  
>  	# Regular files
>  	# - FMT_EXTENTS
> @@ -261,7 +255,7 @@ _scratch_xfs_populate() {
>  
>  	# - BTREE
>  	echo "+ btree dir"
> -	__populate_xfs_create_btree_dir "${SCRATCH_MNT}/S_IFDIR.FMT_BTREE" "$isize" true
> +	__populate_xfs_create_btree_dir "${SCRATCH_MNT}/S_IFDIR.FMT_BTREE" "$isize" "$dblksz" true
>  
>  	# Symlinks
>  	# - FMT_LOCAL
> @@ -340,14 +334,7 @@ _scratch_xfs_populate() {
>  	local rec_per_btblock=16
>  	local nr="$(( 2 * (blksz / rec_per_btblock) * ino_per_rec ))"
>  	local dir="${SCRATCH_MNT}/INOBT"
> -	mkdir -p "${dir}"
> -	seq 0 "${nr}" | while read f; do
> -		touch "${dir}/${f}"
> -	done
> -
> -	seq 0 2 "${nr}" | while read f; do
> -		rm -f "${dir}/${f}"
> -	done
> +	__populate_create_dir "${dir}" "${nr}" true --file-mult 1
>  
>  	# Reverse-mapping btree
>  	is_rmapbt="$(_xfs_has_feature "$SCRATCH_MNT" rmapbt -v)"
> diff --git a/src/popdir.pl b/src/popdir.pl
> new file mode 100755
> index 0000000000..dc0c046b7d
> --- /dev/null
> +++ b/src/popdir.pl
> @@ -0,0 +1,72 @@
> +#!/usr/bin/perl -w
> +
> +# Copyright (c) 2023 Oracle.  All rights reserved.
> +# SPDX-License-Identifier: GPL-2.0
> +#
> +# Create a bunch of files and subdirs in a directory.
> +
> +use Getopt::Long;
> +use File::Basename;
> +
> +$progname=$0;
> +GetOptions("start=i" => \$start,
> +	   "end=i" => \$end,
> +	   "file-mult=i" => \$file_mult,
> +	   "incr=i" => \$incr,
> +	   "format=s" => \$format,
> +	   "dir=s" => \$dir,
> +	   "remove!" => \$remove,
> +	   "help!" => \$help,
> +	   "verbose!" => \$verbose);
> +
> +
> +# check/remove output directory, get filesystem info
> +if (defined $help) {
> +  # newline at end of die message suppresses line number
> +  print STDERR <<"EOF";
> +Usage: $progname [options]
> +Options:
> +  --dir             chdir here before starting
> +  --start=num       create names starting with this number (0)
> +  --incr=num        increment file number by this much (1)
> +  --end=num         stop at this file number (100)
> +  --file-mult       create a regular file when file number is a multiple
> +                    of this quantity (20)
> +  --remove          remove instead of creating
> +  --format=str      printf formatting string for file name ("%08d")
> +  --verbose         verbose output
> +  --help            this help screen
> +EOF
> +  exit(1) unless defined $help;
> +  # otherwise...
> +  exit(0);
> +}
> +
> +if (defined $dir) {
> +	chdir($dir) or die("chdir $dir");
> +}
> +$start = 0 if (!defined $start);
> +$end = 100 if (!defined $end);
> +$file_mult = 20 if (!defined $file_mult);
> +$format = "%08d" if (!defined $format);
> +$incr = 1 if (!defined $incr);
> +
> +for ($i = $start; $i <= $end; $i += $incr) {
> +	$fname = sprintf($format, $i);
> +
> +	if ($remove) {
> +		$verbose && print "rm $fname\n";
> +		unlink($fname) or rmdir($fname) or die("unlink $fname");
> +	} elsif ($file_mult == 0 or ($i % $file_mult) == 0) {
> +		# create a file
> +		$verbose && print "touch $fname\n";
> +		open(DONTCARE, ">$fname") or die("touch $fname");
> +		close(DONTCARE);
> +	} else {
> +		# create a subdir
> +		$verbose && print "mkdir $fname\n";
> +		mkdir($fname, 0755) or die("mkdir $fname");
> +	}
> +}
> +
> +exit(0);
> 

Re: [PATCH 3/4] populate: improve attr creation runtime

[Zorro Lang]

On Tue, Jan 17, 2023 at 04:44:18PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Replace the file creation loops with a python script that does
> everything we want from a single process.  This reduces the runtime of
> _scratch_xfs_populate substantially by avoiding thousands of execve
> overhead.  This patch builds on the previous one by reducing the runtime
> of xfs/349 from ~45s to ~15s.
> 
> For people who don't have python3, use setfattr's "restore" mode to bulk
> create xattrs.  This reduces runtime to about ~25s.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---

Thanks for making a fallback if there's not python3. The python logic and
that fallback logic all look good to me.

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

Thanks,
Zorro

>  common/populate |   22 +++++++++++++++++---
>  src/popattr.py  |   62 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 81 insertions(+), 3 deletions(-)
>  create mode 100755 src/popattr.py
> 
> 
> diff --git a/common/populate b/common/populate
> index 180540aedd..f34551d272 100644
> --- a/common/populate
> +++ b/common/populate
> @@ -12,6 +12,10 @@ _require_populate_commands() {
>  	_require_xfs_io_command "fpunch"
>  	_require_test_program "punch-alternating"
>  	_require_test_program "popdir.pl"
> +	if [ -n "${PYTHON3_PROG}" ]; then
> +		_require_command $PYTHON3_PROG python3
> +		_require_test_program "popattr.py"
> +	fi
>  	case "${FSTYP}" in
>  	"xfs")
>  		_require_command "$XFS_DB_PROG" "xfs_db"
> @@ -108,9 +112,21 @@ __populate_create_attr() {
>  	missing="$3"
>  
>  	touch "${name}"
> -	seq 0 "${nr}" | while read d; do
> -		setfattr -n "user.$(printf "%.08d" "$d")" -v "$(printf "%.08d" "$d")" "${name}"
> -	done
> +
> +	if [ -n "${PYTHON3_PROG}" ]; then
> +		${PYTHON3_PROG} $here/src/popattr.py --file "${name}" --end "${nr}"
> +
> +		test -z "${missing}" && return
> +		${PYTHON3_PROG} $here/src/popattr.py --file "${name}" --start 1 --incr 2 --end "${nr}" --remove
> +		return
> +	fi
> +
> +	# Simulate a getfattr dump file so we can bulk-add attrs.
> +	(
> +		echo "# file: ${name}";
> +		seq --format "user.%08g=\"abcdefgh\"" 0 "${nr}"
> +		echo
> +	) | setfattr --restore -
>  
>  	test -z "${missing}" && return
>  	seq 1 2 "${nr}" | while read d; do
> diff --git a/src/popattr.py b/src/popattr.py
> new file mode 100755
> index 0000000000..397ced9d33
> --- /dev/null
> +++ b/src/popattr.py
> @@ -0,0 +1,62 @@
> +#!/usr/bin/python3
> +
> +# Copyright (c) 2023 Oracle.  All rights reserved.
> +# SPDX-License-Identifier: GPL-2.0
> +#
> +# Create a bunch of xattrs in a file.
> +
> +import argparse
> +import sys
> +import os
> +
> +parser = argparse.ArgumentParser(description = 'Mass create xattrs in a file')
> +parser.add_argument(
> +	'--file', required = True, type = str, help = 'manipulate this file')
> +parser.add_argument(
> +	'--start', type = int, default = 0,
> +	help = 'create xattrs starting with this number')
> +parser.add_argument(
> +	'--incr', type = int, default = 1,
> +	help = 'increment attr number by this much')
> +parser.add_argument(
> +	'--end', type = int, default = 1000,
> +	help = 'stop at this attr number')
> +parser.add_argument(
> +	'--remove', dest = 'remove', action = 'store_true',
> +	help = 'remove instead of creating')
> +parser.add_argument(
> +	'--format', type = str, default = '%08d',
> +	help = 'printf formatting string for attr name')
> +parser.add_argument(
> +	'--verbose', dest = 'verbose', action = 'store_true',
> +	help = 'verbose output')
> +
> +args = parser.parse_args()
> +
> +fmtstring = "user.%s" % args.format
> +
> +# If we are passed a regular file, open it as a proper file descriptor and
> +# pass that around for speed.  Otherwise, we pass the path.
> +fp = None
> +try:
> +	fp = open(args.file, 'r')
> +	fd = fp.fileno()
> +	os.listxattr(fd)
> +	if args.verbose:
> +		print("using fd calls")
> +except:
> +	if args.verbose:
> +		print("using path calls")
> +	fd = args.file
> +
> +for i in range(args.start, args.end + 1, args.incr):
> +	fname = fmtstring % i
> +
> +	if args.remove:
> +		if args.verbose:
> +			print("removexattr %s" % fname)
> +		os.removexattr(fd, fname)
> +	else:
> +		if args.verbose:
> +			print("setxattr %s" % fname)
> +		os.setxattr(fd, fname, b'abcdefgh')
> 

Re: [PATCH 4/4] populate: improve runtime of __populate_fill_fs

[Zorro Lang]

On Tue, Jan 17, 2023 at 04:44:33PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Run the copy loop in parallel to reduce runtime.  If filling the
> populated fs is selected (which it isn't by default in xfs/349), this
> reduces the runtime from ~18s to ~15s, since it's only making enough
> copies to reduce the free space by 5%.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  common/populate |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> 
> diff --git a/common/populate b/common/populate
> index f34551d272..1c3c28463f 100644
> --- a/common/populate
> +++ b/common/populate
> @@ -151,8 +151,9 @@ __populate_fill_fs() {
>  	echo "FILL FS"
>  	echo "src_sz $SRC_SZ fs_sz $FS_SZ nr $NR"
>  	seq 2 "${NR}" | while read nr; do
> -		cp -pRdu "${dir}/test/1" "${dir}/test/${nr}"
> +		cp -pRdu "${dir}/test/1" "${dir}/test/${nr}" &
>  	done
> +	wait

I'm thinking about what'll happen if we do "Ctrl+c" on a running test which
is waiting for these cp operations.

>  }
>  
>  # For XFS, force on all the quota options if quota is enabled
> 

Re: [PATCH 4/4] populate: improve runtime of __populate_fill_fs

[Darrick J. Wong]

On Wed, Jan 18, 2023 at 11:54:03PM +0800, Zorro Lang wrote:
> On Tue, Jan 17, 2023 at 04:44:33PM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > Run the copy loop in parallel to reduce runtime.  If filling the
> > populated fs is selected (which it isn't by default in xfs/349), this
> > reduces the runtime from ~18s to ~15s, since it's only making enough
> > copies to reduce the free space by 5%.
> > 
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > ---
> >  common/populate |    3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > 
> > diff --git a/common/populate b/common/populate
> > index f34551d272..1c3c28463f 100644
> > --- a/common/populate
> > +++ b/common/populate
> > @@ -151,8 +151,9 @@ __populate_fill_fs() {
> >  	echo "FILL FS"
> >  	echo "src_sz $SRC_SZ fs_sz $FS_SZ nr $NR"
> >  	seq 2 "${NR}" | while read nr; do
> > -		cp -pRdu "${dir}/test/1" "${dir}/test/${nr}"
> > +		cp -pRdu "${dir}/test/1" "${dir}/test/${nr}" &
> >  	done
> > +	wait
> 
> I'm thinking about what'll happen if we do "Ctrl+c" on a running test which
> is waiting for these cp operations.

Hmm.  In the context of fstests running on a system with systemd, we run
each test within a systemd scope and kill the scope when the test script
exits.  That will tear down unclaimed background processes, but it's not
a hard and fast guarantee that everyone has systemd.

As for *general* bashisms, I guess the only solution is:

trap 'pkill -P $$' INT TERM QUIT EXIT

To kill all the children of the test script.  Maybe we want that?  But I
hate wrapping my brain around bash child process management, so yuck.

I'll drop the parallel populate work, it's creating a lot of problems
that I don't have time to solve while delivering only modest gains.

--D

> >  }
> >  
> >  # For XFS, force on all the quota options if quota is enabled
> > 
> 

Re: [PATCH 4/4] populate: improve runtime of __populate_fill_fs

[Zorro Lang]

On Wed, Jan 18, 2023 at 11:22:15AM -0800, Darrick J. Wong wrote:
> On Wed, Jan 18, 2023 at 11:54:03PM +0800, Zorro Lang wrote:
> > On Tue, Jan 17, 2023 at 04:44:33PM -0800, Darrick J. Wong wrote:
> > > From: Darrick J. Wong <djwong@kernel.org>
> > > 
> > > Run the copy loop in parallel to reduce runtime.  If filling the
> > > populated fs is selected (which it isn't by default in xfs/349), this
> > > reduces the runtime from ~18s to ~15s, since it's only making enough
> > > copies to reduce the free space by 5%.
> > > 
> > > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > > ---
> > >  common/populate |    3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > 
> > > 
> > > diff --git a/common/populate b/common/populate
> > > index f34551d272..1c3c28463f 100644
> > > --- a/common/populate
> > > +++ b/common/populate
> > > @@ -151,8 +151,9 @@ __populate_fill_fs() {
> > >  	echo "FILL FS"
> > >  	echo "src_sz $SRC_SZ fs_sz $FS_SZ nr $NR"
> > >  	seq 2 "${NR}" | while read nr; do
> > > -		cp -pRdu "${dir}/test/1" "${dir}/test/${nr}"
> > > +		cp -pRdu "${dir}/test/1" "${dir}/test/${nr}" &
> > >  	done
> > > +	wait
> > 
> > I'm thinking about what'll happen if we do "Ctrl+c" on a running test which
> > is waiting for these cp operations.
> 
> Hmm.  In the context of fstests running on a system with systemd, we run
> each test within a systemd scope and kill the scope when the test script
> exits.  That will tear down unclaimed background processes, but it's not
> a hard and fast guarantee that everyone has systemd.
> 
> As for *general* bashisms, I guess the only solution is:
> 
> trap 'pkill -P $$' INT TERM QUIT EXIT
> 
> To kill all the children of the test script.  Maybe we want that?  But I
> hate wrapping my brain around bash child process management, so yuck.
> 
> I'll drop the parallel populate work, it's creating a lot of problems
> that I don't have time to solve while delivering only modest gains.

Yeah, that makes things become complex. So I think if above change can bring
in big performance improvement, we can do that (or use another way to do that,
e.g. an independent program which main process can deal with its children).
If the improvement is not obvious, I'd like not to bring in too many
multi-bash-processes in common helper. What do you think?

Thanks,
Zorro

> 
> --D
> 
> > >  }
> > >  
> > >  # For XFS, force on all the quota options if quota is enabled
> > > 
> > 
> 

Re: [PATCH 4/4] populate: improve runtime of __populate_fill_fs

[Darrick J. Wong]

On Thu, Jan 19, 2023 at 01:04:06PM +0800, Zorro Lang wrote:
> On Wed, Jan 18, 2023 at 11:22:15AM -0800, Darrick J. Wong wrote:
> > On Wed, Jan 18, 2023 at 11:54:03PM +0800, Zorro Lang wrote:
> > > On Tue, Jan 17, 2023 at 04:44:33PM -0800, Darrick J. Wong wrote:
> > > > From: Darrick J. Wong <djwong@kernel.org>
> > > > 
> > > > Run the copy loop in parallel to reduce runtime.  If filling the
> > > > populated fs is selected (which it isn't by default in xfs/349), this
> > > > reduces the runtime from ~18s to ~15s, since it's only making enough
> > > > copies to reduce the free space by 5%.
> > > > 
> > > > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > > > ---
> > > >  common/populate |    3 ++-
> > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > > 
> > > > 
> > > > diff --git a/common/populate b/common/populate
> > > > index f34551d272..1c3c28463f 100644
> > > > --- a/common/populate
> > > > +++ b/common/populate
> > > > @@ -151,8 +151,9 @@ __populate_fill_fs() {
> > > >  	echo "FILL FS"
> > > >  	echo "src_sz $SRC_SZ fs_sz $FS_SZ nr $NR"
> > > >  	seq 2 "${NR}" | while read nr; do
> > > > -		cp -pRdu "${dir}/test/1" "${dir}/test/${nr}"
> > > > +		cp -pRdu "${dir}/test/1" "${dir}/test/${nr}" &
> > > >  	done
> > > > +	wait
> > > 
> > > I'm thinking about what'll happen if we do "Ctrl+c" on a running test which
> > > is waiting for these cp operations.
> > 
> > Hmm.  In the context of fstests running on a system with systemd, we run
> > each test within a systemd scope and kill the scope when the test script
> > exits.  That will tear down unclaimed background processes, but it's not
> > a hard and fast guarantee that everyone has systemd.
> > 
> > As for *general* bashisms, I guess the only solution is:
> > 
> > trap 'pkill -P $$' INT TERM QUIT EXIT
> > 
> > To kill all the children of the test script.  Maybe we want that?  But I
> > hate wrapping my brain around bash child process management, so yuck.
> > 
> > I'll drop the parallel populate work, it's creating a lot of problems
> > that I don't have time to solve while delivering only modest gains.
> 
> Yeah, that makes things become complex. So I think if above change can bring
> in big performance improvement, we can do that (or use another way to do that,
> e.g. an independent program which main process can deal with its children).
> If the improvement is not obvious, I'd like not to bring in too many
> multi-bash-processes in common helper. What do you think?

It's easier to drop the multi subprocess complexity, so I'll do that. :)

Most of the speedup was from algorithmic improvement, not throwing more
CPUs at the problem.

--D

> Thanks,
> Zorro
> 
> > 
> > --D
> > 
> > > >  }
> > > >  
> > > >  # For XFS, force on all the quota options if quota is enabled
> > > > 
> > > 
> > 
>