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

Filipe Manana
2022-02-11T12:07:17Z

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
>