[PATCH 0/5] tmpfs fixes

[PATCH 0/5] tmpfs fixes

[Daniel Gomez]

This series addresses some minor issues found in tmpfs FSQA suit test case.

The last patch adds information about mkfs and mount options for TEST and
SCRATCH devices. This is important to clarify especially if RECREATE_TEST_DEV
is disabled (which is the default case). The new output will look like the
following:

SECTION       -- tmpfs_noswap_huge_always
RECREATING    -- tmpfs on /media/test
FSTYP         -- tmpfs
PLATFORM      -- Linux/aarch64 localhost 6.10.0-rc3-dgc #6 SMP Thu Jun 13 22:48:46 CEST 2024
TEST device:
MKFS_OPTIONS  -- /media/test
MOUNT_OPTIONS -- -o size=1G /media/test /media/test
SCRATCH device:
MKFS_OPTIONS  -- /media/scratch
MOUNT_OPTIONS -- -o size=1G -o noswap,huge=always /media/scratch /media/scratch

Regarding $TMPFS_MOUNT_OPTIONS, when it is defined in the [default] section of
a configuration file, the mount options specified there will be prefixed to the
test section options. Is this the correct and expected behaviour?

Example:

```localhost.config
[default]
FSTYP=tmpfs
TEST_DIR=/media/test
SCRATCH_MNT=/media/scratch
SCRATCH_DEV=/media/scratch
RESULT_BASE=$PWD/results/$HOST/$(uname -r)
TEST_DEV=/media/test
CANON_DEVS=yes
TMPFS_MOUNT_OPTIONS="-o size=3G"

[tmpfs_noswap_huge_always]
TMPFS_MOUNT_OPTIONS="-o noswap,huge=always"
```

Note: The above config would result in '-o size=1G -o size=3G -o
noswap,huge=always' at mount time for tmpfs_noswap_huge_always section. The '-o
size=1G' is a hardcoded prefix mount option.

Daniel

Daniel Gomez (4):
  common/config: fix RECREATE_TEST_DEV initialization
  common/rc: add recreation support for tmpfs
  common/rc: fix scratch mount options for tmpfs
  common/rc: print scratch and test mount options

Hugh Dickins (1):
  generic/449: not run on tmpfs earlier

 check             |  4 ++++
 common/config     |  2 +-
 common/rc         | 25 ++++++++++++++++++++++---
 tests/generic/449 |  6 ++++++
 4 files changed, 33 insertions(+), 4 deletions(-)

-- 
2.43.0

[PATCH 2/5] common/config: fix RECREATE_TEST_DEV initialization

[Daniel Gomez]

Do not allow the overwriting of the RECREATE_TEST_DEV variable. When
this variable is enabled, common/rc -> common/config will reset it
to false after the test device recreation process. This allows for
differentiation in mount options for SCRATCH and TEST.

Signed-off-by: Daniel Gomez <da.gomez@samsung.com>
---
 common/config | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/common/config b/common/config
index a1cd14de6..43b32b93d 100644
--- a/common/config
+++ b/common/config
@@ -94,7 +94,7 @@ export PWD=`pwd`
 export MALLOCLIB=${MALLOCLIB:=/usr/lib/libefence.a}
 export LOCAL_CONFIGURE_OPTIONS=${LOCAL_CONFIGURE_OPTIONS:=--enable-readline=yes}
 
-export RECREATE_TEST_DEV=false
+export RECREATE_TEST_DEV=${RECREATE_TEST_DEV:=false}
 
 # Handle mkfs.$fstyp which does (or does not) require -f to overwrite
 set_mkfs_prog_path_with_opts()
-- 
2.43.0

Re: [PATCH 2/5] common/config: fix RECREATE_TEST_DEV initialization

[Darrick J. Wong]

On Fri, Jun 14, 2024 at 06:17:24AM +0000, Daniel Gomez wrote:
> Do not allow the overwriting of the RECREATE_TEST_DEV variable. When
> this variable is enabled, common/rc -> common/config will reset it
> to false after the test device recreation process. This allows for
> differentiation in mount options for SCRATCH and TEST.
> 
> Signed-off-by: Daniel Gomez <da.gomez@samsung.com>

Seems sensible to me -- all the other config section variables work this
way (previous value left alone unless explicitly set by the new
section).

Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  common/config | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/common/config b/common/config
> index a1cd14de6..43b32b93d 100644
> --- a/common/config
> +++ b/common/config
> @@ -94,7 +94,7 @@ export PWD=`pwd`
>  export MALLOCLIB=${MALLOCLIB:=/usr/lib/libefence.a}
>  export LOCAL_CONFIGURE_OPTIONS=${LOCAL_CONFIGURE_OPTIONS:=--enable-readline=yes}
>  
> -export RECREATE_TEST_DEV=false
> +export RECREATE_TEST_DEV=${RECREATE_TEST_DEV:=false}
>  
>  # Handle mkfs.$fstyp which does (or does not) require -f to overwrite
>  set_mkfs_prog_path_with_opts()
> -- 
> 2.43.0
> 

Re: [PATCH 2/5] common/config: fix RECREATE_TEST_DEV initialization

[Zorro Lang]

On Fri, Jun 14, 2024 at 06:17:24AM +0000, Daniel Gomez wrote:
> Do not allow the overwriting of the RECREATE_TEST_DEV variable. When
> this variable is enabled, common/rc -> common/config will reset it
> to false after the test device recreation process. This allows for
> differentiation in mount options for SCRATCH and TEST.
> 
> Signed-off-by: Daniel Gomez <da.gomez@samsung.com>
> ---
>  common/config | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/common/config b/common/config
> index a1cd14de6..43b32b93d 100644
> --- a/common/config
> +++ b/common/config
> @@ -94,7 +94,7 @@ export PWD=`pwd`
>  export MALLOCLIB=${MALLOCLIB:=/usr/lib/libefence.a}
>  export LOCAL_CONFIGURE_OPTIONS=${LOCAL_CONFIGURE_OPTIONS:=--enable-readline=yes}
>  
> -export RECREATE_TEST_DEV=false
> +export RECREATE_TEST_DEV=${RECREATE_TEST_DEV:=false}

Oh, that makes sense. Looks like the RECREATE_TEST_DEV doesn't work
long time. I'm wondering why we didn't notice that in 87f90a2dae7a4a
("common/rc: Enable _test_mkfs to force a mkfs on a xfs filesystem")

Anyway,

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

>  
>  # Handle mkfs.$fstyp which does (or does not) require -f to overwrite
>  set_mkfs_prog_path_with_opts()
> -- 
> 2.43.0
> 

[PATCH 1/5] generic/449: not run on tmpfs earlier

[Daniel Gomez]

From: Hugh Dickins <hughd@google.com>

Do not waste 14 minutes to discover that tmpfs succeeds in
setting acls despite running out of space for user attrs.

Signed-off-by: Hugh Dickins <hughd@google.com>
---
 tests/generic/449 | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/tests/generic/449 b/tests/generic/449
index 2b77a6a49..ffbad12e9 100755
--- a/tests/generic/449
+++ b/tests/generic/449
@@ -23,6 +23,12 @@ _require_scratch
 _require_test
 _require_acls
 _require_attrs trusted
+_require_block_device $SCRATCH_DEV # hack to exclude tmpfs for now
+
+if [ "$FSTYP" = "tmpfs" ]; then
+	# Do not waste 14 minutes to discover this:
+	_notrun "$FSTYP succeeds in setting acls despite running out of space for user attrs"
+fi
 
 _scratch_mkfs_sized $((256 * 1024 * 1024)) >> $seqres.full 2>&1
 _scratch_mount || _fail "mount failed"
-- 
2.43.0

Re: [PATCH 1/5] generic/449: not run on tmpfs earlier

[Darrick J. Wong]

On Fri, Jun 14, 2024 at 06:17:24AM +0000, Daniel Gomez wrote:
> From: Hugh Dickins <hughd@google.com>
> 
> Do not waste 14 minutes to discover that tmpfs succeeds in
> setting acls despite running out of space for user attrs.
> 
> Signed-off-by: Hugh Dickins <hughd@google.com>
> ---
>  tests/generic/449 | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/tests/generic/449 b/tests/generic/449
> index 2b77a6a49..ffbad12e9 100755
> --- a/tests/generic/449
> +++ b/tests/generic/449
> @@ -23,6 +23,12 @@ _require_scratch
>  _require_test
>  _require_acls
>  _require_attrs trusted
> +_require_block_device $SCRATCH_DEV # hack to exclude tmpfs for now

If you're going to _notrun tmpfs below, why is this ^^^ hack needed?

> +if [ "$FSTYP" = "tmpfs" ]; then
> +	# Do not waste 14 minutes to discover this:
> +	_notrun "$FSTYP succeeds in setting acls despite running out of space for user attrs"

and this should explain /why/ this test should be skipped for tmpfs:

_notrun "$FSTYP does not allocate acls and data from the same space pools"

(assuming tmpfs does not in fact charge acl and data to the same account
like ondisk filesystems have to)

--D

