[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCHv3 RESEND] block: introduce BDRV_O_SEQUENTIAL
From: |
Stefan Hajnoczi |
Subject: |
Re: [Qemu-devel] [PATCHv3 RESEND] block: introduce BDRV_O_SEQUENTIAL |
Date: |
Thu, 5 Jun 2014 09:53:55 +0200 |
User-agent: |
Mutt/1.5.23 (2014-03-12) |
On Wed, Jun 04, 2014 at 05:31:48PM +0200, Peter Lieven wrote:
> Am 04.06.2014 17:12, schrieb Stefan Hajnoczi:
> > On Fri, May 30, 2014 at 11:40:37PM +0200, Peter Lieven wrote:
> >> this patch introduces a new flag to indicate that we are going to
> >> sequentially
> >> read from a file and do not plan to reread/reuse the data after it has
> >> been read.
> >>
> >> The current use of this flag is to open the source(s) of a qemu-img convert
> >> process. If a protocol from block/raw-posix.c is used posix_fadvise is
> >> utilized
> >> to advise to the kernel that we are going to read sequentially from the
> >> file and a POSIX_FADV_DONTNEED advise is issued after each write to
> >> indicate
> >> that there is no advantage keeping the blocks in the buffers.
> >>
> >> Consider the following test case that was created to confirm the behaviour
> >> of
> >> the new flag:
> >>
> >> A 10G logical volume was created and filled with random data.
> >> Then the logical volume was exported via qemu-img convert to an iscsi
> >> target.
> >> Before the export was started all caches of the linux kernel where dropped.
> >>
> >> Old behavior:
> >> - The convert process took 3m45s and the buffer cache grew up to 9.67 GB
> >> close
> >> to the end of the conversion. After qemu-img terminated all the buffers
> >> were
> >> freed by the kernel.
> >>
> >> New behavior with the -N switch:
> >> - The convert process took 3m43s and the buffer cache grew up to 15.48 MB
> >> close
> >> to the end with some small peaks up to 30 MB during the conversion.
> > FADVISE_SEQUENTIAL can be good since it doubles read-ahead on Linux.
> >
> > I'm skeptical of the effort to avoid buffer cache usage using
> > FADVISE_DONTNEED. The performance results tell me that less buffer
> > cache was used but that number doesn't have a direct effect on
> > application performance.
> >
> > Let's check GNU coreutils:
> >
> > $ cd coreutils
> > $ git grep FADVISE_DONTNEED
> > gl/lib/fadvise.h: FADVISE_DONTNEED = POSIX_FADV_DONTNEED,
> > gl/lib/fadvise.h: FADVISE_DONTNEED,
> > $
> >
> > GNU cp(1) does not care about minimizing impact on buffer cache using
> > FADVISE_DONTNEED. It just sets FADVISE_SEQUENTIAL on the source file
> > and calls read() (plus uses FIEMAP to check extents for sparseness).
> >
> > I want to avoid adding code just for the heck of it. We need a deeper
> > understanding:
> >
> > Please drop FADVISE_DONTNEED and compare again to see if it changes the
> > benchmark.
> >
> > By the way, did you perform several runs to check the variance of the
> > running time? I don't know if the 2 seconds difference were noise or
> > because FADVISE_SEQUENTIAL or because FADVISE_DONTNEED or because both.
>
> There was no effect on the runtime as far as I remember. I ran
> some tests, but not a number large enough to filter out the noise.
>
> I created this one because we saw it helps under memory pressure.
> Maybe its too specific to add it into mainline qemu, but I wanted to
> avoid to have too much individual changes we need to maintain.
I'm open to merging it if the improvement can be quantified. Right now
this might be a workaround for Linux memory management heuristics or it
might not have any effect, I don't know.
> >
> >> diff --git a/block/raw-posix.c b/block/raw-posix.c
> >> index 6586a0c..9768cc4 100644
> >> --- a/block/raw-posix.c
> >> +++ b/block/raw-posix.c
> >> @@ -447,6 +447,13 @@ static int raw_open_common(BlockDriverState *bs,
> >> QDict *options,
> >> }
> >> #endif
> >>
> >> +#ifdef POSIX_FADV_SEQUENTIAL
> >> + if (bs->open_flags & BDRV_O_SEQUENTIAL &&
> >> + !(bs->open_flags & BDRV_O_NOCACHE)) {
> >> + posix_fadvise(s->fd, 0, 0, POSIX_FADV_SEQUENTIAL);
> >> + }
> >> +#endif
> > This is only true if the image format is raw. If the image format on
> > top of this raw-posix BDS is non-raw then the read pattern may not be
> > sequential.
>
> You are right, but will the other formats set BDRV_O_SEQUENTIAL?
If the user specifies qemu-img convert -N then it will be set for any
image format.
Maybe qemu-img convert can always set BDRV_O_SEQUENTIAL and the have the
raw_bsd.c format propagate it to bs->file while other formats do not.
Then the user doesn't have to specify a command-line option and we don't
set it for non-raw image formats.
Stefan