linux-fsdevel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/3] bcachefs support
@ 2021-04-27 16:44 Kent Overstreet
  2021-04-27 16:44 ` [PATCH 1/3] Initial " Kent Overstreet
                   ` (3 more replies)
  0 siblings, 4 replies; 19+ messages in thread
From: Kent Overstreet @ 2021-04-27 16:44 UTC (permalink / raw)
  To: fstests, linux-fsdevel, linux-bcachefs; +Cc: Kent Overstreet

A small patch adding bcachefs support, and two other patches for consideration:

Kent Overstreet (3):
  Initial bcachefs support
  Improved .gitignore
  Use --yes option to lvcreate

 .gitignore         |  3 +++
 common/attr        |  6 ++++++
 common/config      |  3 +++
 common/dmlogwrites |  7 +++++++
 common/quota       |  4 ++--
 common/rc          | 31 +++++++++++++++++++++++++++++++
 tests/generic/042  |  3 ++-
 tests/generic/081  |  2 +-
 tests/generic/108  |  2 +-
 tests/generic/425  |  3 +++
 tests/generic/441  |  2 +-
 tests/generic/482  | 27 ++++++++++++++++++++-------
 tests/generic/558  |  2 ++
 13 files changed, 82 insertions(+), 13 deletions(-)

-- 
2.31.1


^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH 1/3] Initial bcachefs support
  2021-04-27 16:44 [PATCH 0/3] bcachefs support Kent Overstreet
@ 2021-04-27 16:44 ` Kent Overstreet
  2021-05-09 14:36   ` Eryu Guan
  2021-04-27 16:44 ` [PATCH 2/3] Improved .gitignore Kent Overstreet
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 19+ messages in thread
From: Kent Overstreet @ 2021-04-27 16:44 UTC (permalink / raw)
  To: fstests, linux-fsdevel, linux-bcachefs; +Cc: Kent Overstreet, Kent Overstreet

From: Kent Overstreet <kmo@daterainc.com>

Signed-off-by: Kent Overstreet <kent.overstreet@gmail.com>
---
 common/attr        |  6 ++++++
 common/config      |  3 +++
 common/dmlogwrites |  7 +++++++
 common/quota       |  4 ++--
 common/rc          | 31 +++++++++++++++++++++++++++++++
 tests/generic/042  |  3 ++-
 tests/generic/425  |  3 +++
 tests/generic/441  |  2 +-
 tests/generic/482  | 27 ++++++++++++++++++++-------
 tests/generic/558  |  2 ++
 10 files changed, 77 insertions(+), 11 deletions(-)

diff --git a/common/attr b/common/attr
index 669909d600..42ceab9233 100644
--- a/common/attr
+++ b/common/attr
@@ -33,6 +33,9 @@ _acl_get_max()
 			echo 506
 		fi
 		;;
+	bcachefs)
+		echo 251
+		;;
 	*)
 		echo 0
 		;;
@@ -273,6 +276,9 @@ pvfs2)
 9p|ceph|nfs)
 	MAX_ATTRVAL_SIZE=65536
 	;;
+bcachefs)
+	MAX_ATTRVAL_SIZE=1024
+	;;
 *)
 	# Assume max ~1 block of attrs
 	BLOCK_SIZE=`_get_block_size $TEST_DIR`
diff --git a/common/config b/common/config
index a47e462c77..8153301483 100644
--- a/common/config
+++ b/common/config
@@ -415,6 +415,9 @@ _mkfs_opts()
 	btrfs)
 		export MKFS_OPTIONS="$BTRFS_MKFS_OPTIONS"
 		;;
+	bcachefs)
+		export MKFS_OPTIONS="--errors=panic"
+		;;
 	*)
 		;;
 	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
+	fi
+
 	$here/src/log-writes/replay-log --log $LOGWRITES_DEV --find \
 		--end-mark $_mark >> $seqres.full 2>&1
 	[ $? -ne 0 ] && _fail "mark '$_mark' does not exist"
diff --git a/common/quota b/common/quota
index 32a9a55593..883a28a20d 100644
--- a/common/quota
+++ b/common/quota
@@ -17,7 +17,7 @@ _require_quota()
 	    _notrun "Installed kernel does not support quotas"
 	fi
 	;;
-    gfs2|ocfs2)
+    gfs2|ocfs2|bcachefs)
 	;;
     xfs)
 	if [ ! -f /proc/fs/xfs/xqmstat ]; then
@@ -278,7 +278,7 @@ _check_quota_usage()
 
 	VFS_QUOTA=0
 	case $FSTYP in
-	ext2|ext3|ext4|ext4dev|f2fs|reiserfs|gfs2)
+	ext2|ext3|ext4|ext4dev|f2fs|reiserfs|gfs2|bcachefs)
 		VFS_QUOTA=1
 		quotaon -f -u -g $SCRATCH_MNT 2>/dev/null
 		;;
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
 }
 
 # mount scratch device with given options and _fail if mount fails
@@ -667,6 +668,9 @@ _test_mkfs()
     ext2|ext3|ext4)
 	$MKFS_PROG -t $FSTYP -- -F $MKFS_OPTIONS $* $TEST_DEV
 	;;
+    bcachefs)
+	$MKFS_PROG -t $FSTYP -- $MKFS_OPTIONS $* $TEST_DEV
+	;;
     *)
 	yes | $MKFS_PROG -t $FSTYP -- $MKFS_OPTIONS $* $TEST_DEV
 	;;
@@ -706,6 +710,10 @@ _mkfs_dev()
 	$MKFS_PROG -t $FSTYP -- -f $MKFS_OPTIONS $* \
 		2>$tmp.mkfserr 1>$tmp.mkfsstd
 	;;
+    bcachefs)
+	$MKFS_PROG -t $FSTYP -- $MKFS_OPTIONS $* \
+		2>$tmp_dir.mkfserr 1>$tmp_dir.mkfsstd
+	;;
     *)
 	yes | $MKFS_PROG -t $FSTYP -- $MKFS_OPTIONS $* \
 		2>$tmp.mkfserr 1>$tmp.mkfsstd
@@ -803,6 +811,10 @@ _scratch_mkfs()
 		mkfs_cmd="yes | $MKFS_PROG -t $FSTYP --"
 		mkfs_filter="grep -v -e ^mkfs\.ocfs2"
 		;;
+	bcachefs)
+		mkfs_cmd="$MKFS_PROG -t $FSTYP --"
+		mkfs_filter="cat"
+		;;
 	*)
 		mkfs_cmd="yes | $MKFS_PROG -t $FSTYP --"
 		mkfs_filter="cat"
@@ -1065,6 +1077,9 @@ _scratch_mkfs_sized()
 		fi
 		export MOUNT_OPTIONS="-o size=$fssize $TMPFS_MOUNT_OPTIONS"
 		;;
+	bcachefs)
+		$MKFS_PROG -t $FSTYP -- $MKFS_OPTIONS --fs_size=$fssize --block_size=$blocksize $SCRATCH_DEV
+		;;
 	*)
 		_notrun "Filesystem $FSTYP not supported in _scratch_mkfs_sized"
 		;;
@@ -1133,6 +1148,9 @@ _scratch_mkfs_blocksized()
     ocfs2)
 	yes | ${MKFS_PROG} -t $FSTYP -F $MKFS_OPTIONS -b $blocksize -C $blocksize $SCRATCH_DEV
 	;;
+    bcachefs)
+	${MKFS_PROG}.$FSTYP $MKFS_OPTIONS --block_size=$blocksize $SCRATCH_DEV
+	;;
     *)
 	_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
+	res=$?
+	case $res in
+	0)
+		res=0
+		;;
+	*)
+		_dump_err2 "fsck.$FSTYP failed, err=$res"
+		;;
+	esac
+	return $res
+	;;
     *)
 	local dev=$SCRATCH_DEV
 	local fstyp=$FSTYP
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
diff --git a/tests/generic/425 b/tests/generic/425
index 51cbe1c67d..be2bc1b02e 100755
--- a/tests/generic/425
+++ b/tests/generic/425
@@ -30,6 +30,9 @@ _cleanup()
 
 # real QA test starts here
 _supported_fs generic
+
+[ $FSTYP = bcachefs ] && _notrun "bcachefs does not store xattrs in blocks"
+
 _require_scratch
 _require_attrs
 _require_xfs_io_command "fiemap" "-a"
diff --git a/tests/generic/441 b/tests/generic/441
index bedbcb0817..814387b2a9 100755
--- a/tests/generic/441
+++ b/tests/generic/441
@@ -40,7 +40,7 @@ case $FSTYP in
 	btrfs)
 		_notrun "btrfs has a specialized test for this"
 		;;
-	ext3|ext4|xfs)
+	ext3|ext4|xfs|bcachefs)
 		# Do the more thorough test if we have a logdev
 		_has_logdev && sflag=''
 		;;
diff --git a/tests/generic/482 b/tests/generic/482
index 86941e8468..3cbe187f2e 100755
--- a/tests/generic/482
+++ b/tests/generic/482
@@ -77,16 +77,29 @@ prev=$(_log_writes_mark_to_entry_number mkfs)
 cur=$(_log_writes_find_next_fua $prev)
 [ -z "$cur" ] && _fail "failed to locate next FUA write"
 
+if [ "$FSTYP" = "bcachefs" ]; then
+    _dmthin_cleanup
+    _dmthin_init $devsize $devsize $csize $lowspace
+fi
+
 while [ ! -z "$cur" ]; do
 	_log_writes_replay_log_range $cur $DMTHIN_VOL_DEV >> $seqres.full
 
-	# Here we need extra mount to replay the log, mainly for journal based
-	# fs, as their fsck will report dirty log as error.
-	# We don't care to preserve any data on the replay dev, as we can replay
-	# back to the point we need, and in fact sometimes creating/deleting
-	# snapshots repeatedly can be slower than replaying the log.
-	_dmthin_mount
-	_dmthin_check_fs
+	if [ "$FSTYP" = "bcachefs" ]; then
+	    # bcachefs will get confused if fsck does writes to replay the log,
+	    # but then we replay writes from an earlier point in time on the
+	    # same fs - but  fsck in -n mode won't do any writes:
+	    _check_scratch_fs -n $DMTHIN_VOL_DEV
+	else
+	    # Here we need extra mount to replay the log, mainly for journal based
+	    # fs, as their fsck will report dirty log as error.
+	    # We don't care to preserve any data on the replay dev, as we can replay
+	    # back to the point we need, and in fact sometimes creating/deleting
+	    # snapshots repeatedly can be slower than replaying the log.
+
+	    _dmthin_mount
+	    _dmthin_check_fs
+	fi
 
 	prev=$cur
 	cur=$(_log_writes_find_next_fua $(($cur + 1)))
diff --git a/tests/generic/558 b/tests/generic/558
index 4bed90e2a7..2eaa8f7686 100755
--- a/tests/generic/558
+++ b/tests/generic/558
@@ -55,6 +55,8 @@ _scratch_mount
 i=0
 free_inode=`_get_free_inode $SCRATCH_MNT`
 file_per_dir=1000
+[ $FSTYP = bcachefs ] && file_per_dir=10000
+
 loop=$((free_inode / file_per_dir + 1))
 mkdir -p $SCRATCH_MNT/testdir
 
-- 
2.31.1


^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH 2/3] Improved .gitignore
  2021-04-27 16:44 [PATCH 0/3] bcachefs support Kent Overstreet
  2021-04-27 16:44 ` [PATCH 1/3] Initial " Kent Overstreet