> +fi
>  
>  _scratch_mkfs_sized $((256 * 1024 * 1024)) >> $seqres.full 2>&1
>  _scratch_mount || _fail "mount failed"
> -- 
> 2.43.0
> 

[PATCH 4/5] common/rc: fix scratch mount options for tmpfs

[Daniel Gomez]

Make sure tmpfs scratch device inherits the extra tmpfs mount options
variable (TMPFS_MOUNT_OPTIONS).

Signed-off-by: Daniel Gomez <da.gomez@samsung.com>
---
 common/rc | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/common/rc b/common/rc
index 7f995b0fa..a42792c37 100644
--- a/common/rc
+++ b/common/rc
@@ -224,7 +224,7 @@ _test_options()
 # hosted on $SCRATCH_DEV, so can't use external scratch devices).
 _common_dev_mount_options()
 {
-	echo $MOUNT_OPTIONS $SELINUX_MOUNT_OPTIONS $*
+	echo $MOUNT_OPTIONS $SELINUX_MOUNT_OPTIONS $TMPFS_MOUNT_OPTIONS $*
 }
 
 _scratch_mount_options()
-- 
2.43.0

Re: [PATCH 4/5] common/rc: fix scratch mount options for tmpfs

[Darrick J. Wong]

On Fri, Jun 14, 2024 at 06:17:25AM +0000, Daniel Gomez wrote:
> Make sure tmpfs scratch device inherits the extra tmpfs mount options
> variable (TMPFS_MOUNT_OPTIONS).
> 
> Signed-off-by: Daniel Gomez <da.gomez@samsung.com>
> ---
>  common/rc | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/common/rc b/common/rc
> index 7f995b0fa..a42792c37 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -224,7 +224,7 @@ _test_options()
>  # hosted on $SCRATCH_DEV, so can't use external scratch devices).
>  _common_dev_mount_options()
>  {
> -	echo $MOUNT_OPTIONS $SELINUX_MOUNT_OPTIONS $*
> +	echo $MOUNT_OPTIONS $SELINUX_MOUNT_OPTIONS $TMPFS_MOUNT_OPTIONS $*

Why is it necessary to include tmpfs mount options for all fs types?
XFS does not accept tmpfs mount options.

For that matter, why is TMPFS_MOUNT_OPTIONS necessary at all?  Shouldn't
that simply be "MOUNT_OPTIONS=<stuff> FSTYP=tmpfs ./check" ?

--D

>  }
>  
>  _scratch_mount_options()
> -- 
> 2.43.0
> 

Re: [PATCH 4/5] common/rc: fix scratch mount options for tmpfs

[Daniel Gomez]

On Fri, Jun 14, 2024 at 08:47:28AM GMT, Darrick J. Wong wrote:
> On Fri, Jun 14, 2024 at 06:17:25AM +0000, Daniel Gomez wrote:
> > Make sure tmpfs scratch device inherits the extra tmpfs mount options
> > variable (TMPFS_MOUNT_OPTIONS).
> > 
> > Signed-off-by: Daniel Gomez <da.gomez@samsung.com>
> > ---
> >  common/rc | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/common/rc b/common/rc
> > index 7f995b0fa..a42792c37 100644
> > --- a/common/rc
> > +++ b/common/rc
> > @@ -224,7 +224,7 @@ _test_options()
> >  # hosted on $SCRATCH_DEV, so can't use external scratch devices).
> >  _common_dev_mount_options()
> >  {
> > -	echo $MOUNT_OPTIONS $SELINUX_MOUNT_OPTIONS $*
> > +	echo $MOUNT_OPTIONS $SELINUX_MOUNT_OPTIONS $TMPFS_MOUNT_OPTIONS $*
> 
> Why is it necessary to include tmpfs mount options for all fs types?
> XFS does not accept tmpfs mount options.

You are right. We should not do this.

However, _scratch_mount_options() calls _common_dev_mount_options() ignoring the
specific mount options based on fstyp. Replacing it with _common_mount_opts()
makes this work. In addition, _common_dev_mount_options() function description
says 'Used for mounting non-scratch devices with the safe set of scratch mount
options'. So, why is it used to mount scratch devices?

This fixes the issue:

diff --git a/common/rc b/common/rc
index 51827119c..627dbaaaa 100644
--- a/common/rc
+++ b/common/rc
@@ -231,8 +231,8 @@ _scratch_mount_options()
 {
        _scratch_options mount

-       echo `_common_dev_mount_options $*` $SCRATCH_OPTIONS \
-                                       $SCRATCH_DEV $SCRATCH_MNT
+       echo `_common_mount_opts` $SCRATCH_OPTIONS \
+                                       $SCRATCH_DEV $SCRATCH_MNT $*
 }

> 
> For that matter, why is TMPFS_MOUNT_OPTIONS necessary at all?  Shouldn't
> that simply be "MOUNT_OPTIONS=<stuff> FSTYP=tmpfs ./check" ?

TMPFS_MOUNT_OPTIONS is used to specify mount options in each section of the
configuration file. For example, the following snippet is part of my conf file:

```
[tmpfs_noswap_huge_always]
TMPFS_MOUNT_OPTIONS="-o noswap,huge=always"

[tmpfs_noswap_huge_within_size]
TMPFS_MOUNT_OPTIONS="-o noswap,huge=within_size"
```

Then I run -s option to switch between profiles, e.g., 'check -s
tmpfs_noswap_huge_within_size -R xunit -g auto'.

I’m not sure if this addresses your question. Please let me know if I
misunderstood.

> 
> --D
> 
> >  }
> >  
> >  _scratch_mount_options()
> > -- 
> > 2.43.0
> > 

Re: [PATCH 4/5] common/rc: fix scratch mount options for tmpfs

[Darrick J. Wong]

