From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33331) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YwClB-0007Uz-70 for qemu-devel@nongnu.org; Sat, 23 May 2015 12:58:58 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YwCl6-0005jR-2o for qemu-devel@nongnu.org; Sat, 23 May 2015 12:58:53 -0400 Message-ID: <5560B1B8.9000500@redhat.com> Date: Sat, 23 May 2015 18:58:32 +0200 From: Max Reitz MIME-Version: 1.0 References: <1432190583-10518-1-git-send-email-famz@redhat.com> <1432190583-10518-10-git-send-email-famz@redhat.com> In-Reply-To: <1432190583-10518-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 v6 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 21.05.2015 08:42, Fam Zheng wrote: > Signed-off-by: Fam Zheng > --- > blockdev.c | 18 ++++++++++++++++-- > 1 file changed, 16 insertions(+), 2 deletions(-) Unchanged from v5, so the same question: Would it suffice to block I/O only inside external_snapshot_commit()? > 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'd prefer state->blocker, because this block only makes sense if it's non-NULL, and if it is non-NULL, state->old_bs will always be non-NULL, too. Max > + 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, > }, > [TRANSACTION_ACTION_KIND_DRIVE_BACKUP] = { > .instance_size = sizeof(DriveBackupState),