[PATCH 0/4] fstests overlay fixes for v2025.05.25

[PATCH 0/4] fstests overlay fixes for v2025.05.25

[Amir Goldstein]

Zorro,

It's been a while since I upgraded my test machine.
A recent quick run with overlay had some failed tests.

This fixes some of them and opts-out of some.
The first patch is a re-post related to upgrade of my distro
to a newer distro (trixie) using libmount v1.41.

Thanks,
Amir.

Amir Goldstein (4):
  overlay: workaround libmount failure to remount,ro
  overlay: fix regression in _repair_overlay_scratch_fs
  generic/604: do not run with overlayfs
  generic/623: do not run with overlayfs

 common/overlay    |  6 +++++-
 common/rc         |  8 ++++++++
 tests/generic/330 |  2 +-
 tests/generic/604 | 11 ++++++-----
 tests/generic/623 |  1 +
 tests/overlay/035 |  2 +-
 6 files changed, 22 insertions(+), 8 deletions(-)

-- 
2.34.1

[PATCH 1/4] overlay: workaround libmount failure to remount,ro

[Amir Goldstein]

libmount v1.41 calls several unneeded fsconfig() calls to reconfigure
lowerdir/upperdir when user requests only -o remount,ro.

Those calls fail because overlayfs does not allow making any config
changes with new mount api, besides MS_RDONLY.

force mount(8) to use mount(2) to remount ro/rw to workaround
this issue, by setting LIBMOUNT_FORCE_MOUNT2=always.

Reported-by: André Almeida <andrealmeid@igalia.com>
Cc: Karel Zak <kzak@redhat.com>
Link: https://lore.kernel.org/linux-fsdevel/20250521-ovl_ro-v1-1-2350b1493d94@igalia.com/
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 common/overlay    | 4 +++-
 tests/overlay/035 | 2 +-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/common/overlay b/common/overlay
