On 2021/12/14 20:41, David Sterba wrote:

On Mon, Dec 13, 2021 at 09:10:51PM +0800, Qu Wenruo wrote:

quoted

[PROBLEMS]
The metadata readahead code is introduced in 2011 (surprisingly, the
commit message even contains changelog), but now there is only one user
for it, and even for the only one user, the readahead mechanism can't
provide all it needs:

- No support for commit tree readahead
   Only support readahead on current tree.

- Bad layer separation
   To manage on-fly bios, btrfs_reada_add() mechanism internally manages
   a kinda complex zone system, and it's bind to per-device.

   This is against the common layer separation, as metadata should all be
   in btrfs logical address space, should not be bound to device physical
   layer.

   In fact, this is the cause of all recent reada related bugs.

- No caller for asynchronous metadata readahead
   Even btrfs_reada_add() is designed to be fully asynchronous, scrub
   only calls it in a synchronous way (call btrfs_reada_add() and
   immediately call btrfs_reada_wait()).
   Thus rendering a lot of code unnecessary.

I agree with removing the reada.c code, it's overengineered perhaps with
intentions to use it in more places but this hasn't happened and nobody
is interested doing the work. We have the path readahead so it's not
we'd lose readahead capabilities at all.

Thanks for benchmarking it, the drop is acceptable and we know people
are more interested in limiting the load anyway.

quoted

[BENCHMARK]
The conclusion looks like this:

For the worst case (no dirty metadata, slow HDD), there will be around 5%
performance drop for scrub.
For other cases (even SATA SSD), there is no distinguishable performance
difference.

The number is reported scrub speed, in MiB/s.
The resolution is limited by the reported duration, which only has a
resolution of 1 second.

	Old		New		Diff
SSD	455.3		466.332		+2.42%
HDD	103.927 	98.012		-5.69%

I'll copy this information to the last patch changelog.

Since I found a bug in the first patch, let me have the chance to
re-order the patches, and put this into the patch removing reada.

Thanks,
Qu