| Message ID | 20240614061722.1080-5-da.gomez@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | tmpfs fixes | expand |
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 >
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 > >
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 > > >
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 > > > >
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()
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(-)