Re: [PATCH] btrfs: defrag: add under utilized extent to defrag target list

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

On 2024/1/10 01:25, Filipe Manana wrote:
> On Fri, Jan 5, 2024 at 7:34 AM Qu Wenruo <wqu@suse.com> wrote:
>>
>> [BUG]
>> The following script can lead to a very under utilized extent and we
>> have no way to use defrag to properly reclaim its wasted space:
>>
>>    # mkfs.btrfs -f $dev
>>    # mount $dev $mnt
>>    # xfs_io -f -c "pwrite 0 128M" $mnt/foobar
>>    # sync
>>    # btrfs filesystem defrag $mnt/foobar
>>    # sync
>
> There's a missing truncate in the example.
>
>>
>> After the above operations, the file "foobar" is still utilizing the
>> whole 128M:
>>
>>          item 4 key (257 INODE_ITEM 0) itemoff 15883 itemsize 160
>>                  generation 7 transid 8 size 4096 nbytes 4096
>>                  block group 0 mode 100600 links 1 uid 0 gid 0 rdev 0
>>                  sequence 32770 flags 0x0(none)
>>          item 5 key (257 INODE_REF 256) itemoff 15869 itemsize 14
>>                  index 2 namelen 4 name: file
>>          item 6 key (257 EXTENT_DATA 0) itemoff 15816 itemsize 53
>>                  generation 7 type 1 (regular)
>>                  extent data disk byte 298844160 nr 134217728 <<<
>>                  extent data offset 0 nr 4096 ram 134217728
>>                  extent compression 0 (none)
>>
>> Meaning the expected defrag way to reclaim the space is not working.
>>
>> [CAUSE]
>> The file extent has no adjacent extent at all, thus all existing defrag
>> code consider it a perfectly good file extent, even if it's only
>> utilizing a very tiny amount of space.
>>
>> [FIX]
>> Add a special handling for under utilized extents, currently the ratio
>> is 6.25% (1/16).
>>
>> This would allow us to add such extent to our defrag target list,
>> resulting it to be properly defragged.
>>
>> Reported-by: Christoph Anton Mitterer <calestyo@scientia.org>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>>   fs/btrfs/defrag.c | 11 +++++++++++
>>   1 file changed, 11 insertions(+)
>>
>> diff --git a/fs/btrfs/defrag.c b/fs/btrfs/defrag.c
>> index c276b136ab63..cc319190b6fb 100644
>> --- a/fs/btrfs/defrag.c
>> +++ b/fs/btrfs/defrag.c
>> @@ -1070,6 +1070,17 @@ static int defrag_collect_targets(struct btrfs_inode *inode,
>>                  if (!next_mergeable) {
>>                          struct defrag_target_range *last;
>>
>> +                       /*
>> +                        * Special entry point utilization ratio under 1/16 (only
>> +                        * referring 1/16 of an on-disk extent).
>> +                        * This can happen for a truncated large extent.
>> +                        * If we don't add them, then for a truncated file
>> +                        * (may be the last 4K of a 128M extent) it will never
>
> may be -> maybe
>
>> +                        * be defraged.
>
> defraged -> defragged
>
>> +                        */
>> +                       if (em->ram_bytes < em->orig_block_len / 16)
>
> Why 1 / 16?
> For a 128M extent for example, even 1 / 2 (64M) is a lot of wasted space.
> So I think a better condition is needed, probably to consider the
> absolute value of wasted/unused space too.

The 1/16 is chosen as a trade-off between wasted space and possible
unnecessary defrag.

E.g. the file extent is only referring to 4K of a 32K extent. Doing a
defrag would not save much bytes.

I can definitely go both (ratio and absolute wasted space), but that
also means I need to find a new threshold (for wasted bytes), and I'm
not confident enough to come up another one.

>
> And this should use em->len and not em->ram_bytes. The latter is
> preserved when spitting an extent map.

Oh indeed, would definitely fix it.

Thanks,
Qu


> You can even notice this in the tree dump example from the change log
> - the file extent's items ram bytes is 128M, not 4K.
>
> Thanks.
>
>> +                               goto add;
>> +
>>                          /* Empty target list, no way to merge with last entry */
>>                          if (list_empty(target_list))
>>                                  goto next;
>> --
>> 2.43.0
>>
>>
>