Re: [PATCH v3] fstests: btrfs: make nospace_cache related test cases to work with latest v2 cache

[Eryu Guan <guan@eryu.me>]

On Wed, Nov 10, 2021 at 08:13:07PM +0800, Qu Wenruo wrote:
> 
> 
> On 2021/11/10 19:01, Eryu Guan wrote:
> > On Wed, Nov 10, 2021 at 06:52:17PM +0800, Qu Wenruo wrote:
> > > 
> > > 
> > > On 2021/11/10 18:48, Eryu Guan wrote:
> > > > On Wed, Nov 10, 2021 at 05:34:17PM +0800, Qu Wenruo wrote:
> > > > > In the coming btrfs-progs v5.15 release, mkfs.btrfs will change to use
> > > > > v2 cache by default.
> > > > > 
> > > > > However nospace_cache mount option will not work with v2 cache, as it
> > > > > would make v2 cache out of sync with on-disk used space.
> > > > > 
> > > > > So mounting a btrfs with v2 cache using "nospace_cache" will make btrfs
> > > > > to reject the mount.
> > > > > 
> > > > > There are quite some test cases relying on nospace_cache to prevent v1
> > > > > cache to take up data space.
> > > > > 
> > > > > For those test cases, we no longer need the "nospace_cache" mount option
> > > > > if the filesystem is already using v2 cache.
> > > > > Since v2 cache is using metadata space, it will no longer take up data
> > > > > space, thus no extra mount options for those test cases.
> > > > > 
> > > > > By this, we can keep those existing tests to run without problem for
> > > > > both v1 and v2 cache.
> > > > > 
> > > > > Signed-off-by: Qu Wenruo <wqu@suse.com>
> > > > > ---
> > > > > Changelog:
> > > > > v2:
> > > > > - Add _scratch_no_v1_cache_opt() function
> > > > > v3:
> > > > > - Add _require_btrfs_command for _scratch_no_v1_cache_opt()
> > > > > ---
> > > > >   common/btrfs    | 11 +++++++++++
> > > > >   tests/btrfs/102 |  2 +-
> > > > >   tests/btrfs/140 |  5 ++---
> > > > >   tests/btrfs/141 |  5 ++---
> > > > >   tests/btrfs/142 |  5 ++---
> > > > >   tests/btrfs/143 |  5 ++---
> > > > >   tests/btrfs/151 |  4 ++--
> > > > >   tests/btrfs/157 |  7 +++----
> > > > >   tests/btrfs/158 |  7 +++----
> > > > >   tests/btrfs/170 |  4 ++--
> > > > >   tests/btrfs/199 |  4 ++--
> > > > >   tests/btrfs/215 |  2 +-
> > > > >   12 files changed, 33 insertions(+), 28 deletions(-)
> > > > > 
> > > > > diff --git a/common/btrfs b/common/btrfs
> > > > > index ac880bdd..e21c452c 100644
> > > > > --- a/common/btrfs
> > > > > +++ b/common/btrfs
> > > > > @@ -445,3 +445,14 @@ _scratch_btrfs_is_zoned()
> > > > >   	[ `_zone_type ${SCRATCH_DEV}` != "none" ] && return 0
> > > > >   	return 1
> > > > >   }
> > > > > +
> > > > > +_scratch_no_v1_cache_opt()
> > > > 
> > > > This name indicates it's a general helper, but it's btrfs-specific, how
> > > > about _scratch_btrfs_no_v1_cache_opt ?
> > > > 
> > > > > +{
> > > > > +	_require_btrfs_command inspect-internal dump-tree
> > > > 
> > > > This will call _notrun if btrfs command doesn't have inspect-internal
> > > > dump-tree sub-command, and _notrun will call exit, but ...
> > > > 
> > > > > +
> > > > > +	if $BTRFS_UTIL_PROG inspect-internal dump-tree $SCRATCH_DEV |\
> > > > > +	   grep -q "FREE_SPACE_TREE"; then
> > > > > +		return
> > > > > +	fi
> > > > > +	echo -n "-onospace_cache"
> > > > > +}
> > > > > diff --git a/tests/btrfs/102 b/tests/btrfs/102
> > > > > index e5a1b068..c1678b5d 100755
> > > > > --- a/tests/btrfs/102
> > > > > +++ b/tests/btrfs/102
> > > > > @@ -22,7 +22,7 @@ _scratch_mkfs >>$seqres.full 2>&1
> > > > >   # Mount our filesystem without space caches enabled so that we do not get any
> > > > >   # space used from the initial data block group that mkfs creates (space caches
> > > > >   # used space from data block groups).
> > > > > -_scratch_mount "-o nospace_cache"
> > > > > +_scratch_mount $(_scratch_no_v1_cache_opt)
> > > > 
> > > > _scratch_no_v1_cache_opt is called in a sub-shell, so the _notrun will
> > > > just exit the sub-shell, not the test itself. Should call the _require
> > > > rule in test.
> > > 
> > > That means we will have a hard dependency on binding
> > > _scratch_btrfs_no_v1_cache_opt() with _require rule then.
> > > 
> > > Then a sudden "_require_btrfs_command inspect-internal dump-tree"
> > > without context could be sometimes confusing AFAIK.
> > 
> > That's true.
> > 
> > > 
> > > Considering "inspect-internal" should be in btrfs-progs for a very long
> > > time, any non-EOF distro should have them already, can we just remove
> > > the _require rule?
> > 
> > It seems like that dump-tree sub-command was added in 2016 in v4.5, so I
> > guess I'm fine with it.
> 
> Mind to remove the _require rule at merge time?
> Or do I need to resend?

A new version is preferred, as it's not only delete the _require rule,
but also needs a function rename, and a rebase against latest master
branch. The patch currently doesn't apply due to conflit.

Thanks,
Eryu