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.
>
