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

[Max Reitz]

On 07.11.2016 17:12, 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
>> 
>> ---
>> 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 

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

[Vladimir Sementsov-Ogievskiy]

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

---
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 

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

[Max Reitz]

On 26.10.2016 11: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
>> 
>> ---
>> 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.. 

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

---
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 

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

[Vladimir Sementsov-Ogievskiy]

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

---
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 

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

[Vladimir Sementsov-Ogievskiy]

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

---
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 

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

[Vladimir Sementsov-Ogievskiy]

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

---
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..



--
Best regards,
Vladimir

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

[Vladimir Sementsov-Ogievskiy]

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

---
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.



--
Best regards,
Vladimir

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

[Max Reitz]

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
>>> 
>>> ---
>>>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.

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



signature.asc
Description: OpenPGP digital signature

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

[Max Reitz]

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
>> 
>> ---
>>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.

Max



signature.asc
Description: OpenPGP digital signature

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

[Max Reitz]

On 24.10.2016 12:32, Vladimir Sementsov-Ogievskiy wrote:
> 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 
> ---
>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...

May happen. But not necessarily, and it will probably still save time as
you can reuse existing allocations and don't have to free everything.

Max



signature.asc
Description: OpenPGP digital signature

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

[Vladimir Sementsov-Ogievskiy]

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 


---
   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).


Also, if I don't free them on open, I'll have to free them on remove of 
bdrv dirty bitmap..










Any way, storing an invalid copy of online data is bad I think.
Therefore I will have to free bitmap data clusters and zero bitmap 
table

in file.

Why can't you just set this flag to mark it invalid? In case qemu
crashes, in one case you'll have some invalid bitmaps and in the other
you won't have any of them at all. I don't think there's a difference
from that perspective.

Max







--
Best regards,
Vladimir

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

