[PATCH v2 0/5] common fixes

[PATCH v2 0/5] common fixes

[Daniel Gomez via B4 Relay]

Config section fixes for scratch and recreation test devices.

Changes since v1:
* Drop tmpfs/449 patch.
Note: I will submit it in a different series with the feedback received.
* Applied review tags from Darrick and Zorro for patch 1.
* Do not inject specific TMPFS mount options in recreation and scratch
mount options. Instead,
  * Fix recreation test device by allowing section parsing to read
specific fs mount options when RECREATE_TEST_DEV is enabled.
  * Use _common_mount_opts() for scratch mounts instead of
_common_dev_mount_options() to allow parsing section fs specific mount
options for scratch devices.
* print only test mkfs and mount devices instead of adding scratch and
test sections.
* Add/export and report new TEST_MKFS_OPTIONS and TEST_MOUNT_OPTIONS.
* Change patch series subject.

Signed-off-by: Daniel Gomez <da.gomez@samsung.com>
---
Daniel Gomez (5):
      common/config: fix RECREATE_TEST_DEV initialization
      common/rc: add recreation support for tmpfs
      common/config: enable section parsing when recreation
      common/rc: read config section mount options for scratch devs
      common/rc: print test mount options

 check         |  2 ++
 common/config |  4 ++--
 common/rc     | 26 +++++++++++++++++++++++---
 common/report |  1 +
 4 files changed, 28 insertions(+), 5 deletions(-)
---
base-commit: 83598d2f839d9c27d2fd4209124d7c288ea2861e
change-id: 20240629-common-fixes-0a5c58db85c9

Best regards,
-- 
Daniel Gomez <da.gomez@samsung.com>

[PATCH v2 1/5] common/config: fix RECREATE_TEST_DEV initialization

[Daniel Gomez via B4 Relay]

From: Daniel Gomez <da.gomez@samsung.com>

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>
Reviewed-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: Zorro Lang <zlang@redhat.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

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

[Daniel Gomez via B4 Relay]

From: Daniel Gomez <da.gomez@samsung.com>

Add support for test device recreation (RECREATE_TEST_DEV=true) for
tmpfs.

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

diff --git a/common/rc b/common/rc
index 163041fea..51827119c 100644
--- a/common/rc
+++ b/common/rc
@@ -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 v2 3/5] common/config: enable section parsing when recreation

[Daniel Gomez via B4 Relay]

From: Daniel Gomez <da.gomez@samsung.com>

When RECREATE_TEST_DEV is enabled, specific fs mount options such as
TMPFS_MOUNT_OPTIONS are ignored during section parsing unless the FSTYP
is modified. This allows the filesystem type to remain the same while
only specific fs mount options are changed during the parsing of section
parameters for test device recreation or when the section with speciic
fs mount options are different from the default.

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

-- 
2.43.0

[PATCH v2 4/5] common/rc: read config section mount options for scratch devs

[Daniel Gomez via B4 Relay]

From: Daniel Gomez <da.gomez@samsung.com>

MOUNT_OPTIONS for scratch devices do not have specific fs mount options
values (such as TMPFS_MOUNT_OPTIONS) from the config section. So, allow
the scratch mount options to inherit section config values by reading
them from _common_mount_opts().

Move $* to the end so scratch device can be umount/mount. Otherwise, we
end up with multiple mounts in the same scratch directory.

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

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 $*
 }
 
 _supports_filetype()

-- 
2.43.0

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

[Daniel Gomez via B4 Relay]

From: Daniel Gomez <da.gomez@samsung.com>

Mount options for a SCRATCH device might not be the same for a TEST
device if RECREATE_TEST_DEV is not enabled. Add mount options for TEST
devices when printing fstest header to clarify this.

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

Export new TEST_{MKFS/MOUNT}_OPTIONS and include them in the report.

Signed-off-by: Daniel Gomez <da.gomez@samsung.com>
---
 check         |  2 ++
 common/rc     | 19 ++++++++++++++++++-
 common/report |  1 +
 3 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/check b/check
