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

[Peter Krempa]

On Fri, Jun 05, 2020 at 11:44:07 +0200, Kevin Wolf wrote:
> Am 05.06.2020 um 11:24 hat Peter Krempa geschrieben:
> > 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.
> 
> So does this mean that a better API wouldn't only take a node-name and
> bitmap name (where the node identified by node-name is not only where
> the target bitmap is, but also the node whose allocation status is
> queried), but that it should take two different node-names for source
> (= reading allocation status) and target (= owner of the bitmap)?

Yes. That way one of the merges would be merged (heh) into the operation
itself preventing us from the need to have an extra temporary bitmap.

> > 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.
> 
> Other people are more familiar with the dirty bitmap code, so I may be
> wrong, but intuitively, I don't see any problem with multiple jobs
> dirtying blocks in the same bitmap. Or, with the internal temporary
> bitmap as used in this version of the series, multiple jobs that, one
> after another, merge their result to the same bitmap on completion.

I don't see a problem with the bitmaps itself since we are only OR-ing
results together. I just wanted to state the desired usage.

The above was actually inspired by a very recent problem I have in my
attempt to use the dirty bitmap populate job to refactor how libvirt
handles bitmaps. I've just figured out that I need to shuffle around
some stuff as I can't run the dirty-bitmap-populate job while an active
layer commit is in synchronised phase and I wanted to do the merging at
that point. That reminded me of a possible gotcha in having to sequence
the blockjobs which certainly would be more painful.