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

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

[Amir Goldstein]

Zorro,

My first batch of fixes [1] was addressing overlayfs test failures after
upgrade of test machine to a newer distro (trixie) with libmount v1.41.

After a closer look, I found some tests that regressed into _notrun,
which is harder to notice at first glance.

Tests generic/{633,697,697} were regressed in v2025.03.17.
Test generic/699 always had a problem AFAICT, but I only just tested
it with custom MOUNT_OPTIONS.

Lastly, following the review discussion between Miklos and Karel Zak,
I changed the workaround to the regressions observed after upgrade to
newer libmount.

Thanks,
Amir.

Changes since v1 [1]:
- Change workaround from LIBMOUNT_FORCE_MOUNT2 to --options-mode=ignore
- Add two commits to fix tests that were not running by mistake

[1] https://lore.kernel.org/fstests/20250526143500.1520660-1-amir73il@gmail.com/

Amir Goldstein (6):
  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
  generic: remove incorrect _require_idmapped_mounts checks
  generic/699: fix failure with MOUNT_OPTIONS

 common/overlay    |  7 ++++++-
 common/rc         | 10 +++++++++-
 tests/generic/330 |  2 +-
 tests/generic/604 | 11 ++++++-----
 tests/generic/623 |  1 +
 tests/generic/633 |  1 -
 tests/generic/696 |  1 -
 tests/generic/697 |  1 -
 tests/generic/699 | 17 ++++++++++-------
 tests/overlay/035 |  2 +-
 10 files changed, 34 insertions(+), 19 deletions(-)

-- 
2.34.1

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

[Amir Goldstein]

libmount >= v1.39 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.

We workaround this problem with --options-mode ignore.

Reported-by: André Almeida <andrealmeid@igalia.com>
Suggested-by: Karel Zak <kzak@redhat.com>
Link: https://lore.kernel.org/linux-fsdevel/20250521-ovl_ro-v1-1-2350b1493d94@igalia.com/
Link: https://lore.kernel.org/fstests/CAJfpegtJ3SDKmC80B4AfWiC3JmtWdW2+78fRZVtsuhe-wSRPvg@mail.gmail.com/
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---

Changes since v1 [1]:
- Change workaround from LIBMOUNT_FORCE_MOUNT2 to --options-mode=ignore

[1] https://lore.kernel.org/fstests/20250526143500.1520660-1-amir73il@gmail.com/

 common/overlay    | 5 ++++-
 tests/overlay/035 | 2 +-
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/common/overlay b/common/overlay
index 01b6622f..0fad6e70 100644
--- a/common/overlay
+++ b/common/overlay
@@ -127,7 +127,10 @@ _overlay_base_scratch_mount()
 _overlay_scratch_mount()
 {
 	if echo "$*" | grep -q remount; then
-		$MOUNT_PROG $SCRATCH_MNT $*
+		# By default, libmount merges remount options with old mount options.
+		# overlayfs does not support re-configuring the same mount options.
+		# We workaround this problem with --options-mode ignore.
+		$MOUNT_PROG $SCRATCH_MNT --options-mode ignore $*
 		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

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

[Zorro Lang]

On Tue, Jun 03, 2025 at 12:07:40PM +0200, Amir Goldstein wrote:
> libmount >= v1.39 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.
> 
> We workaround this problem with --options-mode ignore.
> 
> Reported-by: André Almeida <andrealmeid@igalia.com>
> Suggested-by: Karel Zak <kzak@redhat.com>
> Link: https://lore.kernel.org/linux-fsdevel/20250521-ovl_ro-v1-1-2350b1493d94@igalia.com/
> Link: https://lore.kernel.org/fstests/CAJfpegtJ3SDKmC80B4AfWiC3JmtWdW2+78fRZVtsuhe-wSRPvg@mail.gmail.com/
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
> 
> Changes since v1 [1]:
> - Change workaround from LIBMOUNT_FORCE_MOUNT2 to --options-mode=ignore
> 
> [1] https://lore.kernel.org/fstests/20250526143500.1520660-1-amir73il@gmail.com/

I'm not sure if I understand clearly. Does overlay list are fixing this issue
on kernel side, then providing a workaround to fstests to avoid the issue be
triggered too?

Thanks,
Zorro

> 
>  common/overlay    | 5 ++++-
>  tests/overlay/035 | 2 +-
>  2 files changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/common/overlay b/common/overlay
> index 01b6622f..0fad6e70 100644
> --- a/common/overlay
> +++ b/common/overlay
> @@ -127,7 +127,10 @@ _overlay_base_scratch_mount()
>  _overlay_scratch_mount()
>  {
>  	if echo "$*" | grep -q remount; then
> -		$MOUNT_PROG $SCRATCH_MNT $*
> +		# By default, libmount merges remount options with old mount options.
> +		# overlayfs does not support re-configuring the same mount options.
> +		# We workaround this problem with --options-mode ignore.
> +		$MOUNT_PROG $SCRATCH_MNT --options-mode ignore $*
>  		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
> 

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

[Amir Goldstein]

On Thu, Jun 5, 2025 at 7:51 PM Zorro Lang <zlang@redhat.com> wrote:
>
> On Tue, Jun 03, 2025 at 12:07:40PM +0200, Amir Goldstein wrote:
> > libmount >= v1.39 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.
> >
> > We workaround this problem with --options-mode ignore.
> >
> > Reported-by: André Almeida <andrealmeid@igalia.com>
> > Suggested-by: Karel Zak <kzak@redhat.com>
> > Link: https://lore.kernel.org/linux-fsdevel/20250521-ovl_ro-v1-1-2350b1493d94@igalia.com/
> > Link: https://lore.kernel.org/fstests/CAJfpegtJ3SDKmC80B4AfWiC3JmtWdW2+78fRZVtsuhe-wSRPvg@mail.gmail.com/
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > ---
> >
> > Changes since v1 [1]:
> > - Change workaround from LIBMOUNT_FORCE_MOUNT2 to --options-mode=ignore
> >
> > [1] https://lore.kernel.org/fstests/20250526143500.1520660-1-amir73il@gmail.com/
>
> I'm not sure if I understand clearly. Does overlay list are fixing this issue
> on kernel side, then providing a workaround to fstests to avoid the issue be
> triggered too?
>

Noone agreed to fix it on the kernel side.
At least not yet.

Thanks,
Amir.

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

[Zorro Lang]

On Thu, Jun 05, 2025 at 08:30:53PM +0200, Amir Goldstein wrote:
> On Thu, Jun 5, 2025 at 7:51 PM Zorro Lang <zlang@redhat.com> wrote:
> >
> > On Tue, Jun 03, 2025 at 12:07:40PM +0200, Amir Goldstein wrote:
> > > libmount >= v1.39 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.
> > >
> > > We workaround this problem with --options-mode ignore.
> > >
> > > Reported-by: André Almeida <andrealmeid@igalia.com>
> > > Suggested-by: Karel Zak <kzak@redhat.com>
> > > Link: https://lore.kernel.org/linux-fsdevel/20250521-ovl_ro-v1-1-2350b1493d94@igalia.com/
> > > Link: https://lore.kernel.org/fstests/CAJfpegtJ3SDKmC80B4AfWiC3JmtWdW2+78fRZVtsuhe-wSRPvg@mail.gmail.com/
> > > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > > ---
> > >
> > > Changes since v1 [1]:
> > > - Change workaround from LIBMOUNT_FORCE_MOUNT2 to --options-mode=ignore
> > >
> > > [1] https://lore.kernel.org/fstests/20250526143500.1520660-1-amir73il@gmail.com/
> >
> > I'm not sure if I understand clearly. Does overlay list are fixing this issue
> > on kernel side, then providing a workaround to fstests to avoid the issue be
> > triggered too?
> >
> 
> Noone agreed to fix it on the kernel side.
> At least not yet.

If so, I have two questions:)
1) Will overlay fix it on kernel or mount util side?
2) Do you plan to keep this workaround until the issue be fixed in one day?
   Then revert this workaround?

Thanks,
Zorro

> 
> Thanks,
> Amir.
> 

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

[Amir Goldstein]

On Fri, Jun 6, 2025 at 3:12 AM Zorro Lang <zlang@redhat.com> wrote:
>
> On Thu, Jun 05, 2025 at 08:30:53PM +0200, Amir Goldstein wrote:
> > On Thu, Jun 5, 2025 at 7:51 PM Zorro Lang <zlang@redhat.com> wrote:
> > >
> > > On Tue, Jun 03, 2025 at 12:07:40PM +0200, Amir Goldstein wrote:
> > > > libmount >= v1.39 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.
> > > >
> > > > We workaround this problem with --options-mode ignore.
> > > >
> > > > Reported-by: André Almeida <andrealmeid@igalia.com>
> > > > Suggested-by: Karel Zak <kzak@redhat.com>
> > > > Link: https://lore.kernel.org/linux-fsdevel/20250521-ovl_ro-v1-1-2350b1493d94@igalia.com/
> > > > Link: https://lore.kernel.org/fstests/CAJfpegtJ3SDKmC80B4AfWiC3JmtWdW2+78fRZVtsuhe-wSRPvg@mail.gmail.com/
> > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > > > ---
> > > >
> > > > Changes since v1 [1]:
> > > > - Change workaround from LIBMOUNT_FORCE_MOUNT2 to --options-mode=ignore
> > > >
> > > > [1] https://lore.kernel.org/fstests/20250526143500.1520660-1-amir73il@gmail.com/
> > >
> > > I'm not sure if I understand clearly. Does overlay list are fixing this issue
> > > on kernel side, then providing a workaround to fstests to avoid the issue be
> > > triggered too?
> > >
> >
> > Noone agreed to fix it on the kernel side.
> > At least not yet.
>
> If so, I have two questions:)
> 1) Will overlay fix it on kernel or mount util side?

This is not known at this time.

> 2) Do you plan to keep this workaround until the issue be fixed in one day?
>    Then revert this workaround?

Maybe, but keep in mind that the workaround is simply
telling the library what we want to do.

We want to remount overlay ro and nothing else and that is exactly
what  "--options-mode ignore" tells the library to do.

I could have just as well written a test helper src/remount_rdonly.c
and not have to deal with the question of which libmount version
the test machine is using.

Note that the tests in question are not intended to test the remount,ro
functionality itself, they are intended to test the behavior of fs in
some scenarios involving a rdonly mount.

I do not want to lose important test coverage of these scenarios
because of regressions in the kernel/libmount API.

We can add a new test that ONLY tests remount,ro and let that
test fail on overlayfs to keep us reminded of the real regresion that
needs to be fixed, but the "workaround" or as I prefer to call it
"using the right tool for the test case" has to stay for those other tests.

Thanks,
Amir.

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

[Zorro Lang]

On Fri, Jun 06, 2025 at 09:35:36AM +0200, Amir Goldstein wrote:
> On Fri, Jun 6, 2025 at 3:12 AM Zorro Lang <zlang@redhat.com> wrote:
> >
> > On Thu, Jun 05, 2025 at 08:30:53PM +0200, Amir Goldstein wrote:
> > > On Thu, Jun 5, 2025 at 7:51 PM Zorro Lang <zlang@redhat.com> wrote:
> > > >
> > > > On Tue, Jun 03, 2025 at 12:07:40PM +0200, Amir Goldstein wrote:
> > > > > libmount >= v1.39 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.
> > > > >
> > > > > We workaround this problem with --options-mode ignore.
> > > > >
> > > > > Reported-by: André Almeida <andrealmeid@igalia.com>
> > > > > Suggested-by: Karel Zak <kzak@redhat.com>
> > > > > Link: https://lore.kernel.org/linux-fsdevel/20250521-ovl_ro-v1-1-2350b1493d94@igalia.com/
> > > > > Link: https://lore.kernel.org/fstests/CAJfpegtJ3SDKmC80B4AfWiC3JmtWdW2+78fRZVtsuhe-wSRPvg@mail.gmail.com/
> > > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > > > > ---
> > > > >
> > > > > Changes since v1 [1]:
> > > > > - Change workaround from LIBMOUNT_FORCE_MOUNT2 to --options-mode=ignore
> > > > >
> > > > > [1] https://lore.kernel.org/fstests/20250526143500.1520660-1-amir73il@gmail.com/
> > > >
> > > > I'm not sure if I understand clearly. Does overlay list are fixing this issue
> > > > on kernel side, then providing a workaround to fstests to avoid the issue be
> > > > triggered too?
> > > >
> > >
> > > Noone agreed to fix it on the kernel side.
> > > At least not yet.
> >
> > If so, I have two questions:)
> > 1) Will overlay fix it on kernel or mount util side?
> 
> This is not known at this time.

Oh, I thought it's getting fix :-D

> 
> > 2) Do you plan to keep this workaround until the issue be fixed in one day?
> >    Then revert this workaround?
> 
> Maybe, but keep in mind that the workaround is simply
> telling the library what we want to do.
> 
> We want to remount overlay ro and nothing else and that is exactly
> what  "--options-mode ignore" tells the library to do.
> 
> I could have just as well written a test helper src/remount_rdonly.c
> and not have to deal with the question of which libmount version
> the test machine is using.
> 
> Note that the tests in question are not intended to test the remount,ro
> functionality itself, they are intended to test the behavior of fs in
> some scenarios involving a rdonly mount.
> 
> I do not want to lose important test coverage of these scenarios
> because of regressions in the kernel/libmount API.
> 
> We can add a new test that ONLY tests remount,ro and let that
> test fail on overlayfs to keep us reminded of the real regresion that
> needs to be fixed, but the "workaround" or as I prefer to call it
> "using the right tool for the test case" has to stay for those other tests.

OK, I just tried to figure out if "hide this error output on new mount APIs"
is what overlay list wants. If overlay list (or vfs) acks this patch, and
will track this issue. I'm good to merge this workaround for testing :)

Thanks,
Zorro

> 
> Thanks,
> Amir.
> 

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

[Amir Goldstein]

