Re: [PATCH v2 03/20] qapi: backup: add x-use-copy-range parameter

[Vladimir Sementsov-Ogievskiy]

17.07.2020 16:15, Max Reitz wrote:
> On 01.06.20 20:11, Vladimir Sementsov-Ogievskiy wrote:
> > Add parameter to enable/disable copy_range. Keep current default for
> > now (enabled).
>
> Why x-, though?  I can’t think of a reason why we would have to remove this.

I add some x- arguments in these series:

 1. I'm unsure about default values (still, it can be changed even without x- 
prefixes as I understand)
 2. Probably all this should be wrapped into common "block-copy" options, to 
not add same arguments to different block-jobs, when they all (I believe in bright future 
:) move on block-copy.

So, this series is not about API, but for new backup architecture, and 
experimental APIs are needed only to experiment with performance and adjust 
some iotests.

> > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> > ---
> >   qapi/block-core.json       | 4 +++-
> >   block/backup-top.h         | 1 +
> >   include/block/block-copy.h | 2 +-
> >   include/block/block_int.h  | 1 +
> >   block/backup-top.c         | 4 +++-
> >   block/backup.c             | 4 +++-
> >   block/block-copy.c         | 4 ++--
> >   block/replication.c        | 1 +
> >   blockdev.c                 | 5 +++++
> >   9 files changed, 20 insertions(+), 6 deletions(-)
> >
> > diff --git a/qapi/block-core.json b/qapi/block-core.json
> > index 6fbacddab2..0c7600e4ec 100644
> > --- a/qapi/block-core.json
> > +++ b/qapi/block-core.json
> > @@ -1405,6 +1405,8 @@
> >   #                    above node specified by @drive. If this option is not 
> > given,
> >   #                    a node name is autogenerated. (Since: 4.2)
> >   #
> > +# @x-use-copy-range: use copy offloading if possible. Default true. (Since 5.1)
>
> Would it make more sense to invert it to disable-copy-range?  First,
> this would make for a cleaner meaning, because it would allow dropping
> the “if possible” part.  Setting use-copy-range=true would intuitively
> imply to me that I get an error if copy-range cannot be used.  Sure,
> there’s this little “if possible” in the documentation, but it goes
> against my intuition.  disable-copy-range=true is intuitively clear.
> Second, this would give us a default of false, which is marginally nicer.

Reasonable. But we also should consider disabling copy_range by default as I 
propose.. But yes, if we keep x- for now, and don't change default for 
copy_range in this series, I can invert it just for readability.

Thanks a lot for reviewing this!

--
Best regards,
Vladimir