Defrag fixes for v5.15

In v5.16 btrfs had a big rework (originally considered as a refactor only) on defrag, which brings quite some regressions.

With those regressions, extra scrutiny is brought to defrag code, and some existing bugs are exposed.

For v5.15, there are those patches need to be backported:
(Tracked through github issue:
https://github.com/btrfs/linux/issues/422)

- Already upstreamed:
"btrfs: don't hold CPU for too long when defragging a file" The first patch.

- In maintainer's tree
btrfs: defrag: don't try to merge regular extents with preallocated extents
btrfs: defrag: don't defrag extents which are already at max capacity
btrfs: defrag: remove an ambiguous condition for rejection

Those will be backported when they get merged upstream.

- One special fix for v5.15
The 2nd patch.

The problem is, that patch has no upstream commit, since v5.16 reworked the defrag code and fix the problem unintentionally. It's not a good idea to bring the whole rework (along with its bugs) to stable kernel.

So here I crafted a small fix for v5.15 only.

Not sure if this is conflicted with any stable policy.

Qu Wenruo (2):
btrfs: don't hold CPU for too long when defragging a file
btrfs: defrag: use the same cluster size for defrag ioctl and
autodefrag

 fs/btrfs/ioctl.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

2.35.1

commit 2d192fc4c1abeb0d04d1c8cd54405ff4a0b0255b upstream.

There is a user report about "btrfs filesystem defrag" causing 120s timeout problem.

For btrfs_defrag_file() it will iterate all file extents if called from defrag ioctl, thus it can take a long time.

There is no reason not to release the CPU during such a long operation.

Add cond_resched() after defragged one cluster.

---
 fs/btrfs/ioctl.c | 1 +
 1 file changed, 1 insertion(+)

index 6a863b3f6de0..38a1b68c7851 100644
@@ -1581,6 +1581,7 @@ int btrfs_defrag_file(struct inode *inode, struct file *file,
 				last_len = 0;
 			}
 		}
+		cond_resched();
 	}
 
 	ret = defrag_count;

2.35.1

This commit is already in 5.15.22.

The original commit looks nothing like this commit at all. Are you sure you got this correct?

confused,

greg k-h

It most certainly is not, since it applies cleanly in 5.15.24.
The correct upstream commit of this patch is ea0eba69a2a8125229b1b6011644598039bc53aa

cheers,
Holger

Commit 2d192fc4c1abeb0d04d1c8cd54405ff4a0b0255b is in 5.15.22.

Ah, no wonder the confusion. For obvious reasons, I can not take this as-is :)

thanks,

greg k-h

My bad, wrong commit hash...

No upstream commit.
Since the bug only exists between v5.11 and v5.15. In v5.16 btrfs reworked defrag and no longer has this bug.

[BUG]
Since commit 7f458a3873ae ("btrfs: fix race when defragmenting leads to unnecessary IO") autodefrag no longer works with the following script:

mkfs.btrfs -f $dev
mount $dev $mnt -o datacow,autodefrag

# Create a layout where we have fragmented extents at [0, 64k) (sync write in

 # reserve order), then a hole at [64k, 128k)
 xfs_io -f -s -c "pwrite 48k 16k" -c "pwrite 32k 16k" \
                -c "pwrite 16k 16k" -c "pwrite 0 16k" \
                $mnt/foobar
 truncate -s 128k $mnt/foobar

echo "=== File extent layout before autodefrag ===" xfs_io -c "fiemap -v" "$mnt/foobar"

# Now trigger autodefrag, autodefrag is triggered in the cleaner thread,

 # which will be woken up by commit thread
 mount -o remount,commit=1 $mnt
 sleep 3
 sync

echo "=== File extent layout after autodefrag ===" xfs_io -c "fiemap -v" "$mnt/foobar"

The file "foobar" will not be autodefraged even it should.

[CAUSE]
Commit 7f458a3873ae ("btrfs: fix race when defragmenting leads to unnecessary IO") fixes the race by rejecting the cluster if there is any hole in the cluster.

But unlike regular defrag ioctl, autodefrag ignores the @defrag_end parameter, and always uses a fixed cluster size 256K. While defrag ioctl uses @defrag_end to skip existing holes.

This hidden autodefrag only behavior prevents autodefrag from working in above script.

[FIX]
Remove the special cluster size, and unify the behavior for both autodefrag and defrag ioctl.

This fix is only needed for v5.15 (and maybe v5.10) stable branch, as in v5.16 the whole defrag get reworked in btrfs, which at least solves this particular bug. (although introduced quite some other regressions)

---
 fs/btrfs/ioctl.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

index 38a1b68c7851..61f6e77698a2 100644
@@ -1521,13 +1521,8 @@ int btrfs_defrag_file(struct inode *inode, struct file *file,
 			continue;
 		}
 
-		if (!newer_than) {
-			cluster = (PAGE_ALIGN(defrag_end) >>
-				   PAGE_SHIFT) - i;
-			cluster = min(cluster, max_cluster);
-		} else {
-			cluster = max_cluster;
-		}
+		cluster = (PAGE_ALIGN(defrag_end) >> PAGE_SHIFT) - i;
+		cluster = min(cluster, max_cluster);
 
 		if (i + cluster > ra_index) {
 			ra_index = max(i, ra_index);

2.35.1

I'm not sure this will work as a stable patch. A backport of an existing upstream patch that is only adapted to older stable code base is fine but what is the counterpart of this patch?

The bug does no seem to be significant, autodefrag is basically a heuristic so if it does not work perfectly in all cases it's still OK.

The whole ill-fated rework on defrag.

Normally I'd say yes.

But I don't want to surprise end users by suddenly increase their IO for autodefrag in the next LTS.

This bug is really setting a high bar (or low IO expectation) for end users.

And another thing is, I can definitely create a local branch with this fixed to test against fixed autodefrag code, but that won't make much sense.

Thus getting this merged could provide a more realistic baseline for autodefrag.

Finally, one lesssen I learnt from the defrag thing is, if we allow some untested/undefined corner cases, it will bite us eventually.

So I really want autodefrag to behave just like ioctl defrag, with a pre-defined and predictable (at least not under races) behavior.

Thanks,
Qu