Re: [PATCH 1/2] btrfs: defrag: don't defrag extents which is already at its max capacity

[Filipe Manana <fdmanana@kernel.org>]

On Thu, Jan 27, 2022 at 01:24:42PM +0800, Qu Wenruo wrote:
> [BUG]
> For compressed extents, defrag ioctl will always try to defrag any
> compressed extents, wasting not only IO but also CPU time to
> compress/decompress:
> 
>    mkfs.btrfs -f $DEV
>    mount -o compress $DEV $MNT
>    xfs_io -f -c "pwrite -S 0xab 0 128K" $MNT/foobar
>    sync
>    xfs_io -f -c "pwrite -S 0xcd 128K 128K" $MNT/foobar
>    sync
>    echo "=== before ==="
>    xfs_io -c "fiemap -v" $MNT/foobar
>    btrfs filesystem defrag $MNT/foobar
>    sync
>    echo "=== after ==="
>    xfs_io -c "fiemap -v" $MNT/foobar
> 
> Then it shows the 2 128K extents just get CoW for no extra benefit, with
> extra IO/CPU spent:
> 
>     === before ===
>     /mnt/btrfs/file1:
>      EXT: FILE-OFFSET      BLOCK-RANGE      TOTAL FLAGS
>        0: [0..255]:        26624..26879       256   0x8
>        1: [256..511]:      26632..26887       256   0x9
>     === after ===
>     /mnt/btrfs/file1:
>      EXT: FILE-OFFSET      BLOCK-RANGE      TOTAL FLAGS
>        0: [0..255]:        26640..26895       256   0x8
>        1: [256..511]:      26648..26903       256   0x9
> 
> This affects not only v5.16 (after the defrag rework), but also v5.15
> (before the defrag rework).
> 
> [CAUSE]
> From the very beginning, btrfs defrag never checks if one extent is
> already at its max capacity (128K for compressed extents, 128M
> otherwise).
> 
> And the default extent size threshold is 256K, which is already beyond
> the compressed extent max size.
> 
> This means, by default btrfs defrag ioctl will mark all compressed
> extent which is not adjacent to a hole/preallocated range for defrag.
> 
> [FIX]
> Introduce a helper to grab the maximum extent size, and then in
> defrag_collect_targets() and defrag_check_next_extent(), reject extents
> which are already at their max capacity.
> 
> Reported-by: Filipe Manana <fdmanana@suse.com>
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  fs/btrfs/ioctl.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index 137d3732af33..a03c31e1ff18 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -1049,6 +1049,13 @@ static struct extent_map *defrag_lookup_extent(struct inode *inode, u64 start,
>  	return em;
>  }
>  
> +static u32 get_extent_max_capacity(struct extent_map *em)

Could be made const.

Minor thing apart, which can updated when the patch is picked,

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

Thanks.

> +{
> +	if (test_bit(EXTENT_FLAG_COMPRESSED, &em->flags))
> +		return BTRFS_MAX_COMPRESSED;
> +	return BTRFS_MAX_EXTENT_SIZE;
> +}
> +
>  static bool defrag_check_next_extent(struct inode *inode, struct extent_map *em,
>  				     bool locked)
>  {
> @@ -1065,6 +1072,12 @@ static bool defrag_check_next_extent(struct inode *inode, struct extent_map *em,
>  		goto out;
>  	if (test_bit(EXTENT_FLAG_PREALLOC, &em->flags))
>  		goto out;
> +	/*
> +	 * If the next extent is at its max capcity, defragging current extent
> +	 * makes no sense, as the total number of extents won't change.
> +	 */
> +	if (next->len >= get_extent_max_capacity(em))
> +		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))
> @@ -1229,6 +1242,13 @@ static int defrag_collect_targets(struct btrfs_inode *inode,
>  		if (em->len >= extent_thresh)
>  			goto next;
>  
> +		/*
> +		 * Skip extents already at its max capacity, this is mostly for
> +		 * compressed extents, which max cap is only 128K.
> +		 */
> +		if (em->len >= get_extent_max_capacity(em))
> +			goto next;
> +
>  		next_mergeable = defrag_check_next_extent(&inode->vfs_inode, em,
>  							  locked);
>  		if (!next_mergeable) {
> -- 
> 2.34.1
>