Re: [Qemu-devel] [PATCH v12 07/17] qmp: Add support of "dirty-bitmap" sync mode for drive-backup

[Max Reitz]

On 2015-02-11 at 13:31, John Snow wrote:
> On 02/11/2015 01:18 PM, Max Reitz wrote:
> > On 2015-02-11 at 12:54, John Snow wrote:
> > > On 02/11/2015 12:47 PM, Max Reitz wrote:
> > > > Looks good to me in general, now I need to find out what the successor
> > > > bitmap is used for; but I guess I'll find that out by reviewing the 
> > > > rest
> > > > of this series.
> > > >
> > > > Max
> > >
> > > They don't really come up again, actually.
> > >
> > > The basic idea is this: While the backup is going on, reads and writes
> > > may occur (albeit delayed) and we want to track those writes in a
> > > separate bitmap for the duration of the backup operation.
> >
> > Yes, I thought as much; but where are writes to the named bitmap being
> > redirected to its successor? bdrv_set_dirty() doesn't do that, as far as
> > I can see.
>
> bdrv_dirty_bitmap_create_successor calls bdrv_create_dirty_bitmap, 
> which installs it in the bitmap chain attached to a BDS:
>
>     QLIST_INSERT_HEAD(&bs->dirty_bitmaps, bitmap, list);
>
> which is read by bdrv_set_dirty.

Oooh, clever. Right.

> bdrv_set_dirty operates on all bitmaps attached to a BDS, while 
> bdrv_set_dirty_bitmap operates on a single specific instance.
>
> > > If the backup operation fails, we use the dirty sector tracking info
> > > in the successor to know what has changed since we started the backup,
> > > and we merge this bitmap back into the originating bitmap; then if an
> > > incremental backup is tried again, it includes all of the original
> > > data plus any data changed while we failed to do a backup.
> > >
> > > If the backup operation succeeds, the originating bitmap is deleted
> > > and the successor is installed in its place.
> > >
> > > It's a namespace trick: by having an anonymous bitmap as a child of
> > > the "real" bitmap, the real bitmap can be frozen and prohibited from
> > > being moved, renamed, deleted, etc. This prevents the user from adding
> > > a new bitmap with the same name or similar while the backup is in
> > > progress.
> >
> > Hm, if it's just for that, wouldn't disabling the bitmap suffice?
> >
> > Max
>
> Kind of? We still want to track writes while it's disabled.

Right, I was assuming here that writes are not tracked in the successor.

Thanks for pointing out that they are!

Max

> If we try to use a single bitmap, we have no real way to know which 
> bits to clear after the operation succeeds. I think two bitmaps is a 
> requirement to accommodate both failure and success cases.
>
> A distinction is made between a disabled bitmap (which is just 
> read-only: it can be deleted) and a frozen bitmap (which is in-use by 
> an operation, implicitly disabled, and cannot be enabled, disabled, 
> deleted, cleared, set or reset.)
>
> > > A previous approach was to immediately take the bitmap off of the BDS,
> > > but in the error case here, the logic becomes more complicated when we
> > > need to re-install the bitmap but the user has already installed a new
> > > bitmap with the same name, etc.
> > >
> > > So the general lifetime is this:
> > >
> > > (1) A backup is started. the block/backup routine calls 
> > > create_successor.
> > > (2) If the backup fails to start, the block/backup routine will call
> > > the "reclaim" method, which will merge the (empty) successor back into
> > > the original bitmap, unfreezing it.
> > > (3) If the backup starts, and then fails, the bitmap is "reclaim"ed
> > > (merged back into one bitmap.)
> > > (4) If the backup succeeds, the bitmap "abdicates" to the successor.
> > > (The parent bitmap is erased and the successor is installed in its
> > > place.)
> >
> > Yes, see the graph at the whiteboard behind me. :-)
> >
> > Max