On Fri, Jun 6, 2025 at 12:35 PM Zorro Lang <zlang@redhat.com> wrote:
>
> On Fri, Jun 06, 2025 at 09:35:36AM +0200, Amir Goldstein wrote:
> > On Fri, Jun 6, 2025 at 3:12 AM Zorro Lang <zlang@redhat.com> wrote:
> > >
> > > On Thu, Jun 05, 2025 at 08:30:53PM +0200, Amir Goldstein wrote:
> > > > On Thu, Jun 5, 2025 at 7:51 PM Zorro Lang <zlang@redhat.com> wrote:
> > > > >
> > > > > On Tue, Jun 03, 2025 at 12:07:40PM +0200, Amir Goldstein wrote:
> > > > > > libmount >= v1.39 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.
> > > > > >
> > > > > > We workaround this problem with --options-mode ignore.
> > > > > >
> > > > > > Reported-by: André Almeida <andrealmeid@igalia.com>
> > > > > > Suggested-by: Karel Zak <kzak@redhat.com>
> > > > > > Link: https://lore.kernel.org/linux-fsdevel/20250521-ovl_ro-v1-1-2350b1493d94@igalia.com/
> > > > > > Link: https://lore.kernel.org/fstests/CAJfpegtJ3SDKmC80B4AfWiC3JmtWdW2+78fRZVtsuhe-wSRPvg@mail.gmail.com/
> > > > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > > > > > ---
> > > > > >
> > > > > > Changes since v1 [1]:
> > > > > > - Change workaround from LIBMOUNT_FORCE_MOUNT2 to --options-mode=ignore
> > > > > >
> > > > > > [1] https://lore.kernel.org/fstests/20250526143500.1520660-1-amir73il@gmail.com/
> > > > >
> > > > > I'm not sure if I understand clearly. Does overlay list are fixing this issue
> > > > > on kernel side, then providing a workaround to fstests to avoid the issue be
> > > > > triggered too?
> > > > >
> > > >
> > > > Noone agreed to fix it on the kernel side.
> > > > At least not yet.
> > >
> > > If so, I have two questions:)
> > > 1) Will overlay fix it on kernel or mount util side?
> >
> > This is not known at this time.
>
> Oh, I thought it's getting fix :-D
>
> >
> > > 2) Do you plan to keep this workaround until the issue be fixed in one day?
> > >    Then revert this workaround?
> >
> > Maybe, but keep in mind that the workaround is simply
> > telling the library what we want to do.
> >
> > We want to remount overlay ro and nothing else and that is exactly
> > what  "--options-mode ignore" tells the library to do.
> >
> > I could have just as well written a test helper src/remount_rdonly.c
> > and not have to deal with the question of which libmount version
> > the test machine is using.
> >
> > Note that the tests in question are not intended to test the remount,ro
> > functionality itself, they are intended to test the behavior of fs in
> > some scenarios involving a rdonly mount.
> >
> > I do not want to lose important test coverage of these scenarios
> > because of regressions in the kernel/libmount API.
> >
> > We can add a new test that ONLY tests remount,ro and let that
> > test fail on overlayfs to keep us reminded of the real regresion that
> > needs to be fixed, but the "workaround" or as I prefer to call it
> > "using the right tool for the test case" has to stay for those other tests.
>
> OK, I just tried to figure out if "hide this error output on new mount APIs"
> is what overlay list wants. If overlay list (or vfs) acks this patch, and
> will track this issue. I'm good to merge this workaround for testing :)
>

This workaround in v2 was suggested by libmount maintainer
and approved by overlayfs maintainer:

"> 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,
Amir.

[1] https://lore.kernel.org/linux-unionfs/CAJfpegtJ3SDKmC80B4AfWiC3JmtWdW2+78fRZVtsuhe-wSRPvg@mail.gmail.com/

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

[Zorro Lang]

On Fri, Jun 06, 2025 at 12:58:24PM +0200, Amir Goldstein wrote:
> On Fri, Jun 6, 2025 at 12:35 PM Zorro Lang <zlang@redhat.com> wrote:
> >
> > On Fri, Jun 06, 2025 at 09:35:36AM +0200, Amir Goldstein wrote:
> > > On Fri, Jun 6, 2025 at 3:12 AM Zorro Lang <zlang@redhat.com> wrote:
> > > >
> > > > On Thu, Jun 05, 2025 at 08:30:53PM +0200, Amir Goldstein wrote:
> > > > > On Thu, Jun 5, 2025 at 7:51 PM Zorro Lang <zlang@redhat.com> wrote:
> > > > > >
> > > > > > On Tue, Jun 03, 2025 at 12:07:40PM +0200, Amir Goldstein wrote:
> > > > > > > libmount >= v1.39 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.
> > > > > > >
> > > > > > > We workaround this problem with --options-mode ignore.
> > > > > > >
> > > > > > > Reported-by: André Almeida <andrealmeid@igalia.com>
> > > > > > > Suggested-by: Karel Zak <kzak@redhat.com>
> > > > > > > Link: https://lore.kernel.org/linux-fsdevel/20250521-ovl_ro-v1-1-2350b1493d94@igalia.com/
> > > > > > > Link: https://lore.kernel.org/fstests/CAJfpegtJ3SDKmC80B4AfWiC3JmtWdW2+78fRZVtsuhe-wSRPvg@mail.gmail.com/
> > > > > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > > > > > > ---
> > > > > > >
> > > > > > > Changes since v1 [1]:
> > > > > > > - Change workaround from LIBMOUNT_FORCE_MOUNT2 to --options-mode=ignore
> > > > > > >
> > > > > > > [1] https://lore.kernel.org/fstests/20250526143500.1520660-1-amir73il@gmail.com/
> > > > > >
> > > > > > I'm not sure if I understand clearly. Does overlay list are fixing this issue
> > > > > > on kernel side, then providing a workaround to fstests to avoid the issue be
> > > > > > triggered too?
> > > > > >
> > > > >
> > > > > Noone agreed to fix it on the kernel side.
> > > > > At least not yet.
> > > >
> > > > If so, I have two questions:)
> > > > 1) Will overlay fix it on kernel or mount util side?
> > >
> > > This is not known at this time.
> >
> > Oh, I thought it's getting fix :-D
> >
> > >
> > > > 2) Do you plan to keep this workaround until the issue be fixed in one day?
> > > >    Then revert this workaround?
> > >
> > > Maybe, but keep in mind that the workaround is simply
> > > telling the library what we want to do.
> > >
> > > We want to remount overlay ro and nothing else and that is exactly
> > > what  "--options-mode ignore" tells the library to do.
> > >
> > > I could have just as well written a test helper src/remount_rdonly.c
> > > and not have to deal with the question of which libmount version
> > > the test machine is using.
> > >
> > > Note that the tests in question are not intended to test the remount,ro
> > > functionality itself, they are intended to test the behavior of fs in
> > > some scenarios involving a rdonly mount.
> > >
> > > I do not want to lose important test coverage of these scenarios
> > > because of regressions in the kernel/libmount API.
> > >
> > > We can add a new test that ONLY tests remount,ro and let that
> > > test fail on overlayfs to keep us reminded of the real regresion that
> > > needs to be fixed, but the "workaround" or as I prefer to call it
> > > "using the right tool for the test case" has to stay for those other tests.
> >
> > OK, I just tried to figure out if "hide this error output on new mount APIs"
> > is what overlay list wants. If overlay list (or vfs) acks this patch, and
> > will track this issue. I'm good to merge this workaround for testing :)
> >
> 
> This workaround in v2 was suggested by libmount maintainer
> and approved by overlayfs maintainer:
> 
> "> 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."

OK, just to make sure overlay/vfs folks know why these failures gone :)

Reviewed-by: Zorro Lang <zlang@redhat.com>

With this patch, I've merged 5 patches of this patchset. Now only 4/6
still need more reviewing (I've provided my suggestion about that). If
patch 4/6 can't catch up the release of this week, don't worry, I'll
push these 5 patches at first, feel free to send patch 4/6 singly later :)

Thanks,
Zorro

> 
> Thanks,
> Amir.
> 
> [1] https://lore.kernel.org/linux-unionfs/CAJfpegtJ3SDKmC80B4AfWiC3JmtWdW2+78fRZVtsuhe-wSRPvg@mail.gmail.com/
> 

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

[André Almeida]

Hi Amir,

On 6/6/25 09:35, Amir Goldstein wrote:
> On Fri, Jun 6, 2025 at 3:12 AM Zorro Lang <zlang@redhat.com> wrote:
>> On Thu, Jun 05, 2025 at 08:30:53PM +0200, Amir Goldstein wrote:
>>> On Thu, Jun 5, 2025 at 7:51 PM Zorro Lang <zlang@redhat.com> wrote:
>>>> On Tue, Jun 03, 2025 at 12:07:40PM +0200, Amir Goldstein wrote:
>>>>> libmount >= v1.39 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.
>>>>>
>>>>> We workaround this problem with --options-mode ignore.
>>>>>
>>>>> Reported-by: André Almeida <andrealmeid@igalia.com>
>>>>> Suggested-by: Karel Zak <kzak@redhat.com>
>>>>> Link: https://lore.kernel.org/linux-fsdevel/20250521-ovl_ro-v1-1-2350b1493d94@igalia.com/
>>>>> Link: https://lore.kernel.org/fstests/CAJfpegtJ3SDKmC80B4AfWiC3JmtWdW2+78fRZVtsuhe-wSRPvg@mail.gmail.com/
>>>>> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
>>>>> ---
>>>>>
>>>>> Changes since v1 [1]:
>>>>> - Change workaround from LIBMOUNT_FORCE_MOUNT2 to --options-mode=ignore
>>>>>
>>>>> [1] https://lore.kernel.org/fstests/20250526143500.1520660-1-amir73il@gmail.com/
>>>> I'm not sure if I understand clearly. Does overlay list are fixing this issue
>>>> on kernel side, then providing a workaround to fstests to avoid the issue be
>>>> triggered too?
>>>>
>>> Noone agreed to fix it on the kernel side.
>>> At least not yet.
>> If so, I have two questions:)
>> 1) Will overlay fix it on kernel or mount util side?
> This is not known at this time.

Do you know how calling fsconfig() in a "redundant" way behaves in other 
filesystems? Do they allow to call fsconfig() calls that doesn't change 
the state, like a noop?

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

[Amir Goldstein]

On Fri, Jun 6, 2025 at 4:23 PM André Almeida <andrealmeid@igalia.com> wrote:
>
> Hi Amir,
>
> On 6/6/25 09:35, Amir Goldstein wrote:
> > On Fri, Jun 6, 2025 at 3:12 AM Zorro Lang <zlang@redhat.com> wrote:
> >> On Thu, Jun 05, 2025 at 08:30:53PM +0200, Amir Goldstein wrote:
> >>> On Thu, Jun 5, 2025 at 7:51 PM Zorro Lang <zlang@redhat.com> wrote:
> >>>> On Tue, Jun 03, 2025 at 12:07:40PM +0200, Amir Goldstein wrote:
> >>>>> libmount >= v1.39 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.
> >>>>>
> >>>>> We workaround this problem with --options-mode ignore.
> >>>>>
> >>>>> Reported-by: André Almeida <andrealmeid@igalia.com>
> >>>>> Suggested-by: Karel Zak <kzak@redhat.com>
> >>>>> Link: https://lore.kernel.org/linux-fsdevel/20250521-ovl_ro-v1-1-2350b1493d94@igalia.com/
> >>>>> Link: https://lore.kernel.org/fstests/CAJfpegtJ3SDKmC80B4AfWiC3JmtWdW2+78fRZVtsuhe-wSRPvg@mail.gmail.com/
> >>>>> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> >>>>> ---
> >>>>>
> >>>>> Changes since v1 [1]:
> >>>>> - Change workaround from LIBMOUNT_FORCE_MOUNT2 to --options-mode=ignore
> >>>>>
> >>>>> [1] https://lore.kernel.org/fstests/20250526143500.1520660-1-amir73il@gmail.com/
> >>>> I'm not sure if I understand clearly. Does overlay list are fixing this issue
> >>>> on kernel side, then providing a workaround to fstests to avoid the issue be
> >>>> triggered too?
> >>>>
> >>> Noone agreed to fix it on the kernel side.
> >>> At least not yet.
> >> If so, I have two questions:)
> >> 1) Will overlay fix it on kernel or mount util side?
> > This is not known at this time.
>
> Do you know how calling fsconfig() in a "redundant" way behaves in other
> filesystems? Do they allow to call fsconfig() calls that doesn't change
> the state, like a noop?
>

I don't know. didn't do a survey.
"We did this until now"
Is not a good enough reason to keep doing the same workarounds
in the kernel IMO.

Thanks,
Amir.

[PATCH v2 2/6] 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")
Tested-by: André Almeida <andrealmeid@igalia.com>
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 0fad6e70..0be943b1 100644
--- a/common/overlay
+++ b/common/overlay
@@ -434,6 +434,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

Re: [PATCH v2 2/6] overlay: fix regression in _repair_overlay_scratch_fs

[Zorro Lang]

On Tue, Jun 03, 2025 at 12:07:41PM +0200, Amir Goldstein wrote:
> _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")
> Tested-by: André Almeida <andrealmeid@igalia.com>
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---

Thanks for this regression fix from overlay.

Reviewed-by: Zorro Lang <zlang@redhat.com>

>  common/overlay    | 2 ++
>  tests/generic/330 | 2 +-
>  2 files changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/common/overlay b/common/overlay
> index 0fad6e70..0be943b1 100644
> --- a/common/overlay
> +++ b/common/overlay
> @@ -434,6 +434,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 v2 3/6] 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.

Tested-by: André Almeida <andrealmeid@igalia.com>
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

Re: [PATCH v2 3/6] generic/604: do not run with overlayfs

[Zorro Lang]

On Tue, Jun 03, 2025 at 12:07:42PM +0200, Amir Goldstein wrote:
> 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.
> 
> Tested-by: André Almeida <andrealmeid@igalia.com>
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---

Good to me,

Reviewed-by: Zorro Lang <zlang@redhat.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 v2 4/6] 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>
Tested-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 v2 4/6] generic/623: do not run with overlayfs

[Zorro Lang]

On Tue, Jun 03, 2025 at 12:07:43PM +0200, Amir Goldstein wrote:
> 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>
> Tested-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
> +		;;

Hmm... I'm not sure this's a good way.
For example, overlay/087 does xfs_io shutdown too, generally it should calls
_require_xfs_io_command "shutdown" although it doesn't. If someone overlay
test case hope to test as o/087 does, and it calls _require_xfs_io_command "shutdown",
then it'll be _notrun.

