From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50439) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1byzYL-0004wW-5o for qemu-devel@nongnu.org; Tue, 25 Oct 2016 07:06:01 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1byzYH-0006M7-2z for qemu-devel@nongnu.org; Tue, 25 Oct 2016 07:05:57 -0400 Received: from mailhub.sw.ru ([195.214.232.25]:30919 helo=relay.sw.ru) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1byzYG-0006LD-Mr for qemu-devel@nongnu.org; Tue, 25 Oct 2016 07:05:52 -0400 References: <1475232808-4852-1-git-send-email-vsementsov@virtuozzo.com> <1475232808-4852-17-git-send-email-vsementsov@virtuozzo.com> From: Vladimir Sementsov-Ogievskiy Message-ID: Date: Tue, 25 Oct 2016 14:05:36 +0300 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 16/22] qmp: add persistent flag to block-dirty-bitmap-add List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Max Reitz , qemu-block@nongnu.org, qemu-devel@nongnu.org Cc: kwolf@redhat.com, armbru@redhat.com, eblake@redhat.com, jsnow@redhat.com, famz@redhat.com, den@openvz.org, stefanha@redhat.com, pbonzini@redhat.com 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