|
From: | Eric Blake |
Subject: | Re: [PATCH RFC v2 1/5] block: add bitmap-populate job |
Date: | Tue, 16 Jun 2020 14:46:40 -0500 |
User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.9.0 |
On 6/4/20 4:01 AM, Kevin Wolf wrote:
Am 14.05.2020 um 05:49 hat John Snow geschrieben:This job copies the allocation map into a bitmap. It's a job because there's no guarantee that allocation interrogation will be quick (or won't hang), so it cannot be retrofit into block-dirty-bitmap-merge. It was designed with different possible population patterns in mind, but only top layer allocation was implemented for now.
Other patterns that might make sense someday: all-ones (we already have bitmap-clear for all zeroes) an inverse flag (set bits for all unallocated portions) compressed (set bits for portions that are compressed) but yeah, I don't see it worth implementing any of them without a client.
Signed-off-by: John Snow <jsnow@redhat.com> --- qapi/block-core.json | 48 +++++++++ qapi/job.json | 2 +- include/block/block_int.h | 21 ++++ block/bitmap-alloc.c | 207 ++++++++++++++++++++++++++++++++++++++bitmap-populate.c to be more consistent with the actual job name?
Seems reasonable to me.
blockjob.c | 3 +- block/Makefile.objs | 1 + 6 files changed, 280 insertions(+), 2 deletions(-) create mode 100644 block/bitmap-alloc.c[...]+BlockJob *bitpop_job_create(
+ if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_BACKUP_SOURCE, errp)) { + return NULL; + }What does this protect? And why does BACKUP_SOURCE describe acccurately what this job does?
I'm less certain what the BLOCK_OP_TYPE_* constants are supposed to block, or if this is just copy/paste from backup.c. Does BlockOpType in block.h need a new entry?
+ if (bdrv_dirty_bitmap_check(target_bitmap, BDRV_BITMAP_DEFAULT, errp)) { + return NULL; + } + + if (pattern != BITMAP_PATTERN_ALLOCATION_TOP) { + error_setg(errp, "Unrecognized bitmap pattern"); + return NULL; + } + + len = bdrv_getlength(bs); + if (len < 0) { + error_setg_errno(errp, -len, "unable to get length for '%s'", + bdrv_get_device_name(bs));This operates on the node level, so bdrv_get_device_or_node_name() is necessary to avoid empty strings in the message.
Easy to fix.
+ return NULL; + } + + /* NB: new bitmap is anonymous and enabled */ + cluster_size = bdrv_dirty_bitmap_granularity(target_bitmap); + new_bitmap = bdrv_create_dirty_bitmap(bs, cluster_size, NULL, errp); + if (!new_bitmap) { + return NULL; + } + + /* Take ownership; we reserve the right to write into this on-commit. */ + bdrv_dirty_bitmap_set_busy(target_bitmap, true); + + job = block_job_create(job_id, &bitpop_job_driver, txn, bs, + BLK_PERM_CONSISTENT_READ,I don't think we actually rely on CONSISTENT_READ, but then, using the job on inconsistent nodes probably makes little sense and we can always relax the restriction later if necessary.+ BLK_PERM_ALL & ~BLK_PERM_RESIZE, + 0, creation_flags, + cb, opaque, errp); + if (!job) { + bdrv_dirty_bitmap_set_busy(target_bitmap, false); + bdrv_release_dirty_bitmap(new_bitmap); + return NULL; + } + + job->bs = bs; + job->on_error = on_error; + job->target_bitmap = target_bitmap; + job->new_bitmap = new_bitmap; + job->len = len; + job_progress_set_remaining(&job->common.job, job->len); + + return &job->common; +}Kevin
-- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
[Prev in Thread] | Current Thread | [Next in Thread] |