| Message ID | 20240614061722.1080-4-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: > 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 >
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 >
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 > > >
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 > > > > > >
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 > > > > > > > > > >
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 ;;
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(-)