[PATCH 0/3] fstests: filesystem population fixes

[PATCH 0/3] fstests: filesystem population fixes

[Dave Chinner]

Hi folks,

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.

Cheers,

Dave.

[PATCH 1/3] populate: fix horrible performance due to excessive forking

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

xfs/155 is taking close on 4 minutes to populate the filesystem,
and most of that is because the populate functions are coded without
consideration of performance.

Most of the operations can be executed in parallel as the operate on
separate files or in separate directories.

Creating a zero length file in a shell script can be very fast if we
do the creation within the shell, but running touch, xfs_io or some
other process to create the file is extremely slow - performance is
limited by the process creation/destruction rate, not the filesystem
create rate. Same goes for unlinking files.

We can use 'echo -n > $file' to create or truncate an existing file
to zero length from within the shell. This is much, much faster than
calling touch.

For removing lots of files, there is no shell built in to do this
without forking, but we can easily build a file list and pipe it
to 'xargs rm -f' to execute rm with as many files as possible in one
execution.

Doing this removes approximately 50,000 process creat/destroy cycles
to populate the filesystem, reducing system time from ~200s to ~35s
to populate the filesystem. Along with running operations in
parallel, this brings the population time down from ~235s to less
than 45s.

The long tail of that 45s runtime time is the btree format attribute
tree create. That executes setfattr a very large number of times,
taking 44s to run and consuming 36s of system time mostly just
creating and destroying thousands of setfattr process contexts.
There's no easy shell coding solution to that issue, so that's for
another rainy day.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 common/populate | 179 ++++++++++++++++++++++++++++--------------------
 1 file changed, 104 insertions(+), 75 deletions(-)

diff --git a/common/populate b/common/populate
index 44b4af166..9b60fa5c1 100644
--- a/common/populate
+++ b/common/populate
@@ -52,23 +52,64 @@ __populate_fragment_file() {
 	test -f "${fname}" && $here/src/punch-alternating "${fname}"
 }
 
