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

[Eric Blake]

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