qemu-block
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Vladimir Sementsov-Ogievskiy
Subject: Re: [Qemu-block] [PATCH 12/22] qcow2-bitmap: add IN_USE flag
Date: Mon, 7 Nov 2016 19:12:49 +0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0

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.

Now I think, that loading auto_loading bitmaps in qcow2_open is wrong. I should load them only in bdrv_open, to avoid reloading bitmaps on drv->bdrv_open/drv->bdrv_close (they are called from bdrv_snapshot_goto).

So, it would be like this:

on bdrv_open, after drv->bdrv_open call drv->load_autoloading_bitmaps,
which will load bitmaps, mark them in_use in the image (if it is writable), create corresponding BdrvDirtyBitmaps. If there _any_ conflicts with existing BdrvDirtyBitmaps then fail.

on bdrv_close, before drv->bdrv_close call drv->store_persitstent_bitmaps,
which will store persistent bitmaps, set in_use to false and _delete_ corresponding BdrvDirtyBitmaps.

Also, in qcow2_reopen_prepare in case of changing write-ability from false to true we need to mark corresponding bitmaps in the image as in_use.. And something like this for incoming migration too.

qcow2_open will only load header extension fields to bs->opaque, and check these fields. It will not load bitmap directory.




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.




--
Best regards,
Vladimir




reply via email to

[Prev in Thread] Current Thread [Next in Thread]