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

[Kevin Wolf]

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.

Kevin