index 01b6622f..2e36fa3d 100644
--- a/common/overlay
+++ b/common/overlay
@@ -127,7 +127,9 @@ _overlay_base_scratch_mount()
 _overlay_scratch_mount()
 {
 	if echo "$*" | grep -q remount; then
-		$MOUNT_PROG $SCRATCH_MNT $*
+		# force mount(8) to use mount(2), to workaround libmount v1.41
+		# failed fsconfig() calls to reconfigure lowerdir/upperdir
+		LIBMOUNT_FORCE_MOUNT2=always $MOUNT_PROG $SCRATCH_MNT $*
 		return
 	fi
 
diff --git a/tests/overlay/035 b/tests/overlay/035
index 0b3257c4..2a4df99a 100755
--- a/tests/overlay/035
+++ b/tests/overlay/035
@@ -42,7 +42,7 @@ mkdir -p $lowerdir1 $lowerdir2 $upperdir $workdir
 # Verify that overlay is mounted read-only and that it cannot be remounted rw.
 _overlay_scratch_mount_opts -o"lowerdir=$lowerdir2:$lowerdir1"
 touch $SCRATCH_MNT/foo 2>&1 | _filter_scratch
-$MOUNT_PROG -o remount,rw $SCRATCH_MNT 2>&1 | _filter_ro_mount
+_scratch_remount rw 2>&1 | _filter_ro_mount
 $UMOUNT_PROG $SCRATCH_MNT
 
 # Make workdir immutable to prevent workdir re-create on mount
-- 
2.34.1

[PATCH 2/4] overlay: fix regression in _repair_overlay_scratch_fs

[Amir Goldstein]

_repair_overlay_scratch_fs assumed that the base fs is mounted.
This was a wrong assumption to make, and that was exposed by commit
4c6bc456 ("fstests: clean up mount and unmount operations") that
converted open coded umount in generic/332 to _scratch_unmount.

After this change, there errors were observed when running generic/332
if fsck.overlay is installed:

     Check for damage
    +fsck.overlay:[Error]: Faile to resolve upperdir:/vdf/ovl-upper:
                           No such file or directory
    +fsck.overlay failed, err=8
    +umount: /vdf: not mounted.

Fix this by making sure that base fs is mounted before running the
layers check and fix test generic/330 to conform with the umount
conversion patch.

Fixes: 4c6bc456 ("fstests: clean up mount and unmount operations")
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 common/overlay    | 2 ++
 tests/generic/330 | 2 +-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/common/overlay b/common/overlay
index 2e36fa3d..f05f8180 100644
--- a/common/overlay
+++ b/common/overlay
@@ -433,6 +433,8 @@ _check_overlay_scratch_fs()
 
 _repair_overlay_scratch_fs()
 {
+	# Base fs needs to be mounted for overlayfs check
+	_overlay_base_scratch_mount
 	_overlay_fsck_dirs $OVL_BASE_SCRATCH_MNT/$OVL_LOWER \
 		$OVL_BASE_SCRATCH_MNT/$OVL_UPPER \
 		$OVL_BASE_SCRATCH_MNT/$OVL_WORK -y
diff --git a/tests/generic/330 b/tests/generic/330
index c67defc4..901b17b1 100755
--- a/tests/generic/330
+++ b/tests/generic/330
@@ -61,7 +61,7 @@ md5sum $testdir/file1 | _filter_scratch
 md5sum $testdir/file2 | _filter_scratch
 
 echo "Check for damage"
-umount $SCRATCH_MNT
+_scratch_unmount
 _repair_scratch_fs >> $seqres.full
 
 # success, all done
-- 
2.34.1

[PATCH 3/4] generic/604: do not run with overlayfs

[Amir Goldstein]

Overlayfs does not allow mounting over again with the same layers
until umount is fully completed, so is not appropriate for this test
which tries to mount in parallel to umount.

This is manifested as the test failure below when overlayfs strict mount
checks are enabled by enabling the index feature:

$ echo Y > /sys/module/overlay/parameters/index
...
    +mount: /vdf/ovl-mnt: /vdf already mounted or mount point busy.
    +       dmesg(1) may have more information after failed mount system call.
    +mount /vdf /vdf/ovl-mnt failed

Opt-out of this test with overlayfs and remove the hacks that were placed
by commit 06cee932 ("generic/604: Fix for overlayfs") to make the test pass
with overlayfs in the first place.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 tests/generic/604 | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/tests/generic/604 b/tests/generic/604
index 744d3456..481250fd 100755
--- a/tests/generic/604
+++ b/tests/generic/604
@@ -13,6 +13,9 @@ _begin_fstest auto quick mount
 # Import common functions.
 . ./common/filter
 
+# Overlayfs does not allow mounting over again with the same layers
+# until umount is fully completed, so is not appropriate for this test.
+_exclude_fs overlay
 
 # Modify as appropriate.
 _require_scratch
@@ -22,11 +25,9 @@ _scratch_mount
 for i in $(seq 0 500); do
 	$XFS_IO_PROG -f -c "pwrite 0 4K" $SCRATCH_MNT/$i >/dev/null
 done
-# For overlayfs, avoid unmounting the base fs after _scratch_mount tries to
-# mount the base fs.  Delay the mount attempt by a small amount in the hope
-# that the mount() call will try to lock s_umount /after/ umount has already
-# taken it.
-_unmount $SCRATCH_MNT &
+# Delay the mount attempt by a small amount in the hope that the mount() call
+# will try to lock s_umount /after/ umount has already taken it.
+_scratch_unmount &
 sleep 0.01s ; _scratch_mount
 wait
 
-- 
2.34.1

[PATCH 3/4] generic/604: opt-out with overlayfs

[Amir Goldstein]

Overlayfs does not allow mounting over again with the same layers
until umount is fully completed, so is not appropriate for this test
which tries to mount in parallel to umount.

This is manifested as the test failure below when overlayfs strict mount
checks are enabled by enabling the index feature:

$ echo Y > /sys/module/overlay/parameters/index
...
    +mount: /vdf/ovl-mnt: /vdf already mounted or mount point busy.
    +       dmesg(1) may have more information after failed mount system call.
    +mount /vdf /vdf/ovl-mnt failed

Opt-out of this test with overlayfs and remove the hacks that were placed
by commit 06cee932 ("generic/604: Fix for overlayfs") to make the test pass
with overlayfs in the first place.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 tests/generic/604 | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/tests/generic/604 b/tests/generic/604
index 744d3456..481250fd 100755
--- a/tests/generic/604
+++ b/tests/generic/604
@@ -13,6 +13,9 @@ _begin_fstest auto quick mount
 # Import common functions.
 . ./common/filter
 
+# Overlayfs does not allow mounting over again with the same layers
+# until umount is fully completed, so is not appropriate for this test.
+_exclude_fs overlay
 
 # Modify as appropriate.
 _require_scratch
@@ -22,11 +25,9 @@ _scratch_mount
 for i in $(seq 0 500); do
 	$XFS_IO_PROG -f -c "pwrite 0 4K" $SCRATCH_MNT/$i >/dev/null
 done
-# For overlayfs, avoid unmounting the base fs after _scratch_mount tries to
-# mount the base fs.  Delay the mount attempt by a small amount in the hope
-# that the mount() call will try to lock s_umount /after/ umount has already
-# taken it.
-_unmount $SCRATCH_MNT &
+# Delay the mount attempt by a small amount in the hope that the mount() call
+# will try to lock s_umount /after/ umount has already taken it.
+_scratch_unmount &
 sleep 0.01s ; _scratch_mount
 wait
 
-- 
2.34.1

[PATCH 4/4] generic/623: do not run with overlayfs

[Amir Goldstein]

This test performs shutdown via xfs_io -c shutdown.

Overlayfs tests can use _scratch_shutdown, but they cannot use
"-c shutdown" xfs_io command without jumping through hoops, so by
default we do not support it.

Add this condition to _require_xfs_io_command and add the require
statement to test generic/623 so it wont run with overlayfs.

Reported-by: André Almeida <andrealmeid@igalia.com>
Link: https://lore.kernel.org/linux-fsdevel/20250521-ovl_ro-v1-1-2350b1493d94@igalia.com/
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 common/rc         | 8 ++++++++
 tests/generic/623 | 1 +
 2 files changed, 9 insertions(+)

diff --git a/common/rc b/common/rc
index d8ee8328..bffd576a 100644
--- a/common/rc
+++ b/common/rc
@@ -3033,6 +3033,14 @@ _require_xfs_io_command()
 		touch $testfile
 		testio=`$XFS_IO_PROG -c "syncfs" $testfile 2>&1`
 		;;
+	"shutdown")
+		if [ $FSTYP = "overlay" ]; then
+			# Overlayfs tests can use _scratch_shutdown, but they
+			# cannot use "-c shutdown" xfs_io command without jumping
+			# through hoops, so by default we do not support it.
+			_notrun "xfs_io $command not supported on $FSTYP"
+		fi
+		;;
 	*)
 		testio=`$XFS_IO_PROG -c "help $command" 2>&1`
 	esac
