From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:45512) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1S2MiK-0000tl-JB for qemu-devel@nongnu.org; Tue, 28 Feb 2012 08:03:42 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1S2MiF-0003m1-0Z for qemu-devel@nongnu.org; Tue, 28 Feb 2012 08:03:32 -0500 Received: from mail-pz0-f45.google.com ([209.85.210.45]:47043) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1S2MiE-0003lj-O9 for qemu-devel@nongnu.org; Tue, 28 Feb 2012 08:03:26 -0500 Received: by dadp14 with SMTP id p14so7712559dad.4 for ; Tue, 28 Feb 2012 05:03:24 -0800 (PST) Sender: Paolo Bonzini Message-ID: <4F4CD096.5000309@redhat.com> Date: Tue, 28 Feb 2012 14:03:18 +0100 From: Paolo Bonzini MIME-Version: 1.0 References: <20120228111424.2DC63195@gandalf.tls.msk.ru> <4F4CBBE5.8000707@redhat.com> <4F4CCA17.7080802@msgid.tls.msk.ru> In-Reply-To: <4F4CCA17.7080802@msgid.tls.msk.ru> Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2] Consolidate reads and writes in nbd block device into one common routine List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Michael Tokarev Cc: qemu-devel@nongnu.org Il 28/02/2012 13:35, Michael Tokarev ha scritto: > On 28.02.2012 15:35, Paolo Bonzini wrote: >> Il 28/02/2012 11:24, Michael Tokarev ha scritto: >>> This removes quite some duplicated code. > [] >>> +static int nbd_co_rwv(BlockDriverState *bs, int64_t sector_num, >>> + int nb_sectors, QEMUIOVector *qiov, int iswrite) >> >> Call this nbd_co_rw, and please pass the whole request.type down. > > Originally it is readV and writeV, so why it should not be rwV ? It's more consistent with block.c. > By passing whole request.type (NBD_CMD_WRITE or NBD_CMD_WRITE|NBD_CMD_FLAG_FUA > or NBD_CMD_READ) the condition (iswrite currently) will be larger (request.type > != NBD_CMD_READ). Also, if someday we'll have additional flag for READ as we > already do for write, whole thing will be even more difficult to read. Sure, but why should a generic function deal with NBD_CMD_FLAG_FUA? >> >>> { >>> BDRVNBDState *s = bs->opaque; >>> struct nbd_request request; >>> struct nbd_reply reply; >>> + int offset = 0; >>> >>> - request.type = NBD_CMD_WRITE; >>> - if (!bdrv_enable_write_cache(bs) && (s->nbdflags & NBD_FLAG_SEND_FUA)) { >>> + request.type = iswrite ? NBD_CMD_WRITE : NBD_CMD_READ; >>> + if (iswrite && !bdrv_enable_write_cache(bs) && (s->nbdflags & NBD_FLAG_SEND_FUA)) { >>> request.type |= NBD_CMD_FLAG_FUA; >>> } >>> + reply.error = 0; >>> + >>> + /* we split the request into pieces of at most NBD_MAX_SECTORS size >>> + * and process them in a loop... */ >>> + do { >>> + request.from = sector_num * 512; >>> + request.len = MIN(nb_sectors, NBD_MAX_SECTORS) * 512; >>> + >>> + nbd_coroutine_start(s, &request); >>> + if (nbd_co_send_request(s, &request, iswrite ? qiov->iov : NULL, 0) == -1) { >> >> The last 0 needs to be offset. > > Indeed, this is a bug. I think I tested it with large requests > but it looks like only for reads. > >> ... but thinking more about it, why don't you leave >> nbd_co_readv_1/nbd_co_writev_1 alone, and create a nbd_split_rw function >> that takes a function pointer? > > Because each of these nbd_co_*_1 does the same thing, the diff. is > only quiv->iov vs NULL. While reading the original code it was the > first thing I did - consolidated nbd_co_*_1 into nbd_co_* ;) And offset. I needed to check that non-0 offsets are fine when the iov is NULL; it's not obvious. > Actually it might be a good idea to have single bdrv->bdrv_co_readwritev > method instead of two -- the path of each read and write jumps between > specific read-or-write routine and common readwrite routine several > times. Small amounts of duplicate code can be better than functions with many ifs or complicated conditions. > I see only one correction which needs (really!) to be done - that's > fixing the bug with offset. Do you still not agree? I still disagree. :) I will accept the patch with the function pointer though. Paolo