On Mon, Jun 24, 2024 at 01:50:33PM +0000, Daniel Gomez wrote:
> On Fri, Jun 14, 2024 at 08:47:28AM GMT, Darrick J. Wong wrote:
> > On Fri, Jun 14, 2024 at 06:17:25AM +0000, Daniel Gomez wrote:
> > > Make sure tmpfs scratch device inherits the extra tmpfs mount options
> > > variable (TMPFS_MOUNT_OPTIONS).
> > > 
> > > Signed-off-by: Daniel Gomez <da.gomez@samsung.com>
> > > ---
> > >  common/rc | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/common/rc b/common/rc
> > > index 7f995b0fa..a42792c37 100644
> > > --- a/common/rc
> > > +++ b/common/rc
> > > @@ -224,7 +224,7 @@ _test_options()
> > >  # hosted on $SCRATCH_DEV, so can't use external scratch devices).
> > >  _common_dev_mount_options()
> > >  {
> > > -	echo $MOUNT_OPTIONS $SELINUX_MOUNT_OPTIONS $*
> > > +	echo $MOUNT_OPTIONS $SELINUX_MOUNT_OPTIONS $TMPFS_MOUNT_OPTIONS $*
> > 
> > Why is it necessary to include tmpfs mount options for all fs types?
> > XFS does not accept tmpfs mount options.
> 
> You are right. We should not do this.
> 
> However, _scratch_mount_options() calls _common_dev_mount_options() ignoring the
> specific mount options based on fstyp.

Ah, _mount_opts only gets run for configuration files.

> specific mount options based on fstyp. Replacing it with _common_mount_opts()
> makes this work. In addition, _common_dev_mount_options() function description
> says 'Used for mounting non-scratch devices with the safe set of scratch mount
> options'. So, why is it used to mount scratch devices?

I think the comment isn't very good.  Six of the seven callers:

common/dmdelay|23| _mount -t $FSTYP `_common_dev_mount_options` $SCRATCH_OPTIONS \
common/dmdust|19| _mount -t $FSTYP `_common_dev_mount_options $*` $SCRATCH_OPTIONS \
common/dmerror|94| _mount -t $FSTYP `_common_dev_mount_options $*` $SCRATCH_OPTIONS \
common/dmlogwrites|107| _mount -t $FSTYP `_common_dev_mount_options $*` $SCRATCH_OPTIONS \
common/dmthin|226| echo `_common_dev_mount_options $*` $SCRATCH_OPTIONS $DMTHIN_VOL_DEV $SCRATCH_MNT
common/rc|272| echo `_common_dev_mount_options $*` $SCRATCH_OPTIONS \

appear to use this to mount the scratch filesystem from devices that are
not the regular scratch device.  The one exception is this one:

common/overlay|32| _mount -t overlay $diropts `_common_dev_mount_options $*`

which AFAICT seem to mount arbitrary overlayfs instances with some of
the mount options.

> This fixes the issue:
> 
> diff --git a/common/rc b/common/rc
> index 51827119c..627dbaaaa 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -231,8 +231,8 @@ _scratch_mount_options()
>  {
>         _scratch_options mount
> 
> -       echo `_common_dev_mount_options $*` $SCRATCH_OPTIONS \
> -                                       $SCRATCH_DEV $SCRATCH_MNT
> +       echo `_common_mount_opts` $SCRATCH_OPTIONS \
> +                                       $SCRATCH_DEV $SCRATCH_MNT $*
>  }
> 
> > 
> > For that matter, why is TMPFS_MOUNT_OPTIONS necessary at all?  Shouldn't
> > that simply be "MOUNT_OPTIONS=<stuff> FSTYP=tmpfs ./check" ?
> 
> TMPFS_MOUNT_OPTIONS is used to specify mount options in each section of the
> configuration file. For example, the following snippet is part of my conf file:
> 
> ```
> [tmpfs_noswap_huge_always]
> TMPFS_MOUNT_OPTIONS="-o noswap,huge=always"
> 
> [tmpfs_noswap_huge_within_size]
> TMPFS_MOUNT_OPTIONS="-o noswap,huge=within_size"
> ```
> 
> Then I run -s option to switch between profiles, e.g., 'check -s
> tmpfs_noswap_huge_within_size -R xunit -g auto'.
> 
> I’m not sure if this addresses your question. Please let me know if I
> misunderstood.

Ahah, this is one of those parts of fstests that differ depending on
whether you use configuration files (you do) or not (I don't).  AFAICT,
get_next_config has this slightly odd (to me) behavior where if a config
section doesn't explicitly set MOUNT_OPTIONS, it will reuse
MOUNT_OPTIONS from a previous section if FSTYP hasn't changed.  You
instead want to change the mount between sections

So I /think/ the correct thing to do here is

[tmpfs_noswap_huge_always]
MOUNT_OPTIONS="-o noswap,huge=always"

[tmpfs_noswap_huge_within_size]
MOUNT_OPTIONS="-o noswap,huge=within_size"

Though I'm not exactly an expert on these things.

--D

> 
> > 
> > --D
> > 
> > >  }
> > >  
> > >  _scratch_mount_options()
> > > -- 
> > > 2.43.0
> > > 

Re: [PATCH 4/5] common/rc: fix scratch mount options for tmpfs

[Daniel Gomez]

On Mon, Jun 24, 2024 at 09:47:15AM GMT, Darrick J. Wong wrote:
> On Mon, Jun 24, 2024 at 01:50:33PM +0000, Daniel Gomez wrote:
> > On Fri, Jun 14, 2024 at 08:47:28AM GMT, Darrick J. Wong wrote:
> > > On Fri, Jun 14, 2024 at 06:17:25AM +0000, Daniel Gomez wrote:
> > > > Make sure tmpfs scratch device inherits the extra tmpfs mount options
> > > > variable (TMPFS_MOUNT_OPTIONS).
> > > > 
> > > > Signed-off-by: Daniel Gomez <da.gomez@samsung.com>
> > > > ---
> > > >  common/rc | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/common/rc b/common/rc
> > > > index 7f995b0fa..a42792c37 100644
> > > > --- a/common/rc
> > > > +++ b/common/rc
> > > > @@ -224,7 +224,7 @@ _test_options()
> > > >  # hosted on $SCRATCH_DEV, so can't use external scratch devices).
> > > >  _common_dev_mount_options()
> > > >  {
> > > > -	echo $MOUNT_OPTIONS $SELINUX_MOUNT_OPTIONS $*
> > > > +	echo $MOUNT_OPTIONS $SELINUX_MOUNT_OPTIONS $TMPFS_MOUNT_OPTIONS $*
> > > 
> > > Why is it necessary to include tmpfs mount options for all fs types?
> > > XFS does not accept tmpfs mount options.
> > 
> > You are right. We should not do this.
> > 
> > However, _scratch_mount_options() calls _common_dev_mount_options() ignoring the
> > specific mount options based on fstyp.
> 
> Ah, _mount_opts only gets run for configuration files.

Even if you are running configuration files, _mount_opts() ->
_common_mount_opts() is not run for scratch mount options.

> 
> > specific mount options based on fstyp. Replacing it with _common_mount_opts()
> > makes this work. In addition, _common_dev_mount_options() function description
> > says 'Used for mounting non-scratch devices with the safe set of scratch mount
> > options'. So, why is it used to mount scratch devices?
> 
> I think the comment isn't very good.  Six of the seven callers:
> 
> common/dmdelay|23| _mount -t $FSTYP `_common_dev_mount_options` $SCRATCH_OPTIONS \
> common/dmdust|19| _mount -t $FSTYP `_common_dev_mount_options $*` $SCRATCH_OPTIONS \
> common/dmerror|94| _mount -t $FSTYP `_common_dev_mount_options $*` $SCRATCH_OPTIONS \
> common/dmlogwrites|107| _mount -t $FSTYP `_common_dev_mount_options $*` $SCRATCH_OPTIONS \
> common/dmthin|226| echo `_common_dev_mount_options $*` $SCRATCH_OPTIONS $DMTHIN_VOL_DEV $SCRATCH_MNT
> common/rc|272| echo `_common_dev_mount_options $*` $SCRATCH_OPTIONS \
> 
> appear to use this to mount the scratch filesystem from devices that are
> not the regular scratch device.  The one exception is this one:
> 
> common/overlay|32| _mount -t overlay $diropts `_common_dev_mount_options $*`
> 
> which AFAICT seem to mount arbitrary overlayfs instances with some of
> the mount options.

Thanks for clarifying. I guess the question is if it's okay to do
the replacement suggested (_common_mount_opts()) considering the
current _scratch_mount_options() callers.

> 
> > This fixes the issue:
> > 
> > diff --git a/common/rc b/common/rc
> > index 51827119c..627dbaaaa 100644
> > --- a/common/rc
> > +++ b/common/rc
> > @@ -231,8 +231,8 @@ _scratch_mount_options()
> >  {
> >         _scratch_options mount
> > 
> > -       echo `_common_dev_mount_options $*` $SCRATCH_OPTIONS \
> > -                                       $SCRATCH_DEV $SCRATCH_MNT
> > +       echo `_common_mount_opts` $SCRATCH_OPTIONS \
> > +                                       $SCRATCH_DEV $SCRATCH_MNT $*
> >  }
> > 
> > > 
> > > For that matter, why is TMPFS_MOUNT_OPTIONS necessary at all?  Shouldn't
> > > that simply be "MOUNT_OPTIONS=<stuff> FSTYP=tmpfs ./check" ?
> > 
> > TMPFS_MOUNT_OPTIONS is used to specify mount options in each section of the
> > configuration file. For example, the following snippet is part of my conf file:
> > 
> > ```
> > [tmpfs_noswap_huge_always]
> > TMPFS_MOUNT_OPTIONS="-o noswap,huge=always"
> > 
> > [tmpfs_noswap_huge_within_size]
> > TMPFS_MOUNT_OPTIONS="-o noswap,huge=within_size"
> > ```
> > 
> > Then I run -s option to switch between profiles, e.g., 'check -s
> > tmpfs_noswap_huge_within_size -R xunit -g auto'.
> > 
> > I’m not sure if this addresses your question. Please let me know if I
> > misunderstood.
> 
> Ahah, this is one of those parts of fstests that differ depending on
> whether you use configuration files (you do) or not (I don't).  AFAICT,
> get_next_config has this slightly odd (to me) behavior where if a config
> section doesn't explicitly set MOUNT_OPTIONS, it will reuse
> MOUNT_OPTIONS from a previous section if FSTYP hasn't changed.  You
> instead want to change the mount between sections
> 
> So I /think/ the correct thing to do here is
> 
> [tmpfs_noswap_huge_always]
> MOUNT_OPTIONS="-o noswap,huge=always"
> 
> [tmpfs_noswap_huge_within_size]
> MOUNT_OPTIONS="-o noswap,huge=within_size"
> 
> Though I'm not exactly an expert on these things.

I forgot to mentioned earlier, TMPFS_MOUNT_OPTIONS is used to extend a default
tmpfs mount common option (in _common_mount_opts()):

	tmpfs)
		# We need to specify the size at mount, use 1G by default
		echo "-o size=1G $TMPFS_MOUNT_OPTIONS"
		;;

I guess using MOUNT_OPTIONS would overwrite the default and required size option
for the tests to run. Other fs add their own defaults as well.

> 
> --D
> 
> > 
> > > 
> > > --D
> > > 
> > > >  }
> > > >  
> > > >  _scratch_mount_options()
> > > > -- 
> > > > 2.43.0
> > > > 

[PATCH 3/5] common/rc: add recreation support for tmpfs

[Daniel Gomez]

Add support for test device recreation (RECREATE_TEST_DEV=true) for
tmpfs. This allows to inherit the tmpfs profile extra mount options
(TMPFS_MOUNT_OPTIONS) for the test device.

Signed-off-by: Daniel Gomez <da.gomez@samsung.com>
---
 common/rc | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/common/rc b/common/rc
index 163041fea..7f995b0fa 100644
--- a/common/rc
+++ b/common/rc
@@ -457,7 +457,7 @@ _test_mount()
     fi
 
     _test_options mount
-    _mount -t $FSTYP$FUSE_SUBTYP $TEST_OPTIONS $TEST_FS_MOUNT_OPTS $SELINUX_MOUNT_OPTIONS $* $TEST_DEV $TEST_DIR
+    _mount -t $FSTYP$FUSE_SUBTYP $TEST_OPTIONS $TEST_FS_MOUNT_OPTS $SELINUX_MOUNT_OPTIONS $TMPFS_MOUNT_OPTIONS $* $TEST_DEV $TEST_DIR
     mount_ret=$?
     [ $mount_ret -ne 0 ] && return $mount_ret
     _idmapped_mount $TEST_DEV $TEST_DIR
@@ -604,6 +604,9 @@ _test_mkfs()
     pvfs2)
 	# do nothing for pvfs2
 	;;
