From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57292) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YqPVL-00083r-3J for qemu-devel@nongnu.org; Thu, 07 May 2015 13:22:35 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YqPVH-0000DE-4K for qemu-devel@nongnu.org; Thu, 07 May 2015 13:22:35 -0400 Message-ID: <554B9F52.9020003@redhat.com> Date: Thu, 07 May 2015 13:22:26 -0400 From: John Snow MIME-Version: 1.0 References: <1429747493-24397-1-git-send-email-jsnow@redhat.com> <1429747493-24397-2-git-send-email-jsnow@redhat.com> <20150507145440.GN13985@stefanha-thinkpad.redhat.com> In-Reply-To: <20150507145440.GN13985@stefanha-thinkpad.redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [Qemu-block] [PATCH v3 01/10] qapi: Add transaction support to block-dirty-bitmap operations List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: famz@redhat.com, qemu-block@nongnu.org, qemu-devel@nongnu.org, mreitz@redhat.com, vsementsov@parallels.com, stefanha@redhat.com On 05/07/2015 10:54 AM, Stefan Hajnoczi wrote: > On Wed, Apr 22, 2015 at 08:04:44PM -0400, John Snow wrote: >> +static void block_dirty_bitmap_clear_prepare(BlkTransactionState >> *common, + Error >> **errp) +{ + BlockDirtyBitmapState *state = >> DO_UPCAST(BlockDirtyBitmapState, + >> common, common); + BlockDirtyBitmap *action; + + action = >> common->action->block_dirty_bitmap_clear; + state->bitmap = >> block_dirty_bitmap_lookup(action->node, + >> action->name, + >> &state->bs, + >> &state->aio_context, + >> errp); + if (!state->bitmap) { + return; + } + + >> if (bdrv_dirty_bitmap_frozen(state->bitmap)) { + >> error_setg(errp, "Cannot modify a frozen bitmap"); + >> return; + } else if >> (!bdrv_dirty_bitmap_enabled(state->bitmap)) { + >> error_setg(errp, "Cannot clear a disabled bitmap"); + >> return; + } + + /* AioContext is released in .clean() */ >> +} + +static void >> block_dirty_bitmap_clear_commit(BlkTransactionState *common) +{ + >> BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState, + >> common, common); + bdrv_clear_dirty_bitmap(state->bitmap); +} > > These semantics don't work in this example: > > [block-dirty-bitmap-clear, drive-backup] > > Since drive-backup starts the blockjob in .prepare() but > block-dirty-bitmap-clear only clears the bitmap in .commit() the > order is wrong. > > .prepare() has to do something non-destructive, like stashing away > the HBitmap and replacing it with an empty one. Then .commit() can > discard the old bitmap while .abort() can move the old bitmap back > to undo the operation. > > Stefan > Hmm, that's sort of gross. That means that any transactional command *ever* destined to be used with drive-backup in any conceivable way needs to move a lot more of its action forward to .prepare(). That sort of defeats the premise of .prepare() and .commit(), no? And all because drive-backup jumped the gun. That's going to get hard to maintain as we add more transactions. --js