diff --git a/tests/generic/623 b/tests/generic/623
index b97e2adb..4e36daaf 100755
--- a/tests/generic/623
+++ b/tests/generic/623
@@ -16,6 +16,7 @@ _begin_fstest auto quick shutdown mmap
 
 _require_scratch_nocheck
 _require_scratch_shutdown
+_require_xfs_io_command "shutdown"
 
 _scratch_mkfs &>> $seqres.full
 _scratch_mount
-- 
2.34.1

Re: [PATCH 0/4] fstests overlay fixes for v2025.05.25

[André Almeida]

Hi Amir,

Thanks for the fixes!

First of all, you sent two patches for 3/4:
[PATCH 3/4] generic/604: do not run with overlayfs
[PATCH 3/4] generic/604: opt-out with overlayfs

I tested this with Linux 6.15 with the following command:
   $ sudo FSTYPE=ext4 TEST_DIR=/tmp/dir1 TEST_DEV=/dev/vdb 
SCRATCH_DEV=/dev/vdc SCRATCH_MNT=/tmp/dir2 ./check -overlay

These are the results before applying this patchset:

   Failures: generic/294 generic/306 generic/452 generic/599 generic/623 
overlay/019 overlay/035
   Failed 7 of 859 tests

After applying:

   Failures: generic/294 overlay/019
   Failed 2 of 859 tests

So the tests that I reported in my thread are now working.

All patches are:
Tested-by: André Almeida <andrealmeid@igalia.com>

