Message-ID: <be99ea4a-d036-46b3-a16b-a9348487bcc4@linux.ibm.com>
Date: Tue, 19 Nov 2024 08:38:25 +0530
From: Nirjhar Roy <nirjhar@...ux.ibm.com>
To: "Darrick J. Wong" <djwong@...nel.org>
Cc: fstests@...r.kernel.org, linux-ext4@...r.kernel.org,
        linux-xfs@...r.kernel.org, ritesh.list@...il.com,
        ojaswin@...ux.ibm.com, zlang@...nel.org
Subject: Re: [PATCH v2 2/2] generic: Addition of new tests for extsize hints


On 11/18/24 21:52, Darrick J. Wong wrote:
> 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?

Yes. I can modify the comment to make it more clear.

--NR

>
>>>> +    [[ "$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.
>
> --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
>>
>>
-- 
---
Nirjhar Roy
Linux Kernel Developer
IBM, Bangalore

Powered by blists - more mailing lists