* [Qemu-devel] [PATCH v2] spec/qcow2: bitmaps: zero bitmap table offset
@ 2016-06-30 7:34 Vladimir Sementsov-Ogievskiy
2016-06-30 9:12 ` Denis V. Lunev
0 siblings, 1 reply; 6+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2016-06-30 7:34 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-block, den, kwolf, eblake, jsnow, stefanha, famz,
vsementsov, mreitz
After loading bitmap from image and setting IN_USE flag in it's header,
corresponding data (bitmap table and data clusters) becomes inconsistent
and is no longer needed. It is better to free bitmap table and
corresponding clusters from the image immediately after loading the
bitmap than free them when the bitmap is saved, or deleted or set
non-persistent.
For now it is impossible to store only bitmap header without bitmap
table, as specification requires it. Storing zeroed bitmap table (one or
more clusters) is the only option to implement the behaviour similar to
specified above.
The same problem is for just storing empty bitmaps.
This patch allows storing only bitmap header for empty bitmaps.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
Additional note. Should we also allow here bitmap_table_offset = 1, like
in bitmap table, for the bitmap with all bits set? I am not sure that it
is needed at all, but just to keep the company..
docs/specs/qcow2.txt | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/docs/specs/qcow2.txt b/docs/specs/qcow2.txt
index 80cdfd0..9826222 100644
--- a/docs/specs/qcow2.txt
+++ b/docs/specs/qcow2.txt
@@ -435,9 +435,12 @@ Structure of a bitmap directory entry:
Offset into the image file at which the bitmap table
(described below) for the bitmap starts. Must be aligned to
a cluster boundary.
+ Zero value means that bitmap table is not allocated and the
+ bitmap should be considered as empty (all bits are zero).
8 - 11: bitmap_table_size
- Number of entries in the bitmap table of the bitmap.
+ Number of entries in the bitmap table of the bitmap. It
+ must be zero if bitmap_table_offset is zero.
12 - 15: flags
Bit
--
1.8.3.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH v2] spec/qcow2: bitmaps: zero bitmap table offset
2016-06-30 7:34 [Qemu-devel] [PATCH v2] spec/qcow2: bitmaps: zero bitmap table offset Vladimir Sementsov-Ogievskiy
@ 2016-06-30 9:12 ` Denis V. Lunev
2016-06-30 16:40 ` John Snow
0 siblings, 1 reply; 6+ messages in thread
From: Denis V. Lunev @ 2016-06-30 9:12 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-devel
Cc: qemu-block, kwolf, eblake, jsnow, stefanha, famz, mreitz
On 06/30/2016 10:34 AM, Vladimir Sementsov-Ogievskiy wrote:
> After loading bitmap from image and setting IN_USE flag in it's header,
> corresponding data (bitmap table and data clusters) becomes inconsistent
> and is no longer needed. It is better to free bitmap table and
> corresponding clusters from the image immediately after loading the
> bitmap than free them when the bitmap is saved, or deleted or set
> non-persistent.
>
> For now it is impossible to store only bitmap header without bitmap
> table, as specification requires it. Storing zeroed bitmap table (one or
> more clusters) is the only option to implement the behaviour similar to
> specified above.
>
> The same problem is for just storing empty bitmaps.
>
> This patch allows storing only bitmap header for empty bitmaps.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>
> Additional note. Should we also allow here bitmap_table_offset = 1, like
> in bitmap table, for the bitmap with all bits set? I am not sure that it
> is needed at all, but just to keep the company..
>
> docs/specs/qcow2.txt | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/docs/specs/qcow2.txt b/docs/specs/qcow2.txt
> index 80cdfd0..9826222 100644
> --- a/docs/specs/qcow2.txt
> +++ b/docs/specs/qcow2.txt
> @@ -435,9 +435,12 @@ Structure of a bitmap directory entry:
> Offset into the image file at which the bitmap table
> (described below) for the bitmap starts. Must be aligned to
> a cluster boundary.
> + Zero value means that bitmap table is not allocated and the
> + bitmap should be considered as empty (all bits are zero).
>
> 8 - 11: bitmap_table_size
> - Number of entries in the bitmap table of the bitmap.
> + Number of entries in the bitmap table of the bitmap. It
> + must be zero if bitmap_table_offset is zero.
>
> 12 - 15: flags
> Bit
NACK
no guys, we can not make this change at the moment.
We do have QEMU available in the field which is working
with the currently specified format.
Den
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH v2] spec/qcow2: bitmaps: zero bitmap table offset
2016-06-30 9:12 ` Denis V. Lunev
@ 2016-06-30 16:40 ` John Snow
2016-06-30 17:23 ` Denis V. Lunev
0 siblings, 1 reply; 6+ messages in thread
From: John Snow @ 2016-06-30 16:40 UTC (permalink / raw)
To: Denis V. Lunev, Vladimir Sementsov-Ogievskiy, qemu-devel
Cc: kwolf, famz, qemu-block, mreitz, stefanha
On 06/30/2016 05:12 AM, Denis V. Lunev wrote:
> On 06/30/2016 10:34 AM, Vladimir Sementsov-Ogievskiy wrote:
>> After loading bitmap from image and setting IN_USE flag in it's header,
>> corresponding data (bitmap table and data clusters) becomes inconsistent
>> and is no longer needed. It is better to free bitmap table and
>> corresponding clusters from the image immediately after loading the
>> bitmap than free them when the bitmap is saved, or deleted or set
>> non-persistent.
>>
>> For now it is impossible to store only bitmap header without bitmap
>> table, as specification requires it. Storing zeroed bitmap table (one or
>> more clusters) is the only option to implement the behaviour similar to
>> specified above.
>>
>> The same problem is for just storing empty bitmaps.
>>
>> This patch allows storing only bitmap header for empty bitmaps.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>
>> Additional note. Should we also allow here bitmap_table_offset = 1, like
>> in bitmap table, for the bitmap with all bits set? I am not sure that it
>> is needed at all, but just to keep the company..
>>
>> docs/specs/qcow2.txt | 5 ++++-
>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/docs/specs/qcow2.txt b/docs/specs/qcow2.txt
>> index 80cdfd0..9826222 100644
>> --- a/docs/specs/qcow2.txt
>> +++ b/docs/specs/qcow2.txt
>> @@ -435,9 +435,12 @@ Structure of a bitmap directory entry:
>> Offset into the image file at which the bitmap
>> table
>> (described below) for the bitmap starts. Must be
>> aligned to
>> a cluster boundary.
>> + Zero value means that bitmap table is not
>> allocated and the
>> + bitmap should be considered as empty (all bits
>> are zero).
>> 8 - 11: bitmap_table_size
>> - Number of entries in the bitmap table of the bitmap.
>> + Number of entries in the bitmap table of the
>> bitmap. It
>> + must be zero if bitmap_table_offset is zero.
>> 12 - 15: flags
>> Bit
> NACK
>
> no guys, we can not make this change at the moment.
> We do have QEMU available in the field which is working
> with the currently specified format.
>
> Den
>
But I think the new format is a /compatible/ change. Under the old spec,
I think this field is *NEVER* zero.
Am I wrong?
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH v2] spec/qcow2: bitmaps: zero bitmap table offset
2016-06-30 16:40 ` John Snow
@ 2016-06-30 17:23 ` Denis V. Lunev
2016-07-01 8:12 ` Kevin Wolf
0 siblings, 1 reply; 6+ messages in thread
From: Denis V. Lunev @ 2016-06-30 17:23 UTC (permalink / raw)
To: John Snow, Vladimir Sementsov-Ogievskiy, qemu-devel
Cc: kwolf, famz, qemu-block, mreitz, stefanha
On 06/30/2016 07:40 PM, John Snow wrote:
>
> On 06/30/2016 05:12 AM, Denis V. Lunev wrote:
>> On 06/30/2016 10:34 AM, Vladimir Sementsov-Ogievskiy wrote:
>>> After loading bitmap from image and setting IN_USE flag in it's header,
>>> corresponding data (bitmap table and data clusters) becomes inconsistent
>>> and is no longer needed. It is better to free bitmap table and
>>> corresponding clusters from the image immediately after loading the
>>> bitmap than free them when the bitmap is saved, or deleted or set
>>> non-persistent.
>>>
>>> For now it is impossible to store only bitmap header without bitmap
>>> table, as specification requires it. Storing zeroed bitmap table (one or
>>> more clusters) is the only option to implement the behaviour similar to
>>> specified above.
>>>
>>> The same problem is for just storing empty bitmaps.
>>>
>>> This patch allows storing only bitmap header for empty bitmaps.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> ---
>>>
>>> Additional note. Should we also allow here bitmap_table_offset = 1, like
>>> in bitmap table, for the bitmap with all bits set? I am not sure that it
>>> is needed at all, but just to keep the company..
>>>
>>> docs/specs/qcow2.txt | 5 ++++-
>>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/docs/specs/qcow2.txt b/docs/specs/qcow2.txt
>>> index 80cdfd0..9826222 100644
>>> --- a/docs/specs/qcow2.txt
>>> +++ b/docs/specs/qcow2.txt
>>> @@ -435,9 +435,12 @@ Structure of a bitmap directory entry:
>>> Offset into the image file at which the bitmap
>>> table
>>> (described below) for the bitmap starts. Must be
>>> aligned to
>>> a cluster boundary.
>>> + Zero value means that bitmap table is not
>>> allocated and the
>>> + bitmap should be considered as empty (all bits
>>> are zero).
>>> 8 - 11: bitmap_table_size
>>> - Number of entries in the bitmap table of the bitmap.
>>> + Number of entries in the bitmap table of the
>>> bitmap. It
>>> + must be zero if bitmap_table_offset is zero.
>>> 12 - 15: flags
>>> Bit
>> NACK
>>
>> no guys, we can not make this change at the moment.
>> We do have QEMU available in the field which is working
>> with the currently specified format.
>>
>> Den
>>
> But I think the new format is a /compatible/ change. Under the old spec,
> I think this field is *NEVER* zero.
>
> Am I wrong?
yes
but as far as I can understand this breaks backward compatibility,
i.e. software working with the current revision of the specification
will not be able to load new images.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH v2] spec/qcow2: bitmaps: zero bitmap table offset
2016-06-30 17:23 ` Denis V. Lunev
@ 2016-07-01 8:12 ` Kevin Wolf
2016-07-01 8:39 ` Vladimir Sementsov-Ogievskiy
0 siblings, 1 reply; 6+ messages in thread
From: Kevin Wolf @ 2016-07-01 8:12 UTC (permalink / raw)
To: Denis V. Lunev
Cc: John Snow, Vladimir Sementsov-Ogievskiy, qemu-devel, famz,
qemu-block, mreitz, stefanha
Am 30.06.2016 um 19:23 hat Denis V. Lunev geschrieben:
> On 06/30/2016 07:40 PM, John Snow wrote:
> >
> >On 06/30/2016 05:12 AM, Denis V. Lunev wrote:
> >>On 06/30/2016 10:34 AM, Vladimir Sementsov-Ogievskiy wrote:
> >>>After loading bitmap from image and setting IN_USE flag in it's header,
> >>>corresponding data (bitmap table and data clusters) becomes inconsistent
> >>>and is no longer needed. It is better to free bitmap table and
> >>>corresponding clusters from the image immediately after loading the
> >>>bitmap than free them when the bitmap is saved, or deleted or set
> >>>non-persistent.
> >>>
> >>>For now it is impossible to store only bitmap header without bitmap
> >>>table, as specification requires it. Storing zeroed bitmap table (one or
> >>>more clusters) is the only option to implement the behaviour similar to
> >>>specified above.
> >>>
> >>>The same problem is for just storing empty bitmaps.
> >>>
> >>>This patch allows storing only bitmap header for empty bitmaps.
> >>>
> >>>Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> >>>---
> >>>
> >>>Additional note. Should we also allow here bitmap_table_offset = 1, like
> >>>in bitmap table, for the bitmap with all bits set? I am not sure that it
> >>>is needed at all, but just to keep the company..
> >>>
> >>> docs/specs/qcow2.txt | 5 ++++-
> >>> 1 file changed, 4 insertions(+), 1 deletion(-)
> >>>
> >>>diff --git a/docs/specs/qcow2.txt b/docs/specs/qcow2.txt
> >>>index 80cdfd0..9826222 100644
> >>>--- a/docs/specs/qcow2.txt
> >>>+++ b/docs/specs/qcow2.txt
> >>>@@ -435,9 +435,12 @@ Structure of a bitmap directory entry:
> >>> Offset into the image file at which the bitmap
> >>>table
> >>> (described below) for the bitmap starts. Must be
> >>>aligned to
> >>> a cluster boundary.
> >>>+ Zero value means that bitmap table is not
> >>>allocated and the
> >>>+ bitmap should be considered as empty (all bits
> >>>are zero).
> >>> 8 - 11: bitmap_table_size
> >>>- Number of entries in the bitmap table of the bitmap.
> >>>+ Number of entries in the bitmap table of the
> >>>bitmap. It
> >>>+ must be zero if bitmap_table_offset is zero.
> >>> 12 - 15: flags
> >>> Bit
> >>NACK
> >>
> >>no guys, we can not make this change at the moment.
> >>We do have QEMU available in the field which is working
> >>with the currently specified format.
> >>
> >>Den
> >>
> >But I think the new format is a /compatible/ change. Under the old spec,
> >I think this field is *NEVER* zero.
> >
> >Am I wrong?
> yes
>
> but as far as I can understand this breaks backward compatibility,
> i.e. software working with the current revision of the specification
> will not be able to load new images.
This is okay if it means that it cleanly errors out. I'm a bit worried
that it might take the 0 literally, though, and use the image header as
the bitmap table. Can you check that, Den?
If that's the case, we can make the change only with an incompatible
feature bit, and that's probably not worth it.
Kevin
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH v2] spec/qcow2: bitmaps: zero bitmap table offset
2016-07-01 8:12 ` Kevin Wolf
@ 2016-07-01 8:39 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 6+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2016-07-01 8:39 UTC (permalink / raw)
To: Kevin Wolf, Denis V. Lunev
Cc: John Snow, qemu-devel, famz, qemu-block, mreitz, stefanha
On 01.07.2016 11:12, Kevin Wolf wrote:
> Am 30.06.2016 um 19:23 hat Denis V. Lunev geschrieben:
>> On 06/30/2016 07:40 PM, John Snow wrote:
>>> On 06/30/2016 05:12 AM, Denis V. Lunev wrote:
>>>> On 06/30/2016 10:34 AM, Vladimir Sementsov-Ogievskiy wrote:
>>>>> After loading bitmap from image and setting IN_USE flag in it's header,
>>>>> corresponding data (bitmap table and data clusters) becomes inconsistent
>>>>> and is no longer needed. It is better to free bitmap table and
>>>>> corresponding clusters from the image immediately after loading the
>>>>> bitmap than free them when the bitmap is saved, or deleted or set
>>>>> non-persistent.
>>>>>
>>>>> For now it is impossible to store only bitmap header without bitmap
>>>>> table, as specification requires it. Storing zeroed bitmap table (one or
>>>>> more clusters) is the only option to implement the behaviour similar to
>>>>> specified above.
>>>>>
>>>>> The same problem is for just storing empty bitmaps.
>>>>>
>>>>> This patch allows storing only bitmap header for empty bitmaps.
>>>>>
>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>>> ---
>>>>>
>>>>> Additional note. Should we also allow here bitmap_table_offset = 1, like
>>>>> in bitmap table, for the bitmap with all bits set? I am not sure that it
>>>>> is needed at all, but just to keep the company..
>>>>>
>>>>> docs/specs/qcow2.txt | 5 ++++-
>>>>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/docs/specs/qcow2.txt b/docs/specs/qcow2.txt
>>>>> index 80cdfd0..9826222 100644
>>>>> --- a/docs/specs/qcow2.txt
>>>>> +++ b/docs/specs/qcow2.txt
>>>>> @@ -435,9 +435,12 @@ Structure of a bitmap directory entry:
>>>>> Offset into the image file at which the bitmap
>>>>> table
>>>>> (described below) for the bitmap starts. Must be
>>>>> aligned to
>>>>> a cluster boundary.
>>>>> + Zero value means that bitmap table is not
>>>>> allocated and the
>>>>> + bitmap should be considered as empty (all bits
>>>>> are zero).
>>>>> 8 - 11: bitmap_table_size
>>>>> - Number of entries in the bitmap table of the bitmap.
>>>>> + Number of entries in the bitmap table of the
>>>>> bitmap. It
>>>>> + must be zero if bitmap_table_offset is zero.
>>>>> 12 - 15: flags
>>>>> Bit
>>>> NACK
>>>>
>>>> no guys, we can not make this change at the moment.
>>>> We do have QEMU available in the field which is working
>>>> with the currently specified format.
>>>>
>>>> Den
>>>>
>>> But I think the new format is a /compatible/ change. Under the old spec,
>>> I think this field is *NEVER* zero.
>>>
>>> Am I wrong?
>> yes
>>
>> but as far as I can understand this breaks backward compatibility,
>> i.e. software working with the current revision of the specification
>> will not be able to load new images.
> This is okay if it means that it cleanly errors out. I'm a bit worried
> that it might take the 0 literally, though, and use the image header as
> the bitmap table. Can you check that, Den?
>
> If that's the case, we can make the change only with an incompatible
> feature bit, and that's probably not worth it.
>
> Kevin
Current version will error out with "Bitmap doesn't satisfy the
constraints.", because I check that
int check_constraints(...)
uint64_t phys_bitmap_bytes =
(uint64_t)h->bitmap_table_size * cluster_size;
uint64_t max_virtual_bits = (phys_bitmap_bytes * 8) <<
h->granularity_bits;
int fail =
.... ||
(disk_size > max_virtual_bits) ||
...
return fail ? -EINVAL : 0;
}
(max_virtual_bits will be zero if bitmap_table_size is zero)
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-07-01 8:40 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-06-30 7:34 [Qemu-devel] [PATCH v2] spec/qcow2: bitmaps: zero bitmap table offset Vladimir Sementsov-Ogievskiy
2016-06-30 9:12 ` Denis V. Lunev
2016-06-30 16:40 ` John Snow
2016-06-30 17:23 ` Denis V. Lunev
2016-07-01 8:12 ` Kevin Wolf
2016-07-01 8:39 ` Vladimir Sementsov-Ogievskiy
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.