Re: [Qemu-devel] [PATCH v3 2/2] virtio-blk: Use bdrv_aio_multiwrite

[Christoph Hellwig]

On Wed, Sep 09, 2009 at 05:53:38PM +0200, Kevin Wolf wrote:
> +static void do_multiwrite(BlockDriverState *bs, BlockRequest *blkreq,
> +    int num_writes)
>  {
> -    BlockDriverAIOCB *acb;
> +    int i, ret;
> +    ret = bdrv_aio_multiwrite(bs, blkreq, num_writes);
> +
> +    if (ret != 0) {
> +        for (i = 0; i < num_writes; i++) {
> +            if (blkreq[i].error) {
> +                virtio_blk_req_complete(blkreq[i].opaque, 
> VIRTIO_BLK_S_IOERR);
> +            }
> +        }
> +    }
> +}
>  
> +static void virtio_blk_handle_write(BlockRequest *blkreq, int *num_writes,
> +    VirtIOBlockReq *req, BlockDriverState **old_bs)
> +{
> +    if (req->dev->bs != *old_bs || *num_writes == 32) {

I was under the impression we had one queue per device, so the first
condition should never happen.

> +        if (*old_bs != NULL) {
> +            do_multiwrite(*old_bs, blkreq, *num_writes);
> +        }
> +        *num_writes = 0;
> +        *old_bs = req->dev->bs;
>      }
> +
> +    blkreq[*num_writes].sector = req->out->sector;
> +    blkreq[*num_writes].nb_sectors = req->qiov.size / 512;
> +    blkreq[*num_writes].qiov = &req->qiov;
> +    blkreq[*num_writes].cb = virtio_blk_rw_complete;
> +    blkreq[*num_writes].opaque = req;
> +    blkreq[*num_writes].error = 0;
> +
> +    (*num_writes)++;

If you pass the completion routine to the function and map the error case
to calling completion routine (which is the usual way to handle errors
anyway) this function could become copletely generic.

I think we also need to actually store the iocb in the BlockRequest
ide and scsi use it to cancel I/O on migration (why virtio
doesn't is on my TODO list to investigate) or some other cases.

Another improvement to the data structure would be to have a container
structure containing the BlockRequest array and num_writes, at which
point this actually becomes a pretty clean abstraction, which we
could also use to submit multiple iocbs in the native AIO code.

Any chance to just use this batches subsmission unconditionally and
also for reads?  I'd hate to grow even more confusing I/O methods
in the block later.