If g/623 is not suitable for overlay, how about skip it for overlay clearly, by
`_exclude_fs overlay` ?

Thanks,
Zorro

>  	*)
>  		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 v2 4/6] generic/623: do not run with overlayfs

[Amir Goldstein]

On Thu, Jun 5, 2025 at 7:32 PM Zorro Lang <zlang@redhat.com> wrote:
>
> On Tue, Jun 03, 2025 at 12:07:43PM +0200, Amir Goldstein wrote:
> > 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>
> > Tested-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
> > +             ;;
>
> Hmm... I'm not sure this's a good way.
> For example, overlay/087 does xfs_io shutdown too,

Yes it does but look at the effort needed to do that properly:

$XFS_IO_PROG -x -c "open $(_scratch_shutdown_handle)" -c 'shutdown -f
' -c close -c syncfs $SCRATCH_MNT | \
        grep -vF '[00'

> generally it should calls
> _require_xfs_io_command "shutdown" although it doesn't. If someone overlay
> test case hope to test as o/087 does, and it calls _require_xfs_io_command "shutdown",
> then it'll be _notrun.

If someone knows enough to perform the dance above with _scratch_shutdown_handle
then that someone should know enough not to call
_require_xfs_io_command "shutdown".
OTOH, if someone doesn't know then default is to not run.

>
> If g/623 is not suitable for overlay, how about skip it for overlay clearly, by
> `_exclude_fs overlay` ?
>

I do not personally mind doing this _exclude_fs overlay, but it is usually
prefered to require what the test needs.

Whatever you prefer is fine by me.

Thanks,
Amir.

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

[Zorro Lang]

On Thu, Jun 05, 2025 at 08:38:30PM +0200, Amir Goldstein wrote:
> On Thu, Jun 5, 2025 at 7:32 PM Zorro Lang <zlang@redhat.com> wrote:
> >
> > On Tue, Jun 03, 2025 at 12:07:43PM +0200, Amir Goldstein wrote:
> > > 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>
> > > Tested-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
> > > +             ;;
> >
> > Hmm... I'm not sure this's a good way.
> > For example, overlay/087 does xfs_io shutdown too,
> 
> Yes it does but look at the effort needed to do that properly:
> 
> $XFS_IO_PROG -x -c "open $(_scratch_shutdown_handle)" -c 'shutdown -f
> ' -c close -c syncfs $SCRATCH_MNT | \
>         grep -vF '[00'
> 
> > generally it should calls
> > _require_xfs_io_command "shutdown" although it doesn't. If someone overlay
> > test case hope to test as o/087 does, and it calls _require_xfs_io_command "shutdown",
> > then it'll be _notrun.
> 
> If someone knows enough to perform the dance above with _scratch_shutdown_handle
> then that someone should know enough not to call
> _require_xfs_io_command "shutdown".
> OTOH, if someone doesn't know then default is to not run.

Sure, I can understand that, just this logic is a bit *obscure* :) It sounds like:
"If an overlay test case wants to do xfs_io shutdown, it shouldn't call
_require_xfs_io_command "shutdown". Or call that to skip a shutdown test
on overlay :)"

And the expected result of _require_xfs_io_command "shutdown" will be totally
opposite with _require_scratch_shutdown on overlay, that might be confused.
Can we have a clearer way to deal with that?

> 
> >
> > If g/623 is not suitable for overlay, how about skip it for overlay clearly, by
> > `_exclude_fs overlay` ?
> >
> 
> I do not personally mind doing this _exclude_fs overlay, but it is usually
> prefered to require what the test needs.
> 
> Whatever you prefer is fine by me.

I don't perfer "_exclude_fs overlay", just before we have a clear way to deal with
overlay shutdown, this might be simple and forthright :)

Thanks,
Zorro

> 
> Thanks,
> Amir.
> 

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

[Amir Goldstein]

[-- Attachment #1: Type: text/plain, Size: 4034 bytes --]

On Fri, Jun 6, 2025 at 3:45 AM Zorro Lang <zlang@redhat.com> wrote:
>
> On Thu, Jun 05, 2025 at 08:38:30PM +0200, Amir Goldstein wrote:
> > On Thu, Jun 5, 2025 at 7:32 PM Zorro Lang <zlang@redhat.com> wrote:
> > >
> > > On Tue, Jun 03, 2025 at 12:07:43PM +0200, Amir Goldstein wrote:
> > > > 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>
> > > > Tested-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
> > > > +             ;;
> > >
> > > Hmm... I'm not sure this's a good way.
> > > For example, overlay/087 does xfs_io shutdown too,
> >
> > Yes it does but look at the effort needed to do that properly:
> >
> > $XFS_IO_PROG -x -c "open $(_scratch_shutdown_handle)" -c 'shutdown -f
> > ' -c close -c syncfs $SCRATCH_MNT | \
> >         grep -vF '[00'
> >
> > > generally it should calls
> > > _require_xfs_io_command "shutdown" although it doesn't. If someone overlay
> > > test case hope to test as o/087 does, and it calls _require_xfs_io_command "shutdown",
> > > then it'll be _notrun.
> >
> > If someone knows enough to perform the dance above with _scratch_shutdown_handle
> > then that someone should know enough not to call
> > _require_xfs_io_command "shutdown".
> > OTOH, if someone doesn't know then default is to not run.
>
> Sure, I can understand that, just this logic is a bit *obscure* :) It sounds like:
> "If an overlay test case wants to do xfs_io shutdown, it shouldn't call
> _require_xfs_io_command "shutdown". Or call that to skip a shutdown test
> on overlay :)"
>
> And the expected result of _require_xfs_io_command "shutdown" will be totally
> opposite with _require_scratch_shutdown on overlay, that might be confused.
> Can we have a clearer way to deal with that?
>

I don't really understand the confusion.

_require_xfs_io_command "shutdown"

Like any other _require statement
requires support for what this test does -
meaning that a test does xfs_io -c shutdown, just like test generic/623 does

and _require_scratch_shutdown implies that the test does
_scratch_shutdown

FSTYP overlay happens to be able to do _scratch_shutdown
but not able to do xfs_io -c shutdown $SCRATCH_MNT

The different _require statements simply reflect reality as it is.

We can solve the confused about o/087 not having
_require_xfs_io_command "shutdown"
by moving the special hand crafted xfs_io command in o/087
to a helper _scratch_shutdown_and_syncfs to hide those internal
implementation details from test writers.
See attached patch.

Thanks,
Amir.

[-- Attachment #2: 0001-fstests-add-helper-_scratch_shutdown_and_syncfs.patch --]
[-- Type: text/x-patch, Size: 4181 bytes --]

From 3b4d6ff3ce909fa4b385a8d075bbac1f32b82cb8 Mon Sep 17 00:00:00 2001
From: Amir Goldstein <amir73il@gmail.com>
Date: Fri, 6 Jun 2025 09:11:17 +0200
Subject: [PATCH] fstests: add helper _scratch_shutdown_and_syncfs

Test xfs/546 has to chain syncfs after shutdown and cannot
use the _scratch_shitdown helper, because after shutdown a fd
cannot be opened to execute syncfs on.

The xfs_io command of chaining syncfs after shutdown is rather
more complex to execute in the derived overlayfs test overlay/087.

Add a helper to abstract this complexity from test writers.
Add a _require statement to match.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 common/rc         | 27 +++++++++++++++++++++++++++
 tests/overlay/087 | 13 +++----------
 tests/xfs/546     |  5 ++---
 3 files changed, 32 insertions(+), 13 deletions(-)

diff --git a/common/rc b/common/rc
index 96d65d1c..77683b82 100644
--- a/common/rc
+++ b/common/rc
@@ -595,6 +595,27 @@ _scratch_shutdown_handle()
 	fi
 }
 
+_scratch_shutdown_and_syncfs()
+{
+	if [ $FSTYP = "overlay" ]; then
+		# In lagacy overlay usage, it may specify directory as
+		# SCRATCH_DEV, in this case OVL_BASE_SCRATCH_DEV
+		# will be null, so check OVL_BASE_SCRATCH_DEV before
+		# running shutdown to avoid shutting down base fs accidently.
+		if [ -z $OVL_BASE_SCRATCH_DEV ]; then
+			_fail "_scratch_shutdown: call _require_scratch_shutdown first in test"
+		fi
+		# This command is complicated a bit because in the case of overlayfs the
+		# syncfs fd needs to be opened before shutdown and it is different from the
+		# shutdown fd, so we cannot use the _scratch_shutdown() helper.
+		# Filter out xfs_io output of active fds.
+		$XFS_IO_PROG -x -c "open $(_scratch_shutdown_handle)" -c 'shutdown -f ' \
+				-c close -c syncfs $SCRATCH_MNT | grep -vF '[00'
+	else
+		$XFS_IO_PROG -x -c 'shutdown -f ' -c syncfs $SCRATCH_MNT
+	fi
+}
+
 _move_mount()
 {
 	local mnt=$1
@@ -4110,6 +4131,12 @@ _require_scratch_shutdown()
 	_scratch_unmount
 }
 
+_require_scratch_shutdown_and_syncfs()
+{
+	_require_xfs_io_command syncfs
+	_require_scratch_shutdown
+}
+
 _check_s_dax()
 {
 	local target=$1
diff --git a/tests/overlay/087 b/tests/overlay/087
index a5afb0d5..2ad069db 100755
--- a/tests/overlay/087
+++ b/tests/overlay/087
@@ -32,9 +32,8 @@ _begin_fstest auto quick mount shutdown
 
 
 # Modify as appropriate.
-_require_xfs_io_command syncfs
 _require_scratch_nocheck
-_require_scratch_shutdown
+_require_scratch_shutdown_and_syncfs
 
 [ "$OVL_BASE_FSTYP" == "xfs" ] || \
 	_notrun "base fs $OVL_BASE_FSTYP has unknown behavior with syncfs after shutdown"
@@ -43,19 +42,13 @@ _require_scratch_shutdown
 # bother checking the filesystem afterwards since we never wrote anything.
 echo "=== syncfs after shutdown"
 _scratch_mount
-# This command is complicated a bit because in the case of overlayfs the
-# syncfs fd needs to be opened before shutdown and it is different from the
-# shutdown fd, so we cannot use the _scratch_shutdown() helper.
-# Filter out xfs_io output of active fds.
-$XFS_IO_PROG -x -c "open $(_scratch_shutdown_handle)" -c 'shutdown -f ' -c close -c syncfs $SCRATCH_MNT | \
-	grep -vF '[00'
+_scratch_shutdown_and_syncfs
 
 # Now repeat the same test with a volatile overlayfs mount and expect no error
 _scratch_unmount
 echo "=== syncfs after shutdown (volatile)"
 _scratch_mount -o volatile
-$XFS_IO_PROG -x -c "open $(_scratch_shutdown_handle)" -c 'shutdown -f ' -c close -c syncfs $SCRATCH_MNT | \
-	grep -vF '[00'
+_scratch_shutdown_and_syncfs
 
 # success, all done
 status=0
diff --git a/tests/xfs/546 b/tests/xfs/546
index 316ffc50..c50d41a6 100755
--- a/tests/xfs/546
+++ b/tests/xfs/546
@@ -27,14 +27,13 @@ _begin_fstest auto quick shutdown
 
 
 # Modify as appropriate.
-_require_xfs_io_command syncfs
 _require_scratch_nocheck
-_require_scratch_shutdown
+_require_scratch_shutdown_and_syncfs
 
 # Reuse the fs formatted when we checked for the shutdown ioctl, and don't
 # bother checking the filesystem afterwards since we never wrote anything.
 _scratch_mount
-$XFS_IO_PROG -x -c 'shutdown -f ' -c syncfs $SCRATCH_MNT
+_scratch_shutdown_and_syncfs
 
 # success, all done
 status=0
-- 
2.34.1

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

[Zorro Lang]

On Fri, Jun 06, 2025 at 09:23:26AM +0200, Amir Goldstein wrote:
> On Fri, Jun 6, 2025 at 3:45 AM Zorro Lang <zlang@redhat.com> wrote:
> >
> > On Thu, Jun 05, 2025 at 08:38:30PM +0200, Amir Goldstein wrote:
> > > On Thu, Jun 5, 2025 at 7:32 PM Zorro Lang <zlang@redhat.com> wrote:
> > > >
> > > > On Tue, Jun 03, 2025 at 12:07:43PM +0200, Amir Goldstein wrote:
> > > > > 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>
> > > > > Tested-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
> > > > > +             ;;
> > > >
> > > > Hmm... I'm not sure this's a good way.
> > > > For example, overlay/087 does xfs_io shutdown too,
> > >
> > > Yes it does but look at the effort needed to do that properly:
> > >
> > > $XFS_IO_PROG -x -c "open $(_scratch_shutdown_handle)" -c 'shutdown -f
> > > ' -c close -c syncfs $SCRATCH_MNT | \
> > >         grep -vF '[00'
> > >
> > > > generally it should calls
> > > > _require_xfs_io_command "shutdown" although it doesn't. If someone overlay
> > > > test case hope to test as o/087 does, and it calls _require_xfs_io_command "shutdown",
> > > > then it'll be _notrun.
> > >
> > > If someone knows enough to perform the dance above with _scratch_shutdown_handle
> > > then that someone should know enough not to call
> > > _require_xfs_io_command "shutdown".
> > > OTOH, if someone doesn't know then default is to not run.
> >
> > Sure, I can understand that, just this logic is a bit *obscure* :) It sounds like:
> > "If an overlay test case wants to do xfs_io shutdown, it shouldn't call
> > _require_xfs_io_command "shutdown". Or call that to skip a shutdown test
> > on overlay :)"
> >
> > And the expected result of _require_xfs_io_command "shutdown" will be totally
> > opposite with _require_scratch_shutdown on overlay, that might be confused.
> > Can we have a clearer way to deal with that?
> >
> 
> I don't really understand the confusion.
> 
> _require_xfs_io_command "shutdown"
> 
> Like any other _require statement
> requires support for what this test does -
> meaning that a test does xfs_io -c shutdown, just like test generic/623 does
> 
> and _require_scratch_shutdown implies that the test does
> _scratch_shutdown
> 
> FSTYP overlay happens to be able to do _scratch_shutdown
> but not able to do xfs_io -c shutdown $SCRATCH_MNT
> 
> The different _require statements simply reflect reality as it is.
> 
> We can solve the confused about o/087 not having
> _require_xfs_io_command "shutdown"
> by moving the special hand crafted xfs_io command in o/087
> to a helper _scratch_shutdown_and_syncfs to hide those internal
> implementation details from test writers.
> See attached patch.