Em 26/05/2025 11:34, Amir Goldstein escreveu:
> Zorro,
> 
> It's been a while since I upgraded my test machine.
> A recent quick run with overlay had some failed tests.
> 
> This fixes some of them and opts-out of some.
> The first patch is a re-post related to upgrade of my distro
> to a newer distro (trixie) using libmount v1.41.
> 
> Thanks,
> Amir.
> 
> Amir Goldstein (4):
>    overlay: workaround libmount failure to remount,ro
>    overlay: fix regression in _repair_overlay_scratch_fs
>    generic/604: do not run with overlayfs
>    generic/623: do not run with overlayfs
> 
>   common/overlay    |  6 +++++-
>   common/rc         |  8 ++++++++
>   tests/generic/330 |  2 +-
>   tests/generic/604 | 11 ++++++-----
>   tests/generic/623 |  1 +
>   tests/overlay/035 |  2 +-
>   6 files changed, 22 insertions(+), 8 deletions(-)
> 

Re: [PATCH 0/4] fstests overlay fixes for v2025.05.25

[Amir Goldstein]

On Mon, May 26, 2025 at 7:05 PM André Almeida <andrealmeid@igalia.com> wrote:
>
> Hi Amir,
>
> Thanks for the fixes!
>
> First of all, you sent two patches for 3/4:
> [PATCH 3/4] generic/604: do not run with overlayfs
> [PATCH 3/4] generic/604: opt-out with overlayfs

OOPS I meant to post the first one - it's just a change of
wording in the title.

>
> I tested this with Linux 6.15 with the following command:
>    $ sudo FSTYPE=ext4 TEST_DIR=/tmp/dir1 TEST_DEV=/dev/vdb
> SCRATCH_DEV=/dev/vdc SCRATCH_MNT=/tmp/dir2 ./check -overlay
>
> These are the results before applying this patchset:
>
>    Failures: generic/294 generic/306 generic/452 generic/599 generic/623
> overlay/019 overlay/035
>    Failed 7 of 859 tests
>
> After applying:
>
>    Failures: generic/294 overlay/019
>    Failed 2 of 859 tests
>

Those two tests pass for me on both xfs and ext4 base fs.

I am running with trixie libmount v1.41
with fstests tag v25.05.2025 (+my patches).

Please analyse the failures so we know if I am missing
something again.

> So the tests that I reported in my thread are now working.
>
> All patches are:
> Tested-by: André Almeida <andrealmeid@igalia.com>
>

Thanks!
Amir.

Re: [PATCH 1/4] overlay: workaround libmount failure to remount,ro

[Miklos Szeredi]

On Mon, 26 May 2025 at 16:35, Amir Goldstein <amir73il@gmail.com> wrote:
>
> libmount v1.41 calls several unneeded fsconfig() calls to reconfigure
> lowerdir/upperdir when user requests only -o remount,ro.

Isn't this a libmount bug then?

Working around it in xfstests just hides this, which seems counter productive.

Thanks,
Miklos

Re: [PATCH 1/4] overlay: workaround libmount failure to remount,ro

[Amir Goldstein]

On Tue, May 27, 2025 at 4:49 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Mon, 26 May 2025 at 16:35, Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > libmount v1.41 calls several unneeded fsconfig() calls to reconfigure
> > lowerdir/upperdir when user requests only -o remount,ro.
>
> Isn't this a libmount bug then?
>
> Working around it in xfstests just hides this, which seems counter productive.
>

Yes, to some extent, however, IMO the purpose of fstests is to test the
filesystem and the vfs and for very fs-specific tests it also tests the
*progs utils.

I do not think we can afford fstest being a test for libc+libmount and the
entire ecosystem. That's part of the reason that fstest implements
some of its own utilities to exercise syscalls.

If we leave the tests failing, we will loose test coverage from all
the people that are running with not-cutting-edge distro.
I don't think this is a desired outcome for us.
Test coverage for remount,ro is pretty important IMO.

FWIW, we already used LIBMOUNT_FORCE_MOUNT2 to workaround
another libmount bug I believe you were in the loop when we did that
(see blow).

Any other idea how to address those libmount bugs in the test suite
other than keeping the tests failing or not running for libmount >= v1.39?

Thanks,
Amir.

