Re: [PATCH] fstests: btrfs: Test if btrfs can trim adjacent extents across adjacent block groups boundary

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

On Fri, Oct 25, 2019 at 11:11:37AM +0100, Filipe Manana wrote:
> On Fri, Oct 25, 2019 at 7:59 AM Qu Wenruo <wqu@suse.com> wrote:
> >
> > The test case checks if btrfs can trim adjacent extents across adjacent
> > block groups boundary correctly.
> >
> > The test case craft the following extents layout:
> >
> >          |  BG1 |      BG2        |       BG3            |
> >  Bytenr: X-8M   X      X+512M     X+1G  X+1G+128M
> >          |//////|//////|          |     |//|
> >
> > There is a long existing bug that, when btrfs is trying to trim the
> > range at [X+512M, X+1G+128M), it will only trim [X+512M, X+1G).
> >
> > This test case is the regression test for this long existing bug.
> >
> > It will verify the trimmed bytes by using loopback device backed up by a
> > file, and checking the bytes used by the file to determine how many
> > bytes are trimmed.
> >
> > Signed-off-by: Qu Wenruo <wqu@suse.com>
> > ---
> >  tests/btrfs/198     | 154 ++++++++++++++++++++++++++++++++++++++++++++
> >  tests/btrfs/198.out |   2 +
> >  tests/btrfs/group   |   1 +
> >  3 files changed, 157 insertions(+)
> >  create mode 100755 tests/btrfs/198
> >  create mode 100644 tests/btrfs/198.out
> >
> > diff --git a/tests/btrfs/198 b/tests/btrfs/198
> > new file mode 100755
> > index 00000000..851e44c1
> > --- /dev/null
> > +++ b/tests/btrfs/198
> > @@ -0,0 +1,154 @@
> > +#! /bin/bash
> > +# SPDX-License-Identifier: GPL-2.0
> > +# Copyright (C) 2019 SUSE Linux Products GmbH. All Rights Reserved.
> > +#
> > +# FS QA Test 198
> > +#
> > +# Test if btrfs discard mount option is trimming adjacent extents across
> > +# block groups boundary.
> > +# The test case uses loopback device and file used space to detect trimmed
> > +# bytes.
> > +#
> > +# There is a long existing bug that btrfs doesn't discard all space for
> > +# above mentioned case.
> > +#
> > +# The fix is: "btrfs: extent-tree: Ensure we trim ranges across block group
> > +# boundary"
> > +#
> > +seq=`basename $0`
> > +seqres=$RESULT_DIR/$seq
> > +echo "QA output created by $seq"
> > +
> > +here=`pwd`
> > +tmp=/tmp/$$
> > +status=1       # failure is the default!
> > +trap "_cleanup; exit \$status" 0 1 2 3 15
> > +
> > +_cleanup()
> > +{
> > +       cd /
> > +       umount $loop_mnt &> /dev/null
> > +       _destroy_loop_device $loop_dev &> /dev/null
> > +       rm -rf $tmp.*
> > +}
> > +
> > +# get standard environment, filters and checks
> > +. ./common/rc
> > +. ./common/filter
> > +
> > +# remove previous $seqres.full before test
> > +rm -f $seqres.full
> > +
> > +# real QA test starts here
> > +
> > +# Modify as appropriate.
> > +_supported_fs btrfs
> > +_supported_os Linux
> > +_require_fstrim
> 
> This is not needed, as it only tests if the fstrim program is available.
> And this doesn't use it, only requires the fs to supported "-o
> discard" option, which btrfs does for a very long time.
> 
> > +_require_loop
> > +_require_xfs_io_command "fiemap"
> > +
> > +# We need less than 2G data write, consider it 2G and double it just in case
> > +_require_scratch_size  $((4 * 1024 * 1024))
> > +
> > +loop_file="$SCRATCH_MNT/image"
> > +
> > +# The margin when checking trimmed size.
> > +#
> > +# The margin is for tree blocks, calculated by
> > +# 3 * max_tree_block_size
> > +# |   |- 64K
> > +# |- 3 trees get modified (root tree, extent tree, fs tree)
> > +#
> > +# In reality, there should be no margin at all due to the mount options.
> > +# But who knows what will happen in later kernels.
> > +margin_kb=$(( 3 * 64 ))
> > +trimmed_kb=$(( 768 * 1024 )) # 768M
> > +
> > +_scratch_mkfs >> $seqres.full
> > +_scratch_mount
> > +
> > +# Create a sparse file as the loopback target.
> > +# 10G size makes sure we have 1G chunk size.
> > +truncate -s 10G "$loop_file"
> > +
> > +_mkfs_dev -d SINGLE "$loop_file"
> > +
> > +loop_dev=$(_create_loop_device "$loop_file")
> > +loop_mnt=$tmp/loop_mnt
> > +
> > +mkdir -p $loop_mnt
> > +# - nospace_cache
> > +#   Since v1 cache using DATA space, it can break data extent bytenr
> > +#   continuousness.
> > +# - nodatasum
> > +#   As there will be 1.5G data write, generating 1.5M csum.
> > +#   Disabling datasum could reduce the margin caused by metadata to minimal
> > +# - discard
> > +#   What we're testing
> > +_mount -o nospace_cache,nodatasum,discard $loop_dev $loop_mnt
> > +
> > +# Craft the following extent layout:
> > +#         |  BG1 |      BG2        |       BG3            |
> > +# Bytenr: X-8M   X      X+512M     X+1G  X+1G+128M
> > +#         |//////|//////|                |//|
> > +#            V      V           V          V
> > +#            |      |           |          |- file 'tail_padding'
> > +#            |      |           |- file 'cross_boundary'
> > +#            |      |- file 'lead_padding2'
> > +#            |- file 'lead_padding1'
> > +# So that all extents of file 'cross_boundary' are all adjacent and crosses the
> > +# boundary of BG1 and BG2
> > +# File 'lead_padding1' and 'lead_padding2' are all paddings to fill the
> > +# leading gap.
> > +# File 'tail_padding' is to ensure after deleting file 'cross_boundary' we still
> > +# have used extent in BG3, to prevent trimming the whole BG3.
> > +# And since BG1 needs exactly 8M to fill, we need to sync write to ensure
> > +# the write sequence.
> > +_pwrite_byte 0xcd 0 8M $loop_mnt/lead_padding1 > /dev/null
> > +sync
> > +
> > +_pwrite_byte 0xcd 0 512M $loop_mnt/lead_padding2 > /dev/null
> > +sync
> > +_pwrite_byte 0xcd 0 $(($trimmed_kb * 1024)) $loop_mnt/cross_boundary \
> > +       > /dev/null
> > +sync
> > +
> > +_pwrite_byte 0xcd 0 1M $loop_mnt/tail_padding > /dev/null
> > +sync
> > +
> > +
> > +$XFS_IO_PROG -c "fiemap" $loop_mnt/cross_boundary >> $seqres.full
> > +# Ensure all extent are continuous
> > +# Btrfs fiemap will merge continuous results, so the output should be only
> > +# 2 lines, 1 line for filename, 1 line for a large merged fiemap result.
> > +if [ $($XFS_IO_PROG -c "fiemap" $loop_mnt/cross_boundary | wc -l) -ne 2 ]; then
> > +       _notrun "Non-continuous extent bytenr detected"
> > +fi
> > +
> > +size1_kb=$(du $loop_file| cut -f1)
> > +
> > +# Delete the file 'cross_boundary'.
> > +# This will delete $trimmed_kb data extents across the chunk boundary.
> > +rm -f $loop_mnt/cross_boundary
> > +
> > +# sync so btrfs will commit transaction and trim the freed extents
> > +sync
> > +
> > +size2_kb=$(du $loop_file | cut -f1)
> > +
> > +echo "loopback file size before discard: $size1_kb KiB" >> $seqres.full
> > +echo "loopback file size after discard:  $size2_kb KiB" >> $seqres.full
> > +echo "Expect trimmed size:               $trimmed_kb KiB" >> $seqres.full
> > +echo "Have trimmed size:                 $(($size1_kb - $size2_kb)) KiB" >> $seqres.full
> > +
> > +if [ $(($size2_kb+ $trimmed_kb)) -gt $(($size1_kb + $margin_kb)) -o \
> > +     $(($size2_kb+ $trimmed_kb)) -lt $(($size1_kb - $margin_kb)) ]; then
> > +       _fail "Btrfs doesn't trim the range correctly"
> 
> Use of _fail is discouraged and only for very special cases.
> A simple 'echo ...' is enough here to make the test fail.
> 
> Perhaps these are things that Eryu can fixup when he picks the patch,
> as they are trivial.

Sure, I can do that :)

> 
> With that,
> 
> Reviewed-by: Filipe Manana <fdmanana@suse.com>

Thanks a lot for the review!

Eryu

> 
> Thanks.
> 
> > +fi
> > +
> > +echo "Silence is golden"
> > +
> > +# success, all done
> > +status=0
> > +exit
> > diff --git a/tests/btrfs/198.out b/tests/btrfs/198.out
> > new file mode 100644
> > index 00000000..cb4c7854
> > --- /dev/null
> > +++ b/tests/btrfs/198.out
> > @@ -0,0 +1,2 @@
> > +QA output created by 198
> > +Silence is golden
> > diff --git a/tests/btrfs/group b/tests/btrfs/group
> > index ee35fa59..1b1cbbde 100644
> > --- a/tests/btrfs/group
> > +++ b/tests/btrfs/group
> > @@ -200,3 +200,4 @@
> >  195 auto volume
> >  196 auto metadata log volume
> >  197 auto replay trim
> > +198 auto trim quick
> > --
> > 2.23.0
> >
> 
> 
> -- 
> Filipe David Manana,
> 
> “Whether you think you can, or you think you can't — you're right.”