Re: [PATCH v3 0/2] btrfs: defrag: bring back the old file extent search behavior and address merged extent map generation problem

[Filipe Manana <fdmanana@kernel.org>]

On Fri, Feb 11, 2022 at 02:46:11PM +0800, Qu Wenruo wrote:
> Filipe reported that the old defrag code using btrfs_search_forward() to
> do the following optimization:
> 
> - Don't cache extent maps
>   To save memory in the long run
> 
> - Skip entire file ranges which doesn't meet generation requirement
> 
> - Don't use merged extent maps which will have unreliable geneartion
> 
> The first patch will bring back the old behavior, along with the old
> optimizations.
> 
> However the 3rd problem is not that easy to solve, as data
> read/readahead can also load extent maps into the cache, and causing
> extent maps being merged.
> 
> Such already cached and merged extent maps will still confuse autodefrag,
> as if we found cached extent maps, we will not try to read them from
> disk again.
> 
> So to completely prevent merged extent maps tricking autodefrag, here
> comes the 2nd patch, to mark merged extent maps for defrag.
> 
> If we hit an merged extent, and its generation meets our requirement, we
> will not trust it but read from disk to get a reliable generation.
> 
> This should reduce defrag IO caused by the hidden extent map merging
> behavior.
> 
> Changelog:
> v2:
> - Make defrag_get_em() to be more flexiable to handle file extent
>   iteartion
>   Now it will not reject item key which is smaller than our target but
>   doesn't have the wanted type/objectid.
>   It will continue go next next instead, to prevent skipping an extent.
> 
> - Properly reduce path.slots[0]
>   There is a bug where I want to put "if (path.slots[0] == 0)" but I put
>   "if (btrfs_header_nritems(path.slots[0]))".
>   This is fixed with reworked file extent iteration code.
> 
> - Address merged extent maps properly
>   With fixed defrag_get_extent(), we can rely on it to get original em
>   from disk.
>   So what we need to do is just to ignore merged extents which meets
>   our generation requirement.
> 
> v3:
> - Rebased to latest misc-next
> 
> - Fix several generation spell typo
> 
> - Fix a case where btrfs_search_slot() can lead to path->slots[0] >=
>   nritems
> 
> - Fix the commit message on modified extent map
>   Now that part mentioning fsync() doesn't help on the autodefrag bug.
> 
> - Update the wording on extent map read from subvolume trees
> 
> Qu Wenruo (2):
>   btrfs: defrag: bring back the old file extent search behavior
>   btrfs: defrag: don't use merged extent map for their generation check

Ok, for the both patches:

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

Thanks.

> 
>  fs/btrfs/extent_map.c |   2 +
>  fs/btrfs/extent_map.h |   8 ++
>  fs/btrfs/ioctl.c      | 174 +++++++++++++++++++++++++++++++++++++++++-
>  3 files changed, 180 insertions(+), 4 deletions(-)
> 
> -- 
> 2.35.0
>