diff mbox series

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

Message ID 20240630-common-fixes-v2-4-16d26fb1dee0@samsung.com (mailing list archive)
State New, archived
Headers show
Series common fixes | expand

Commit Message

Daniel Gomez via B4 Relay June 30, 2024, 9:52 p.m. UTC 
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(-)

Comments

Zorro Lang July 12, 2024, 9:26 a.m. UTC | #1 
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
>
>
>
Zorro Lang July 12, 2024, 9:49 a.m. UTC | #2 
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
> 
> 
>
Daniel Gomez July 15, 2024, 8:17 a.m. UTC | #3 
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
> > 
> > 
> >
Daniel Gomez Aug. 5, 2024, 12:04 p.m. UTC | #4 
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
> > 
> > 
> >
Zorro Lang Aug. 6, 2024, 1:11 p.m. UTC | #5 
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
> > > 
> > > 
> > > 
>
diff mbox series

Patch

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