| Message ID | 20211213131054.102526-1-wqu@suse.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | btrfs: use btrfs_path::reada to replace the | expand |
On Mon, Dec 13, 2021 at 09:10:51PM +0800, Qu Wenruo wrote: > [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. > [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.
On 2021/12/14 20:41, David Sterba wrote: > On Mon, Dec 13, 2021 at 09:10:51PM +0800, Qu Wenruo wrote: >> [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. > >> [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