@ 2021-04-27 16:44 ` Kent Overstreet
  2021-04-27 16:44 ` [PATCH 3/3] Use --yes option to lvcreate Kent Overstreet
  2021-05-09 14:20 ` [PATCH 0/3] bcachefs support Eryu Guan
  3 siblings, 0 replies; 19+ messages in thread
From: Kent Overstreet @ 2021-04-27 16:44 UTC (permalink / raw)
  To: fstests, linux-fsdevel, linux-bcachefs; +Cc: Kent Overstreet

Signed-off-by: Kent Overstreet <kent.overstreet@gmail.com>
---
 .gitignore | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/.gitignore b/.gitignore
index 4cc9c80724..5a0e5206da 100644
--- a/.gitignore
+++ b/.gitignore
@@ -4,6 +4,9 @@
 .dep
 .libs
 .ltdep
+.*
+cscope.*
+tags
 
 /local.config
 /results
-- 
2.31.1


^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH 3/3] Use --yes option to lvcreate
  2021-04-27 16:44 [PATCH 0/3] bcachefs support Kent Overstreet
  2021-04-27 16:44 ` [PATCH 1/3] Initial " Kent Overstreet
  2021-04-27 16:44 ` [PATCH 2/3] Improved .gitignore Kent Overstreet
@ 2021-04-27 16:44 ` Kent Overstreet
  2021-04-27 17:03   ` Eryu Guan
  2021-05-09 14:20 ` [PATCH 0/3] bcachefs support Eryu Guan
  3 siblings, 1 reply; 19+ messages in thread
From: Kent Overstreet @ 2021-04-27 16:44 UTC (permalink / raw)
  To: fstests, linux-fsdevel, linux-bcachefs; +Cc: Kent Overstreet

This fixes spurious test failures caused by broken pipe messages.

Signed-off-by: Kent Overstreet <kent.overstreet@gmail.com>
---
 tests/generic/081 | 2 +-
 tests/generic/108 | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/generic/081 b/tests/generic/081
index 5dff079852..26702007ab 100755
--- a/tests/generic/081
+++ b/tests/generic/081
@@ -70,7 +70,7 @@ _scratch_mkfs_sized $((300 * 1024 * 1024)) >>$seqres.full 2>&1
 $LVM_PROG vgcreate -f $vgname $SCRATCH_DEV >>$seqres.full 2>&1
 # We use yes pipe instead of 'lvcreate --yes' because old version of lvm
 # (like 2.02.95 in RHEL6) don't support --yes option
