Re: [PATCH RFC v2 1/5] block: add bitmap-populate job

[Kevin Wolf]

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.
> 
> 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?

>  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(
> +    const char *job_id,
> +    BlockDriverState *bs,
> +    BdrvDirtyBitmap *target_bitmap,
> +    BitmapPattern pattern,
> +    BlockdevOnError on_error,
> +    int creation_flags,
> +    BlockCompletionFunc *cb,
> +    void *opaque,
> +    JobTxn *txn,
> +    Error **errp)
> +{
> +    int64_t len;
> +    BitpopBlockJob *job = NULL;
> +    int64_t cluster_size;
> +    BdrvDirtyBitmap *new_bitmap = NULL;
> +
> +    assert(bs);
> +    assert(target_bitmap);
> +
> +    if (!bdrv_is_inserted(bs)) {
> +        error_setg(errp, "Device is not inserted: %s",
> +                   bdrv_get_device_name(bs));
> +        return NULL;
> +    }
> +
> +    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?

> +    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.

> +        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