Re: [Qemu-block] [PATCH v3 2/3] block/backup: avoid copying less than full target clusters

[John Snow]

On 02/23/2016 07:27 PM, Fam Zheng wrote:
> On Tue, 02/23 19:17, John Snow wrote:
>> During incremental backups, if the target has a cluster size that is
>> larger than the backup cluster size and we are backing up to a target
>> that cannot (for whichever reason) pull clusters up from a backing image,
>> we may inadvertantly create unusable incremental backup images.
>>
>> For example:
>>
>> If the bitmap tracks changes at a 64KB granularity and we transmit 64KB
>> of data at a time but the target uses a 128KB cluster size, it is
>> possible that only half of a target cluster will be recognized as dirty
>> by the backup block job. When the cluster is allocated on the target
>> image but only half populated with data, we lose the ability to
>> distinguish between zero padding and uninitialized data.
>>
>> This does not happen if the target image has a backing file that points
>> to the last known good backup.
>>
>> Even if we have a backing file, though, it's likely going to be faster
>> to just buffer the redundant data ourselves from the live image than
>> fetching it from the backing file, so let's just always round up to the
>> target granularity.
>>
>> The same logic applies to backup modes top, none, and full. Copying
>> fractional clusters without the guarantee of COW is dangerous, but even
>> if we can rely on COW, it's likely better to just re-copy the data.
>>
>> Reported-by: Fam Zheng <address@hidden>
>> Signed-off-by: John Snow <address@hidden>
>> ---
>>  block/backup.c | 23 ++++++++++++++++++++---
>>  1 file changed, 20 insertions(+), 3 deletions(-)
>>
>> diff --git a/block/backup.c b/block/backup.c
>> index 76addef..6e9f53d 100644
>> --- a/block/backup.c
>> +++ b/block/backup.c
>> @@ -501,6 +501,8 @@ void backup_start(BlockDriverState *bs, BlockDriverState 
>> *target,
>>                    BlockJobTxn *txn, Error **errp)
>>  {
>>      int64_t len;
>> +    BlockDriverInfo bdi;
>> +    int ret;
>>  
>>      assert(bs);
>>      assert(target);
>> @@ -570,15 +572,30 @@ void backup_start(BlockDriverState *bs, 
>> BlockDriverState *target,
>>          goto error;
>>      }
>>  
>> -    bdrv_op_block_all(target, job->common.blocker);
>> -
>>      job->on_source_error = on_source_error;
>>      job->on_target_error = on_target_error;
>>      job->target = target;
>>      job->sync_mode = sync_mode;
>>      job->sync_bitmap = sync_mode == MIRROR_SYNC_MODE_INCREMENTAL ?
>>                         sync_bitmap : NULL;
>> -    job->cluster_size = BACKUP_CLUSTER_SIZE_DEFAULT;
>> +
>> +    /* If there is no backing file on the target, we cannot rely on COW if 
>> our
>> +     * backup cluster size is smaller than the target cluster size. Even for
>> +     * targets with a backing file, try to avoid COW if possible. */
>> +    ret = bdrv_get_info(job->target, &bdi);
>> +    if (ret < 0 && !target->backing) {
>> +        error_setg_errno(errp, -ret,
>> +            "Can't determine cluster size of target that has no backing 
>> file. "
>> +            "This may create an unusable destination image. Aborting.");
> 
> Most error messages don't end with a period, but that's not a hard rule 
> AFAICT.

If that's the only problem, Jeff can touch it up.

I want to ask, though:

"Are there any cases where we will have no backing file and no
bdrv_get_info implementation, but backup should still succeed?"

I think the answer is /no/ since everything we claim to support for use
with QEMU (instead of qemu-img only) should implement bdrv_get_info.

> 
>> +        goto error;
>> +    } else if (ret < 0 && target->backing) {
>> +        /* Not fatal; just trudge on ahead. */
>> +        job->cluster_size = BACKUP_CLUSTER_SIZE_DEFAULT;
>> +    } else {
>> +        job->cluster_size = MAX(BACKUP_CLUSTER_SIZE_DEFAULT, 
>> bdi.cluster_size);
>> +    }
>> +
>> +    bdrv_op_block_all(target, job->common.blocker);
>>      job->common.len = len;
>>      job->common.co = qemu_coroutine_create(backup_run);
>>      block_job_txn_add_job(txn, &job->common);
>> -- 
>> 2.4.3
>>
> 
> Reviewed-by: Fam Zheng <address@hidden>
> 

Thanks.