Re: [Qemu-devel] [PATCH 3/6] add-cow file format

[Kevin Wolf <kwolf@redhat.com>]

Am 01.08.2012 16:14, schrieb Stefan Hajnoczi:
> On Wed, Aug 1, 2012 at 2:57 PM, Eric Blake <eblake@redhat.com> wrote:
>> On 07/31/2012 10:51 AM, Dong Xu Wang wrote:
>>> This is the implementation code for add-cow file format. Because image_file
>>> might be very huge, then we can't read entire bitmap into memory, we must use
>>> a cache. Since qcow-cache.c has implemted cache code, we can create our cache
>>
>> s/implemted/implemented/
>>
>>> code based on it.
>>
>> Just wondering if Paolo's HBitmap code for drive-mirror might be a more
>> efficient way to implement your caching.
>>
>>>
>>> Signed-off-by: Dong Xu Wang <wdongxu@linux.vnet.ibm.com>
>>> ---
>>>  block/Makefile.objs   |    1 +
>>>  block/add-cow-cache.c |  206 +++++++++++++++++
>>>  block/add-cow.c       |  599 +++++++++++++++++++++++++++++++++++++++++++++++++
>>>  block/add-cow.h       |  101 +++++++++
>>>  block_int.h           |    2 +
>>
>> Rather than adding a new implementation for code duplication, can you
>> refactor the existing implementation to be reusable, and update
>> qcow-cache.c to call into the common refactored code?
> 
> I looked at this and block/qcow2-cache.c seems fairly clean and
> generic.  The only things that need to be parameterized are the
> s->l2_table_cache/s->refcount_block_cache and s->cluster_size.
> 
> These can be fixed by adding fields to Qcow2Cache:
> size_t table_size; /* cached table size in bytes */
> BlkDebugEvent flush_event;
> BlkDebugEvent load_event;
> 
> Or maybe a table type enum (L2/refcount blocks/bitmap blocks).
> 
> If Kevin agrees with extracting this from qcow2 (i.e. we can't add
> more qcow2-specific behavior easily in the future), then this looks
> doable.

I don't think we'd want to add qcow2 specific stuff there, and I've been
meaning to generalise it for quite a while, so feel free to do it.

One thing to note is that it currently depends on qcow2's s->lock being
taken while it's called. Eventually we'll want finer grained locking in
the cache itself and I believe this will also require some changes in
the interface, but for now add-cow can just do the same qcow2 and take
its global CoMutex.

Kevin