Re: [Qemu-block] [PATCH 16/22] qmp: add persistent flag to block-dirty-bitmap-add

[Vladimir Sementsov-Ogievskiy]

24.10.2016 20:30, Max Reitz wrote:
> On 24.10.2016 17:12, Vladimir Sementsov-Ogievskiy wrote:
> > 10.10.2016 19:08, Max Reitz wrote:
> > > On 30.09.2016 12:53, Vladimir Sementsov-Ogievskiy wrote:
> > [...]
> >
> > > > +    }
> > > >       out:
> > > >        aio_context_release(aio_context);
> > > > diff --git a/qapi/block-core.json b/qapi/block-core.json
> > > > index 31f9990..2bf56cd 100644
> > > > --- a/qapi/block-core.json
> > > > +++ b/qapi/block-core.json
> > > > @@ -1235,10 +1235,15 @@
> > > >    # @granularity: #optional the bitmap granularity, default is 64k for
> > > >    #               block-dirty-bitmap-add
> > > >    #
> > > > +# @persistent: #optional the bitmap is persistent, i.e. it will be
> > > > saved to
> > > > +#              corresponding block device on it's close. Default is
> > > > false.
> > > > +#              For block-dirty-bitmap-add. (Since 2.8)
> > > I'm not sure what the "For block-dirty-bitmap-add." is supposed to mean,
> > > because this whole struct is for block-dirty-bitmap-add (and for
> > > block-dirty-bitmap-add transactions, to be exact, but @persistent will
> > > surely work there, too, won't it?).
> > >
> > > Also, I'd say "will be saved to the corresponding block device image
> > > file" instead of just "block device", because in my understanding a
> > > block device and its image file are two separate things.
> > Hmm.. But 'its close' is block device close, not file close.
> In my understanding, it is the file close.
>
> >                                                               And we call
> > common bdrv_* function to save it, so we actually save it to device, and
> > then the device puzzles out, how to actually save it.
> Well, OK, it depends on what you mean by "block device". There are many
> things we call a "block device", but nowadays I think it mostly refers
> to either a guest block device or a BlockBackend (and as of lately,
> we're more and more trying to hide the BlockBackend from the user, so
> you could argue that it's only the guest device now).
>
> The bdrv_* functions operate on block layer BDS nodes, and I don't think
> we call them "block devices" (at least not any more).
>
> In any case, I think for users the term "block device" refers to either
> the device presented to the guest or to all of the block layer stuff
> that's underneath, and it's not quite clear how you could save a bitmap
> to that, or what it's supposed to mean to "close" a block device (you
> can remove it, you can destroy it, you can delete it, but "close" it?).
>
> But saying that it will be saved to the image file that is attached to
> the block device will make it absolutely clear what we mean.
>
> So what you have called a "device" here is neither what I'd call a
> device (I'd call it a "node" or "BDS"), nor what I think users would
> call a device. Also, it's not the BDS that puzzles out how to save it
> but some block driver.

Ok, thank you.

> > > > +#
> > > >    # Since 2.4
> > > >    ##
> > > >    { 'struct': 'BlockDirtyBitmapAdd',
> > > > -  'data': { 'node': 'str', 'name': 'str', '*granularity': 'uint32' } }
> > > > +  'data': { 'node': 'str', 'name': 'str', '*granularity': 'uint32',
> > > > +  '*persistent': 'bool' } }
> > > I think normally we'd align the new line so that the opening ' of
> > > '*persistent' is under the opening ' of 'node'.
> > >
> > > >      ##
> > > >    # @block-dirty-bitmap-add
> > > > diff --git a/qmp-commands.hx b/qmp-commands.hx
> > > > index ba2a916..434b418 100644
> > > > --- a/qmp-commands.hx
> > > > +++ b/qmp-commands.hx
> > > > @@ -1441,7 +1441,7 @@ EQMP
> > > >          {
> > > >            .name       = "block-dirty-bitmap-add",
> > > > -        .args_type  = "node:B,name:s,granularity:i?",
> > > > +        .args_type  = "node:B,name:s,granularity:i?,persistent:b?",
> > > >            .mhandler.cmd_new = qmp_marshal_block_dirty_bitmap_add,
> > > >        },
> > > >    @@ -1458,6 +1458,9 @@ Arguments:
> > > >    - "node": device/node on which to create dirty bitmap (json-string)
> > > >    - "name": name of the new dirty bitmap (json-string)
> > > >    - "granularity": granularity to track writes with (int, optional)
> > > > +- "persistent": bitmap will be saved to corresponding block device
> > > > +                on it's close. Block driver should maintain
> > > > persistent bitmaps
> > > > +                (json-bool, optional, default false) (Since 2.8)
> > > And I don't know what the user is supposed to make of the information
> > > that block drivers will take care of maintaining persistent bitmaps. All
> > > they care about is that it will be stored in the corresponding image
> > > file, so in my opinion it would be better to just omit the last sentence
> > > here.
> > Users shoud know, that only qcow2 supports persistent bitmaps. Instead
> > of last sentence I can write "For now only Qcow2 disks supports
> > persistent bitmaps".
> s/supports/support/, but yes, that sounds preferable to me.
>
> Max


--
Best regards,
Vladimir