Hmm... give me a moment to order my thoughts step by step :)

There're only 2 cases tend to do xfs_io shutdown on overlay currently
(others are xfs specific test cases):

  $ grep -rsn shutdown tests/|grep -- "-c"
  tests/generic/623:29:$XFS_IO_PROG -x -c "mmap 0 4k" -c "mwrite 0 4k" -c shutdown -c fsync \
  tests/overlay/087:50:$XFS_IO_PROG -x -c "open $(_scratch_shutdown_handle)" -c 'shutdown -f ' -c close -c syncfs $SCRATCH_MNT | \
  tests/overlay/087:57:$XFS_IO_PROG -x -c "open $(_scratch_shutdown_handle)" -c 'shutdown -f ' -c close -c syncfs $SCRATCH_MNT | \
  ...

others shutdown cases nearly all use *_scratch_shutdown* with
*_require_scratch_shutdown*, these two functions are consistent in
code logic. And no one calls "_require_xfs_io_command shutdown" currently.

So g/623 and o/087 are specifal, actually they call _require_scratch_shutdown
too, that makes sense for o/087. Now only g/623 doesn't make sense. Now we
need to help it to make sense.

I think the key is in _require_scratch_shutdown function [1], how about add an
argument to clearly tell it we need to check shutdown "only on the top layer
$SCRATCH_MNT" or "try the lowest layer $BASE_SCRATCH_MNT if there is".

For example:

diff --git a/common/rc b/common/rc
index c3af8485c..5f30143e4 100644
--- a/common/rc
+++ b/common/rc
@@ -4075,15 +4075,17 @@ _require_exportfs()
        _require_open_by_handle
 }
 
-# Does shutdown work on this fs?
+# Does shutdown work on this [lower|top] layer fs?
 _require_scratch_shutdown()
 {
+       local layer="${1:-lower}"
+
        [ -x $here/src/godown ] || _notrun "src/godown executable not found"
 
        _scratch_mkfs > /dev/null 2>&1 || _notrun "_scratch_mkfs failed on $SCRATCH_DEV"
        _scratch_mount
 
-       if [ $FSTYP = "overlay" ]; then
+       if [ $FSTYP = "overlay" -a "$level" = "lower" ]; then
                if [ -z $OVL_BASE_SCRATCH_DEV ]; then
                        # In lagacy overlay usage, it may specify directory as
                        # SCRATCH_DEV, in this case OVL_BASE_SCRATCH_DEV
diff --git a/tests/generic/623 b/tests/generic/623
index b97e2adbe..af0f55397 100755
--- a/tests/generic/623
+++ b/tests/generic/623
@@ -15,7 +15,7 @@ _begin_fstest auto quick shutdown mmap
        "xfs: restore shutdown check in mapped write fault path"
 
 _require_scratch_nocheck
-_require_scratch_shutdown
+_require_scratch_shutdown top
 
 _scratch_mkfs &>> $seqres.full
 _scratch_mount


Thanks,
Zorro

[1]
_require_scratch_shutdown()
{
        [ -x $here/src/godown ] || _notrun "src/godown executable not found"

        _scratch_mkfs > /dev/null 2>&1 || _notrun "_scratch_mkfs failed on $SCRATCH_DEV"
        _scratch_mount

        if [ $FSTYP = "overlay" ]; then
                if [ -z $OVL_BASE_SCRATCH_DEV ]; then
                        # In lagacy overlay usage, it may specify directory as
                        # SCRATCH_DEV, in this case OVL_BASE_SCRATCH_DEV
                        # will be null, so check OVL_BASE_SCRATCH_DEV before
                        # running shutdown to avoid shutting down base fs accidently.
                        _notrun "This test requires a valid $OVL_BASE_SCRATCH_DEV as ovl base fs"
                else
                        $here/src/godown -f $OVL_BASE_SCRATCH_MNT 2>&1 \
                        || _notrun "Underlying filesystem does not support shutdown"
                fi
        else
                $here/src/godown -f $SCRATCH_MNT 2>&1 \
                        || _notrun "$FSTYP does not support shutdown"
        fi

        _scratch_unmount
}


> 
> Thanks,
> Amir.

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

[Amir Goldstein]

On Fri, Jun 6, 2025 at 12:29 PM Zorro Lang <zlang@redhat.com> wrote:
>
> On Fri, Jun 06, 2025 at 09:23:26AM +0200, Amir Goldstein wrote:
> > On Fri, Jun 6, 2025 at 3:45 AM Zorro Lang <zlang@redhat.com> wrote:
> > >
> > > On Thu, Jun 05, 2025 at 08:38:30PM +0200, Amir Goldstein wrote:
> > > > On Thu, Jun 5, 2025 at 7:32 PM Zorro Lang <zlang@redhat.com> wrote:
> > > > >
> > > > > On Tue, Jun 03, 2025 at 12:07:43PM +0200, Amir Goldstein wrote:
> > > > > > 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>
> > > > > > Tested-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
> > > > > > +             ;;
> > > > >
> > > > > Hmm... I'm not sure this's a good way.
> > > > > For example, overlay/087 does xfs_io shutdown too,
> > > >
> > > > Yes it does but look at the effort needed to do that properly:
> > > >
> > > > $XFS_IO_PROG -x -c "open $(_scratch_shutdown_handle)" -c 'shutdown -f
> > > > ' -c close -c syncfs $SCRATCH_MNT | \
> > > >         grep -vF '[00'
> > > >
> > > > > generally it should calls
> > > > > _require_xfs_io_command "shutdown" although it doesn't. If someone overlay
> > > > > test case hope to test as o/087 does, and it calls _require_xfs_io_command "shutdown",
> > > > > then it'll be _notrun.
> > > >
> > > > If someone knows enough to perform the dance above with _scratch_shutdown_handle
> > > > then that someone should know enough not to call
> > > > _require_xfs_io_command "shutdown".
> > > > OTOH, if someone doesn't know then default is to not run.
> > >
> > > Sure, I can understand that, just this logic is a bit *obscure* :) It sounds like:
> > > "If an overlay test case wants to do xfs_io shutdown, it shouldn't call
> > > _require_xfs_io_command "shutdown". Or call that to skip a shutdown test
> > > on overlay :)"
> > >
> > > And the expected result of _require_xfs_io_command "shutdown" will be totally
> > > opposite with _require_scratch_shutdown on overlay, that might be confused.
> > > Can we have a clearer way to deal with that?
> > >
> >
> > I don't really understand the confusion.
> >
> > _require_xfs_io_command "shutdown"
> >
> > Like any other _require statement
> > requires support for what this test does -
> > meaning that a test does xfs_io -c shutdown, just like test generic/623 does
> >
> > and _require_scratch_shutdown implies that the test does
> > _scratch_shutdown
> >
> > FSTYP overlay happens to be able to do _scratch_shutdown
> > but not able to do xfs_io -c shutdown $SCRATCH_MNT
> >
> > The different _require statements simply reflect reality as it is.
> >
> > We can solve the confused about o/087 not having
> > _require_xfs_io_command "shutdown"
> > by moving the special hand crafted xfs_io command in o/087
> > to a helper _scratch_shutdown_and_syncfs to hide those internal
> > implementation details from test writers.
> > See attached patch.
>
> Hmm... give me a moment to order my thoughts step by step :)
>
> There're only 2 cases tend to do xfs_io shutdown on overlay currently
> (others are xfs specific test cases):
>
>   $ grep -rsn shutdown tests/|grep -- "-c"
>   tests/generic/623:29:$XFS_IO_PROG -x -c "mmap 0 4k" -c "mwrite 0 4k" -c shutdown -c fsync \
>   tests/overlay/087:50:$XFS_IO_PROG -x -c "open $(_scratch_shutdown_handle)" -c 'shutdown -f ' -c close -c syncfs $SCRATCH_MNT | \
>   tests/overlay/087:57:$XFS_IO_PROG -x -c "open $(_scratch_shutdown_handle)" -c 'shutdown -f ' -c close -c syncfs $SCRATCH_MNT | \
>   ...
>
> others shutdown cases nearly all use *_scratch_shutdown* with
> *_require_scratch_shutdown*, these two functions are consistent in
> code logic. And no one calls "_require_xfs_io_command shutdown" currently.
>
> So g/623 and o/087 are specifal, actually they call _require_scratch_shutdown
> too, that makes sense for o/087. Now only g/623 doesn't make sense. Now we
> need to help it to make sense.
>
> I think the key is in _require_scratch_shutdown function [1], how about add an
> argument to clearly tell it we need to check shutdown "only on the top layer
> $SCRATCH_MNT" or "try the lowest layer $BASE_SCRATCH_MNT if there is".
>
> For example:
>
> diff --git a/common/rc b/common/rc
> index c3af8485c..5f30143e4 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -4075,15 +4075,17 @@ _require_exportfs()
>         _require_open_by_handle
>  }
>
> -# Does shutdown work on this fs?
> +# Does shutdown work on this [lower|top] layer fs?
>  _require_scratch_shutdown()
>  {
> +       local layer="${1:-lower}"
> +
>         [ -x $here/src/godown ] || _notrun "src/godown executable not found"
>
>         _scratch_mkfs > /dev/null 2>&1 || _notrun "_scratch_mkfs failed on $SCRATCH_DEV"
>         _scratch_mount
>
> -       if [ $FSTYP = "overlay" ]; then
> +       if [ $FSTYP = "overlay" -a "$level" = "lower" ]; then
>                 if [ -z $OVL_BASE_SCRATCH_DEV ]; then
>                         # In lagacy overlay usage, it may specify directory as
>                         # SCRATCH_DEV, in this case OVL_BASE_SCRATCH_DEV
> diff --git a/tests/generic/623 b/tests/generic/623
> index b97e2adbe..af0f55397 100755
> --- a/tests/generic/623
> +++ b/tests/generic/623
> @@ -15,7 +15,7 @@ _begin_fstest auto quick shutdown mmap
>         "xfs: restore shutdown check in mapped write fault path"
>
>  _require_scratch_nocheck
> -_require_scratch_shutdown
> +_require_scratch_shutdown top

Sorry I find this utterly confusing.

Think of all the 95% of fstests developers that do not care about overlayfs
what does this top mean to them and why should they use it for tests
that do xfs_io -c shutdown and not for tests that do _scratch_shutdown?

The test author and reviewers should be able to look at the tests and
easily derive what the test requirements should be according to simple rules.
For example:

1. A test that calls _scrash_shutdown needs to _require_scratch_shutdown
2. A test that calls _scratch_shutdown_and_syncfs needs to
_require_scratch_shutdown_and_syncfs
3. A test that calls xfs_io -c shutdown needs to _require_xfs_io_shutdown

I completely understand why you do not like my hack of
_require_xfs_io_command "shutdown"

Would you approve if it was an explicit _require_xfs_io_shutdown helper?

# Requirements for tests that call xfs_io -c shutdown instead of using the
# _scratch_shutdown helper
_require_xfs_io_shutdown()
{
                if [ $FSTYP = "overlay" ]; then
                        # Overlayfs tests can use _scratch_shutdown, but they
                        # cannot use "xfs_io -c shutdown"  command
without jumping
                        # through hoops, so by default we do not support it.
                        _notrun "xfs_io -c shutdown not supported on $FSTYP"
                fi
                _require_xfs_io_command "shutdown"
                _require_scratch_shutdown
}

Thanks,
Amir

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

[Zorro Lang]

On Fri, Jun 06, 2025 at 02:12:21PM +0200, Amir Goldstein wrote:
> On Fri, Jun 6, 2025 at 12:29 PM Zorro Lang <zlang@redhat.com> wrote:
> >
> > On Fri, Jun 06, 2025 at 09:23:26AM +0200, Amir Goldstein wrote:
> > > On Fri, Jun 6, 2025 at 3:45 AM Zorro Lang <zlang@redhat.com> wrote:
> > > >
> > > > On Thu, Jun 05, 2025 at 08:38:30PM +0200, Amir Goldstein wrote:
> > > > > On Thu, Jun 5, 2025 at 7:32 PM Zorro Lang <zlang@redhat.com> wrote:
> > > > > >
> > > > > > On Tue, Jun 03, 2025 at 12:07:43PM +0200, Amir Goldstein wrote:
> > > > > > > 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>
> > > > > > > Tested-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
> > > > > > > +             ;;
> > > > > >
> > > > > > Hmm... I'm not sure this's a good way.
> > > > > > For example, overlay/087 does xfs_io shutdown too,
> > > > >
> > > > > Yes it does but look at the effort needed to do that properly:
> > > > >
> > > > > $XFS_IO_PROG -x -c "open $(_scratch_shutdown_handle)" -c 'shutdown -f
> > > > > ' -c close -c syncfs $SCRATCH_MNT | \
> > > > >         grep -vF '[00'
> > > > >
> > > > > > generally it should calls
> > > > > > _require_xfs_io_command "shutdown" although it doesn't. If someone overlay
> > > > > > test case hope to test as o/087 does, and it calls _require_xfs_io_command "shutdown",
> > > > > > then it'll be _notrun.
> > > > >
> > > > > If someone knows enough to perform the dance above with _scratch_shutdown_handle
> > > > > then that someone should know enough not to call
> > > > > _require_xfs_io_command "shutdown".
> > > > > OTOH, if someone doesn't know then default is to not run.
> > > >
> > > > Sure, I can understand that, just this logic is a bit *obscure* :) It sounds like:
> > > > "If an overlay test case wants to do xfs_io shutdown, it shouldn't call
> > > > _require_xfs_io_command "shutdown". Or call that to skip a shutdown test
> > > > on overlay :)"
> > > >
> > > > And the expected result of _require_xfs_io_command "shutdown" will be totally
> > > > opposite with _require_scratch_shutdown on overlay, that might be confused.
> > > > Can we have a clearer way to deal with that?
> > > >
> > >
> > > I don't really understand the confusion.
> > >
> > > _require_xfs_io_command "shutdown"
> > >
> > > Like any other _require statement
> > > requires support for what this test does -
> > > meaning that a test does xfs_io -c shutdown, just like test generic/623 does
> > >
> > > and _require_scratch_shutdown implies that the test does
> > > _scratch_shutdown
> > >
> > > FSTYP overlay happens to be able to do _scratch_shutdown
> > > but not able to do xfs_io -c shutdown $SCRATCH_MNT
> > >
> > > The different _require statements simply reflect reality as it is.
> > >
> > > We can solve the confused about o/087 not having
> > > _require_xfs_io_command "shutdown"
> > > by moving the special hand crafted xfs_io command in o/087
> > > to a helper _scratch_shutdown_and_syncfs to hide those internal
> > > implementation details from test writers.
> > > See attached patch.
> >
> > Hmm... give me a moment to order my thoughts step by step :)
> >
> > There're only 2 cases tend to do xfs_io shutdown on overlay currently
> > (others are xfs specific test cases):
> >
> >   $ grep -rsn shutdown tests/|grep -- "-c"
> >   tests/generic/623:29:$XFS_IO_PROG -x -c "mmap 0 4k" -c "mwrite 0 4k" -c shutdown -c fsync \
> >   tests/overlay/087:50:$XFS_IO_PROG -x -c "open $(_scratch_shutdown_handle)" -c 'shutdown -f ' -c close -c syncfs $SCRATCH_MNT | \
> >   tests/overlay/087:57:$XFS_IO_PROG -x -c "open $(_scratch_shutdown_handle)" -c 'shutdown -f ' -c close -c syncfs $SCRATCH_MNT | \
> >   ...
> >
> > others shutdown cases nearly all use *_scratch_shutdown* with
> > *_require_scratch_shutdown*, these two functions are consistent in
> > code logic. And no one calls "_require_xfs_io_command shutdown" currently.
> >
> > So g/623 and o/087 are specifal, actually they call _require_scratch_shutdown
> > too, that makes sense for o/087. Now only g/623 doesn't make sense. Now we
> > need to help it to make sense.
> >
> > I think the key is in _require_scratch_shutdown function [1], how about add an
> > argument to clearly tell it we need to check shutdown "only on the top layer
> > $SCRATCH_MNT" or "try the lowest layer $BASE_SCRATCH_MNT if there is".
> >
> > For example:
> >
> > diff --git a/common/rc b/common/rc
> > index c3af8485c..5f30143e4 100644
> > --- a/common/rc
> > +++ b/common/rc
> > @@ -4075,15 +4075,17 @@ _require_exportfs()
> >         _require_open_by_handle
> >  }
> >
> > -# Does shutdown work on this fs?
> > +# Does shutdown work on this [lower|top] layer fs?
> >  _require_scratch_shutdown()
> >  {
> > +       local layer="${1:-lower}"
> > +
> >         [ -x $here/src/godown ] || _notrun "src/godown executable not found"
> >
> >         _scratch_mkfs > /dev/null 2>&1 || _notrun "_scratch_mkfs failed on $SCRATCH_DEV"
> >         _scratch_mount
> >
> > -       if [ $FSTYP = "overlay" ]; then
> > +       if [ $FSTYP = "overlay" -a "$level" = "lower" ]; then
> >                 if [ -z $OVL_BASE_SCRATCH_DEV ]; then
> >                         # In lagacy overlay usage, it may specify directory as
> >                         # SCRATCH_DEV, in this case OVL_BASE_SCRATCH_DEV
> > diff --git a/tests/generic/623 b/tests/generic/623
> > index b97e2adbe..af0f55397 100755
> > --- a/tests/generic/623
> > +++ b/tests/generic/623
> > @@ -15,7 +15,7 @@ _begin_fstest auto quick shutdown mmap
> >         "xfs: restore shutdown check in mapped write fault path"
> >
> >  _require_scratch_nocheck
> > -_require_scratch_shutdown
> > +_require_scratch_shutdown top
> 
> Sorry I find this utterly confusing.
> 
> Think of all the 95% of fstests developers that do not care about overlayfs
> what does this top mean to them and why should they use it for tests
> that do xfs_io -c shutdown and not for tests that do _scratch_shutdown?
> 
> The test author and reviewers should be able to look at the tests and
> easily derive what the test requirements should be according to simple rules.
> For example:
> 
> 1. A test that calls _scrash_shutdown needs to _require_scratch_shutdown
> 2. A test that calls _scratch_shutdown_and_syncfs needs to
> _require_scratch_shutdown_and_syncfs
> 3. A test that calls xfs_io -c shutdown needs to _require_xfs_io_shutdown
> 
> I completely understand why you do not like my hack of
> _require_xfs_io_command "shutdown"
> 
> Would you approve if it was an explicit _require_xfs_io_shutdown helper?
> 
> # Requirements for tests that call xfs_io -c shutdown instead of using the
> # _scratch_shutdown helper

OK, but you might metion that it's better not be used if _scratch_shutdown_handle
is called for xfs_io, as we hope the lower layer fs supports shutdown at that time:)

