Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 23 Feb 2022 20:15:08 -0800
From:   "Darrick J. Wong" <djwong@kernel.org>
To:     Anna Schumaker <anna@kernel.org>
Cc:     fstests@vger.kernel.org,
        Linux NFS Mailing List <linux-nfs@vger.kernel.org>
Subject: Re: [PATCH 3/4] generic/578: Test that filefrag is supported before
 running
Message-ID: <20220224041508.GJ8288@magnolia>
References: <20220208215232.491780-1-anna@kernel.org>
 <20220208215232.491780-4-anna@kernel.org>
 <20220223165729.GH8288@magnolia>
 <CAFX2JfmJmeFusSEGLEtCXtNGyCW_3faCtMFXEzBKonw+rUO54A@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAFX2JfmJmeFusSEGLEtCXtNGyCW_3faCtMFXEzBKonw+rUO54A@mail.gmail.com>
Precedence: bulk

On Wed, Feb 23, 2022 at 01:24:08PM -0500, Anna Schumaker wrote:
> On Wed, Feb 23, 2022 at 11:57 AM Darrick J. Wong <djwong@kernel.org> wrote:
> >
> > On Tue, Feb 08, 2022 at 04:52:31PM -0500, Anna Schumaker wrote:
> > > From: Anna Schumaker <Anna.Schumaker@Netapp.com>
> > >
> > > NFS does not support FIBMAP/FIEMAP, so the check for non-shared extents
> > > on NFS v4.2 always fails with the message: "FIBMAP/FIEMAP unsupported".
> > > I added the _require_filefrag check for NFS and other filesystems that
> > > don't have FIEMAP or FIBMAP support.
> > >
> > > Signed-off-by: Anna Schumaker <Anna.Schumaker@Netapp.com>
> > > ---
> > >  common/rc         | 14 ++++++++++++++
> > >  tests/generic/578 |  2 +-
> > >  2 files changed, 15 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/common/rc b/common/rc
> > > index b3289de985d8..73d17da9430e 100644
> > > --- a/common/rc
> > > +++ b/common/rc
> > > @@ -4673,6 +4673,20 @@ _require_inode_limits()
> > >       fi
> > >  }
> > >
> > > +_require_filefrag()
> > > +{
> > > +     _require_command "$FILEFRAG_PROG" filefrag
> > > +
> > > +     local file="$TEST_DIR/filefrag_testfile"
> > > +
> > > +     echo "XX" > $file
> >
> > Nit: You might want to rm -f $file before echoing into it, just in case
> > some future knave ;) sets up that pathname to point to a named pipe or
> > something that will hang fstests...
> >
> > > +     ${FILEFRAG_PROG} $file 2>&1 | grep -q "FIBMAP/FIEMAP[[:space:]]*unsupported"
> > > +     if [ $? -eq 0 ]; then
> >
> > ...and rm it again here to avoid leaving test files around.
> 
> Sure, I can make that change. Should I update
> _require_filefrag_options() and _require_fibmap() to follow the same
> pattern while I'm at it? The new function was based off of those.

Yes please, and thank you! :)

--D

> Anna
> 
> >
> > Otherwise this looks ok to me.
> >
> > --D
> >
> > > +             _notrun "FIBMAP/FIEMAP not supported by this filesystem"
> > > +     fi
> > > +     rm -f $file
> > > +}
> > > +
> > >  _require_filefrag_options()
> > >  {
> > >       _require_command "$FILEFRAG_PROG" filefrag
> > > diff --git a/tests/generic/578 b/tests/generic/578
> > > index 01929a280f8c..64c813032cf8 100755
> > > --- a/tests/generic/578
> > > +++ b/tests/generic/578
> > > @@ -23,7 +23,7 @@ _cleanup()
> > >  # real QA test starts here
> > >  _supported_fs generic
> > >  _require_test_program "mmap-write-concurrent"
> > > -_require_command "$FILEFRAG_PROG" filefrag
> > > +_require_filefrag
> > >  _require_test_reflink
> > >  _require_cp_reflink
> > >
> > > --
> > > 2.35.1
> > >