commit 30fc8ed13aa241e7caf1c27d1b60c64ab3ae7a18
Author: Amir Goldstein <amir73il@gmail.com>
Date:   Mon Oct 23 19:32:59 2023 +0300

    overlay: add test for lowerdir mount option parsing

    Check parsing and display of spaces and escaped colons and commans in
    lowerdir mount option.

    This is a regression test for two bugs introduced in v6.5 with the
    conversion to new mount api.

    There is another regression of new mount api related to libmount parsing
    of escaped commas, but this needs a fix in libmount - this test only
    verifies the fixes in the kernel, so it uses LIBMOUNT_FORCE_MOUNT2=always
    to force mount(2) and kernel pasring of the comma separated options list.

...

+# Kernel commit c34706acf40b fixes parsing of mount options with escaped comma
+# when the mount options string is provided via data argument to
mount(2) syscall.
+# With libmount >= 2.39, libmount itself will try to split the comma separated
+# options list provided to mount(8) commnad line and call fsconfig(2) for each
+# mount option seperately.  Since libmount does not obay to overlayfs escaped
+# commas format, it will call fsconfig(2) with the wrong path (i.e.
".../lower3")
+# and this test will fail, but the failure would indicate a libmount issue, not
+# a kernel issue.  Therefore, force libmount to use mount(2) syscall,
so we only
+# test the kernel fix.
+LIBMOUNT_FORCE_MOUNT2=always $MOUNT_PROG -t overlay
$OVL_BASE_SCRATCH_DEV $SCRATCH_MNT \
+       -o"upperdir=$upperdir,workdir=$workdir,lowerdir=$lowerdir_commas_esc"
2>> $seqres.full || \
+       echo "ERROR: incorrect parsing of escaped comma in lowerdir
mount option"

Re: [PATCH 1/4] overlay: workaround libmount failure to remount,ro

[Karel Zak]

On Wed, May 28, 2025 at 08:54:51AM +0200, Amir Goldstein wrote:
> On Tue, May 27, 2025 at 4:49 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
> >
> > On Mon, 26 May 2025 at 16:35, Amir Goldstein <amir73il@gmail.com> wrote:
> > >
> > > libmount v1.41 calls several unneeded fsconfig() calls to reconfigure
> > > lowerdir/upperdir when user requests only -o remount,ro.
> >
> > Isn't this a libmount bug then?

It's not just "only -o remount,ro"; the mount manual is quite explicit about it:

   The remount functionality follows the standard way the mount command works with
   options from fstab. This means that mount does not read fstab (or mtab) only when
   both device and dir are specified.

        mount -o remount,rw /dev/foo /dir

   After this call all old mount options are replaced and arbitrary stuff from fstab
   (or mtab) is ignored, except the loop= option which is internally generated and
   maintained by the mount command.

        mount -o remount,rw /dir

   After this call, mount reads fstab and merges these options with the options from
   the command line (-o). If no mountpoint is found in fstab, then it defaults to
   mount options from /proc/self/mountinfo.

   ...

   --options-mode mode
           Controls how to combine options from fstab/mtab with options from the command line.
           mode can be one of ignore, append, prepend or replace. For example, append means
           that options from fstab are appended to options from the command line. The default
           value is prepend — it means command line options are evaluated after fstab options.
           Note that the last option wins if there are conflicting ones.


Anyway, I agree that this semantics sucks, and from my point of view,
the best approach would be to introduce a new mount(8) command line
semantics to reflect the new kernel API, something like:

   mount modify [--clear noexec] [--set nodev,ro] [--make-private] [--recursive] /mnt
   mount reconfigure data=journal,errors=continue,foo,bar /mnt

and do not include options from fstab in this by default.

> > Working around it in xfstests just hides this, which seems counter productive.
> >
> 
> Yes, to some extent, however, IMO the purpose of fstests is to test the
> filesystem and the vfs and for very fs-specific tests it also tests the
> *progs utils.
> 
> I do not think we can afford fstest being a test for libc+libmount and the
> entire ecosystem. That's part of the reason that fstest implements
> some of its own utilities to exercise syscalls.
> 
> If we leave the tests failing, we will loose test coverage from all
> the people that are running with not-cutting-edge distro.
> I don't think this is a desired outcome for us.
> Test coverage for remount,ro is pretty important IMO.
> 
> FWIW, we already used LIBMOUNT_FORCE_MOUNT2 to workaround
> another libmount bug I believe you were in the loop when we did that
> (see blow).
> 
> Any other idea how to address those libmount bugs in the test suite
> other than keeping the tests failing or not running for libmount >= v1.39?