+	tmpfs)
+	# do nothing for tmpfs
+	;;
     udf)
         $MKFS_UDF_PROG $MKFS_OPTIONS $* $TEST_DEV > /dev/null
 	;;
-- 
2.43.0

Re: [PATCH 3/5] common/rc: add recreation support for tmpfs

[Darrick J. Wong]

On Fri, Jun 14, 2024 at 06:17:25AM +0000, Daniel Gomez wrote:
> Add support for test device recreation (RECREATE_TEST_DEV=true) for
> tmpfs. This allows to inherit the tmpfs profile extra mount options
> (TMPFS_MOUNT_OPTIONS) for the test device.
> 
> Signed-off-by: Daniel Gomez <da.gomez@samsung.com>
> ---
>  common/rc | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/common/rc b/common/rc
> index 163041fea..7f995b0fa 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -457,7 +457,7 @@ _test_mount()
>      fi
>  
>      _test_options mount
> -    _mount -t $FSTYP$FUSE_SUBTYP $TEST_OPTIONS $TEST_FS_MOUNT_OPTS $SELINUX_MOUNT_OPTIONS $* $TEST_DEV $TEST_DIR
> +    _mount -t $FSTYP$FUSE_SUBTYP $TEST_OPTIONS $TEST_FS_MOUNT_OPTS $SELINUX_MOUNT_OPTIONS $TMPFS_MOUNT_OPTIONS $* $TEST_DEV $TEST_DIR

Same question as the last patch -- why is it ok to inject
TMPFS_MOUNT_OPTIONS here when FSTYP is not tmpfs?

--D

>      mount_ret=$?
>      [ $mount_ret -ne 0 ] && return $mount_ret
>      _idmapped_mount $TEST_DEV $TEST_DIR
> @@ -604,6 +604,9 @@ _test_mkfs()
>      pvfs2)
>  	# do nothing for pvfs2
>  	;;
> +	tmpfs)
> +	# do nothing for tmpfs
> +	;;
>      udf)
>          $MKFS_UDF_PROG $MKFS_OPTIONS $* $TEST_DEV > /dev/null
>  	;;
> -- 
> 2.43.0
> 

Re: [PATCH 3/5] common/rc: add recreation support for tmpfs

[Zorro Lang]

On Fri, Jun 14, 2024 at 06:17:25AM +0000, Daniel Gomez wrote:
> Add support for test device recreation (RECREATE_TEST_DEV=true) for
> tmpfs. This allows to inherit the tmpfs profile extra mount options
> (TMPFS_MOUNT_OPTIONS) for the test device.
> 
> Signed-off-by: Daniel Gomez <da.gomez@samsung.com>
> ---
>  common/rc | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/common/rc b/common/rc
> index 163041fea..7f995b0fa 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -457,7 +457,7 @@ _test_mount()
>      fi
>  
>      _test_options mount
> -    _mount -t $FSTYP$FUSE_SUBTYP $TEST_OPTIONS $TEST_FS_MOUNT_OPTS $SELINUX_MOUNT_OPTIONS $* $TEST_DEV $TEST_DIR
> +    _mount -t $FSTYP$FUSE_SUBTYP $TEST_OPTIONS $TEST_FS_MOUNT_OPTS $SELINUX_MOUNT_OPTIONS $TMPFS_MOUNT_OPTIONS $* $TEST_DEV $TEST_DIR

We shouldn't use a FSTYP specific parameter in a common mount line.
Better to set it in a "$FSTYP=tmpfs" branch. I notice that we set:

        tmpfs)
                # We need to specify the size at mount, use 1G by default
                echo "-o size=1G $TMPFS_MOUNT_OPTIONS"

in _common_mount_opts(). Then it will be set in MOUNT_OPTIONS and
TEST_FS_MOUNT_OPTS, won't that help?

Thanks,
Zorro


>      mount_ret=$?
>      [ $mount_ret -ne 0 ] && return $mount_ret
>      _idmapped_mount $TEST_DEV $TEST_DIR
> @@ -604,6 +604,9 @@ _test_mkfs()
>      pvfs2)
>  	# do nothing for pvfs2
>  	;;
> +	tmpfs)
> +	# do nothing for tmpfs
> +	;;
>      udf)
>          $MKFS_UDF_PROG $MKFS_OPTIONS $* $TEST_DEV > /dev/null
>  	;;
> -- 
> 2.43.0
> 

Re: [PATCH 3/5] common/rc: add recreation support for tmpfs

[Daniel Gomez]

On Mon, Jun 17, 2024 at 03:06:54PM GMT, Zorro Lang wrote:
> On Fri, Jun 14, 2024 at 06:17:25AM +0000, Daniel Gomez wrote:
> > Add support for test device recreation (RECREATE_TEST_DEV=true) for
> > tmpfs. This allows to inherit the tmpfs profile extra mount options
> > (TMPFS_MOUNT_OPTIONS) for the test device.
> > 
> > Signed-off-by: Daniel Gomez <da.gomez@samsung.com>
> > ---
> >  common/rc | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/common/rc b/common/rc
> > index 163041fea..7f995b0fa 100644
> > --- a/common/rc
> > +++ b/common/rc
> > @@ -457,7 +457,7 @@ _test_mount()
> >      fi
> >  
> >      _test_options mount
> > -    _mount -t $FSTYP$FUSE_SUBTYP $TEST_OPTIONS $TEST_FS_MOUNT_OPTS $SELINUX_MOUNT_OPTIONS $* $TEST_DEV $TEST_DIR
> > +    _mount -t $FSTYP$FUSE_SUBTYP $TEST_OPTIONS $TEST_FS_MOUNT_OPTS $SELINUX_MOUNT_OPTIONS $TMPFS_MOUNT_OPTIONS $* $TEST_DEV $TEST_DIR
> 
> We shouldn't use a FSTYP specific parameter in a common mount line.
> Better to set it in a "$FSTYP=tmpfs" branch. I notice that we set:
> 
>         tmpfs)
>                 # We need to specify the size at mount, use 1G by default
>                 echo "-o size=1G $TMPFS_MOUNT_OPTIONS"
> 
> in _common_mount_opts(). Then it will be set in MOUNT_OPTIONS and
> TEST_FS_MOUNT_OPTS, won't that help?

No for the recreation path with specific section variables as we have not yet
source '. common/rc'. Dropping changes in _test_mount() and force reading test
mount options makes $TMPFS_MOUNT_OPTION available for the test in the recreation
path.

diff --git a/check b/check
index 723a52e30..b3f7d7b14 100755
--- a/check
+++ b/check
@@ -773,6 +773,7 @@ function run_section()
                        status=1
                        exit
                fi
+               _test_mount_opts
                if ! _test_mount
                then
                        echo "check: failed to mount $TEST_DEV on $TEST_DIR"

I understand from Darrick's comment that the change above in _test_mount() is
wrong. Would this solution be okay? Not sure if this is a bug but I'd guess all
specific mount options are now ignored in the recreation path (not only tmpfs).

> 
> Thanks,
> Zorro
> 
> 
> >      mount_ret=$?
> >      [ $mount_ret -ne 0 ] && return $mount_ret
> >      _idmapped_mount $TEST_DEV $TEST_DIR
> > @@ -604,6 +604,9 @@ _test_mkfs()
> >      pvfs2)
> >  	# do nothing for pvfs2
> >  	;;
> > +	tmpfs)
> > +	# do nothing for tmpfs
> > +	;;
> >      udf)
> >          $MKFS_UDF_PROG $MKFS_OPTIONS $* $TEST_DEV > /dev/null
> >  	;;
> > -- 
> > 2.43.0
> > 
> 

Re: [PATCH 3/5] common/rc: add recreation support for tmpfs

[Zorro Lang]

