Re: [PATCH v2 2/5] common/rc: add recreation support for tmpfs

["Darrick J. Wong" <djwong@kernel.org>]

On Fri, Jul 05, 2024 at 11:46:53PM +0200, Daniel Gomez (Samsung) wrote:
> On Tue, Jul 2, 2024 at 12:36 AM Darrick J. Wong <djwong@kernel.org> wrote:
> >
> > On Sun, Jun 30, 2024 at 11:52:41PM +0200, Daniel Gomez via B4 Relay wrote:
> > > From: Daniel Gomez <da.gomez@samsung.com>
> > >
> > > Add support for test device recreation (RECREATE_TEST_DEV=true) for
> > > tmpfs.
> > >
> > > Signed-off-by: Daniel Gomez <da.gomez@samsung.com>
> > > ---
> > >  common/rc | 3 +++
> > >  1 file changed, 3 insertions(+)
> > >
> > > diff --git a/common/rc b/common/rc
> > > index 163041fea..51827119c 100644
> > > --- a/common/rc
> > > +++ b/common/rc
> > > @@ -604,6 +604,9 @@ _test_mkfs()
> > >      pvfs2)
> > >       # do nothing for pvfs2
> > >       ;;
> > > +     tmpfs)
> >
> > Indentation problem here.
> 
> What's actually the format for indentation in bash scripts across
> xfstests-dev project? I see a mix of tabs and spaces everywhere. For
> this particular _test_mkfs(), I see:
> <spaces>overlay)
> <tabs># do nothing for overlay
> <tabs>;;
> <spaces>pvfs2)
> <tabs># do nothing for pvfs2
> <tabs>;;
> <spaces>udf)
> <spaces><spaces>$MKFS_UDF_PROG ...
> <tabs>;;
> <spaces>btrfs)
> <spaces><spaces>$MKFS_BTRFS_PROG...

I don't think there's a set convention anywhere, I usually just copy the
thing above it.  I was reacting to the diff; if you actually just copied
the pvfs2 clause and then s/pvfs2/tmpfs/ then I withdraw the comment.

> > > +     # do nothing for tmpfs
> >
> > If we're recreating the test filesystem, shouldn't that unmount and
> > remount for tmpfs?  Or at least rm -rf everything underneath it?  That's
> > generally the effect of _test_mkfs for disk filesystems.
> 
> I thought this was already supported. When I enable recreation, I get
> the following error:
> 
> ./check -s tmpfs_huge_always -R xunit generic/003
> SECTION       -- tmpfs_huge_always
> RECREATING    -- tmpfs on /media/test
> our local _test_mkfs routine ...
> mkfs: failed to execute mkfs.tmpfs: No such file or directory
> check: failed to mkfs $TEST_DEV using specified options
> Interrupted!
> Interrupted!
> Passed all 0 tests
> Xunit report: /result.xml
> SECTION       -- tmpfs_huge_always
> =========================
> Passed all 0 tests
> 
> So, adding the tmpfs) case to _test_mkfs() just lets recreation go
> through, and mount and umount when the test is finished. I'm not sure
> if I'm missing something or maybe I should rename this patch as a fix?

Ah!  Yes, you're right, I forgot that there is no mkfs.tmpfs, everything
is done by mount -t tmpfs.  Can you change the comment to explain why
_test_mkfs does nothing for tmpfs?

    tmpfs)
	# mount initializes the fs, no need to format anything
	;;

for dunces like me? :)

> >
> > (That said, I'm much less familiar with non-disk filesystems...)
> 
> When umounting a tmpfs mount dir, data is lost. So in the recreation
> case, _test_unmount() would take care of that.

<nod>

--D

> >
> > --D
> >
> > > +     ;;
> > >      udf)
> > >          $MKFS_UDF_PROG $MKFS_OPTIONS $* $TEST_DEV > /dev/null
> > >       ;;
> > >
> > > --
> > > 2.43.0
> > >
> > >
> > >
>