Actually I'm wondering if we should help xfstests to support BASE_FSTYP and FSTYP
for more upper layer fs, likes nfs, cifs, and so on. If so, overlay will not be
the only one fs who uses BASE_FSTYP and BASE_SCRATCH_DEV things, then we need to
differentiate if a feature (e.g. shutdown) is needed by upper layer fs or underlying
fs in a case.

...
BTW, a question which isn't belong to this patch:)  There're also some failures
from those xfstests overlay cases which run unionmount-testsuite (can't rememember
all, maybe o/102~109, o/144~117). The error (diff) output are similar as [1].
Is there a fix for that too? Or I missed the fix?

Thanks,
Zorro

[1]
--- /dev/fd/63	2025-06-07 05:16:59.489929312 -0400
+++ overlay/103.out.bad	2025-06-07 05:16:59.445716549 -0400
@@ -1,2 +1,17 @@
 QA output created by 103
+mount: /mnt/fstests/SCRATCH_DIR/union/m: wrong fs type, bad option, bad superblock on overlay, missing codepage or helper program, or other error.
+       dmesg(1) may have more information after failed mount system call.
+Traceback (most recent call last):
+  File "/mnt/tests/gitlab.cee.redhat.com/kernel-qe/kernel/-/archive/master/kernel-master.tar.bz2/filesystems/xfstests/unionmount-testsuite/./run", line 362, in <module>
+    func(ctx)
+  File "/mnt/tests/gitlab.cee.redhat.com/kernel-qe/kernel/-/archive/master/kernel-master.tar.bz2/filesystems/xfstests/unionmount-testsuite/tests/rename-file.py", line 71, in subtest_5
+    ctx.rename(f, f2)
+  File "/mnt/tests/gitlab.cee.redhat.com/kernel-qe/kernel/-/archive/master/kernel-master.tar.bz2/filesystems/xfstests/unionmount-testsuite/context.py", line 1254, in rename
+    remount_union(self, rotate_upper=True)
+  File "/mnt/tests/gitlab.cee.redhat.com/kernel-qe/kernel/-/archive/master/kernel-master.tar.bz2/filesystems/xfstests/unionmount-testsuite/remount_union.py", line 35, in remount_union
+    system(cmd)
+  File "/mnt/tests/gitlab.cee.redhat.com/kernel-qe/kernel/-/archive/master/kernel-master.tar.bz2/filesystems/xfstests/unionmount-testsuite/tool_box.py", line 25, in system
+    raise RuntimeError("Command failed: " + command)
+RuntimeError: Command failed: mount -t overlay overlay /mnt/fstests/SCRATCH_DIR/union/m -orw,xino=on -olowerdir=/mnt/fstests/SCRATCH_DIR/union/6/u:/mnt/fstests/SCRATCH_DIR/union/5/u:/mnt/fstests/SCRATCH_DIR/union/4/u:/mnt/fstests/SCRATCH_DIR/union/3/u:/mnt/fstests/SCRATCH_DIR/union/2/u:/mnt/fstests/SCRATCH_DIR/union/1/u:/mnt/fstests/SCRATCH_DIR/union/0/u:/mnt/fstests/SCRATCH_DIR/union/l,upperdir=/mnt/fstests/SCRATCH_DIR/union/7/u,workdir=/mnt/fstests/SCRATCH_DIR/union/7/w
+unionmount testsuite failed! see /var/lib/xfstests/results//overlay/103.full for details.
 Silence is golden

> _require_xfs_io_shutdown()
> {
>                 if [ $FSTYP = "overlay" ]; then
>                         # Overlayfs tests can use _scratch_shutdown, but they
>                         # cannot use "xfs_io -c shutdown"  command
> without jumping
>                         # through hoops, so by default we do not support it.
>                         _notrun "xfs_io -c shutdown not supported on $FSTYP"
>                 fi
>                 _require_xfs_io_command "shutdown"
>                 _require_scratch_shutdown
> }



> 
> Thanks,
> Amir
> 

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

[Amir Goldstein]

On Sun, Jun 8, 2025 at 3:16 PM Zorro Lang <zlang@redhat.com> wrote:
>
> On Fri, Jun 06, 2025 at 02:12:21PM +0200, Amir Goldstein wrote:
> > On Fri, Jun 6, 2025 at 12:29 PM Zorro Lang <zlang@redhat.com> wrote:
> > >
> > > On Fri, Jun 06, 2025 at 09:23:26AM +0200, Amir Goldstein wrote:
> > > > On Fri, Jun 6, 2025 at 3:45 AM Zorro Lang <zlang@redhat.com> wrote:
> > > > >
> > > > > On Thu, Jun 05, 2025 at 08:38:30PM +0200, Amir Goldstein wrote:
> > > > > > On Thu, Jun 5, 2025 at 7:32 PM Zorro Lang <zlang@redhat.com> wrote:
> > > > > > >
> > > > > > > On Tue, Jun 03, 2025 at 12:07:43PM +0200, Amir Goldstein wrote:
> > > > > > > > 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>
> > > > > > > > Tested-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
> > > > > > > > +             ;;
> > > > > > >
> > > > > > > Hmm... I'm not sure this's a good way.
> > > > > > > For example, overlay/087 does xfs_io shutdown too,
> > > > > >
> > > > > > Yes it does but look at the effort needed to do that properly:
> > > > > >
> > > > > > $XFS_IO_PROG -x -c "open $(_scratch_shutdown_handle)" -c 'shutdown -f
> > > > > > ' -c close -c syncfs $SCRATCH_MNT | \
> > > > > >         grep -vF '[00'
> > > > > >
> > > > > > > generally it should calls
> > > > > > > _require_xfs_io_command "shutdown" although it doesn't. If someone overlay
> > > > > > > test case hope to test as o/087 does, and it calls _require_xfs_io_command "shutdown",
> > > > > > > then it'll be _notrun.
> > > > > >
> > > > > > If someone knows enough to perform the dance above with _scratch_shutdown_handle
> > > > > > then that someone should know enough not to call
> > > > > > _require_xfs_io_command "shutdown".
> > > > > > OTOH, if someone doesn't know then default is to not run.
> > > > >
> > > > > Sure, I can understand that, just this logic is a bit *obscure* :) It sounds like:
> > > > > "If an overlay test case wants to do xfs_io shutdown, it shouldn't call
> > > > > _require_xfs_io_command "shutdown". Or call that to skip a shutdown test
> > > > > on overlay :)"
> > > > >
> > > > > And the expected result of _require_xfs_io_command "shutdown" will be totally
> > > > > opposite with _require_scratch_shutdown on overlay, that might be confused.
> > > > > Can we have a clearer way to deal with that?
> > > > >
> > > >
> > > > I don't really understand the confusion.
> > > >
> > > > _require_xfs_io_command "shutdown"
> > > >
> > > > Like any other _require statement
> > > > requires support for what this test does -
> > > > meaning that a test does xfs_io -c shutdown, just like test generic/623 does
> > > >
> > > > and _require_scratch_shutdown implies that the test does
> > > > _scratch_shutdown
> > > >
> > > > FSTYP overlay happens to be able to do _scratch_shutdown
> > > > but not able to do xfs_io -c shutdown $SCRATCH_MNT
> > > >
> > > > The different _require statements simply reflect reality as it is.
> > > >
> > > > We can solve the confused about o/087 not having
> > > > _require_xfs_io_command "shutdown"
> > > > by moving the special hand crafted xfs_io command in o/087
> > > > to a helper _scratch_shutdown_and_syncfs to hide those internal
> > > > implementation details from test writers.
> > > > See attached patch.
> > >
> > > Hmm... give me a moment to order my thoughts step by step :)
> > >
> > > There're only 2 cases tend to do xfs_io shutdown on overlay currently
> > > (others are xfs specific test cases):
> > >
> > >   $ grep -rsn shutdown tests/|grep -- "-c"
> > >   tests/generic/623:29:$XFS_IO_PROG -x -c "mmap 0 4k" -c "mwrite 0 4k" -c shutdown -c fsync \
> > >   tests/overlay/087:50:$XFS_IO_PROG -x -c "open $(_scratch_shutdown_handle)" -c 'shutdown -f ' -c close -c syncfs $SCRATCH_MNT | \
> > >   tests/overlay/087:57:$XFS_IO_PROG -x -c "open $(_scratch_shutdown_handle)" -c 'shutdown -f ' -c close -c syncfs $SCRATCH_MNT | \
> > >   ...
> > >
> > > others shutdown cases nearly all use *_scratch_shutdown* with
> > > *_require_scratch_shutdown*, these two functions are consistent in
> > > code logic. And no one calls "_require_xfs_io_command shutdown" currently.
> > >
> > > So g/623 and o/087 are specifal, actually they call _require_scratch_shutdown
> > > too, that makes sense for o/087. Now only g/623 doesn't make sense. Now we
> > > need to help it to make sense.
> > >
> > > I think the key is in _require_scratch_shutdown function [1], how about add an
> > > argument to clearly tell it we need to check shutdown "only on the top layer
> > > $SCRATCH_MNT" or "try the lowest layer $BASE_SCRATCH_MNT if there is".
> > >
> > > For example:
> > >
> > > diff --git a/common/rc b/common/rc
> > > index c3af8485c..5f30143e4 100644
> > > --- a/common/rc
> > > +++ b/common/rc
> > > @@ -4075,15 +4075,17 @@ _require_exportfs()
> > >         _require_open_by_handle
> > >  }
> > >
> > > -# Does shutdown work on this fs?
> > > +# Does shutdown work on this [lower|top] layer fs?
> > >  _require_scratch_shutdown()
> > >  {
> > > +       local layer="${1:-lower}"
> > > +
> > >         [ -x $here/src/godown ] || _notrun "src/godown executable not found"
> > >
> > >         _scratch_mkfs > /dev/null 2>&1 || _notrun "_scratch_mkfs failed on $SCRATCH_DEV"
> > >         _scratch_mount
> > >
> > > -       if [ $FSTYP = "overlay" ]; then
> > > +       if [ $FSTYP = "overlay" -a "$level" = "lower" ]; then
> > >                 if [ -z $OVL_BASE_SCRATCH_DEV ]; then
> > >                         # In lagacy overlay usage, it may specify directory as
> > >                         # SCRATCH_DEV, in this case OVL_BASE_SCRATCH_DEV
> > > diff --git a/tests/generic/623 b/tests/generic/623
> > > index b97e2adbe..af0f55397 100755
> > > --- a/tests/generic/623
> > > +++ b/tests/generic/623
> > > @@ -15,7 +15,7 @@ _begin_fstest auto quick shutdown mmap
> > >         "xfs: restore shutdown check in mapped write fault path"
> > >
> > >  _require_scratch_nocheck
> > > -_require_scratch_shutdown
> > > +_require_scratch_shutdown top
> >
> > Sorry I find this utterly confusing.
> >
> > Think of all the 95% of fstests developers that do not care about overlayfs
> > what does this top mean to them and why should they use it for tests
> > that do xfs_io -c shutdown and not for tests that do _scratch_shutdown?
> >
> > The test author and reviewers should be able to look at the tests and
> > easily derive what the test requirements should be according to simple rules.
> > For example:
> >
> > 1. A test that calls _scrash_shutdown needs to _require_scratch_shutdown
> > 2. A test that calls _scratch_shutdown_and_syncfs needs to
> > _require_scratch_shutdown_and_syncfs
> > 3. A test that calls xfs_io -c shutdown needs to _require_xfs_io_shutdown
> >
> > I completely understand why you do not like my hack of
> > _require_xfs_io_command "shutdown"
> >
> > Would you approve if it was an explicit _require_xfs_io_shutdown helper?
> >
> > # Requirements for tests that call xfs_io -c shutdown instead of using the
> > # _scratch_shutdown helper
>
> OK, but you might metion that it's better not be used if _scratch_shutdown_handle
> is called for xfs_io, as we hope the lower layer fs supports shutdown at that time:)
>

