Re: [Qemu-devel] [PATCH 1/8] block: add bdrv_aio_stream

[Kevin Wolf]

Am 06.05.2011 15:21, schrieb Stefan Hajnoczi:
> On Fri, Apr 29, 2011 at 12:56 PM, Kevin Wolf <address@hidden> wrote:
>> Am 27.04.2011 15:27, schrieb Stefan Hajnoczi:
>>> +/**
>>> + * Attempt to stream an image starting from sector_num.
>>> + *
>>> + * @sector_num - the first sector to start streaming from
>>> + * @cb - block completion callback
>>> + * @opaque - data to pass completion callback
>>> + *
>>> + * Returns NULL if the image format not support streaming, the image is
>>> + * read-only, or no image is open.
>>> + *
>>> + * The intention of this function is for a user to execute it once with a
>>> + * sector_num of 0 and then upon receiving a completion callback, to 
>>> remember
>>> + * the number of sectors "streamed", and then to call this function again 
>>> with
>>> + * an offset adjusted by the number of sectors previously streamed.
>>> + *
>>> + * This allows a user to progressive stream in an image at a pace that 
>>> makes
>>> + * sense.  In general, this function tries to do the smallest amount of I/O
>>> + * possible to do some useful work.
>>> + *
>>> + * This function only really makes sense in combination with a block format
>>> + * that supports copy on read and has it enabled.  If copy on read is not
>>> + * enabled, a block format driver may return NULL.
>>> + */
>>> +BlockDriverAIOCB *bdrv_aio_stream(BlockDriverState *bs, int64_t sector_num,
>>> +                                  BlockDriverCompletionFunc *cb, void 
>>> *opaque)
>>
>> I think bdrv_aio_stream is a bad name for this. It only becomes image
>> streaming because the caller repeatedly calls this function. What the
>> function really does is copying some data from the backing file into the
>> overlay image.
> 
> That's true but bdrv_aio_copy_from_backing_file() is a bit long. 

bdrv_copy_backing() or something should be short enough and still
describes what it's really doing.

> The
> special thing about this operation is that it takes a starting
> sector_num but no length.  The callback receives the nb_sectors.  So
> this operation isn't an ordinary [start, length) copy either so
> bdrv_aio_stream() isn't that bad?

Well, you're going to introduce nb_sectors anyway, so it's not really
special any more.

> I actually think the copy-on-read statement is an implementation
> detail.  I can imagine doing essentially the same behavior without
> exposing copy on read to the user.  But in QED streaming is based on
> copy-on-read.  Let's remove this comment.

Ok. Removing the comment and calling it something with "copy" in the
name should make clear what the intention is.

Kevin