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

[Vladimir Sementsov-Ogievskiy]

04.11.2020 20:19, Max Reitz wrote:
> On 22.10.20 22:35, Vladimir Sementsov-Ogievskiy wrote:
> > 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()...
>
> Isn’t the logic basically just “cluster size of the target image file
> (min 64k)”?  The other cases just cover error cases, and they always
> return 64k, which would effectively be documented by stating that 64k is
> the minimum.
>
> But in the common cases (qcow2 or raw), we’ll never get an error anyway.
>
> I think it’d be good to describe the cluster size somewhere, because
> otherwise I find it a bit malicious to throw an error at the user that
> they could not have anticipated because they have no idea what valid
> values for the parameter are (until they try).

OK

> > >   (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.
>
> Ah, OK.  Right.
>
> Max


--
Best regards,
Vladimir