On Sun, May 09, 2021 at 10:36:05PM +0800, Eryu Guan wrote:

On Tue, Apr 27, 2021 at 12:44:17PM -0400, Kent Overstreet wrote:

quoted

From: Kent Overstreet <redacted>

Better to add commit logs at least to give an example about how to setup
fstests to test bcachefs.

You could always set MKFS_OPTIONS to "--errors=panic" explicitly when
needed.

Forgot that was an option - doing that now.

quoted

+		;;
 	*)
 		;;
 	esac

diff --git a/common/dmlogwrites b/common/dmlogwrites
index 573f4b8a56..668d49e995 100644
--- a/common/dmlogwrites
+++ b/common/dmlogwrites

@@ -111,6 +111,13 @@ _log_writes_replay_log()
 	[ -z "$_blkdev" ] && _fail \
 	"block dev must be specified for _log_writes_replay_log"
 
+	if [ "$FSTYP" = "bcachefs" ]; then
+		# bcachefs gets confused if we're replaying the history out of
+		# order, and we see writes on the device from a newer point in
+		# time than what the superblock points to:
+		dd if=/dev/zero of=$SCRATCH_DEV bs=1M oflag=direct >& /dev/null

I don't know bcachefs internals, I'm not sure I understand this,
clearing the first 1M of SCRATCH_DEV seems to clear superblock, but I'm
still not sure why it's needed. Does wipefs work?

It's not just the superblock we need to clear, it's really all metadata - the
journal, and btree nodes are also log structured in bcachefs. So 1M actually
isn't sufficient - the better solution would be to either

 - change the tests to check the markers in the log in the correct order, so
   we never see metadata from a future point in time, also making sure we don't
   do any writes to the filesystem when we're checking the different markers, or
 - just replay to a new dm-thin device

This is basically what I did for generic/482, the 1M zerout is really just a
hack for 455 and 457 and should probably be moved there, unless you've got
another suggestion.

quoted

diff --git a/common/rc b/common/rc
index 2cf550ec68..0e03846aeb 100644
--- a/common/rc
+++ b/common/rc

@@ -334,6 +334,7 @@ _try_scratch_mount()
 		return $?
 	fi
 	_mount -t $FSTYP `_scratch_mount_options $*`
+	return

Seems not necessary.

Not sure how that got in, dropped it.

quoted

+    bcachefs)
+	$MKFS_PROG -t $FSTYP -- $MKFS_OPTIONS $* $TEST_DEV
+	;;

I think we could just use the default mkfs command below. The only
difference is dropping the "yes | " part, but that does nothing if mkfs
doesn't read "yes" or "no" from stdin.

That dates from when my test environment had SIGPIPE set up wrong (systemd!),
it's fixed now so I've dropped these.

quoted

     *)
 	_notrun "Filesystem $FSTYP not supported in _scratch_mkfs_blocksized"
 	;;

@@ -1179,6 +1197,19 @@ _repair_scratch_fs()
 	fi
 	return $res
         ;;
+    bcachefs)
+	fsck -t $FSTYP -n $SCRATCH_DEV 2>&1

_repair_scratch_fs() is supposed to actually fix the errors, does
"fsck -n" fix errors for bcachefs?

No - but with bcachefs fsck finding errors _always_ indicates a bug, so for the
purposes of these tests I think this is the right thing to do - I don't want the
tests to pass if fsck is finding and fixing errors.

quoted

diff --git a/tests/generic/042 b/tests/generic/042
index 35727bcbc6..42919e2313 100755
--- a/tests/generic/042
+++ b/tests/generic/042

@@ -63,7 +63,8 @@ _crashtest()
 
 	# We should /never/ see 0xCD in the file, because we wrote that pattern
 	# to the filesystem image to expose stale data.
-	if hexdump -v -e '/1 "%02X "' $file | grep -q "CD"; then
+	# The file is not required to exist since we didn't sync before going down:
+	if [[ -f $file ]] && hexdump -v -e '/1 "%02X "' $file | grep -q "CD"; then
 		echo "Saw stale data!!!"
 		hexdump $file
 	fi

Updates for individual test should be in a separate patch.

Ok, I'll split those out.