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

[Vladimir Sementsov-Ogievskiy]

01.09.2021 14:57, Hanna Reitz wrote:
> 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?

If it helps, than OK of course, thanks!

> 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


--
Best regards,
Vladimir