diff mbox series

[5/9] common/dmthin: make this work with external log devices

Message ID 161836230182.2754991.16864806174255630147.stgit@magnolia (mailing list archive)
State New, archived
Headers show
Series fstests: random fixes | expand

Commit Message

Darrick J. Wong April 14, 2021, 1:05 a.m. UTC
From: Darrick J. Wong <djwong@kernel.org>

Provide a mkfs helper to format the dm thin device when external devices
are in use, and fix the dmthin mount helper to support them.  This fixes
regressions in generic/347 and generic/500 when external logs are in
use.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 common/dmthin     |    9 ++++++++-
 tests/generic/223 |    3 +++
 tests/generic/347 |    2 +-
 tests/generic/500 |    2 +-
 4 files changed, 13 insertions(+), 3 deletions(-)

Comments

Christoph Hellwig April 14, 2021, 6:17 a.m. UTC | #1
Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>
Eryu Guan April 18, 2021, 12:25 p.m. UTC | #2
On Tue, Apr 13, 2021 at 06:05:01PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Provide a mkfs helper to format the dm thin device when external devices
> are in use, and fix the dmthin mount helper to support them.  This fixes
> regressions in generic/347 and generic/500 when external logs are in
> use.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  common/dmthin     |    9 ++++++++-
>  tests/generic/223 |    3 +++
>  tests/generic/347 |    2 +-
>  tests/generic/500 |    2 +-
>  4 files changed, 13 insertions(+), 3 deletions(-)
> 
> 
> diff --git a/common/dmthin b/common/dmthin
> index c58c3948..3b1c7d45 100644
> --- a/common/dmthin
> +++ b/common/dmthin
> @@ -218,10 +218,17 @@ _dmthin_set_fail()
>  
>  _dmthin_mount_options()
>  {
> -	echo `_common_dev_mount_options $*` $DMTHIN_VOL_DEV $SCRATCH_MNT
> +	_scratch_options mount
> +	echo `_common_dev_mount_options $*` $SCRATCH_OPTIONS $DMTHIN_VOL_DEV $SCRATCH_MNT
>  }
>  
>  _dmthin_mount()
>  {
>  	_mount -t $FSTYP `_dmthin_mount_options $*`
>  }
> +
> +_dmthin_mkfs()
> +{
> +	_scratch_options mkfs
> +	_mkfs_dev $SCRATCH_OPTIONS $@ $DMTHIN_VOL_DEV
> +}
> diff --git a/tests/generic/223 b/tests/generic/223
> index 1f85efe5..a5ace82f 100755
> --- a/tests/generic/223
> +++ b/tests/generic/223
> @@ -43,6 +43,9 @@ for SUNIT_K in 8 16 32 64 128; do
>  	_scratch_mkfs_geom $SUNIT_BYTES 4 $BLOCKSIZE >> $seqres.full 2>&1
>  	_scratch_mount
>  
> +	# Make sure everything is on the data device
> +	$XFS_IO_PROG -c 'chattr -t' $SCRATCH_MNT

What does this do for non-xfs filesystems? Do we need a FSTYP check and
do chattr only on XFS?

Thanks,
Eryu

> +
>  	for SIZE_MULT in 1 2 8 64 256; do
>  		let SIZE=$SIZE_MULT*$SUNIT_BYTES
>  
> diff --git a/tests/generic/347 b/tests/generic/347
> index cbc5150a..e970ac10 100755
> --- a/tests/generic/347
> +++ b/tests/generic/347
> @@ -31,7 +31,7 @@ _setup_thin()
>  {
>  	_dmthin_init $BACKING_SIZE $VIRTUAL_SIZE
>  	_dmthin_set_queue
> -	_mkfs_dev $DMTHIN_VOL_DEV
> +	_dmthin_mkfs
>  	_dmthin_mount
>  }
>  
> diff --git a/tests/generic/500 b/tests/generic/500
> index 085ddbf3..5ab2f78c 100755
> --- a/tests/generic/500
> +++ b/tests/generic/500
> @@ -68,7 +68,7 @@ CLUSTER_SIZE=$((64 * 1024 / 512))		# 64K
>  
>  _dmthin_init $BACKING_SIZE $VIRTUAL_SIZE $CLUSTER_SIZE 0
>  _dmthin_set_fail
> -_mkfs_dev $DMTHIN_VOL_DEV
> +_dmthin_mkfs
>  _dmthin_mount
>  
>  # There're two bugs at here, one is dm-thin bug, the other is filesystem
Theodore Ts'o April 18, 2021, 2:44 p.m. UTC | #3
On Sun, Apr 18, 2021 at 08:25:48PM +0800, Eryu Guan wrote:
> > diff --git a/tests/generic/223 b/tests/generic/223
> > index 1f85efe5..a5ace82f 100755
> > --- a/tests/generic/223
> > +++ b/tests/generic/223
> > @@ -43,6 +43,9 @@ for SUNIT_K in 8 16 32 64 128; do
> >  	_scratch_mkfs_geom $SUNIT_BYTES 4 $BLOCKSIZE >> $seqres.full 2>&1
> >  	_scratch_mount
> >  
> > +	# Make sure everything is on the data device
> > +	$XFS_IO_PROG -c 'chattr -t' $SCRATCH_MNT
> 
> What does this do for non-xfs filesystems? Do we need a FSTYP check and
> do chattr only on XFS?

