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

[Eryu Guan <eguan@linux.alibaba.com>]

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.

Thanks,
Eryu