Sure I'll add that

> Actually I'm wondering if we should help xfstests to support BASE_FSTYP and FSTYP
> for more upper layer fs, likes nfs, cifs, and so on.

I think that could be very useful, but will require cifs/nfs to implement
more complicated _mount/umount/remount helpers like overlay.

> If so, overlay will not be
> the only one fs who uses BASE_FSTYP and BASE_SCRATCH_DEV things, then we need to
> differentiate if a feature (e.g. shutdown) is needed by upper layer fs or underlying
> fs in a case.
>

First of all, terminology. Many get this wrong.
In overlayfs, the "upper" and "lower" refer to the different underlying layers.
In fstests BASE_SCRATCH_DEV is always the same for both OVL_UPPER and
OVL_LOWER layers.

Referring to the BASE_SCRATCH_DEV as "underlying" or "base" fs is
unambiguous in all cases of overlay/nfs/cifs.

I do not have a good terminology to offer for referring to the "fs under test"
be it overlay/cifs/nfs. You are welcome to offer your suggestions.

> ...
> BTW, a question which isn't belong to this patch:)  There're also some failures
> from those xfstests overlay cases which run unionmount-testsuite (can't rememember
> all, maybe o/102~109, o/144~117). The error (diff) output are similar as [1].
> Is there a fix for that too? Or I missed the fix?

I do not get these errors.
I suppose you are using the latest unionmount-testsuite,
although it hasn't been changed for a while anyway.

>
> Thanks,
> Zorro
>
> [1]
> --- /dev/fd/63  2025-06-07 05:16:59.489929312 -0400
> +++ overlay/103.out.bad 2025-06-07 05:16:59.445716549 -0400
> @@ -1,2 +1,17 @@
>  QA output created by 103
> +mount: /mnt/fstests/SCRATCH_DIR/union/m: wrong fs type, bad option, bad superblock on overlay, missing codepage or helper program, or other error.
> +       dmesg(1) may have more information after failed mount system call.

Reason for failure to mount is likely to be found in dmesg.
Please try to see if it is there.

> +Traceback (most recent call last):
> +  File "/mnt/tests/gitlab.cee.redhat.com/kernel-qe/kernel/-/archive/master/kernel-master.tar.bz2/filesystems/xfstests/unionmount-testsuite/./run", line 362, in <module>
> +    func(ctx)
> +  File "/mnt/tests/gitlab.cee.redhat.com/kernel-qe/kernel/-/archive/master/kernel-master.tar.bz2/filesystems/xfstests/unionmount-testsuite/tests/rename-file.py", line 71, in subtest_5
> +    ctx.rename(f, f2)
> +  File "/mnt/tests/gitlab.cee.redhat.com/kernel-qe/kernel/-/archive/master/kernel-master.tar.bz2/filesystems/xfstests/unionmount-testsuite/context.py", line 1254, in rename
> +    remount_union(self, rotate_upper=True)
> +  File "/mnt/tests/gitlab.cee.redhat.com/kernel-qe/kernel/-/archive/master/kernel-master.tar.bz2/filesystems/xfstests/unionmount-testsuite/remount_union.py", line 35, in remount_union
> +    system(cmd)
> +  File "/mnt/tests/gitlab.cee.redhat.com/kernel-qe/kernel/-/archive/master/kernel-master.tar.bz2/filesystems/xfstests/unionmount-testsuite/tool_box.py", line 25, in system
> +    raise RuntimeError("Command failed: " + command)
> +RuntimeError: Command failed: mount -t overlay overlay /mnt/fstests/SCRATCH_DIR/union/m -orw,xino=on -olowerdir=/mnt/fstests/SCRATCH_DIR/union/6/u:/mnt/fstests/SCRATCH_DIR/union/5/u:/mnt/fstests/SCRATCH_DIR/union/4/u:/mnt/fstests/SCRATCH_DIR/union/3/u:/mnt/fstests/SCRATCH_DIR/union/2/u:/mnt/fstests/SCRATCH_DIR/union/1/u:/mnt/fstests/SCRATCH_DIR/union/0/u:/mnt/fstests/SCRATCH_DIR/union/l,upperdir=/mnt/fstests/SCRATCH_DIR/union/7/u,workdir=/mnt/fstests/SCRATCH_DIR/union/7/w

Nothing looks obviously wrong in the mount command above.
Was this a regression for you?
with kernel upgrade? libmount upgrade?

Thanks,
Amir.

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

[Amir Goldstein]

> > Actually I'm wondering if we should help xfstests to support BASE_FSTYP and FSTYP
> > for more upper layer fs, likes nfs, cifs, and so on.
>
> I think that could be very useful, but will require cifs/nfs to implement
> more complicated _mount/umount/remount helpers like overlay.
>
> > If so, overlay will not be
> > the only one fs who uses BASE_FSTYP and BASE_SCRATCH_DEV things, then we need to
> > differentiate if a feature (e.g. shutdown) is needed by upper layer fs or underlying
> > fs in a case.
> >
>
> First of all, terminology. Many get this wrong.
> In overlayfs, the "upper" and "lower" refer to the different underlying layers.
> In fstests BASE_SCRATCH_DEV is always the same for both OVL_UPPER and
> OVL_LOWER layers.
>
> Referring to the BASE_SCRATCH_DEV as "underlying" or "base" fs is
> unambiguous in all cases of overlay/nfs/cifs.
>
> I do not have a good terminology to offer for referring to the "fs under test"
> be it overlay/cifs/nfs. You are welcome to offer your suggestions.
>

Sorry, I forgot to answer the question.

So far, with overlayfs there was no need for anything other than
_require_scratch_shutdown and _scratch_shutdown and
_scratch_remount for generic tests that do shutdown and remount
to test consistency after a crash.