-yes | $LVM_PROG lvcreate -L 256M -n $lvname $vgname >>$seqres.full 2>&1
+$LVM_PROG lvcreate --yes -L 256M -n $lvname $vgname >>$seqres.full 2>&1
 # wait for lvcreation to fully complete
 $UDEV_SETTLE_PROG >>$seqres.full 2>&1
 
diff --git a/tests/generic/108 b/tests/generic/108
index 6fb194f43c..74945fdf3c 100755
--- a/tests/generic/108
+++ b/tests/generic/108
@@ -56,7 +56,7 @@ $LVM_PROG pvcreate -f $SCSI_DEBUG_DEV $SCRATCH_DEV >>$seqres.full 2>&1
 $LVM_PROG vgcreate -f $vgname $SCSI_DEBUG_DEV $SCRATCH_DEV >>$seqres.full 2>&1
 # We use yes pipe instead of 'lvcreate --yes' because old version of lvm
 # (like 2.02.95 in RHEL6) don't support --yes option
-yes | $LVM_PROG lvcreate -i 2 -I 4m -L 275m -n $lvname $vgname \
+$LVM_PROG lvcreate --yes -i 2 -I 4m -L 275m -n $lvname $vgname \
 	>>$seqres.full 2>&1
 # wait for lv creation to fully complete
 $UDEV_SETTLE_PROG >>$seqres.full 2>&1
-- 
2.31.1


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 3/3] Use --yes option to lvcreate
  2021-04-27 16:44 ` [PATCH 3/3] Use --yes option to lvcreate Kent Overstreet
@ 2021-04-27 17:03   ` Eryu Guan
  2021-04-27 20:29     ` Kent Overstreet
  0 siblings, 1 reply; 19+ messages in thread
From: Eryu Guan @ 2021-04-27 17:03 UTC (permalink / raw)
  To: Kent Overstreet; +Cc: fstests, linux-fsdevel, linux-bcachefs

On Tue, Apr 27, 2021 at 12:44:19PM -0400, Kent Overstreet wrote:
> This fixes spurious test failures caused by broken pipe messages.
> 
> Signed-off-by: Kent Overstreet <kent.overstreet@gmail.com>
> ---
>  tests/generic/081 | 2 +-
>  tests/generic/108 | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/generic/081 b/tests/generic/081
> index 5dff079852..26702007ab 100755
> --- a/tests/generic/081
> +++ b/tests/generic/081
> @@ -70,7 +70,7 @@ _scratch_mkfs_sized $((300 * 1024 * 1024)) >>$seqres.full 2>&1
>  $LVM_PROG vgcreate -f $vgname $SCRATCH_DEV >>$seqres.full 2>&1
>  # We use yes pipe instead of 'lvcreate --yes' because old version of lvm
>  # (like 2.02.95 in RHEL6) don't support --yes option
> -yes | $LVM_PROG lvcreate -L 256M -n $lvname $vgname >>$seqres.full 2>&1
> +$LVM_PROG lvcreate --yes -L 256M -n $lvname $vgname >>$seqres.full 2>&1

Please see above comments, we use yes pipe intentionally. I don't see
how this would result in broken pipe. Would you please provide more
details? And let's see if we could fix the broken pipe issue.

Thanks,
Eryu

>  # wait for lvcreation to fully complete
>  $UDEV_SETTLE_PROG >>$seqres.full 2>&1
>  
> diff --git a/tests/generic/108 b/tests/generic/108
> index 6fb194f43c..74945fdf3c 100755
> --- a/tests/generic/108
> +++ b/tests/generic/108
> @@ -56,7 +56,7 @@ $LVM_PROG pvcreate -f $SCSI_DEBUG_DEV $SCRATCH_DEV >>$seqres.full 2>&1
>  $LVM_PROG vgcreate -f $vgname $SCSI_DEBUG_DEV $SCRATCH_DEV >>$seqres.full 2>&1
>  # We use yes pipe instead of 'lvcreate --yes' because old version of lvm
>  # (like 2.02.95 in RHEL6) don't support --yes option
> -yes | $LVM_PROG lvcreate -i 2 -I 4m -L 275m -n $lvname $vgname \
> +$LVM_PROG lvcreate --yes -i 2 -I 4m -L 275m -n $lvname $vgname \
>  	>>$seqres.full 2>&1
>  # wait for lv creation to fully complete
>  $UDEV_SETTLE_PROG >>$seqres.full 2>&1
> -- 
> 2.31.1

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 3/3] Use --yes option to lvcreate
  2021-04-27 17:03   ` Eryu Guan
@ 2021-04-27 20:29     ` Kent Overstreet
  2021-04-27 20:43       ` Matthew Wilcox
  0 siblings, 1 reply; 19+ messages in thread
From: Kent Overstreet @ 2021-04-27 20:29 UTC (permalink / raw)
  To: Eryu Guan; +Cc: fstests, linux-fsdevel, linux-bcachefs

On Wed, Apr 28, 2021 at 01:03:39AM +0800, Eryu Guan wrote:
> On Tue, Apr 27, 2021 at 12:44:19PM -0400, Kent Overstreet wrote:
> > This fixes spurious test failures caused by broken pipe messages.
> > 
> > Signed-off-by: Kent Overstreet <kent.overstreet@gmail.com>
> > ---
> >  tests/generic/081 | 2 +-
> >  tests/generic/108 | 2 +-
> >  2 files changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/tests/generic/081 b/tests/generic/081
> > index 5dff079852..26702007ab 100755
> > --- a/tests/generic/081
> > +++ b/tests/generic/081
> > @@ -70,7 +70,7 @@ _scratch_mkfs_sized $((300 * 1024 * 1024)) >>$seqres.full 2>&1
> >  $LVM_PROG vgcreate -f $vgname $SCRATCH_DEV >>$seqres.full 2>&1
> >  # We use yes pipe instead of 'lvcreate --yes' because old version of lvm
> >  # (like 2.02.95 in RHEL6) don't support --yes option
> > -yes | $LVM_PROG lvcreate -L 256M -n $lvname $vgname >>$seqres.full 2>&1
> > +$LVM_PROG lvcreate --yes -L 256M -n $lvname $vgname >>$seqres.full 2>&1
> 
> Please see above comments, we use yes pipe intentionally. I don't see
> how this would result in broken pipe. Would you please provide more
> details? And let's see if we could fix the broken pipe issue.