Why do you think it's a libmount bug? Do we really expect that
userspace will parse filesystem-specific mount options and "if
(overlayfs)" then it will exclude some options from fsconfig()?

# LIBMOUNT_FORCE_MOUNT2=always strace -e mount mount -o remount,ro /mnt/merged
mount("overlay", "/mnt/merged", 0x561d36355030, MS_RDONLY|MS_REMOUNT, "lowerdir=/mnt/low,upperdir=/mnt/"... ) = 0

# strace -e fsconfig mount -o remount,ro /mnt/merged
fsconfig(4, FSCONFIG_SET_STRING, "lowerdir", "/mnt/low", 0) = -1 EINVAL (Invalid argument)

As you can see, mount(2) is absolutely fine with lowerdir=. Why not fsconfig()?

I think the way libmount uses fsconfig() is exactly the same as how it
uses the classic mount(2), just sending the mount options to the
kernel. The difference is only that for the new mount kernel API, we
differentiate between filesystem options (fsconfig()) and VFS
attributes (mount_setattr()).

Would it be better to ignore attempts to reconfigure the
lowerdir/upperdir in the kernel?


And example if you suppress fstab/mountinfo:

# strace -e fsconfig mount --options-mode ignore -o remount,ro /mnt/merged
fsconfig(4, FSCONFIG_SET_FLAG, "ro", NULL, 0) = 0
fsconfig(4, FSCONFIG_CMD_RECONFIGURE, NULL, NULL, 0) = 0

or if you specify both, source and target:

# strace -e fsconfig mount  -o remount,ro overlay /mnt/merged
fsconfig(4, FSCONFIG_SET_FLAG, "ro", NULL, 0) = 0
fsconfig(4, FSCONFIG_CMD_RECONFIGURE, NULL, NULL, 0) = 0


So, you do not need LIBMOUNT_FORCE_MOUNT2= workaround, use
"--options-mode ignore" or source and target ;-)

    Karel


-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: [PATCH 1/4] overlay: workaround libmount failure to remount,ro

[Miklos Szeredi]

On Wed, 28 May 2025 at 10:47, Karel Zak <kzak@redhat.com> wrote:

> Anyway, I agree that this semantics sucks, and from my point of view,
> the best approach would be to introduce a new mount(8) command line
> semantics to reflect the new kernel API, something like:
>
>    mount modify [--clear noexec] [--set nodev,ro] [--make-private] [--recursive] /mnt
>    mount reconfigure data=journal,errors=continue,foo,bar /mnt
>
> and do not include options from fstab in this by default.

But there's no fstab entry in the testcase.  The no-fstab case likely
gets way more use in real life then remounting something in fstab.
And this should not need to get the current options from the kernel,
since the kernel is the source of the current options.

With the KISS principle in mind the non-fstab "mount -oremount,ro
/mnt/foo" should just be translated into:

fd = fspick(AT_FDCWD, "/mnt/foo", 0);
fsconfig(fd, FSCONFIG_SET_FLAG, "ro", NULL, 0);
fsconfig(fd, FSCONFIG_CMD_RECONFIGURE, NULL, NULL, 0);

and the kernel should take care of the rest.  I assume this doesn't
generally work, which is a pity, but I'd still think about salvaging
the concept.

> So, you do not need LIBMOUNT_FORCE_MOUNT2= workaround, use
> "--options-mode ignore" or source and target ;-)

Yeah, that's definitely a better workaround.

I wouldn't call it a fix, since "mount -oremount,ro /overlay" still
doesn't work the way it is supposed to, and the thought of adding code
to the kernel to work around the current libmount behavior makes me go
bleah.

Thanks,
Miklos

Re: [PATCH 1/4] overlay: workaround libmount failure to remount,ro

[Karel Zak]