index 9222cd7e4..ca30771c5 100755
--- a/check
+++ b/check
@@ -819,6 +819,8 @@ function run_section()
 	# print out our test configuration
 	echo "FSTYP         -- `_full_fstyp_details`"
 	echo "PLATFORM      -- `_full_platform_details`"
+	echo "TEST_MKFS_OPTIONS  -- `_test_mkfs_options`"
+	echo "TEST_MOUNT_OPTIONS -- `_test_mount_options`"
 	if [ ! -z "$SCRATCH_DEV" ]; then
 	  echo "MKFS_OPTIONS  -- `_scratch_mkfs_options`"
 	  echo "MOUNT_OPTIONS -- `_scratch_mount_options`"
diff --git a/common/rc b/common/rc
index 627dbaaaa..bbbd274a7 100644
--- a/common/rc
+++ b/common/rc
@@ -235,6 +235,15 @@ _scratch_mount_options()
 					$SCRATCH_DEV $SCRATCH_MNT $*
 }
 
+_test_mount_options()
+{
+	_test_options mount
+
+	export TEST_MOUNT_OPTIONS="$TEST_OPTIONS $TEST_FS_MOUNT_OPTS \
+				$SELINUX_MOUNT_OPTIONS $* $TEST_DEV $TEST_DIR"
+	echo $TEST_MOUNT_OPTIONS
+}
+
 _supports_filetype()
 {
 	local dir=$1
@@ -457,7 +466,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_mount_options $*`
     mount_ret=$?
     [ $mount_ret -ne 0 ] && return $mount_ret
     _idmapped_mount $TEST_DEV $TEST_DIR
@@ -571,6 +580,14 @@ _metadump_dev() {
 	esac
 }
 
+_test_mkfs_options()
+{
+	_test_options mkfs
+
+	export TEST_MKFS_OPTIONS="$TEST_OPTIONS $MKFS_OPTIONS $* $TEST_DEV"
+	echo $TEST_MKFS_OPTIONS
+}
+
 _test_mkfs()
 {
     case $FSTYP in
diff --git a/common/report b/common/report
index 0e91e481f..44de33e61 100644
--- a/common/report
+++ b/common/report
@@ -5,6 +5,7 @@
 # List of xfstests's enviroment variables to include reports
 ## TODO automate list population inside common/conf
 REPORT_ENV_LIST=("SECTION" "FSTYP" "PLATFORM" "MKFS_OPTIONS" "MOUNT_OPTIONS" \
+		 "TEST_MKFS_OPTIONS" "TEST_MOUNT_OPTIONS" \
 		 "HOST_OPTIONS" "CHECK_OPTIONS" "XFS_MKFS_OPTIONS" \
 		 "TIME_FACTOR" "LOAD_FACTOR" "TEST_DIR" "TEST_DEV" \
 		 "SCRATCH_DEV" "SCRATCH_MNT" "OVL_UPPER" "OVL_LOWER" "OVL_WORK")

-- 
2.43.0

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

[Darrick J. Wong]

On Sun, Jun 30, 2024 at 11:52:41PM +0200, Daniel Gomez via B4 Relay wrote:
> From: Daniel Gomez <da.gomez@samsung.com>
> 
> Add support for test device recreation (RECREATE_TEST_DEV=true) for
> tmpfs.
> 
> Signed-off-by: Daniel Gomez <da.gomez@samsung.com>
> ---
>  common/rc | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/common/rc b/common/rc
> index 163041fea..51827119c 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -604,6 +604,9 @@ _test_mkfs()
>      pvfs2)
>  	# do nothing for pvfs2
>  	;;
> +	tmpfs)

Indentation problem here.

> +	# do nothing for tmpfs

If we're recreating the test filesystem, shouldn't that unmount and
remount for tmpfs?  Or at least rm -rf everything underneath it?  That's
generally the effect of _test_mkfs for disk filesystems.

(That said, I'm much less familiar with non-disk filesystems...)

--D

> +	;;
>      udf)
>          $MKFS_UDF_PROG $MKFS_OPTIONS $* $TEST_DEV > /dev/null
>  	;;
> 
> -- 
> 2.43.0
> 
> 
> 

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

[Daniel Gomez (Samsung)]

On Tue, Jul 2, 2024 at 12:36 AM Darrick J. Wong <djwong@kernel.org> wrote:
>
> On Sun, Jun 30, 2024 at 11:52:41PM +0200, Daniel Gomez via B4 Relay wrote:
> > From: Daniel Gomez <da.gomez@samsung.com>
> >
> > Add support for test device recreation (RECREATE_TEST_DEV=true) for
> > tmpfs.
> >
> > Signed-off-by: Daniel Gomez <da.gomez@samsung.com>
> > ---
> >  common/rc | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/common/rc b/common/rc
> > index 163041fea..51827119c 100644
> > --- a/common/rc
> > +++ b/common/rc
> > @@ -604,6 +604,9 @@ _test_mkfs()
> >      pvfs2)
> >       # do nothing for pvfs2
> >       ;;
> > +     tmpfs)
>
> Indentation problem here.

What's actually the format for indentation in bash scripts across
xfstests-dev project? I see a mix of tabs and spaces everywhere. For
this particular _test_mkfs(), I see:
<spaces>overlay)
<tabs># do nothing for overlay
<tabs>;;
<spaces>pvfs2)
<tabs># do nothing for pvfs2
<tabs>;;
<spaces>udf)
<spaces><spaces>$MKFS_UDF_PROG ...
<tabs>;;
<spaces>btrfs)
<spaces><spaces>$MKFS_BTRFS_PROG...



>
> > +     # do nothing for tmpfs
>
> If we're recreating the test filesystem, shouldn't that unmount and
> remount for tmpfs?  Or at least rm -rf everything underneath it?  That's
> generally the effect of _test_mkfs for disk filesystems.

I thought this was already supported. When I enable recreation, I get
the following error:

./check -s tmpfs_huge_always -R xunit generic/003
SECTION       -- tmpfs_huge_always
RECREATING    -- tmpfs on /media/test
our local _test_mkfs routine ...
mkfs: failed to execute mkfs.tmpfs: No such file or directory
check: failed to mkfs $TEST_DEV using specified options
Interrupted!
Interrupted!
Passed all 0 tests
Xunit report: /result.xml
SECTION       -- tmpfs_huge_always
=========================
Passed all 0 tests

So, adding the tmpfs) case to _test_mkfs() just lets recreation go
through, and mount and umount when the test is finished. I'm not sure
if I'm missing something or maybe I should rename this patch as a fix?

>
> (That said, I'm much less familiar with non-disk filesystems...)

When umounting a tmpfs mount dir, data is lost. So in the recreation
case, _test_unmount() would take care of that.

>
> --D
>
> > +     ;;
> >      udf)
> >          $MKFS_UDF_PROG $MKFS_OPTIONS $* $TEST_DEV > /dev/null
> >       ;;
> >
> > --
> > 2.43.0
> >
> >
> >

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

[Darrick J. Wong]

On Fri, Jul 05, 2024 at 11:46:53PM +0200, Daniel Gomez (Samsung) wrote:
> On Tue, Jul 2, 2024 at 12:36 AM Darrick J. Wong <djwong@kernel.org> wrote:
> >
> > On Sun, Jun 30, 2024 at 11:52:41PM +0200, Daniel Gomez via B4 Relay wrote:
> > > From: Daniel Gomez <da.gomez@samsung.com>
> > >
> > > Add support for test device recreation (RECREATE_TEST_DEV=true) for
> > > tmpfs.
> > >
> > > Signed-off-by: Daniel Gomez <da.gomez@samsung.com>
> > > ---
> > >  common/rc | 3 +++
> > >  1 file changed, 3 insertions(+)
> > >
> > > diff --git a/common/rc b/common/rc
> > > index 163041fea..51827119c 100644
> > > --- a/common/rc
> > > +++ b/common/rc
> > > @@ -604,6 +604,9 @@ _test_mkfs()
> > >      pvfs2)
> > >       # do nothing for pvfs2
> > >       ;;
> > > +     tmpfs)
> >
> > Indentation problem here.
> 
> What's actually the format for indentation in bash scripts across
> xfstests-dev project? I see a mix of tabs and spaces everywhere. For
> this particular _test_mkfs(), I see:
> <spaces>overlay)
> <tabs># do nothing for overlay
> <tabs>;;
> <spaces>pvfs2)
> <tabs># do nothing for pvfs2
> <tabs>;;
> <spaces>udf)
> <spaces><spaces>$MKFS_UDF_PROG ...
> <tabs>;;
> <spaces>btrfs)
> <spaces><spaces>$MKFS_BTRFS_PROG...

I don't think there's a set convention anywhere, I usually just copy the
thing above it.  I was reacting to the diff; if you actually just copied
the pvfs2 clause and then s/pvfs2/tmpfs/ then I withdraw the comment.

> > > +     # do nothing for tmpfs
> >
> > If we're recreating the test filesystem, shouldn't that unmount and
> > remount for tmpfs?  Or at least rm -rf everything underneath it?  That's
> > generally the effect of _test_mkfs for disk filesystems.
> 
> I thought this was already supported. When I enable recreation, I get
> the following error:
> 
> ./check -s tmpfs_huge_always -R xunit generic/003
> SECTION       -- tmpfs_huge_always
> RECREATING    -- tmpfs on /media/test
> our local _test_mkfs routine ...
> mkfs: failed to execute mkfs.tmpfs: No such file or directory
> check: failed to mkfs $TEST_DEV using specified options
> Interrupted!
> Interrupted!
> Passed all 0 tests
> Xunit report: /result.xml
> SECTION       -- tmpfs_huge_always
> =========================
> Passed all 0 tests
> 
> So, adding the tmpfs) case to _test_mkfs() just lets recreation go
> through, and mount and umount when the test is finished. I'm not sure
> if I'm missing something or maybe I should rename this patch as a fix?

Ah!  Yes, you're right, I forgot that there is no mkfs.tmpfs, everything
is done by mount -t tmpfs.  Can you change the comment to explain why
_test_mkfs does nothing for tmpfs?

    tmpfs)
	# mount initializes the fs, no need to format anything
	;;

for dunces like me? :)

> >
> > (That said, I'm much less familiar with non-disk filesystems...)
> 
> When umounting a tmpfs mount dir, data is lost. So in the recreation
> case, _test_unmount() would take care of that.

<nod>

--D

> >
> > --D
> >
> > > +     ;;
> > >      udf)
> > >          $MKFS_UDF_PROG $MKFS_OPTIONS $* $TEST_DEV > /dev/null
> > >       ;;
> > >
> > > --
> > > 2.43.0
> > >
> > >
> > >
> 

Re: [PATCH v2 4/5] common/rc: read config section mount options for scratch devs

[Zirong Lang]

On Mon, Jul 1, 2024 at 10:52 PM Daniel Gomez via B4 Relay
<devnull+da.gomez.samsung.com@kernel.org> wrote:
>
> From: Daniel Gomez <da.gomez@samsung.com>
>
> MOUNT_OPTIONS for scratch devices do not have specific fs mount options
> values (such as TMPFS_MOUNT_OPTIONS) from the config section. So, allow
> the scratch mount options to inherit section config values by reading
> them from _common_mount_opts().
>
> Move $* to the end so scratch device can be umount/mount. Otherwise, we
> end up with multiple mounts in the same scratch directory.
>
> Signed-off-by: Daniel Gomez <da.gomez@samsung.com>
> ---
>  common/rc | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> 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 $*

Why change the _common_dev_mount_options to _common_mount_opts? They're totally
different two functions. And this change will cause many test cases fail.

Thanks,
Zorro


>
>  }
>
>  _supports_filetype()
>
> --
> 2.43.0
>
>
>

Re: [PATCH v2 4/5] common/rc: read config section mount options for scratch devs

[Zorro Lang]

On Sun, Jun 30, 2024 at 11:52:43PM +0200, Daniel Gomez via B4 Relay wrote:
> From: Daniel Gomez <da.gomez@samsung.com>
> 
> MOUNT_OPTIONS for scratch devices do not have specific fs mount options
> values (such as TMPFS_MOUNT_OPTIONS) from the config section. So, allow
> the scratch mount options to inherit section config values by reading
> them from _common_mount_opts().
> 
> Move $* to the end so scratch device can be umount/mount. Otherwise, we
> end up with multiple mounts in the same scratch directory.
> 
> Signed-off-by: Daniel Gomez <da.gomez@samsung.com>
> ---
>  common/rc | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> 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 $*

Oh, you want to get the "specific fs mount options". But sorry this
change brings in many regression failures, e.g. generic/219, generic/691,
generc/381, generic/382 ... fail (on ext4 and so on).

Thanks,
Zorro

>  }
>  
>  _supports_filetype()
> 
> -- 
> 2.43.0
> 
> 
> 

Re: [PATCH v2 4/5] common/rc: read config section mount options for scratch devs

[Daniel Gomez]

On Fri, Jul 12, 2024 at 05:49:24PM GMT, Zorro Lang wrote:
> On Sun, Jun 30, 2024 at 11:52:43PM +0200, Daniel Gomez via B4 Relay wrote:
> > From: Daniel Gomez <da.gomez@samsung.com>
> > 
> > MOUNT_OPTIONS for scratch devices do not have specific fs mount options
> > values (such as TMPFS_MOUNT_OPTIONS) from the config section. So, allow
> > the scratch mount options to inherit section config values by reading
> > them from _common_mount_opts().
> > 
> > Move $* to the end so scratch device can be umount/mount. Otherwise, we
> > end up with multiple mounts in the same scratch directory.
> > 
> > Signed-off-by: Daniel Gomez <da.gomez@samsung.com>
> > ---
> >  common/rc | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > 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 $*
> 
> Oh, you want to get the "specific fs mount options". But sorry this
> change brings in many regression failures, e.g. generic/219, generic/691,
> generc/381, generic/382 ... fail (on ext4 and so on).

I'm assuming the scratch device should have the default mount options provided
by _common_mount_opts(). Otherwise, I would have to include the default mount
options ('-o size=1G <section-mount-option>') in every section of my config. So,
why a scratch device should not have the defaults?

> 
> Thanks,
> Zorro
> 
> >  }
> >  
> >  _supports_filetype()
> > 
> > -- 
> > 2.43.0
> > 
> > 
> > 

Re: [PATCH v2 4/5] common/rc: read config section mount options for scratch devs

[Daniel Gomez]

On Fri, Jul 12, 2024 at 05:49:24PM GMT, Zorro Lang wrote:
> On Sun, Jun 30, 2024 at 11:52:43PM +0200, Daniel Gomez via B4 Relay wrote:
> > From: Daniel Gomez <da.gomez@samsung.com>
> > 
> > MOUNT_OPTIONS for scratch devices do not have specific fs mount options
> > values (such as TMPFS_MOUNT_OPTIONS) from the config section. So, allow
> > the scratch mount options to inherit section config values by reading
> > them from _common_mount_opts().
> > 
> > Move $* to the end so scratch device can be umount/mount. Otherwise, we
> > end up with multiple mounts in the same scratch directory.
> > 
> > Signed-off-by: Daniel Gomez <da.gomez@samsung.com>
> > ---
> >  common/rc | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > 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 $*
> 
> Oh, you want to get the "specific fs mount options". But sorry this
> change brings in many regression failures, e.g. generic/219, generic/691,
> generc/381, generic/382 ... fail (on ext4 and so on).

Kindly ping.

Following my previous question, I'm trying to elaborate more:

Could you please clarify why these specific filesystem mount options should not
be applied to scratch devices? If this is a regression, could it be due to
the application of $EXT_MOUNT_OPTIONS? And maybe a configuration issue?

The purpose of this change was to provide the common mount options for scratch
devices, so that I can inherit the required options for tmpfs (e.g., '-o size=1G
$TMPFS_MOUNT_OPTIONS').

Here is my current configuration, but it seems I'm missing something, as I'm
expecting the scratch (and test) mounts to inherit $TMPFS_MOUNT_OPTIONS when
running ./check -s <config>.

mkdir -p /media/scratch
mkdir -p /media/test
export FSTYP=tmpfs
export SCRATCH_DEV=/media/scratch
export SCRATCH_MNT=/media/scratch
export TEST_DEV=/media/test
export TEST_DIR=/media/test
export RECREATE_TEST_DEV=true

# Test with default mount options
[tmpfs_default]

[tmpfs_noswap_huge_never]
TMPFS_MOUNT_OPTIONS="-o noswap,huge=never"

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

> 
> Thanks,
> Zorro
> 
> >  }
> >  
> >  _supports_filetype()
> > 
> > -- 
> > 2.43.0
> > 
> > 
> > 

Re: [PATCH v2 4/5] common/rc: read config section mount options for scratch devs

[Zorro Lang]

On Mon, Aug 05, 2024 at 12:04:29PM +0000, Daniel Gomez wrote:
> On Fri, Jul 12, 2024 at 05:49:24PM GMT, Zorro Lang wrote:
> > On Sun, Jun 30, 2024 at 11:52:43PM +0200, Daniel Gomez via B4 Relay wrote:
> > > From: Daniel Gomez <da.gomez@samsung.com>
> > > 
> > > MOUNT_OPTIONS for scratch devices do not have specific fs mount options
> > > values (such as TMPFS_MOUNT_OPTIONS) from the config section. So, allow
> > > the scratch mount options to inherit section config values by reading
> > > them from _common_mount_opts().
> > > 
> > > Move $* to the end so scratch device can be umount/mount. Otherwise, we
> > > end up with multiple mounts in the same scratch directory.
> > > 
> > > Signed-off-by: Daniel Gomez <da.gomez@samsung.com>
> > > ---
> > >  common/rc | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > 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 $*
> > 
> > Oh, you want to get the "specific fs mount options". But sorry this
> > change brings in many regression failures, e.g. generic/219, generic/691,
> > generc/381, generic/382 ... fail (on ext4 and so on).
> 
> Kindly ping.
> 
> Following my previous question, I'm trying to elaborate more:
> 
> Could you please clarify why these specific filesystem mount options should not
> be applied to scratch devices? If this is a regression, could it be due to
> the application of $EXT_MOUNT_OPTIONS? And maybe a configuration issue?

It's not an issue about "if these specific filesystem mount options should not
be applied to scratch devices". The thing is _common_dev_mount_options() and
_common_mount_opts() are totally different two functions, we shouldn't replace
one with the other rudely. You change brought in _common_mount_opts, then removed
the _common_dev_mount_options directly, but they're not equivalent substitution.

> 
> The purpose of this change was to provide the common mount options for scratch
> devices, so that I can inherit the required options for tmpfs (e.g., '-o size=1G
> $TMPFS_MOUNT_OPTIONS').

BTW, I'm wondering, as you've done this below change in [PATCH 3/5]:

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

Won't that help to reset "MOUNT_OPTIONS" to "-o size=1G $TMPFS_MOUNT_OPTIONS", if
you set RECREATE_TEST_DEV=true in a new section?

Thanks,
Zorro

> 
> Here is my current configuration, but it seems I'm missing something, as I'm
> expecting the scratch (and test) mounts to inherit $TMPFS_MOUNT_OPTIONS when
> running ./check -s <config>.
> 
> mkdir -p /media/scratch
> mkdir -p /media/test
> export FSTYP=tmpfs
> export SCRATCH_DEV=/media/scratch
> export SCRATCH_MNT=/media/scratch
> export TEST_DEV=/media/test
> export TEST_DIR=/media/test
> export RECREATE_TEST_DEV=true
> 
> # Test with default mount options
> [tmpfs_default]
> 
> [tmpfs_noswap_huge_never]
> TMPFS_MOUNT_OPTIONS="-o noswap,huge=never"
> 
> [tmpfs_noswap_huge_always]
> TMPFS_MOUNT_OPTIONS="-o noswap,huge=always"
> 
> > 
> > Thanks,
> > Zorro
> > 
> > >  }
> > >  
> > >  _supports_filetype()
> > > 
> > > -- 
> > > 2.43.0
> > > 
> > > 
> > > 
>