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
