[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCHv3 RESEND] block: introduce BDRV_O_SEQUENTIAL
From: |
Peter Lieven |
Subject: |
Re: [Qemu-devel] [PATCHv3 RESEND] block: introduce BDRV_O_SEQUENTIAL |
Date: |
Wed, 04 Jun 2014 17:31:48 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.4.0 |
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.
>
>> 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?
>
> Perhaps the extra I/O in that case doesn't matter but conceptually it's
> wrong to think that a raw-posix file will have a sequential access
> pattern just because bdrv_read() is called sequentially.
Peter