From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:38962)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <mreitz@redhat.com>) id 1Xsp7w-0000ki-9X
	for qemu-devel@nongnu.org; Mon, 24 Nov 2014 03:36:14 -0500
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <mreitz@redhat.com>) id 1Xsp7p-0000oF-NS
	for qemu-devel@nongnu.org; Mon, 24 Nov 2014 03:36:08 -0500
Received: from mx1.redhat.com ([209.132.183.28]:33040)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <mreitz@redhat.com>) id 1Xsp7p-0000nx-HB
	for qemu-devel@nongnu.org; Mon, 24 Nov 2014 03:36:01 -0500
Message-ID: <5472EDE4.6050509@redhat.com>
Date: Mon, 24 Nov 2014 09:35:48 +0100
From: Max Reitz <mreitz@redhat.com>
MIME-Version: 1.0
References: <1414639364-4499-1-git-send-email-famz@redhat.com>	<1414639364-4499-9-git-send-email-famz@redhat.com>
	<5458B279.8010300@redhat.com> <546FBB93.7000809@redhat.com>
In-Reply-To: <546FBB93.7000809@redhat.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 7bit
Subject: Re: [Qemu-devel] [PATCH v6 08/10] qapi: Add transaction support to
 block-dirty-bitmap-{add, enable, disable}
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: John Snow <jsnow@redhat.com>, Fam Zheng <famz@redhat.com>, qemu-devel@nongnu.org
Cc: Kevin Wolf <kwolf@redhat.com>, Benoit Canet <benoit@irqsave.net>, Markus Armbruster <armbru@redhat.com>, Luiz Capitulino <lcapitulino@redhat.com>, Stefan Hajnoczi <stefanha@redhat.com>, Jd <jd_jedi@convirture.com>, Paolo Bonzini <pbonzini@redhat.com>, Vladimir Sementsov-Ogievskij <etendren@gmail.com>

On 2014-11-21 at 23:24, John Snow wrote:
>
>
> On 11/04/2014 06:03 AM, Max Reitz wrote:
>> On 2014-10-30 at 04:22, Fam Zheng wrote:
>>> This adds three qmp commands to transactions.
>>>
>>> Users can stop a dirty bitmap, start backup of it, and start another
>>> dirty bitmap atomically, so that the dirty bitmap is tracked
>>> incrementally and we don't miss any write.
>>>
>>> Signed-off-by: Fam Zheng <famz@redhat.com>
>>> ---
>>>   blockdev.c       | 92
>>> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>   qapi-schema.json |  5 ++-
>>>   2 files changed, 96 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/blockdev.c b/blockdev.c
>>> index fca909b..34fa314 100644
>>> --- a/blockdev.c
>>> +++ b/blockdev.c
>>> @@ -1490,6 +1490,83 @@ static void
>>> drive_backup_abort(BlkTransactionState *common)
>>>       }
>>>   }
>>> +static void block_dirty_bitmap_add_prepare(BlkTransactionState 
>>> *common,
>>> +                                           Error **errp)
>>> +{
>>> +    BlockDirtyBitmapAdd *action;
>>> +
>>> +    action = common->action->block_dirty_bitmap_add;
>>> +    qmp_block_dirty_bitmap_add(action->device, action->name,
>>> +                               action->has_granularity,
>>> action->granularity,
>>> +                               errp);
>>> +}
>>> +
>>> +static void block_dirty_bitmap_add_abort(BlkTransactionState *common)
>>> +{
>>> +    BlockDirtyBitmapAdd *action;
>>> +    BdrvDirtyBitmap *bm;
>>> +    BlockDriverState *bs;
>>> +
>>> +    action = common->action->block_dirty_bitmap_add;
>>> +    bs = bdrv_find(action->device);
>>> +    if (bs) {
>>> +        bm = bdrv_find_dirty_bitmap(bs, action->name);
>>> +        if (bm) {
>>> +            bdrv_release_dirty_bitmap(bs, bm);
>>> +        }
>>> +    }
>>> +}
>>> +
>>> +static void block_dirty_bitmap_enable_prepare(BlkTransactionState
>>> *common,
>>> +                                               Error **errp)
>>> +{
>>> +    BlockDirtyBitmap *action;
>>> +
>>> +    action = common->action->block_dirty_bitmap_enable;
>>> +    qmp_block_dirty_bitmap_enable(action->device, action->name, errp);
>>> +}
>>> +
>>> +static void block_dirty_bitmap_enable_abort(BlkTransactionState 
>>> *common)
>>> +{
>>> +    BlockDirtyBitmap *action;
>>> +    BdrvDirtyBitmap *bitmap;
>>> +    BlockDriverState *bs;
>>> +
>>> +    action = common->action->block_dirty_bitmap_enable;
>>> +    bs = bdrv_find(action->device);
>>> +    if (bs) {
>>> +        bitmap = bdrv_find_dirty_bitmap(bs, action->name);
>>> +        if (bitmap) {
>>> +            bdrv_disable_dirty_bitmap(bs, bitmap);
>>
>> But what if the dirty bitmap was enabled before so that this enabling
>> transaction was supposed to be a no-op?
>
> Maybe it's not a problem: The only case in which the enable/disable 
> operation will fail is if it cannot find the device or bitmap -- in 
> which case, the abort operation isn't going to be able to affect 
> anything either.

Well, it's part of a transaction. As far as I understand it, one groups 
several operations in one transaction and if any fails, all are aborted. 
Therefore, the "enable the dirty bitmap" operation can trivially succeed 
(because it was enabled before), but some different operation may fail, 
which then results in invocation of this abort function.

Max

> Unless you know something I don't.
>
> Still, it does look goofy. Thoughts?