btrfs: defrag: don't waste CPU time on non-target extent
Subsystems (2)
v4 cover btrfs: defrag: don't waste CPU time on non-target extent
Changelog:
v2:
- Rebased to lastest misc-next
Just one small conflict with static_assert() update.
And this time only those patches are rebased to misc-next, thus it may cause conflicts with fixes for defrag_check_next_extent() in the future.
- Several grammar fixes
- Report accurate btrfs_defrag_ctrl::sectors_defragged This is inspired by a comment from Filipe that the skip check should be done in the defrag_collect_targets() call inside defrag_one_range().
This results a new patch in v2.
- Change the timing of btrfs_defrag_ctrl::last_scanned update Now it's updated inside defrag_one_range(), which will give us an accurate view, unlike the previous call site in defrag_one_cluster().
- Don't change the timing of extent threshold.
- Rename @last_target to @last_is_target in defrag_collect_targets()
v3:
- Add Reviewed-by tags
- Fix a wrong value in commit message of the 1st patch
- Make @orig_start const for the 3rd patch
- Fix a missing word "skip" in the 5th patch
- Remove one unnecessary assignment in the 5th patch As we don't return the defragged sectors to user space.
v4:
- Move the skip behavior before the btrfs_defrag_ctrl refactor So it can be backported to v5.16
- Slightly change how we use btrfs_defrag_ctrl::last_scanned Previously we use that value almost as range start, but that's not correct, as defrag_collect_targets() can update it before we do any defrag.
Now we pass explicit range start/len to defrag_one_cluster(), without re-using ctrl::last_scanned.
- Remove one incorrect range skip in defrag_one_cluster() There used to be a check against last_scanned to skip certain ranges. But that is not correct, as the last_scanned can already be larger than the first target after the 1st defrag_collect_targets() call.
Now last_scanned is only really utilized after one cluster is defragged.
This also removes the reviewed-by tag for the refactor patch
Qu Wenruo (5):
btrfs: defrag: allow defrag_one_cluster() to skip large extent which
is not a target
btrfs: uapi: introduce BTRFS_DEFRAG_RANGE_MASK for later sanity check
btrfs: defrag: introduce btrfs_defrag_ctrl structure for later usage
btrfs: defrag: use btrfs_defrag_ctrl to replace
btrfs_ioctl_defrag_range_args for btrfs_defrag_file()
btrfs: defrag: make btrfs_defrag_file() to report accurate number of
defragged sectors
fs/btrfs/ctree.h | 22 +++- fs/btrfs/file.c | 21 ++-- fs/btrfs/ioctl.c | 200 ++++++++++++++++++++++--------------- include/uapi/linux/btrfs.h | 6 +- 4 files changed, 155 insertions(+), 94 deletions(-)
Signature · 1 line
2.35.0
PATCH v4 1/5 btrfs: defrag: allow defrag_one_cluster() to skip large extent which is not a target
In the rework of btrfs_defrag_file(), we always call defrag_one_cluster() and increase the offset by cluster size, which is only 256K.
But there are cases where we have a large extent (e.g. 128M) which doesn't need to be defragged at all.
Before the refactor, we can directly skip the range, but now we have to scan that extent map again and again until the cluster moves after the non-target extent.
Fix the problem by allow defrag_one_cluster() to increase btrfs_defrag_ctrl::last_scanned to the end of an extent, if and only if the last extent of the cluster is not a target.
The test script looks like this:
mkfs.btrfs -f $dev > /dev/null mount $dev $mnt # As btrfs ioctl uses 32M as extent_threshold xfs_io -f -c "pwrite 0 64M" $mnt/file1 sync # Some fragemented range to defrag xfs_io -s -c "pwrite 65548k 4k" \ -c "pwrite 65544k 4k" \ -c "pwrite 65540k 4k" \ -c "pwrite 65536k 4k" \ $mnt/file1 sync echo "=== before ===" xfs_io -c "fiemap -v" $mnt/file1 echo "=== after ===" btrfs fi defrag $mnt/file1 sync xfs_io -c "fiemap -v" $mnt/file1 umount $mnt
With extra ftrace put into defrag_one_cluster(), before the patch it would result tons of loops:
(As defrag_one_cluster() is inlined, the function name is its caller)
btrfs-126062 [005] ..... 4682.816026: btrfs_defrag_file: r/i=5/257 start=0 len=262144 btrfs-126062 [005] ..... 4682.816027: btrfs_defrag_file: r/i=5/257 start=262144 len=262144 btrfs-126062 [005] ..... 4682.816028: btrfs_defrag_file: r/i=5/257 start=524288 len=262144 btrfs-126062 [005] ..... 4682.816028: btrfs_defrag_file: r/i=5/257 start=786432 len=262144 btrfs-126062 [005] ..... 4682.816028: btrfs_defrag_file: r/i=5/257 start=1048576 len=262144
...
btrfs-126062 [005] ..... 4682.816043: btrfs_defrag_file: r/i=5/257 start=67108864 len=262144
But with this patch there will be just one loop, then directly to the end of the extent:
btrfs-130471 [014] ..... 5434.029558: defrag_one_cluster: r/i=5/257 start=0 len=262144 btrfs-130471 [014] ..... 5434.029559: defrag_one_cluster: r/i=5/257 start=67108864 len=16384
--- fs/btrfs/ioctl.c | 50 ++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 40 insertions(+), 10 deletions(-)
fs/btrfs/ioctl.c +40 -10
@@ -1204,9 +1204,11 @@ struct defrag_target_range {*/ static int defrag_collect_targets (struct btrfs_inode *inode,u64 start,u64 len,u32 extent_thresh, -u64 newer_than,bool do_compress, -bool locked,struct list_head *target_list) +u64 newer_than,bool do_compress,bool locked, +struct list_head *target_list, +u64 *last_scanned_ret) { +bool last_is_target = false;u64 cur = start;int ret =0 ;
@@ -1216,6 +1218,7 @@ static int defrag_collect_targets(struct btrfs_inode *inode,bool next_mergeable = true;u64 range_len; + last_is_target = false; em =defrag_lookup_extent (&inode->vfs_inode, cur, locked);if (!em)break ;
@@ -1298,6 +1301,7 @@ static int defrag_collect_targets(struct btrfs_inode *inode, } add: + last_is_target = true; range_len =min (extent_map_end (em), start + len) - cur;/* * This one is a good target, check if it can be merged into
@@ -1341,6 +1345,17 @@ static int defrag_collect_targets(struct btrfs_inode *inode,kfree (entry); } } +if (!ret && last_scanned_ret) { +/* +* If the last extent is not a target, the caller can skip to +* the end of that extent. +* Otherwise, we can only go the end of the spcified range. +*/ +if (!last_is_target) + *last_scanned_ret =max (cur, *last_scanned_ret); +else + *last_scanned_ret =max (start + len, *last_scanned_ret); + }return ret; }
@@ -1400,7 +1415,8 @@ static int defrag_one_locked_target(struct btrfs_inode *inode, }static int defrag_one_range (struct btrfs_inode *inode,u64 start,u32 len, -u32 extent_thresh,u64 newer_than,bool do_compress) +u32 extent_thresh,u64 newer_than,bool do_compress, +u64 *last_scanned_ret) {struct extent_state *cached_state =NULL ;struct defrag_target_range *entry;
@@ -1446,7 +1462,7 @@ static int defrag_one_range(struct btrfs_inode *inode, u64 start, u32 len,*/ ret =defrag_collect_targets (inode, start, len, extent_thresh, newer_than, do_compress, true, - &target_list); + &target_list, last_scanned_ret);if (ret <0 ) goto unlock_extent;
@@ -1481,7 +1497,8 @@ static int defrag_one_cluster(struct btrfs_inode *inode, u64 start, u32 len, u32 extent_thresh, u64 newer_than, bool do_compress, unsigned long *sectors_defragged, - unsigned long max_sectors) + unsigned long max_sectors, + u64 *last_scanned_ret) { const u32 sectorsize = inode->root->fs_info->sectorsize;struct defrag_target_range *entry;
@@ -1491,7 +1508,7 @@ static int defrag_one_cluster(struct btrfs_inode *inode, ret =defrag_collect_targets (inode, start, len, extent_thresh, newer_than, do_compress, false, - &target_list); + &target_list,NULL );if (ret <0 ) goto out;
@@ -1508,6 +1525,15 @@ static int defrag_one_cluster(struct btrfs_inode *inode, range_len =min_t (u32, range_len, (max_sectors - *sectors_defragged) * sectorsize); +/* +* If defrag_one_range() has updated last_scanned_ret, +* our range may already be invalid (e.g. hole punched). +* Skip if our range is before last_scanned_ret, as there is +* no need to defrag the range anymore. +*/ +if (entry->start + range_len <= *last_scanned_ret) +continue ; +if (ra) page_cache_sync_readahead(inode->vfs_inode.i_mapping, ra, NULL, entry->start >> PAGE_SHIFT,
@@ -1520,7 +1546,8 @@ static int defrag_one_cluster(struct btrfs_inode *inode,* accounting. */ ret =defrag_one_range (inode, entry->start, range_len, - extent_thresh, newer_than, do_compress); + extent_thresh, newer_than, do_compress, + last_scanned_ret);if (ret <0 )break ; *sectors_defragged += range_len >>
@@ -1531,6 +1558,8 @@ static int defrag_one_cluster(struct btrfs_inode *inode,list_del_init (&entry->list);kfree (entry); } +if (ret >=0 ) + *last_scanned_ret =max (*last_scanned_ret, start + len);return ret; }
@@ -1616,6 +1645,7 @@ int btrfs_defrag_file(struct inode *inode, struct file_ra_state *ra,while (cur < last_byte) {const unsigned long prev_sectors_defragged = sectors_defragged; +u64 last_scanned = cur;u64 cluster_end;if (btrfs_defrag_cancelled (fs_info)) {
@@ -1642,8 +1672,8 @@ int btrfs_defrag_file(struct inode *inode, struct file_ra_state *ra,BTRFS_I (inode)->defrag_compress = compress_type; ret =defrag_one_cluster (BTRFS_I (inode), ra, cur, cluster_end +1 - cur, extent_thresh, - newer_than, do_compress, - §ors_defragged, max_to_defrag); + newer_than, do_compress, §ors_defragged, + max_to_defrag, &last_scanned);if (sectors_defragged > prev_sectors_defragged)balance_dirty_pages_ratelimited (inode->i_mapping);
@@ -1651,7 +1681,7 @@ int btrfs_defrag_file(struct inode *inode, struct file_ra_state *ra,btrfs_inode_unlock (inode,0 );if (ret <0 )break ; - cur = cluster_end +1 ; + cur =max (cluster_end +1 , last_scanned);if (ret >0 ) { ret =0 ;break ;
Signature · 1 line
2.35.0
PATCH v4 2/5 btrfs: uapi: introduce BTRFS_DEFRAG_RANGE_MASK for later sanity check
In reply to Qu WenruoAnd since we're here, replace the hardcoded bit flags (1, 2) with (1UL << 0) and (1UL << 1), respectively.
--- include/uapi/linux/btrfs.h | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
include/uapi/linux/btrfs.h +4 -2
@@ -576,8 +576,10 @@ struct btrfs_ioctl_clone_range_args {* Used by: * struct btrfs_ioctl_defrag_range_args.flags */ -#define BTRFS_DEFRAG_RANGE_COMPRESS 1 -#define BTRFS_DEFRAG_RANGE_START_IO 2 +#define BTRFS_DEFRAG_RANGE_COMPRESS (1UL << 0) +#define BTRFS_DEFRAG_RANGE_START_IO (1UL << 1) +#define BTRFS_DEFRAG_RANGE_FLAGS_MASK (BTRFS_DEFRAG_RANGE_COMPRESS |\ + BTRFS_DEFRAG_RANGE_START_IO)struct btrfs_ioctl_defrag_range_args {/* start of the defrag operation */ __u64 start;
Signature · 1 line
2.35.0
PATCH v4 3/5 btrfs: defrag: introduce btrfs_defrag_ctrl structure for later usage
In reply to Qu WenruoCurrently btrfs_defrag_file() accepts not only btrfs_ioctl_defrag_range_args but also other parameters like @newer_than and @max_sectors_to_defrag for extra policies.
Those extra values are hidden from defrag ioctl and even caused bugs in the past due to different behaviors based on those extra values.
Here we introduce a new structure, btrfs_defrag_ctrl, to include:
- all members in btrfs_ioctl_defrag_range_args
- @max_sectors_to_defrag and @newer_than
- Extra values which callers of btrfs_defrag_file() may care Like @sectors_defragged and @last_scanned.
With the new structure, also introduce a new helper, btrfs_defrag_ioctl_args_to_ctrl() to:
- Do extra sanity check on @compress and @flags
- Do range alignment when possible
- Set default values.
--- fs/btrfs/ctree.h | 19 +++++++++++++++++++ fs/btrfs/ioctl.c | 41 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 60 insertions(+)
fs/btrfs/ctree.h +19 -0
@@ -3318,6 +3318,25 @@ int btrfs_fileattr_set(struct user_namespace *mnt_userns,int btrfs_ioctl_get_supported_features (void __user *arg);void btrfs_sync_inode_flags_to_i_flags (struct inode *inode);int __pure btrfs_is_empty_uuid (u8 *uuid); + +struct btrfs_defrag_ctrl { +/* Input, read-only fields */ +u64 start; +u64 len; +u32 extent_thresh; +u64 newer_than; +u64 max_sectors_to_defrag; +u8 compress; +u8 flags; + +/* Output fields */ +u64 sectors_defragged; +u64 last_scanned;/* Exclusive bytenr */ +}; +int btrfs_defrag_ioctl_args_to_ctrl (struct btrfs_fs_info *fs_info, +struct btrfs_ioctl_defrag_range_args *args, +struct btrfs_defrag_ctrl *ctrl, +u64 max_sectors_to_defrag,u64 newer_than);int btrfs_defrag_file (struct inode *inode,struct file_ra_state *ra,struct btrfs_ioctl_defrag_range_args *range,u64 newer_than,unsigned long max_to_defrag);
fs/btrfs/ioctl.c +41 -0
47 lines not on this page · raw message
Signature · 1 line
2.35.0
PATCH v4 4/5 btrfs: defrag: use btrfs_defrag_ctrl to replace btrfs_ioctl_defrag_range_args for btrfs_defrag_file()
In reply to Qu WenruoThis brings the following benefits:
- No more strange range->start update to indicate last scanned bytenr We have btrfs_defrag_ctrl::last_scanned (exclusive) for it directly.
- No more return value to indicate defragged sectors Now btrfs_defrag_file() will just return 0 if no error happened. And btrfs_defrag_ctrl::sectors_defragged will show that value.
- Less parameters to carry around
Now most defrag_* functions only need to fetch its policy parameters from btrfs_defrag_ctrl directly.
--- fs/btrfs/ctree.h | 3 +- fs/btrfs/file.c | 21 +++--- fs/btrfs/ioctl.c | 165 ++++++++++++++++++++--------------------------- 3 files changed, 82 insertions(+), 107 deletions(-)
fs/btrfs/ctree.h +1 -2
@@ -3338,8 +3338,7 @@ int btrfs_defrag_ioctl_args_to_ctrl(struct btrfs_fs_info *fs_info,struct btrfs_defrag_ctrl *ctrl, u64 max_sectors_to_defrag, u64 newer_than);int btrfs_defrag_file (struct inode *inode,struct file_ra_state *ra, -struct btrfs_ioctl_defrag_range_args *range, -u64 newer_than,unsigned long max_to_defrag); +struct btrfs_defrag_ctrl *ctrl);void btrfs_get_block_group_info (struct list_head *groups_list,struct btrfs_ioctl_space_info *space); void btrfs_update_ioctl_balance_args(struct btrfs_fs_info *fs_info,
fs/btrfs/file.c +11 -10
@@ -277,8 +277,7 @@ static int __btrfs_run_defrag_inode(struct btrfs_fs_info *fs_info, {struct btrfs_root *inode_root;struct inode *inode; -struct btrfs_ioctl_defrag_range_args range; -int num_defrag; +struct btrfs_defrag_ctrl ctrl = {0 };int ret;/* get the inode */
@@ -297,21 +296,23 @@ static int __btrfs_run_defrag_inode(struct btrfs_fs_info *fs_info,/* do a chunk of defrag */ clear_bit (BTRFS_INODE_IN_DEFRAG , &BTRFS_I (inode)->runtime_flags); -memset (&range,0 ,sizeof (range)); - range.len = (u64)-1 ; - range.start = defrag->last_offset; + ctrl.len = (u64)-1 ; + ctrl.start = defrag->last_offset; + ctrl.newer_than = defrag->transid; + ctrl.max_sectors_to_defrag =BTRFS_DEFRAG_BATCH ;sb_start_write (fs_info->sb); - num_defrag =btrfs_defrag_file (inode,NULL , &range, defrag->transid, -BTRFS_DEFRAG_BATCH ); + ret =btrfs_defrag_file (inode,NULL , &ctrl);sb_end_write (fs_info->sb); +if (ret <0 ) + goto out;/* * if we filled the whole defrag batch, there * must be more work to do. Queue this defrag * again */ -if (num_defrag ==BTRFS_DEFRAG_BATCH ) { - defrag->last_offset = range.start; +if (ctrl.sectors_defragged ==BTRFS_DEFRAG_BATCH ) { + defrag->last_offset = ctrl.last_scanned;btrfs_requeue_inode_defrag (BTRFS_I (inode), defrag); }else if (defrag->last_offset && !defrag->cycled) {/*
@@ -325,7 +326,7 @@ static int __btrfs_run_defrag_inode(struct btrfs_fs_info *fs_info, }else {kmem_cache_free (btrfs_inode_defrag_cachep, defrag); } - +out:iput (inode);return 0 ; cleanup:
fs/btrfs/ioctl.c +70 -95
334 lines not on this page · raw message
Signature · 1 line
2.35.0
PATCH v4 5/5 btrfs: defrag: make btrfs_defrag_file() to report accurate number of defragged sectors
In reply to Qu WenruoPreviously rework btrfs_defrag_file() can only report the number of sectors from the first run of defrag_collect_targets().
This number is not accurate as if holes are punched after the first defrag_collect_targets() call, we will not choose to defrag the holes.
Originally this is to avoid passing @sectors_defragged to every involved functions.
But now since we have btrfs_defrag_ctrl, there is no need to do such inaccurate accounting, just update btrfs_defrag_ctrl::sectors_defragged after a successful defrag_one_locked_target() call.
--- fs/btrfs/ioctl.c | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-)
fs/btrfs/ioctl.c +2 -8
@@ -1471,6 +1471,8 @@ static int defrag_one_range(struct btrfs_inode *inode, &cached_state);if (ret <0 )break ; + ctrl->sectors_defragged += entry->len >> + inode->root->fs_info->sectorsize_bits; }list_for_each_entry_safe (entry, tmp, &target_list, list) {
@@ -1535,17 +1537,9 @@ static int defrag_one_cluster(struct btrfs_inode *inode, ra,NULL , entry->start >>PAGE_SHIFT , ((entry->start + range_len -1 ) >>PAGE_SHIFT ) - (entry->start >> PAGE_SHIFT) + 1); -/* -* Here we may not defrag any range if holes are punched before -* we locked the pages. -* But that's fine, it only affects the @sectors_defragged -* accounting. -*/ ret =defrag_one_range (inode, ctrl, entry->start, range_len);if (ret <0 )break ; - ctrl->sectors_defragged += range_len >> - inode->root->fs_info->sectorsize_bits; } out:list_for_each_entry_safe (entry, tmp, &target_list, list) {
Signature · 1 line
2.35.0
Quoted 33 lines · Qu Wenruo
Changelog:
v2:
- Rebased to lastest misc-next
Just one small conflict withstatic_assert()update.
And this time only those patches are rebased to misc-next, thus it may cause conflicts with fixes fordefrag_check_next_extent()in the future.- Several grammar fixes
- Report accurate
btrfs_defrag_ctrl::sectors_defraggedThis is inspired by a comment from Filipe that the skip check should be done in thedefrag_collect_targets()call insidedefrag_one_range().This results a new patch in v2.
- Change the timing of
btrfs_defrag_ctrl::last_scannedupdate Now it's updated insidedefrag_one_range(), which will give us an accurate view, unlike the previous call site indefrag_one_cluster().- Don't change the timing of extent threshold.
- Rename @last_target to @last_is_target in
defrag_collect_targets()v3:
- Add Reviewed-by tags- Fix a wrong value in commit message of the 1st patch
- Make @orig_start const for the 3rd patch
Quoted 8 lines
- Fix a missing word "skip" in the 5th patch
- Remove one unnecessary assignment in the 5th patch As we don't return the defragged sectors to user space.
v4:
- Move the skip behavior before thebtrfs_defrag_ctrlrefactor So it can be backported to v5.16
Thanks, I've picked the patch to misc-next. I'm not sure about the rest because of the ctrl structure cleanup and there are still some pending defrag fixes. So I'll add the remainig patches as topic branch for testing.
Quoted 39 lines · David Sterba
Quoted 38 lines · Qu Wenruo
Changelog:
v2:
- Rebased to lastest misc-next
Just one small conflict withstatic_assert()update.
And this time only those patches are rebased to misc-next, thus it may cause conflicts with fixes fordefrag_check_next_extent()in the future.- Several grammar fixes
- Report accurate
btrfs_defrag_ctrl::sectors_defraggedThis is inspired by a comment from Filipe that the skip check should be done in thedefrag_collect_targets()call insidedefrag_one_range().This results a new patch in v2.
- Change the timing of
btrfs_defrag_ctrl::last_scannedupdate Now it's updated insidedefrag_one_range(), which will give us an accurate view, unlike the previous call site indefrag_one_cluster().- Don't change the timing of extent threshold.
- Rename @last_target to @last_is_target in
defrag_collect_targets()v3:
- Add Reviewed-by tags- Fix a wrong value in commit message of the 1st patch
- Make @orig_start const for the 3rd patch
- Fix a missing word "skip" in the 5th patch
- Remove one unnecessary assignment in the 5th patch As we don't return the defragged sectors to user space.
Quoted 8 lines
Quoted 3 lines
v4:
- Move the skip behavior before thebtrfs_defrag_ctrlrefactor So it can be backported to v5.16Thanks, I've picked the patch to misc-next. I'm not sure about the rest because of the ctrl structure cleanup and there are still some pending defrag fixes. So I'll add the remainig patches as topic branch for testing.
Rest of the patchset added to misc-next, as the remaining fixups ported to older stable trees will be easy enough.