From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59480) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YwCWv-00081h-DQ for qemu-devel@nongnu.org; Sat, 23 May 2015 12:44:14 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YwCWq-0007Db-Mp for qemu-devel@nongnu.org; Sat, 23 May 2015 12:44:09 -0400 Message-ID: <5560AE42.4020903@redhat.com> Date: Sat, 23 May 2015 18:43:46 +0200 From: Max Reitz MIME-Version: 1.0 References: <1432102576-6637-1-git-send-email-famz@redhat.com> <1432102576-6637-10-git-send-email-famz@redhat.com> In-Reply-To: <1432102576-6637-10-git-send-email-famz@redhat.com> Content-Type: text/plain; charset=iso-8859-15; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v5 09/13] blockdev: Block device IO during external snapshot transaction List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Fam Zheng , qemu-devel@nongnu.org Cc: Kevin Wolf , qemu-block@nongnu.org, jcody@redhat.com, armbru@redhat.com, Stefan Hajnoczi , amit.shah@redhat.com, pbonzini@redhat.com On 20.05.2015 08:16, Fam Zheng wrote: > Signed-off-by: Fam Zheng > --- > blockdev.c | 18 ++++++++++++++++-- > 1 file changed, 16 insertions(+), 2 deletions(-) Can this be pulled into external_snapshot_commit()? Or might that pose problems with other operations in the same transaction? > diff --git a/blockdev.c b/blockdev.c > index 7f763d9..923fc90 100644 > --- a/blockdev.c > +++ b/blockdev.c > @@ -1404,6 +1404,7 @@ typedef struct ExternalSnapshotState { > BlockDriverState *old_bs; > BlockDriverState *new_bs; > AioContext *aio_context; > + Error *blocker; > } ExternalSnapshotState; > > static void external_snapshot_prepare(BlkTransactionState *common, > @@ -1525,6 +1526,9 @@ static void external_snapshot_prepare(BlkTransactionState *common, > if (ret != 0) { > error_propagate(errp, local_err); > } > + > + error_setg(&state->blocker, "snapshot in progress"); > + bdrv_op_block(state->old_bs, BLOCK_OP_TYPE_DEVICE_IO, state->blocker); > } > > static void external_snapshot_commit(BlkTransactionState *common) > @@ -1541,8 +1545,6 @@ static void external_snapshot_commit(BlkTransactionState *common) > * don't want to abort all of them if one of them fails the reopen */ > bdrv_reopen(state->new_bs, state->new_bs->open_flags & ~BDRV_O_RDWR, > NULL); > - > - aio_context_release(state->aio_context); > } > > static void external_snapshot_abort(BlkTransactionState *common) > @@ -1552,6 +1554,17 @@ static void external_snapshot_abort(BlkTransactionState *common) > if (state->new_bs) { > bdrv_unref(state->new_bs); > } > +} > + > +static void external_snapshot_clean(BlkTransactionState *common) > +{ > + ExternalSnapshotState *state = > + DO_UPCAST(ExternalSnapshotState, common, common); > + > + if (state->old_bs) { I think it would be better to make this state->blocker. > + bdrv_op_unblock(state->old_bs, BLOCK_OP_TYPE_DEVICE_IO, state->blocker); > + error_free(state->blocker); > + } > if (state->aio_context) { > aio_context_release(state->aio_context); > } > @@ -1716,6 +1729,7 @@ static const BdrvActionOps actions[] = { > .prepare = external_snapshot_prepare, > .commit = external_snapshot_commit, > .abort = external_snapshot_abort, > + .clean = external_snapshot_clean, Oh. Er, that's nice... Max > }, > [TRANSACTION_ACTION_KIND_DRIVE_BACKUP] = { > .instance_size = sizeof(DriveBackupState),