qemu-devel
[Top][All Lists]
Advanced

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

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


From: Kevin Wolf
Subject: Re: [Qemu-devel] [PATCH 1/3 v9] add-cow file format
Date: Wed, 30 May 2012 09:33:48 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:12.0) Gecko/20120430 Thunderbird/12.0.1

Am 30.05.2012 04:10, schrieb Anthony Liguori:
> On 05/08/2012 01:34 AM, Dong Xu Wang wrote:
>> Provide a new file format: add-cow. The usage can be found in add-cow.txt of
>> this patch.
>>
>> CC: Kevin Wolf<address@hidden>
>> CC: Stefan Hajnoczi<address@hidden>
>> Signed-off-by: Dong Xu Wang<address@hidden>
> 
> You should split out the spec to be the first patch.  That makes it easier 
> for 
> people to review the specification independent of the code and also makes 
> subsequent code review easier.

Yes, please.

>> ---
>>   Makefile.objs          |    1 +
>>   block.c                |    2 +-
>>   block.h                |    1 +
>>   block/add-cow-cache.c  |  193 +++++++++++++++++++++
>>   block/add-cow.c        |  446 
>> ++++++++++++++++++++++++++++++++++++++++++++++++
>>   block/add-cow.h        |   83 +++++++++
>>   block_int.h            |    1 +
>>   docs/specs/add-cow.txt |   68 ++++++++
>>   qemu-img.c             |   39 +++++
>>   9 files changed, 833 insertions(+), 1 deletion(-)
>>   create mode 100644 block/add-cow-cache.c
>>   create mode 100644 block/add-cow.c
>>   create mode 100644 block/add-cow.h
>>   create mode 100644 docs/specs/add-cow.txt
>>
>> diff --git a/Makefile.objs b/Makefile.objs
>> index 70c5c79..10c5c52 100644
>> --- a/Makefile.objs
>> +++ b/Makefile.objs
>> @@ -52,6 +52,7 @@ block-nested-y += raw.o cow.o qcow.o vdi.o vmdk.o cloop.o 
>> dmg.o bochs.o vpc.o vv
>>   block-nested-y += qcow2.o qcow2-refcount.o qcow2-cluster.o 
>> qcow2-snapshot.o qcow2-cache.o
>>   block-nested-y += qed.o qed-gencb.o qed-l2-cache.o qed-table.o 
>> qed-cluster.o
>>   block-nested-y += qed-check.o
>> +block-nested-y += add-cow.o add-cow-cache.o
>>   block-nested-y += parallels.o nbd.o blkdebug.o sheepdog.o blkverify.o
>>   block-nested-y += stream.o
>>   block-nested-$(CONFIG_WIN32) += raw-win32.o
>> diff --git a/block.c b/block.c
>> index 43c794c..206860c 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -196,7 +196,7 @@ static void bdrv_io_limits_intercept(BlockDriverState 
>> *bs,
>>   }
>>
>>   /* check if the path starts with "<protocol>:" */
>> -static int path_has_protocol(const char *path)
>> +int path_has_protocol(const char *path)
>>   {
>>   #ifdef _WIN32
>>       if (is_windows_drive(path) ||
>> diff --git a/block.h b/block.h
>> index f163e54..f74c79e 100644
>> --- a/block.h
>> +++ b/block.h
>> @@ -319,6 +319,7 @@ char *bdrv_snapshot_dump(char *buf, int buf_size, 
>> QEMUSnapshotInfo *sn);
>>
>>   char *get_human_readable_size(char *buf, int buf_size, int64_t size);
>>   int path_is_absolute(const char *path);
>> +int path_has_protocol(const char *path);
>>   void path_combine(char *dest, int dest_size,
>>                     const char *base_path,
>>                     const char *filename);
>> diff --git a/block/add-cow-cache.c b/block/add-cow-cache.c
>> new file mode 100644
>> index 0000000..3ae23c1
>> --- /dev/null
>> +++ b/block/add-cow-cache.c
>> @@ -0,0 +1,193 @@
>> +/*
>> + * Cache For QEMU ADD-COW Disk Format
>> + *
>> + * This work is licensed under the terms of the GNU LGPL, version 2 or 
>> later.
>> + * See the COPYING.LIB file in the top-level directory.
>> + *
>> + */
>>
> 
> This is not a valid copyright block.  Look at hw/virtio.c for an example.  If 
> you want to do LGPL instead of GPL, it should also be 2.1 or later.

You mean because the name of the copyright holder is missing? Would be
nice to include indeed, but I don't think it makes a legal difference.
And the vast majority of source files doesn't have the names of all
copyright holders there anyway.

>> +#include "block_int.h"
>> +#include "qemu-common.h"
>> +#include "add-cow.h"
>> +
>> +/* Based on qcow2-cache.c */
> 
> If the code is based on qcow2, then you need to preserve the qcow2 copyrights 
> in 
> this file.

Or ideally we would generalise qcow2-cache.c so that it can be used by
both. Not sure how easy it would be, though.

>> +AddCowCache *add_cow_cache_create(BlockDriverState *bs, int num_tables,
>> +    bool writethrough)
>> +{
>> +    AddCowCache *c;
>> +    int i;
>> +
>> +    c = g_malloc0(sizeof(*c));
>> +    c->size = num_tables;
>> +    c->entries = g_malloc0(sizeof(*c->entries) * num_tables);
>> +    c->writethrough = writethrough;
>> +    c->entry_size = ADD_COW_CACHE_ENTRY_SIZE;
>> +
>> +    for (i = 0; i<  c->size; i++) {
>> +        c->entries[i].table = qemu_blockalign(bs, c->entry_size);
>> +        c->entries[i].offset = -1;
>> +    }
>> +
>> +    return c;
>> +}
>> +
>> +int add_cow_cache_destroy(BlockDriverState *bs, AddCowCache *c)
>> +{
>> +    int i;
>> +
>> +    for (i = 0; i<  c->size; i++) {
>> +        qemu_vfree(c->entries[i].table);
>> +    }
>> +
>> +    g_free(c->entries);
>> +    g_free(c);
>> +
>> +    return 0;
>> +}
>> +
>> +static int add_cow_cache_entry_flush(BlockDriverState *bs,
>> +    AddCowCache *c,
>> +    int i)
>> +{
>> +    BDRVAddCowState *s = bs->opaque;
>> +    int ret = 0;
>> +
>> +    if (!c->entries[i].dirty || -1 == c->entries[i].offset) {
>> +        return ret;
>> +    }
>> +
>> +    ret = bdrv_pwrite(bs->file, sizeof(AddCowHeader) + c->entries[i].offset,
>> +        c->entries[i].table,
>> +        MIN(c->entry_size, s->bitmap_size - c->entries[i].offset));
> 
> This is a synchronous I/O function.  We shouldn't introduce new formats that 
> have *any* synchronous I/O in them.

No, this is fine. We're inside a coroutine, this doesn't block anything.

> Honestly, I think using coroutines for this format is a mistake.  It's simple 
> enough that doing a state machine would be straight forward enough.

Introducing new state machines would be backwards. Eventually we want to
have a threaded block layer and state machines just don't fit there.
Coroutine based code is much closer.

Kevin



reply via email to

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