On 2021/12/7 19:02, Filipe Manana wrote:

On Tue, Dec 07, 2021 at 03:43:58PM +0800, Qu Wenruo wrote:

quoted

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.

quoted

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.

quoted

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).

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.

Thanks,
Qu

quoted

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