If lvcreate never ask y/n - never reads from standard input, then echo sees a
broken pipe when it tries to write. That's what I get without this patch.

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 3/3] Use --yes option to lvcreate
  2021-04-27 20:29     ` Kent Overstreet
@ 2021-04-27 20:43       ` Matthew Wilcox
  2021-04-27 21:02         ` Eric Biggers
  0 siblings, 1 reply; 19+ messages in thread
From: Matthew Wilcox @ 2021-04-27 20:43 UTC (permalink / raw)
  To: Kent Overstreet; +Cc: Eryu Guan, fstests, linux-fsdevel, linux-bcachefs

On Tue, Apr 27, 2021 at 04:29:23PM -0400, Kent Overstreet wrote:
> On Wed, Apr 28, 2021 at 01:03:39AM +0800, Eryu Guan wrote:
> > On Tue, Apr 27, 2021 at 12:44:19PM -0400, Kent Overstreet wrote:
> > > This fixes spurious test failures caused by broken pipe messages.
> > > 
> > > Signed-off-by: Kent Overstreet <kent.overstreet@gmail.com>
> > > ---
> > >  tests/generic/081 | 2 +-
> > >  tests/generic/108 | 2 +-
> > >  2 files changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/tests/generic/081 b/tests/generic/081
> > > index 5dff079852..26702007ab 100755
> > > --- a/tests/generic/081
> > > +++ b/tests/generic/081
> > > @@ -70,7 +70,7 @@ _scratch_mkfs_sized $((300 * 1024 * 1024)) >>$seqres.full 2>&1
> > >  $LVM_PROG vgcreate -f $vgname $SCRATCH_DEV >>$seqres.full 2>&1
> > >  # We use yes pipe instead of 'lvcreate --yes' because old version of lvm
> > >  # (like 2.02.95 in RHEL6) don't support --yes option
> > > -yes | $LVM_PROG lvcreate -L 256M -n $lvname $vgname >>$seqres.full 2>&1
> > > +$LVM_PROG lvcreate --yes -L 256M -n $lvname $vgname >>$seqres.full 2>&1
> > 
> > Please see above comments, we use yes pipe intentionally. I don't see
> > how this would result in broken pipe. Would you please provide more
> > details? And let's see if we could fix the broken pipe issue.
> 
> If lvcreate never ask y/n - never reads from standard input, then echo sees a
> broken pipe when it tries to write. That's what I get without this patch.

I think it's something in how ktest sets up the environment.  I also see
the SIGPIPEs when using your ktest scripts, but not when ssh'ing into
the guest and running the test.

What that thing is, I don't know.  I'm not tall enough to understand
signal handling.

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 3/3] Use --yes option to lvcreate
  2021-04-27 20:43       ` Matthew Wilcox
@ 2021-04-27 21:02         ` Eric Biggers
  2021-04-27 21:18           ` Kent Overstreet
  0 siblings, 1 reply; 19+ messages in thread
From: Eric Biggers @ 2021-04-27 21:02 UTC (permalink / raw)
  To: Matthew Wilcox
  Cc: Kent Overstreet, Eryu Guan, fstests, linux-fsdevel, linux-bcachefs

On Tue, Apr 27, 2021 at 09:43:19PM +0100, Matthew Wilcox wrote:
> On Tue, Apr 27, 2021 at 04:29:23PM -0400, Kent Overstreet wrote:
> > On Wed, Apr 28, 2021 at 01:03:39AM +0800, Eryu Guan wrote:
> > > On Tue, Apr 27, 2021 at 12:44:19PM -0400, Kent Overstreet wrote:
> > > > This fixes spurious test failures caused by broken pipe messages.
> > > > 
> > > > Signed-off-by: Kent Overstreet <kent.overstreet@gmail.com>
> > > > ---
> > > >  tests/generic/081 | 2 +-
> > > >  tests/generic/108 | 2 +-
> > > >  2 files changed, 2 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/tests/generic/081 b/tests/generic/081
> > > > index 5dff079852..26702007ab 100755
> > > > --- a/tests/generic/081
> > > > +++ b/tests/generic/081
> > > > @@ -70,7 +70,7 @@ _scratch_mkfs_sized $((300 * 1024 * 1024)) >>$seqres.full 2>&1
> > > >  $LVM_PROG vgcreate -f $vgname $SCRATCH_DEV >>$seqres.full 2>&1
> > > >  # We use yes pipe instead of 'lvcreate --yes' because old version of lvm
> > > >  # (like 2.02.95 in RHEL6) don't support --yes option
> > > > -yes | $LVM_PROG lvcreate -L 256M -n $lvname $vgname >>$seqres.full 2>&1
> > > > +$LVM_PROG lvcreate --yes -L 256M -n $lvname $vgname >>$seqres.full 2>&1
> > > 
> > > Please see above comments, we use yes pipe intentionally. I don't see
> > > how this would result in broken pipe. Would you please provide more
> > > details? And let's see if we could fix the broken pipe issue.
> > 
> > If lvcreate never ask y/n - never reads from standard input, then echo sees a
> > broken pipe when it tries to write. That's what I get without this patch.
> 
> I think it's something in how ktest sets up the environment.  I also see
> the SIGPIPEs when using your ktest scripts, but not when ssh'ing into
> the guest and running the test.
> 
> What that thing is, I don't know.  I'm not tall enough to understand
> signal handling.

See xfstests commit 9bcb266cd778 ("generic/397: be compatible with ignored
SIGPIPE") for an example of the same problem being fixed in another test, with
some more explanation.

But it's better to just not execute xfstests (or any shell script, for that
matter) in an environment where SIGPIPE is ignored.

- Eric

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 3/3] Use --yes option to lvcreate
  2021-04-27 21:02         ` Eric Biggers
@ 2021-04-27 21:18           ` Kent Overstreet
  0 siblings, 0 replies; 19+ messages in thread
From: Kent Overstreet @ 2021-04-27 21:18 UTC (permalink / raw)
  To: Eric Biggers
  Cc: Matthew Wilcox, Eryu Guan, fstests, linux-fsdevel, linux-bcachefs

On Tue, Apr 27, 2021 at 02:02:15PM -0700, Eric Biggers wrote:
> On Tue, Apr 27, 2021 at 09:43:19PM +0100, Matthew Wilcox wrote:
> > On Tue, Apr 27, 2021 at 04:29:23PM -0400, Kent Overstreet wrote:
> > > On Wed, Apr 28, 2021 at 01:03:39AM +0800, Eryu Guan wrote:
> > > > On Tue, Apr 27, 2021 at 12:44:19PM -0400, Kent Overstreet wrote:
> > > > > This fixes spurious test failures caused by broken pipe messages.
> > > > > 
> > > > > Signed-off-by: Kent Overstreet <kent.overstreet@gmail.com>
> > > > > ---
> > > > >  tests/generic/081 | 2 +-
> > > > >  tests/generic/108 | 2 +-
> > > > >  2 files changed, 2 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/tests/generic/081 b/tests/generic/081
> > > > > index 5dff079852..26702007ab 100755
> > > > > --- a/tests/generic/081
> > > > > +++ b/tests/generic/081
> > > > > @@ -70,7 +70,7 @@ _scratch_mkfs_sized $((300 * 1024 * 1024)) >>$seqres.full 2>&1
> > > > >  $LVM_PROG vgcreate -f $vgname $SCRATCH_DEV >>$seqres.full 2>&1
> > > > >  # We use yes pipe instead of 'lvcreate --yes' because old version of lvm
> > > > >  # (like 2.02.95 in RHEL6) don't support --yes option
> > > > > -yes | $LVM_PROG lvcreate -L 256M -n $lvname $vgname >>$seqres.full 2>&1
> > > > > +$LVM_PROG lvcreate --yes -L 256M -n $lvname $vgname >>$seqres.full 2>&1
> > > > 
> > > > Please see above comments, we use yes pipe intentionally. I don't see
> > > > how this would result in broken pipe. Would you please provide more
> > > > details? And let's see if we could fix the broken pipe issue.
> > > 
> > > If lvcreate never ask y/n - never reads from standard input, then echo sees a
> > > broken pipe when it tries to write. That's what I get without this patch.
> > 
> > I think it's something in how ktest sets up the environment.  I also see
> > the SIGPIPEs when using your ktest scripts, but not when ssh'ing into
> > the guest and running the test.
> > 
> > What that thing is, I don't know.  I'm not tall enough to understand
> > signal handling.
> 
> See xfstests commit 9bcb266cd778 ("generic/397: be compatible with ignored
> SIGPIPE") for an example of the same problem being fixed in another test, with
> some more explanation.
> 
> But it's better to just not execute xfstests (or any shell script, for that
> matter) in an environment where SIGPIPE is ignored.

