Re: [PATCH v8 09/34] block/backup: move cluster size calculation to block-copy

[Hanna Reitz <hreitz@redhat.com>]

On 24.08.21 10:38, Vladimir Sementsov-Ogievskiy wrote:
> The main consumer of cluster-size is block-copy. Let's calculate it
> here instead of passing through backup-top.
>
> We are going to publish copy-before-write filter soon, so it will be
> created through options. But we don't want for now to make explicit
> option for cluster-size, let's continue to calculate it automatically.
> So, now is the time to get rid of cluster_size argument for
> bdrv_cbw_append().
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> ---
>   block/copy-before-write.h  |  1 -
>   include/block/block-copy.h |  5 +--
>   block/backup.c             | 62 ++++++--------------------------------
>   block/block-copy.c         | 51 ++++++++++++++++++++++++++++++-
>   block/copy-before-write.c  | 10 +++---
>   5 files changed, 66 insertions(+), 63 deletions(-)

[...]

> diff --git a/block/block-copy.c b/block/block-copy.c
> index e83870ff81..b0e0a38330 100644
> --- a/block/block-copy.c
> +++ b/block/block-copy.c

[...]

> @@ -342,14 +343,57 @@ void block_copy_set_copy_opts(BlockCopyState *s, bool use_copy_range,
>       }
>   }
>   
> +static int64_t block_copy_calculate_cluster_size(BlockDriverState *target,
> +                                                 Error **errp)
> +{
> +    int ret;
> +    BlockDriverInfo bdi;
> +    bool target_does_cow = bdrv_backing_chain_next(target);
> +
> +    /*
> +     * 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(target, &bdi);
> +    if (ret == -ENOTSUP && !target_does_cow) {
> +        /* Cluster size is not defined */
> +        warn_report("The target block device doesn't provide "
> +                    "information about the block size and it doesn't have a "
> +                    "backing file. The default block size of %u bytes is "
> +                    "used. If the actual block size of the target exceeds "
> +                    "this default, the backup may be unusable",
> +                    BLOCK_COPY_CLUSTER_SIZE_DEFAULT);

I get some build errors in the gitlab CI because of this – I think we 
need to add a qemu/error-report.h include in block/block-copy.c now.  (I 
don’t know why this works for some configurations, apparently, but not 
for others...)

Is it OK if I add it to this patch?

diff --git a/block/block-copy.c b/block/block-copy.c
index b0e0a38330..5d0076ac93 100644
--- a/block/block-copy.c
+++ b/block/block-copy.c
@@ -21,6 +21,7 @@
  #include "qemu/units.h"
  #include "qemu/coroutine.h"
  #include "block/aio_task.h"
+#include "qemu/error-report.h"

  #define BLOCK_COPY_MAX_COPY_RANGE (16 * MiB)
  #define BLOCK_COPY_MAX_BUFFER (1 * MiB)

Hanna