On Wed, May 28, 2025 at 11:56:48AM +0200, Miklos Szeredi wrote:
> On Wed, 28 May 2025 at 10:47, Karel Zak <kzak@redhat.com> wrote:
> 
> > Anyway, I agree that this semantics sucks, and from my point of view,
> > the best approach would be to introduce a new mount(8) command line
> > semantics to reflect the new kernel API, something like:
> >
> >    mount modify [--clear noexec] [--set nodev,ro] [--make-private] [--recursive] /mnt
> >    mount reconfigure data=journal,errors=continue,foo,bar /mnt
> >
> > and do not include options from fstab in this by default.
> 
> But there's no fstab entry in the testcase.  The no-fstab case likely

Well, in this case it uses mountinfo

> gets way more use in real life then remounting something in fstab.
> And this should not need to get the current options from the kernel,
> since the kernel is the source of the current options.

This is how mount(8) works for decades, and I do not like it, but ...

The problem is something else. 

Do you see the paradox? The suggested LIBMOUNT_FORCE_MOUNT2 workaround
just switches to mount(2), but everything else remains the same; it
sends all the mount options to the kernel.

Why is it fine for mount(2) but wrong for fsconfig()? This is the
question. There is an incompatibility between the APIs.

> With the KISS principle in mind the non-fstab "mount -oremount,ro
> /mnt/foo" should just be translated into:
>
> fd = fspick(AT_FDCWD, "/mnt/foo", 0);
> fsconfig(fd, FSCONFIG_SET_FLAG, "ro", NULL, 0);
> fsconfig(fd, FSCONFIG_CMD_RECONFIGURE, NULL, NULL, 0);

The classic MS_REMOUNT has been interpreted for decades as "replace"
mount option, not just "replace only specified". This is why mount(8)
sends all options on remount.

However, "remount,ro" is such a common and specific use case that
perhaps we can make an exception and focus only on "ro".

> and the kernel should take care of the rest.  I assume this doesn't
> generally work, which is a pity, but I'd still think about salvaging
> the concept.
> 
> > So, you do not need LIBMOUNT_FORCE_MOUNT2= workaround, use
> > "--options-mode ignore" or source and target ;-)
> 
> Yeah, that's definitely a better workaround.
> 
> I wouldn't call it a fix, since "mount -oremount,ro /overlay" still
> doesn't work the way it is supposed to, and the thought of adding code
> to the kernel to work around the current libmount behavior makes me go
> bleah.

I sent straces; fsconfig() doesn't accept the options, but
mount(MS_REMOUNT) does. Why blame libmount?

We can change how libmount works with fstab on remount, but it will
just hide, not resolve, the problem with fsconfig().

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: [PATCH 1/4] overlay: workaround libmount failure to remount,ro

[Amir Goldstein]

On Wed, May 28, 2025 at 11:57 AM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Wed, 28 May 2025 at 10:47, Karel Zak <kzak@redhat.com> wrote:
>
> > Anyway, I agree that this semantics sucks, and from my point of view,
> > the best approach would be to introduce a new mount(8) command line
> > semantics to reflect the new kernel API, something like:
> >
> >    mount modify [--clear noexec] [--set nodev,ro] [--make-private] [--recursive] /mnt
> >    mount reconfigure data=journal,errors=continue,foo,bar /mnt
> >
> > and do not include options from fstab in this by default.
>
> But there's no fstab entry in the testcase.  The no-fstab case likely
> gets way more use in real life then remounting something in fstab.
> And this should not need to get the current options from the kernel,
> since the kernel is the source of the current options.
>
> With the KISS principle in mind the non-fstab "mount -oremount,ro
> /mnt/foo" should just be translated into:
>
> fd = fspick(AT_FDCWD, "/mnt/foo", 0);
> fsconfig(fd, FSCONFIG_SET_FLAG, "ro", NULL, 0);
> fsconfig(fd, FSCONFIG_CMD_RECONFIGURE, NULL, NULL, 0);
>

+1

> and the kernel should take care of the rest.  I assume this doesn't
> generally work, which is a pity, but I'd still think about salvaging
> the concept.
>
> > So, you do not need LIBMOUNT_FORCE_MOUNT2= workaround, use
> > "--options-mode ignore" or source and target ;-)
>
> Yeah, that's definitely a better workaround.
>

Agree. I will use a different workaround for fstest.

