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