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

[Josef Bacik <josef@toxicpanda.com>]

On Thu, Dec 09, 2021 at 09:25:58PM +0800, Qu Wenruo wrote:
> 
> 
> On 2021/12/9 18:25, Filipe Manana wrote:
> > On Wed, Dec 08, 2021 at 03:04:11PM +0100, David Sterba wrote:
> > > On Tue, Dec 07, 2021 at 03:53:22PM +0000, Filipe Manana wrote:
> > > > > > I'm doing some tests, in a VM on a dedicated HDD.
> > > > > 
> > > > > There's some measurable difference:
> > > > > 
> > > > > With readahead:
> > > > > 
> > > > > Duration:         0:00:20
> > > > > Total to scrub:   7.02GiB
> > > > > Rate:             236.92MiB/s
> > > > > 
> > > > > Duration:         0:00:48
> > > > > Total to scrub:   12.02GiB
> > > > > Rate:             198.02MiB/s
> > > > > 
> > > > > Without readahead:
> > > > > 
> > > > > Duration:         0:00:22
> > > > > Total to scrub:   7.02GiB
> > > > > Rate:             215.10MiB/s
> > > > > 
> > > > > Duration:         0:00:50
> > > > > Total to scrub:   12.02GiB
> > > > > Rate:             190.66MiB/s
> > > > > 
> > > > > The setup is: data/single, metadata/dup, no-holes, free-space-tree,
> > > > > there are 8 backing devices but all reside on one HDD.
> > > > > 
> > > > > Data generated by fio like
> > > > > 
> > > > > fio --rw=randrw --randrepeat=1 --size=3000m \
> > > > >           --bsrange=512b-64k --bs_unaligned \
> > > > >           --ioengine=libaio --fsync=1024 \
> > > > >           --name=job0 --name=job1 \
> > > > > 
> > > > > and scrub starts right away this. VM has 4G or memory and 4 CPUs.
> > > > 
> > > > How about using bare metal? And was it a debug kernel, or a default
> > > > kernel config from a distro?
> > > 
> > > It was the debug config I use for normal testing, I'll try to redo it on
> > > another physical box.
> > > 
> > > > Those details often make all the difference (either for the best or
> > > > for the worse).
> > > > 
> > > > I'm curious to see as well the results when:
> > > > 
> > > > 1) The reada.c code is changed to work with commit roots;
> > > > 
> > > > 2) The standard btree readahead (struct btrfs_path::reada) is used
> > > > instead of the reada.c code.
> > > > 
> > > > > 
> > > > > The difference is 2 seconds, roughly 4% but the sample is not large
> > > > > enough to be conclusive.
> > > > 
> > > > A bit too small.
> > > 
> > > What's worse, I did a few more rounds and the results were too unstable,
> > > from 44 seconds to 25 seconds (all on the removed readahead branch), but
> > > the machine was not quiescent.
> > 
> > I get such huge variations too when using a debug kernel and virtualized
> > disks for any tests, even for single threaded tests.
> > 
> > That's why I use a default, non-debug, kernel config from a popular distro
> > and without any virtualization (or at least have qemu use a raw device, not
> > a file backed disk on top of another filesystem) when measuring performance.
> > 
> I got my 2.5' HDD installed and tested.
> 
> [CONCLUSION]
> 
> There is a small but very consistent performance drop for HDD.
> 
> Without patchset:	average rate = 106.46 MiB/s
> With patchset:		average rate = 100.74 MiB/s
> 
> Diff = -5.67%
> 

5% isn't that big of a deal to get rid of an infrastructure we don't really use
and that is in the way.

Additionally I'm going to be reworking scrub quite a bit to work with the new
extent tree v2 stuff, the readahead stuff may not fit into the new scheme very
well.

Scrub isn't meant to be performant, it's meant to be correct.  In fact, many
admins would prefer to throttle it and keep it from overwhelming the main
workload, so a 5% drop isn't likely to upset anybody.

In my mind we're getting rid of a lot of code for very little cost, which I'm a
fan of.  I think this is a reasonable trade off, we have more important projects
to tackle.  In the future if we want to improve things with a readahead
framework we can tackle the problem as a whole and make it useful for all of
btrfs, not just scrub.  Thanks,

Josef