Re: [PATCH RFC 0/3] btrfs: make autodefrag to defrag and only defrag small write ranges

[Filipe Manana <fdmanana@kernel.org>]

On Wed, Feb 09, 2022 at 05:23:11PM +0800, Qu Wenruo wrote:
> Previously autodefrag works by scanning the whole file with a minimal
> generation threshold.
> 
> Although we have various optimization to skip ranges which don't meet
> the generation requirement, it can still waste some time on scanning the
> whole file, especially if the inode got an almost full overwrite.
> 
> There is another problem, there is a gap between our small writes and
> defrag extent size threshold.
> 
> In fact, for compressed writes, <16K will be considered as small writes,
> while for uncompressed writes, <32K will be considered as small writes.
> 
> On the other hand, autodefrag uses 256K as default extent size
> threshold.
> 
> 
> This means if one file has a lot of writes larger than 32K, which
> normally will not trigger autodefrag, but if one small write happens,
> all writes between 32K and 256K will be defragged.
> 
> This double standards is causing extra IO.
> 
> This patchset will address it by only defragging the small writes which
> trigger autodefrag.
> 
> 
> This rework will cause the following behavior change:
> 
> - Only small write ranges will be defragged
>   Exactly what we want.
> 
> - Enlarged critical section for fs_info::defrag_inodes_lock
>   Now we need to not only add the inode_defrag structure to rb tree, but
>   also call set_extent_bits() inside the critical section.
> 
>   Thus defrag_inodes_lock is upgraded to mutex.
> 
>   No benchmark for the possible performance impact though.
> 
> - No inode re-queue if there are large sectors to defrag
>   Not sure if this will make a difference, as we no longer requeue, and
>   only scan forward.
> 
> Reason for RFC:
> 
> I'm not sure if this is the correct way to go, but with my biased eyes,
> it looks very solid.
> 
> Another concern is how to backport for v5.16.

This is a whole new behaviour that 5.15, and older kernels, did not have.
And it's quite a huge change in behaviour, it would need to be well tested.
There's the potential for much more extra memory usage, blocking writeback
and the cleaner kthread for longer periods and delaying other operations the
cleaner does besides defrag (run delayed iputs, delete empty block groups,
delete subvolumes/snapshots, etc). Right now, it seems to risky to backport.

Anyway, I left my comments and concerns on patch 3/3.
Patch 2/3 seems pointless, it's trivial and short, and it's only used after
patch 3/3, so it could be squashed with 3/3.

Thanks.

> 
> Qu Wenruo (3):
>   btrfs: remove an unused parameter of btrfs_add_inode_defrag()
>   btrfs: introduce IO_TREE_AUTODEFRAG owner type
>   btrfs: make autodefrag to defrag small writes without rescanning the
>     whole file
> 
>  fs/btrfs/ctree.h          |   5 +-
>  fs/btrfs/disk-io.c        |   2 +-
>  fs/btrfs/extent-io-tree.h |   1 +
>  fs/btrfs/file.c           | 217 +++++++++++++++++++++-----------------
>  fs/btrfs/inode.c          |   2 +-
>  5 files changed, 125 insertions(+), 102 deletions(-)
> 
> -- 
> 2.35.0
>