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

[Kevin Wolf]

Am 27.04.2011 15:27, schrieb Stefan Hajnoczi:
> From: Anthony Liguori <address@hidden>
> 
> Signed-off-by: Anthony Liguori <address@hidden>
> ---
>  block.c     |   32 ++++++++++++++++++++++++++++++++
>  block.h     |    2 ++
>  block_int.h |    3 +++
>  3 files changed, 37 insertions(+), 0 deletions(-)
> 
> diff --git a/block.c b/block.c
> index f731c7a..5e3476c 100644
> --- a/block.c
> +++ b/block.c
> @@ -2248,6 +2248,38 @@ BlockDriverAIOCB *bdrv_aio_writev(BlockDriverState 
> *bs, int64_t sector_num,
>      return ret;
>  }
>  
> +/**
> + * 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.

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.

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.

Kevin