[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH RFC v2 1/5] block: add bitmap-populate job
From: |
Peter Krempa |
Subject: |
Re: [PATCH RFC v2 1/5] block: add bitmap-populate job |
Date: |
Fri, 5 Jun 2020 11:58:10 +0200 |
User-agent: |
Mutt/1.13.4 (2020-02-15) |
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.
- Re: [PATCH RFC v2 1/5] block: add bitmap-populate job, (continued)
- Re: [PATCH RFC v2 1/5] block: add bitmap-populate job, Kevin Wolf, 2020/06/04
- Re: [PATCH RFC v2 1/5] block: add bitmap-populate job, Peter Krempa, 2020/06/04
- Re: [PATCH RFC v2 1/5] block: add bitmap-populate job, Kevin Wolf, 2020/06/04
- Re: [PATCH RFC v2 1/5] block: add bitmap-populate job, Peter Krempa, 2020/06/04
- Re: [PATCH RFC v2 1/5] block: add bitmap-populate job, Kevin Wolf, 2020/06/05
- Re: [PATCH RFC v2 1/5] block: add bitmap-populate job, Peter Krempa, 2020/06/05
- Re: [PATCH RFC v2 1/5] block: add bitmap-populate job, Kevin Wolf, 2020/06/05
- Re: [PATCH RFC v2 1/5] block: add bitmap-populate job,
Peter Krempa <=
- Re: [PATCH RFC v2 1/5] block: add bitmap-populate job, Kevin Wolf, 2020/06/05
- Re: [PATCH RFC v2 1/5] block: add bitmap-populate job, Peter Krempa, 2020/06/05
- Re: [PATCH RFC v2 1/5] block: add bitmap-populate job, Vladimir Sementsov-Ogievskiy, 2020/06/06
- Re: [PATCH RFC v2 1/5] block: add bitmap-populate job, Kevin Wolf, 2020/06/08
- Re: [PATCH RFC v2 1/5] block: add bitmap-populate job, Vladimir Sementsov-Ogievskiy, 2020/06/08
- Re: [PATCH RFC v2 1/5] block: add bitmap-populate job, Kevin Wolf, 2020/06/08
- Re: [PATCH RFC v2 1/5] block: add bitmap-populate job, Peter Krempa, 2020/06/08
- Re: [PATCH RFC v2 1/5] block: add bitmap-populate job, Vladimir Sementsov-Ogievskiy, 2020/06/08
- Re: [PATCH RFC v2 1/5] block: add bitmap-populate job, Peter Krempa, 2020/06/08