Re: [Qemu-devel] [PATCH 12/22] qcow2-bitmap: add IN_USE flag

[Vladimir Sementsov-Ogievskiy]

26.10.2016 15:13, Vladimir Sementsov-Ogievskiy wrote:
> 26.10.2016 12:21, Vladimir Sementsov-Ogievskiy wrote:
> > 26.10.2016 12:04, Vladimir Sementsov-Ogievskiy wrote:
> > > 25.10.2016 13:53, Vladimir Sementsov-Ogievskiy wrote:
> > > > 24.10.2016 20:18, Max Reitz wrote:
> > > > > On 24.10.2016 19:08, Max Reitz wrote:
> > > > > > On 24.10.2016 13:35, Vladimir Sementsov-Ogievskiy wrote:
> > > > > > > 24.10.2016 13:32, Vladimir Sementsov-Ogievskiy пишет:
> > > > > > > > 21.10.2016 22:58, Max Reitz пишет:
> > > > > > > > > On 21.10.2016 17:34, Vladimir Sementsov-Ogievskiy wrote:
> > > > > > > > > > 07.10.2016 22:44, Max Reitz пишет:
> > > > > > > > > > > On 30.09.2016 12:53, Vladimir Sementsov-Ogievskiy wrote:
> > > > > > > > > > > > This flag means that the bitmap is now in use by the 
> > > > > > > > > > > > software or
> > > > > > > > > > > > was not
> > > > > > > > > > > > successfully saved. In any way, with this flag set the 
> > > > > > > > > > > > bitmap data
> > > > > > > > > > > > must
> > > > > > > > > > > > be considered inconsistent and should not be loaded.
> > > > > > > > > > > >
> > > > > > > > > > > > With current implementation this flag is never set, as we 
> > > > > > > > > > > > just remove
> > > > > > > > > > > > bitmaps from the image after loading. But it defined in 
> > > > > > > > > > > > qcow2 spec
> > > > > > > > > > > > and
> > > > > > > > > > > > must be handled. Also, it can be used in future, if async 
> > > > > > > > > > > > schemes of
> > > > > > > > > > > > bitmap loading/saving are implemented.
> > > > > > > > > > > >
> > > > > > > > > > > > We also remove in-use bitmaps from the image on open.
> > > > > > > > > > > >
> > > > > > > > > > > > Signed-off-by: Vladimir Sementsov-Ogievskiy
> > > > > > > > > > > > <address@hidden>
> > > > > > > > > > > > ---
> > > > > > > > > > > >     block/qcow2-bitmap.c | 17 ++++++++++++++++-
> > > > > > > > > > > >     1 file changed, 16 insertions(+), 1 deletion(-)
> > > > > > > > > > > Don't you want to make use of this flag? It would appear 
> > > > > > > > > > > useful to
> > > > > > > > > > > me if
> > > > > > > > > > > you just marked autoload bitmaps as in_use instead of 
> > > > > > > > > > > deleting them
> > > > > > > > > > > from
> > > > > > > > > > > the image when it's opened and then overwrite them when the 
> > > > > > > > > > > image is
> > > > > > > > > > > closed.
> > > > > > > > > > And what is the use of it?
> > > > > > > > > You don't need to free any bitmaps when opening the file, and 
> > > > > > > > > you don't
> > > > > > > > > need to allocate any new bitmap space when closing it.
> > > > > > > > As bitmaps are sparce in file, I need to allocate new space when
> > > > > > > > closing. Or free it...
> > > > > > >
> > > > > > > Is it a real problem to reallocate space in qcow2? If so, to 
> > > > > > > minimaze
> > > > > > > allocations, I will have to make a list of clusters of in_use 
> > > > > > > bitmaps on
> > > > > > > close, and then save current bitmaps to these clusters (and 
> > > > > > > allocated
> > > > > > > some additional clusters, or free extra clusters).
> > > > > > It's not a real problem, but it does take time, and I maintain 
> > > > > > that it's
> > > > > > time it doesn't need to take because you can just use the in_use 
> > > > > > flag.
> > > > > >
> > > > > > I wouldn't worry about reusing clusters of other bitmaps. Of 
> > > > > > course we
> > > > > > can do that later on in some optimization but not now.
> > > > > >
> > > > > > I just mean the basic case of some auto-loaded bitmap that is not 
> > > > > > being
> > > > > > deleted while the VM is running and is just saved back to the 
> > > > > > image file
> > > > > > once it is closed. I don't expect that users will always consume 
> > > > > > all of
> > > > > > the auto-loaded bitmaps while the VM is active...
> > > > > >
> > > > > > > Also, if I don't free them on open, I'll have to free them on 
> > > > > > > remove of
> > > > > > > bdrv dirty bitmap..
> > > > > > Yes, so? That takes time, but that is something the user will 
> > > > > > probably
> > > > > > expect. I wouldn't expect opening of the file to take that time.
> > > > > >
> > > > > > Overall, it doesn't matter time-wise whether you free the bitmap 
> > > > > > data
> > > > > > when opening the image file or when the dirty bitmap is actually 
> > > > > > deleted
> > > > > > by the user or when the image file is closed. Just setting the 
> > > > > > single
> > > > > > in_use flag for all of the auto-loaded bitmaps will basically not 
> > > > > > take
> > > > > > any time.
> > > > > >
> > > > > > On the other hand, as soon as just a single auto-loaded bitmap 
> > > > > > survives
> > > > > > a VM (or qemu-img) invocation, you will very, very likely safe at 
> > > > > > least
> > > > > > some time because writing the bitmap to the disk can reuse at 
> > > > > > least some
> > > > > > of the existing clusters.
> > > > > By the way, dealing with removal of bitmaps when they are deleted 
> > > > > by the
> > > > > user is also positive when it comes to migration or read-only 
> > > > > disks that
> > > > > are later reopened R/W: Currently, as far as I can see, you just keep
> > > > > the auto-load bitmaps in the image if the image is part of an 
> > > > > incoming
> > > > > migration or opened read-only, but then you continue under the
> > > > > assumption that they are removed from the image. That's not very 
> > > > > good.
> > > >
> > > > You are right, I need to handle reopening more carefully. In my 
> > > > way, I need to delete bitmaps when reopening R/W and in yours - set 
> > > > in_use bit.
> > > >
> > > > > If you delete the bitmaps only when the user asks you to, then you 
> > > > > can
> > > > > just return an error that the bitmap cannot be removed at this time
> > > > > because the image file is read-only or used in migration (and I don't
> > > > > think the latter can even happen when it's the user who has requested
> > > > > removal of the bitmap).
> > > > >
> > > > > Max
> > > >
> > > > Ok, I'll rewrite this way, using 'in_use' bit and trying to avoid 
> > > > allocate/free overhead.
> > >
> > > Trying to reuse clusters of in_use bitmaps (including contiguous 
> > > allocations for bitmap tables) will complicate things a lot.. We can 
> > > use extra clusters of one in_use bitmap to save another one, the 
> > > same is for bitmap tables. Extra clusters of old bitmap table (in 
> > > case of resize) can be used for saving other bitmap data, etc..
> >
> > A compromise strategy of reusing:
> >
> > 1. accumulate all data clusters of in_use bitmaps, which was loaded 
> > by this qcow2 instance (we will not touch other old in_use bitmaps) 
> > to free_clusters list
> > 2. for each persistent BdrvDirtyBitmap:
> >        If there is corresponding in_use bitmap in the image, and its 
> > table size == needed table size (there was no resizes), then let's 
> > reuse bitmap table.
> >        else, move old bitmap table clusters to free_clusters and 
> > allocate new table.
> > 3. for each persistent BdrvDirtyBitmap:
> >        For bitmap data clusters, take them from free_clusters list, 
> > and if it is empty - allocate new clusters.
> > 4. free extra clusters from free_clusters list if any.
> >
> >
> > This strategy is not optimal, but not bad I thing. Is it ok for you? 
> > (I'm not sure that this all is not premature optimization, and may be 
> > true way is to just free all old staff and reallocate it, as I do.)
>
> With something like this it is not trivial to maintain consistency in 
> qcow2 image in case of fail on reusing (we can get double-linked 
> clusters, or dead links in the image on disk).. Simple solution will 
> be to remove all in_use bitmaps from header extension, or reallocate 
> for them bitmap tables with all zeroes. So, finally:
>
> target: we want to reuse clusters of in_use bitmaps, which was loaded 
> by current qcow2 driver (let's call them our in_use bitmaps.
>
> sequence:
> 1. save bitmap directory in memory
> 2. remove our in_use bitmaps from bitmap directory in disk. from this 
> point, in case of fail we will finish with some leaking clusters and 
> no corruptions.
> 3. free_clusters list = all data clusters of our in_use bitmaps
> 4. for each our in_use bitmap: If table size is not appropriate move 
> bitmap table clusters to free_clusters

better is to free it, to not deceiveqcow2 allocator.. Can I allocate 
several clusters by alloc(size), then assume that I have (size + 
cluster_size - 1)/cluster_size clusters and that I can free these 
clusters separately?

> 5. save bitmaps, using free_clusters list for data clusters, old 
> existing bitmap tables and allocating new space when needed.
> 6. free extra clusters from free_clusters list if any


--
Best regards,
Vladimir