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

[Andrei Borzenkov <arvidjaar@gmail.com>]

On 05.01.2024 10:33, Qu Wenruo 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
> 
> 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.
> 

This sounds like the very wrong thing to do unconditionally. You have no 
knowledge of application I/O pattern and do not know whether application 
is going to fill this extent or not. Instead of de-fragmenting you are 
effectively fragmenting.

This sounds more like a target for a different operation, which may be 
called "garbage collection". But it should be explicit operation not 
tied to defragmentation (or at least explicit opt-in for defragmentation).

> 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
> +			 * be defraged.
> +			 */
> +			if (em->ram_bytes < em->orig_block_len / 16)
> +				goto add;
> +
>   			/* Empty target list, no way to merge with last entry */
>   			if (list_empty(target_list))
>   				goto next;