-# Create a large directory
-__populate_create_dir() {
-	name="$1"
-	nr="$2"
-	missing="$3"
+# Create a specified number of files or until the maximum extent count is
+# reached. If the extent count is reached, return the number of files created.
+# This is optimised for speed - do not add anything that executes a separate
+# process in every loop as this will slow it down by a factor of at least 5.
+__populate_create_nfiles() {
+	local name="$1"
+	local nr="$2"
+	local max_nextents="$3"
+	local d=0
 
 	mkdir -p "${name}"
-	seq 0 "${nr}" | while read d; do
-		creat=mkdir
-		test "$((d % 20))" -eq 0 && creat=touch
-		$creat "${name}/$(printf "%.08d" "$d")"
+	for d in `seq 0 "${nr}"`; do
+		local fname=""
+		printf -v fname "${name}/%.08d" "$d"
+
+		if [ "$((d % 20))" -eq 0 ]; then
+			mkdir ${fname}
+		else
+			echo -n > ${fname}
+		fi
+
+		if [ "${max_nextents}" -eq 0 ]; then
+			continue
+		fi
+		if [ "$((d % 40))" -ne 0 ]; then
+			continue
+		fi
+
+		local nextents="$(_xfs_get_fsxattr nextents $name)"
+		if [ "${nextents}" -gt "${max_nextents}" ]; then
+			echo ${d}
+			break
+		fi
 	done
+}
+
+# remove every second file in the given directory. This is optimised for speed -
+# do not add anything that executes a separate process in each loop as this will
+# slow it down by at least factor of 10.
+__populate_remove_nfiles() {
+	local name="$1"
+	local nr="$2"
+	local d=1
+
+	for d in `seq 1 2 "${nr}"`; do
+		printf "${name}/%.08d " "$d"
+	done | xargs rm -f
+}
 
+# Create a large directory
+__populate_create_dir() {
+	local name="$1"
+	local nr="$2"
+	local missing="$3"
+
+	__populate_create_nfiles "${name}" "${nr}" 0
 	test -z "${missing}" && return
-	seq 1 2 "${nr}" | while read d; do
-		rm -rf "${name}/$(printf "%.08d" "$d")"
-	done
+	__populate_remove_nfiles "${name}" "${nr}"
 }
 
 # Create a large directory and ensure that it's a btree format
@@ -82,31 +123,18 @@ __populate_xfs_create_btree_dir() {
 	# watch for when the extent count exceeds the space after the
 	# inode core.
 	local max_nextents="$(((isize - icore_size) / 16))"
-	local nr=0
-
-	mkdir -p "${name}"
-	while true; do
-		local creat=mkdir
-		test "$((nr % 20))" -eq 0 && creat=touch
-		$creat "${name}/$(printf "%.08d" "$nr")"
-		if [ "$((nr % 40))" -eq 0 ]; then
-			local nextents="$(_xfs_get_fsxattr nextents $name)"
-			[ $nextents -gt $max_nextents ] && break
-		fi
-		nr=$((nr+1))
-	done
+	local nr=100000
 
+	nr=$(__populate_create_nfiles "${name}" "${nr}" "${max_nextents}")
 	test -z "${missing}" && return
-	seq 1 2 "${nr}" | while read d; do
-		rm -rf "${name}/$(printf "%.08d" "$d")"
-	done
+	__populate_remove_nfiles "${name}" "${nr}"
 }
 
 # Add a bunch of attrs to a file
 __populate_create_attr() {
-	name="$1"
-	nr="$2"
-	missing="$3"
+	local name="$1"
+	local nr="$2"
+	local missing="$3"
 
 	touch "${name}"
 	seq 0 "${nr}" | while read d; do
@@ -121,17 +149,18 @@ __populate_create_attr() {
 
 # Fill up some percentage of the remaining free space
 __populate_fill_fs() {
-	dir="$1"
-	pct="$2"
+	local dir="$1"
+	local pct="$2"
+	local nr=0
 	test -z "${pct}" && pct=60
 
 	mkdir -p "${dir}/test/1"
 	cp -pRdu "${dir}"/S_IFREG* "${dir}/test/1/"
 
-	SRC_SZ="$(du -ks "${dir}/test/1" | cut -f 1)"
-	FS_SZ="$(( $(stat -f "${dir}" -c '%a * %S') / 1024 ))"
+	local SRC_SZ="$(du -ks "${dir}/test/1" | cut -f 1)"
+	local FS_SZ="$(( $(stat -f "${dir}" -c '%a * %S') / 1024 ))"
 
-	NR="$(( (FS_SZ * ${pct} / 100) / SRC_SZ ))"
+	local NR="$(( (FS_SZ * ${pct} / 100) / SRC_SZ ))"
 
 	echo "FILL FS"
 	echo "src_sz $SRC_SZ fs_sz $FS_SZ nr $NR"
@@ -220,45 +249,45 @@ _scratch_xfs_populate() {
 	# Data:
 
 	# Fill up the root inode chunk
-	echo "+ fill root ino 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
+		echo -n > "${SCRATCH_MNT}/dummy${f}"
+	done ) &
 
 	# Regular files
 	# - FMT_EXTENTS
 	echo "+ extents file"
-	__populate_create_file $blksz "${SCRATCH_MNT}/S_IFREG.FMT_EXTENTS"
+	__populate_create_file $blksz "${SCRATCH_MNT}/S_IFREG.FMT_EXTENTS" &
 
 	# - FMT_BTREE
 	echo "+ btree extents file"
 	nr="$((blksz * 2 / 16))"
-	__populate_create_file $((blksz * nr)) "${SCRATCH_MNT}/S_IFREG.FMT_BTREE"
+	__populate_create_file $((blksz * nr)) "${SCRATCH_MNT}/S_IFREG.FMT_BTREE" &
 
 	# Directories
 	# - INLINE
-	echo "+ inline dir"
-	__populate_create_dir "${SCRATCH_MNT}/S_IFDIR.FMT_INLINE" 1
+	 echo "+ inline dir"
+	__populate_create_dir "${SCRATCH_MNT}/S_IFDIR.FMT_INLINE" 1 "" &
 
 	# - BLOCK
 	echo "+ block dir"
-	__populate_create_dir "${SCRATCH_MNT}/S_IFDIR.FMT_BLOCK" "$((dblksz / 40))"
+	__populate_create_dir "${SCRATCH_MNT}/S_IFDIR.FMT_BLOCK" "$((dblksz / 40))" "" &
 
 	# - LEAF
 	echo "+ leaf dir"
-	__populate_create_dir "${SCRATCH_MNT}/S_IFDIR.FMT_LEAF" "$((dblksz / 12))"
+	__populate_create_dir "${SCRATCH_MNT}/S_IFDIR.FMT_LEAF" "$((dblksz / 12))" "" &
 
 	# - LEAFN
 	echo "+ leafn dir"
-	__populate_create_dir "${SCRATCH_MNT}/S_IFDIR.FMT_LEAFN" "$(( ((dblksz - leaf_hdr_size) / 8) - 3 ))"
+	__populate_create_dir "${SCRATCH_MNT}/S_IFDIR.FMT_LEAFN" "$(( ((dblksz - leaf_hdr_size) / 8) - 3 ))" "" &
 
 	# - NODE
 	echo "+ node dir"
-	__populate_create_dir "${SCRATCH_MNT}/S_IFDIR.FMT_NODE" "$((16 * dblksz / 40))" true
+	__populate_create_dir "${SCRATCH_MNT}/S_IFDIR.FMT_NODE" "$((16 * dblksz / 40))" true &
 
 	# - 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" true &
 
 	# Symlinks
 	# - FMT_LOCAL
@@ -280,20 +309,20 @@ _scratch_xfs_populate() {
 
 	# Attribute formats
 	# LOCAL
-	echo "+ local attr"
-	__populate_create_attr "${SCRATCH_MNT}/ATTR.FMT_LOCAL" 1
+	 echo "+ local attr"
+	__populate_create_attr "${SCRATCH_MNT}/ATTR.FMT_LOCAL" 1 "" &
 
 	# LEAF
-	echo "+ leaf attr"
-	__populate_create_attr "${SCRATCH_MNT}/ATTR.FMT_LEAF" "$((blksz / 40))"
+	 echo "+ leaf attr"
+	__populate_create_attr "${SCRATCH_MNT}/ATTR.FMT_LEAF" "$((blksz / 40))" "" &
 
 	# NODE
 	echo "+ node attr"
-	__populate_create_attr "${SCRATCH_MNT}/ATTR.FMT_NODE" "$((8 * blksz / 40))"
+	__populate_create_attr "${SCRATCH_MNT}/ATTR.FMT_NODE" "$((8 * blksz / 40))" "" &
 
 	# BTREE
 	echo "+ btree attr"
-	__populate_create_attr "${SCRATCH_MNT}/ATTR.FMT_BTREE" "$((64 * blksz / 40))" true
+	__populate_create_attr "${SCRATCH_MNT}/ATTR.FMT_BTREE" "$((64 * blksz / 40))" true &
 
 	# trusted namespace
 	touch ${SCRATCH_MNT}/ATTR.TRUSTED
@@ -321,68 +350,68 @@ _scratch_xfs_populate() {
 	rm -rf "${SCRATCH_MNT}/attrvalfile"
 
 	# Make an unused inode
-	echo "+ empty file"
+	( echo "+ empty file"
 	touch "${SCRATCH_MNT}/unused"
 	$XFS_IO_PROG -f -c 'fsync' "${SCRATCH_MNT}/unused"
-	rm -rf "${SCRATCH_MNT}/unused"
+	rm -rf "${SCRATCH_MNT}/unused" ) &
 
 	# Free space btree
 	echo "+ freesp btree"
 	nr="$((blksz * 2 / 8))"
-	__populate_create_file $((blksz * nr)) "${SCRATCH_MNT}/BNOBT"
+	__populate_create_file $((blksz * nr)) "${SCRATCH_MNT}/BNOBT" &
 
 	# Inode btree
-	echo "+ inobt btree"
+	( echo "+ inobt btree"
 	local ino_per_rec=64
 	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 "${SCRATCH_MNT}/INOBT" "${nr}" true
+	) &
 
 	# Reverse-mapping btree
 	is_rmapbt="$(_xfs_has_feature "$SCRATCH_MNT" rmapbt -v)"
 	if [ $is_rmapbt -gt 0 ]; then
-		echo "+ rmapbt btree"
+		( echo "+ rmapbt btree"
 		nr="$((blksz * 2 / 24))"
 		__populate_create_file $((blksz * nr)) "${SCRATCH_MNT}/RMAPBT"
+		) &
 	fi
 
 	# Realtime Reverse-mapping btree
 	is_rt="$(_xfs_get_rtextents "$SCRATCH_MNT")"
 	if [ $is_rmapbt -gt 0 ] && [ $is_rt -gt 0 ]; then
-		echo "+ rtrmapbt btree"
+		( echo "+ rtrmapbt btree"
 		nr="$((blksz * 2 / 32))"
 		$XFS_IO_PROG -R -f -c 'truncate 0' "${SCRATCH_MNT}/RTRMAPBT"
 		__populate_create_file $((blksz * nr)) "${SCRATCH_MNT}/RTRMAPBT"
+		) &
 	fi
 
 	# Reference-count btree
 	is_reflink="$(_xfs_has_feature "$SCRATCH_MNT" reflink -v)"
 	if [ $is_reflink -gt 0 ]; then
-		echo "+ reflink btree"
+		( echo "+ reflink btree"
 		nr="$((blksz * 2 / 12))"
 		__populate_create_file $((blksz * nr)) "${SCRATCH_MNT}/REFCOUNTBT"
 		cp --reflink=always "${SCRATCH_MNT}/REFCOUNTBT" "${SCRATCH_MNT}/REFCOUNTBT2"
+		) &
 	fi
 
 	# Copy some real files (xfs tests, I guess...)
 	echo "+ real files"
 	test $fill -ne 0 && __populate_fill_fs "${SCRATCH_MNT}" 5
 
-	# Make sure we get all the fragmentation we asked for
-	__populate_fragment_file "${SCRATCH_MNT}/S_IFREG.FMT_BTREE"
-	__populate_fragment_file "${SCRATCH_MNT}/BNOBT"
-	__populate_fragment_file "${SCRATCH_MNT}/RMAPBT"
-	__populate_fragment_file "${SCRATCH_MNT}/RTRMAPBT"
-	__populate_fragment_file "${SCRATCH_MNT}/REFCOUNTBT"
+	# Wait for all file creation to complete before we start fragmenting
+	# the files as needed.
+	wait
+	__populate_fragment_file "${SCRATCH_MNT}/S_IFREG.FMT_BTREE" &
+	__populate_fragment_file "${SCRATCH_MNT}/BNOBT" &
+	__populate_fragment_file "${SCRATCH_MNT}/RMAPBT" &
+	__populate_fragment_file "${SCRATCH_MNT}/RTRMAPBT" &
+	__populate_fragment_file "${SCRATCH_MNT}/REFCOUNTBT" &
 
+	wait
 	umount "${SCRATCH_MNT}"
 }
 
-- 
2.38.1

[PATCH 2/3] populate: ensure btree directories are created reliably

[Dave Chinner]

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>
---
 common/populate | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/common/populate b/common/populate
index 9b60fa5c1..7b5b16fb8 100644
--- a/common/populate
+++ b/common/populate
@@ -80,8 +80,11 @@ __populate_create_nfiles() {
 			continue
 		fi
 
-		local nextents="$(_xfs_get_fsxattr nextents $name)"
-		if [ "${nextents}" -gt "${max_nextents}" ]; then
+		# 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.
+		local nextents="$(xfs_bmap ${name} |grep -v hole | wc -l)"
+		if [ "$((nextents - 1))" -gt "${max_nextents}" ]; then
 			echo ${d}
 			break
 		fi
-- 
2.38.1

[PATCH 3/3] xfs/294: performance is unreasonably slow

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

This creates a bunch of files in a dir, then deletes 97% of them
attempting to leave 1 allocated inode per inode chunk so that they
aren't freed. Performance is badly limited by task creation and
destruction for each inode created. Fix this by using "echo -n >
file" rather than touch so that the shell creates the empty files
without needing to fork/exec a separate task for each creation.

This reduces runtime from 45s down to 15s.

Also add more debug with inode counts and internal superblock
counter information for determining why this test may ENOSPC on the
final creation loop.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 tests/xfs/294 | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/tests/xfs/294 b/tests/xfs/294
index d381e2c85..1ce0d1cc5 100755
--- a/tests/xfs/294
+++ b/tests/xfs/294
@@ -28,6 +28,13 @@ _require_test_program "punch-alternating"
 _require_xfs_io_command "falloc"
 _require_xfs_io_command "fpunch"
 
+dump_freespace()
+{
+	df $SCRATCH_MNT
+	df -i $SCRATCH_MNT
+	$XFS_IO_PROG -rc "statfs -c" $SCRATCH_MNT
+}
+
 # We want to mkfs with a very specific geometry
 MKFS_OPTIONS=""
 _scratch_mkfs "-d size=512m -n size=8192 -i size=1024" >> $seqres.full 2>&1 \
@@ -37,7 +44,7 @@ _scratch_mount
 # Make a ton of mostly-empty inode clusters so we can always
 # make more inodes
 mkdir $SCRATCH_MNT/tmp
-for I in `seq 1 10000`; do touch $SCRATCH_MNT/tmp/$I; done
+for I in `seq 1 10000`; do echo -n > $SCRATCH_MNT/tmp/$I; done
 
 # These mostly-empty clusters will live here:
 mkdir $SCRATCH_MNT/clusters
@@ -50,7 +57,7 @@ rm -rf $SCRATCH_MNT/tmp
 mkdir $SCRATCH_MNT/testdir
 # roughly 20 chars per file
 for I in `seq 1 100`; do
-	touch $SCRATCH_MNT/testdir/12345678901234567890$I;
+	echo -n > $SCRATCH_MNT/testdir/12345678901234567890$I;
 done
 
 # File to fragment:
@@ -63,7 +70,7 @@ space=$(stat -f -c '%f * %S * 95 / 100' $SCRATCH_MNT | $BC_PROG)
 $XFS_IO_PROG -f -c "falloc 0 $space" $SCRATCH_MNT/fillfile ||
 	_fail "Could not allocate space"
 
-df -h $SCRATCH_MNT >> $seqres.full 2>&1
+dump_freespace >> $seqres.full 2>&1
 
 # Fill remaining space; let this run to failure
 dd if=/dev/zero of=$SCRATCH_MNT/spacefile1 oflag=direct >> $seqres.full 2>&1
@@ -75,12 +82,16 @@ $here/src/punch-alternating $SCRATCH_MNT/fragfile >> $seqres.full 2>&1
 # (and then some for good measure)
 dd conv=fsync if=/dev/zero of=$SCRATCH_MNT/spacefile2 bs=1M count=64 >> $seqres.full 2>&1
 
+dump_freespace >> $seqres.full 2>&1
+
 # Now populate the directory so that it must allocate these
 # fragmented blocks
 for I in `seq 1 1400`; do
-	touch $SCRATCH_MNT/testdir/12345678901234567890$I;
+	echo -n > $SCRATCH_MNT/testdir/12345678901234567890$I;
 done
 
+dump_freespace >> $seqres.full 2>&1
+
 # Now traverse that ugly thing!
 find $SCRATCH_MNT/testdir | sort | _filter_scratch | md5sum
 
-- 
2.38.1

Re: [PATCH 2/3] populate: ensure btree directories are created reliably

[Darrick J. Wong]

On Wed, Jan 11, 2023 at 09:49:05AM +1100, Dave Chinner wrote:
> 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>
> ---
>  common/populate | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/common/populate b/common/populate
> index 9b60fa5c1..7b5b16fb8 100644
> --- a/common/populate
> +++ b/common/populate
> @@ -80,8 +80,11 @@ __populate_create_nfiles() {
>  			continue
>  		fi
>  
> -		local nextents="$(_xfs_get_fsxattr nextents $name)"
> -		if [ "${nextents}" -gt "${max_nextents}" ]; then
> +		# 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.
> +		local nextents="$(xfs_bmap ${name} |grep -v hole | wc -l)"
> +		if [ "$((nextents - 1))" -gt "${max_nextents}" ]; then

Pretty much the same patch I had in my tree...
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

>  			echo ${d}
>  			break
>  		fi
> -- 
> 2.38.1
> 

Re: [PATCH 1/3] populate: fix horrible performance due to excessive forking

[Darrick J. Wong]

On Wed, Jan 11, 2023 at 09:49:04AM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> xfs/155 is taking close on 4 minutes to populate the filesystem,
> and most of that is because the populate functions are coded without
> consideration of performance.
> 
> Most of the operations can be executed in parallel as the operate on
> separate files or in separate directories.
> 
> Creating a zero length file in a shell script can be very fast if we
> do the creation within the shell, but running touch, xfs_io or some
> other process to create the file is extremely slow - performance is
> limited by the process creation/destruction rate, not the filesystem
> create rate. Same goes for unlinking files.
> 
> We can use 'echo -n > $file' to create or truncate an existing file
> to zero length from within the shell. This is much, much faster than
> calling touch.
> 
> For removing lots of files, there is no shell built in to do this
> without forking, but we can easily build a file list and pipe it
> to 'xargs rm -f' to execute rm with as many files as possible in one
> execution.
> 
> Doing this removes approximately 50,000 process creat/destroy cycles
> to populate the filesystem, reducing system time from ~200s to ~35s
> to populate the filesystem. Along with running operations in
> parallel, this brings the population time down from ~235s to less
> than 45s.

Hmm.  I took the nerdsnipe bait and came up with my own approach.  I
replaced the shell loops with a perl script.  I didn't parallelize
anything, but the perl script cut the runtime down to about ~35s.

> The long tail of that 45s runtime time is the btree format attribute
> tree create. That executes setfattr a very large number of times,
> taking 44s to run and consuming 36s of system time mostly just
> creating and destroying thousands of setfattr process contexts.
> There's no easy shell coding solution to that issue, so that's for
> another rainy day.

...well it's pouring on the west coast here, so I'll post my solution
that uses setfattr --restore tomorrow when I get it back from QA.
Granted, I hadn't found a solution to the removexattr stuff yet, so I
might keep working on that.

(removexattr looks like a pain in perl though...)

Anyway it's late now, I'll look at the diff tomorrow.

--D

> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---
>  common/populate | 179 ++++++++++++++++++++++++++++--------------------
>  1 file changed, 104 insertions(+), 75 deletions(-)
> 
> diff --git a/common/populate b/common/populate
> index 44b4af166..9b60fa5c1 100644
> --- a/common/populate
> +++ b/common/populate
> @@ -52,23 +52,64 @@ __populate_fragment_file() {
>  	test -f "${fname}" && $here/src/punch-alternating "${fname}"
>  }
>  
> -# Create a large directory
> -__populate_create_dir() {
> -	name="$1"
> -	nr="$2"
> -	missing="$3"
> +# Create a specified number of files or until the maximum extent count is
> +# reached. If the extent count is reached, return the number of files created.
> +# This is optimised for speed - do not add anything that executes a separate
> +# process in every loop as this will slow it down by a factor of at least 5.
> +__populate_create_nfiles() {
> +	local name="$1"
> +	local nr="$2"
> +	local max_nextents="$3"
> +	local d=0
>  
>  	mkdir -p "${name}"
> -	seq 0 "${nr}" | while read d; do
> -		creat=mkdir
> -		test "$((d % 20))" -eq 0 && creat=touch
> -		$creat "${name}/$(printf "%.08d" "$d")"
> +	for d in `seq 0 "${nr}"`; do
> +		local fname=""
> +		printf -v fname "${name}/%.08d" "$d"
> +
> +		if [ "$((d % 20))" -eq 0 ]; then
> +			mkdir ${fname}
> +		else
> +			echo -n > ${fname}
> +		fi
> +
> +		if [ "${max_nextents}" -eq 0 ]; then
> +			continue
> +		fi
> +		if [ "$((d % 40))" -ne 0 ]; then
> +			continue
> +		fi
> +
> +		local nextents="$(_xfs_get_fsxattr nextents $name)"
> +		if [ "${nextents}" -gt "${max_nextents}" ]; then
> +			echo ${d}
> +			break
> +		fi
>  	done
> +}
> +
> +# remove every second file in the given directory. This is optimised for speed -
> +# do not add anything that executes a separate process in each loop as this will
> +# slow it down by at least factor of 10.
> +__populate_remove_nfiles() {
> +	local name="$1"
> +	local nr="$2"
> +	local d=1
> +
> +	for d in `seq 1 2 "${nr}"`; do
> +		printf "${name}/%.08d " "$d"
> +	done | xargs rm -f
> +}
>  
> +# Create a large directory
> +__populate_create_dir() {
> +	local name="$1"
> +	local nr="$2"
> +	local missing="$3"
> +
> +	__populate_create_nfiles "${name}" "${nr}" 0
>  	test -z "${missing}" && return
> -	seq 1 2 "${nr}" | while read d; do
> -		rm -rf "${name}/$(printf "%.08d" "$d")"
> -	done
> +	__populate_remove_nfiles "${name}" "${nr}"
>  }
>  
>  # Create a large directory and ensure that it's a btree format
> @@ -82,31 +123,18 @@ __populate_xfs_create_btree_dir() {
>  	# watch for when the extent count exceeds the space after the
>  	# inode core.
>  	local max_nextents="$(((isize - icore_size) / 16))"
> -	local nr=0
> -
> -	mkdir -p "${name}"
> -	while true; do
> -		local creat=mkdir
> -		test "$((nr % 20))" -eq 0 && creat=touch
> -		$creat "${name}/$(printf "%.08d" "$nr")"
> -		if [ "$((nr % 40))" -eq 0 ]; then
> -			local nextents="$(_xfs_get_fsxattr nextents $name)"
> -			[ $nextents -gt $max_nextents ] && break
> -		fi
> -		nr=$((nr+1))
> -	done
> +	local nr=100000
>  
> +	nr=$(__populate_create_nfiles "${name}" "${nr}" "${max_nextents}")
>  	test -z "${missing}" && return
> -	seq 1 2 "${nr}" | while read d; do
> -		rm -rf "${name}/$(printf "%.08d" "$d")"
> -	done
> +	__populate_remove_nfiles "${name}" "${nr}"
>  }
>  
>  # Add a bunch of attrs to a file
>  __populate_create_attr() {
> -	name="$1"
> -	nr="$2"
> -	missing="$3"
> +	local name="$1"
> +	local nr="$2"
> +	local missing="$3"
>  
>  	touch "${name}"
>  	seq 0 "${nr}" | while read d; do
> @@ -121,17 +149,18 @@ __populate_create_attr() {
>  
>  # Fill up some percentage of the remaining free space
>  __populate_fill_fs() {
> -	dir="$1"
> -	pct="$2"
> +	local dir="$1"
> +	local pct="$2"
> +	local nr=0
>  	test -z "${pct}" && pct=60
>  
>  	mkdir -p "${dir}/test/1"
>  	cp -pRdu "${dir}"/S_IFREG* "${dir}/test/1/"
>  
> -	SRC_SZ="$(du -ks "${dir}/test/1" | cut -f 1)"
> -	FS_SZ="$(( $(stat -f "${dir}" -c '%a * %S') / 1024 ))"
> +	local SRC_SZ="$(du -ks "${dir}/test/1" | cut -f 1)"
> +	local FS_SZ="$(( $(stat -f "${dir}" -c '%a * %S') / 1024 ))"
>  
> -	NR="$(( (FS_SZ * ${pct} / 100) / SRC_SZ ))"
> +	local NR="$(( (FS_SZ * ${pct} / 100) / SRC_SZ ))"
>  
>  	echo "FILL FS"
>  	echo "src_sz $SRC_SZ fs_sz $FS_SZ nr $NR"
> @@ -220,45 +249,45 @@ _scratch_xfs_populate() {
>  	# Data:
>  
>  	# Fill up the root inode chunk
> -	echo "+ fill root ino 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
> +		echo -n > "${SCRATCH_MNT}/dummy${f}"
> +	done ) &
>  
>  	# Regular files
>  	# - FMT_EXTENTS
>  	echo "+ extents file"
> -	__populate_create_file $blksz "${SCRATCH_MNT}/S_IFREG.FMT_EXTENTS"
> +	__populate_create_file $blksz "${SCRATCH_MNT}/S_IFREG.FMT_EXTENTS" &
>  
>  	# - FMT_BTREE
>  	echo "+ btree extents file"
>  	nr="$((blksz * 2 / 16))"
> -	__populate_create_file $((blksz * nr)) "${SCRATCH_MNT}/S_IFREG.FMT_BTREE"
> +	__populate_create_file $((blksz * nr)) "${SCRATCH_MNT}/S_IFREG.FMT_BTREE" &
>  
>  	# Directories
>  	# - INLINE
> -	echo "+ inline dir"
> -	__populate_create_dir "${SCRATCH_MNT}/S_IFDIR.FMT_INLINE" 1
> +	 echo "+ inline dir"
> +	__populate_create_dir "${SCRATCH_MNT}/S_IFDIR.FMT_INLINE" 1 "" &
>  
>  	# - BLOCK
>  	echo "+ block dir"
> -	__populate_create_dir "${SCRATCH_MNT}/S_IFDIR.FMT_BLOCK" "$((dblksz / 40))"
> +	__populate_create_dir "${SCRATCH_MNT}/S_IFDIR.FMT_BLOCK" "$((dblksz / 40))" "" &
>  
>  	# - LEAF
>  	echo "+ leaf dir"
> -	__populate_create_dir "${SCRATCH_MNT}/S_IFDIR.FMT_LEAF" "$((dblksz / 12))"
> +	__populate_create_dir "${SCRATCH_MNT}/S_IFDIR.FMT_LEAF" "$((dblksz / 12))" "" &
>  
>  	# - LEAFN
>  	echo "+ leafn dir"
> -	__populate_create_dir "${SCRATCH_MNT}/S_IFDIR.FMT_LEAFN" "$(( ((dblksz - leaf_hdr_size) / 8) - 3 ))"
> +	__populate_create_dir "${SCRATCH_MNT}/S_IFDIR.FMT_LEAFN" "$(( ((dblksz - leaf_hdr_size) / 8) - 3 ))" "" &
>  
>  	# - NODE
>  	echo "+ node dir"
> -	__populate_create_dir "${SCRATCH_MNT}/S_IFDIR.FMT_NODE" "$((16 * dblksz / 40))" true
> +	__populate_create_dir "${SCRATCH_MNT}/S_IFDIR.FMT_NODE" "$((16 * dblksz / 40))" true &
>  
>  	# - 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" true &
>  
>  	# Symlinks
>  	# - FMT_LOCAL
> @@ -280,20 +309,20 @@ _scratch_xfs_populate() {
>  
>  	# Attribute formats
>  	# LOCAL
> -	echo "+ local attr"
> -	__populate_create_attr "${SCRATCH_MNT}/ATTR.FMT_LOCAL" 1
> +	 echo "+ local attr"
> +	__populate_create_attr "${SCRATCH_MNT}/ATTR.FMT_LOCAL" 1 "" &
>  
>  	# LEAF
> -	echo "+ leaf attr"
> -	__populate_create_attr "${SCRATCH_MNT}/ATTR.FMT_LEAF" "$((blksz / 40))"
> +	 echo "+ leaf attr"
> +	__populate_create_attr "${SCRATCH_MNT}/ATTR.FMT_LEAF" "$((blksz / 40))" "" &
>  
>  	# NODE
>  	echo "+ node attr"
> -	__populate_create_attr "${SCRATCH_MNT}/ATTR.FMT_NODE" "$((8 * blksz / 40))"
> +	__populate_create_attr "${SCRATCH_MNT}/ATTR.FMT_NODE" "$((8 * blksz / 40))" "" &
>  
>  	# BTREE
>  	echo "+ btree attr"
> -	__populate_create_attr "${SCRATCH_MNT}/ATTR.FMT_BTREE" "$((64 * blksz / 40))" true
> +	__populate_create_attr "${SCRATCH_MNT}/ATTR.FMT_BTREE" "$((64 * blksz / 40))" true &
>  
>  	# trusted namespace
>  	touch ${SCRATCH_MNT}/ATTR.TRUSTED
> @@ -321,68 +350,68 @@ _scratch_xfs_populate() {
>  	rm -rf "${SCRATCH_MNT}/attrvalfile"
>  
>  	# Make an unused inode
> -	echo "+ empty file"
> +	( echo "+ empty file"
>  	touch "${SCRATCH_MNT}/unused"
>  	$XFS_IO_PROG -f -c 'fsync' "${SCRATCH_MNT}/unused"
> -	rm -rf "${SCRATCH_MNT}/unused"
> +	rm -rf "${SCRATCH_MNT}/unused" ) &
>  
>  	# Free space btree
>  	echo "+ freesp btree"
>  	nr="$((blksz * 2 / 8))"
> -	__populate_create_file $((blksz * nr)) "${SCRATCH_MNT}/BNOBT"
> +	__populate_create_file $((blksz * nr)) "${SCRATCH_MNT}/BNOBT" &
>  
>  	# Inode btree
> -	echo "+ inobt btree"
> +	( echo "+ inobt btree"
>  	local ino_per_rec=64
>  	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 "${SCRATCH_MNT}/INOBT" "${nr}" true
> +	) &
>  
>  	# Reverse-mapping btree
>  	is_rmapbt="$(_xfs_has_feature "$SCRATCH_MNT" rmapbt -v)"
>  	if [ $is_rmapbt -gt 0 ]; then
> -		echo "+ rmapbt btree"
> +		( echo "+ rmapbt btree"
>  		nr="$((blksz * 2 / 24))"
>  		__populate_create_file $((blksz * nr)) "${SCRATCH_MNT}/RMAPBT"
> +		) &
>  	fi
>  
>  	# Realtime Reverse-mapping btree
>  	is_rt="$(_xfs_get_rtextents "$SCRATCH_MNT")"
>  	if [ $is_rmapbt -gt 0 ] && [ $is_rt -gt 0 ]; then
> -		echo "+ rtrmapbt btree"
> +		( echo "+ rtrmapbt btree"
>  		nr="$((blksz * 2 / 32))"
>  		$XFS_IO_PROG -R -f -c 'truncate 0' "${SCRATCH_MNT}/RTRMAPBT"
>  		__populate_create_file $((blksz * nr)) "${SCRATCH_MNT}/RTRMAPBT"
> +		) &
>  	fi
>  
>  	# Reference-count btree
>  	is_reflink="$(_xfs_has_feature "$SCRATCH_MNT" reflink -v)"
>  	if [ $is_reflink -gt 0 ]; then
> -		echo "+ reflink btree"
> +		( echo "+ reflink btree"
>  		nr="$((blksz * 2 / 12))"
>  		__populate_create_file $((blksz * nr)) "${SCRATCH_MNT}/REFCOUNTBT"
>  		cp --reflink=always "${SCRATCH_MNT}/REFCOUNTBT" "${SCRATCH_MNT}/REFCOUNTBT2"
> +		) &
>  	fi
>  
>  	# Copy some real files (xfs tests, I guess...)
>  	echo "+ real files"
>  	test $fill -ne 0 && __populate_fill_fs "${SCRATCH_MNT}" 5
>  
> -	# Make sure we get all the fragmentation we asked for
> -	__populate_fragment_file "${SCRATCH_MNT}/S_IFREG.FMT_BTREE"
> -	__populate_fragment_file "${SCRATCH_MNT}/BNOBT"
> -	__populate_fragment_file "${SCRATCH_MNT}/RMAPBT"
> -	__populate_fragment_file "${SCRATCH_MNT}/RTRMAPBT"
> -	__populate_fragment_file "${SCRATCH_MNT}/REFCOUNTBT"
> +	# Wait for all file creation to complete before we start fragmenting
> +	# the files as needed.
> +	wait
> +	__populate_fragment_file "${SCRATCH_MNT}/S_IFREG.FMT_BTREE" &
> +	__populate_fragment_file "${SCRATCH_MNT}/BNOBT" &
> +	__populate_fragment_file "${SCRATCH_MNT}/RMAPBT" &
> +	__populate_fragment_file "${SCRATCH_MNT}/RTRMAPBT" &
> +	__populate_fragment_file "${SCRATCH_MNT}/REFCOUNTBT" &
>  
> +	wait
>  	umount "${SCRATCH_MNT}"
>  }
>  
> -- 
> 2.38.1
> 

Re: [PATCH 3/3] xfs/294: performance is unreasonably slow

[David Disseldorp]

Hi Dave!

On Wed, 11 Jan 2023 09:49:06 +1100, Dave Chinner wrote:

> From: Dave Chinner <dchinner@redhat.com>
> 
> This creates a bunch of files in a dir, then deletes 97% of them
> attempting to leave 1 allocated inode per inode chunk so that they
> aren't freed. Performance is badly limited by task creation and
> destruction for each inode created. Fix this by using "echo -n >
> file" rather than touch so that the shell creates the empty files
> without needing to fork/exec a separate task for each creation.
> 
> This reduces runtime from 45s down to 15s.

I see >7x improvement on a zram device, be it from a lower baseline :)

Reviewed-by: David Disseldorp <ddiss@suse.de>

Cheers, David

Re: [PATCH 1/3] populate: fix horrible performance due to excessive forking

[Darrick J. Wong]

On Tue, Jan 10, 2023 at 10:02:37PM -0800, Darrick J. Wong wrote:
> On Wed, Jan 11, 2023 at 09:49:04AM +1100, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> > 
> > xfs/155 is taking close on 4 minutes to populate the filesystem,
> > and most of that is because the populate functions are coded without
> > consideration of performance.
> > 
> > Most of the operations can be executed in parallel as the operate on
> > separate files or in separate directories.
> > 
> > Creating a zero length file in a shell script can be very fast if we
> > do the creation within the shell, but running touch, xfs_io or some
> > other process to create the file is extremely slow - performance is
> > limited by the process creation/destruction rate, not the filesystem
> > create rate. Same goes for unlinking files.
> > 
> > We can use 'echo -n > $file' to create or truncate an existing file
> > to zero length from within the shell. This is much, much faster than
> > calling touch.
> > 
> > For removing lots of files, there is no shell built in to do this
> > without forking, but we can easily build a file list and pipe it
> > to 'xargs rm -f' to execute rm with as many files as possible in one
> > execution.
> > 
> > Doing this removes approximately 50,000 process creat/destroy cycles
> > to populate the filesystem, reducing system time from ~200s to ~35s
> > to populate the filesystem. Along with running operations in
> > parallel, this brings the population time down from ~235s to less
> > than 45s.
> 
> Hmm.  I took the nerdsnipe bait and came up with my own approach.  I
> replaced the shell loops with a perl script.  I didn't parallelize
> anything, but the perl script cut the runtime down to about ~35s.
> 
> > The long tail of that 45s runtime time is the btree format attribute
> > tree create. That executes setfattr a very large number of times,
> > taking 44s to run and consuming 36s of system time mostly just
> > creating and destroying thousands of setfattr process contexts.
> > There's no easy shell coding solution to that issue, so that's for
> > another rainy day.
> 
> ...well it's pouring on the west coast here, so I'll post my solution
> that uses setfattr --restore tomorrow when I get it back from QA.
> Granted, I hadn't found a solution to the removexattr stuff yet, so I
> might keep working on that.
> 
> (removexattr looks like a pain in perl though...)
> 
> Anyway it's late now, I'll look at the diff tomorrow.

...or thursday now, since I decided to reply to the online fsck design
doc review comments, which took most of the workday.  I managed to bang
out a python script (perl doesn't support setxattr!) that cut the xattr
overhead down to nearly zero.

--D

> --D
> 
> > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> > ---
> >  common/populate | 179 ++++++++++++++++++++++++++++--------------------
> >  1 file changed, 104 insertions(+), 75 deletions(-)
> > 
> > diff --git a/common/populate b/common/populate
> > index 44b4af166..9b60fa5c1 100644
> > --- a/common/populate
> > +++ b/common/populate
> > @@ -52,23 +52,64 @@ __populate_fragment_file() {
> >  	test -f "${fname}" && $here/src/punch-alternating "${fname}"
> >  }
> >  
> > -# Create a large directory
> > -__populate_create_dir() {
> > -	name="$1"
> > -	nr="$2"
> > -	missing="$3"
> > +# Create a specified number of files or until the maximum extent count is
> > +# reached. If the extent count is reached, return the number of files created.
> > +# This is optimised for speed - do not add anything that executes a separate
> > +# process in every loop as this will slow it down by a factor of at least 5.
> > +__populate_create_nfiles() {
> > +	local name="$1"
> > +	local nr="$2"
> > +	local max_nextents="$3"
> > +	local d=0
> >  
> >  	mkdir -p "${name}"
> > -	seq 0 "${nr}" | while read d; do
> > -		creat=mkdir
> > -		test "$((d % 20))" -eq 0 && creat=touch
> > -		$creat "${name}/$(printf "%.08d" "$d")"
> > +	for d in `seq 0 "${nr}"`; do
> > +		local fname=""
> > +		printf -v fname "${name}/%.08d" "$d"
> > +
> > +		if [ "$((d % 20))" -eq 0 ]; then
> > +			mkdir ${fname}
> > +		else
> > +			echo -n > ${fname}
> > +		fi
> > +
> > +		if [ "${max_nextents}" -eq 0 ]; then
> > +			continue
> > +		fi
> > +		if [ "$((d % 40))" -ne 0 ]; then
> > +			continue
> > +		fi
> > +
> > +		local nextents="$(_xfs_get_fsxattr nextents $name)"
> > +		if [ "${nextents}" -gt "${max_nextents}" ]; then
> > +			echo ${d}
> > +			break
> > +		fi
> >  	done
> > +}
> > +
> > +# remove every second file in the given directory. This is optimised for speed -
> > +# do not add anything that executes a separate process in each loop as this will
> > +# slow it down by at least factor of 10.
> > +__populate_remove_nfiles() {
> > +	local name="$1"
> > +	local nr="$2"
> > +	local d=1
> > +
> > +	for d in `seq 1 2 "${nr}"`; do
> > +		printf "${name}/%.08d " "$d"
> > +	done | xargs rm -f
> > +}
> >  
> > +# Create a large directory
> > +__populate_create_dir() {
> > +	local name="$1"
> > +	local nr="$2"
> > +	local missing="$3"
> > +
> > +	__populate_create_nfiles "${name}" "${nr}" 0
> >  	test -z "${missing}" && return
> > -	seq 1 2 "${nr}" | while read d; do
> > -		rm -rf "${name}/$(printf "%.08d" "$d")"
> > -	done
> > +	__populate_remove_nfiles "${name}" "${nr}"
> >  }
> >  
> >  # Create a large directory and ensure that it's a btree format
> > @@ -82,31 +123,18 @@ __populate_xfs_create_btree_dir() {
> >  	# watch for when the extent count exceeds the space after the
> >  	# inode core.
> >  	local max_nextents="$(((isize - icore_size) / 16))"
> > -	local nr=0
> > -
> > -	mkdir -p "${name}"
> > -	while true; do
> > -		local creat=mkdir
> > -		test "$((nr % 20))" -eq 0 && creat=touch
> > -		$creat "${name}/$(printf "%.08d" "$nr")"
> > -		if [ "$((nr % 40))" -eq 0 ]; then
> > -			local nextents="$(_xfs_get_fsxattr nextents $name)"
> > -			[ $nextents -gt $max_nextents ] && break
> > -		fi
> > -		nr=$((nr+1))
> > -	done
> > +	local nr=100000
> >  
> > +	nr=$(__populate_create_nfiles "${name}" "${nr}" "${max_nextents}")
> >  	test -z "${missing}" && return
> > -	seq 1 2 "${nr}" | while read d; do
> > -		rm -rf "${name}/$(printf "%.08d" "$d")"
> > -	done
> > +	__populate_remove_nfiles "${name}" "${nr}"
> >  }
> >  
> >  # Add a bunch of attrs to a file
> >  __populate_create_attr() {
> > -	name="$1"
> > -	nr="$2"
> > -	missing="$3"
> > +	local name="$1"
> > +	local nr="$2"
> > +	local missing="$3"
> >  
> >  	touch "${name}"
> >  	seq 0 "${nr}" | while read d; do
> > @@ -121,17 +149,18 @@ __populate_create_attr() {
> >  
> >  # Fill up some percentage of the remaining free space
> >  __populate_fill_fs() {
> > -	dir="$1"
> > -	pct="$2"
> > +	local dir="$1"
> > +	local pct="$2"
> > +	local nr=0
> >  	test -z "${pct}" && pct=60
> >  
> >  	mkdir -p "${dir}/test/1"
> >  	cp -pRdu "${dir}"/S_IFREG* "${dir}/test/1/"
> >  
> > -	SRC_SZ="$(du -ks "${dir}/test/1" | cut -f 1)"
> > -	FS_SZ="$(( $(stat -f "${dir}" -c '%a * %S') / 1024 ))"
> > +	local SRC_SZ="$(du -ks "${dir}/test/1" | cut -f 1)"
> > +	local FS_SZ="$(( $(stat -f "${dir}" -c '%a * %S') / 1024 ))"
> >  
> > -	NR="$(( (FS_SZ * ${pct} / 100) / SRC_SZ ))"
> > +	local NR="$(( (FS_SZ * ${pct} / 100) / SRC_SZ ))"
> >  
> >  	echo "FILL FS"
> >  	echo "src_sz $SRC_SZ fs_sz $FS_SZ nr $NR"
> > @@ -220,45 +249,45 @@ _scratch_xfs_populate() {
> >  	# Data:
> >  
> >  	# Fill up the root inode chunk
> > -	echo "+ fill root ino 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
> > +		echo -n > "${SCRATCH_MNT}/dummy${f}"
> > +	done ) &
> >  
> >  	# Regular files
> >  	# - FMT_EXTENTS
> >  	echo "+ extents file"
> > -	__populate_create_file $blksz "${SCRATCH_MNT}/S_IFREG.FMT_EXTENTS"
> > +	__populate_create_file $blksz "${SCRATCH_MNT}/S_IFREG.FMT_EXTENTS" &
> >  
> >  	# - FMT_BTREE
> >  	echo "+ btree extents file"
> >  	nr="$((blksz * 2 / 16))"
> > -	__populate_create_file $((blksz * nr)) "${SCRATCH_MNT}/S_IFREG.FMT_BTREE"
> > +	__populate_create_file $((blksz * nr)) "${SCRATCH_MNT}/S_IFREG.FMT_BTREE" &
> >  
> >  	# Directories
> >  	# - INLINE
> > -	echo "+ inline dir"
> > -	__populate_create_dir "${SCRATCH_MNT}/S_IFDIR.FMT_INLINE" 1
> > +	 echo "+ inline dir"
> > +	__populate_create_dir "${SCRATCH_MNT}/S_IFDIR.FMT_INLINE" 1 "" &
> >  
> >  	# - BLOCK
> >  	echo "+ block dir"
> > -	__populate_create_dir "${SCRATCH_MNT}/S_IFDIR.FMT_BLOCK" "$((dblksz / 40))"
> > +	__populate_create_dir "${SCRATCH_MNT}/S_IFDIR.FMT_BLOCK" "$((dblksz / 40))" "" &
> >  
> >  	# - LEAF
> >  	echo "+ leaf dir"
> > -	__populate_create_dir "${SCRATCH_MNT}/S_IFDIR.FMT_LEAF" "$((dblksz / 12))"
> > +	__populate_create_dir "${SCRATCH_MNT}/S_IFDIR.FMT_LEAF" "$((dblksz / 12))" "" &
> >  
> >  	# - LEAFN
> >  	echo "+ leafn dir"
> > -	__populate_create_dir "${SCRATCH_MNT}/S_IFDIR.FMT_LEAFN" "$(( ((dblksz - leaf_hdr_size) / 8) - 3 ))"
> > +	__populate_create_dir "${SCRATCH_MNT}/S_IFDIR.FMT_LEAFN" "$(( ((dblksz - leaf_hdr_size) / 8) - 3 ))" "" &
> >  
> >  	# - NODE
> >  	echo "+ node dir"
> > -	__populate_create_dir "${SCRATCH_MNT}/S_IFDIR.FMT_NODE" "$((16 * dblksz / 40))" true
> > +	__populate_create_dir "${SCRATCH_MNT}/S_IFDIR.FMT_NODE" "$((16 * dblksz / 40))" true &
> >  
> >  	# - 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" true &
> >  
> >  	# Symlinks
> >  	# - FMT_LOCAL
> > @@ -280,20 +309,20 @@ _scratch_xfs_populate() {
> >  
> >  	# Attribute formats
> >  	# LOCAL
> > -	echo "+ local attr"
> > -	__populate_create_attr "${SCRATCH_MNT}/ATTR.FMT_LOCAL" 1
> > +	 echo "+ local attr"
> > +	__populate_create_attr "${SCRATCH_MNT}/ATTR.FMT_LOCAL" 1 "" &
> >  
> >  	# LEAF
> > -	echo "+ leaf attr"
> > -	__populate_create_attr "${SCRATCH_MNT}/ATTR.FMT_LEAF" "$((blksz / 40))"
> > +	 echo "+ leaf attr"
> > +	__populate_create_attr "${SCRATCH_MNT}/ATTR.FMT_LEAF" "$((blksz / 40))" "" &
> >  
> >  	# NODE
> >  	echo "+ node attr"
> > -	__populate_create_attr "${SCRATCH_MNT}/ATTR.FMT_NODE" "$((8 * blksz / 40))"
> > +	__populate_create_attr "${SCRATCH_MNT}/ATTR.FMT_NODE" "$((8 * blksz / 40))" "" &
> >  
> >  	# BTREE
> >  	echo "+ btree attr"
> > -	__populate_create_attr "${SCRATCH_MNT}/ATTR.FMT_BTREE" "$((64 * blksz / 40))" true
> > +	__populate_create_attr "${SCRATCH_MNT}/ATTR.FMT_BTREE" "$((64 * blksz / 40))" true &
> >  
> >  	# trusted namespace
> >  	touch ${SCRATCH_MNT}/ATTR.TRUSTED
> > @@ -321,68 +350,68 @@ _scratch_xfs_populate() {
> >  	rm -rf "${SCRATCH_MNT}/attrvalfile"
> >  
> >  	# Make an unused inode
> > -	echo "+ empty file"
> > +	( echo "+ empty file"
> >  	touch "${SCRATCH_MNT}/unused"
> >  	$XFS_IO_PROG -f -c 'fsync' "${SCRATCH_MNT}/unused"
> > -	rm -rf "${SCRATCH_MNT}/unused"
> > +	rm -rf "${SCRATCH_MNT}/unused" ) &
> >  
> >  	# Free space btree
> >  	echo "+ freesp btree"
> >  	nr="$((blksz * 2 / 8))"
> > -	__populate_create_file $((blksz * nr)) "${SCRATCH_MNT}/BNOBT"
> > +	__populate_create_file $((blksz * nr)) "${SCRATCH_MNT}/BNOBT" &
> >  
> >  	# Inode btree
> > -	echo "+ inobt btree"
> > +	( echo "+ inobt btree"
> >  	local ino_per_rec=64
> >  	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 "${SCRATCH_MNT}/INOBT" "${nr}" true
> > +	) &
> >  
> >  	# Reverse-mapping btree
> >  	is_rmapbt="$(_xfs_has_feature "$SCRATCH_MNT" rmapbt -v)"
> >  	if [ $is_rmapbt -gt 0 ]; then
> > -		echo "+ rmapbt btree"
> > +		( echo "+ rmapbt btree"
> >  		nr="$((blksz * 2 / 24))"
> >  		__populate_create_file $((blksz * nr)) "${SCRATCH_MNT}/RMAPBT"
> > +		) &
> >  	fi
> >  
> >  	# Realtime Reverse-mapping btree
> >  	is_rt="$(_xfs_get_rtextents "$SCRATCH_MNT")"
> >  	if [ $is_rmapbt -gt 0 ] && [ $is_rt -gt 0 ]; then
> > -		echo "+ rtrmapbt btree"
> > +		( echo "+ rtrmapbt btree"
> >  		nr="$((blksz * 2 / 32))"
> >  		$XFS_IO_PROG -R -f -c 'truncate 0' "${SCRATCH_MNT}/RTRMAPBT"
> >  		__populate_create_file $((blksz * nr)) "${SCRATCH_MNT}/RTRMAPBT"
> > +		) &
> >  	fi
> >  
> >  	# Reference-count btree
> >  	is_reflink="$(_xfs_has_feature "$SCRATCH_MNT" reflink -v)"
> >  	if [ $is_reflink -gt 0 ]; then
> > -		echo "+ reflink btree"
> > +		( echo "+ reflink btree"
> >  		nr="$((blksz * 2 / 12))"
> >  		__populate_create_file $((blksz * nr)) "${SCRATCH_MNT}/REFCOUNTBT"
> >  		cp --reflink=always "${SCRATCH_MNT}/REFCOUNTBT" "${SCRATCH_MNT}/REFCOUNTBT2"
> > +		) &
> >  	fi
> >  
> >  	# Copy some real files (xfs tests, I guess...)
> >  	echo "+ real files"
> >  	test $fill -ne 0 && __populate_fill_fs "${SCRATCH_MNT}" 5
> >  
> > -	# Make sure we get all the fragmentation we asked for
> > -	__populate_fragment_file "${SCRATCH_MNT}/S_IFREG.FMT_BTREE"
> > -	__populate_fragment_file "${SCRATCH_MNT}/BNOBT"
> > -	__populate_fragment_file "${SCRATCH_MNT}/RMAPBT"
> > -	__populate_fragment_file "${SCRATCH_MNT}/RTRMAPBT"
> > -	__populate_fragment_file "${SCRATCH_MNT}/REFCOUNTBT"
> > +	# Wait for all file creation to complete before we start fragmenting
> > +	# the files as needed.
> > +	wait
> > +	__populate_fragment_file "${SCRATCH_MNT}/S_IFREG.FMT_BTREE" &
> > +	__populate_fragment_file "${SCRATCH_MNT}/BNOBT" &
> > +	__populate_fragment_file "${SCRATCH_MNT}/RMAPBT" &
> > +	__populate_fragment_file "${SCRATCH_MNT}/RTRMAPBT" &
> > +	__populate_fragment_file "${SCRATCH_MNT}/REFCOUNTBT" &
> >  
> > +	wait
> >  	umount "${SCRATCH_MNT}"
> >  }
> >  
> > -- 
> > 2.38.1
> > 

Re: [PATCH 2/3] populate: ensure btree directories are created reliably

[Gao Xiang]

On 2023/1/11 06:49, Dave Chinner wrote:
> 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: Gao Xiang <hsiangkao@linux.alibaba.com>

Thanks,
Gao Xiang

> ---
>   common/populate | 7 +++++--
>   1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/common/populate b/common/populate
> index 9b60fa5c1..7b5b16fb8 100644
> --- a/common/populate
> +++ b/common/populate
> @@ -80,8 +80,11 @@ __populate_create_nfiles() {
>   			continue
>   		fi
>   
> -		local nextents="$(_xfs_get_fsxattr nextents $name)"
> -		if [ "${nextents}" -gt "${max_nextents}" ]; then
> +		# 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.
> +		local nextents="$(xfs_bmap ${name} |grep -v hole | wc -l)"
> +		if [ "$((nextents - 1))" -gt "${max_nextents}" ]; then
>   			echo ${d}
>   			break
>   		fi

Re: [PATCH 3/3] xfs/294: performance is unreasonably slow

[Zorro Lang]

On Wed, Jan 11, 2023 at 09:49:06AM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> This creates a bunch of files in a dir, then deletes 97% of them
> attempting to leave 1 allocated inode per inode chunk so that they
> aren't freed. Performance is badly limited by task creation and
> destruction for each inode created. Fix this by using "echo -n >
> file" rather than touch so that the shell creates the empty files
> without needing to fork/exec a separate task for each creation.
> 
> This reduces runtime from 45s down to 15s.
> 
> Also add more debug with inode counts and internal superblock
> counter information for determining why this test may ENOSPC on the
> final creation loop.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---



>  tests/xfs/294 | 19 +++++++++++++++----
>  1 file changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/tests/xfs/294 b/tests/xfs/294
> index d381e2c85..1ce0d1cc5 100755
> --- a/tests/xfs/294
> +++ b/tests/xfs/294
> @@ -28,6 +28,13 @@ _require_test_program "punch-alternating"
>  _require_xfs_io_command "falloc"
>  _require_xfs_io_command "fpunch"
>  
> +dump_freespace()
> +{
> +	df $SCRATCH_MNT
> +	df -i $SCRATCH_MNT
> +	$XFS_IO_PROG -rc "statfs -c" $SCRATCH_MNT
> +}
> +
>  # We want to mkfs with a very specific geometry
>  MKFS_OPTIONS=""
>  _scratch_mkfs "-d size=512m -n size=8192 -i size=1024" >> $seqres.full 2>&1 \
> @@ -37,7 +44,7 @@ _scratch_mount
>  # Make a ton of mostly-empty inode clusters so we can always
>  # make more inodes
>  mkdir $SCRATCH_MNT/tmp
> -for I in `seq 1 10000`; do touch $SCRATCH_MNT/tmp/$I; done
> +for I in `seq 1 10000`; do echo -n > $SCRATCH_MNT/tmp/$I; done

Make sense to me, thanks for this improvement.

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

>  
>  # These mostly-empty clusters will live here:
>  mkdir $SCRATCH_MNT/clusters
> @@ -50,7 +57,7 @@ rm -rf $SCRATCH_MNT/tmp
>  mkdir $SCRATCH_MNT/testdir
>  # roughly 20 chars per file
>  for I in `seq 1 100`; do
> -	touch $SCRATCH_MNT/testdir/12345678901234567890$I;
> +	echo -n > $SCRATCH_MNT/testdir/12345678901234567890$I;
>  done
>  
>  # File to fragment:
> @@ -63,7 +70,7 @@ space=$(stat -f -c '%f * %S * 95 / 100' $SCRATCH_MNT | $BC_PROG)
>  $XFS_IO_PROG -f -c "falloc 0 $space" $SCRATCH_MNT/fillfile ||
>  	_fail "Could not allocate space"
>  
> -df -h $SCRATCH_MNT >> $seqres.full 2>&1
> +dump_freespace >> $seqres.full 2>&1
>  
>  # Fill remaining space; let this run to failure
>  dd if=/dev/zero of=$SCRATCH_MNT/spacefile1 oflag=direct >> $seqres.full 2>&1
> @@ -75,12 +82,16 @@ $here/src/punch-alternating $SCRATCH_MNT/fragfile >> $seqres.full 2>&1
>  # (and then some for good measure)
>  dd conv=fsync if=/dev/zero of=$SCRATCH_MNT/spacefile2 bs=1M count=64 >> $seqres.full 2>&1
>  
> +dump_freespace >> $seqres.full 2>&1
> +
>  # Now populate the directory so that it must allocate these
>  # fragmented blocks
>  for I in `seq 1 1400`; do
> -	touch $SCRATCH_MNT/testdir/12345678901234567890$I;
> +	echo -n > $SCRATCH_MNT/testdir/12345678901234567890$I;
>  done
>  
> +dump_freespace >> $seqres.full 2>&1
> +
>  # Now traverse that ugly thing!
>  find $SCRATCH_MNT/testdir | sort | _filter_scratch | md5sum
>  
> -- 
> 2.38.1
> 

Re: [PATCH 1/3] more python dependence. was: populate: fix horrible performance due to excessive forking

[David Disseldorp]

Hi Darrick,

On Wed, 11 Jan 2023 17:58:17 -0800, Darrick J. Wong wrote:

> > (removexattr looks like a pain in perl though...)
> > 
> > Anyway it's late now, I'll look at the diff tomorrow.  
> 
> ...or thursday now, since I decided to reply to the online fsck design
> doc review comments, which took most of the workday.  I managed to bang
> out a python script (perl doesn't support setxattr!) that cut the xattr
> overhead down to nearly zero.

IIUC we currently only depend on python for the fio perf tests and
btrfs/154 . My preference would be to not see it spread further
(especially if it's just to shave off a little runtime), mostly because
it's a pain for dependency tracking.
Perhaps you could use perl's syscall(SYS_fsetxattr(), ...)? Well, that or
rewrite it again in awk ;-P

Cheers, David

Re: [PATCH 1/3] more python dependence. was: populate: fix horrible performance due to excessive forking

[Darrick J. Wong]

On Thu, Jan 12, 2023 at 11:24:58AM +0100, David Disseldorp wrote:
> Hi Darrick,
> 
> On Wed, 11 Jan 2023 17:58:17 -0800, Darrick J. Wong wrote:
> 
> > > (removexattr looks like a pain in perl though...)
> > > 
> > > Anyway it's late now, I'll look at the diff tomorrow.  
> > 
> > ...or thursday now, since I decided to reply to the online fsck design
> > doc review comments, which took most of the workday.  I managed to bang
> > out a python script (perl doesn't support setxattr!) that cut the xattr
> > overhead down to nearly zero.
> 
> IIUC we currently only depend on python for the fio perf tests and
> btrfs/154 . My preference would be to not see it spread further

I don't appreciate your dismissal of the patch before I've even posted
it!

The fstests README clearly lists python3 as a dependency.  Argument
parsing and xattr calls are provided by the base python3 runtime.  No
third party libraries are required for this new program, and if they
were, they'd be added to the README.

> (especially if it's just to shave off a little runtime), mostly because
> it's a pain for dependency tracking.
> Perhaps you could use perl's syscall(SYS_fsetxattr(), ...)? Well, that or

Raw system calls are a terrible idea for maintainability.  You'd
*seriously* rather I open-code the glibc xattr wrappers and make the
fstests community maintain that for the sake of your preference?

> rewrite it again in awk ;-P

WTAF?

--D

> Cheers, David

Re: [PATCH 1/3] more python dependence. was: populate: fix horrible performance due to excessive forking

[David Disseldorp]

On Thu, 12 Jan 2023 09:07:56 -0800, Darrick J. Wong wrote:

> On Thu, Jan 12, 2023 at 11:24:58AM +0100, David Disseldorp wrote:
> > Hi Darrick,
...
> > IIUC we currently only depend on python for the fio perf tests and
> > btrfs/154 . My preference would be to not see it spread further  
> 
> I don't appreciate your dismissal of the patch before I've even posted
> it!

Alright, fair enough. I apologise for that.

> The fstests README clearly lists python3 as a dependency.  Argument
> parsing and xattr calls are provided by the base python3 runtime.  No
> third party libraries are required for this new program, and if they
> were, they'd be added to the README.
> 
> > (especially if it's just to shave off a little runtime), mostly because
> > it's a pain for dependency tracking.
> > Perhaps you could use perl's syscall(SYS_fsetxattr(), ...)? Well, that or  
> 
> Raw system calls are a terrible idea for maintainability.  You'd
> *seriously* rather I open-code the glibc xattr wrappers and make the
> fstests community maintain that for the sake of your preference?

That's not what I said my preference was.

> > rewrite it again in awk ;-P  
> 
> WTAF?

<sigh>

Re: [PATCH 1/3] more python dependence. was: populate: fix horrible performance due to excessive forking

[Zorro Lang]

On Thu, Jan 12, 2023 at 09:07:56AM -0800, Darrick J. Wong wrote:
> On Thu, Jan 12, 2023 at 11:24:58AM +0100, David Disseldorp wrote:
> > Hi Darrick,
> > 
> > On Wed, 11 Jan 2023 17:58:17 -0800, Darrick J. Wong wrote:
> > 
> > > > (removexattr looks like a pain in perl though...)
> > > > 
> > > > Anyway it's late now, I'll look at the diff tomorrow.  
> > > 
> > > ...or thursday now, since I decided to reply to the online fsck design
> > > doc review comments, which took most of the workday.  I managed to bang
> > > out a python script (perl doesn't support setxattr!) that cut the xattr
> > > overhead down to nearly zero.
> > 
> > IIUC we currently only depend on python for the fio perf tests and
> > btrfs/154 . My preference would be to not see it spread further
> 
> I don't appreciate your dismissal of the patch before I've even posted
> it!
> 
> The fstests README clearly lists python3 as a dependency.  Argument
> parsing and xattr calls are provided by the base python3 runtime.  No
> third party libraries are required for this new program, and if they
> were, they'd be added to the README.

Sorry Darrick, that README description might not exact enough :/ some packages
are not *necessary* for the whole fstests running. Their missing might just
cause some single cases be skipped.

The python3 isn't necessary running/building dependence of fstests. Some
people might run fstests without python3 currently. And I don't plan to
make it become *necessary* (no running if no python3) now. If you'd like
to add python3 dependence to common helpers, that might affect more cases.

So how about fall back to old code if no python3? Or you'd like to skipped
populate related testing if no python3? Or use another way to reduce the
hard dependence change.

Thanks,
Zorro

> 
> > (especially if it's just to shave off a little runtime), mostly because
> > it's a pain for dependency tracking.
> > Perhaps you could use perl's syscall(SYS_fsetxattr(), ...)? Well, that or
> 
> Raw system calls are a terrible idea for maintainability.  You'd
> *seriously* rather I open-code the glibc xattr wrappers and make the
> fstests community maintain that for the sake of your preference?
> 
> > rewrite it again in awk ;-P
> 
> WTAF?
> 
> --D
> 
> > Cheers, David
> 

Re: [PATCH 1/3] more python dependence. was: populate: fix horrible performance due to excessive forking

[Darrick J. Wong]

On Fri, Jan 13, 2023 at 04:42:13AM +0800, Zorro Lang wrote:
> On Thu, Jan 12, 2023 at 09:07:56AM -0800, Darrick J. Wong wrote:
> > On Thu, Jan 12, 2023 at 11:24:58AM +0100, David Disseldorp wrote:
> > > Hi Darrick,
> > > 
> > > On Wed, 11 Jan 2023 17:58:17 -0800, Darrick J. Wong wrote:
> > > 
> > > > > (removexattr looks like a pain in perl though...)
> > > > > 
> > > > > Anyway it's late now, I'll look at the diff tomorrow.  
> > > > 
> > > > ...or thursday now, since I decided to reply to the online fsck design
> > > > doc review comments, which took most of the workday.  I managed to bang
> > > > out a python script (perl doesn't support setxattr!) that cut the xattr
> > > > overhead down to nearly zero.
> > > 
> > > IIUC we currently only depend on python for the fio perf tests and
> > > btrfs/154 . My preference would be to not see it spread further
> > 
> > I don't appreciate your dismissal of the patch before I've even posted
> > it!
> > 
> > The fstests README clearly lists python3 as a dependency.  Argument
> > parsing and xattr calls are provided by the base python3 runtime.  No
> > third party libraries are required for this new program, and if they
> > were, they'd be added to the README.
> 
> Sorry Darrick, that README description might not exact enough :/ some packages
> are not *necessary* for the whole fstests running. Their missing might just
> cause some single cases be skipped.
> 
> The python3 isn't necessary running/building dependence of fstests. Some
> people might run fstests without python3 currently. And I don't plan to
> make it become *necessary* (no running if no python3) now. If you'd like
> to add python3 dependence to common helpers, that might affect more cases.
> 
> So how about fall back to old code if no python3? Or you'd like to skipped
> populate related testing if no python3? Or use another way to reduce the
> hard dependence change.

I'm still working on getting this to pass the online fsck fuzz QA tests.

I started by moving the btree dir creation fix to the front of the
series, and then split this first patch into one patch to speed up
directory creation and a second patch to run most of the creation
helpers in parallel.

It turned out that my earlier patch moving the loop to a perl script was
even faster than Dave's thing, so I substituted that one.

Next I reworked the xattr creation speedups to incorporate all the
review comments.  Zorro pointed out that unlike perl, there's nothing in
fstests that _fatals the lack of python.

For people with python3, I still provide a new script to handle creation
and selective deletion of xattrs.  This reduces the xattr part of the
runtime to nearly zero.  For people who don't want python3, I
resurrected the previous version of the patch that formats a fake xattr
dump file and pipes it to setattr --restore.  The deletion loop is still
slow, but overall it's faster than before.

That leaves the parallel creation patch.  This has caused lots of
headaches for me because the previous populate() implementation would
(perhaps accidentally) create a filesystem where most of the field
fuzzing didn't get in the way of mounting the filesystem, but with
parallel creation enabled, I see a lot of online fsck test failures
resulting from mount failure.

For example, the function creates a directory tree under
SCRATCH_MNT/INOBT/ with enough files to create a multilevel inode btree.
When the function ran serially, the inode allocation rotor would not
generally create the diretory (and its children) in AG 0.  This is what
we want, since problems in the AG 0 inode btree usually result in mount
failures when we try to look up the root directory.  With parallelism
enabled, the inobt gets created in AG 0 sometimes, which causes the
online fsck fuzz tests to fail because mount failed.

The populate function *could* get smarter about avoiding AG 0, though
that would come with an increase in code complexity.

I then measured the performance speedup of each of the individual
patches in my branch.  Removing the execve overhead resulted in a 10x
decrease in runtime (~170s to ~15s), but parallelizing after that only
netted a ~2s decrease in runtime.

Online fsck testing seems fine if I pop the parallelization patches off,
so I'll keep trying, but I think I've hit the point of diminished
returns.

--D

> Thanks,
> Zorro
> 
> > 
> > > (especially if it's just to shave off a little runtime), mostly because
> > > it's a pain for dependency tracking.
> > > Perhaps you could use perl's syscall(SYS_fsetxattr(), ...)? Well, that or
> > 
> > Raw system calls are a terrible idea for maintainability.  You'd
> > *seriously* rather I open-code the glibc xattr wrappers and make the
> > fstests community maintain that for the sake of your preference?
> > 
> > > rewrite it again in awk ;-P
> > 
> > WTAF?
> > 
> > --D
> > 
> > > Cheers, David
> > 
>