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