On 28.05.19 г. 16:24 ч., Qu Wenruo wrote:
>
>
> On 2019/5/28 下午8:48, Nikolay Borisov wrote:
>>
>>
>> On 28.05.19 г. 11:18 ч., Qu Wenruo wrote:
>>> If a filesystem doesn't map its logical address space (normally the
>>> bytenr/blocknr returned by fiemap) directly to its devices(s), the
>>> following assumptions used in the test case is no longer true:
>>> - trim range start beyond the end of fs should fail
>>> - trim range start beyond the end of fs with len set should fail
>>>
>>> Under the following example, even with just one device, btrfs can still
>>> trim the fs correctly while breaking above assumption:
>>>
>>> 0 1G 1.25G
>>> |---------------|///////////////|-----------------| <- btrfs logical
>>> | address space
>>> ------------ mapped as SINGLE
>>> |
>>> 0 V 256M
>>> |///////////////| <- device address space
>>>
>>> Thus trim range start=1G len=256M will cause btrfs to trim the 256M
>>> block group, thus return correct result.
>>>
>>> Furthermore, there is no definitely behavior for whether a fs should
>>> trim the unmapped space.
>>> Btrfs currently will always trim the unmapped space, but the behavior
>>> can change as large trim can be very expensive.
>>>
>>> Despite the change to skip certain tests for btrfs, still run the
>>> following tests for btrfs:
>>> - trim start=U64_MAX with lenght set
>>> This will expose a bug that btrfs doesn't check overflow of the range.
>>> This bug will be fixed soon.
>>>
>>> - trim beyond the end of the fs
>>> This will expose a bug where btrfs could send trim command beyond the
>>> end of its device.
>>> This bug is a regression, can be fixed by reverting c2d1b3aae336 ("btrfs:
>>> Honour FITRIM range constraints during free space trim")
>>>
>>> With proper fixes for btrfs, this test case should pass on btrfs, ext4,
>>> xfs.
>>>
>>> Signed-off-by: Qu Wenruo <wqu@xxxxxxxx>
>>> ---
>>> common/rc | 11 +++++++
>>> tests/generic/260 | 75 ++++++++++++++++++++++++++++---------------
>>> tests/generic/260.out | 9 +-----
>>> 3 files changed, 62 insertions(+), 33 deletions(-)
>>>
>>> diff --git a/common/rc b/common/rc
>>> index 17b89d5d..d7a5898f 100644
>>> --- a/common/rc
>>> +++ b/common/rc
>>> @@ -4005,6 +4005,17 @@ _require_fibmap()
>>> rm -f $file
>>> }
>>>
>>> +# Check if the logical address (returned by fiemap) of a fs is 1:1 mapped to
>>> +# its underlying fs
>>
>> "underlying device" ?
>
> What's the proper expression?
>
> I mean the block device on which the fs lies above.
I got confused by the "of a fs is 1:1 mapped to its underlying fs" this
seems to have tautology and I gathered you meant if the extents as
returned by fiemap of a fs are 1:1 mapped to the underlying device that
the fs sits on ?
<snip>
>>
>> this check is rather ugly, instead if you return 0 on success (i.e it's
>> directly mapped) it can be rewritten simply as:
>>
>> if is_fs_direct_mapped; then
>>
>> which is a lot cleaner than the $() fuckery
>
> I tried that before reverting back to the echo one.
>
> The biggest concern is, "return 0" is completely OK for regular
> functions which does some work.
> But for bool return, especially for case like this _is_xxx function,
> return 0 for true is really confusing.
>
> Or this is just the preferred way in bash?
This is the the unix convention - 0 indicates success,
http://tldp.org/LDP/Bash-Beginners-Guide/html/sect_07_01.html
And https://www.tldp.org/LDP/abs/html/exit-status.html re. exit statuses.
>
>>
>>> + echo "[+] Start beyond the end of fs (should fail)" >> $seqres.full
>>> + $FSTRIM_PROG -o $beyond_eofs $SCRATCH_MNT >> $seqres.full 2>&1
>>> + [ $? -eq 0 ] && status=1
>>> +
>>> + echo "[+] Start beyond the end of fs with len set (should fail)" >> $seqres.full
>>> + $FSTRIM_PROG -o $beyond_eofs -l1M $SCRATCH_MNT >> $seqres.full 2>&1
>>> + [ $? -eq 0 ] && status=1
>>> +
>>> + # indirectly mapped fs may use this special value to trim their
>>> + # unmapped space, so don't do this for indirectly mapped fs.
>>> + echo "[+] Start = 2^64-1 (should fail)" >> $seqres.full
>>> + $FSTRIM_PROG -o $max_64bit $SCRATCH_MNT 2>&1 >> $seqres.full 2>&1
>>> + [ $? -eq 0 ] && status=1
>>> +fi
>>>
>>> -echo "[+] Start = 2^64-1 and len is set (should fail)"
>>> -out=$($FSTRIM_PROG -o $max_64bit -l1M $SCRATCH_MNT 2>&1)
>>> +# This should fail due to overflow no matter how the fs is implemented
>>> +echo "[+] Start = 2^64-1 and len is set (should fail)" >> $seqres.full
>>> +$FSTRIM_PROG -o $max_64bit -l1M $SCRATCH_MNT >> $seqres.full 2>&1
>>> [ $? -eq 0 ] && status=1
>>> -echo $out | _filter_scratch
>>>
>>> _scratch_unmount
>>> _scratch_mkfs >/dev/null 2>&1
>>> @@ -86,10 +97,12 @@ _scratch_unmount
>>> _scratch_mkfs >/dev/null 2>&1
>>> _scratch_mount
>>>
>>> +echo "[+] Trim an empty fs" >> $seqres.full
>>> # This is a bit fuzzy, but since the file system is fresh
>>> # there should be at least (fssize/2) free space to trim.
>>> # This is supposed to catch wrong FITRIM argument handling
>>> bytes=$($FSTRIM_PROG -v -o10M $SCRATCH_MNT | _filter_fstrim)
>>> +echo "$bytes trimed" >> $seqres.full
>>
>> Does it bring any value printing those strings in seqres.full given the
>> output of the executed command won't be there. I think not.
>
> When something went wrong, like no bytes get trimmed but still return 0,
> then this is definitely valuable.
>
> BTW, there are cases btrfs trimmed just 0 bytes. And it's the same command.
> To me, this can't be more valuable.
>
> In fact, I even considered to check $bytes to make the btrfs failure
> more explicit.
Fair enough but perhaps you can make
echo -n "Trim an empty fs" >> $seqres.full
exec commands
echo "$bytes trimmed". That way there is going to be a single line
only. Anyway this is a nit so feel free to ignore.
>
> Thanks,
> Qu
>
>>
>>>
>>> if [ $bytes -gt $(_math "$fssize*1024") ]; then
>>> status=1
>>> @@ -101,7 +114,7 @@ fi
>>> # It is because btrfs does not have not-yet-used parts of the device
>>> # mapped and since we got here right after the mkfs, there is not
>>> # enough free extents in the root tree.
>>> -if [ $bytes -le $(_math "$fssize*512") ] && [ $FSTYP != "btrfs" ]; then
>>> +if [ $bytes -le $(_math "$fssize*512") ] && [ $(_is_fs_direct_mapped) -eq 1 ]; then
>>
>> same thing here - make is_fs_direct_mapped plain 'return 0/1' and check
>> its ret val directly.
>>
>>> status=1
>>> echo "After the full fs discard $bytes bytes were discarded"\
>>> "however the file system is $(_math "$fssize*1024") bytes long."
>>> @@ -141,14 +154,24 @@ esac
>>> _scratch_unmount
>>> _scratch_mkfs >/dev/null 2>&1
>>> _scratch_mount
>>> +
>>> +echo "[+] Try to trim beyond the end of the fs" >> $seqres.full
>>> # It should fail since $start is beyond the end of file system
>>> -$FSTRIM_PROG -o$start -l10M $SCRATCH_MNT &> /dev/null
>>> -if [ $? -eq 0 ]; then
>>> +$FSTRIM_PROG -o$start -l10M $SCRATCH_MNT >> $seqres.full 2>&1
>>> +ret=$?
>>> +if [ $ret -eq 0 ] && [ $(_is_fs_direct_mapped) -eq 1 ]; then
>>> status=1
>>> echo "It seems that fs logic handling start"\
>>> "argument overflows"
>>> fi
>>>
>>> +# For indirectly mapped fs, it shouldn't fail.
>>> +# Btrfs will fail due to a bug in boundary check
>>> +if [ $ret -ne 0 ] && [ $(_is_fs_direct_mapped) -eq 0 ]; then
>>> + status=1
>>> + echo "Unexpected error happened during trim"
>>> +fi
>>> +
>>> _scratch_unmount
>>> _scratch_mkfs >/dev/null 2>&1
>>> _scratch_mount
>>> @@ -160,8 +183,10 @@ _scratch_mount
>>> # It is because btrfs does not have not-yet-used parts of the device
>>> # mapped and since we got here right after the mkfs, there is not
>>> # enough free extents in the root tree.
>>> +echo "[+] Try to trim the fs with large enough len" >> $seqres.full
>>> bytes=$($FSTRIM_PROG -v -l$len $SCRATCH_MNT | _filter_fstrim)
>>> -if [ $bytes -le $(_math "$fssize*512") ] && [ $FSTYP != "btrfs" ]; then
>>> +echo "$bytes trimed" >> $seqres.full
>>> +if [ $bytes -le $(_math "$fssize*512") ] && [ $(_is_fs_direct_mapped) == 1 ]; then
>>> status=1
>>> echo "It seems that fs logic handling len argument overflows"
>>> fi
>>> diff --git a/tests/generic/260.out b/tests/generic/260.out
>>> index a16c4f74..f4ee2f72 100644
>>> --- a/tests/generic/260.out
>>> +++ b/tests/generic/260.out
>>> @@ -1,12 +1,5 @@
>>> QA output created by 260
>>> -[+] Start beyond the end of fs (should fail)
>>> -fstrim: SCRATCH_MNT: FITRIM ioctl failed: Invalid argument
>>> -[+] Start beyond the end of fs with len set (should fail)
>>> -fstrim: SCRATCH_MNT: FITRIM ioctl failed: Invalid argument
>>> -[+] Start = 2^64-1 (should fail)
>>> -fstrim: SCRATCH_MNT: FITRIM ioctl failed: Invalid argument
>>> -[+] Start = 2^64-1 and len is set (should fail)
>>> -fstrim: SCRATCH_MNT: FITRIM ioctl failed: Invalid argument
>>> +[+] Optional trim range test (fs dependent)
>>> [+] Default length (should succeed)
>>> [+] Default length with start set (should succeed)
>>> [+] Length beyond the end of fs (should succeed)
>>>
