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

[Kevin Wolf]

Am 11.09.2009 00:41, schrieb 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.

Maybe, don't know? ;-) Makes perfect sense, so probably you're right. I
was just trying to be better safe than sorry. I can take it out if you
prefer.

>> +        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.

Except that VirtIOBlockReq doesn't seem to be a type commonly used in
generic code.

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

Right, this is something that is still missing. Considering that virtio
as the only multiwrite user doesn't use it (yet) anyway and that it's
not completely trivial (the request to be cancelled could have been
merged), I would prefer to introduce this in a patch on top. The
bdrv_aio_multiwrite patch is already larger than I had liked it to be.

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

Ok, I'm not opposed to this.

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

If we want to completely obsolete bdrv_aio_readv/writev by batch
submission functions (not only in block.c but also in each block
driver), we certainly can do that. I think this would make a lot of
sense, but it's quite some work and definitely out of scope for this
patch which is basically meant to be a qcow2 performance fix.

Kevin