From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1Mm0IG-00018m-7I for qemu-devel@nongnu.org; Fri, 11 Sep 2009 03:11:40 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1Mm0IB-00016W-Iy for qemu-devel@nongnu.org; Fri, 11 Sep 2009 03:11:39 -0400 Received: from [199.232.76.173] (port=36373 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Mm0IB-00016Q-Az for qemu-devel@nongnu.org; Fri, 11 Sep 2009 03:11:35 -0400 Received: from mx20.gnu.org ([199.232.41.8]:64004) by monty-python.gnu.org with esmtps (TLS-1.0:RSA_AES_256_CBC_SHA1:32) (Exim 4.60) (envelope-from ) id 1Mm0IA-000682-Rg for qemu-devel@nongnu.org; Fri, 11 Sep 2009 03:11:35 -0400 Received: from mx1.redhat.com ([209.132.183.28]) by mx20.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1Mm0IA-0000yn-35 for qemu-devel@nongnu.org; Fri, 11 Sep 2009 03:11:34 -0400 Message-ID: <4AA9F7DC.1050608@redhat.com> Date: Fri, 11 Sep 2009 09:10:20 +0200 From: Kevin Wolf MIME-Version: 1.0 Subject: Re: [Qemu-devel] [PATCH v3 2/2] virtio-blk: Use bdrv_aio_multiwrite References: <1252511618-19497-1-git-send-email-kwolf@redhat.com> <1252511618-19497-3-git-send-email-kwolf@redhat.com> <20090910224141.GA25700@lst.de> In-Reply-To: <20090910224141.GA25700@lst.de> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Christoph Hellwig Cc: qemu-devel@nongnu.org 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