Re: cluster_size got from backup_calculate_cluster_size()

[John Snow]

On 5/21/20 5:56 AM, Derek Su wrote:
> Hi,
> 
> The cluster_size got from backup_calculate_cluster_size(),
> MAX(BACKUP_CLUSTER_SIZE_DEFAULT, bdi.cluster_size), is 64K regardless
> of the target image's cluster size.

Not regardless -- but it is using 64K as a minimum.

> 
> 
> For example:
> 
> If the cluster size of source and target qcow2 images are both 16K, the
> 64K from backup_calculate_cluster_size() results in unwanted copies of
> clusters.
> 
> The behavior slows down the backup (block-copy) process when the
> source image receives lots of rand writes.

Are we talking about incremental, full, or top?

> 
> 
> Is the following calculation reasonable for the above issue?
> 
> 
> ```
> static int64_t backup_calculate_cluster_size(BlockDriverState *target,
>                                              Error **errp)
> {
>     ...
> 
>     ret = bdrv_get_info(target, &bdi);
> 
>     ...
> 
>     return (bdi.cluster_size == 0 ?
>                 BACKUP_CLUSTER_SIZE_DEFAULT : cluster_size);
> }
> 
> ```
> 
> Thanks.
> Regards,
> 
> Derek
> 


Might be reasonable. When I made this the "default", it actually used to
just be "hardcoded." We could continue in this direction and go all the
way and turn it into a tune-able.

I didn't think to make it lower than 64K because I was thinking about
the linear, full backup case where cluster sizes that were too small
might slow down the loop with too much metadata.

(And the default qcow2 is 64K, so it seemed like a good choice at the time.)

We could create a default-cluster-size tunable, perhaps. It's at 64K
now, but perhaps you can override it down to 16 or lower. We could
possibly treat a value of 0 as "no minimum; use the smallest common size."

This is a separate issue entirely, but we may also wish to begin
offering a cluster-size override directly. In the case that we know this
value is unsafe, we reject the command. In the case that it's ambiguous
due to lack of info, we can defer to the user's judgment. This would
allow us to force the backup to run in cases where we presently abort
out of fear.

CCing Vladimir who has been working on the backup loop extensively.

--js