Re: [Qemu-devel] [PATCH 1/8] spec: add qcow2-dirty-bitmaps specification

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

On 10.06.2015 16:24, Stefan Hajnoczi wrote:
> On Wed, Jun 10, 2015 at 11:19:30AM +0300, Vladimir Sementsov-Ogievskiy wrote:
>> On 09.06.2015 20:03, Stefan Hajnoczi wrote:
>>> On Mon, Jun 08, 2015 at 06:21:19PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>>>> @@ -166,6 +167,19 @@ the header extension data. Each entry look like this:
>>>>                       terminated if it has full length)
>>>> +== Dirty bitmaps ==
>>>> +
>>>> +Dirty bitmaps is an optional header extension. It provides a possibility of
>>>> +storing dirty bitmaps in qcow2 image. The fields are:
>>>> +
>>>> +          0 -  3:  nb_dirty_bitmaps
>>>> +                   Number of dirty bitmaps contained in the image
>>> Is there a maximum?
>> hmm. any proposals for this?
> 65535 seems practical.

So, you suggest to reduce this field width to 2b? And additional 2 bytes 
reserved field, to achieve 8b-alignment?

>
>>>> +=== Cluster mapping ===
>>>> +
>>>> +Dirty bitmaps are stored using a ONE-level structure for the mapping of
>>>> +bitmaps to host clusters. There is only an L1 table.
>>>> +
>>>> +The L1 table has a variable size (stored in the Bitmap table entry) and may
>>>> +use multiple clusters, however it must be contiguous in the image file.
>>> The use of "L1 table" could be confusing.  The refcount metadata uses
>>> "refcount table" and "refcount block" to describe a one-level table.
>> I agree. Hmm.. dirty bitmaps table? ok?
> Yes, that is good.
>
>>>> +
>>>> +Given an offset into the bitmap, the offset into the image file can be
>>>> +obtained as follows:
>>>> +
>>>> +    offset = l1_table[offset / cluster_size] + (offset % cluster_size)
>>> It might help to add granularity to this formula.
>>>
>>> Instead of "offset", "bit_number" or "bitnr" might be clearer since
>>> "offset" means something different in other parts of the document.
>> Hmm. In my opinion, the bitmap here is stored as raw data. And granularity
>> is an additional parameter (for deserializing this data). So, it is an
>> offset in bytes for this data. The format is not for accessing bitmap bits,
>> it's only for loading the whole bitmap one time.
> You are right, it wasn't clear when I read this the first time.  My
> problem was the "offset into the bitmap" doesn't have any units.  So
> let's make this more explicit.  Can you document how to go from a bit
> number down to the offset?


-- 
Best regards,
Vladimir
* now, @virtuozzo.com instead of @parallels.com. Sorry for this inconvenience.