Ah, thanks for the info. I'll see if I can figure out what's mucking with the
signal handler (probably systemd...)

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 0/3] bcachefs support
  2021-04-27 16:44 [PATCH 0/3] bcachefs support Kent Overstreet
                   ` (2 preceding siblings ...)
  2021-04-27 16:44 ` [PATCH 3/3] Use --yes option to lvcreate Kent Overstreet
@ 2021-05-09 14:20 ` Eryu Guan
  2021-05-10 23:14   ` Dave Chinner
  3 siblings, 1 reply; 19+ messages in thread
From: Eryu Guan @ 2021-05-09 14:20 UTC (permalink / raw)
  To: Kent Overstreet; +Cc: fstests, linux-fsdevel, linux-bcachefs

On Tue, Apr 27, 2021 at 12:44:16PM -0400, Kent Overstreet wrote:
> A small patch adding bcachefs support, and two other patches for consideration:

As bcachefs is not upstream yet, I think we should re-visit bcachefs
support after it's in upstream.

OTOH, I have some minor comments go to patch 1.

Thanks,
Eryu

> 
> Kent Overstreet (3):
>   Initial bcachefs support
>   Improved .gitignore
>   Use --yes option to lvcreate
> 
>  .gitignore         |  3 +++
>  common/attr        |  6 ++++++
>  common/config      |  3 +++
>  common/dmlogwrites |  7 +++++++
>  common/quota       |  4 ++--
>  common/rc          | 31 +++++++++++++++++++++++++++++++
>  tests/generic/042  |  3 ++-
>  tests/generic/081  |  2 +-
>  tests/generic/108  |  2 +-
>  tests/generic/425  |  3 +++
>  tests/generic/441  |  2 +-
>  tests/generic/482  | 27 ++++++++++++++++++++-------
>  tests/generic/558  |  2 ++
>  13 files changed, 82 insertions(+), 13 deletions(-)
> 
> -- 
> 2.31.1

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 1/3] Initial bcachefs support
  2021-04-27 16:44 ` [PATCH 1/3] Initial " Kent Overstreet
@ 2021-05-09 14:36   ` Eryu Guan
  2021-05-23 22:51     ` Kent Overstreet
  0 siblings, 1 reply; 19+ messages in thread
From: Eryu Guan @ 2021-05-09 14:36 UTC (permalink / raw)
  To: Kent Overstreet; +Cc: fstests, linux-fsdevel, linux-bcachefs, Kent Overstreet

On Tue, Apr 27, 2021 at 12:44:17PM -0400, Kent Overstreet wrote:
> From: Kent Overstreet <kmo@daterainc.com>

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

> 
> Signed-off-by: Kent Overstreet <kent.overstreet@gmail.com>
> ---
>  common/attr        |  6 ++++++
>  common/config      |  3 +++
>  common/dmlogwrites |  7 +++++++
>  common/quota       |  4 ++--
>  common/rc          | 31 +++++++++++++++++++++++++++++++
>  tests/generic/042  |  3 ++-
>  tests/generic/425  |  3 +++
>  tests/generic/441  |  2 +-
>  tests/generic/482  | 27 ++++++++++++++++++++-------
>  tests/generic/558  |  2 ++
>  10 files changed, 77 insertions(+), 11 deletions(-)
> 
> diff --git a/common/attr b/common/attr
> index 669909d600..42ceab9233 100644
> --- a/common/attr
> +++ b/common/attr
> @@ -33,6 +33,9 @@ _acl_get_max()
>  			echo 506
>  		fi
>  		;;
> +	bcachefs)
> +		echo 251
> +		;;
>  	*)
>  		echo 0
>  		;;
> @@ -273,6 +276,9 @@ pvfs2)
>  9p|ceph|nfs)
>  	MAX_ATTRVAL_SIZE=65536
>  	;;
> +bcachefs)
> +	MAX_ATTRVAL_SIZE=1024
> +	;;
>  *)
>  	# Assume max ~1 block of attrs
>  	BLOCK_SIZE=`_get_block_size $TEST_DIR`
> diff --git a/common/config b/common/config
> index a47e462c77..8153301483 100644
> --- a/common/config
> +++ b/common/config
> @@ -415,6 +415,9 @@ _mkfs_opts()
>  	btrfs)
>  		export MKFS_OPTIONS="$BTRFS_MKFS_OPTIONS"
>  		;;
> +	bcachefs)
> +		export MKFS_OPTIONS="--errors=panic"

I think this might be useful for bcachefs developers to find bugs
earlier, but not suitable for people like QA, as this may crash their
test machines and stop the whole test.

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

> +		;;
>  	*)
>  		;;
>  	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?

> +	fi
> +
>  	$here/src/log-writes/replay-log --log $LOGWRITES_DEV --find \
>  		--end-mark $_mark >> $seqres.full 2>&1
>  	[ $? -ne 0 ] && _fail "mark '$_mark' does not exist"
> diff --git a/common/quota b/common/quota
> index 32a9a55593..883a28a20d 100644
> --- a/common/quota
> +++ b/common/quota
> @@ -17,7 +17,7 @@ _require_quota()
>  	    _notrun "Installed kernel does not support quotas"
>  	fi
>  	;;
> -    gfs2|ocfs2)
> +    gfs2|ocfs2|bcachefs)
>  	;;
>      xfs)
>  	if [ ! -f /proc/fs/xfs/xqmstat ]; then
> @@ -278,7 +278,7 @@ _check_quota_usage()
>  
>  	VFS_QUOTA=0
>  	case $FSTYP in
> -	ext2|ext3|ext4|ext4dev|f2fs|reiserfs|gfs2)
> +	ext2|ext3|ext4|ext4dev|f2fs|reiserfs|gfs2|bcachefs)
>  		VFS_QUOTA=1
>  		quotaon -f -u -g $SCRATCH_MNT 2>/dev/null
>  		;;
> 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.

>  }
>  
>  # mount scratch device with given options and _fail if mount fails
> @@ -667,6 +668,9 @@ _test_mkfs()
>      ext2|ext3|ext4)
>  	$MKFS_PROG -t $FSTYP -- -F $MKFS_OPTIONS $* $TEST_DEV
>  	;;
> +    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.

>      *)
>  	yes | $MKFS_PROG -t $FSTYP -- $MKFS_OPTIONS $* $TEST_DEV
>  	;;
> @@ -706,6 +710,10 @@ _mkfs_dev()
>  	$MKFS_PROG -t $FSTYP -- -f $MKFS_OPTIONS $* \
>  		2>$tmp.mkfserr 1>$tmp.mkfsstd
>  	;;
> +    bcachefs)
> +	$MKFS_PROG -t $FSTYP -- $MKFS_OPTIONS $* \
> +		2>$tmp_dir.mkfserr 1>$tmp_dir.mkfsstd
> +	;;

Same as above.

>      *)
>  	yes | $MKFS_PROG -t $FSTYP -- $MKFS_OPTIONS $* \
>  		2>$tmp.mkfserr 1>$tmp.mkfsstd
> @@ -803,6 +811,10 @@ _scratch_mkfs()
>  		mkfs_cmd="yes | $MKFS_PROG -t $FSTYP --"
>  		mkfs_filter="grep -v -e ^mkfs\.ocfs2"
>  		;;
> +	bcachefs)
> +		mkfs_cmd="$MKFS_PROG -t $FSTYP --"
> +		mkfs_filter="cat"
> +		;;

Same here.

>  	*)
>  		mkfs_cmd="yes | $MKFS_PROG -t $FSTYP --"
>  		mkfs_filter="cat"
> @@ -1065,6 +1077,9 @@ _scratch_mkfs_sized()
>  		fi
>  		export MOUNT_OPTIONS="-o size=$fssize $TMPFS_MOUNT_OPTIONS"
>  		;;
> +	bcachefs)
> +		$MKFS_PROG -t $FSTYP -- $MKFS_OPTIONS --fs_size=$fssize --block_size=$blocksize $SCRATCH_DEV
> +		;;
>  	*)
>  		_notrun "Filesystem $FSTYP not supported in _scratch_mkfs_sized"
>  		;;
> @@ -1133,6 +1148,9 @@ _scratch_mkfs_blocksized()
>      ocfs2)
>  	yes | ${MKFS_PROG} -t $FSTYP -F $MKFS_OPTIONS -b $blocksize -C $blocksize $SCRATCH_DEV
>  	;;
> +    bcachefs)
> +	${MKFS_PROG}.$FSTYP $MKFS_OPTIONS --block_size=$blocksize $SCRATCH_DEV
> +	;;

Better to be consistent and use "${MKFS_PROG} -t $FSTYP ..." instead of
${MKFS_PROG}.$FSTYP

>      *)
>  	_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?

> +	res=$?
> +	case $res in
> +	0)
> +		res=0
> +		;;
> +	*)
> +		_dump_err2 "fsck.$FSTYP failed, err=$res"
> +		;;
> +	esac
> +	return $res
> +	;;
>      *)
>  	local dev=$SCRATCH_DEV
>  	local fstyp=$FSTYP
> 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.

> diff --git a/tests/generic/425 b/tests/generic/425
> index 51cbe1c67d..be2bc1b02e 100755
> --- a/tests/generic/425
> +++ b/tests/generic/425
> @@ -30,6 +30,9 @@ _cleanup()
>  
>  # real QA test starts here
>  _supported_fs generic
> +
> +[ $FSTYP = bcachefs ] && _notrun "bcachefs does not store xattrs in blocks"
> +
>  _require_scratch
>  _require_attrs
>  _require_xfs_io_command "fiemap" "-a"
> diff --git a/tests/generic/441 b/tests/generic/441
> index bedbcb0817..814387b2a9 100755
> --- a/tests/generic/441
> +++ b/tests/generic/441
> @@ -40,7 +40,7 @@ case $FSTYP in
>  	btrfs)
>  		_notrun "btrfs has a specialized test for this"
>  		;;
> -	ext3|ext4|xfs)
> +	ext3|ext4|xfs|bcachefs)
>  		# Do the more thorough test if we have a logdev
>  		_has_logdev && sflag=''
>  		;;
> diff --git a/tests/generic/482 b/tests/generic/482
> index 86941e8468..3cbe187f2e 100755
> --- a/tests/generic/482
> +++ b/tests/generic/482
> @@ -77,16 +77,29 @@ prev=$(_log_writes_mark_to_entry_number mkfs)
>  cur=$(_log_writes_find_next_fua $prev)
>  [ -z "$cur" ] && _fail "failed to locate next FUA write"
>  
> +if [ "$FSTYP" = "bcachefs" ]; then
> +    _dmthin_cleanup
> +    _dmthin_init $devsize $devsize $csize $lowspace
> +fi
> +
>  while [ ! -z "$cur" ]; do
>  	_log_writes_replay_log_range $cur $DMTHIN_VOL_DEV >> $seqres.full
>  
> -	# Here we need extra mount to replay the log, mainly for journal based
> -	# fs, as their fsck will report dirty log as error.
> -	# We don't care to preserve any data on the replay dev, as we can replay
> -	# back to the point we need, and in fact sometimes creating/deleting
> -	# snapshots repeatedly can be slower than replaying the log.
> -	_dmthin_mount
> -	_dmthin_check_fs
> +	if [ "$FSTYP" = "bcachefs" ]; then
> +	    # bcachefs will get confused if fsck does writes to replay the log,
> +	    # but then we replay writes from an earlier point in time on the
> +	    # same fs - but  fsck in -n mode won't do any writes:
> +	    _check_scratch_fs -n $DMTHIN_VOL_DEV
> +	else
> +	    # Here we need extra mount to replay the log, mainly for journal based
> +	    # fs, as their fsck will report dirty log as error.
> +	    # We don't care to preserve any data on the replay dev, as we can replay
> +	    # back to the point we need, and in fact sometimes creating/deleting
> +	    # snapshots repeatedly can be slower than replaying the log.
> +
> +	    _dmthin_mount
> +	    _dmthin_check_fs
> +	fi
>  
>  	prev=$cur
>  	cur=$(_log_writes_find_next_fua $(($cur + 1)))
> diff --git a/tests/generic/558 b/tests/generic/558
> index 4bed90e2a7..2eaa8f7686 100755
> --- a/tests/generic/558
> +++ b/tests/generic/558
> @@ -55,6 +55,8 @@ _scratch_mount
>  i=0
>  free_inode=`_get_free_inode $SCRATCH_MNT`
>  file_per_dir=1000
> +[ $FSTYP = bcachefs ] && file_per_dir=10000
> +

Some comments would be good.

Thanks,
Eryu

>  loop=$((free_inode / file_per_dir + 1))
>  mkdir -p $SCRATCH_MNT/testdir
>  
> -- 
> 2.31.1

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 0/3] bcachefs support
  2021-05-09 14:20 ` [PATCH 0/3] bcachefs support Eryu Guan
