On Mon, Jun 9, 2025 at 7:26 PM Zorro Lang <zlang@xxxxxxxxxx> 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@xxxxxxxxx/
Thanks,
Amir.
