Re: [PATCH] btrfs: defrag: don't try to defrag extent which is going to be written back

[Filipe Manana <fdmanana@kernel.org>]

On Mon, Feb 07, 2022 at 01:17:15PM +0800, Qu Wenruo wrote:
> In defrag_collect_targets() if we hit an extent map which is created by
> create_io_em(), it will be considered as target as its generation is
> (u64)-1, thus will pass the generation check.
> 
> Furthermore since all delalloc functions will clear EXTENT_DELALLOC,

What are delalloc functions?
This should say that once we start writeback (we run delalloc), we allocate
an extent, create an extent map point to that extent, with a generation of
(u64)-1, created the ordered extent and then clear the DELALLOC bit from the
range in the inode's io tree.

> such extent map will also pass the EXTENT_DELALLOC check.
> 
> Defragging such extent will make no sense, in fact this will cause extra
> IO as we will just re-dirty the range and submit it for writeback again,
> causing wasted IO.

defrag_prepare_one_page() will wait for the ordered extent to complete,
and after the wait, the extent map's generation is updated from (u64)-1 to
something else.

So the second pass of defrag_collect_targets() will see a generation that
is not (u64)-1.

> 
> Unfortunately this behavior seems to exist in older kernels too (v5.15
> and older), but I don't have a solid test case to prove it nor test the
> patched behavior.

This is exactly the first patch I sent François when the first report
of unusable autodefrag popped up:

https://lore.kernel.org/linux-btrfs/YeVawBBE3r6hVhgs@debian9.Home/T/#ma1c8a9848c9b7e4edb471f7be184599d38e288bb

> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  fs/btrfs/ioctl.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index 133e3e2e2e79..0ba98e1d9329 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -1353,6 +1353,10 @@ static int defrag_collect_targets(struct btrfs_inode *inode,
>  		if (em->generation < ctrl->newer_than)
>  			goto next;
>  
> +		/* This em is goging to be written back, no need to defrag */

goging -> going

Saying that it is under writeback is more correct.
By saying is "going to be", it gives the idea that writeback may have not started yet,
but an extent map with a generation of (u64)-1 is created when writeback starts.


> +		if (em->generation == (u64)-1)
> +			goto next;
> +
>  		/*
>  		 * Our start offset might be in the middle of an existing extent
>  		 * map, so take that into account.
> -- 
> 2.35.0
>