Message-ID: <87sero8mpy.fsf@gmail.com>
Date: Mon, 18 Nov 2024 23:10:09 +0530
From: Ritesh Harjani (IBM) <ritesh.list@...il.com>
To: "Darrick J. Wong" <djwong@...nel.org>, Nirjhar Roy <nirjhar@...ux.ibm.com>
Cc: fstests@...r.kernel.org, linux-ext4@...r.kernel.org, linux-xfs@...r.kernel.org, ojaswin@...ux.ibm.com, zlang@...nel.org
Subject: Re: [PATCH v2 2/2] generic: Addition of new tests for extsize hints

"Darrick J. Wong" <djwong@...nel.org> writes:

> On Mon, Nov 18, 2024 at 02:54:06PM +0530, Nirjhar Roy wrote:
>> 
>> On 11/15/24 22:20, Darrick J. Wong wrote:
>> > On Fri, Nov 15, 2024 at 09:45:59AM +0530, Nirjhar Roy wrote:
>> > > This commit adds new tests that checks the behaviour of xfs/ext4
>> > > filesystems when extsize hint is set on file with inode size as 0, non-empty
>> > > files with allocated and delalloc extents and so on.
>> > > Although currently this test is placed under tests/generic, it
>> > > only runs on xfs and there is an ongoing patch series[1] to enable
>> > > extsize hints for ext4 as well.
>> > > 
>> > > [1] https://lore.kernel.org/linux-ext4/cover.1726034272.git.ojaswin@linux.ibm.com/
>> > > 
>> > > Reviewed-by Ritesh Harjani (IBM) <ritesh.list@...il.com>
>> > > Reviewed-by: Ojaswin Mujoo <ojaswin@...ux.ibm.com>
>> > > Suggested-by: Ojaswin Mujoo <ojaswin@...ux.ibm.com>
>> > > Signed-off-by: Nirjhar Roy <nirjhar@...ux.ibm.com>
>> > > ---
>> > >   tests/generic/366     | 172 ++++++++++++++++++++++++++++++++++++++++++
>> > >   tests/generic/366.out |  26 +++++++
>> > >   2 files changed, 198 insertions(+)
>> > >   create mode 100755 tests/generic/366
>> > >   create mode 100644 tests/generic/366.out
>> > > 
>> > > diff --git a/tests/generic/366 b/tests/generic/366
>> > > new file mode 100755
>> > > index 00000000..7ff4e8e2
>> > > --- /dev/null
>> > > +++ b/tests/generic/366
>> > > @@ -0,0 +1,172 @@
>> > > +#! /bin/bash
>> > > +# SPDX-License-Identifier: GPL-2.0
>> > > +# Copyright (c) 2024 Nirjhar Roy (nirjhar@...ux.ibm.com).  All Rights Reserved.
>> > > +#
>> > > +# FS QA Test 366
>> > > +#
>> > > +# This test verifies that extent allocation hint setting works correctly on files with
>> > > +# no extents allocated and non-empty files which are truncated. It also checks that the
>> > > +# extent hints setting fails with non-empty file i.e, with any file with allocated
>> > > +# extents or delayed allocation. We also check if the extsize value and the
>> > > +# xflag bit actually got reflected after setting/re-setting the extsize value.
>> > > +
>> > > +. ./common/config
>> > > +. ./common/filter
>> > > +. ./common/preamble
>> > > +
>> > > +_begin_fstest ioctl quick
>> > > +
>> > > +_supported_fs xfs
>> > Aren't you all adding extsize support for ext4?  I would've expected
>> > some kind of _require_extsize helper to _notrun on filesystems that
>> > don't support it.
>> Yes, this is a good idea. I will try to have something like this. Thank you.
>> > 
>> > > +
>> > > +_fixed_by_kernel_commit "2a492ff66673 \
>> > > +                        xfs: Check for delayed allocations before setting extsize"
>> > > +
>> > > +_require_scratch
>> > > +
>> > > +FILE_DATA_SIZE=1M
>> > > +
>> > > +get_default_extsize()
>> > > +{
>> > > +    if [ -z $1 ] || [ ! -d $1 ]; then
>> > > +        echo "Missing mount point argument for get_default_extsize"
>> > > +        exit 1
>> > > +    fi
>> > > +    $XFS_IO_PROG -c "extsize" "$1" | sed 's/^\[\([0-9]\+\)\].*/\1/'
>> > Doesn't this need to check for extszinherit on $SCRATCH_MNT?
>> 
>> The above function tries to get the default extsize set on a directory
>> ($SCRATCH_MNT for this test). Even if there is an extszinherit set or
>> extsize (with -d extsize=<size> [1]), the function will get the extsize (in
>> bytes) which is what the function intends to do. In case there is
>> no extszinherit or extsize set on the directory, it will return 0.  Does
>> this answer your question, or are you asking something else?
>> 
>> [1]
>> https://lore.kernel.org/all/20230929095342.2976587-7-john.g.garry@oracle.com/
>
> Nah, I think I got confused there.  Disregard the question. :(
>
>> > 
>> > > +}
>> > > +
>> > > +filter_extsz()
>> > > +{
>> > > +    sed "s/\[$1\]/\[EXTSIZE\]/g"
>> > > +}
>> > > +
>> > > +setup()
>> > > +{
>> > > +    _scratch_mkfs >> "$seqres.full"  2>&1
>> > > +    _scratch_mount >> "$seqres.full" 2>&1
>> > > +    BLKSZ=`_get_block_size $SCRATCH_MNT`
>> > > +    DEFAULT_EXTSIZE=`get_default_extsize $SCRATCH_MNT`
>> > > +    EXTSIZE=$(( BLKSZ*2 ))
>> > > +    # Making sure the new extsize is not the same as the default extsize
>> > Er... why?
>> The test behaves a bit differently when the new and old extsizes are equal
>> and the intention of this test is to check if the kernel behaves as expected
>> when we are trying to *change* the extsize. Two of the sub-tests
>> (test_data_delayed(), test_data_allocated()) test whether extsize settting
>> fails if there are allocated extents or delayed allocation. The failure
>> doesn't take place when the new and the default extsizes are equal, i.e,
>> when the extsize is not changing. If the default and the new extsize are
>> equal, the xfs_io command succeeds, which is not what we want the test to
>> do. So we are always ensuring that the new extsize is not equal to the
>> default extsize. Does this answer your question?
>
> Yep.  Can you add that ("Make sure the new extsize is not the same as
> the default extsize so that we can observe it changing") to the comment?
>
>> > > +    [[ "$DEFAULT_EXTSIZE" -eq "$EXTSIZE" ]] && EXTSIZE=$(( BLKSZ*4 ))
>> > > +}
>> > > +
>> > > +read_file_extsize()
>> > > +{
>> > > +    $XFS_IO_PROG -c "extsize" $1 | _filter_scratch | filter_extsz $2
>> > > +}
>> > > +
>> > > +check_extsz_and_xflag()
>> > > +{
>> > > +    local filename=$1
>> > > +    local extsize=$2
>> > > +    read_file_extsize $filename $extsize
>> > > +    _test_fsx_xflags_field $filename "e" && echo "e flag set" || echo "e flag unset"
>> > I almost asked in the last patch if the _test_fsxattr_flag function
>> > should be running xfs_io -c 'stat -v' so that you could grep for whole
>> > words instead of individual letters.
>> > 
>> > "extsize flag unset"
>> > 
>> > "cowextsize flag set"
>> > 
>> > is a bit easier to figure out what's going wrong.
>> > 
>> > The rest of the logic looks reasonable to me.
>> > 
>> > --D
>> 
>> Yes, that makes sense. So do you mean something like the following?
>> 
>> # Check whether a fsxattr xflags name ($2) field is set on a given file
>> ($1).
>> # e.g, fsxattr.xflags = 0x80000800 [extsize, has-xattr]
>> _test_fsxattr_flag_field()
>> {
>>     grep -q "fsxattr.xflags.*\[.*$2.*\]" <($XFS_IO_PROG -c "stat -v" "$1")
>> }
>> 
>> and the call sites can be like
>> 
>> _test_fsx_xflags_field $filename "extsize" && echo "e flag set" || echo "e
>> flag unset"
>> 
>> THE OTHER OPTION IS:
>> 
>> We can embed the "<flag name> flag set/unset" message, inside the
>> _test_fsx_xflags_field() function. Something like
>> 
>> _test_fsxattr_flag_field()
>> {
>>     grep -q "fsxattr.xflags.*\[.*$2.*\]" <($XFS_IO_PROG -c "stat -v" "$1")
>> && echo "$2 flag set" || echo "$2 flag unset"
>> }
>> 
>> Which one do you prefer?
>
> You might as well go for this second form since that's how all the
> callers behave.
>

Sorry, I missed reading the rest of the email from Nirjhar or else I
would have responded earlier.  

Adding an echo command in the common helper routine for it to go into
the .out file might become confusing. So if folks don't have a hard
preference here, then can we please keep the same previous logic of
returning success or failure from ("_test_fsxattr_xflag" common helper)
and let the caller decide whatever string it wants to add (or even not)
for it's .out file please?

-ritesh

> --D
>
>> > 
>> > > +}
>> > > +
>> > > +check_extsz_xflag_across_remount()
>> > > +{
>> > > +    local filename=$1
>> > > +    local extsize=$2
>> > > +    _scratch_cycle_mount
>> > > +    check_extsz_and_xflag $filename $extsize
>> > > +}
>> > > +
>> > > +# Extsize flag should be cleared when extsize is reset, so this function
>> > > +# checks that this behavior is followed.
>> > > +reset_extsz_and_recheck_extsz_xflag()
>> > > +{
>> > > +    local filename=$1
>> > > +    echo "Re-setting extsize hint to 0"
>> > > +    $XFS_IO_PROG -c "extsize 0" $filename
>> > > +    check_extsz_xflag_across_remount $filename "0"
>> > > +}
>> > > +
>> > > +check_extsz_xflag_before_and_after_reset()
>> > > +{
>> > > +    local filename=$1
>> > > +    local extsize=$2
>> > > +    check_extsz_xflag_across_remount $filename $extsize
>> > > +    reset_extsz_and_recheck_extsz_xflag $filename
>> > > +}
>> > > +
>> > > +test_empty_file()
>> > > +{
>> > > +    echo "TEST: Set extsize on empty file"
>> > > +    local filename=$1
>> > > +    $XFS_IO_PROG \
>> > > +        -c "open -f $filename" \
>> > > +        -c "extsize $EXTSIZE" \
>> > > +
>> > > +    check_extsz_xflag_before_and_after_reset $filename $EXTSIZE
>> > > +    echo
>> > > +}
>> > > +
>> > > +test_data_delayed()
>> > > +{
>> > > +    echo "TEST: Set extsize on non-empty file with delayed allocation"
>> > > +    local filename=$1
>> > > +    $XFS_IO_PROG \
>> > > +        -c "open -f $filename" \
>> > > +        -c "pwrite -q  0 $FILE_DATA_SIZE" \
>> > > +        -c "extsize $EXTSIZE" | _filter_scratch
>> > > +
>> > > +    echo "test for default extsize setting if any"
>> > > +    read_file_extsize $filename $DEFAULT_EXTSIZE
>> > > +    echo
>> > > +}
>> > > +
>> > > +test_data_allocated()
>> > > +{
>> > > +    echo "TEST: Set extsize on non-empty file with allocated extents"
>> > > +    local filename=$1
>> > > +    $XFS_IO_PROG \
>> > > +        -c "open -f $filename" \
>> > > +        -c "pwrite -qW  0 $FILE_DATA_SIZE" \
>> > > +        -c "extsize $EXTSIZE" | _filter_scratch
>> > > +
>> > > +    echo "test for default extsize setting if any"
>> > > +    read_file_extsize $filename $DEFAULT_EXTSIZE
>> > > +    echo
>> > > +}
>> > > +
>> > > +test_truncate_allocated()
>> > > +{
>> > > +    echo "TEST: Set extsize after truncating a file with allocated extents"
>> > > +    local filename=$1
>> > > +    $XFS_IO_PROG \
>> > > +        -c "open -f $filename" \
>> > > +        -c "pwrite -qW  0 $FILE_DATA_SIZE" \
>> > > +        -c "truncate 0" \
>> > > +        -c "extsize $EXTSIZE" \
>> > > +
>> > > +    check_extsz_xflag_across_remount $filename $EXTSIZE
>> > > +    echo
>> > > +}
>> > > +
>> > > +test_truncate_delayed()
>> > > +{
>> > > +    echo "TEST: Set extsize after truncating a file with delayed allocation"
>> > > +    local filename=$1
>> > > +    $XFS_IO_PROG \
>> > > +        -c "open -f $filename" \
>> > > +        -c "pwrite -q  0 $FILE_DATA_SIZE" \
>> > > +        -c "truncate 0" \
>> > > +        -c "extsize $EXTSIZE" \
>> > > +
>> > > +    check_extsz_xflag_across_remount $filename $EXTSIZE
>> > > +    echo
>> > > +}
>> > > +
>> > > +setup
>> > > +echo -e "EXTSIZE = $EXTSIZE DEFAULT_EXTSIZE = $DEFAULT_EXTSIZE BLOCKSIZE = $BLKSZ\n" >> "$seqres.full"
>> > > +
>> > > +NEW_FILE_NAME_PREFIX=$SCRATCH_MNT/new-file-
>> > > +
>> > > +test_empty_file "$NEW_FILE_NAME_PREFIX"00
>> > > +test_data_delayed "$NEW_FILE_NAME_PREFIX"01
>> > > +test_data_allocated "$NEW_FILE_NAME_PREFIX"02
>> > > +test_truncate_allocated "$NEW_FILE_NAME_PREFIX"03
>> > > +test_truncate_delayed "$NEW_FILE_NAME_PREFIX"04
>> > > +
>> > > +status=0
>> > > +exit
>> > > diff --git a/tests/generic/366.out b/tests/generic/366.out
>> > > new file mode 100644
>> > > index 00000000..cdd2f5fa
>> > > --- /dev/null
>> > > +++ b/tests/generic/366.out
>> > > @@ -0,0 +1,26 @@
>> > > +QA output created by 366
>> > > +TEST: Set extsize on empty file
>> > > +[EXTSIZE] SCRATCH_MNT/new-file-00
>> > > +e flag set
>> > > +Re-setting extsize hint to 0
>> > > +[EXTSIZE] SCRATCH_MNT/new-file-00
>> > > +e flag unset
>> > > +
>> > > +TEST: Set extsize on non-empty file with delayed allocation
>> > > +xfs_io: FS_IOC_FSSETXATTR SCRATCH_MNT/new-file-01: Invalid argument
>> > > +test for default extsize setting if any
>> > > +[EXTSIZE] SCRATCH_MNT/new-file-01
>> > > +
>> > > +TEST: Set extsize on non-empty file with allocated extents
>> > > +xfs_io: FS_IOC_FSSETXATTR SCRATCH_MNT/new-file-02: Invalid argument
>> > > +test for default extsize setting if any
>> > > +[EXTSIZE] SCRATCH_MNT/new-file-02
>> > > +
>> > > +TEST: Set extsize after truncating a file with allocated extents
>> > > +[EXTSIZE] SCRATCH_MNT/new-file-03
>> > > +e flag set
>> > > +
>> > > +TEST: Set extsize after truncating a file with delayed allocation
>> > > +[EXTSIZE] SCRATCH_MNT/new-file-04
>> > > +e flag set
>> > > +
>> > > -- 
>> > > 2.43.5
>> > > 
>> > > 
>> -- 
>> ---
>> Nirjhar Roy
>> Linux Kernel Developer
>> IBM, Bangalore
>> 
>> 

Powered by blists - more mailing lists