On Mon, Jun 24, 2024 at 01:33:55PM +0000, Daniel Gomez wrote:
> On Mon, Jun 17, 2024 at 03:06:54PM GMT, Zorro Lang wrote:
> > On Fri, Jun 14, 2024 at 06:17:25AM +0000, Daniel Gomez wrote:
> > > Add support for test device recreation (RECREATE_TEST_DEV=true) for
> > > tmpfs. This allows to inherit the tmpfs profile extra mount options
> > > (TMPFS_MOUNT_OPTIONS) for the test device.
> > > 
> > > Signed-off-by: Daniel Gomez <da.gomez@samsung.com>
> > > ---
> > >  common/rc | 5 ++++-
> > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/common/rc b/common/rc
> > > index 163041fea..7f995b0fa 100644
> > > --- a/common/rc
> > > +++ b/common/rc
> > > @@ -457,7 +457,7 @@ _test_mount()
> > >      fi
> > >  
> > >      _test_options mount
> > > -    _mount -t $FSTYP$FUSE_SUBTYP $TEST_OPTIONS $TEST_FS_MOUNT_OPTS $SELINUX_MOUNT_OPTIONS $* $TEST_DEV $TEST_DIR
> > > +    _mount -t $FSTYP$FUSE_SUBTYP $TEST_OPTIONS $TEST_FS_MOUNT_OPTS $SELINUX_MOUNT_OPTIONS $TMPFS_MOUNT_OPTIONS $* $TEST_DEV $TEST_DIR
> > 
> > We shouldn't use a FSTYP specific parameter in a common mount line.
> > Better to set it in a "$FSTYP=tmpfs" branch. I notice that we set:
> > 
> >         tmpfs)
> >                 # We need to specify the size at mount, use 1G by default
> >                 echo "-o size=1G $TMPFS_MOUNT_OPTIONS"
> > 
> > in _common_mount_opts(). Then it will be set in MOUNT_OPTIONS and
> > TEST_FS_MOUNT_OPTS, won't that help?
> 
> No for the recreation path with specific section variables as we have not yet
> source '. common/rc'. Dropping changes in _test_mount() and force reading test
> mount options makes $TMPFS_MOUNT_OPTION available for the test in the recreation
> path.
> 
> diff --git a/check b/check
> index 723a52e30..b3f7d7b14 100755
> --- a/check
> +++ b/check
> @@ -773,6 +773,7 @@ function run_section()
>                         status=1
>                         exit
>                 fi
> +               _test_mount_opts

Actually the get_next_config function (from common/config) should have reloaded
the mount and mkfs paramters.

run_section()
{
        ...
        get_next_config $section
        ...

        if $RECREATE_TEST_DEV || [ "$OLD_FSTYP" != "$FSTYP" ]; then
                ...
                if ! _test_mkfs >$tmp.err 2>&1
                ...
                if ! _test_mount
                ...
}

But why it doesn't?

Oh, due to the get_next_config does things as this:

get_next_config() {
        ...
        unset MOUNT_OPTIONS
        unset TEST_FS_MOUNT_OPTS
        unset MKFS_OPTIONS
        unset FSCK_OPTIONS
        ...
        if [ ! -z "$OLD_FSTYP" ] && [ $OLD_FSTYP != $FSTYP ]; then
                [ -z "$MOUNT_OPTIONS" ] && _mount_opts
                [ -z "$TEST_FS_MOUNT_OPTS" ] && _test_mount_opts
                [ -z "$MKFS_OPTIONS" ] && _mkfs_opts
                [ -z "$FSCK_OPTIONS" ] && _fsck_opts
                ...
        else
                [ -z "$MOUNT_OPTIONS" ] && export MOUNT_OPTIONS=$OLD_MOUNT_OPTIONS
                [ -z "$TEST_FS_MOUNT_OPTS" ] && export TEST_FS_MOUNT_OPTS=$OLD_TEST_FS_MOUNT_OPTS
                [ -z "$MKFS_OPTIONS" ] && export MKFS_OPTIONS=$OLD_MKFS_OPTIONS
                [ -z "$FSCK_OPTIONS" ] && export FSCK_OPTIONS=$OLD_FSCK_OPTIONS
                [ -z "$USE_EXTERNAL" ] && export USE_EXTERNAL=$OLD_USE_EXTERNAL
        fi
        ...
}

Only if the later FSTYP is different with the former one, then get_next_config
reloads those OPTIONS.

1) That helps you don't need to set duplicated paramters in each section.
2) But that causes you can't reset/reload something likes TEST_FS_MOUNT_OPTS,
except you set MOUNT_OPTIONS, TEST_FS_MOUNT_OPTS, ... directly.

I'm not sure how many people depends on the 1)st behavior long time. And how
many people hope to remove the limitation of [ $OLD_FSTYP != $FSTYP ]. Any change
will be default behavior change.

But we might can bring the $RECREATE_TEST_DEV into get_next_config(), likes:
  if $RECREATE_TEST_DEV || ([ -n "$OLD_FSTYP" ] && [ "$OLD_FSTYP" != "$FSTYP" ])

I didn't give it a test, any better ideas are welcome :)

Thanks,
Zorro

>                 if ! _test_mount
>                 then
>                         echo "check: failed to mount $TEST_DEV on $TEST_DIR"
> 
> I understand from Darrick's comment that the change above in _test_mount() is
> wrong. Would this solution be okay? Not sure if this is a bug but I'd guess all
> specific mount options are now ignored in the recreation path (not only tmpfs).



> 
> > 
> > Thanks,
> > Zorro
> > 
> > 
> > >      mount_ret=$?
> > >      [ $mount_ret -ne 0 ] && return $mount_ret
> > >      _idmapped_mount $TEST_DEV $TEST_DIR
> > > @@ -604,6 +604,9 @@ _test_mkfs()
> > >      pvfs2)
> > >  	# do nothing for pvfs2
> > >  	;;
> > > +	tmpfs)
> > > +	# do nothing for tmpfs
> > > +	;;
> > >      udf)
> > >          $MKFS_UDF_PROG $MKFS_OPTIONS $* $TEST_DEV > /dev/null
> > >  	;;
> > > -- 
> > > 2.43.0
> > > 
> > 
> 

Re: [PATCH 3/5] common/rc: add recreation support for tmpfs

[Daniel Gomez]

On Fri, Jun 28, 2024 at 11:13:28AM GMT, Zorro Lang wrote:
> On Mon, Jun 24, 2024 at 01:33:55PM +0000, Daniel Gomez wrote:
> > On Mon, Jun 17, 2024 at 03:06:54PM GMT, Zorro Lang wrote:
> > > On Fri, Jun 14, 2024 at 06:17:25AM +0000, Daniel Gomez wrote:
> > > > Add support for test device recreation (RECREATE_TEST_DEV=true) for
> > > > tmpfs. This allows to inherit the tmpfs profile extra mount options
> > > > (TMPFS_MOUNT_OPTIONS) for the test device.
> > > > 
> > > > Signed-off-by: Daniel Gomez <da.gomez@samsung.com>
> > > > ---
> > > >  common/rc | 5 ++++-
> > > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/common/rc b/common/rc
> > > > index 163041fea..7f995b0fa 100644
> > > > --- a/common/rc
> > > > +++ b/common/rc
> > > > @@ -457,7 +457,7 @@ _test_mount()
> > > >      fi
> > > >  
> > > >      _test_options mount
> > > > -    _mount -t $FSTYP$FUSE_SUBTYP $TEST_OPTIONS $TEST_FS_MOUNT_OPTS $SELINUX_MOUNT_OPTIONS $* $TEST_DEV $TEST_DIR
> > > > +    _mount -t $FSTYP$FUSE_SUBTYP $TEST_OPTIONS $TEST_FS_MOUNT_OPTS $SELINUX_MOUNT_OPTIONS $TMPFS_MOUNT_OPTIONS $* $TEST_DEV $TEST_DIR
> > > 
> > > We shouldn't use a FSTYP specific parameter in a common mount line.
> > > Better to set it in a "$FSTYP=tmpfs" branch. I notice that we set:
> > > 
> > >         tmpfs)
> > >                 # We need to specify the size at mount, use 1G by default
> > >                 echo "-o size=1G $TMPFS_MOUNT_OPTIONS"
> > > 
> > > in _common_mount_opts(). Then it will be set in MOUNT_OPTIONS and
> > > TEST_FS_MOUNT_OPTS, won't that help?
> > 
> > No for the recreation path with specific section variables as we have not yet
> > source '. common/rc'. Dropping changes in _test_mount() and force reading test
> > mount options makes $TMPFS_MOUNT_OPTION available for the test in the recreation
> > path.
> > 
> > diff --git a/check b/check
> > index 723a52e30..b3f7d7b14 100755
> > --- a/check
> > +++ b/check
> > @@ -773,6 +773,7 @@ function run_section()
> >                         status=1
> >                         exit
> >                 fi
> > +               _test_mount_opts
> 
> Actually the get_next_config function (from common/config) should have reloaded
> the mount and mkfs paramters.
> 
> run_section()
> {
>         ...
>         get_next_config $section
>         ...
> 
>         if $RECREATE_TEST_DEV || [ "$OLD_FSTYP" != "$FSTYP" ]; then
>                 ...
>                 if ! _test_mkfs >$tmp.err 2>&1
>                 ...
>                 if ! _test_mount
>                 ...
> }
> 
> But why it doesn't?
> 
> Oh, due to the get_next_config does things as this:
> 
> get_next_config() {
>         ...
>         unset MOUNT_OPTIONS
>         unset TEST_FS_MOUNT_OPTS
>         unset MKFS_OPTIONS
>         unset FSCK_OPTIONS
>         ...
>         if [ ! -z "$OLD_FSTYP" ] && [ $OLD_FSTYP != $FSTYP ]; then
>                 [ -z "$MOUNT_OPTIONS" ] && _mount_opts
>                 [ -z "$TEST_FS_MOUNT_OPTS" ] && _test_mount_opts
>                 [ -z "$MKFS_OPTIONS" ] && _mkfs_opts
>                 [ -z "$FSCK_OPTIONS" ] && _fsck_opts
>                 ...
>         else
>                 [ -z "$MOUNT_OPTIONS" ] && export MOUNT_OPTIONS=$OLD_MOUNT_OPTIONS
>                 [ -z "$TEST_FS_MOUNT_OPTS" ] && export TEST_FS_MOUNT_OPTS=$OLD_TEST_FS_MOUNT_OPTS
>                 [ -z "$MKFS_OPTIONS" ] && export MKFS_OPTIONS=$OLD_MKFS_OPTIONS
>                 [ -z "$FSCK_OPTIONS" ] && export FSCK_OPTIONS=$OLD_FSCK_OPTIONS
>                 [ -z "$USE_EXTERNAL" ] && export USE_EXTERNAL=$OLD_USE_EXTERNAL
>         fi
>         ...
> }
> 
> Only if the later FSTYP is different with the former one, then get_next_config
> reloads those OPTIONS.
> 
> 1) That helps you don't need to set duplicated paramters in each section.
> 2) But that causes you can't reset/reload something likes TEST_FS_MOUNT_OPTS,
> except you set MOUNT_OPTIONS, TEST_FS_MOUNT_OPTS, ... directly.
> 
> I'm not sure how many people depends on the 1)st behavior long time. And how
> many people hope to remove the limitation of [ $OLD_FSTYP != $FSTYP ]. Any change
> will be default behavior change.
> 
> But we might can bring the $RECREATE_TEST_DEV into get_next_config(), likes:
>   if $RECREATE_TEST_DEV || ([ -n "$OLD_FSTYP" ] && [ "$OLD_FSTYP" != "$FSTYP" ])
> 
> I didn't give it a test, any better ideas are welcome :)

