From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49461) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bAfZV-00056A-Ny for qemu-devel@nongnu.org; Wed, 08 Jun 2016 11:39:12 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bAfZO-0007CN-Is for qemu-devel@nongnu.org; Wed, 08 Jun 2016 11:39:08 -0400 References: <1465395011-26088-1-git-send-email-kwolf@redhat.com> <1465395011-26088-6-git-send-email-kwolf@redhat.com> From: Eric Blake Message-ID: <57583C0E.4070409@redhat.com> Date: Wed, 8 Jun 2016 09:38:54 -0600 MIME-Version: 1.0 In-Reply-To: <1465395011-26088-6-git-send-email-kwolf@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="gBsJORrwBHOAkp7U9WgMxwRxvvrmDk0Td" Subject: Re: [Qemu-devel] [PATCH 5/6] raw-posix: Implement .bdrv_co_preadv/pwritev List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf , qemu-block@nongnu.org Cc: pbonzini@redhat.com, mreitz@redhat.com, stefanha@redhat.com, famz@redhat.com, qemu-devel@nongnu.org This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --gBsJORrwBHOAkp7U9WgMxwRxvvrmDk0Td Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 06/08/2016 08:10 AM, Kevin Wolf wrote: > The raw-posix block driver actually supports byte-aligned requests now > on non-O_DIRECT images, like it already (and previously incorrectly) > claimed in bs->request_alignment. >=20 > For some block drivers this means that a RMW cycle can be avoided when > they write sub-sector metadata e.g. for cluster allocation. [well, there's still probably a RMW going on, but it's being done by the kernel, rather than qemu - and choice of caching may let the kernel optimize things... not worth cluttering the commit message with this, though] >=20 > Signed-off-by: Kevin Wolf > --- > +++ b/block/linux-aio.c > @@ -272,14 +272,12 @@ static int laio_do_submit(int fd, struct qemu_lai= ocb *laiocb, off_t offset, > } > =20 > int laio_submit_co(BlockDriverState *bs, LinuxAioState *s, int fd, > - int64_t sector_num, QEMUIOVector *qiov, int nb_sectors, int ty= pe) > + uint64_t offset, QEMUIOVector *qiov, int type) > { > - off_t offset =3D sector_num * 512; > int ret; > - > struct qemu_laiocb laiocb =3D { > .co =3D qemu_coroutine_self(), > - .nbytes =3D nb_sectors * 512, > + .nbytes =3D qiov->size, So for this interface, we require non-NULL qiov and no duplicated length; I guess it isn't used for write_zeroes. We may still want to do some consistency sweep to decide what level of NULL-ness we want for representing write_zeroes, rather than ad hoc decisions at each layer of the call stack, but that's a task for another day. > @@ -1344,26 +1344,27 @@ static int coroutine_fn raw_co_rw(BlockDriverSt= ate *bs, int64_t sector_num, > type |=3D QEMU_AIO_MISALIGNED; > #ifdef CONFIG_LINUX_AIO > } else if (s->use_aio) { > - return laio_submit_co(bs, s->aio_ctx, s->fd, sector_num, q= iov, > - nb_sectors, type); > + assert(qiov->size =3D=3D bytes); Worth hoisting the assertion outside of the #ifdef?... > + return laio_submit_co(bs, s->aio_ctx, s->fd, offset, qiov,= type); > #endif > } > } > =20 > - return paio_submit_co(bs, s->fd, sector_num * BDRV_SECTOR_SIZE, qi= ov, > - nb_sectors * BDRV_SECTOR_SIZE, type); > + return paio_submit_co(bs, s->fd, offset, qiov, bytes, type); =2E..then again, paio_submit_co() also does the assert - and this is more= evidence of our inconsistency on whether we duplicate a separate bytes parameter or reuse qiov->size. > =20 > -static int coroutine_fn raw_co_readv(BlockDriverState *bs, int64_t sec= tor_num, > - int nb_sectors, QEMUIOVector *qio= v) > +static int coroutine_fn raw_co_preadv(BlockDriverState *bs, uint64_t o= ffset, > + uint64_t bytes, QEMUIOVector *qi= ov, > + int flags) > { > - return raw_co_rw(bs, sector_num, nb_sectors, qiov, QEMU_AIO_READ);= > + return raw_co_prw(bs, offset, bytes, qiov, QEMU_AIO_READ); We ignore flags, but that's not a change in semantics. (Maybe someday we need .supported_read_flags) > } > =20 > -static int coroutine_fn raw_co_writev(BlockDriverState *bs, int64_t se= ctor_num, > - int nb_sectors, QEMUIOVector *qi= ov) > +static int coroutine_fn raw_co_pwritev(BlockDriverState *bs, uint64_t = offset, > + uint64_t bytes, QEMUIOVector *q= iov, > + int flags) > { > - return raw_co_rw(bs, sector_num, nb_sectors, qiov, QEMU_AIO_WRITE)= ; > + return raw_co_prw(bs, offset, bytes, qiov, QEMU_AIO_WRITE); And here, we could assert(!flags) (since we intentionally don't set =2Esupported_write_flags) - but I won't insist. None of my comments require a code change, other than a possible added assertion, so: Reviewed-by: Eric Blake --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --gBsJORrwBHOAkp7U9WgMxwRxvvrmDk0Td Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBCAAGBQJXWDwOAAoJEKeha0olJ0NqM+kH/2mQZDhdO3+O6rdoXyP8QAhm BbUcfDfXuIvJvAA+njRbNAkpBTEu7DWCGV/Olz51FIrOArxSpL4SThTvdObM+UBW zIvQTTY7Lx3DqiDD7QkVekcY79d62El6RvRVfqG2tvvpIbeOJrABczRxxTTWVRkh GpXsJk5lpwuE38EMjWtgZ5oFGVgRzkXOT8WL8neoCVpRLwoD2jJ4HUPlfe9qP0hX SClnlBbjqSIRQ9eeqVq4sQHgeQs1pYdDsB3hCgCLYyCbLD/6UKiK3OwHGktfZLDd 4JSJW78Gu9SFayuaMqEqroaZE0afYpb1GLa9B7Bwkxo6imgSzVHGk0BWFuDIXss= =3z9c -----END PGP SIGNATURE----- --gBsJORrwBHOAkp7U9WgMxwRxvvrmDk0Td--