@ 2021-05-10 23:14   ` Dave Chinner
  2021-05-11  1:26     ` Darrick J. Wong
  0 siblings, 1 reply; 19+ messages in thread
From: Dave Chinner @ 2021-05-10 23:14 UTC (permalink / raw)
  To: Eryu Guan; +Cc: Kent Overstreet, fstests, linux-fsdevel, linux-bcachefs

On Sun, May 09, 2021 at 10:20:38PM +0800, Eryu Guan wrote:
> On Tue, Apr 27, 2021 at 12:44:16PM -0400, Kent Overstreet wrote:
> > A small patch adding bcachefs support, and two other patches for consideration:
> 
> As bcachefs is not upstream yet, I think we should re-visit bcachefs
> support after it's in upstream.

I disagree completely. I've been waiting for this to land for some
time so I can actually run fstests against bcachefs easily to
evaluate it's current state of stability and support.  The plans are
to get bcachefs merged upstream, and so having support already in
fstests makes it much easier for reviewers and developers to
actually run tests and find problems prior to merging.

As an upstream developer and someone who will be reviewing bcachefs
when it is next proposed for merge,  I would much prefer to see
extensive and long term fstests coverage *before* the code is even
merged upstream. Given that filesystems take years to develop to the
point where they are stable and ready for merge, saying "can't
enable the test environment until it is merged upstream" is not very
helpful.

