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

[Filipe Manana <fdmanana@kernel.org>]

On Tue, Dec 07, 2021 at 07:43:49PM +0800, Qu Wenruo wrote:
> 
> 
> On 2021/12/7 19:02, Filipe Manana wrote:
> > 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?
> > 
> 
> Will craft a benchmark for that.
> 
> Although I don't have any HDD available for benchmark, thus would only
> have result from SATA SSD.
> 
> > > 
> > > 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.
> 
> For the "such facility" I mean the btrfs_reada_add() facility, not the
> btrfs_path::reada one.
> 
> > 
> > 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).
> 
> Unfortunately, not really in the background.
> 
> For scrub usage, it kicks readahead and wait for it, not really doing it
> in the background.
> 
> (Nor btrfs_path::reada either though, btrfs_path reada also happens at
> tree search time, and it's synchronous).

No, the btrfs_path::reada mechanism is not synchronous - it does not wait
for the reads (IO) to complete.

btrfs_readahead_node_child() triggers a read for the extent buffer's
pages but does not wait for the reads to complete. I.e. we end up calling:

    read_extent_buffer_pages(eb, WAIT_NONE, 0);

So it does not wait on the read bios to complete.
Otherwise that would be pointless.

> 
> Another reason why the existing btrfs_reada_add() facility is not
> suitable for scrub is, our default tree block size is way larger than
> the scrub data length.
> 
> The current data length is 16 pages (64K), while even one 16K leaf can
> contain at least csum for 8M (CRC32) or 1M (SHA256).
> This means for most readahead, it doesn't make much sense as it won't
> cross leaf boundaries that frequently.
> 
> (BTW, in this particular case, btrfs_path::reada may perform better than
> the start/end based reada, as that would really do some readahead)
> 
> Anyway, only benchmark can prove whether I'm correct or wrong.

Yep, and preferably on a spinning disk and bare metal (no VM).

Thanks.

> 
> Thanks,
> Qu
> > 
> > > 
> > > 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
> > >