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

[Kevin Wolf]

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

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

Kevin