[Qemu-devel] Re: [RFC PATCH v3 1/4] block: Implement bdrv_aio_pwrite

[Stefan Hajnoczi]

On Tue, Nov 30, 2010 at 12:48 PM, Kevin Wolf <address@hidden> wrote:
> This implements an asynchronous version of bdrv_pwrite.
>
> Signed-off-by: Kevin Wolf <address@hidden>
> ---
>  block.c |  167 
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  block.h |    2 +
>  2 files changed, 169 insertions(+), 0 deletions(-)

Is this function is necessary?

Current synchronous code uses pwrite() so this function makes it easy
to convert existing code.  But if that code took the block-based
nature of storage into account then this read-modify-write helper
isn't needed.

I guess what I'm saying is that this function should only be used when
you really need rmw (in many cases with image metadata it can be
avoided because you have enough metadata cached in memory to do full
sector writes).  If it turns out we don't need rmw then we can
eliminate this function.

> +    switch (acb->state) {
> +    case 0: {
> +        /* Read first sector if needed */

Please use an enum instead of int literals with comments.  Or you
could try separate functions and see if the switch statement really
saves that many lines of code.

> +    case 3: {
> +        /* Read last sector if needed */
> +        if (acb->bytes == 0) {
> +            goto done;
> +        }
> +
> +        acb->state = 4;
> +        acb->iov.iov_base = acb->tmp_buf;

acb->tmp_buf may be NULL here if we took the state transition to 2
instead of doing 1.

> +done:
> +    qemu_free(acb->tmp_buf);
> +    acb->common.cb(acb->common.opaque, ret);

Callback not invoked from a BH.  In an error case we might have made
no blocking calls, i.e. never returned and this callback can cause
reentrancy.

> +BlockDriverAIOCB *bdrv_aio_pwrite(BlockDriverState *bs, int64_t offset,
> +    void* buf, size_t bytes, BlockDriverCompletionFunc *cb, void *opaque)
> +{
> +    PwriteAIOCB *acb;
> +
> +    acb = qemu_aio_get(&blkqueue_aio_pool, bs, cb, opaque);
> +    acb->state      = 0;
> +    acb->offset     = offset;
> +    acb->buf        = buf;
> +    acb->bytes      = bytes;
> +    acb->tmp_buf    = NULL;
> +
> +    bdrv_aio_pwrite_cb(acb, 0);

We're missing the usual !bs->drv, bs->read_only, bdrv_check_request()
checks here.  Are we okay to wait until calling
bdrv_aio_readv/bdrv_aio_writev for these checks?

Stefan