Re: [Qemu-block] [PATCH v10 04/14] block/backup: introduce BlockCopyState

[Vladimir Sementsov-Ogievskiy]

09.09.2019 15:59, Max Reitz wrote:
> On 30.08.19 18:12, Vladimir Sementsov-Ogievskiy wrote:
>> Split copying code part from backup to "block-copy", including separate
>> state structure and function renaming. This is needed to share it with
>> backup-top filter driver in further commits.
>>
>> Notes:
>>
>> 1. As BlockCopyState keeps own BlockBackend objects, remaining
>> job->common.blk users only use it to get bs by blk_bs() call, so clear
>> job->commen.blk permissions set in block_job_create and add
>> job->source_bs to be used instead of blk_bs(job->common.blk), to keep
>> it more clear which bs we use when introduce backup-top filter in
>> further commit.
>>
>> 2. Rename s/initializing_bitmap/skip_unallocated/ to sound a bit better
>> as interface to BlockCopyState
>>
>> 3. Split is not very clean: there left some duplicated fields, backup
>> code uses some BlockCopyState fields directly, let's postpone it for
>> further improvements and keep this comment simpler for review.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
>> ---
>>   block/backup.c     | 357 ++++++++++++++++++++++++++++-----------------
>>   block/trace-events |  12 +-
>>   2 files changed, 231 insertions(+), 138 deletions(-)
>>
>> diff --git a/block/backup.c b/block/backup.c
>> index abb5099fa3..002dee4d7f 100644
>> --- a/block/backup.c
>> +++ b/block/backup.c
>> @@ -35,12 +35,43 @@ typedef struct CowRequest {
>>       CoQueue wait_queue; /* coroutines blocked on this request */
>>   } CowRequest;
>>   
>> +typedef void (*ProgressBytesCallbackFunc)(int64_t bytes, void *opaque);
>> +typedef void (*ProgressResetCallbackFunc)(void *opaque);
>> +typedef struct BlockCopyState {
>> +    BlockBackend *source;
>> +    BlockBackend *target;
>> +    BdrvDirtyBitmap *copy_bitmap;
>> +    int64_t cluster_size;
>> +    bool use_copy_range;
>> +    int64_t copy_range_size;
>> +    uint64_t len;
>> +
>> +    BdrvRequestFlags write_flags;
>> +
>> +    /*
>> +     * skip_unallocated: if true, on copy operation firstly reset areas
>> +     * unallocated in top layer of source (and then of course don't copy
>> +     * corresponding clusters). If some bytes reset, call
>> +     * progress_reset_callback.
>> +     */
> 
> It isn’t quite clear that this refers to the copy_bitmap.  Maybe
> something like
> 
> “If true, the copy operation prepares a sync=top job: It scans the

hmm, now it's not refactored to scan it before copying loop, so it's not precise
wording..

> source's top layer to find all unallocated areas and resets them in the

Not all, but mostly inside block-copy requested area (but may be more)

> copy_bitmap (so they will not be copied).  Whenever any such area is
> cleared, progress_reset_callback will be invoked.
> Once the whole top layer has been scanned, skip_unallocated is cleared
> and the actual copying begins.”

Last sentence sounds like it's a block-copy who will clear skip_unallocated,
but it's not so. It's not very good design and may be refactored in future,
but for now, I'd better drop last sentence.

> 
> instead?

Or, what about the following mix:

Used for job sync=top mode. If true, block_copy() will reset in copy_bitmap 
areas
unallocated in top image (so they will not be copied). Whenever any such area 
is cleared,
progress_reset_callback will be invoked. User is assumed to call in background
block_copy_reset_unallocated() several times to cover the whole copied disk and
then clear skip_unallocated, to prevent extra effort.

> 
>> +    bool skip_unallocated;
>> +
>> +    /* progress_bytes_callback called when some copying progress is done. */
> 
> Maybe add a colon after the attribute name?  (Or drop the name altogether)

OK

> 
>> +    ProgressBytesCallbackFunc progress_bytes_callback;
>> +
>> +    /*
>> +     * progress_reset_callback called when some bytes reset from copy_bitmap
>> +     * (see @skip_unallocated above)
> 
> Maybe you should keep the note you before on what to do then, i.e. that
> the callee should probably recalculate how many bytes remain based on
> the copy_bitmap’s dirty bit count.

OK

> 
>> +     */
>> +    ProgressResetCallbackFunc progress_reset_callback;
>> +    void *progress_opaque;
>> +} BlockCopyState;
> 
> [...]
> 
>> @@ -415,16 +535,16 @@ static void backup_abort(Job *job)
>>   static void backup_clean(Job *job)
>>   {
>>       BackupBlockJob *s = container_of(job, BackupBlockJob, common.job);
>> -    BlockDriverState *bs = blk_bs(s->common.blk);
>> +    BlockCopyState *bcs = s->bcs;
>>   
>> -    if (s->copy_bitmap) {
>> -        bdrv_release_dirty_bitmap(bs, s->copy_bitmap);
>> -        s->copy_bitmap = NULL;
>> -    }
>> +    /*
>> +     * Zero pointer first, to not interleave with backup_drain during some
>> +     * yield. TODO: just block_copy_state_free(s->bcs) after backup_drain
>> +     * dropped.
>> +     */
> 
> I suppose that‘s now. :-)

