Re: [PATCH RFC 0/2] btrfs: remove the metadata readahead mechanism

[Filipe Manana <fdmanana@kernel.org>]

On Tue, Dec 07, 2021 at 03:43:58PM +0800, Qu Wenruo wrote:
> This is originally just my preparation for scrub refactors, but when the
> readahead is involved, it won't just be a small cleanup.
> 
> The metadata readahead code is introduced in 2011 (surprisingly, the
> commit message even contains changelog), but now only one user for it,
> and even for the only one user, the readahead mechanism can't provide
> much help in fact.
> 
> Scrub needs readahead for commit root, but the existing one can only do
> current root readahead.

If support for the commit root is added, is there a noticeable speedup?
Have you tested that?

> 
> And the code is at a very bad layer inside btrfs, all metadata are at
> btrfs logical address space, but the readahead is kinda working at
> device layer (to manage the in-flight readahead).
> 
> Personally speaking, I don't think such "optimization" is really even
> needed, since we have better way like setting IO priority.

Have you done any benchmarks?
How? On physical machines or VMs?

Please include such details in the changelogs.

> 
> I really prefer to let the professional block layer guys do whatever
> they are good at (and in fact, those block layer guys rock!).
> Immature optimization is the cause of bugs, and it has already caused
> several bugs recently.
> 
> Nowadays we have btrfs_path::reada to do the readahead, I doubt if we
> really need such facility.

btrfs_path:reada is important and it makes a difference.
I recently changed send to use it, and benchmarks can be found in the
changelogs.

There are also other places where it makes a difference, such as when
reading a large chunk tree during mount or when reading a large directory.

It's all about reading other leaves/nodes in the background that will be
needed in the near future while the task is doing something else. Even if
the nodes/leaves are not physically contiguous on disk (that's the main
reason why the mechanism exists).

> 
> So here I purpose to completely remove the old and under utilized
> metadata readahead system.
> 
> Qu Wenruo (2):
>   btrfs: remove the unnecessary path parameter for scrub_raid56_parity()
>   btrfs: remove reada mechanism
> 
>  fs/btrfs/Makefile      |    2 +-
>  fs/btrfs/ctree.h       |   25 -
>  fs/btrfs/dev-replace.c |    5 -
>  fs/btrfs/disk-io.c     |   20 +-
>  fs/btrfs/extent_io.c   |    3 -
>  fs/btrfs/reada.c       | 1086 ----------------------------------------
>  fs/btrfs/scrub.c       |   64 +--
>  fs/btrfs/super.c       |    1 -
>  fs/btrfs/volumes.c     |    7 -
>  fs/btrfs/volumes.h     |    7 -
>  10 files changed, 17 insertions(+), 1203 deletions(-)
>  delete mode 100644 fs/btrfs/reada.c
> 
> -- 
> 2.34.1
>