This clears the FS_NOTAIL_FL flag, which prevents tail merging, on the
root directory of the mounted scratch file system.  That should be
harmless on non-xfs file systems; in fact, the only file system that
even uses NOTAIL_FL flag is reiserfs, and the NOTAIL_FL flag is not
set by default on the root directory of a newly created reiserfs file
system.

However, by default reiserfs does not support the
FS_IOC_{GET,SET}FLAGS ioctl unless the mount option "attrs" is given.
Why, I have no idea:

root@kvm-xfstests:~# mount -t reiserfs /vtmp/foo.img /mnt
root@kvm-xfstests:~# xfs_io -c 'chattr -t' /mnt
xfs_io: cannot get flags on /mnt: Inappropriate ioctl for device

So it might be a good idea to redirect stderr for the xfs_io
invocation to /dev/null, for those file systems which do not support
the FS_IOC_{GET,SET}FLAGS ioctls.

I also have no idea why this helps for xfs --- I would think it's a
no-op, but I'm guessing there's some magical side-effect which is
taking place when FS_IOC_SETFLAGS ioctl is processed?  Maybe it would
be worth a comment explaining what is going on --- and whether this is
going to make any difference if the patch series which unifies
FS_IOC_{GETSET}FLAGS handling is merged?

						- Ted
Dave Chinner April 19, 2021, 2:04 a.m. UTC | #4
On Sun, Apr 18, 2021 at 10:44:08AM -0400, Theodore Ts'o wrote:
> On Sun, Apr 18, 2021 at 08:25:48PM +0800, Eryu Guan wrote:
> > > diff --git a/tests/generic/223 b/tests/generic/223
> > > index 1f85efe5..a5ace82f 100755
> > > --- a/tests/generic/223
> > > +++ b/tests/generic/223
> > > @@ -43,6 +43,9 @@ for SUNIT_K in 8 16 32 64 128; do
> > >  	_scratch_mkfs_geom $SUNIT_BYTES 4 $BLOCKSIZE >> $seqres.full 2>&1
> > >  	_scratch_mount
> > >  
> > > +	# Make sure everything is on the data device
> > > +	$XFS_IO_PROG -c 'chattr -t' $SCRATCH_MNT
> > 
> > What does this do for non-xfs filesystems? Do we need a FSTYP check and
> > do chattr only on XFS?
> 
> This clears the FS_NOTAIL_FL flag, which prevents tail merging, on the

No, this is not the 'chattr' CLI program. This is the xfs_io
'chattr' command, and they have different attribute namespaces. See
xfs_io for the definitions, but in this case:

	 t    inherit realtime flag (XFS_XFLAG_RTINHERIT)

And so clearing that flag ensures that all newly created files
are on the data device, as per the comment...

> I also have no idea why this helps for xfs --- I would think it's a
> no-op,

Because it's not what you think it is. :)

Cheers,

