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

[Kevin Wolf <kwolf@redhat.com>]

Am 05.06.2020 um 11:58 hat Peter Krempa geschrieben:
> 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.

It would probably be good to have not only an iotests case that tests
the low-level functionality of the block job, but also one that
resembles the way libvirt would actually use it in combination with
other jobs.

Kevin