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

[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.  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?

> I'm not sure how the caller would know how many sectors have been
> copied. A BlockDriverCompletionFunc usually returns 0 on success, did
> you change it here to use positive numbers for something else? At least
> this must be documented somewhere, but I would suggest to add a
> nb_sectors argument instead so that the caller decides how many sectors
> to copy.

Yes, I agree that a separate nb_sectors argument would be clearer.

> If you say that it only makes sense with copy on read, should one think
> of it as a read that throws the read data away? I think considering it a
> copy function makes more sense and is independent of copy on read.

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.

Stefan