[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCHv3 RESEND] block: introduce BDRV_O_SEQUENTIAL
From: |
Kevin Wolf |
Subject: |
Re: [Qemu-devel] [PATCHv3 RESEND] block: introduce BDRV_O_SEQUENTIAL |
Date: |
Thu, 5 Jun 2014 10:13:04 +0200 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
Am 05.06.2014 um 10:09 hat Peter Lieven geschrieben:
> On 05.06.2014 09:53, Stefan Hajnoczi wrote:
> >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.
>
> I understand that you are critical about it. I can just say it solved
> the problem with the specific setup, kernel version etc.
>
> I found that FADVISE_DONTNEED solves problems also in other applications.
> Offtopic: i have an raspberry pi running as tvheadend and observed desync
> of the DVBS2 signal at some times. Since I FADV_DONTNEED all written
> frames away it runs smothly. I this case the feeing of the page cache was
> CPU intensive for the small device and caused the desync.
>
> >
> >>>>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.
>
> Of course, but when e.g. qcow2 opens its underlying file, then
> BDRV_O_SEQUENTIAL
> is not passed on, or is it?
It isn't qcow2 but block.c that opens bs->file, and unless you
explicitly filter out a flag, bs->file inherits it. (If it didn't do
that, your patch would have no effect for raw either.)
> >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.
>
> This would be an option.
I agree, though it's not quite clear how raw_bsd would do that. Would
that involve a bdrv_reopen() for bs->file?
Kevin