Re: [RFC PATCH v2 3/4] block: Support multiple reopening with x-blockdev-reopen

[Vladimir Sementsov-Ogievskiy]

01.03.2021 14:07, Kevin Wolf wrote:
> Am 25.02.2021 um 18:02 hat Vladimir Sementsov-Ogievskiy geschrieben:
> > 24.02.2021 15:33, Kevin Wolf wrote:
> > > Am 09.02.2021 um 09:03 hat Vladimir Sementsov-Ogievskiy geschrieben:
> > > > 08.02.2021 21:44, Alberto Garcia wrote:
> > > > > Signed-off-by: Alberto Garcia <berto@igalia.com>
> > > > > ---
> > > > >     qapi/block-core.json       |  2 +-
> > > > >     include/block/block.h      |  1 +
> > > > >     block.c                    | 16 +++++--
> > > > >     blockdev.c                 | 85 +++++++++++++++++++++-----------------
> > > > >     tests/qemu-iotests/155     |  9 ++--
> > > > >     tests/qemu-iotests/165     |  4 +-
> > > > >     tests/qemu-iotests/245     | 27 +++++++-----
> > > > >     tests/qemu-iotests/248     |  2 +-
> > > > >     tests/qemu-iotests/248.out |  2 +-
> > > > >     tests/qemu-iotests/298     |  4 +-
> > > > >     10 files changed, 89 insertions(+), 63 deletions(-)
> > > > >
> > > > > diff --git a/qapi/block-core.json b/qapi/block-core.json
> > > > > index c0e7c23331..b9fcf20a81 100644
> > > > > --- a/qapi/block-core.json
> > > > > +++ b/qapi/block-core.json
> > > > > @@ -4177,7 +4177,7 @@
> > > > >     # Since: 4.0
> > > > >     ##
> > > > >     { 'command': 'x-blockdev-reopen',
> > > > > -  'data': 'BlockdevOptions', 'boxed': true }
> > > > > +  'data': { 'options': ['BlockdevOptions'] } }
> > > >
> > > > Do we also want to drop x- prefix?
> > >
> > > libvirt really wants to have a stable blockdev-reopen interface in 6.0
> > > because enabling the incremental backup code depends on this (they just
> > > toggle the readonly flag if I understand correctly, so most of the work
> > > we're currently doing isn't even relevant at this moment for libvirt).
> >
> > Do you know what is the case exactly? If they do it to remove dirty bitmap
> > from backing image after snapshot operation, probably we'd better improve
> > block-dirty-bitmap-remove command to be able to reopen r-o image?
> >
> > (I just recently faced such a task)
>
> I think it was to switch nodes between read-only and read-write, but I
> don't remember the exact context.

I already don't think that making implicit reopen-to-rw is a good idea. It's OK 
for blockdev-commit, but may be unexpected for bitmaps manipulation.

> > > Given that the soft freeze is coming closer (March 16), I wonder if we
> > > should just make this API change and declare the interface stable. We
> > > can then make Vladimir's fixes and the file reopening on top of it - if
> > > it's in time for 6.0, that would be good, but if not we could move it to
> > > 6.1 without impacting libvirt.
> > >
> > > I think we're reasonable confident that the QAPI interfaces are right,
> > > even if maybe not that all aspects of the implementation are right yet.
> > >
> > > What do you think?
> >
> > I think it's OK.. We have it since 4.0. What will we win keeping -x
> > for years? Even latest change from updating one device to several
> > could be easily done with help of 'alternate' if the command was
> > already stable.
>
> I think your series is kind of important to really call the
> implementation stable. We can always feature flags to indicate the fixes
> if necessary, but it would still feel better to declare something stable
> that doesn't have known bugs. :-)
>
> Do you think your series will still take a while? Maybe my first
> comments sounded a bit negative because it was really hard to review at
> first without knowing the final state, but after all I think the
> approach is sane and apart from some implementation details, we're not
> that far away from getting it into a mergable state.

Thanks :)

I'm now busy with our bugs for Virtuozzo release.. Still, I hope, I'll have a 
chance to reroll permission-update series this week.

--
Best regards,
Vladimir