Dave.
Darrick J. Wong April 19, 2021, 4:14 p.m. UTC | #5
On Mon, Apr 19, 2021 at 12:04:42PM +1000, Dave Chinner wrote:
> On Sun, Apr 18, 2021 at 10:44:08AM -0400, Theodore Ts'o wrote:
> > On Sun, Apr 18, 2021 at 08:25:48PM +0800, Eryu Guan wrote:
> > > > diff --git a/tests/generic/223 b/tests/generic/223
> > > > index 1f85efe5..a5ace82f 100755
> > > > --- a/tests/generic/223
> > > > +++ b/tests/generic/223
> > > > @@ -43,6 +43,9 @@ for SUNIT_K in 8 16 32 64 128; do
> > > >  	_scratch_mkfs_geom $SUNIT_BYTES 4 $BLOCKSIZE >> $seqres.full 2>&1
> > > >  	_scratch_mount
> > > >  
> > > > +	# Make sure everything is on the data device
> > > > +	$XFS_IO_PROG -c 'chattr -t' $SCRATCH_MNT
> > > 
> > > What does this do for non-xfs filesystems? Do we need a FSTYP check and
> > > do chattr only on XFS?

Yes, I think so.  Will fix.

> > This clears the FS_NOTAIL_FL flag, which prevents tail merging, on the
> 
> No, this is not the 'chattr' CLI program. This is the xfs_io
> 'chattr' command, and they have different attribute namespaces. See
> xfs_io for the definitions, but in this case:
> 
> 	 t    inherit realtime flag (XFS_XFLAG_RTINHERIT)
> 
> And so clearing that flag ensures that all newly created files
> are on the data device, as per the comment...
> 
> > I also have no idea why this helps for xfs --- I would think it's a
> > no-op,
> 
> Because it's not what you think it is. :)

(Indeed.)

--D

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com
diff mbox series

Patch

diff --git a/common/dmthin b/common/dmthin
index c58c3948..3b1c7d45 100644
--- a/common/dmthin
+++ b/common/dmthin
@@ -218,10 +218,17 @@  _dmthin_set_fail()
 
 _dmthin_mount_options()
 {
-	echo `_common_dev_mount_options $*` $DMTHIN_VOL_DEV $SCRATCH_MNT
+	_scratch_options mount
+	echo `_common_dev_mount_options $*` $SCRATCH_OPTIONS $DMTHIN_VOL_DEV $SCRATCH_MNT
 }
 
 _dmthin_mount()
 {
 	_mount -t $FSTYP `_dmthin_mount_options $*`
 }
+
+_dmthin_mkfs()
+{
+	_scratch_options mkfs
+	_mkfs_dev $SCRATCH_OPTIONS $@ $DMTHIN_VOL_DEV
+}
diff --git a/tests/generic/223 b/tests/generic/223
index 1f85efe5..a5ace82f 100755
--- a/tests/generic/223
+++ b/tests/generic/223
@@ -43,6 +43,9 @@  for SUNIT_K in 8 16 32 64 128; do
 	_scratch_mkfs_geom $SUNIT_BYTES 4 $BLOCKSIZE >> $seqres.full 2>&1
 	_scratch_mount
 
+	# Make sure everything is on the data device
+	$XFS_IO_PROG -c 'chattr -t' $SCRATCH_MNT
+
 	for SIZE_MULT in 1 2 8 64 256; do
 		let SIZE=$SIZE_MULT*$SUNIT_BYTES
 
diff --git a/tests/generic/347 b/tests/generic/347
index cbc5150a..e970ac10 100755
--- a/tests/generic/347
+++ b/tests/generic/347
@@ -31,7 +31,7 @@  _setup_thin()
 {
 	_dmthin_init $BACKING_SIZE $VIRTUAL_SIZE
 	_dmthin_set_queue
-	_mkfs_dev $DMTHIN_VOL_DEV
+	_dmthin_mkfs
 	_dmthin_mount
 }
 
diff --git a/tests/generic/500 b/tests/generic/500
index 085ddbf3..5ab2f78c 100755
--- a/tests/generic/500
+++ b/tests/generic/500
@@ -68,7 +68,7 @@  CLUSTER_SIZE=$((64 * 1024 / 512))		# 64K
 
 _dmthin_init $BACKING_SIZE $VIRTUAL_SIZE $CLUSTER_SIZE 0
 _dmthin_set_fail
-_mkfs_dev $DMTHIN_VOL_DEV
+_dmthin_mkfs
 _dmthin_mount
 
 # There're two bugs at here, one is dm-thin bug, the other is filesystem