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

[Peter Krempa]

On Fri, Jun 05, 2020 at 11:01:23 +0200, Kevin Wolf wrote:
> Am 04.06.2020 um 18:22 hat Peter Krempa geschrieben:
> > On Thu, Jun 04, 2020 at 13:31:45 +0200, Kevin Wolf wrote:
> > > Am 04.06.2020 um 11:16 hat Peter Krempa geschrieben:
> > > > On Thu, Jun 04, 2020 at 11:12:31 +0200, Kevin Wolf wrote:
> > > > > Am 18.05.2020 um 22:49 hat Eric Blake geschrieben:
> > > > > > > +
> > > > > > > +    /* 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;
> > > > > > > +    }
> > > > > > 
> > > > > > This means if the guest writes to the disk while the job is 
> > > > > > ongoing, the
> > > > > > bitmap will be updated to mark that portion of the bitmap as set, 
> > > > > > even if it
> > > > > > was not allocated at the time the job started.  But then again, the 
> > > > > > guest
> > > > > > writes are causing allocation, so this seems like the right thing 
> > > > > > to do.
> > > > > 
> > > > > Is the target bitmap active at the same time, i.e. will it get the
> > > > > correct information only from new_bitmap or are the bits already set 
> > > > > in
> > > > > it anyway?
> > > > 
> > > > Yes, libvirt plans to use it with an active non-persistent bitmap which
> > > > will in subsequent steps be merged into others. The bitmap is added in
> > > > the same transaction. The bitmap must be active, because we need to wait
> > > > for the block jobs to finish before it becomes usable and thus can't
> > > > sequence in other operations until later.
> > > 
> > > A lot of bitmap merging then, because the block job in this series
> > > already creates a temporary internal bitmap that is merged into the
> > > target bitmap on completion. But if the target bitmap is only libvirt's
> > > temporary bitmap to be merged to yet another bitmap, I wonder if this
> > > process shouldn't be simplified.
> > 
> > Possibly yes, but I'll leave that for later. All of this is done when
> > executin very expensive operations anyways so for our first
> > implementation it IMO won't matter that much.
> 
> I'm not necessarily saying that the change is needed on the libvirt
> side. It could also be that the block job should directly work with the
> given bitmap instead of having its internal temporary bitmap. Changing
> this later would mean changing the semantics of the block job, so it
> would be somewhat problematic.
> 
> It would be good to have a clear picture of what we want the final
> result to look like.

Well with current semantics of the 'nodename' argument controling both
where the populated bitmap is located and also which node's allocation
bitmap to take I don't think we can optimize it further in libvirt.

Current usage scenario is that we use a temporary bitmap populated with
the job to merge with bitmaps present in nodes which are removed by
blockjobs into the destination node of the block job. This means that
the real destination of the bits populated is in a different node than
it was originally and the above job semantics don't allow that.

Either way I'd strongly prefer to be able to kick off all the populate
jobs at once rather than having to sequence them so any semantic change
towards making it possible to target bitmaps in a different node would
also require that multiple jobs can run in parallel with a single bitmap
as destination. I'm not sure if that doesn't overcomplicate things
though.