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

Filipe Manana
2021-12-07T11:02:23Z

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
>