As a general principle, we want developers of new filesystems to
start using fstests early in the development process of their
filesystem. We should be encouraging new filesystems to be added to
fstests, not saying "we only support upstream filesystems". If the
filesystem plans to be merged upstream, then fstests support for
that filesystem should be there long before the filesytsem is even
proposed for merge.  We need to help people get new filesystems
upstream, not place arbitrary "not upstream so not supported"
catch-22s in their way...

Hence I ask that you merge bcachefs support to help the process of
getting bcachefs suport upstream.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 0/3] bcachefs support
  2021-05-10 23:14   ` Dave Chinner
@ 2021-05-11  1:26     ` Darrick J. Wong
  2021-05-16 13:54       ` Eryu Guan
  0 siblings, 1 reply; 19+ messages in thread
From: Darrick J. Wong @ 2021-05-11  1:26 UTC (permalink / raw)
  To: Dave Chinner
  Cc: Eryu Guan, Kent Overstreet, fstests, linux-fsdevel, linux-bcachefs

On Tue, May 11, 2021 at 09:14:46AM +1000, Dave Chinner wrote:
> On Sun, May 09, 2021 at 10:20:38PM +0800, Eryu Guan wrote:
> > On Tue, Apr 27, 2021 at 12:44:16PM -0400, Kent Overstreet wrote:
> > > A small patch adding bcachefs support, and two other patches for consideration:
> > 
> > As bcachefs is not upstream yet, I think we should re-visit bcachefs
> > support after it's in upstream.
> 
> I disagree completely. I've been waiting for this to land for some
> time so I can actually run fstests against bcachefs easily to
> evaluate it's current state of stability and support.  The plans are
> to get bcachefs merged upstream, and so having support already in
> fstests makes it much easier for reviewers and developers to
> actually run tests and find problems prior to merging.
> 
> As an upstream developer and someone who will be reviewing bcachefs
> when it is next proposed for merge,  I would much prefer to see
> extensive and long term fstests coverage *before* the code is even
> merged upstream. Given that filesystems take years to develop to the
> point where they are stable and ready for merge, saying "can't
> enable the test environment until it is merged upstream" is not very
> helpful.
> 
> As a general principle, we want developers of new filesystems to
> start using fstests early in the development process of their
> filesystem. We should be encouraging new filesystems to be added to
> fstests, not saying "we only support upstream filesystems". If the
> filesystem plans to be merged upstream, then fstests support for
> that filesystem should be there long before the filesytsem is even
> proposed for merge.  We need to help people get new filesystems
> upstream, not place arbitrary "not upstream so not supported"
> catch-22s in their way...
> 
> Hence I ask that you merge bcachefs support to help the process of
> getting bcachefs suport upstream.

/me notes that both Eryu and Dave have been willing to merge
surprisingly large quantities of code for XFS reflink and online fsck
long before either of those features landed in mainline, so I think it's
perfectly fine to merge Kent's relatively small changes to enable
'FSTYP=bcachefs'.

(With all of Eryu's review comments fixed, obviously...)

--D

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 0/3] bcachefs support
  2021-05-11  1:26     ` Darrick J. Wong
@ 2021-05-16 13:54       ` Eryu Guan
  0 siblings, 0 replies; 19+ messages in thread
From: Eryu Guan @ 2021-05-16 13:54 UTC (permalink / raw)
  To: Darrick J. Wong
  Cc: Dave Chinner, Kent Overstreet, fstests, linux-fsdevel, linux-bcachefs

On Mon, May 10, 2021 at 06:26:45PM -0700, Darrick J. Wong wrote:
> On Tue, May 11, 2021 at 09:14:46AM +1000, Dave Chinner wrote:
> > On Sun, May 09, 2021 at 10:20:38PM +0800, Eryu Guan wrote:
> > > On Tue, Apr 27, 2021 at 12:44:16PM -0400, Kent Overstreet wrote:
> > > > A small patch adding bcachefs support, and two other patches for consideration:
> > > 
> > > As bcachefs is not upstream yet, I think we should re-visit bcachefs
> > > support after it's in upstream.
> > 
> > I disagree completely. I've been waiting for this to land for some
> > time so I can actually run fstests against bcachefs easily to
> > evaluate it's current state of stability and support.  The plans are
> > to get bcachefs merged upstream, and so having support already in
> > fstests makes it much easier for reviewers and developers to
> > actually run tests and find problems prior to merging.
> > 
> > As an upstream developer and someone who will be reviewing bcachefs
> > when it is next proposed for merge,  I would much prefer to see
> > extensive and long term fstests coverage *before* the code is even
> > merged upstream. Given that filesystems take years to develop to the
> > point where they are stable and ready for merge, saying "can't
> > enable the test environment until it is merged upstream" is not very
> > helpful.
> > 
> > As a general principle, we want developers of new filesystems to
> > start using fstests early in the development process of their
> > filesystem. We should be encouraging new filesystems to be added to
> > fstests, not saying "we only support upstream filesystems". If the
> > filesystem plans to be merged upstream, then fstests support for
> > that filesystem should be there long before the filesytsem is even
> > proposed for merge.  We need to help people get new filesystems
> > upstream, not place arbitrary "not upstream so not supported"
> > catch-22s in their way...

OK, that makes sense. Actually, I should have made my "upstream first"
more clear. I'd love to merge non-upstream features/new fs supports if
the proposed new feature/new filesystem has been developmented actively
and the community has generally made the agreement that will merge the
new feature/filesystem when it's in a good shape. IOW, it's not some
random new features/filesystems, which are very likely to be dropped in
the half way.

And providing such info in the patch is very helpful to reviewers.

> > 
> > Hence I ask that you merge bcachefs support to help the process of
> > getting bcachefs suport upstream.

Sure, bcachefs looks promising to me now :)

> 
> /me notes that both Eryu and Dave have been willing to merge
> surprisingly large quantities of code for XFS reflink and online fsck
> long before either of those features landed in mainline, so I think it's

Because I know you and the xfs commnity will work on it continuously and
very unlikely make the new tests dead code :)

Thanks,
Eryu

> perfectly fine to merge Kent's relatively small changes to enable
> 'FSTYP=bcachefs'.
> 
> (With all of Eryu's review comments fixed, obviously...)
> 
> --D
> 
> > Cheers,
> > 
> > Dave.
> > -- 
> > Dave Chinner
> > david@fromorbit.com

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 1/3] Initial bcachefs support
  2021-05-09 14:36   ` Eryu Guan