This looks good to me to allow parsing specific fs mount options in sections.
I've also tested and works for my test case. Below my changes after dropping
the above:

diff --git a/common/config b/common/config
index 43b32b93d..c739c4578 100644
--- a/common/config
+++ b/common/config
@@ -802,7 +802,7 @@ get_next_config() {
        fi

        parse_config_section $1
-       if [ ! -z "$OLD_FSTYP" ] && [ $OLD_FSTYP != $FSTYP ]; then
+       if $RECREATE_TEST_DEV || ([ ! -z "$OLD_FSTYP" ] && [ $OLD_FSTYP != $FSTYP ]); then
                [ -z "$MOUNT_OPTIONS" ] && _mount_opts
                [ -z "$TEST_FS_MOUNT_OPTS" ] && _test_mount_opts
                [ -z "$MKFS_OPTIONS" ] && _mkfs_opts

I think this is also the original intention of the recreation TEST_DEV
(commit [1]).

[1] f8e4f532f18d7517430d9849bfc042305d7f7f4d (check: Allow to recreate
TEST_DEV)

> Thanks,
> Zorro
> 
> >                 if ! _test_mount
> >                 then
> >                         echo "check: failed to mount $TEST_DEV on $TEST_DIR"
> > 
> > I understand from Darrick's comment that the change above in _test_mount() is
> > wrong. Would this solution be okay? Not sure if this is a bug but I'd guess all
> > specific mount options are now ignored in the recreation path (not only tmpfs).
> 
> 
> 
> > 
> > > 
> > > Thanks,
> > > Zorro
> > > 
> > > 
> > > >      mount_ret=$?
> > > >      [ $mount_ret -ne 0 ] && return $mount_ret
> > > >      _idmapped_mount $TEST_DEV $TEST_DIR
> > > > @@ -604,6 +604,9 @@ _test_mkfs()
> > > >      pvfs2)
> > > >  	# do nothing for pvfs2
> > > >  	;;
> > > > +	tmpfs)
> > > > +	# do nothing for tmpfs
> > > > +	;;
> > > >      udf)
> > > >          $MKFS_UDF_PROG $MKFS_OPTIONS $* $TEST_DEV > /dev/null
> > > >  	;;
> > > > -- 
> > > > 2.43.0
> > > > 
> > > 
> > 
> 

[PATCH 5/5] common/rc: print scratch and test mount options

[Daniel Gomez]

Mount options for a SCRATCH device might not be the same in the TEST
device if RECREATE_TEST_DEV is not enabled. So, print mount options for
each device to clarify this.

Add mount and mkfs info for TEST devices so we get the same information
being printed for both devices.

Signed-off-by: Daniel Gomez <da.gomez@samsung.com>
---
 check     |  4 ++++
 common/rc | 20 ++++++++++++++++++--
 2 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/check b/check
index 723a52e30..e02d28b39 100755
--- a/check
+++ b/check
@@ -807,7 +807,11 @@ function run_section()
 	# print out our test configuration
 	echo "FSTYP         -- `_full_fstyp_details`"
 	echo "PLATFORM      -- `_full_platform_details`"
+	echo "TEST device:"
+	echo "MKFS_OPTIONS  -- `_test_mkfs_options`"
+	echo "MOUNT_OPTIONS -- `_test_mount_options`"
 	if [ ! -z "$SCRATCH_DEV" ]; then
+	  echo "SCRATCH device:"
 	  echo "MKFS_OPTIONS  -- `_scratch_mkfs_options`"
 	  echo "MOUNT_OPTIONS -- `_scratch_mount_options`"
 	fi
diff --git a/common/rc b/common/rc
index a42792c37..b351a82eb 100644
--- a/common/rc
+++ b/common/rc
@@ -235,6 +235,17 @@ _scratch_mount_options()
 					$SCRATCH_DEV $SCRATCH_MNT
 }
 
