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

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

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

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