* [PATCH 0/2] btrfs: defrag: don't defrag extents already at their max capacity, then remove an ambiguous check @ 2022-01-27 5:24 Qu Wenruo 2022-01-27 5:24 ` [PATCH 1/2] btrfs: defrag: don't defrag extents which is already at its max capacity Qu Wenruo ` (2 more replies) 0 siblings, 3 replies; 6+ messages in thread From: Qu Wenruo @ 2022-01-27 5:24 UTC (permalink / raw) To: linux-btrfs Thanks to the report from Filipe where he found a new bug in compressed defrag, that we will try to defrag all compressed extents even it's already at its max compacity. This behavior is from the beginning of btrfs defrag. The first patch is to fix the behavior, by just rejecting extents which are already at their max capacity, nor allowing extents to be merged with extents at their max capacity. The second patch is to remove an ambiguous rejection condition. The condition is believed to reject compressed extent, but it never really works due to the check is > 128K, not >= 128K. And the physically adajcent check may prevent users to reduce the number of extents. Qu Wenruo (2): btrfs: defrag: don't defrag extents which is already at its max capacity btrfs: defrag: remove an ambiguous condition for rejection fs/btrfs/ioctl.c | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-) -- 2.34.1 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/2] btrfs: defrag: don't defrag extents which is already at its max capacity 2022-01-27 5:24 [PATCH 0/2] btrfs: defrag: don't defrag extents already at their max capacity, then remove an ambiguous check Qu Wenruo @ 2022-01-27 5:24 ` Qu Wenruo 2022-01-27 10:48 ` Filipe Manana 2022-01-27 5:24 ` [PATCH 2/2] btrfs: defrag: remove an ambiguous condition for rejection Qu Wenruo 2022-01-27 10:53 ` [PATCH 0/2] btrfs: defrag: don't defrag extents already at their max capacity, then remove an ambiguous check Filipe Manana 2 siblings, 1 reply; 6+ messages in thread From: Qu Wenruo @ 2022-01-27 5:24 UTC (permalink / raw) To: linux-btrfs; +Cc: Filipe Manana [BUG] For compressed extents, defrag ioctl will always try to defrag any compressed extents, wasting not only IO but also CPU time to compress/decompress: mkfs.btrfs -f $DEV mount -o compress $DEV $MNT xfs_io -f -c "pwrite -S 0xab 0 128K" $MNT/foobar sync xfs_io -f -c "pwrite -S 0xcd 128K 128K" $MNT/foobar sync echo "=== before ===" xfs_io -c "fiemap -v" $MNT/foobar btrfs filesystem defrag $MNT/foobar sync echo "=== after ===" xfs_io -c "fiemap -v" $MNT/foobar Then it shows the 2 128K extents just get CoW for no extra benefit, with extra IO/CPU spent: === before === /mnt/btrfs/file1: EXT: FILE-OFFSET BLOCK-RANGE TOTAL FLAGS 0: [0..255]: 26624..26879 256 0x8 1: [256..511]: 26632..26887 256 0x9 === after === /mnt/btrfs/file1: EXT: FILE-OFFSET BLOCK-RANGE TOTAL FLAGS 0: [0..255]: 26640..26895 256 0x8 1: [256..511]: 26648..26903 256 0x9 This affects not only v5.16 (after the defrag rework), but also v5.15 (before the defrag rework). [CAUSE] From the very beginning, btrfs defrag never checks if one extent is already at its max capacity (128K for compressed extents, 128M otherwise). And the default extent size threshold is 256K, which is already beyond the compressed extent max size. This means, by default btrfs defrag ioctl will mark all compressed extent which is not adjacent to a hole/preallocated range for defrag. [FIX] Introduce a helper to grab the maximum extent size, and then in defrag_collect_targets() and defrag_check_next_extent(), reject extents which are already at their max capacity. Reported-by: Filipe Manana <fdmanana@suse.com> Signed-off-by: Qu Wenruo <wqu@suse.com> --- fs/btrfs/ioctl.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c index 137d3732af33..a03c31e1ff18 100644 --- a/fs/btrfs/ioctl.c +++ b/fs/btrfs/ioctl.c @@ -1049,6 +1049,13 @@ static struct extent_map *defrag_lookup_extent(struct inode *inode, u64 start, return em; } +static u32 get_extent_max_capacity(struct extent_map *em) +{ + if (test_bit(EXTENT_FLAG_COMPRESSED, &em->flags)) + return BTRFS_MAX_COMPRESSED; + return BTRFS_MAX_EXTENT_SIZE; +} + static bool defrag_check_next_extent(struct inode *inode, struct extent_map *em, bool locked) { @@ -1065,6 +1072,12 @@ static bool defrag_check_next_extent(struct inode *inode, struct extent_map *em, goto out; if (test_bit(EXTENT_FLAG_PREALLOC, &em->flags)) goto out; + /* + * If the next extent is at its max capcity, defragging current extent + * makes no sense, as the total number of extents won't change. + */ + if (next->len >= get_extent_max_capacity(em)) + goto out; /* Physically adjacent and large enough */ if ((em->block_start + em->block_len == next->block_start) && (em->block_len > SZ_128K && next->block_len > SZ_128K)) @@ -1229,6 +1242,13 @@ static int defrag_collect_targets(struct btrfs_inode *inode, if (em->len >= extent_thresh) goto next; + /* + * Skip extents already at its max capacity, this is mostly for + * compressed extents, which max cap is only 128K. + */ + if (em->len >= get_extent_max_capacity(em)) + goto next; + next_mergeable = defrag_check_next_extent(&inode->vfs_inode, em, locked); if (!next_mergeable) { -- 2.34.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] btrfs: defrag: don't defrag extents which is already at its max capacity 2022-01-27 5:24 ` [PATCH 1/2] btrfs: defrag: don't defrag extents which is already at its max capacity Qu Wenruo @ 2022-01-27 10:48 ` Filipe Manana 0 siblings, 0 replies; 6+ messages in thread From: Filipe Manana @ 2022-01-27 10:48 UTC (permalink / raw) To: Qu Wenruo; +Cc: linux-btrfs, Filipe Manana On Thu, Jan 27, 2022 at 01:24:42PM +0800, Qu Wenruo wrote: > [BUG] > For compressed extents, defrag ioctl will always try to defrag any > compressed extents, wasting not only IO but also CPU time to > compress/decompress: > > mkfs.btrfs -f $DEV > mount -o compress $DEV $MNT > xfs_io -f -c "pwrite -S 0xab 0 128K" $MNT/foobar > sync > xfs_io -f -c "pwrite -S 0xcd 128K 128K" $MNT/foobar > sync > echo "=== before ===" > xfs_io -c "fiemap -v" $MNT/foobar > btrfs filesystem defrag $MNT/foobar > sync > echo "=== after ===" > xfs_io -c "fiemap -v" $MNT/foobar > > Then it shows the 2 128K extents just get CoW for no extra benefit, with > extra IO/CPU spent: > > === before === > /mnt/btrfs/file1: > EXT: FILE-OFFSET BLOCK-RANGE TOTAL FLAGS > 0: [0..255]: 26624..26879 256 0x8 > 1: [256..511]: 26632..26887 256 0x9 > === after === > /mnt/btrfs/file1: > EXT: FILE-OFFSET BLOCK-RANGE TOTAL FLAGS > 0: [0..255]: 26640..26895 256 0x8 > 1: [256..511]: 26648..26903 256 0x9 > > This affects not only v5.16 (after the defrag rework), but also v5.15 > (before the defrag rework). > > [CAUSE] > From the very beginning, btrfs defrag never checks if one extent is > already at its max capacity (128K for compressed extents, 128M > otherwise). > > And the default extent size threshold is 256K, which is already beyond > the compressed extent max size. > > This means, by default btrfs defrag ioctl will mark all compressed > extent which is not adjacent to a hole/preallocated range for defrag. > > [FIX] > Introduce a helper to grab the maximum extent size, and then in > defrag_collect_targets() and defrag_check_next_extent(), reject extents > which are already at their max capacity. > > Reported-by: Filipe Manana <fdmanana@suse.com> > Signed-off-by: Qu Wenruo <wqu@suse.com> > --- > fs/btrfs/ioctl.c | 20 ++++++++++++++++++++ > 1 file changed, 20 insertions(+) > > diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c > index 137d3732af33..a03c31e1ff18 100644 > --- a/fs/btrfs/ioctl.c > +++ b/fs/btrfs/ioctl.c > @@ -1049,6 +1049,13 @@ static struct extent_map *defrag_lookup_extent(struct inode *inode, u64 start, > return em; > } > > +static u32 get_extent_max_capacity(struct extent_map *em) Could be made const. Minor thing apart, which can updated when the patch is picked, Reviewed-by: Filipe Manana <fdmanana@suse.com> Thanks. > +{ > + if (test_bit(EXTENT_FLAG_COMPRESSED, &em->flags)) > + return BTRFS_MAX_COMPRESSED; > + return BTRFS_MAX_EXTENT_SIZE; > +} > + > static bool defrag_check_next_extent(struct inode *inode, struct extent_map *em, > bool locked) > { > @@ -1065,6 +1072,12 @@ static bool defrag_check_next_extent(struct inode *inode, struct extent_map *em, > goto out; > if (test_bit(EXTENT_FLAG_PREALLOC, &em->flags)) > goto out; > + /* > + * If the next extent is at its max capcity, defragging current extent > + * makes no sense, as the total number of extents won't change. > + */ > + if (next->len >= get_extent_max_capacity(em)) > + goto out; > /* Physically adjacent and large enough */ > if ((em->block_start + em->block_len == next->block_start) && > (em->block_len > SZ_128K && next->block_len > SZ_128K)) > @@ -1229,6 +1242,13 @@ static int defrag_collect_targets(struct btrfs_inode *inode, > if (em->len >= extent_thresh) > goto next; > > + /* > + * Skip extents already at its max capacity, this is mostly for > + * compressed extents, which max cap is only 128K. > + */ > + if (em->len >= get_extent_max_capacity(em)) > + goto next; > + > next_mergeable = defrag_check_next_extent(&inode->vfs_inode, em, > locked); > if (!next_mergeable) { > -- > 2.34.1 > ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 2/2] btrfs: defrag: remove an ambiguous condition for rejection 2022-01-27 5:24 [PATCH 0/2] btrfs: defrag: don't defrag extents already at their max capacity, then remove an ambiguous check Qu Wenruo 2022-01-27 5:24 ` [PATCH 1/2] btrfs: defrag: don't defrag extents which is already at its max capacity Qu Wenruo @ 2022-01-27 5:24 ` Qu Wenruo 2022-01-27 10:49 ` Filipe Manana 2022-01-27 10:53 ` [PATCH 0/2] btrfs: defrag: don't defrag extents already at their max capacity, then remove an ambiguous check Filipe Manana 2 siblings, 1 reply; 6+ messages in thread From: Qu Wenruo @ 2022-01-27 5:24 UTC (permalink / raw) To: linux-btrfs From the very beginning of btrfs defrag, there is a check to reject extents which meet both conditions: - Physically adjacent We may want to defrag physically adjacent extents to reduce the number of extents or the size of subvolume tree. - Larger than 128K This may be there for compressed extents, but unfortunately 128K is exactly the max capacity for compressed extents. And the check is > 128K, thus it never rejects compressed extents. Furthermore, the compressed extent capacity bug is fixed by previous patch, there is no reason for that check anymore. The original check has a very small ranges to reject (the target extent size is > 128K, and default extent threshold is 256K), and for compressed extent it doesn't work at all. So it's better just to remove the rejection, and allow us to defrag physically adjacent extents. Signed-off-by: Qu Wenruo <wqu@suse.com> --- fs/btrfs/ioctl.c | 4 ---- 1 file changed, 4 deletions(-) diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c index a03c31e1ff18..af95e3b7aa72 100644 --- a/fs/btrfs/ioctl.c +++ b/fs/btrfs/ioctl.c @@ -1078,10 +1078,6 @@ static bool defrag_check_next_extent(struct inode *inode, struct extent_map *em, */ if (next->len >= get_extent_max_capacity(em)) goto out; - /* Physically adjacent and large enough */ - if ((em->block_start + em->block_len == next->block_start) && - (em->block_len > SZ_128K && next->block_len > SZ_128K)) - goto out; ret = true; out: free_extent_map(next); -- 2.34.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] btrfs: defrag: remove an ambiguous condition for rejection 2022-01-27 5:24 ` [PATCH 2/2] btrfs: defrag: remove an ambiguous condition for rejection Qu Wenruo @ 2022-01-27 10:49 ` Filipe Manana 0 siblings, 0 replies; 6+ messages in thread From: Filipe Manana @ 2022-01-27 10:49 UTC (permalink / raw) To: Qu Wenruo; +Cc: linux-btrfs On Thu, Jan 27, 2022 at 01:24:43PM +0800, Qu Wenruo wrote: > From the very beginning of btrfs defrag, there is a check to reject > extents which meet both conditions: > > - Physically adjacent > > We may want to defrag physically adjacent extents to reduce the number > of extents or the size of subvolume tree. > > - Larger than 128K > > This may be there for compressed extents, but unfortunately 128K is > exactly the max capacity for compressed extents. > And the check is > 128K, thus it never rejects compressed extents. > > Furthermore, the compressed extent capacity bug is fixed by previous > patch, there is no reason for that check anymore. > > The original check has a very small ranges to reject (the target extent > size is > 128K, and default extent threshold is 256K), and for > compressed extent it doesn't work at all. > > So it's better just to remove the rejection, and allow us to defrag > physically adjacent extents. > > Signed-off-by: Qu Wenruo <wqu@suse.com> Reviewed-by: Filipe Manana <fdmanana@suse.com> Looks good now, thanks. > --- > fs/btrfs/ioctl.c | 4 ---- > 1 file changed, 4 deletions(-) > > diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c > index a03c31e1ff18..af95e3b7aa72 100644 > --- a/fs/btrfs/ioctl.c > +++ b/fs/btrfs/ioctl.c > @@ -1078,10 +1078,6 @@ static bool defrag_check_next_extent(struct inode *inode, struct extent_map *em, > */ > if (next->len >= get_extent_max_capacity(em)) > goto out; > - /* Physically adjacent and large enough */ > - if ((em->block_start + em->block_len == next->block_start) && > - (em->block_len > SZ_128K && next->block_len > SZ_128K)) > - goto out; > ret = true; > out: > free_extent_map(next); > -- > 2.34.1 > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 0/2] btrfs: defrag: don't defrag extents already at their max capacity, then remove an ambiguous check 2022-01-27 5:24 [PATCH 0/2] btrfs: defrag: don't defrag extents already at their max capacity, then remove an ambiguous check Qu Wenruo 2022-01-27 5:24 ` [PATCH 1/2] btrfs: defrag: don't defrag extents which is already at its max capacity Qu Wenruo 2022-01-27 5:24 ` [PATCH 2/2] btrfs: defrag: remove an ambiguous condition for rejection Qu Wenruo @ 2022-01-27 10:53 ` Filipe Manana 2 siblings, 0 replies; 6+ messages in thread From: Filipe Manana @ 2022-01-27 10:53 UTC (permalink / raw) To: Qu Wenruo; +Cc: linux-btrfs On Thu, Jan 27, 2022 at 01:24:41PM +0800, Qu Wenruo wrote: > Thanks to the report from Filipe where he found a new bug in compressed > defrag, that we will try to defrag all compressed extents even it's > already at its max compacity. > > This behavior is from the beginning of btrfs defrag. > > The first patch is to fix the behavior, by just rejecting extents > which are already at their max capacity, nor allowing extents to be > merged with extents at their max capacity. > > The second patch is to remove an ambiguous rejection condition. > The condition is believed to reject compressed extent, but it never > really works due to the check is > 128K, not >= 128K. > > And the physically adajcent check may prevent users to reduce the number > of extents. With so many patches sent in such short periods of time, with some in series and others not in properly formatted series, it's likely hard for other people to follow what's going on. So this patchset, applies on top of the following patch: https://patchwork.kernel.org/project/linux-btrfs/patch/20220126005850.14729-1-wqu@suse.com/ That patch is part of another series consisting of 3 patches, but this patchset here makes the patches 2/3 and 3/3 of that other patchset obsolete. Thanks. > > Qu Wenruo (2): > btrfs: defrag: don't defrag extents which is already at its max > capacity > btrfs: defrag: remove an ambiguous condition for rejection > > fs/btrfs/ioctl.c | 22 +++++++++++++++++++--- > 1 file changed, 19 insertions(+), 3 deletions(-) > > -- > 2.34.1 > ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-01-27 10:54 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-01-27 5:24 [PATCH 0/2] btrfs: defrag: don't defrag extents already at their max capacity, then remove an ambiguous check Qu Wenruo 2022-01-27 5:24 ` [PATCH 1/2] btrfs: defrag: don't defrag extents which is already at its max capacity Qu Wenruo 2022-01-27 10:48 ` Filipe Manana 2022-01-27 5:24 ` [PATCH 2/2] btrfs: defrag: remove an ambiguous condition for rejection Qu Wenruo 2022-01-27 10:49 ` Filipe Manana 2022-01-27 10:53 ` [PATCH 0/2] btrfs: defrag: don't defrag extents already at their max capacity, then remove an ambiguous check Filipe Manana
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.