overlay tests that are aware of the underlying layers and perform
operations explicitly on underlying layers are not generic tests
they are overlay/* tests, so the generic helpers are not for them.

Bottom line is that I do not think that _require_scratch_shutdown
should refer to fs under test or base fs, until I see a generic
test case that suggests otherwise.

What we could do is a re-factoring:

_require_shutdown()
{
        local dev=$1
        local mnt=$2
...
}

_require_scratch_shutdown()
{
        _require_shutdown $SCRATCH_DEV $SCRATCH_MNT
}

Then overlay,cifs,nfs/* tests can call _require_shutdown on base fs
if they want to, but first, let's see a test case that needs it.

Thanks,
Amir.

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

[Zorro Lang]

On Mon, Jun 09, 2025 at 01:12:35PM +0200, Amir Goldstein wrote:
> > > Actually I'm wondering if we should help xfstests to support BASE_FSTYP and FSTYP
> > > for more upper layer fs, likes nfs, cifs, and so on.
> >
> > I think that could be very useful, but will require cifs/nfs to implement
> > more complicated _mount/umount/remount helpers like overlay.
> >
> > > If so, overlay will not be
> > > the only one fs who uses BASE_FSTYP and BASE_SCRATCH_DEV things, then we need to
> > > differentiate if a feature (e.g. shutdown) is needed by upper layer fs or underlying
> > > fs in a case.
> > >
> >
> > First of all, terminology. Many get this wrong.
> > In overlayfs, the "upper" and "lower" refer to the different underlying layers.
> > In fstests BASE_SCRATCH_DEV is always the same for both OVL_UPPER and
> > OVL_LOWER layers.
> >
> > Referring to the BASE_SCRATCH_DEV as "underlying" or "base" fs is
> > unambiguous in all cases of overlay/nfs/cifs.
> >
> > I do not have a good terminology to offer for referring to the "fs under test"
> > be it overlay/cifs/nfs. You are welcome to offer your suggestions.
> >
> 
> Sorry, I forgot to answer the question.
> 
> So far, with overlayfs there was no need for anything other than
> _require_scratch_shutdown and _scratch_shutdown and
> _scratch_remount for generic tests that do shutdown and remount
> to test consistency after a crash.
> 
> overlay tests that are aware of the underlying layers and perform
> operations explicitly on underlying layers are not generic tests
> they are overlay/* tests, so the generic helpers are not for them.
> 
> Bottom line is that I do not think that _require_scratch_shutdown
> should refer to fs under test or base fs, until I see a generic
> test case that suggests otherwise.
> 
> What we could do is a re-factoring:
> 
> _require_shutdown()
> {
>         local dev=$1
>         local mnt=$2
> ...
> }
> 
> _require_scratch_shutdown()
> {
>         _require_shutdown $SCRATCH_DEV $SCRATCH_MNT
> }
> 
> Then overlay,cifs,nfs/* tests can call _require_shutdown on base fs
> if they want to, but first, let's see a test case that needs it.

Actually I thought about the _require_shutdown when I reviewed this
patch. Something likes:

g/623: _require_shutdown $SCRATCH_MNT ...
o/087: _require_shutdown $BASE_SCRATCH_MNT ...

But I dropped this idea for two reasons:
1) _require_shutdown needs to do real "shutdown" on a directory, a flexible
   argument to be "shutdown" is dangerous. If someone would like to check
   "shutdown" is supported on a $mnt, but if some exceptions cause the $mnt
   isn't mounted the expected fs, then the "/" might be down.
2) shutdown checking need to shutdown the $mnt and remount it. It's not
   only about $mnt and $dev, but also about mount options. _require_scratch_shutdown
   can use _scratch_mount, but how to deal with a flexible $mnt?

The 1) problem might can be fixed by checking the $mnt is a mountpoint, or return.
The 2) problem is difficult to me, except we let _require_shutdown accept the mount
options list as the 3rd argument. I think that looks ugly for a _require_ function :)

If you have good idea, it can replace this patch 4/6 :)

Thanks,
Zorro

> 
> Thanks,
> Amir.
> 

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

[Amir Goldstein]

On Mon, Jun 9, 2025 at 7:26 PM Zorro Lang <zlang@redhat.com> wrote:
>
> On Mon, Jun 09, 2025 at 01:12:35PM +0200, Amir Goldstein wrote:
> > > > Actually I'm wondering if we should help xfstests to support BASE_FSTYP and FSTYP
> > > > for more upper layer fs, likes nfs, cifs, and so on.
> > >
> > > I think that could be very useful, but will require cifs/nfs to implement
> > > more complicated _mount/umount/remount helpers like overlay.
> > >
> > > > If so, overlay will not be
> > > > the only one fs who uses BASE_FSTYP and BASE_SCRATCH_DEV things, then we need to
> > > > differentiate if a feature (e.g. shutdown) is needed by upper layer fs or underlying
> > > > fs in a case.
> > > >
> > >
> > > First of all, terminology. Many get this wrong.
> > > In overlayfs, the "upper" and "lower" refer to the different underlying layers.
> > > In fstests BASE_SCRATCH_DEV is always the same for both OVL_UPPER and
> > > OVL_LOWER layers.
> > >
> > > Referring to the BASE_SCRATCH_DEV as "underlying" or "base" fs is
> > > unambiguous in all cases of overlay/nfs/cifs.
> > >
> > > I do not have a good terminology to offer for referring to the "fs under test"
> > > be it overlay/cifs/nfs. You are welcome to offer your suggestions.
> > >
> >
> > Sorry, I forgot to answer the question.
> >
> > So far, with overlayfs there was no need for anything other than
> > _require_scratch_shutdown and _scratch_shutdown and
> > _scratch_remount for generic tests that do shutdown and remount
> > to test consistency after a crash.
> >
> > overlay tests that are aware of the underlying layers and perform
> > operations explicitly on underlying layers are not generic tests
> > they are overlay/* tests, so the generic helpers are not for them.
> >
> > Bottom line is that I do not think that _require_scratch_shutdown
> > should refer to fs under test or base fs, until I see a generic
> > test case that suggests otherwise.
> >
> > What we could do is a re-factoring:
> >
> > _require_shutdown()
> > {
> >         local dev=$1
> >         local mnt=$2
> > ...
> > }
> >
> > _require_scratch_shutdown()
> > {
> >         _require_shutdown $SCRATCH_DEV $SCRATCH_MNT
> > }
> >
> > Then overlay,cifs,nfs/* tests can call _require_shutdown on base fs
> > if they want to, but first, let's see a test case that needs it.
>
> Actually I thought about the _require_shutdown when I reviewed this
> patch. Something likes:
>
> g/623: _require_shutdown $SCRATCH_MNT ...
> o/087: _require_shutdown $BASE_SCRATCH_MNT ...
>
> But I dropped this idea for two reasons:
> 1) _require_shutdown needs to do real "shutdown" on a directory, a flexible
>    argument to be "shutdown" is dangerous. If someone would like to check
>    "shutdown" is supported on a $mnt, but if some exceptions cause the $mnt
>    isn't mounted the expected fs, then the "/" might be down.
> 2) shutdown checking need to shutdown the $mnt and remount it. It's not
>    only about $mnt and $dev, but also about mount options. _require_scratch_shutdown
>    can use _scratch_mount, but how to deal with a flexible $mnt?
>
> The 1) problem might can be fixed by checking the $mnt is a mountpoint, or return.
> The 2) problem is difficult to me, except we let _require_shutdown accept the mount
> options list as the 3rd argument. I think that looks ugly for a _require_ function :)
>

I don't like it either

> If you have good idea, it can replace this patch 4/6 :)
>

Already sent a replacement patch per our earlier discussion
https://lore.kernel.org/fstests/20250609151915.2638057-3-amir73il@gmail.com/

Thanks,
Amir.

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

[Zorro Lang]

On Mon, Jun 09, 2025 at 01:01:37PM +0200, Amir Goldstein wrote:
> On Sun, Jun 8, 2025 at 3:16 PM Zorro Lang <zlang@redhat.com> wrote:
> >
> > On Fri, Jun 06, 2025 at 02:12:21PM +0200, Amir Goldstein wrote:
> > > On Fri, Jun 6, 2025 at 12:29 PM Zorro Lang <zlang@redhat.com> wrote:
> > > >
> > > > On Fri, Jun 06, 2025 at 09:23:26AM +0200, Amir Goldstein wrote:
> > > > > On Fri, Jun 6, 2025 at 3:45 AM Zorro Lang <zlang@redhat.com> wrote:
> > > > > >
> > > > > > On Thu, Jun 05, 2025 at 08:38:30PM +0200, Amir Goldstein wrote:
> > > > > > > On Thu, Jun 5, 2025 at 7:32 PM Zorro Lang <zlang@redhat.com> wrote:
> > > > > > > >
> > > > > > > > On Tue, Jun 03, 2025 at 12:07:43PM +0200, Amir Goldstein wrote:
> > > > > > > > > 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>
> > > > > > > > > Tested-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
> > > > > > > > > +             ;;
> > > > > > > >
> > > > > > > > Hmm... I'm not sure this's a good way.
> > > > > > > > For example, overlay/087 does xfs_io shutdown too,
> > > > > > >
> > > > > > > Yes it does but look at the effort needed to do that properly:
> > > > > > >
> > > > > > > $XFS_IO_PROG -x -c "open $(_scratch_shutdown_handle)" -c 'shutdown -f
> > > > > > > ' -c close -c syncfs $SCRATCH_MNT | \
> > > > > > >         grep -vF '[00'
> > > > > > >
> > > > > > > > generally it should calls
> > > > > > > > _require_xfs_io_command "shutdown" although it doesn't. If someone overlay
> > > > > > > > test case hope to test as o/087 does, and it calls _require_xfs_io_command "shutdown",
> > > > > > > > then it'll be _notrun.
> > > > > > >
> > > > > > > If someone knows enough to perform the dance above with _scratch_shutdown_handle
> > > > > > > then that someone should know enough not to call
> > > > > > > _require_xfs_io_command "shutdown".
> > > > > > > OTOH, if someone doesn't know then default is to not run.
> > > > > >
> > > > > > Sure, I can understand that, just this logic is a bit *obscure* :) It sounds like:
> > > > > > "If an overlay test case wants to do xfs_io shutdown, it shouldn't call
> > > > > > _require_xfs_io_command "shutdown". Or call that to skip a shutdown test
> > > > > > on overlay :)"
> > > > > >
> > > > > > And the expected result of _require_xfs_io_command "shutdown" will be totally
> > > > > > opposite with _require_scratch_shutdown on overlay, that might be confused.
> > > > > > Can we have a clearer way to deal with that?
> > > > > >
> > > > >
> > > > > I don't really understand the confusion.
> > > > >
> > > > > _require_xfs_io_command "shutdown"
> > > > >
> > > > > Like any other _require statement
> > > > > requires support for what this test does -
> > > > > meaning that a test does xfs_io -c shutdown, just like test generic/623 does
> > > > >
> > > > > and _require_scratch_shutdown implies that the test does
> > > > > _scratch_shutdown
> > > > >
> > > > > FSTYP overlay happens to be able to do _scratch_shutdown
> > > > > but not able to do xfs_io -c shutdown $SCRATCH_MNT
> > > > >
> > > > > The different _require statements simply reflect reality as it is.
> > > > >
> > > > > We can solve the confused about o/087 not having
> > > > > _require_xfs_io_command "shutdown"
> > > > > by moving the special hand crafted xfs_io command in o/087
> > > > > to a helper _scratch_shutdown_and_syncfs to hide those internal
> > > > > implementation details from test writers.
> > > > > See attached patch.
> > > >
> > > > Hmm... give me a moment to order my thoughts step by step :)
> > > >
> > > > There're only 2 cases tend to do xfs_io shutdown on overlay currently
> > > > (others are xfs specific test cases):
> > > >
> > > >   $ grep -rsn shutdown tests/|grep -- "-c"
> > > >   tests/generic/623:29:$XFS_IO_PROG -x -c "mmap 0 4k" -c "mwrite 0 4k" -c shutdown -c fsync \
> > > >   tests/overlay/087:50:$XFS_IO_PROG -x -c "open $(_scratch_shutdown_handle)" -c 'shutdown -f ' -c close -c syncfs $SCRATCH_MNT | \
> > > >   tests/overlay/087:57:$XFS_IO_PROG -x -c "open $(_scratch_shutdown_handle)" -c 'shutdown -f ' -c close -c syncfs $SCRATCH_MNT | \
> > > >   ...
> > > >
> > > > others shutdown cases nearly all use *_scratch_shutdown* with
> > > > *_require_scratch_shutdown*, these two functions are consistent in
> > > > code logic. And no one calls "_require_xfs_io_command shutdown" currently.
> > > >
> > > > So g/623 and o/087 are specifal, actually they call _require_scratch_shutdown
> > > > too, that makes sense for o/087. Now only g/623 doesn't make sense. Now we
> > > > need to help it to make sense.
> > > >
> > > > I think the key is in _require_scratch_shutdown function [1], how about add an
> > > > argument to clearly tell it we need to check shutdown "only on the top layer
> > > > $SCRATCH_MNT" or "try the lowest layer $BASE_SCRATCH_MNT if there is".
> > > >
> > > > For example:
> > > >
> > > > diff --git a/common/rc b/common/rc
> > > > index c3af8485c..5f30143e4 100644
> > > > --- a/common/rc
> > > > +++ b/common/rc
> > > > @@ -4075,15 +4075,17 @@ _require_exportfs()
> > > >         _require_open_by_handle
> > > >  }
> > > >
> > > > -# Does shutdown work on this fs?
> > > > +# Does shutdown work on this [lower|top] layer fs?
> > > >  _require_scratch_shutdown()
> > > >  {
> > > > +       local layer="${1:-lower}"
> > > > +
> > > >         [ -x $here/src/godown ] || _notrun "src/godown executable not found"
> > > >
> > > >         _scratch_mkfs > /dev/null 2>&1 || _notrun "_scratch_mkfs failed on $SCRATCH_DEV"
> > > >         _scratch_mount
> > > >
> > > > -       if [ $FSTYP = "overlay" ]; then
> > > > +       if [ $FSTYP = "overlay" -a "$level" = "lower" ]; then
> > > >                 if [ -z $OVL_BASE_SCRATCH_DEV ]; then
> > > >                         # In lagacy overlay usage, it may specify directory as
> > > >                         # SCRATCH_DEV, in this case OVL_BASE_SCRATCH_DEV
> > > > diff --git a/tests/generic/623 b/tests/generic/623
> > > > index b97e2adbe..af0f55397 100755
> > > > --- a/tests/generic/623
> > > > +++ b/tests/generic/623
> > > > @@ -15,7 +15,7 @@ _begin_fstest auto quick shutdown mmap
> > > >         "xfs: restore shutdown check in mapped write fault path"
> > > >
> > > >  _require_scratch_nocheck
> > > > -_require_scratch_shutdown
> > > > +_require_scratch_shutdown top
> > >
> > > Sorry I find this utterly confusing.
> > >
> > > Think of all the 95% of fstests developers that do not care about overlayfs
> > > what does this top mean to them and why should they use it for tests
> > > that do xfs_io -c shutdown and not for tests that do _scratch_shutdown?
> > >
> > > The test author and reviewers should be able to look at the tests and
> > > easily derive what the test requirements should be according to simple rules.
> > > For example:
> > >
> > > 1. A test that calls _scrash_shutdown needs to _require_scratch_shutdown
> > > 2. A test that calls _scratch_shutdown_and_syncfs needs to
> > > _require_scratch_shutdown_and_syncfs
> > > 3. A test that calls xfs_io -c shutdown needs to _require_xfs_io_shutdown
> > >
> > > I completely understand why you do not like my hack of
> > > _require_xfs_io_command "shutdown"
> > >
> > > Would you approve if it was an explicit _require_xfs_io_shutdown helper?
> > >
> > > # Requirements for tests that call xfs_io -c shutdown instead of using the
> > > # _scratch_shutdown helper
> >
> > OK, but you might metion that it's better not be used if _scratch_shutdown_handle
> > is called for xfs_io, as we hope the lower layer fs supports shutdown at that time:)
> >
> 
> Sure I'll add that
> 
> > Actually I'm wondering if we should help xfstests to support BASE_FSTYP and FSTYP
> > for more upper layer fs, likes nfs, cifs, and so on.
> 
> I think that could be very useful, but will require cifs/nfs to implement
> more complicated _mount/umount/remount helpers like overlay.
> 
> > If so, overlay will not be
> > the only one fs who uses BASE_FSTYP and BASE_SCRATCH_DEV things, then we need to
> > differentiate if a feature (e.g. shutdown) is needed by upper layer fs or underlying
> > fs in a case.
> >
> 
> First of all, terminology. Many get this wrong.
> In overlayfs, the "upper" and "lower" refer to the different underlying layers.
> In fstests BASE_SCRATCH_DEV is always the same for both OVL_UPPER and
> OVL_LOWER layers.
> 
> Referring to the BASE_SCRATCH_DEV as "underlying" or "base" fs is
> unambiguous in all cases of overlay/nfs/cifs.
> 
> I do not have a good terminology to offer for referring to the "fs under test"
> be it overlay/cifs/nfs. You are welcome to offer your suggestions.
> 
> > ...
> > BTW, a question which isn't belong to this patch:)  There're also some failures
> > from those xfstests overlay cases which run unionmount-testsuite (can't rememember
> > all, maybe o/102~109, o/144~117). The error (diff) output are similar as [1].
> > Is there a fix for that too? Or I missed the fix?
> 
> I do not get these errors.
> I suppose you are using the latest unionmount-testsuite,
> although it hasn't been changed for a while anyway.
> 
> >
> > Thanks,
> > Zorro
> >
> > [1]
> > --- /dev/fd/63  2025-06-07 05:16:59.489929312 -0400
> > +++ overlay/103.out.bad 2025-06-07 05:16:59.445716549 -0400
> > @@ -1,2 +1,17 @@
> >  QA output created by 103
> > +mount: /mnt/fstests/SCRATCH_DIR/union/m: wrong fs type, bad option, bad superblock on overlay, missing codepage or helper program, or other error.
> > +       dmesg(1) may have more information after failed mount system call.
> 
> Reason for failure to mount is likely to be found in dmesg.
> Please try to see if it is there.
> 
> > +Traceback (most recent call last):
> > +  File "/mnt/tests/gitlab.cee.redhat.com/kernel-qe/kernel/-/archive/master/kernel-master.tar.bz2/filesystems/xfstests/unionmount-testsuite/./run", line 362, in <module>
> > +    func(ctx)
> > +  File "/mnt/tests/gitlab.cee.redhat.com/kernel-qe/kernel/-/archive/master/kernel-master.tar.bz2/filesystems/xfstests/unionmount-testsuite/tests/rename-file.py", line 71, in subtest_5
> > +    ctx.rename(f, f2)
> > +  File "/mnt/tests/gitlab.cee.redhat.com/kernel-qe/kernel/-/archive/master/kernel-master.tar.bz2/filesystems/xfstests/unionmount-testsuite/context.py", line 1254, in rename
> > +    remount_union(self, rotate_upper=True)
> > +  File "/mnt/tests/gitlab.cee.redhat.com/kernel-qe/kernel/-/archive/master/kernel-master.tar.bz2/filesystems/xfstests/unionmount-testsuite/remount_union.py", line 35, in remount_union
> > +    system(cmd)
> > +  File "/mnt/tests/gitlab.cee.redhat.com/kernel-qe/kernel/-/archive/master/kernel-master.tar.bz2/filesystems/xfstests/unionmount-testsuite/tool_box.py", line 25, in system
> > +    raise RuntimeError("Command failed: " + command)
> > +RuntimeError: Command failed: mount -t overlay overlay /mnt/fstests/SCRATCH_DIR/union/m -orw,xino=on -olowerdir=/mnt/fstests/SCRATCH_DIR/union/6/u:/mnt/fstests/SCRATCH_DIR/union/5/u:/mnt/fstests/SCRATCH_DIR/union/4/u:/mnt/fstests/SCRATCH_DIR/union/3/u:/mnt/fstests/SCRATCH_DIR/union/2/u:/mnt/fstests/SCRATCH_DIR/union/1/u:/mnt/fstests/SCRATCH_DIR/union/0/u:/mnt/fstests/SCRATCH_DIR/union/l,upperdir=/mnt/fstests/SCRATCH_DIR/union/7/u,workdir=/mnt/fstests/SCRATCH_DIR/union/7/w
> 
> Nothing looks obviously wrong in the mount command above.
> Was this a regression for you?
> with kernel upgrade? libmount upgrade?

Thanks Amir, I'll check more and send another email to talk about this
failure :)

> 
> Thanks,
> Amir.
> 

[PATCH v2 5/6] generic: remove incorrect _require_idmapped_mounts checks

[Amir Goldstein]

commit f5661920 ("generic: add missed _require_idmapped_mounts check")
wrongly adds _require_idmapped_mounts to tests that do not require
idmapped mounts support.

The added _require_idmapped_mounts in test generic/633 goes against
commit d8dee122 ("idmapped-mounts: always run generic vfs tests")
that intentionally removed this requirement from the generic tests.

The added _require_idmapped_mounts in tests generic/69{6,7} causes
those tests not to run with overlayfs, which does not support idmapped
mounts. However, those tests are regression tests to kernel commit
1639a49ccdce ("fs: move S_ISGID stripping into the vfs_*() helpers")
which is documented as also solving a correction issue with overlayfs,
so removing this test converage is very much undesired.

Remove the incorrectly added _require_idmapped_mounts checks.
Also fix the log in _require_idmapped_mounts to say that
"idmapped mounts not support by $FSTYP", which is what the helper
checks instead of "vfstests not support by $FSTYP" which is incorrect.

Cc: Yang Xu <xuyang2018.jy@fujitsu.com>
Cc: Anthony Iliopoulos <ailiop@suse.com>
Cc: David Disseldorp <ddiss@suse.de>
Fixes: commit f5661920 ("generic: add missed _require_idmapped_mounts check")
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 common/rc         | 2 +-
 tests/generic/633 | 1 -
 tests/generic/696 | 1 -
 tests/generic/697 | 1 -
 4 files changed, 1 insertion(+), 4 deletions(-)

diff --git a/common/rc b/common/rc
index bffd576a..96d65d1c 100644
--- a/common/rc
+++ b/common/rc
@@ -2639,7 +2639,7 @@ _require_idmapped_mounts()
 		--fstype "$FSTYP"
 
 	if [ $? -ne 0 ]; then
-		_notrun "vfstest not support by $FSTYP"
+		_notrun "idmapped mounts not support by $FSTYP"
 	fi
 }
 
diff --git a/tests/generic/633 b/tests/generic/633
index f58dbbf5..b683c427 100755
--- a/tests/generic/633
+++ b/tests/generic/633
@@ -12,7 +12,6 @@ _begin_fstest auto quick atime attr cap idmapped io_uring mount perms rw unlink
 # Import common functions.
 . ./common/filter
 
-_require_idmapped_mounts
 _require_test
 
 echo "Silence is golden"
diff --git a/tests/generic/696 b/tests/generic/696
index d2e86c96..48b3aea0 100755
--- a/tests/generic/696
+++ b/tests/generic/696
@@ -17,7 +17,6 @@ _begin_fstest auto quick cap idmapped mount perms rw unlink
 # Import common functions.
 . ./common/filter
 
-_require_idmapped_mounts
 _require_test
 _require_scratch
 _fixed_by_kernel_commit ac6800e279a2 \
diff --git a/tests/generic/697 b/tests/generic/697
index 1ce673f7..66444a95 100755
--- a/tests/generic/697
+++ b/tests/generic/697
@@ -17,7 +17,6 @@ _begin_fstest auto quick cap acl idmapped mount perms rw unlink
 . ./common/filter
 . ./common/attr
 
-_require_idmapped_mounts
 _require_test
 _require_acls
 _fixed_by_kernel_commit 1639a49ccdce \
-- 
2.34.1

Re: [PATCH v2 5/6] generic: remove incorrect _require_idmapped_mounts checks

[Zorro Lang]

On Tue, Jun 03, 2025 at 12:07:44PM +0200, Amir Goldstein wrote:
> commit f5661920 ("generic: add missed _require_idmapped_mounts check")
> wrongly adds _require_idmapped_mounts to tests that do not require
> idmapped mounts support.
> 
> The added _require_idmapped_mounts in test generic/633 goes against
> commit d8dee122 ("idmapped-mounts: always run generic vfs tests")
> that intentionally removed this requirement from the generic tests.
> 
> The added _require_idmapped_mounts in tests generic/69{6,7} causes
> those tests not to run with overlayfs, which does not support idmapped
> mounts. However, those tests are regression tests to kernel commit
> 1639a49ccdce ("fs: move S_ISGID stripping into the vfs_*() helpers")
> which is documented as also solving a correction issue with overlayfs,
> so removing this test converage is very much undesired.
> 
> Remove the incorrectly added _require_idmapped_mounts checks.
> Also fix the log in _require_idmapped_mounts to say that
> "idmapped mounts not support by $FSTYP", which is what the helper
> checks instead of "vfstests not support by $FSTYP" which is incorrect.
> 
> Cc: Yang Xu <xuyang2018.jy@fujitsu.com>
> Cc: Anthony Iliopoulos <ailiop@suse.com>
> Cc: David Disseldorp <ddiss@suse.de>
> Fixes: commit f5661920 ("generic: add missed _require_idmapped_mounts check")
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---

Is this one changed anything from this ?
https://lore.kernel.org/fstests/20250526175437.1528310-1-amir73il@gmail.com/

Due to above link has been reviewed by Christian Brauner, do you want to
add his RVB to this version?

Thanks,
Zorro

>  common/rc         | 2 +-
>  tests/generic/633 | 1 -
>  tests/generic/696 | 1 -
>  tests/generic/697 | 1 -
>  4 files changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/common/rc b/common/rc
> index bffd576a..96d65d1c 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -2639,7 +2639,7 @@ _require_idmapped_mounts()
>  		--fstype "$FSTYP"
>  
>  	if [ $? -ne 0 ]; then
> -		_notrun "vfstest not support by $FSTYP"
> +		_notrun "idmapped mounts not support by $FSTYP"
>  	fi
>  }
>  
> diff --git a/tests/generic/633 b/tests/generic/633
> index f58dbbf5..b683c427 100755
> --- a/tests/generic/633
> +++ b/tests/generic/633
> @@ -12,7 +12,6 @@ _begin_fstest auto quick atime attr cap idmapped io_uring mount perms rw unlink
>  # Import common functions.
>  . ./common/filter
>  
> -_require_idmapped_mounts
>  _require_test
>  
>  echo "Silence is golden"
> diff --git a/tests/generic/696 b/tests/generic/696
> index d2e86c96..48b3aea0 100755
> --- a/tests/generic/696
> +++ b/tests/generic/696
> @@ -17,7 +17,6 @@ _begin_fstest auto quick cap idmapped mount perms rw unlink
>  # Import common functions.
>  . ./common/filter
>  
> -_require_idmapped_mounts
>  _require_test
>  _require_scratch
>  _fixed_by_kernel_commit ac6800e279a2 \
> diff --git a/tests/generic/697 b/tests/generic/697
> index 1ce673f7..66444a95 100755
> --- a/tests/generic/697
> +++ b/tests/generic/697
> @@ -17,7 +17,6 @@ _begin_fstest auto quick cap acl idmapped mount perms rw unlink
>  . ./common/filter
>  . ./common/attr
>  
> -_require_idmapped_mounts
>  _require_test
>  _require_acls
>  _fixed_by_kernel_commit 1639a49ccdce \
> -- 
> 2.34.1
> 

Re: [PATCH v2 5/6] generic: remove incorrect _require_idmapped_mounts checks

[Amir Goldstein]

On Thu, Jun 5, 2025 at 7:24 PM Zorro Lang <zlang@redhat.com> wrote:
>
> On Tue, Jun 03, 2025 at 12:07:44PM +0200, Amir Goldstein wrote:
> > commit f5661920 ("generic: add missed _require_idmapped_mounts check")
> > wrongly adds _require_idmapped_mounts to tests that do not require
> > idmapped mounts support.
> >
> > The added _require_idmapped_mounts in test generic/633 goes against
> > commit d8dee122 ("idmapped-mounts: always run generic vfs tests")
> > that intentionally removed this requirement from the generic tests.
> >
> > The added _require_idmapped_mounts in tests generic/69{6,7} causes
> > those tests not to run with overlayfs, which does not support idmapped
> > mounts. However, those tests are regression tests to kernel commit
> > 1639a49ccdce ("fs: move S_ISGID stripping into the vfs_*() helpers")
> > which is documented as also solving a correction issue with overlayfs,
> > so removing this test converage is very much undesired.
> >
> > Remove the incorrectly added _require_idmapped_mounts checks.
> > Also fix the log in _require_idmapped_mounts to say that
> > "idmapped mounts not support by $FSTYP", which is what the helper
> > checks instead of "vfstests not support by $FSTYP" which is incorrect.
> >
> > Cc: Yang Xu <xuyang2018.jy@fujitsu.com>
> > Cc: Anthony Iliopoulos <ailiop@suse.com>
> > Cc: David Disseldorp <ddiss@suse.de>
> > Fixes: commit f5661920 ("generic: add missed _require_idmapped_mounts check")
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > ---
>
> Is this one changed anything from this ?
> https://lore.kernel.org/fstests/20250526175437.1528310-1-amir73il@gmail.com/

There is no change. I just forgot to add the RVB.

>
> Due to above link has been reviewed by Christian Brauner, do you want to
> add his RVB to this version?
>

Yes please.

Thanks,
Amir.

[PATCH v2 6/6] generic/699: fix failure with MOUNT_OPTIONS

[Amir Goldstein]

generic/699 uses overalyfs helper _overlay_mount_dirs, which is meant to
be used by overlayfs tests, where MOUNT_OPTIONS refer to overalyfs mount
options.

Using this helper from a generic test when FSTYP is not overlay is
causing undesired results. For example, when MOUNT_OPTIONS is defined
and includes a mount option not supported by overalyfs (e.g. 'acl'),
the test is notrun because of:

mount: /vdc/ovl-merge: fsconfig() failed: overlay: Unknown parameter 'acl'.

There is no other generic test that includes the common/overlay helpers
and uses them, so remove this practice from generic/699 as well.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 tests/generic/699 | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/tests/generic/699 b/tests/generic/699
index 620a40aa..2a6f857d 100755
--- a/tests/generic/699
+++ b/tests/generic/699
@@ -8,7 +8,6 @@
 # mounts specifically.
 #
 . ./common/preamble
-. ./common/overlay
 _begin_fstest auto quick perms attr idmapped mount
 
 # Override the default cleanup function.
@@ -96,20 +95,24 @@ reset_ownership()
 	stat -c '%u:%g' $path
 }
 
+setup_overlayfs()
+{
+	mkdir -p $upper $work $merge
+	_mount -t overlay -o lowerdir=$lower,upperdir=$upper,workdir=$work \
+		overlay $merge $*
+}
+
 # Prepare overlayfs with metacopy turned off.
 setup_overlayfs_idmapped_lower_metacopy_off()
 {
-	mkdir -p $upper $work $merge
-	_overlay_mount_dirs $lower $upper $work \
-			    overlay $merge -ometacopy=off || \
-			    _notrun "overlayfs doesn't support idmappped layers"
+	setup_overlayfs -ometacopy=off || \
+	    _notrun "overlayfs doesn't support idmappped layers"
 }
 
 # Prepare overlayfs with metacopy turned on.
 setup_overlayfs_idmapped_lower_metacopy_on()
 {
-	mkdir -p $upper $work $merge
-	_overlay_mount_dirs $lower $upper $work overlay $merge -ometacopy=on
+	setup_overlayfs -ometacopy=on
 }
 
 reset_overlayfs()
-- 
2.34.1

Re: [PATCH v2 6/6] generic/699: fix failure with MOUNT_OPTIONS

[Zorro Lang]

On Tue, Jun 03, 2025 at 12:07:45PM +0200, Amir Goldstein wrote:
> generic/699 uses overalyfs helper _overlay_mount_dirs, which is meant to
> be used by overlayfs tests, where MOUNT_OPTIONS refer to overalyfs mount
> options.
> 
> Using this helper from a generic test when FSTYP is not overlay is
> causing undesired results. For example, when MOUNT_OPTIONS is defined
> and includes a mount option not supported by overalyfs (e.g. 'acl'),
> the test is notrun because of:
> 
> mount: /vdc/ovl-merge: fsconfig() failed: overlay: Unknown parameter 'acl'.
> 
> There is no other generic test that includes the common/overlay helpers
> and uses them, so remove this practice from generic/699 as well.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---

Good to me,
Reviewed-by: Zorro Lang <zlang@redhat.com>

>  tests/generic/699 | 17 ++++++++++-------
>  1 file changed, 10 insertions(+), 7 deletions(-)
> 
> diff --git a/tests/generic/699 b/tests/generic/699
> index 620a40aa..2a6f857d 100755
> --- a/tests/generic/699
> +++ b/tests/generic/699
> @@ -8,7 +8,6 @@
>  # mounts specifically.
>  #
>  . ./common/preamble
> -. ./common/overlay
>  _begin_fstest auto quick perms attr idmapped mount
>  
>  # Override the default cleanup function.
> @@ -96,20 +95,24 @@ reset_ownership()
>  	stat -c '%u:%g' $path
>  }
>  
> +setup_overlayfs()
> +{
> +	mkdir -p $upper $work $merge
> +	_mount -t overlay -o lowerdir=$lower,upperdir=$upper,workdir=$work \
> +		overlay $merge $*
> +}
> +
>  # Prepare overlayfs with metacopy turned off.
>  setup_overlayfs_idmapped_lower_metacopy_off()
>  {
> -	mkdir -p $upper $work $merge
> -	_overlay_mount_dirs $lower $upper $work \
> -			    overlay $merge -ometacopy=off || \
> -			    _notrun "overlayfs doesn't support idmappped layers"
> +	setup_overlayfs -ometacopy=off || \
> +	    _notrun "overlayfs doesn't support idmappped layers"
>  }
>  
>  # Prepare overlayfs with metacopy turned on.
>  setup_overlayfs_idmapped_lower_metacopy_on()
>  {
> -	mkdir -p $upper $work $merge
> -	_overlay_mount_dirs $lower $upper $work overlay $merge -ometacopy=on
> +	setup_overlayfs -ometacopy=on
>  }
>  
>  reset_overlayfs()
> -- 
> 2.34.1
>