Hmm, it's in Kevin's branch. Should I rebase on it?

> 
>> +    s->bcs = NULL;
>>   
>> -    assert(s->target);
>> -    blk_unref(s->target);
>> -    s->target = NULL;
>> +    block_copy_state_free(bcs);
>>   }
> 
> [...]
> 
>> @@ -449,8 +569,8 @@ static void backup_drain(BlockJob *job)
>>       /* Need to keep a reference in case blk_drain triggers execution
>>        * of backup_complete...
>>        */
>> -    if (s->target) {
>> -        BlockBackend *target = s->target;
>> +    if (s->bcs) {
>> +        BlockBackend *target = s->bcs->target;
>>           blk_ref(target);
>>           blk_drain(target);
>>           blk_unref(target);
> 
> (And this hunk can go away now.)
> 
> [...]
> 
>> diff --git a/block/trace-events b/block/trace-events
>> index 04209f058d..453792ed87 100644
>> --- a/block/trace-events
>> +++ b/block/trace-events
>> @@ -40,12 +40,12 @@ mirror_yield_in_flight(void *s, int64_t offset, int 
>> in_flight) "s %p offset %" P
>>   # backup.c
>>   backup_do_cow_enter(void *job, int64_t start, int64_t offset, uint64_t 
>> bytes) "job %p start %" PRId64 " offset %" PRId64 " bytes %" PRIu64
>>   backup_do_cow_return(void *job, int64_t offset, uint64_t bytes, int ret) 
>> "job %p offset %" PRId64 " bytes %" PRIu64 " ret %d"
>> -backup_do_cow_skip(void *job, int64_t start) "job %p start %"PRId64
>> -backup_do_cow_skip_range(void *job, int64_t start, uint64_t bytes) "job %p 
>> start %"PRId64" bytes %"PRId64
>> -backup_do_cow_process(void *job, int64_t start) "job %p start %"PRId64
>> -backup_do_cow_read_fail(void *job, int64_t start, int ret) "job %p start 
>> %"PRId64" ret %d"
>> -backup_do_cow_write_fail(void *job, int64_t start, int ret) "job %p start 
>> %"PRId64" ret %d"
>> -backup_do_cow_copy_range_fail(void *job, int64_t start, int ret) "job %p 
>> start %"PRId64" ret %d"
>> +block_copy_skip(void *bcs, int64_t start) "job %p start %"PRId64
>> +block_copy_skip_range(void *bcs, int64_t start, uint64_t bytes) "job %p 
>> start %"PRId64" bytes %"PRId64
>> +block_copy_process(void *bcs, int64_t start) "job %p start %"PRId64
>> +block_copy_with_bounce_buffer_read_fail(void *bcs, int64_t start, int ret) 
>> "job %p start %"PRId64" ret %d"
>> +block_copy_with_bounce_buffer_write_fail(void *bcs, int64_t start, int ret) 
>> "job %p start %"PRId64" ret %d"
>> +block_copy_with_offload_fail(void *bcs, int64_t start, int ret) "job %p 
>> start %"PRId64" ret %d"
> 
> The messages probably should stop calling it a “job”, too.
> 

Oops sorry, I'd better do s/job/bcs/g instead of fixing first job argument by 
hand.


-- 
Best regards,
Vladimir