Re: [PATCH 2/3] btrfs: defrag: use extent_thresh to replace the hardcoded size limit

[Forza <forza@tnonline.net>]

---- From: Filipe Manana <fdmanana@kernel.org> -- Sent: 2022-01-27 - 11:58 ----

> On Thu, Jan 27, 2022 at 07:57:32AM +0800, Qu Wenruo wrote:
>> 
>> 
>> On 2022/1/26 21:37, Filipe Manana wrote:
>> > On Wed, Jan 26, 2022 at 1:00 PM Qu Wenruo <quwenruo.btrfs@gmx.com> wrote:
>> > > 
>> > > 
>> > > 
>> > > On 2022/1/26 20:36, Filipe Manana wrote:
>> > > > On Wed, Jan 26, 2022 at 12:26 PM Qu Wenruo <wqu@suse.com> wrote:
>> > > > > 
>> > > > > 
>> > > > > 
>> > > > > On 2022/1/26 19:40, Filipe Manana wrote:
>> > > > > > On Wed, Jan 26, 2022 at 08:58:49AM +0800, Qu Wenruo wrote:
>> > > > > > > In defrag_lookup_extent() we use hardcoded extent size threshold, SZ_128K,
>> > > > > > > other than @extent_thresh in btrfs_defrag_file().
>> > > > > > > 
>> > > > > > > This can lead to some inconsistent behavior, especially the default
>> > > > > > > extent size threshold is 256K.
>> > > > > > > 
>> > > > > > > Fix this by passing @extent_thresh into defrag_check_next_extent() and
>> > > > > > > use that value.
>> > > > > > > 
>> > > > > > > Also, since the extent_thresh check should be applied to all extents,
>> > > > > > > not only physically adjacent extents, move the threshold check into a
>> > > > > > > dedicate if ().
>> > > > > > > 
>> > > > > > > Signed-off-by: Qu Wenruo <wqu@suse.com>
>> > > > > > > ---
>> > > > > > >     fs/btrfs/ioctl.c | 12 +++++++-----
>> > > > > > >     1 file changed, 7 insertions(+), 5 deletions(-)
>> > > > > > > 
>> > > > > > > diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
>> > > > > > > index 0d8bfc716e6b..2911df12fc48 100644
>> > > > > > > --- a/fs/btrfs/ioctl.c
>> > > > > > > +++ b/fs/btrfs/ioctl.c
>> > > > > > > @@ -1050,7 +1050,7 @@ static struct extent_map *defrag_lookup_extent(struct inode *inode, u64 start,
>> > > > > > >     }
>> > > > > > > 
>> > > > > > >     static bool defrag_check_next_extent(struct inode *inode, struct extent_map *em,
>> > > > > > > -                                 bool locked)
>> > > > > > > +                                 u32 extent_thresh, bool locked)
>> > > > > > >     {
>> > > > > > >        struct extent_map *next;
>> > > > > > >        bool ret = false;
>> > > > > > > @@ -1066,9 +1066,11 @@ static bool defrag_check_next_extent(struct inode *inode, struct extent_map *em,
>> > > > > > >        /* Preallocated */
>> > > > > > >        if (test_bit(EXTENT_FLAG_PREALLOC, &em->flags))
>> > > > > > >                goto out;
>> > > > > > > -    /* Physically adjacent and large enough */
>> > > > > > > -    if ((em->block_start + em->block_len == next->block_start) &&
>> > > > > > > -        (em->block_len > SZ_128K && next->block_len > SZ_128K))
>> > > > > > > +    /* Extent is already large enough */
>> > > > > > > +    if (next->len >= extent_thresh)
>> > > > > > > +            goto out;
>> > > > > > 
>> > > > > > So this will trigger unnecessary rewrites of compressed extents.
>> > > > > > The SZ_128K is there to deal with compressed extents, it has nothing to
>> > > > > > do with the threshold passed to the ioctl.
>> > > > > 
>> > > > > Then there is still something wrong.
>> > > > > 
>> > > > > The original check will only reject it when both conditions are met.
>> > > > > 
>> > > > > So based on your script, I can still find a way to defrag the extents,
>> > > > > with or without this modification:
>> > > > 
>> > > > Right, without the intermediary write to file "baz", this patchset
>> > > > brings a regression in regards to
>> > > > compressed extents - when they are adjacent, which is typically the
>> > > > case when doing large writes,
>> > > > as they'll create multiple extents covering consecutive 128K ranges.
>> > > > 
>> > > > With the write to file "baz", as I pasted it, it happens before and
>> > > > after the patchset.
>> > > > 
>> > > > > 
>> > > > >           mkfs.btrfs -f $DEV
>> > > > >           mount -o compress $DEV $MNT
>> > > > > 
>> > > > >           xfs_io -f -c "pwrite -S 0xab 0 128K" $MNT/file1
>> > > > >           sync
>> > > > >           xfs_io -f -c "pwrite -S 0xab 0 128K" $MNT/file2
>> > > > >           sync
>> > > > >           xfs_io -f -c "pwrite -S 0xab 128K 128K" $MNT/file1
>> > > > >           sync
>> > > > > 
>> > > > >           echo "=== file1 before defrag ==="
>> > > > >           xfs_io -f -c "fiemap -v" $MNT/file1
>> > > > >           echo "=== file1 after defrag ==="
>> > > > >           btrfs fi defrag $MNT/file1
>> > > > >           sync
>> > > > >           xfs_io -f -c "fiemap -v" $MNT/file1
>> > > > > 
>> > > > > The output looks like this:
>> > > > > 
>> > > > > === before ===
>> > > > > /mnt/btrfs/file1:
>> > > > >     EXT: FILE-OFFSET      BLOCK-RANGE      TOTAL FLAGS
>> > > > >       0: [0..255]:        26624..26879       256   0x8
>> > > > >       1: [256..511]:      26640..26895       256   0x9
>> > > > > === after ===
>> > > > > /mnt/btrfs/file1:
>> > > > >     EXT: FILE-OFFSET      BLOCK-RANGE      TOTAL FLAGS
>> > > > >       0: [0..255]:        26648..26903       256   0x8
>> > > > >       1: [256..511]:      26656..26911       256   0x9
>> > > > > 
>> > > > > No matter if the patch is applied, the result is the same.
>> > > > 
>> > > > Yes, explained above.
>> > > > 
>> > > > > 
>> > > > > So thank you very much for finding another case we're not handling well...
>> > > > > 
>> > > > > 
>> > > > > BTW, if the check is want to reject adjacent non-compressed extent, the
>> > > > > original one is still incorrect, we can have extents smaller than 128K
>> > > > > and is still uncompressed.
>> > > > > 
>> > > > > So what we really want is to reject physically adjacent, non-compressed
>> > > > > extents?
>> > > > 
>> > > > We want to avoid doing work that does nothing.
>> > > > If 2 consecutive extents are compressed and at least one is already
>> > > > 128K, then it's a waste of time, IO and CPU.
>> > > 
>> > > So can we define the behavior like this?
>> > > 
>> > >    If the extent is already at its max capacity (compressed 128K,
>> > >     non-compressed 128M), we don't defrag it.
>> > 
>> > My previous suggestion was: if one of the extents is compressed and
>> > its size is 128K, don't include it for defrag.
>> 
>> Yep, your previous one can handling it well, I'd just want to add the
>> similar check for uncompressed one (which may be too rare to hit though)
>> 
>> > 
>> > There's probably other cases to think about: 1 compressed extent
>> > representing 100K of data, followed by another compressed extent
>> > representing 64K of data for example.
>> > In that case using both for defrag will still result in 2 extents, 1
>> > for 128K of data and another for 36K of data - still not worth it to
>> > defrag them, we end up with 2 extents, just different sizes.
>> 
>> Yes, that's also a factor to consider.
>> 
>> And with the target list we can determine how many compressed extents it
>> would result.
>> The missing piece is the number of the original extents.
>> 
>> I think this would be a target for later optimization.
> 
> Sure. Don't forget about possibly less common cases like having a mix
> of compressed extents, smaller than 128K followed with non-compressed
> extents, with sizes like in that above example, where after defrag
> we would still end up with 2 extents, just of different sizes.
> 
> It's certainly possible after cloning from a compressed file to one
> without compression and vice-versa.
> 


It may also be possible that two compressed extents with a combined size >128K would compress better when combined into a single extent, than each of them individually, and so end up fitting inside one final 128K extent. 

How advanced logic is reasonable to aim for? 

>> 
>> > 
>> > At the very least we should not regress on what we did not defrag before:
>> > 
>> > 2 extents with physically contiguous ranges, representing 128K of data
>> > each, both compressed.
>> 
>> Although the original code is not really working as it's doing block_len
>> > SZ_128K, while our maximum compressed extent size is exactly 128K.
>> 
>> I'll fix the problem first with better check and comments.
> 
> Sounds fine.
> 
> Thanks.
> 
>> 
>> Thanks,
>> Qu
>> 
>> > 
>> > Which is a very common case.
>> > 
>> > Thanks.
>> > 
>> > > 
>> > > This also means, we need to do the same check in
>> > > defrag_collect_targets() to avoid defragging such extent.
>> > > 
>> > > Thanks,
>> > > Qu
>> > > 
>> > > 
>> > > > 
>> > > > And that's a fairly common scenario. Do a one megabyte write for
>> > > > example, then after writeback we end up with several 128K extents with
>> > > > compression.
>> > > > In that case defrag should do nothing for the whole range.
>> > > > 
>> > > > 
>> > > > > 
>> > > > > Thanks,
>> > > > > Qu
>> > > > > > 
>> > > > > > After applying this patchset, if you run a trivial test like this:
>> > > > > > 
>> > > > > >       #!/bin/bash
>> > > > > > 
>> > > > > >       DEV=/dev/sdj
>> > > > > >       MNT=/mnt/sdj
>> > > > > > 
>> > > > > >       mkfs.btrfs -f $DEV
>> > > > > >       mount -o compress $DEV $MNT
>> > > > > > 
>> > > > > >       xfs_io -f -c "pwrite -S 0xab 0 128K" $MNT/foobar
>> > > > > >       sync
>> > > > > >       # Write to some other file so that the next extent for foobar
>> > > > > >       # is not contiguous with the first extent.
>> > > > > >       xfs_io -f -c "pwrite 0 128K" $MNT/baz
>> > > > > >       sync
>> > > > > >       xfs_io -f -c "pwrite -S 0xcd 128K 128K" $MNT/foobar
>> > > > > >       sync
>> > > > > > 
>> > > > > >       echo -e "\n\nTree after creating file:\n\n"
>> > > > > >       btrfs inspect-internal dump-tree -t 5 $DEV
>> > > > > > 
>> > > > > >       btrfs filesystem defragment $MNT/foobar
>> > > > > >       sync
>> > > > > > 
>> > > > > >       echo -e "\n\nTree after defrag:\n\n"
>> > > > > >       btrfs inspect-internal dump-tree -t 5 $DEV
>> > > > > > 
>> > > > > >       umount $MNT
>> > > > > > 
>> > > > > > It will result in rewriting the two 128K compressed extents:
>> > > > > > 
>> > > > > > (...)
>> > > > > > Tree after write and sync:
>> > > > > > 
>> > > > > > btrfs-progs v5.12.1
>> > > > > > fs tree key (FS_TREE ROOT_ITEM 0)
>> > > > > > (...)
>> > > > > >         item 7 key (257 INODE_REF 256) itemoff 15797 itemsize 16
>> > > > > >                 index 2 namelen 6 name: foobar
>> > > > > >         item 8 key (257 EXTENT_DATA 0) itemoff 15744 itemsize 53
>> > > > > >                 generation 6 type 1 (regular)
>> > > > > >                 extent data disk byte 13631488 nr 4096
>> > > > > >                 extent data offset 0 nr 131072 ram 131072
>> > > > > >                 extent compression 1 (zlib)
>> > > > > >         item 9 key (257 EXTENT_DATA 131072) itemoff 15691 itemsize 53
>> > > > > >                 generation 8 type 1 (regular)
>> > > > > >                 extent data disk byte 14163968 nr 4096
>> > > > > >                 extent data offset 0 nr 131072 ram 131072
>> > > > > >                 extent compression 1 (zlib)
>> > > > > > (...)
>> > > > > > 
>> > > > > > Tree after defrag:
>> > > > > > 
>> > > > > > btrfs-progs v5.12.1
>> > > > > > fs tree key (FS_TREE ROOT_ITEM 0)
>> > > > > > (...)
>> > > > > >         item 7 key (257 INODE_REF 256) itemoff 15797 itemsize 16
>> > > > > >                 index 2 namelen 6 name: foobar
>> > > > > >         item 8 key (257 EXTENT_DATA 0) itemoff 15744 itemsize 53
>> > > > > >                 generation 9 type 1 (regular)
>> > > > > >                 extent data disk byte 14430208 nr 4096
>> > > > > >                 extent data offset 0 nr 131072 ram 131072
>> > > > > >                 extent compression 1 (zlib)
>> > > > > >         item 9 key (257 EXTENT_DATA 131072) itemoff 15691 itemsize 53
>> > > > > >                 generation 9 type 1 (regular)
>> > > > > >                 extent data disk byte 13635584 nr 4096
>> > > > > >                 extent data offset 0 nr 131072 ram 131072
>> > > > > >                 extent compression 1 (zlib)
>> > > > > > 
>> > > > > > In other words, a waste of IO and CPU time.
>> > > > > > 
>> > > > > > So it needs to check if we are dealing with compressed extents, and
>> > > > > > if so, skip either of them has a size of SZ_128K (and changelog updated).
>> > > > > > 
>> > > > > > Thanks.
>> > > > > > 
>> > > > > > > +    /* Physically adjacent */
>> > > > > > > +    if ((em->block_start + em->block_len == next->block_start))
>> > > > > > >                goto out;
>> > > > > > >        ret = true;
>> > > > > > >     out:
>> > > > > > > @@ -1231,7 +1233,7 @@ static int defrag_collect_targets(struct btrfs_inode *inode,
>> > > > > > >                        goto next;
>> > > > > > > 
>> > > > > > >                next_mergeable = defrag_check_next_extent(&inode->vfs_inode, em,
>> > > > > > > -                                                      locked);
>> > > > > > > +                                                      extent_thresh, locked);
>> > > > > > >                if (!next_mergeable) {
>> > > > > > >                        struct defrag_target_range *last;
>> > > > > > > 
>> > > > > > > --
>> > > > > > > 2.34.1
>> > > > > > > 
>> > > > > > 
>> > > > >