From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47895) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ayh4v-0003Ix-C4 for qemu-devel@nongnu.org; Fri, 06 May 2016 10:50:12 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ayh4j-00041m-4N for qemu-devel@nongnu.org; Fri, 06 May 2016 10:49:59 -0400 Date: Fri, 6 May 2016 16:49:15 +0200 From: Kevin Wolf Message-ID: <20160506144915.GL5093@noname.redhat.com> References: <1462406126-22946-1-git-send-email-eblake@redhat.com> <1462406126-22946-8-git-send-email-eblake@redhat.com> <20160506125041.GF5093@noname.redhat.com> <572CA7A4.5000402@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="ALfTUftag+2gvp1h" Content-Disposition: inline In-Reply-To: <572CA7A4.5000402@redhat.com> Subject: Re: [Qemu-devel] [PATCH v6 07/20] scsi-disk: Switch to byte-based aio block access List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: qemu-devel@nongnu.org, qemu-block@nongnu.org, Paolo Bonzini --ALfTUftag+2gvp1h Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Am 06.05.2016 um 16:18 hat Eric Blake geschrieben: > On 05/06/2016 06:50 AM, Kevin Wolf wrote: > > Am 05.05.2016 um 01:55 hat Eric Blake geschrieben: > >> @@ -1730,13 +1730,11 @@ static void scsi_write_same_complete(void *opa= que, int ret) > >> if (data->iov.iov_len) { > >> block_acct_start(blk_get_stats(s->qdev.conf.blk), &r->acct, > >> data->iov.iov_len, BLOCK_ACCT_WRITE); > >> - /* blk_aio_write doesn't like the qiov size being different f= rom > >> - * nb_sectors, make sure they match. > >> - */ > >=20 > > Wouldn't it be better to update the comment instead of deleting it? If I > > understand correctly, this additional qemu_iovec_init_external() is for > > the last part of an unaligned WRITE SAME request, where the qiov can > > become shorter than in the previous iterations. >=20 > The comment mattered when you were passing two sizes to blk_aio_writev > (one embedded in data->qiov, and one in terms of sectors as a direct > argument); the block layer asserted the two were equal. But now that we > are handling bytes, we pass in a single size (that of data->qiov), so > the comment no longer makes sense. Yes, with the current text it doesn't make sense any more. But it explains why we have another qemu_iovec_init_external() here, and as that call remains, it would still be good to have an explanation. Or maybe it's obvious, don't know... > >=20 > >> qemu_iovec_init_external(&data->qiov, &data->iov, 1); > >> - r->req.aiocb =3D blk_aio_writev(s->qdev.conf.blk, data->secto= r, > >> - &data->qiov, data->iov.iov_len = / 512, > >> - scsi_write_same_complete, data); > >> + r->req.aiocb =3D blk_aio_pwritev(s->qdev.conf.blk, > >> + data->sector << BDRV_SECTOR_BI= TS, > >> + &data->qiov, 0, >=20 > It caught me more than once while writing the patch that my new function > replaced nb_sectors (the duplicated size) by flags, rather than adding a > parameter (as it was harder to get the compiler to gripe about > incomplete conversions, such as forgetting to s/write/pwrite/). But by > the end of the series, when the old interface is gone, everything still > compiles under the new name, so at least we're assured that the series > worked on that front. Yes, I was a bit worried that I might miss possible cases where you still pass bytes instead of flags when the compiler can't complain about them. I didn't see any, so hopefully that means it's good. Kevin --ALfTUftag+2gvp1h Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.22 (GNU/Linux) iQIcBAEBAgAGBQJXLK7rAAoJEH8JsnLIjy/WTdIQAKfOEKuTR27jucCShTEp95hy 67DFClyl3N3DX7Nr/HYLblK4hYyzSp+30gtBSoFB6nNyw7dGAwxZ2MTSRxyLJInF VRR9SMivR4pu9gyRowhlKFvbwoo26I0zqgeEp9A2ZiXyqY8ElMqExpD/pOKXqJjA r7Ynd53fwSFp/A9l7p4ivTv/VK238R4hTKAkkOqwpUsJxEJXgoI8fuvU9i4h5ACr X0IQPkjkrGd3J/26w/r3WlkBucYB7iPzwSfoGixf38I1WVxXh9cRtEaKEM3vVdkt tQQKyh7/e+AHpRPzq3ATabpX4bJIPvmDYaUte4rTboNo+lysWsunLRk1oIvy5F7s t2rcuaC0iX52DAAHMk5n+XDdiv2sEgx6vN+eC28IMd81USxUFs1VUab1b5Jl5sS2 BczcpbNgJRlvWSvLOIrgblES+kTgBhpeetqnmF5WwaLMdtvjsgSaCqb7y3VPoaqB STbcQAI24MjLIevmtQfAuoDpPEjfk8mgLMRkIqEymq5SooRV6FLFfpBRwepAOwlj 9H/5Rknx3pm0Ybfrm9Vzsx22bnfxuI/rHB+0h1jaTC5UKq9yXv/AGl8gdn8Tv/uY CRlhubIhFt7wPml/A6LpVgqBJwVBfkpNFSab0aFoUkKyc0PHsuk/+lMDI9aXvG0Q 6Q8rhYmHk8Aq4E71yIcP =sWMf -----END PGP SIGNATURE----- --ALfTUftag+2gvp1h--