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