@ 2021-05-23 22:51     ` Kent Overstreet
  2021-05-24  3:56       ` Eryu Guan
  0 siblings, 1 reply; 19+ messages in thread
From: Kent Overstreet @ 2021-05-23 22:51 UTC (permalink / raw)
  To: Eryu Guan; +Cc: fstests, linux-fsdevel, linux-bcachefs, Kent Overstreet

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:
> > From: Kent Overstreet <kmo@daterainc.com>
> 
> 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.
> 
> > +		;;
> >  	*)
> >  		;;
> >  	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.

> > 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.

> > +    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.
> >      *)
> >  	_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.

> > 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.

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 1/3] Initial bcachefs support
  2021-05-23 22:51     ` Kent Overstreet
@ 2021-05-24  3:56       ` Eryu Guan
  2021-05-24  4:04         ` Kent Overstreet
  0 siblings, 1 reply; 19+ messages in thread
From: Eryu Guan @ 2021-05-24  3:56 UTC (permalink / raw)
  To: Kent Overstreet
  Cc: Eryu Guan, fstests, linux-fsdevel, linux-bcachefs, Kent Overstreet

On Sun, May 23, 2021 at 06:51:49PM -0400, Kent Overstreet wrote:
[snip]

> > >  	;;
> > > @@ -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.

Then _check_scratch_fs() should be used instead, which will fail the
test if any fsck finds any corruptions. _repair_scratch_fs() is meant to
fix errors, and only report failure when there's unfixable errors.

Thanks,
Eryu

> 
> > > 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.

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 1/3] Initial bcachefs support
  2021-05-24  3:56       ` Eryu Guan
@ 2021-05-24  4:04         ` Kent Overstreet
  2021-05-24  4:22           ` Eryu Guan
  0 siblings, 1 reply; 19+ messages in thread
From: Kent Overstreet @ 2021-05-24  4:04 UTC (permalink / raw)
  To: Eryu Guan
  Cc: Eryu Guan, fstests, linux-fsdevel, linux-bcachefs, Kent Overstreet

On Mon, May 24, 2021 at 11:56:04AM +0800, Eryu Guan wrote:
> On Sun, May 23, 2021 at 06:51:49PM -0400, Kent Overstreet wrote:
> [snip]
> 
> > > >  	;;
> > > > @@ -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.
> 
> Then _check_scratch_fs() should be used instead, which will fail the
> test if any fsck finds any corruptions. _repair_scratch_fs() is meant to
> fix errors, and only report failure when there's unfixable errors.

I see no reason to make such a change to generic tests that were written for
other filesystems, when this gets me exactly what I want.

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 1/3] Initial bcachefs support
  2021-05-24  4:04         ` Kent Overstreet
@ 2021-05-24  4:22           ` Eryu Guan
  2021-05-24  4:48             ` Kent Overstreet
  0 siblings, 1 reply; 19+ messages in thread
From: Eryu Guan @ 2021-05-24  4:22 UTC (permalink / raw)
  To: Kent Overstreet
  Cc: Eryu Guan, fstests, linux-fsdevel, linux-bcachefs, Kent Overstreet

On Mon, May 24, 2021 at 12:04:32AM -0400, Kent Overstreet wrote:
> On Mon, May 24, 2021 at 11:56:04AM +0800, Eryu Guan wrote:
> > On Sun, May 23, 2021 at 06:51:49PM -0400, Kent Overstreet wrote:
> > [snip]
> > 
> > > > >  	;;
> > > > > @@ -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.
> > 
> > Then _check_scratch_fs() should be used instead, which will fail the
> > test if any fsck finds any corruptions. _repair_scratch_fs() is meant to
> > fix errors, and only report failure when there's unfixable errors.
> 
> I see no reason to make such a change to generic tests that were written for
> other filesystems, when this gets me exactly what I want.

Oh, I noticed that "with bcachefs fsck finding errors _always_ indicates
a bug" now.. Then I think using fsck -n is fine here, but better with
comments to describe why this is ok for bcachefs.

Also, we could just use _check_scratch_fs here instead of the open-coded
fsck command. As _check_scratch_fs calls _check_generic_filesystem()
which calls fsck -n internally, and handles mount/umount device
correctly and prints pretty log if there's any errors.

Thanks,
Eryu

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 1/3] Initial bcachefs support
  2021-05-24  4:22           ` Eryu Guan
@ 2021-05-24  4:48             ` Kent Overstreet
  0 siblings, 0 replies; 19+ messages in thread
From: Kent Overstreet @ 2021-05-24  4:48 UTC (permalink / raw)
  To: Eryu Guan
  Cc: Eryu Guan, fstests, linux-fsdevel, linux-bcachefs, Kent Overstreet

On Mon, May 24, 2021 at 12:22:53PM +0800, Eryu Guan wrote:
> Oh, I noticed that "with bcachefs fsck finding errors _always_ indicates
> a bug" now.. Then I think using fsck -n is fine here, but better with
> comments to describe why this is ok for bcachefs.
> 
> Also, we could just use _check_scratch_fs here instead of the open-coded
> fsck command. As _check_scratch_fs calls _check_generic_filesystem()
> which calls fsck -n internally, and handles mount/umount device
> correctly and prints pretty log if there's any errors.

That should work.

^ permalink raw reply	[flat|nested] 19+ messages in thread

end of thread, other threads:[~2021-05-24  4:48 UTC | newest]

Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-27 16:44 [PATCH 0/3] bcachefs support Kent Overstreet
2021-04-27 16:44 ` [PATCH 1/3] Initial " Kent Overstreet
2021-05-09 14:36   ` Eryu Guan
2021-05-23 22:51     ` Kent Overstreet
2021-05-24  3:56       ` Eryu Guan
2021-05-24  4:04         ` Kent Overstreet
2021-05-24  4:22           ` Eryu Guan
2021-05-24  4:48             ` Kent Overstreet
2021-04-27 16:44 ` [PATCH 2/3] Improved .gitignore Kent Overstreet
2021-04-27 16:44 ` [PATCH 3/3] Use --yes option to lvcreate Kent Overstreet
2021-04-27 17:03   ` Eryu Guan
2021-04-27 20:29     ` Kent Overstreet
2021-04-27 20:43       ` Matthew Wilcox
2021-04-27 21:02         ` Eric Biggers
2021-04-27 21:18           ` Kent Overstreet
2021-05-09 14:20 ` [PATCH 0/3] bcachefs support Eryu Guan
2021-05-10 23:14   ` Dave Chinner
2021-05-11  1:26     ` Darrick J. Wong
2021-05-16 13:54       ` Eryu Guan

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).