From patchwork Thu Jun 6 18:41:56 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: John Snow X-Patchwork-Id: 10980327 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id 9AFD692A for ; Thu, 6 Jun 2019 19:12:29 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 8A3BF22B39 for ; Thu, 6 Jun 2019 19:12:29 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 7DBE9286DB; Thu, 6 Jun 2019 19:12:29 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-2.9 required=2.0 tests=BAYES_00,MAILING_LIST_MULTI autolearn=ham version=3.3.1 Received: from lists.gnu.org (lists.gnu.org [209.51.188.47]) (using TLSv1.2 with cipher AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id 76E5022B39 for ; Thu, 6 Jun 2019 19:12:27 +0000 (UTC) Received: from localhost ([::1]:53956 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.86_2) (envelope-from ) id 1hYxoE-0006eX-NL for patchwork-qemu-devel@patchwork.kernel.org; Thu, 06 Jun 2019 15:12:26 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:53352) by lists.gnu.org with esmtp (Exim 4.86_2) (envelope-from ) id 1hYxlF-0004T3-W5 for qemu-devel@nongnu.org; Thu, 06 Jun 2019 15:09:22 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1hYxLT-0000R8-FG for qemu-devel@nongnu.org; Thu, 06 Jun 2019 14:42:42 -0400 Received: from mx1.redhat.com ([209.132.183.28]:43852) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1hYxLG-0007qf-OM; Thu, 06 Jun 2019 14:42:28 -0400 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id ED22830832CE; Thu, 6 Jun 2019 18:42:06 +0000 (UTC) Received: from probe.bos.redhat.com (dhcp-17-177.bos.redhat.com [10.18.17.177]) by smtp.corp.redhat.com (Postfix) with ESMTP id 089115C2EB; Thu, 6 Jun 2019 18:42:05 +0000 (UTC) From: John Snow To: qemu-block@nongnu.org, qemu-devel@nongnu.org Date: Thu, 6 Jun 2019 14:41:56 -0400 Message-Id: <20190606184159.979-3-jsnow@redhat.com> In-Reply-To: <20190606184159.979-1-jsnow@redhat.com> References: <20190606184159.979-1-jsnow@redhat.com> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.44]); Thu, 06 Jun 2019 18:42:07 +0000 (UTC) X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-Received-From: 209.132.183.28 Subject: [Qemu-devel] [PATCH 2/5] block/dirty-bitmap: Refactor bdrv_can_store_new_bitmap X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Fam Zheng , Kevin Wolf , vsementsov@virtuozzo.com, Markus Armbruster , Max Reitz , John Snow Errors-To: qemu-devel-bounces+patchwork-qemu-devel=patchwork.kernel.org@nongnu.org Sender: "Qemu-devel" X-Virus-Scanned: ClamAV using ClamSMTP Instead of bdrv_can_store_new_bitmap, rework this as bdrv_add_persistent_dirty_bitmap. This makes a more obvious symmetry with bdrv_remove_persistent_dirty_bitmap. Most importantly, we are free to modify the driver state because we know we ARE adding a bitmap instead of simply being asked if we CAN store one. As part of this symmetry, move this function next to bdrv_remove_persistent_bitmap in block/dirty-bitmap.c. To cement this semantic distinction, use a bitmap object instead of the (name, granularity) pair as this allows us to set persistence as a condition of the "add" succeeding. Notably, the qcow2 implementation still does not actually store the new bitmap at add time, but merely ensures that it will be able to at store time. Signed-off-by: John Snow Reviewed-by: Eric Blake Reviewed-by: Vladimir Sementsov-Ogievskiy --- block/qcow2.h | 5 ++--- include/block/block.h | 2 -- include/block/block_int.h | 5 ++--- include/block/dirty-bitmap.h | 3 +++ block.c | 22 --------------------- block/dirty-bitmap.c | 38 ++++++++++++++++++++++++++++++++++++ block/qcow2-bitmap.c | 24 ++++++++++++++--------- block/qcow2.c | 2 +- blockdev.c | 19 +++++++----------- 9 files changed, 68 insertions(+), 52 deletions(-) diff --git a/block/qcow2.h b/block/qcow2.h index fc1b0d3c1e..95d723d3c0 100644 --- a/block/qcow2.h +++ b/block/qcow2.h @@ -742,9 +742,8 @@ int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp); int qcow2_truncate_bitmaps_check(BlockDriverState *bs, Error **errp); void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp); int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp); -bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs, - const char *name, - uint32_t granularity, +int qcow2_add_persistent_dirty_bitmap(BlockDriverState *bs, + BdrvDirtyBitmap *bitmap, Error **errp); void qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs, const char *name, diff --git a/include/block/block.h b/include/block/block.h index f9415ed740..6d520f3b59 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -683,8 +683,6 @@ void bdrv_add_child(BlockDriverState *parent, BlockDriverState *child, Error **errp); void bdrv_del_child(BlockDriverState *parent, BdrvChild *child, Error **errp); -bool bdrv_can_store_new_dirty_bitmap(BlockDriverState *bs, const char *name, - uint32_t granularity, Error **errp); /** * * bdrv_register_buf/bdrv_unregister_buf: diff --git a/include/block/block_int.h b/include/block/block_int.h index 06df2bda1b..93bbb66cd0 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -537,9 +537,8 @@ struct BlockDriver { * field of BlockDirtyBitmap's in case of success. */ int (*bdrv_reopen_bitmaps_rw)(BlockDriverState *bs, Error **errp); - bool (*bdrv_can_store_new_dirty_bitmap)(BlockDriverState *bs, - const char *name, - uint32_t granularity, + int (*bdrv_add_persistent_dirty_bitmap)(BlockDriverState *bs, + BdrvDirtyBitmap *bitmap, Error **errp); void (*bdrv_remove_persistent_dirty_bitmap)(BlockDriverState *bs, const char *name, diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h index 8044ace63e..c37edbe05f 100644 --- a/include/block/dirty-bitmap.h +++ b/include/block/dirty-bitmap.h @@ -38,6 +38,9 @@ int bdrv_dirty_bitmap_check(const BdrvDirtyBitmap *bitmap, uint32_t flags, Error **errp); void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap); void bdrv_release_named_dirty_bitmaps(BlockDriverState *bs); +int bdrv_add_persistent_dirty_bitmap(BlockDriverState *bs, + BdrvDirtyBitmap *bitmap, + Error **errp); void bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs, const char *name, Error **errp); diff --git a/block.c b/block.c index e3e77feee0..6aec36b7c9 100644 --- a/block.c +++ b/block.c @@ -6379,25 +6379,3 @@ void bdrv_del_child(BlockDriverState *parent_bs, BdrvChild *child, Error **errp) parent_bs->drv->bdrv_del_child(parent_bs, child, errp); } - -bool bdrv_can_store_new_dirty_bitmap(BlockDriverState *bs, const char *name, - uint32_t granularity, Error **errp) -{ - BlockDriver *drv = bs->drv; - - if (!drv) { - error_setg_errno(errp, ENOMEDIUM, - "Can't store persistent bitmaps to %s", - bdrv_get_device_or_node_name(bs)); - return false; - } - - if (!drv->bdrv_can_store_new_dirty_bitmap) { - error_setg_errno(errp, ENOTSUP, - "Can't store persistent bitmaps to %s", - bdrv_get_device_or_node_name(bs)); - return false; - } - - return drv->bdrv_can_store_new_dirty_bitmap(bs, name, granularity, errp); -} diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c index 49646a30e6..615f8480b2 100644 --- a/block/dirty-bitmap.c +++ b/block/dirty-bitmap.c @@ -466,6 +466,44 @@ void bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs, } } +int bdrv_add_persistent_dirty_bitmap(BlockDriverState *bs, + BdrvDirtyBitmap *bitmap, + Error **errp) +{ + BlockDriver *drv = bs->drv; + int ret; + + if (bdrv_dirty_bitmap_get_persistence(bitmap)) { + error_setg(errp, "Bitmap '%s' is already persistent, " + "and cannot be re-added to node '%s'", + bdrv_dirty_bitmap_name(bitmap), + bdrv_get_device_or_node_name(bs)); + return -EINVAL; + } + + if (!drv) { + error_setg_errno(errp, ENOMEDIUM, + "Can't store persistent bitmaps to %s", + bdrv_get_device_or_node_name(bs)); + return -ENOMEDIUM; + } + + if (!drv->bdrv_add_persistent_dirty_bitmap) { + error_setg_errno(errp, ENOTSUP, + "Can't store persistent bitmaps to %s", + bdrv_get_device_or_node_name(bs)); + return -ENOTSUP; + } + + ret = drv->bdrv_add_persistent_dirty_bitmap(bs, bitmap, errp); + if (ret == 0) { + bdrv_dirty_bitmap_set_persistence(bitmap, true); + } + + return ret; +} + + void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap) { bdrv_dirty_bitmap_lock(bitmap); diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c index 83aee1a42a..c3a72ca782 100644 --- a/block/qcow2-bitmap.c +++ b/block/qcow2-bitmap.c @@ -1607,14 +1607,16 @@ int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp) return 0; } -bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs, - const char *name, - uint32_t granularity, +int qcow2_add_persistent_dirty_bitmap(BlockDriverState *bs, + BdrvDirtyBitmap *bitmap, Error **errp) { BDRVQcow2State *s = bs->opaque; bool found; Qcow2BitmapList *bm_list; + const char *name = bdrv_dirty_bitmap_name(bitmap); + uint32_t granularity = bdrv_dirty_bitmap_granularity(bitmap); + int ret = 0; if (s->qcow_version < 3) { /* Without autoclear_features, we would always have to assume @@ -1623,20 +1625,23 @@ bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs, * have to drop all dirty bitmaps (defeating their purpose). */ error_setg(errp, "Cannot store dirty bitmaps in qcow2 v2 files"); + ret = -ENOTSUP; goto fail; } - if (check_constraints_on_bitmap(bs, name, granularity, errp) != 0) { + ret = check_constraints_on_bitmap(bs, name, granularity, errp); + if (ret) { goto fail; } if (s->nb_bitmaps == 0) { - return true; + return 0; } if (s->nb_bitmaps >= QCOW2_MAX_BITMAPS) { error_setg(errp, "Maximum number of persistent bitmaps is already reached"); + ret = -EOVERFLOW; goto fail; } @@ -1644,24 +1649,25 @@ bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs, QCOW2_MAX_BITMAP_DIRECTORY_SIZE) { error_setg(errp, "Not enough space in the bitmap directory"); + ret = -EOVERFLOW; goto fail; } - if (bitmap_list_load(bs, &bm_list, errp)) { + if ((ret = bitmap_list_load(bs, &bm_list, errp))) { goto fail; } found = find_bitmap_by_name(bm_list, name); bitmap_list_free(bm_list); if (found) { + ret = -EEXIST; error_setg(errp, "Bitmap with the same name is already stored"); goto fail; } - return true; - + return 0; fail: error_prepend(errp, "Can't make bitmap '%s' persistent in '%s': ", name, bdrv_get_device_or_node_name(bs)); - return false; + return ret; } diff --git a/block/qcow2.c b/block/qcow2.c index 9396d490d5..1c78eba71b 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -5229,7 +5229,7 @@ BlockDriver bdrv_qcow2 = { .bdrv_attach_aio_context = qcow2_attach_aio_context, .bdrv_reopen_bitmaps_rw = qcow2_reopen_bitmaps_rw, - .bdrv_can_store_new_dirty_bitmap = qcow2_can_store_new_dirty_bitmap, + .bdrv_add_persistent_dirty_bitmap = qcow2_add_persistent_dirty_bitmap, .bdrv_remove_persistent_dirty_bitmap = qcow2_remove_persistent_dirty_bitmap, }; diff --git a/blockdev.c b/blockdev.c index 3f44b891eb..169a8da831 100644 --- a/blockdev.c +++ b/blockdev.c @@ -2851,26 +2851,21 @@ void qmp_block_dirty_bitmap_add(const char *node, const char *name, disabled = false; } - if (persistent) { - aio_context = bdrv_get_aio_context(bs); - aio_context_acquire(aio_context); - if (!bdrv_can_store_new_dirty_bitmap(bs, name, granularity, errp)) { - goto out; - } - } - bitmap = bdrv_create_dirty_bitmap(bs, granularity, name, errp); if (bitmap == NULL) { - goto out; + return; } if (disabled) { bdrv_disable_dirty_bitmap(bitmap); } - bdrv_dirty_bitmap_set_persistence(bitmap, persistent); - out: - if (aio_context) { + if (persistent) { + aio_context = bdrv_get_aio_context(bs); + aio_context_acquire(aio_context); + if (bdrv_add_persistent_dirty_bitmap(bs, bitmap, errp)) { + bdrv_release_dirty_bitmap(bs, bitmap); + } aio_context_release(aio_context); } }