Re: [Qemu-devel] [PATCH v2] mirror: double performance of the bulk stage if the disc is full

["Denis V. Lunev" <den@openvz.org>]

On 07/20/2016 10:08 PM, Paolo Bonzini wrote:
>
> ----- Original Message -----
>> From: "Denis V. Lunev" <den@openvz.org>
>> To: "Vladimir Sementsov-Ogievskiy" <vsementsov@virtuozzo.com>, qemu-block@nongnu.org, qemu-devel@nongnu.org
>> Cc: stefanha@redhat.com, famz@redhat.com, mreitz@redhat.com, jcody@redhat.com, eblake@redhat.com,
>> pbonzini@redhat.com, "Kevin Wolf" <kwolf@redhat.com>
>> Sent: Wednesday, July 20, 2016 7:36:00 PM
>> Subject: Re: [PATCH v2] mirror: double performance of the bulk stage if the disc is full
>>
>> On 07/14/2016 08:19 PM, Vladimir Sementsov-Ogievskiy wrote:
>>> Mirror can do up to 16 in-flight requests, but actually on full copy
>>> (the whole source disk is non-zero) in-flight is always 1. This happens
>>> as the request is not limited in size: the data occupies maximum available
>>> capacity of s->buf.
>>>
>>> The patch limits the size of the request to some artificial constant
>>> (1 Mb here), which is not that big or small. This effectively enables
>>> back parallelism in mirror code as it was designed.
>>>
>>> The result is important: the time to migrate 10 Gb disk is reduced from
>>> ~350 sec to 170 sec.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>>> CC: Stefan Hajnoczi <stefanha@redhat.com>
>>> CC: Fam Zheng <famz@redhat.com>
>>> CC: Kevin Wolf <kwolf@redhat.com>
>>> CC: Max Reitz <mreitz@redhat.com>
>>> CC: Jeff Cody <jcody@redhat.com>
>>> CC: Eric Blake <eblake@redhat.com>
>>> ---
>>>
>>> v2: in case of s->buf_size larger than default use it to limit io_sectors
>>>
>>>    block/mirror.c | 10 ++++++++--
>>>    1 file changed, 8 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/block/mirror.c b/block/mirror.c
>>> index b1e633e..3ac3b4d 100644
>>> --- a/block/mirror.c
>>> +++ b/block/mirror.c
>>> @@ -23,7 +23,9 @@
>>>    
>>>    #define SLICE_TIME    100000000ULL /* ns */
>>>    #define MAX_IN_FLIGHT 16
>>> -#define DEFAULT_MIRROR_BUF_SIZE   (10 << 20)
>>> +#define MAX_IO_SECTORS ((1 << 20) >> BDRV_SECTOR_BITS) /* 1 Mb */
>>> +#define DEFAULT_MIRROR_BUF_SIZE \
>>> +    (MAX_IN_FLIGHT * MAX_IO_SECTORS * BDRV_SECTOR_SIZE)
>>>    
>>>    /* The mirroring buffer is a list of granularity-sized chunks.
>>>     * Free chunks are organized in a list.
>>> @@ -322,6 +324,8 @@ static uint64_t coroutine_fn
>>> mirror_iteration(MirrorBlockJob *s)
>>>        int nb_chunks = 1;
>>>        int64_t end = s->bdev_length / BDRV_SECTOR_SIZE;
>>>        int sectors_per_chunk = s->granularity >> BDRV_SECTOR_BITS;
>>> +    int max_io_sectors = MAX((s->buf_size >> BDRV_SECTOR_BITS) /
>>> MAX_IN_FLIGHT,
>>> +                             MAX_IO_SECTORS);
>>>    
>>>        sector_num = hbitmap_iter_next(&s->hbi);
>>>        if (sector_num < 0) {
>>> @@ -385,7 +389,9 @@ static uint64_t coroutine_fn
>>> mirror_iteration(MirrorBlockJob *s)
>>>                                              nb_chunks * sectors_per_chunk,
>>>                                              &io_sectors, &file);
>>>            if (ret < 0) {
>>> -            io_sectors = nb_chunks * sectors_per_chunk;
>>> +            io_sectors = MIN(nb_chunks * sectors_per_chunk,
>>> max_io_sectors);
>>> +        } else if (ret & BDRV_BLOCK_DATA) {
>>> +            io_sectors = MIN(io_sectors, max_io_sectors);
>>>            }
>>>    
>>>            io_sectors -= io_sectors % sectors_per_chunk;
>> guys, what about this patch?
> I think that at this point it has missed hard freeze.  It's not up to me
> whether to consider it a bugfix.
>
> Paolo
I see. It's pity this patch has missed deadline...
The difference is really big.

OK ;) I think it will not be missed in the next
release.

Den