Skip to content

Commit

Permalink
btrfs: avoid defragging extents whose next extents are not targets
Browse files Browse the repository at this point in the history
[BUG]
There is a report that autodefrag is defragging single sector, which
is completely waste of IO, and no help for defragging:

   btrfs-cleaner-808 defrag_one_locked_range: root=256 ino=651122 start=0 len=4096

[CAUSE]
In defrag_collect_targets(), we check if the current range (A) can be merged
with next one (B).

If mergeable, we will add range A into target for defrag.

However there is a catch for autodefrag, when checking mergeability
against range B, we intentionally pass 0 as @newer_than, hoping to get a
higher chance to merge with the next extent.

But in the next iteration, range B will looked up by defrag_lookup_extent(),
with non-zero @newer_than.

And if range B is not really newer, it will rejected directly, causing
only range A being defragged, while we expect to defrag both range A and
B.

[FIX]
Since the root cause is the difference in check condition of
defrag_check_next_extent() and defrag_collect_targets(), we fix it by:

1. Pass @newer_than to defrag_check_next_extent()
2. Pass @extent_thresh to defrag_check_next_extent()

This makes the check between defrag_collect_targets() and
defrag_check_next_extent() more consistent.

While there is still some minor difference, the remaining checks are
focus on runtime flags like writeback/delalloc, which are mostly
transient and safe to be checked only in defrag_collect_targets().

Link: https://github.com/btrfs/linux/issues/423#issuecomment-1066981856
CC: stable@vger.kernel.org # 5.16+
Reviewed-by: Filipe Manana <fdmanana@suse.com>
Signed-off-by: Qu Wenruo <wqu@suse.com>
Signed-off-by: David Sterba <dsterba@suse.com>
  • Loading branch information
Qu Wenruo authored and David Sterba committed Mar 24, 2022
1 parent 05fd956 commit 75a36a7
Showing 1 changed file with 14 additions and 6 deletions.
20 changes: 14 additions & 6 deletions fs/btrfs/ioctl.c
Original file line number Diff line number Diff line change
Expand Up @@ -1239,7 +1239,7 @@ static u32 get_extent_max_capacity(const struct extent_map *em)
}

static bool defrag_check_next_extent(struct inode *inode, struct extent_map *em,
bool locked)
u32 extent_thresh, u64 newer_than, bool locked)
{
struct extent_map *next;
bool ret = false;
Expand All @@ -1249,11 +1249,12 @@ static bool defrag_check_next_extent(struct inode *inode, struct extent_map *em,
return false;

/*
* We want to check if the next extent can be merged with the current
* one, which can be an extent created in a past generation, so we pass
* a minimum generation of 0 to defrag_lookup_extent().
* Here we need to pass @newer_then when checking the next extent, or
* we will hit a case we mark current extent for defrag, but the next
* one will not be a target.
* This will just cause extra IO without really reducing the fragments.
*/
next = defrag_lookup_extent(inode, em->start + em->len, 0, locked);
next = defrag_lookup_extent(inode, em->start + em->len, newer_than, locked);
/* No more em or hole */
if (!next || next->block_start >= EXTENT_MAP_LAST_BYTE)
goto out;
Expand All @@ -1265,6 +1266,13 @@ static bool defrag_check_next_extent(struct inode *inode, struct extent_map *em,
*/
if (next->len >= get_extent_max_capacity(em))
goto out;
/* Skip older extent */
if (next->generation < newer_than)
goto out;
/* Also check extent size */
if (next->len >= extent_thresh)
goto out;

ret = true;
out:
free_extent_map(next);
Expand Down Expand Up @@ -1470,7 +1478,7 @@ static int defrag_collect_targets(struct btrfs_inode *inode,
goto next;

next_mergeable = defrag_check_next_extent(&inode->vfs_inode, em,
locked);
extent_thresh, newer_than, locked);
if (!next_mergeable) {
struct defrag_target_range *last;

Expand Down

0 comments on commit 75a36a7

Please sign in to comment.