Re: [Qemu-devel] [PATCH] support add-cow file format

[Dong Xu Wang <wdongxu@linux.vnet.ibm.com>]

于Fri 09 Sep 2011 10:27:26 PM CST,Kevin Wolf写到:
> Am 09.09.2011 07:48, schrieb Dong Xu Wang:
>> As raw file format does not support backing_file and copy on write feature, so 
>> I add COW to it to support backing_file option. I store dirty bitmap in an 
>> add-cow file. When executed, it looks like this:
>> qemu-img create -f add-cow -o backing_file=ubuntu.img,image_file=test.img test.add-cow
>> qemu -drive if=virtio,file=test.add-cow -m 1024 
>>
>> (test.img is a raw format file; test.add-cow stores bitmap)
>>
>> Signed-off-by: Dong Xu Wang <wdongxu@linux.vnet.ibm.com>
>
> You should not make any changes to generic code, except maybe add
> something to bdrv_get_info(). In particular you shouldn't need to touch
> bdrv_open() or bdrv_create() at all.
>
> The one required change in the approach for this to work is that you
> shouldn't view raw+add_cow as a unit, but add_cow should be treated as
> something separate that happens to be stacked on a raw file (which is
> created separately).
>
> Then you can do almost everything in block/add-cow.c.
>
>> ---
>>  Makefile.objs   |    1 +
>>  block.c         |   83 ++++++++++-
>>  block.h         |    2 +
>>  block/add-cow.c |  456 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  block_int.h     |    6 +
>>  qemu-img.c      |   10 ++
>>  6 files changed, 555 insertions(+), 3 deletions(-)
>>  create mode 100644 block/add-cow.c
>>
>> diff --git a/Makefile.objs b/Makefile.objs
>> index 26b885b..1402f9f 100644
>> --- a/Makefile.objs
>> +++ b/Makefile.objs
>> @@ -31,6 +31,7 @@ block-obj-$(CONFIG_LINUX_AIO) += linux-aio.o
>>  
>>  block-nested-y += raw.o cow.o qcow.o vdi.o vmdk.o cloop.o dmg.o bochs.o vpc.o vvfat.o
>>  block-nested-y += qcow2.o qcow2-refcount.o qcow2-cluster.o qcow2-snapshot.o qcow2-cache.o
>> +block-nested-y += add-cow.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 += parallels.o nbd.o blkdebug.o sheepdog.o blkverify.o
>> diff --git a/block.c b/block.c
>> index a8c789a..c797cfc 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -369,7 +369,7 @@ static int find_image_format(const char *filename, BlockDriver **pdrv)
>>  {
>>      int ret, score, score_max;
>>      BlockDriver *drv1, *drv;
>> -    uint8_t buf[2048];
>> +    uint8_t buf[4096];
>>      BlockDriverState *bs;
>
> What's the reason for this change?
>
The size of add_cow_header in my code is larger than 2048.
>> diff --git a/block/add-cow.c b/block/add-cow.c
>> new file mode 100644
>> index 0000000..f4b67e5
>> --- /dev/null
>> +++ b/block/add-cow.c
>> @@ -0,0 +1,456 @@
>> +#include "qemu-common.h"
>> +#include "block_int.h"
>> +#include "module.h"
>> +
>> +#define ADD_COW_MAGIC  (((uint64_t)'A' << 56) | ((uint64_t)'D' << 48) | \
>> +                        ((uint64_t)'D' << 40) | ((uint64_t)'_' << 32) | \
>> +                        ((uint64_t)'C' << 24) | ((uint64_t)'O' << 16) | \
>> +                        ((uint64_t)'W' << 8) | 0xFF)
>> +#define ADD_COW_VERSION 1
>> +
>> +struct add_cow_header {
>> +    uint64_t magic;
>> +    uint32_t version;
>> +    char backing_file[1024];
>> +    char image_file[1024];
>> +    uint64_t size;
>> +    uint32_t sectorsize;
>> +} add_cow_header;
>
> QEMU_PACKED
Sorry, what does QEMU_PACKED mean?
>
>> +typedef struct BDRVAddCowState {
>> +    CoMutex lock;
>> +    CoMutex bitmap_lock;
>> +} BDRVAddCowState;
>> +
>> +typedef struct AddCowAIOCB {
>> +    BlockDriverAIOCB common;
>> +    int64_t sector_num;
>> +    QEMUIOVector *qiov;
>> +    int remaining_sectors;
>> +    int cur_nr_sectors;
>> +    uint64_t bytes_done;
>> +    bool is_write;
>> +    QEMUIOVector hd_qiov;
>> +    QEMUBH *bh;
>> +
>> +} AddCowAIOCB;
>
> You shouldn't be using AIOCBs with a coroutine-based block driver.
> Instead you should just use variables on the stack and function parameters.
>
>> +static int add_cow_flush(BlockDriverState *bs)
>> +{
>> +    return bdrv_flush(bs->file);
>> +}
>
> What about bs->image_hd?
>
>> @@ -208,6 +209,11 @@ struct BlockDriverState {
>>      int in_use; /* users other than guest access, eg. block migration */
>>      QTAILQ_ENTRY(BlockDriverState) list;
>>      void *private;
>> +
>> +    char image_file[1024];
>> +    BlockDriverState *image_hd;
>> +    uint8_t *bitmap;
>> +    uint64_t bitmap_size;
>>  };
>
> These belong in BDRVAddCowState.
>
> Kevin
>
>
Thanks for your comments Kevin, I will produce a second version soon.