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

[Stefan Hajnoczi <stefanha@gmail.com>]

On Fri, May 6, 2011 at 2:36 PM, Kevin Wolf <kwolf@redhat.com> wrote:
> Am 06.05.2011 15:21, schrieb Stefan Hajnoczi:
>> On Fri, Apr 29, 2011 at 12:56 PM, Kevin Wolf <kwolf@redhat.com> 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 guess you're right.  First I wasn't planning on passing nb_sectors
to this function since its the blockdev.c streaming loop that drives
the streaming process - we may not need nb_sectors here.  But I guess
this is like a read(2) function and the block driver can return short
reads if that is convenient due to cluster sizes or other image format
internals.  By passing in nb_sectors we avoid streaming too much at
the end.

Stefan