+_test_mount_options()
+{
+	_test_options mount
+
+	if $RECREATE_TEST_DEV || [ "$OLD_FSTYP" != "$FSTYP" ]; then
+		echo $TEST_OPTIONS $TEST_FS_MOUNT_OPTS $SELINUX_MOUNT_OPTIONS $TMPFS_MOUNT_OPTIONS $* $TEST_DEV $TEST_DIR
+	else
+		echo $TEST_OPTIONS $TEST_FS_MOUNT_OPTS $SELINUX_MOUNT_OPTIONS $* $TEST_DEV $TEST_DIR
+	fi
+}
+
 _supports_filetype()
 {
 	local dir=$1
@@ -456,8 +467,7 @@ _test_mount()
         return $?
     fi
 
-    _test_options mount
-    _mount -t $FSTYP$FUSE_SUBTYP $TEST_OPTIONS $TEST_FS_MOUNT_OPTS $SELINUX_MOUNT_OPTIONS $TMPFS_MOUNT_OPTIONS $* $TEST_DEV $TEST_DIR
+    _mount -t $FSTYP$FUSE_SUBTYP `_test_mount_options $*`
     mount_ret=$?
     [ $mount_ret -ne 0 ] && return $mount_ret
     _idmapped_mount $TEST_DEV $TEST_DIR
@@ -571,6 +581,12 @@ _metadump_dev() {
 	esac
 }
 
+_test_mkfs_options()
+{
+	_test_options mkfs
+	echo $TEST_OPTIONS $MKFS_OPTIONS $* $TEST_DEV
+}
+
 _test_mkfs()
 {
     case $FSTYP in
-- 
2.43.0

Re: [PATCH 5/5] common/rc: print scratch and test mount options

[Darrick J. Wong]

On Fri, Jun 14, 2024 at 06:17:26AM +0000, Daniel Gomez wrote:
> Mount options for a SCRATCH device might not be the same in the TEST
> device if RECREATE_TEST_DEV is not enabled. So, print mount options for
> each device to clarify this.
> 
> Add mount and mkfs info for TEST devices so we get the same information
> being printed for both devices.
> 
> Signed-off-by: Daniel Gomez <da.gomez@samsung.com>
> ---
>  check     |  4 ++++
>  common/rc | 20 ++++++++++++++++++--
>  2 files changed, 22 insertions(+), 2 deletions(-)
> 
> diff --git a/check b/check
> index 723a52e30..e02d28b39 100755
> --- a/check
> +++ b/check
> @@ -807,7 +807,11 @@ function run_section()
>  	# print out our test configuration
>  	echo "FSTYP         -- `_full_fstyp_details`"
>  	echo "PLATFORM      -- `_full_platform_details`"
> +	echo "TEST device:"
> +	echo "MKFS_OPTIONS  -- `_test_mkfs_options`"
> +	echo "MOUNT_OPTIONS -- `_test_mount_options`"
>  	if [ ! -z "$SCRATCH_DEV" ]; then
> +	  echo "SCRATCH device:"
>  	  echo "MKFS_OPTIONS  -- `_scratch_mkfs_options`"
>  	  echo "MOUNT_OPTIONS -- `_scratch_mount_options`"

Now there are two lines that start with "MKFS_OPTIONS"; will this break
anyone's parsing scripts?  Or should these be prefixed:

echo "TEST_MKFS_OPTIONS -- `_test_mkfs_options`"
...
echo "SCRATCH_MKFS_OPTIONS -- `_scratch_mkfs_options`"

?

Also should these four variables be captured explicitly by the reports
that are generated by common/report ?

--D

>  	fi
> diff --git a/common/rc b/common/rc
> index a42792c37..b351a82eb 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -235,6 +235,17 @@ _scratch_mount_options()
>  					$SCRATCH_DEV $SCRATCH_MNT
>  }
>  
> +_test_mount_options()
> +{
> +	_test_options mount
> +
> +	if $RECREATE_TEST_DEV || [ "$OLD_FSTYP" != "$FSTYP" ]; then
> +		echo $TEST_OPTIONS $TEST_FS_MOUNT_OPTS $SELINUX_MOUNT_OPTIONS $TMPFS_MOUNT_OPTIONS $* $TEST_DEV $TEST_DIR
> +	else
> +		echo $TEST_OPTIONS $TEST_FS_MOUNT_OPTS $SELINUX_MOUNT_OPTIONS $* $TEST_DEV $TEST_DIR
> +	fi
> +}
> +
>  _supports_filetype()
>  {
>  	local dir=$1
> @@ -456,8 +467,7 @@ _test_mount()
>          return $?
>      fi
>  
> -    _test_options mount
> -    _mount -t $FSTYP$FUSE_SUBTYP $TEST_OPTIONS $TEST_FS_MOUNT_OPTS $SELINUX_MOUNT_OPTIONS $TMPFS_MOUNT_OPTIONS $* $TEST_DEV $TEST_DIR
> +    _mount -t $FSTYP$FUSE_SUBTYP `_test_mount_options $*`
>      mount_ret=$?
>      [ $mount_ret -ne 0 ] && return $mount_ret
>      _idmapped_mount $TEST_DEV $TEST_DIR
> @@ -571,6 +581,12 @@ _metadump_dev() {
>  	esac
>  }
>  
> +_test_mkfs_options()
> +{
> +	_test_options mkfs
> +	echo $TEST_OPTIONS $MKFS_OPTIONS $* $TEST_DEV
> +}
> +
>  _test_mkfs()
>  {
>      case $FSTYP in
> -- 
> 2.43.0
> 

Re: [PATCH 5/5] common/rc: print scratch and test mount options

[Daniel Gomez]

On Fri, Jun 14, 2024 at 08:55:55AM GMT, Darrick J. Wong wrote:
> On Fri, Jun 14, 2024 at 06:17:26AM +0000, Daniel Gomez wrote:
> > Mount options for a SCRATCH device might not be the same in the TEST
> > device if RECREATE_TEST_DEV is not enabled. So, print mount options for
> > each device to clarify this.
> > 
> > Add mount and mkfs info for TEST devices so we get the same information
> > being printed for both devices.
> > 
> > Signed-off-by: Daniel Gomez <da.gomez@samsung.com>
> > ---
> >  check     |  4 ++++
> >  common/rc | 20 ++++++++++++++++++--
> >  2 files changed, 22 insertions(+), 2 deletions(-)
> > 
> > diff --git a/check b/check
> > index 723a52e30..e02d28b39 100755
> > --- a/check
> > +++ b/check
> > @@ -807,7 +807,11 @@ function run_section()
> >  	# print out our test configuration
> >  	echo "FSTYP         -- `_full_fstyp_details`"
> >  	echo "PLATFORM      -- `_full_platform_details`"
> > +	echo "TEST device:"
> > +	echo "MKFS_OPTIONS  -- `_test_mkfs_options`"
> > +	echo "MOUNT_OPTIONS -- `_test_mount_options`"
> >  	if [ ! -z "$SCRATCH_DEV" ]; then
> > +	  echo "SCRATCH device:"
> >  	  echo "MKFS_OPTIONS  -- `_scratch_mkfs_options`"
> >  	  echo "MOUNT_OPTIONS -- `_scratch_mount_options`"
> 
> Now there are two lines that start with "MKFS_OPTIONS"; will this break
> anyone's parsing scripts?  Or should these be prefixed:
> 
> echo "TEST_MKFS_OPTIONS -- `_test_mkfs_options`"
> ...
> echo "SCRATCH_MKFS_OPTIONS -- `_scratch_mkfs_options`"
> 
> ?

This looks like my initial version, but I prefer the 'sub-menu style' because
we did not have these variables before. However, I think it makes sense to
introduce them so we can differentiate between them. 

I guess introducing this change would break anyone's parsing scripts regardless?
However, I do think is necessary to specify the mount differences.

> 
> Also should these four variables be captured explicitly by the reports
> that are generated by common/report ?

I guess the report only includes the scratch mount options. I will update it to
include the new specific test and scratch mount/mkfs options.

> 
> --D
> 
> >  	fi
> > diff --git a/common/rc b/common/rc
> > index a42792c37..b351a82eb 100644
> > --- a/common/rc
> > +++ b/common/rc
> > @@ -235,6 +235,17 @@ _scratch_mount_options()
> >  					$SCRATCH_DEV $SCRATCH_MNT
> >  }
> >  
> > +_test_mount_options()
> > +{
> > +	_test_options mount
> > +
> > +	if $RECREATE_TEST_DEV || [ "$OLD_FSTYP" != "$FSTYP" ]; then
> > +		echo $TEST_OPTIONS $TEST_FS_MOUNT_OPTS $SELINUX_MOUNT_OPTIONS $TMPFS_MOUNT_OPTIONS $* $TEST_DEV $TEST_DIR
> > +	else
> > +		echo $TEST_OPTIONS $TEST_FS_MOUNT_OPTS $SELINUX_MOUNT_OPTIONS $* $TEST_DEV $TEST_DIR
> > +	fi
> > +}
> > +
> >  _supports_filetype()
> >  {
> >  	local dir=$1
> > @@ -456,8 +467,7 @@ _test_mount()
> >          return $?
> >      fi
> >  
> > -    _test_options mount
> > -    _mount -t $FSTYP$FUSE_SUBTYP $TEST_OPTIONS $TEST_FS_MOUNT_OPTS $SELINUX_MOUNT_OPTIONS $TMPFS_MOUNT_OPTIONS $* $TEST_DEV $TEST_DIR
> > +    _mount -t $FSTYP$FUSE_SUBTYP `_test_mount_options $*`
> >      mount_ret=$?
> >      [ $mount_ret -ne 0 ] && return $mount_ret
> >      _idmapped_mount $TEST_DEV $TEST_DIR
> > @@ -571,6 +581,12 @@ _metadump_dev() {
> >  	esac
> >  }
> >  
> > +_test_mkfs_options()
> > +{
> > +	_test_options mkfs
> > +	echo $TEST_OPTIONS $MKFS_OPTIONS $* $TEST_DEV
> > +}
> > +
> >  _test_mkfs()
> >  {
> >      case $FSTYP in
> > -- 
> > 2.43.0
> > 

Re: [PATCH 5/5] common/rc: print scratch and test mount options

[Darrick J. Wong]

On Mon, Jun 24, 2024 at 02:02:33PM +0000, Daniel Gomez wrote:
> On Fri, Jun 14, 2024 at 08:55:55AM GMT, Darrick J. Wong wrote:
> > On Fri, Jun 14, 2024 at 06:17:26AM +0000, Daniel Gomez wrote:
> > > Mount options for a SCRATCH device might not be the same in the TEST
> > > device if RECREATE_TEST_DEV is not enabled. So, print mount options for
> > > each device to clarify this.
> > > 
> > > Add mount and mkfs info for TEST devices so we get the same information
> > > being printed for both devices.
> > > 
> > > Signed-off-by: Daniel Gomez <da.gomez@samsung.com>
> > > ---
> > >  check     |  4 ++++
> > >  common/rc | 20 ++++++++++++++++++--
> > >  2 files changed, 22 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/check b/check
> > > index 723a52e30..e02d28b39 100755
> > > --- a/check
> > > +++ b/check
> > > @@ -807,7 +807,11 @@ function run_section()
> > >  	# print out our test configuration
> > >  	echo "FSTYP         -- `_full_fstyp_details`"
> > >  	echo "PLATFORM      -- `_full_platform_details`"
> > > +	echo "TEST device:"
> > > +	echo "MKFS_OPTIONS  -- `_test_mkfs_options`"
> > > +	echo "MOUNT_OPTIONS -- `_test_mount_options`"
> > >  	if [ ! -z "$SCRATCH_DEV" ]; then
> > > +	  echo "SCRATCH device:"
> > >  	  echo "MKFS_OPTIONS  -- `_scratch_mkfs_options`"
> > >  	  echo "MOUNT_OPTIONS -- `_scratch_mount_options`"
> > 
> > Now there are two lines that start with "MKFS_OPTIONS"; will this break
> > anyone's parsing scripts?  Or should these be prefixed:
> > 
> > echo "TEST_MKFS_OPTIONS -- `_test_mkfs_options`"
> > ...
> > echo "SCRATCH_MKFS_OPTIONS -- `_scratch_mkfs_options`"
> > 
> > ?
> 
> This looks like my initial version, but I prefer the 'sub-menu style' because
> we did not have these variables before. However, I think it makes sense to
> introduce them so we can differentiate between them. 

The trouble is, if your parsing script were doing something like
splitting on "-- " then the "TEST device:" string would not split
properly and you'd have to add context retention of some sort to know
which "MKFS_OPTIONS" this was for.  Better just to namespace the new
lines from the start.  I guess for compatibility's sake then we'd have
to have:

TEST_MKFS_OPTIONS -- -o foo=bar
MKFS_OPTIONS      -- -o foo=baz

in the output, along with a comment somewhere in the source that the
non-prefixed scratch mkfs options is that way for hyst[eo]rical output
parsing reasons.

--D

> I guess introducing this change would break anyone's parsing scripts regardless?
> However, I do think is necessary to specify the mount differences.
> > 
> > Also should these four variables be captured explicitly by the reports
> > that are generated by common/report ?
> 
> I guess the report only includes the scratch mount options. I will update it to
> include the new specific test and scratch mount/mkfs options.
> 
> > 
> > --D
> > 
> > >  	fi
> > > diff --git a/common/rc b/common/rc
> > > index a42792c37..b351a82eb 100644
> > > --- a/common/rc
> > > +++ b/common/rc
> > > @@ -235,6 +235,17 @@ _scratch_mount_options()
> > >  					$SCRATCH_DEV $SCRATCH_MNT
> > >  }
> > >  
> > > +_test_mount_options()
> > > +{
> > > +	_test_options mount
> > > +
> > > +	if $RECREATE_TEST_DEV || [ "$OLD_FSTYP" != "$FSTYP" ]; then
> > > +		echo $TEST_OPTIONS $TEST_FS_MOUNT_OPTS $SELINUX_MOUNT_OPTIONS $TMPFS_MOUNT_OPTIONS $* $TEST_DEV $TEST_DIR
> > > +	else
> > > +		echo $TEST_OPTIONS $TEST_FS_MOUNT_OPTS $SELINUX_MOUNT_OPTIONS $* $TEST_DEV $TEST_DIR
> > > +	fi
> > > +}
> > > +
> > >  _supports_filetype()
> > >  {
> > >  	local dir=$1
> > > @@ -456,8 +467,7 @@ _test_mount()
> > >          return $?
> > >      fi
> > >  
> > > -    _test_options mount
> > > -    _mount -t $FSTYP$FUSE_SUBTYP $TEST_OPTIONS $TEST_FS_MOUNT_OPTS $SELINUX_MOUNT_OPTIONS $TMPFS_MOUNT_OPTIONS $* $TEST_DEV $TEST_DIR
> > > +    _mount -t $FSTYP$FUSE_SUBTYP `_test_mount_options $*`
> > >      mount_ret=$?
> > >      [ $mount_ret -ne 0 ] && return $mount_ret
> > >      _idmapped_mount $TEST_DEV $TEST_DIR
> > > @@ -571,6 +581,12 @@ _metadump_dev() {
> > >  	esac
> > >  }
> > >  
> > > +_test_mkfs_options()
> > > +{
> > > +	_test_options mkfs
> > > +	echo $TEST_OPTIONS $MKFS_OPTIONS $* $TEST_DEV
> > > +}
> > > +
> > >  _test_mkfs()
> > >  {
> > >      case $FSTYP in
> > > -- 
> > > 2.43.0
> > > 

Re: [PATCH 5/5] common/rc: print scratch and test mount options

[Daniel Gomez]

On Mon, Jun 24, 2024 at 09:24:34AM GMT, Darrick J. Wong wrote:
> On Mon, Jun 24, 2024 at 02:02:33PM +0000, Daniel Gomez wrote:
> > On Fri, Jun 14, 2024 at 08:55:55AM GMT, Darrick J. Wong wrote:
> > > On Fri, Jun 14, 2024 at 06:17:26AM +0000, Daniel Gomez wrote:
> > > > Mount options for a SCRATCH device might not be the same in the TEST
> > > > device if RECREATE_TEST_DEV is not enabled. So, print mount options for
> > > > each device to clarify this.
> > > > 
> > > > Add mount and mkfs info for TEST devices so we get the same information
> > > > being printed for both devices.
> > > > 
> > > > Signed-off-by: Daniel Gomez <da.gomez@samsung.com>
> > > > ---
> > > >  check     |  4 ++++
> > > >  common/rc | 20 ++++++++++++++++++--
> > > >  2 files changed, 22 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/check b/check
> > > > index 723a52e30..e02d28b39 100755
> > > > --- a/check
> > > > +++ b/check
> > > > @@ -807,7 +807,11 @@ function run_section()
> > > >  	# print out our test configuration
> > > >  	echo "FSTYP         -- `_full_fstyp_details`"
> > > >  	echo "PLATFORM      -- `_full_platform_details`"
> > > > +	echo "TEST device:"
> > > > +	echo "MKFS_OPTIONS  -- `_test_mkfs_options`"
> > > > +	echo "MOUNT_OPTIONS -- `_test_mount_options`"
> > > >  	if [ ! -z "$SCRATCH_DEV" ]; then
> > > > +	  echo "SCRATCH device:"
> > > >  	  echo "MKFS_OPTIONS  -- `_scratch_mkfs_options`"
> > > >  	  echo "MOUNT_OPTIONS -- `_scratch_mount_options`"
> > > 
> > > Now there are two lines that start with "MKFS_OPTIONS"; will this break
> > > anyone's parsing scripts?  Or should these be prefixed:
> > > 
> > > echo "TEST_MKFS_OPTIONS -- `_test_mkfs_options`"
> > > ...
> > > echo "SCRATCH_MKFS_OPTIONS -- `_scratch_mkfs_options`"
> > > 
> > > ?
> > 
> > This looks like my initial version, but I prefer the 'sub-menu style' because
> > we did not have these variables before. However, I think it makes sense to
> > introduce them so we can differentiate between them. 
> 
> The trouble is, if your parsing script were doing something like
> splitting on "-- " then the "TEST device:" string would not split
> properly and you'd have to add context retention of some sort to know
> which "MKFS_OPTIONS" this was for.  Better just to namespace the new
> lines from the start.  I guess for compatibility's sake then we'd have
> to have:
> 
> TEST_MKFS_OPTIONS -- -o foo=bar
> MKFS_OPTIONS      -- -o foo=baz
> 
> in the output, along with a comment somewhere in the source that the
> non-prefixed scratch mkfs options is that way for hyst[eo]rical output
> parsing reasons.

We are also reporting as MOUNT_OPTIONS the scratch mount options. But
$MOUNT_OPTIONS are just the 'common' options, a subset of the scratch mount
options.

But I agree with only adding information for TEST device.

> 
> --D
> 
> > I guess introducing this change would break anyone's parsing scripts regardless?
> > However, I do think is necessary to specify the mount differences.
> > > 
> > > Also should these four variables be captured explicitly by the reports
> > > that are generated by common/report ?
> > 
> > I guess the report only includes the scratch mount options. I will update it to
> > include the new specific test and scratch mount/mkfs options.
> > 
> > > 
> > > --D
> > > 
> > > >  	fi
> > > > diff --git a/common/rc b/common/rc
> > > > index a42792c37..b351a82eb 100644
> > > > --- a/common/rc
> > > > +++ b/common/rc
> > > > @@ -235,6 +235,17 @@ _scratch_mount_options()
> > > >  					$SCRATCH_DEV $SCRATCH_MNT
> > > >  }
> > > >  
> > > > +_test_mount_options()
> > > > +{
> > > > +	_test_options mount
> > > > +
> > > > +	if $RECREATE_TEST_DEV || [ "$OLD_FSTYP" != "$FSTYP" ]; then
> > > > +		echo $TEST_OPTIONS $TEST_FS_MOUNT_OPTS $SELINUX_MOUNT_OPTIONS $TMPFS_MOUNT_OPTIONS $* $TEST_DEV $TEST_DIR
> > > > +	else
> > > > +		echo $TEST_OPTIONS $TEST_FS_MOUNT_OPTS $SELINUX_MOUNT_OPTIONS $* $TEST_DEV $TEST_DIR
> > > > +	fi
> > > > +}
> > > > +
> > > >  _supports_filetype()
> > > >  {
> > > >  	local dir=$1
> > > > @@ -456,8 +467,7 @@ _test_mount()
> > > >          return $?
> > > >      fi
> > > >  
> > > > -    _test_options mount
> > > > -    _mount -t $FSTYP$FUSE_SUBTYP $TEST_OPTIONS $TEST_FS_MOUNT_OPTS $SELINUX_MOUNT_OPTIONS $TMPFS_MOUNT_OPTIONS $* $TEST_DEV $TEST_DIR
> > > > +    _mount -t $FSTYP$FUSE_SUBTYP `_test_mount_options $*`
> > > >      mount_ret=$?
> > > >      [ $mount_ret -ne 0 ] && return $mount_ret
> > > >      _idmapped_mount $TEST_DEV $TEST_DIR
> > > > @@ -571,6 +581,12 @@ _metadump_dev() {
> > > >  	esac
> > > >  }
> > > >  
> > > > +_test_mkfs_options()
> > > > +{
> > > > +	_test_options mkfs
> > > > +	echo $TEST_OPTIONS $MKFS_OPTIONS $* $TEST_DEV
> > > > +}
> > > > +
> > > >  _test_mkfs()
> > > >  {
> > > >      case $FSTYP in
> > > > -- 
> > > > 2.43.0
> > > >