[RFC PATCH 36/41] qemu: Rewrite bitmap handling for block commit

Reuse qemuBlockGetBitmapMergeActions which allows to remove the ad-hoc implementatio of bitmap merging for block commit. The new approach is way simpler and more robust and also allows us to get rid of the disabling of bitmaps done prior to the start as we actually do want to update the bitmaps in the base. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/qemu/qemu_block.c | 203 +----------------- src/qemu/qemu_block.h | 10 +- src/qemu/qemu_blockjob.c | 24 ++- src/qemu/qemu_driver.c | 56 +---- tests/qemublocktest.c | 21 +- .../qemublocktestdata/bitmapblockcommit/empty | 1 - 6 files changed, 47 insertions(+), 268 deletions(-) diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c index 6eab9cb4e2..fd3fe4c354 100644 --- a/src/qemu/qemu_block.c +++ b/src/qemu/qemu_block.c @@ -3176,117 +3176,7 @@ qemuBlockBitmapsHandleBlockcopy(virStorageSourcePtr src, /** * @topsrc: virStorageSource representing 'top' of the job * @basesrc: virStorageSource representing 'base' of the job - * @blockNamedNodeData: hash table containing data about bitmaps - * @actions: filled with arguments for a 'transaction' command - * @disabledBitmapsBase: filled with a list of bitmap names which must be disabled - * - * Prepares data for correctly handling bitmaps during the start of a commit - * job. The bitmaps in the 'base' image must be disabled, so that the writes - * done by the blockjob don't dirty the enabled bitmaps. - * - * @actions and @disabledBitmapsBase are untouched if no bitmaps need - * to be disabled. - */ -int -qemuBlockBitmapsHandleCommitStart(virStorageSourcePtr topsrc, - virStorageSourcePtr basesrc, - virHashTablePtr blockNamedNodeData, - virJSONValuePtr *actions, - char ***disabledBitmapsBase) -{ - g_autoptr(virJSONValue) act = virJSONValueNewArray(); - VIR_AUTOSTRINGLIST bitmaplist = NULL; - size_t curbitmapstr = 0; - qemuBlockNamedNodeDataPtr entry; - bool disable_bitmaps = false; - size_t i; - - if (!(entry = virHashLookup(blockNamedNodeData, basesrc->nodeformat))) - return 0; - - bitmaplist = g_new0(char *, entry->nbitmaps + 1); - - for (i = 0; i < entry->nbitmaps; i++) { - qemuBlockNamedNodeDataBitmapPtr bitmap = entry->bitmaps[i]; - - if (!bitmap->recording || bitmap->inconsistent || - !qemuBlockBitmapChainIsValid(topsrc, bitmap->name, blockNamedNodeData)) - continue; - - disable_bitmaps = true; - - if (qemuMonitorTransactionBitmapDisable(act, basesrc->nodeformat, - bitmap->name) < 0) - return -1; - - bitmaplist[curbitmapstr++] = g_strdup(bitmap->name); - } - - if (disable_bitmaps) { - *actions = g_steal_pointer(&act); - *disabledBitmapsBase = g_steal_pointer(&bitmaplist); - } - - return 0; -} - - -struct qemuBlockBitmapsHandleCommitData { - bool skip; - bool create; - bool enable; - const char *basenode; - virJSONValuePtr merge; - unsigned long long granularity; - bool persistent; -}; - - -static void -qemuBlockBitmapsHandleCommitDataFree(void *opaque) -{ - struct qemuBlockBitmapsHandleCommitData *data = opaque; - - virJSONValueFree(data->merge); - g_free(data); -} - - -static int -qemuBlockBitmapsHandleCommitFinishIterate(void *payload, - const void *entryname, - void *opaque) -{ - struct qemuBlockBitmapsHandleCommitData *data = payload; - const char *bitmapname = entryname; - virJSONValuePtr actions = opaque; - - if (data->skip) - return 0; - - if (data->create) { - if (qemuMonitorTransactionBitmapAdd(actions, data->basenode, bitmapname, - data->persistent, !data->enable, - data->granularity) < 0) - return -1; - } else { - if (data->enable && - qemuMonitorTransactionBitmapEnable(actions, data->basenode, bitmapname) < 0) - return -1; - } - - if (data->merge && - qemuMonitorTransactionBitmapMerge(actions, data->basenode, bitmapname, - &data->merge) < 0) - return -1; - - return 0; -} - - -/** - * @topsrc: virStorageSource representing 'top' of the job - * @basesrc: virStorageSource representing 'base' of the job + * @active: commit job is an active layer block-commit * @blockNamedNodeData: hash table containing data about bitmaps * @actions: filled with arguments for a 'transaction' command * @disabledBitmapsBase: bitmap names which were disabled @@ -3299,95 +3189,22 @@ qemuBlockBitmapsHandleCommitFinishIterate(void *payload, int qemuBlockBitmapsHandleCommitFinish(virStorageSourcePtr topsrc, virStorageSourcePtr basesrc, + bool active, virHashTablePtr blockNamedNodeData, virJSONValuePtr *actions, - char **disabledBitmapsBase) + GSList **allocationbitmapnodes) { - g_autoptr(virJSONValue) act = virJSONValueNewArray(); - virStorageSourcePtr n; - qemuBlockNamedNodeDataPtr entry; - g_autoptr(virHashTable) commitdata = NULL; - struct qemuBlockBitmapsHandleCommitData *bitmapdata; - size_t i; - - commitdata = virHashNew(qemuBlockBitmapsHandleCommitDataFree); - - for (n = topsrc; n != basesrc; n = n->backingStore) { - if (!(entry = virHashLookup(blockNamedNodeData, n->nodeformat))) - continue; - - for (i = 0; i < entry->nbitmaps; i++) { - qemuBlockNamedNodeDataBitmapPtr bitmap = entry->bitmaps[i]; - - if (!(bitmapdata = virHashLookup(commitdata, bitmap->name))) { - bitmapdata = g_new0(struct qemuBlockBitmapsHandleCommitData, 1); + virStorageSourcePtr writebitmapsrc = NULL; - /* we must mirror the state of the topmost bitmap and merge - * everything else */ - bitmapdata->create = true; - bitmapdata->enable = bitmap->recording; - bitmapdata->basenode = basesrc->nodeformat; - bitmapdata->merge = virJSONValueNewArray(); - bitmapdata->granularity = bitmap->granularity; - bitmapdata->persistent = bitmap->persistent; + if (active) + writebitmapsrc = basesrc; - if (virHashAddEntry(commitdata, bitmap->name, bitmapdata) < 0) { - qemuBlockBitmapsHandleCommitDataFree(bitmapdata); - return -1; - } - } - - if (bitmap->inconsistent || - !qemuBlockBitmapChainIsValid(topsrc, bitmap->name, blockNamedNodeData)) - bitmapdata->skip = true; - - if (qemuMonitorTransactionBitmapMergeSourceAddBitmap(bitmapdata->merge, - n->nodeformat, - bitmap->name) < 0) - return -1; - } - } - - if ((entry = virHashLookup(blockNamedNodeData, basesrc->nodeformat))) { - /* note that all bitmaps in 'base' were disabled when commit was started */ - for (i = 0; i < entry->nbitmaps; i++) { - qemuBlockNamedNodeDataBitmapPtr bitmap = entry->bitmaps[i]; - - if ((bitmapdata = virHashLookup(commitdata, bitmap->name))) { - bitmapdata->create = false; - } else { - if (disabledBitmapsBase) { - char **disabledbitmaps; - - for (disabledbitmaps = disabledBitmapsBase; *disabledbitmaps; disabledbitmaps++) { - if (STREQ(*disabledbitmaps, bitmap->name)) { - bitmapdata = g_new0(struct qemuBlockBitmapsHandleCommitData, 1); - - bitmapdata->create = false; - bitmapdata->enable = true; - bitmapdata->basenode = basesrc->nodeformat; - bitmapdata->granularity = bitmap->granularity; - bitmapdata->persistent = bitmap->persistent; - - if (virHashAddEntry(commitdata, bitmap->name, bitmapdata) < 0) { - qemuBlockBitmapsHandleCommitDataFree(bitmapdata); - return -1; - } - - break; - } - } - } - } - } - } - - if (virHashForEach(commitdata, qemuBlockBitmapsHandleCommitFinishIterate, act) < 0) + if (qemuBlockGetBitmapMergeActions(topsrc, basesrc, basesrc, NULL, NULL, + writebitmapsrc, actions, + allocationbitmapnodes, + blockNamedNodeData) < 0) return -1; - if (virJSONValueArraySize(act) > 0) - *actions = g_steal_pointer(&act); - return 0; } diff --git a/src/qemu/qemu_block.h b/src/qemu/qemu_block.h index 6bec1e2e29..5d98f7ce6b 100644 --- a/src/qemu/qemu_block.h +++ b/src/qemu/qemu_block.h @@ -244,19 +244,13 @@ qemuBlockBitmapsHandleBlockcopy(virStorageSourcePtr src, bool shallow, virJSONValuePtr *actions); -int -qemuBlockBitmapsHandleCommitStart(virStorageSourcePtr topsrc, - virStorageSourcePtr basesrc, - virHashTablePtr blockNamedNodeData, - virJSONValuePtr *actions, - char ***disabledBitmapsBase); - int qemuBlockBitmapsHandleCommitFinish(virStorageSourcePtr topsrc, virStorageSourcePtr basesrc, + bool active, virHashTablePtr blockNamedNodeData, virJSONValuePtr *actions, - char **disabledBitmapsBase); + GSList **r_tmpbitmapnodes); int qemuBlockReopenReadWrite(virDomainObjPtr vm, diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c index b19d96b312..ef22bc60ab 100644 --- a/src/qemu/qemu_blockjob.c +++ b/src/qemu/qemu_blockjob.c @@ -1102,6 +1102,8 @@ qemuBlockJobProcessEventCompletedCommitBitmaps(virDomainObjPtr vm, qemuDomainObjPrivatePtr priv = vm->privateData; g_autoptr(virHashTable) blockNamedNodeData = NULL; g_autoptr(virJSONValue) actions = NULL; + g_autoptr(GSList) allocationbitmapnodes = NULL; + bool active = job->type == QEMU_BLOCKJOB_TYPE_ACTIVE_COMMIT; if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKDEV_REOPEN)) return 0; @@ -1111,15 +1113,22 @@ qemuBlockJobProcessEventCompletedCommitBitmaps(virDomainObjPtr vm, if (qemuBlockBitmapsHandleCommitFinish(job->data.commit.top, job->data.commit.base, + active, blockNamedNodeData, &actions, - job->data.commit.disabledBitmapsBase) < 0) + &allocationbitmapnodes) < 0) return 0; if (!actions) return 0; - if (qemuBlockReopenReadWrite(vm, job->data.commit.base, asyncJob) < 0) + if (!active) { + if (qemuBlockReopenReadWrite(vm, job->data.commit.base, asyncJob) < 0) + return -1; + } + + if (qemuBlockBitmapTemporaryAdd(vm, blockNamedNodeData, + &allocationbitmapnodes, asyncJob) < 0) return -1; if (qemuDomainObjEnterMonitorAsync(priv->driver, vm, asyncJob) < 0) @@ -1130,8 +1139,12 @@ qemuBlockJobProcessEventCompletedCommitBitmaps(virDomainObjPtr vm, if (qemuDomainObjExitMonitor(priv->driver, vm) < 0) return -1; - if (qemuBlockReopenReadOnly(vm, job->data.commit.base, asyncJob) < 0) - return -1; + qemuBlockBitmapTemporaryRemove(vm, allocationbitmapnodes, asyncJob); + + if (!active) { + if (qemuBlockReopenReadOnly(vm, job->data.commit.base, asyncJob) < 0) + return -1; + } return 0; } @@ -1301,6 +1314,9 @@ qemuBlockJobProcessEventCompletedActiveCommit(virQEMUDriverPtr driver, job->disk->src = job->data.commit.base; job->disk->src->readonly = job->data.commit.top->readonly; + if (qemuBlockJobProcessEventCompletedCommitBitmaps(vm, job, asyncJob) < 0) + return; + qemuBlockJobEventProcessConcludedRemoveChain(driver, vm, asyncJob, job->data.commit.top); if (job->data.commit.deleteCommittedImages) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index febe417faf..98a60ec2e2 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17272,9 +17272,9 @@ qemuDomainBlockPivot(virQEMUDriverPtr driver, case QEMU_BLOCKJOB_TYPE_COPY: if (blockdev && !job->jobflagsmissing) { - g_autoptr(virHashTable) blockNamedNodeData = NULL; bool shallow = job->jobflags & VIR_DOMAIN_BLOCK_COPY_SHALLOW; bool reuse = job->jobflags & VIR_DOMAIN_BLOCK_COPY_REUSE_EXT; + g_autoptr(virHashTable) blockNamedNodeData = NULL; if (!(blockNamedNodeData = qemuBlockGetNamedNodeData(vm, QEMU_ASYNC_JOB_NONE))) return -1; @@ -17312,16 +17312,15 @@ qemuDomainBlockPivot(virQEMUDriverPtr driver, * the bitmaps if it wasn't present thus must skip this */ if (blockdev && virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKDEV_REOPEN)) { - g_autoptr(virHashTable) blockNamedNodeData = NULL; - if (!(blockNamedNodeData = qemuBlockGetNamedNodeData(vm, QEMU_ASYNC_JOB_NONE))) - return -1; + actions = virJSONValueNewArray(); - if (qemuBlockBitmapsHandleCommitFinish(job->data.commit.top, - job->data.commit.base, - blockNamedNodeData, - &actions, - job->data.commit.disabledBitmapsBase) < 0) + if (qemuMonitorTransactionBitmapAdd(actions, + job->data.commit.base->nodeformat, + "libvirt-tmp-activewrite", + false, + false, + 0) < 0) return -1; } @@ -18442,7 +18441,6 @@ qemuDomainBlockCommit(virDomainPtr dom, const char *nodebase = NULL; bool persistjob = false; bool blockdev = false; - g_autoptr(virJSONValue) bitmapDisableActions = NULL; VIR_AUTOSTRINGLIST bitmapDisableList = NULL; virCheckFlags(VIR_DOMAIN_BLOCK_COMMIT_SHALLOW | @@ -18607,27 +18605,6 @@ qemuDomainBlockCommit(virDomainPtr dom, goto endjob; } - if (blockdev && - virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKDEV_REOPEN)) { - g_autoptr(virHashTable) blockNamedNodeData = NULL; - if (!(blockNamedNodeData = qemuBlockGetNamedNodeData(vm, QEMU_ASYNC_JOB_NONE))) - goto endjob; - - if (qemuBlockBitmapsHandleCommitStart(topSource, baseSource, - blockNamedNodeData, - &bitmapDisableActions, - &bitmapDisableList) < 0) - goto endjob; - - /* if we don't have terminator on 'base' we can't reopen it */ - if (bitmapDisableActions && !baseSource->backingStore) { - virReportError(VIR_ERR_OPERATION_UNSUPPORTED, - _("can't handle bitmaps on unterminated backing image '%s'"), - base); - goto endjob; - } - } - if (!(job = qemuBlockJobDiskNewCommit(vm, disk, top_parent, topSource, baseSource, &bitmapDisableList, flags & VIR_DOMAIN_BLOCK_COMMIT_DELETE, @@ -18652,23 +18629,6 @@ qemuDomainBlockCommit(virDomainPtr dom, !(backingPath = qemuBlockGetBackingStoreString(baseSource, false))) goto endjob; - if (bitmapDisableActions) { - int rc; - - if (qemuBlockReopenReadWrite(vm, baseSource, QEMU_ASYNC_JOB_NONE) < 0) - goto endjob; - - qemuDomainObjEnterMonitor(driver, vm); - rc = qemuMonitorTransaction(priv->mon, &bitmapDisableActions); - if (qemuDomainObjExitMonitor(driver, vm) < 0) - goto endjob; - - if (qemuBlockReopenReadOnly(vm, baseSource, QEMU_ASYNC_JOB_NONE) < 0) - goto endjob; - - if (rc < 0) - goto endjob; - } } else { device = job->name; } diff --git a/tests/qemublocktest.c b/tests/qemublocktest.c index a04719b2df..5624c96d5f 100644 --- a/tests/qemublocktest.c +++ b/tests/qemublocktest.c @@ -932,12 +932,12 @@ testQemuBlockBitmapBlockcommit(const void *opaque) g_autofree char *actual = NULL; g_autofree char *expectpath = NULL; - g_autoptr(virJSONValue) actionsDisable = NULL; g_autoptr(virJSONValue) actionsMerge = NULL; g_autoptr(virJSONValue) nodedatajson = NULL; g_autoptr(virHashTable) nodedata = NULL; g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER; - VIR_AUTOSTRINGLIST bitmapsDisable = NULL; + g_autoptr(GSList) allocationbitmapnodes = NULL; + bool active = data->top == data->chain; expectpath = g_strdup_printf("%s/%s%s", abs_srcdir, blockcommitPrefix, data->name); @@ -951,26 +951,18 @@ testQemuBlockBitmapBlockcommit(const void *opaque) return -1; } - if (qemuBlockBitmapsHandleCommitStart(data->top, data->base, nodedata, - &actionsDisable, &bitmapsDisable) < 0) - return -1; - - virBufferAddLit(&buf, "pre job bitmap disable:\n"); - - if (actionsDisable && - virJSONValueToBuffer(actionsDisable, &buf, true) < 0) - return -1; - virBufferAddLit(&buf, "merge bitmpas:\n"); - if (qemuBlockBitmapsHandleCommitFinish(data->top, data->base, nodedata, - &actionsMerge, bitmapsDisable) < 0) + if (qemuBlockBitmapsHandleCommitFinish(data->top, data->base, active, nodedata, + &actionsMerge, &allocationbitmapnodes) < 0) return -1; if (actionsMerge && virJSONValueToBuffer(actionsMerge, &buf, true) < 0) return -1; + testQemuBitmapListPrint("allocation bitmap:", allocationbitmapnodes, &buf); + actual = virBufferContentAndReset(&buf); return virTestCompareToFile(actual, expectpath); @@ -1361,6 +1353,7 @@ mymain(void) #define TEST_BITMAP_BLOCKCOMMIT(testname, topimg, baseimg, ndf) \ do {\ blockbitmapblockcommitdata.name = testname; \ + blockbitmapblockcommitdata.chain = bitmapSourceChain; \ blockbitmapblockcommitdata.top = testQemuBitmapGetFakeChainEntry(bitmapSourceChain, topimg); \ blockbitmapblockcommitdata.base = testQemuBitmapGetFakeChainEntry(bitmapSourceChain, baseimg); \ blockbitmapblockcommitdata.nodedatafile = ndf; \ diff --git a/tests/qemublocktestdata/bitmapblockcommit/empty b/tests/qemublocktestdata/bitmapblockcommit/empty index bfc58f994e..9260011852 100644 --- a/tests/qemublocktestdata/bitmapblockcommit/empty +++ b/tests/qemublocktestdata/bitmapblockcommit/empty @@ -1,2 +1 @@ -pre job bitmap disable: merge bitmpas: -- 2.26.2