Re: [Qemu-block] [PATCH v3 6/7] block/backup: teach backup_cow_with_bounce_buffer to copy more at once

[Vladimir Sementsov-Ogievskiy]

12.08.2019 18:10, Max Reitz wrote:
> On 10.08.19 21:31, Vladimir Sementsov-Ogievskiy wrote:
>> backup_cow_with_offload can transfer more than one cluster. Let
>> backup_cow_with_bounce_buffer behave similarly. It reduces the number
>> of IO requests, since there is no need to copy cluster by cluster.
>>
>> Logic around bounce_buffer allocation changed: we can't just allocate
>> one-cluster-sized buffer to share for all iterations. We can't also
>> allocate buffer of full-request length it may be too large, so
>> BACKUP_MAX_BOUNCE_BUFFER is introduced. And finally, allocation logic
>> is to allocate a buffer sufficient to handle all remaining iterations
>> at the point where we need the buffer for the first time.
>>
>> Bonus: get rid of pointer-to-pointer.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
>> ---
>>   block/backup.c | 65 +++++++++++++++++++++++++++++++-------------------
>>   1 file changed, 41 insertions(+), 24 deletions(-)
>>
>> diff --git a/block/backup.c b/block/backup.c
>> index d482d93458..65f7212c85 100644
>> --- a/block/backup.c
>> +++ b/block/backup.c
>> @@ -27,6 +27,7 @@
>>   #include "qemu/error-report.h"
>>   
>>   #define BACKUP_CLUSTER_SIZE_DEFAULT (1 << 16)
>> +#define BACKUP_MAX_BOUNCE_BUFFER (64 * 1024 * 1024)
>>   
>>   typedef struct CowRequest {
>>       int64_t start_byte;
>> @@ -98,44 +99,55 @@ static void cow_request_end(CowRequest *req)
>>       qemu_co_queue_restart_all(&req->wait_queue);
>>   }
>>   
>> -/* Copy range to target with a bounce buffer and return the bytes copied. If
>> - * error occurred, return a negative error number */
>> +/*
>> + * Copy range to target with a bounce buffer and return the bytes copied. If
>> + * error occurred, return a negative error number
>> + *
>> + * @bounce_buffer is assumed to enough to store
> 
> s/to/to be/
> 
>> + * MIN(BACKUP_MAX_BOUNCE_BUFFER, @end - @start) bytes
>> + */
>>   static int coroutine_fn backup_cow_with_bounce_buffer(BackupBlockJob *job,
>>                                                         int64_t start,
>>                                                         int64_t end,
>>                                                         bool 
>> is_write_notifier,
>>                                                         bool *error_is_read,
>> -                                                      void **bounce_buffer)
>> +                                                      void *bounce_buffer)
>>   {
>>       int ret;
>>       BlockBackend *blk = job->common.blk;
>> -    int nbytes;
>> +    int nbytes, remaining_bytes;
>>       int read_flags = is_write_notifier ? BDRV_REQ_NO_SERIALISING : 0;
>>   
>>       assert(QEMU_IS_ALIGNED(start, job->cluster_size));
>> -    bdrv_reset_dirty_bitmap(job->copy_bitmap, start, job->cluster_size);
>> -    nbytes = MIN(job->cluster_size, job->len - start);
>> -    if (!*bounce_buffer) {
>> -        *bounce_buffer = blk_blockalign(blk, job->cluster_size);
>> -    }
>> +    bdrv_reset_dirty_bitmap(job->copy_bitmap, start, end - start);
>> +    nbytes = MIN(end - start, job->len - start);
>>   
>> -    ret = blk_co_pread(blk, start, nbytes, *bounce_buffer, read_flags);
>> -    if (ret < 0) {
>> -        trace_backup_do_cow_read_fail(job, start, ret);
>> -        if (error_is_read) {
>> -            *error_is_read = true;
>> +
>> +    remaining_bytes = nbytes;
>> +    while (remaining_bytes) {
>> +        int chunk = MIN(BACKUP_MAX_BOUNCE_BUFFER, remaining_bytes);
>> +
>> +        ret = blk_co_pread(blk, start, chunk, bounce_buffer, read_flags);
>> +        if (ret < 0) {
>> +            trace_backup_do_cow_read_fail(job, start, ret);
>> +            if (error_is_read) {
>> +                *error_is_read = true;
>> +            }
>> +            goto fail;
>>           }
>> -        goto fail;
>> -    }
>>   
>> -    ret = blk_co_pwrite(job->target, start, nbytes, *bounce_buffer,
>> -                        job->write_flags);
>> -    if (ret < 0) {
>> -        trace_backup_do_cow_write_fail(job, start, ret);
>> -        if (error_is_read) {
>> -            *error_is_read = false;
>> +        ret = blk_co_pwrite(job->target, start, chunk, bounce_buffer,
>> +                            job->write_flags);
>> +        if (ret < 0) {
>> +            trace_backup_do_cow_write_fail(job, start, ret);
>> +            if (error_is_read) {
>> +                *error_is_read = false;
>> +            }
>> +            goto fail;
>>           }
>> -        goto fail;
>> +
>> +        start += chunk;
>> +        remaining_bytes -= chunk;
>>       }
>>   
>>       return nbytes;
>> @@ -301,9 +313,14 @@ static int coroutine_fn backup_do_cow(BackupBlockJob 
>> *job,
>>               }
>>           }
>>           if (!job->use_copy_range) {
>> +            if (!bounce_buffer) {
>> +                size_t len = MIN(BACKUP_MAX_BOUNCE_BUFFER,
>> +                                 MAX(dirty_end - start, end - dirty_end));
>> +                bounce_buffer = blk_try_blockalign(job->common.blk, len);
>> +            }
> 
> If you use _try_, you should probably also check whether it succeeded.

Oops, you are right, of course.

> 
> Anyway, I wonder whether it’d be better to just allocate this buffer
> once per job (the first time we get here, probably) to be of size
> BACKUP_MAX_BOUNCE_BUFFER and put it into BackupBlockJob.  (And maybe add
> a buf-size parameter similar to what the mirror jobs have.)
> 

Once per job will not work, as we may have several guest writes in parallel and 
therefore
several parallel copy-before-write operations. Or if you mean writing an 
allocator based
on once-allocated buffer like in mirror, I really dislike this idea, as we 
already have
allocator: memalign/malloc/free and it works well, no reason to invent a new 
one and
hardcode it into block layer (look at my answer to Eric on v2 of this patch for 
more info).

Or, if you mean only backup_loop generated copy-requests, yes we may keep only 
one buffer for them,
but:
1. it is not how it works now, so my patch is not a degradation in this case
2. I'm going to parallelize backup loop too, like my series "qcow2: async 
handling of fragmented io",
    which will need several allocated buffers anyway.

> 
>>               ret = backup_cow_with_bounce_buffer(job, start, dirty_end,
>>                                                   is_write_notifier,
>> -                                                error_is_read, 
>> &bounce_buffer);
>> +                                                error_is_read, 
>> bounce_buffer);
>>           }
>>           if (ret < 0) {
>>               break;
>>
> 
> 


-- 
Best regards,
Vladimir