Thanks,
Amir.


> I wouldn't call it a fix, since "mount -oremount,ro /overlay" still
> doesn't work the way it is supposed to, and the thought of adding code
> to the kernel to work around the current libmount behavior makes me go
> bleah.
>
> Thanks,
> Miklos

Re: [PATCH 1/4] overlay: workaround libmount failure to remount,ro

[Miklos Szeredi]

On Wed, 28 May 2025 at 12:55, Karel Zak <kzak@redhat.com> wrote:

> Why is it fine for mount(2) but wrong for fsconfig()? This is the
> question. There is an incompatibility between the APIs.

Where is it documented that the fsconfig(2) shall have the same
replace semantics as mount(2)?

When I reviewed the new API, I certainly didn't think that it should
have these semantics, and I'm not even sure it did have it back then.
But of course I may have missed it.

I think this is just a bad accident, and I'm wondering if this could
still be fixed in a way that doesn't introduce more hackery.

One idea is to introduce a flag (e.g. FSPICK_REPLACE_ONLY) that makes
the filesystem initialize the fs_context with the current options.
That would work, no?

Thanks,
Miklos

Re: [PATCH 1/4] overlay: workaround libmount failure to remount,ro

[Karel Zak]

On Wed, May 28, 2025 at 01:47:37PM +0200, Miklos Szeredi wrote:
> On Wed, 28 May 2025 at 12:55, Karel Zak <kzak@redhat.com> wrote:
> 
> > Why is it fine for mount(2) but wrong for fsconfig()? This is the
> > question. There is an incompatibility between the APIs.
> 
> Where is it documented that the fsconfig(2) shall have the same
> replace semantics as mount(2)?

Okay, fair enough, but you understand that userspace needs to adapt to
the new API, and ideally, changes should not be introduced to
end-users.

Frankly, I already have a private TODO file for libmount2 because I
feel we need to move forward, simplify things, move away from the
mount(2) syscall, and fully adapt to the new kernel API, which
provides features we do not yet support on the command line. The
current mount(8) command line and semantics are based on the mount(2)
era. The remount with all the obscure options (like MS_REMOUNT|MS_BIND)
is the worst legacy.

> I think this is just a bad accident, and I'm wondering if this could
> still be fixed in a way that doesn't introduce more hackery.

Why can't filesystems silently ignore fsconfig() requests that do not
introduce a change? For example, if the current setting is foo=123 and
fsconfig() is used to change it to foo=123, why is it reported as an
error? It's stupid, but just no op.

> One idea is to introduce a flag (e.g. FSPICK_REPLACE_ONLY) that makes
> the filesystem initialize the fs_context with the current options.
> That would work, no?

I'm not sure I understand how it will affect userspace. Do you mean
that with the flag, the kernel will assume a completely new set of
options from userspace, and the filesystem will adapt (if possible) to
the new settings?

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: [PATCH 1/4] overlay: workaround libmount failure to remount,ro

[Miklos Szeredi]

On Wed, 28 May 2025 at 15:13, Karel Zak <kzak@redhat.com> wrote:

> Why can't filesystems silently ignore fsconfig() requests that do not
> introduce a change? For example, if the current setting is foo=123 and
> fsconfig() is used to change it to foo=123, why is it reported as an
> error? It's stupid, but just no op.

It's a workaround for legacy behavior.   For overlayfs it doesn't make
sense to specify "lowerdir=" option for reconfigure and as such it is
rejected.  What you suggest is a hack, which if there was no other way
I'd accept, but I think there are better ways to fix this.

> I'm not sure I understand how it will affect userspace. Do you mean
> that with the flag, the kernel will assume a completely new set of
> options from userspace, and the filesystem will adapt (if possible) to
> the new settings?

Maybe the naming wasn't good.

I meant the opposite of what you describe: the kernel would guarantee
that only the supplied options change.  This is already what sane
filesystems do and they would not have to be changed at all to support
this flag.

Or maybe all filesystems do this?  I haven't checked, but I assumed
that libmount does the "append to current options" even for the new
API because without it some fs are broke.

Thanks,
Miklos




>
>     Karel
>
> --
>  Karel Zak  <kzak@redhat.com>
>  http://karelzak.blogspot.com
>