Re: [Qemu-devel] [PATCH] Add new block driver for the VDI format (use aio)

[Kevin Wolf]

Stefan Weil schrieb:
>>> +/* Enable (currently) unsupported features (not implemented yet). */
>>> +//~ #define CONFIG_VDI_UNSUPPORTED
>>> +
>>> +/* Support non-standard block (cluster) size. */
>>> +//~ #define CONFIG_VDI_BLOCK_SIZE
>>>     
>> Actually, this is only about support for image creation. Any reason why
>> we shouldn't support creating images with non-standard block sizes? The
>> code already supports opening such images unconditionally, so the only
>> effect of turning it off for image creation is that we can't test that
>> functionality in qemu-iotests.
>>
>> [Oh, sorry, actually there is a check in open which I missed at first.
>> Any reason why we can't support it? But it's consistent at least.]
>>   
> 
> Multiples of 512 (SECTOR_SIZE) might work.
> 
> VirtualBox uses 1 MiB blocks, and I did not see options to create images
> with different block sizes. Maybe they even don't support such images.
> So I did not spend the time to test other block sizes.
> Why implement things nobody needs?

Ok, that makes sense. Probably we should remove the #define completely
then. I mean, why creating images that nobody - not even we ourselves -
can read?

>>   
>>> +/* Support static (pre-allocated) images. */
>>> +#define CONFIG_VDI_STATIC_IMAGE
>>> +
>>> +/* Command line option for static images. */
>>> +#define BLOCK_OPT_STATIC "static"
>>>     
>> What about calling it "preallocate" and moving it to block_int.h? I
>> think this could make sense for other drivers, too.
>>   
> 
> Yes, this would be reasonable if we had more drivers with support
> for "preallocate".
> 
> The VDI documentation calls these images "static", and they prefer
> dynamic images, so this static option is not really very important.

I might consider implementing it for qcow2. Cluster allocation is the
really slow part, so having complete L1/L2 tables in place from the very
beginning could speed up things.

Though I guess that for static images typically not only metadata is
preallocated, but zeros are written for the whole disk content? Maybe we
could implement a three-way flag like preallocate=[no,metadata,data] and
let qemu-img handle the data part (writing zeros is the same for all
formats and would even work with raw).

>>> +static int vdi_make_empty(BlockDriverState *bs)
>>> +{
>>> +    /* TODO: missing code. */
>>> +    logout("\n");
>>> +    return 0;
>>> +}
>>>     
>> If you don't implement it, leave it out. Setting
>> bdrv_vdi.bdrv_make_empty != NULL means that you claim to have that
>> functionality.
>>
>>   
> 
> I did not analyse what *_make_empty is supposed to do.
> This is one of the details were hints of the block driver experts
> would be helpful.

It's used after committing to a backing file. qcow1 seems to be the only
format actually implementing it. It complete clears the L1/L2 tables (=
the block map for VDI) so that all accesses go to the backing file again
and it can shrink the image file.

>>> +
>>> +#if defined(CONFIG_AIO)
>>> +
>>> +#if 0
>>>     
>> I guess you should remove this block before the patch is included.
>>   
> 
> This is also one of the details were hints of the block driver experts
> would be helpful as I did not understand this aio_remove / aio_cancel
> mechanism.

I wouldn't consider myself an AIO expert and I don't want to tell you
something wrong, so maybe Christoph would be the right one here?

>> Does VDI support compression even theoretically?
>>   
> 
> I think it would be possible to extend the specification
> to support compression or encryption.
> 
> The official specification (as far as I know it) does not
> support compression (nor encryption).

Then remove the entry. The function vdi_write_compressed doesn't exist
and doesn't even make sense with the current specification. The same
applies for vdi_set_key.

Kevin