Re: [PATCH 1/2] btrfs/049: remove the test

Re: [PATCH 1/2] btrfs/049: remove the test

[Eryu Guan]

[cc'ed btrfs list]

On Mon, Sep 27, 2021 at 03:20:18PM +0800, Ma Xinjian wrote:
> inode_cache is deprecated and will never appear in mount
> options; remove it entirely

Please also cc btrfs list in future patches for btrfs-specific tests.

And I'd like an ack from btrfs folks.

Thanks,
Eryu

> 
> Link: https://www.spinics.net/lists/linux-btrfs/msg107910.html
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: Ma Xinjian <xinjianx.ma@intel.com>
> ---
>  tests/btrfs/049 | 85 -------------------------------------------------
>  1 file changed, 85 deletions(-)
>  delete mode 100755 tests/btrfs/049
> 
> diff --git a/tests/btrfs/049 b/tests/btrfs/049
> deleted file mode 100755
> index 87c205ca..00000000
> --- a/tests/btrfs/049
> +++ /dev/null
> @@ -1,85 +0,0 @@
> -#! /bin/bash
> -# SPDX-License-Identifier: GPL-2.0
> -# Copyright (c) 2014 Fujitsu.  All Rights Reserved.
> -#
> -# FS QA Test No. btrfs/049
> -#
> -# Regression test for btrfs inode caching vs tree log which was
> -# addressed by the following kernel patch.
> -#
> -# Btrfs: fix inode caching vs tree log
> -#
> -. ./common/preamble
> -_begin_fstest auto quick
> -
> -# Override the default cleanup function.
> -_cleanup()
> -{
> -	_cleanup_flakey
> -	rm -rf $tmp
> -}
> -
> -# Import common functions.
> -. ./common/filter
> -. ./common/dmflakey
> -
> -# real QA test starts here
> -_supported_fs btrfs
> -_require_scratch
> -_require_dm_target flakey
> -# Zoned btrfs does not support inode cache
> -_require_non_zoned_device "$SCRATCH_DEV"
> -
> -_scratch_mkfs >> $seqres.full 2>&1
> -
> -SAVE_MOUNT_OPTIONS="$MOUNT_OPTIONS"
> -MOUNT_OPTIONS="$MOUNT_OPTIONS -o inode_cache,commit=100"
> -
> -# create a basic flakey device that will never error out
> -_init_flakey
> -_mount_flakey
> -
> -_get_inode_id()
> -{
> -	local inode_id
> -	inode_id=`stat $1 | grep Inode: | $AWK_PROG '{print $4}'`
> -	echo $inode_id
> -}
> -
> -$XFS_IO_PROG -f -c "pwrite 0 10M" -c "fsync" \
> -	$SCRATCH_MNT/data >& /dev/null
> -
> -inode_id=`_get_inode_id "$SCRATCH_MNT/data"`
> -rm -f $SCRATCH_MNT/data
> -
> -for i in `seq 1 5`;
> -do
> -	mkdir $SCRATCH_MNT/dir_$i
> -	new_inode_id=`_get_inode_id $SCRATCH_MNT/dir_$i`
> -	if [ $new_inode_id -eq $inode_id ]
> -	then
> -		$XFS_IO_PROG -f -c "pwrite 0 1M" -c "fsync" \
> -			$SCRATCH_MNT/dir_$i/data1 >& /dev/null
> -		_load_flakey_table 1
> -		_unmount_flakey
> -		need_umount=1
> -		break
> -	fi
> -	sleep 1
> -done
> -
> -# restore previous mount options
> -export MOUNT_OPTIONS="$SAVE_MOUNT_OPTIONS"
> -
> -# ok mount so that any recovery that needs to happen is done
> -if [ $new_inode_id -eq $inode_id ];then
> -	_load_flakey_table $FLAKEY_ALLOW_WRITES
> -	_mount_flakey
> -	_unmount_flakey
> -fi
> -
> -# make sure we got a valid fs after replay
> -_check_scratch_fs $FLAKEY_DEV
> -
> -status=0
> -exit
> -- 
> 2.20.1

Re: [PATCH 1/2] btrfs/049: remove the test

[David Sterba]

On Sun, Oct 10, 2021 at 09:32:20PM +0800, Eryu Guan wrote:
> [cc'ed btrfs list]
> 
> On Mon, Sep 27, 2021 at 03:20:18PM +0800, Ma Xinjian wrote:
> > inode_cache is deprecated and will never appear in mount
> > options; remove it entirely
> 
> Please also cc btrfs list in future patches for btrfs-specific tests.
> 
> And I'd like an ack from btrfs folks.

Acked-by: David Sterba <dsterba@suse.com>

Re: [PATCH 2/2] btrfs/091: remove noinode_cache option

[Eryu Guan]

On Mon, Sep 27, 2021 at 03:20:19PM +0800, Ma Xinjian wrote:
> inode cache feature has been removed
> 
> Link: https://www.spinics.net/lists/linux-btrfs/msg107910.html

From above link, the inode cache feathre has been removed since v5.11
kernel, which is relatively a recent kernel,  but old kernels may still
need this noinode_cache mount option.  So we'd better to keep this
check.

We could add check for inode_cache option as well, and only check for
noinode_cache when inode cache is supported.

> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: Ma Xinjian <xinjianx.ma@intel.com>
> ---
>  tests/btrfs/091 | 21 +--------------------
>  1 file changed, 1 insertion(+), 20 deletions(-)
> 
> diff --git a/tests/btrfs/091 b/tests/btrfs/091
> index 307289b1..f2cd00b2 100755
> --- a/tests/btrfs/091
> +++ b/tests/btrfs/091
> @@ -23,21 +23,12 @@ _require_cp_reflink
>  # use largest node/leaf size (64K) to allow the test to be run on arch with
>  # page size > 4k.
>  NODESIZE=65536
> -SUPPORT_NOINODE_CACHE="yes"
>  
>  run_check _scratch_mkfs "--nodesize $NODESIZE"
>  
> -# inode cache will also take space in fs tree, disable them to get consistent
> -# result.
>  # discard error output since we will check return value manually.
>  # also disable all compression, or output will mismatch with golden output
> -_try_scratch_mount "-o noinode_cache,compress=no,compress-force=no" 2> /dev/null
> -
> -# Check for old kernel which doesn't support 'noinode_cache' mount option
> -if [ $? -ne 0 ]; then
> -	support_noinode_cache="no"

This seems like a type in the original code, it should be in upper case.

Thanks,
Eryu

> -	_scratch_mount
> -fi
> +_try_scratch_mount "-o compress=no,compress-force=no" 2> /dev/null
>  
>  _run_btrfs_util_prog subvolume create $SCRATCH_MNT/subv1
>  _run_btrfs_util_prog subvolume create $SCRATCH_MNT/subv2
> @@ -46,16 +37,6 @@ _run_btrfs_util_prog subvolume create $SCRATCH_MNT/subv3
>  _run_btrfs_util_prog quota enable $SCRATCH_MNT
>  _run_btrfs_util_prog quota rescan -w $SCRATCH_MNT
>  
> -# if we don't support noinode_cache mount option, then we should double check
> -# whether inode cache is enabled before executing the real test payload.
> -if [ $SUPPORT_NOINODE_CACHE == "no" ]; then
> -	EMPTY_SIZE=`$BTRFS_UTIL_PROG qgroup show $units $SCRATCH_MNT | \
> -		$SED_PROG -n '/[0-9]/p' | $AWK_PROG '{print $2}' | head -n1`
> -	if [ $EMPTY_SIZE != $NODESIZE ]; then
> -		_notrun "Kernel doesn't support to disable inode cache"
> -	fi
> -fi
> -
>  $XFS_IO_PROG -f -c "pwrite 0 256K" $SCRATCH_MNT/subv1/file1 | _filter_xfs_io
>  cp --reflink $SCRATCH_MNT/subv1/file1 $SCRATCH_MNT/subv2/file1
>  cp --reflink $SCRATCH_MNT/subv1/file1 $SCRATCH_MNT/subv3/file1
> -- 
> 2.20.1

Re: [PATCH 2/2] btrfs/091: remove noinode_cache option

[David Sterba]

On Sun, Oct 24, 2021 at 09:34:15PM +0800, Eryu Guan wrote:
> On Mon, Sep 27, 2021 at 03:20:19PM +0800, Ma Xinjian wrote:
> > inode cache feature has been removed
> > 
> > Link: https://www.spinics.net/lists/linux-btrfs/msg107910.html
> 
> >From above link, the inode cache feathre has been removed since v5.11
> kernel, which is relatively a recent kernel,  but old kernels may still
> need this noinode_cache mount option.  So we'd better to keep this
> check.
> 
> We could add check for inode_cache option as well, and only check for
> noinode_cache when inode cache is supported.

I don't think we should keep the option support in the testsuite. There
was deprecation period before actual removal from kernel and long before
that we discouraged everybody from using it. There were known bugs and I
don't think anybody cares for old kernels anyway.