Re: [PATCH v2 11/20] qapi: backup: add x-max-chunk and x-max-workers parameters

[Vladimir Sementsov-Ogievskiy]

22.07.2020 15:22, Max Reitz wrote:
> On 01.06.20 20:11, Vladimir Sementsov-Ogievskiy wrote:
> > Add new parameters to configure future backup features. The patch
> > doesn't introduce aio backup requests (so we actually have only one
> > worker) neither requests larger than one cluster. Still, formally we
> > satisfy these maximums anyway, so add the parameters now, to facilitate
> > further patch which will really change backup job behavior.
> >
> > Options are added with x- prefixes, as the only use for them are some
> > very conservative iotests which will be updated soon.
> >
> > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> > ---
> >   qapi/block-core.json      |  9 ++++++++-
> >   include/block/block_int.h |  7 +++++++
> >   block/backup.c            | 21 +++++++++++++++++++++
> >   block/replication.c       |  2 +-
> >   blockdev.c                |  5 +++++
> >   5 files changed, 42 insertions(+), 2 deletions(-)

[..]

> > @@ -422,6 +436,11 @@ BlockJob *backup_job_create(const char *job_id, 
> > BlockDriverState *bs,
> >       if (cluster_size < 0) {
> >           goto error;
> >       }
> > +    if (max_chunk && max_chunk < cluster_size) {
> > +        error_setg(errp, "Required max-chunk (%" PRIi64") is less than backup "
>
> (missing a space after PRIi64)
>
> > +                   "cluster size (%" PRIi64 ")", max_chunk, cluster_size);
>
> Should this be noted in the QAPI documentation?

Hmm.. It makes sense, but I don't know what to write: should be >= job 
cluster_size? But then I'll have to describe the logic of 
backup_calculate_cluster_size()...

>  (And perhaps the fact
> that without copy offloading, we’ll never copy anything bigger than one
> cluster at a time anyway?)

This is a parameter for background copying. Look at block_copy_task_create(), 
if call_state has own max_chunk, it's used instead of common copy_size (derived 
from cluster_size). But at a moment of this patch background process through 
async block-copy is not  yet implemented, and the parameter doesn't work, which 
is described in commit message.

> > +        return NULL;
> > +    }
> >   
> >       /*

[..]


--
Best regards,
Vladimir