[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 
---
   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...





Any way, storing an invalid copy of online data is bad I think.
Therefore I will have to free bitmap data clusters and zero bitmap table
in file.

Why can't you just set this flag to mark it invalid? In case qemu
crashes, in one case you'll have some invalid bitmaps and in the other
you won't have any of them at all. I don't think there's a difference
from that perspective.

Max




--
Best regards,
Vladimir

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

[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 
>>> ---
>>>   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.

> Any way, storing an invalid copy of online data is bad I think.
> Therefore I will have to free bitmap data clusters and zero bitmap table
> in file.

Why can't you just set this flag to mark it invalid? In case qemu
crashes, in one case you'll have some invalid bitmaps and in the other
you won't have any of them at all. I don't think there's a difference
from that perspective.

Max



signature.asc
Description: OpenPGP digital signature

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

[Vladimir Sementsov-Ogievskiy]

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 
---
  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?

Any way, storing an invalid copy of online data is bad I think. 
Therefore I will have to free bitmap data clusters and zero bitmap table 
in file.





diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index a5be25a..8cf40f0 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -28,6 +28,7 @@
  #include "qemu/osdep.h"
  #include "qapi/error.h"
  #include "qemu/cutils.h"
+#include "exec/log.h"
  
  #include "block/block_int.h"

  #include "block/qcow2.h"
@@ -44,7 +45,8 @@
  #define BME_MAX_NAME_SIZE 1023
  
  /* Bitmap directory entry flags */

-#define BME_RESERVED_FLAGS 0xfffd
+#define BME_RESERVED_FLAGS 0xfffc
+#define BME_FLAG_IN_USE 1

This should be (1U << 0) to be consistent with the definition of
BME_FLAG_AUTO.


  #define BME_FLAG_AUTO   (1U << 1)
  
  /* bits [1, 8] U [56, 63] are reserved */

@@ -287,6 +289,11 @@ static BdrvDirtyBitmap *load_bitmap(BlockDriverState *bs,
  BdrvDirtyBitmap *bitmap = NULL;
  char *name = g_strndup(dir_entry_name_notcstr(entry), entry->name_size);
  
+if (entry->flags & BME_FLAG_IN_USE) {

+error_setg(errp, "Bitmap '%s' is in use", name);
+goto fail;
+}
+
  ret = bitmap_table_load(bs, entry, _table);
  if (ret < 0) {
  error_setg_errno(errp, -ret,
@@ -533,6 +540,14 @@ int qcow2_read_bitmaps(BlockDriverState *bs, Error **errp)
  for_each_bitmap_dir_entry(e, dir, dir_size) {
  uint32_t fl = e->flags;
  
+if (fl & BME_FLAG_IN_USE) {

+qemu_log("qcow2 warning: "
+ "removing in_use bitmap '%.*s' for image %s.\n",
+ e->name_size, (char *)(e + 1), 
bdrv_get_device_or_node_name(bs));

I'm not sure whether qemu_log() is the right way to do this. I think
fprintf() to stderr might actually be more appropriate. qemu_log() looks
like it's mostly used for debugging purposes.

Also, this is not a warning. You are just doing it. You are not warning
someone about a cliff when he's already fallen off.

But you can actually make it a warning: Just leave the bitmap in the
image (maybe someone can do something with it?) and instead let qemu-img
check clean it up. The warning should then just be "Warning: Ignoring
in_use bitmap %.*s, use qemu-img check -r all to delete it".

Max


+
+continue;
+}
+
  if (fl & BME_FLAG_AUTO) {
  BdrvDirtyBitmap *bitmap = load_bitmap(bs, e, errp);
  if (bitmap == NULL) {






--
Best regards,
Vladimir

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

[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 
> ---
>  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.

> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
> index a5be25a..8cf40f0 100644
> --- a/block/qcow2-bitmap.c
> +++ b/block/qcow2-bitmap.c
> @@ -28,6 +28,7 @@
>  #include "qemu/osdep.h"
>  #include "qapi/error.h"
>  #include "qemu/cutils.h"
> +#include "exec/log.h"
>  
>  #include "block/block_int.h"
>  #include "block/qcow2.h"
> @@ -44,7 +45,8 @@
>  #define BME_MAX_NAME_SIZE 1023
>  
>  /* Bitmap directory entry flags */
> -#define BME_RESERVED_FLAGS 0xfffd
> +#define BME_RESERVED_FLAGS 0xfffc
> +#define BME_FLAG_IN_USE 1

This should be (1U << 0) to be consistent with the definition of
BME_FLAG_AUTO.

>  #define BME_FLAG_AUTO   (1U << 1)
>  
>  /* bits [1, 8] U [56, 63] are reserved */
> @@ -287,6 +289,11 @@ static BdrvDirtyBitmap *load_bitmap(BlockDriverState *bs,
>  BdrvDirtyBitmap *bitmap = NULL;
>  char *name = g_strndup(dir_entry_name_notcstr(entry), entry->name_size);
>  
> +if (entry->flags & BME_FLAG_IN_USE) {
> +error_setg(errp, "Bitmap '%s' is in use", name);
> +goto fail;
> +}
> +
>  ret = bitmap_table_load(bs, entry, _table);
>  if (ret < 0) {
>  error_setg_errno(errp, -ret,
> @@ -533,6 +540,14 @@ int qcow2_read_bitmaps(BlockDriverState *bs, Error 
> **errp)
>  for_each_bitmap_dir_entry(e, dir, dir_size) {
>  uint32_t fl = e->flags;
>  
> +if (fl & BME_FLAG_IN_USE) {
> +qemu_log("qcow2 warning: "
> + "removing in_use bitmap '%.*s' for image %s.\n",
> + e->name_size, (char *)(e + 1), 
> bdrv_get_device_or_node_name(bs));

I'm not sure whether qemu_log() is the right way to do this. I think
fprintf() to stderr might actually be more appropriate. qemu_log() looks
like it's mostly used for debugging purposes.

Also, this is not a warning. You are just doing it. You are not warning
someone about a cliff when he's already fallen off.

But you can actually make it a warning: Just leave the bitmap in the
image (maybe someone can do something with it?) and instead let qemu-img
check clean it up. The warning should then just be "Warning: Ignoring
in_use bitmap %.*s, use qemu-img check -r all to delete it".

Max

> +
> +continue;
> +}
> +
>  if (fl & BME_FLAG_AUTO) {
>  BdrvDirtyBitmap *bitmap = load_bitmap(bs, e, errp);
>  if (bitmap == NULL) {
> 




signature.asc
Description: OpenPGP digital signature