From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57165) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b8uc8-0003St-6d for qemu-devel@nongnu.org; Fri, 03 Jun 2016 15:18:37 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1b8uc6-00084N-K1 for qemu-devel@nongnu.org; Fri, 03 Jun 2016 15:18:36 -0400 References: <1464974478-23598-1-git-send-email-kwolf@redhat.com> <1464974478-23598-3-git-send-email-kwolf@redhat.com> From: Eric Blake Message-ID: <5751D7FF.8030504@redhat.com> Date: Fri, 3 Jun 2016 13:18:23 -0600 MIME-Version: 1.0 In-Reply-To: <1464974478-23598-3-git-send-email-kwolf@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="mTe9PttbtdTkPBP3HJjnnxIqFrTGFmlIm" Subject: Re: [Qemu-devel] [PATCH 2/5] qcow2: Implement .bdrv_co_preadv() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf , qemu-block@nongnu.org Cc: jsnow@redhat.com, qemu-devel@nongnu.org, mreitz@redhat.com This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --mTe9PttbtdTkPBP3HJjnnxIqFrTGFmlIm Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 06/03/2016 11:21 AM, Kevin Wolf wrote: > Reading from qcow2 images is now byte granularity. >=20 > Most of the affected code in qcow2 actually gets simpler with this > change. The only exception is encryption, which is fixed on 512 bytes > blocks; in order to keep this working, bs->request_alignment is set for= > encrypted images. >=20 > Signed-off-by: Kevin Wolf > --- > block/qcow2-cluster.c | 18 ++++----- > block/qcow2.c | 108 +++++++++++++++++++++++++++---------------= -------- > block/qcow2.h | 2 +- > 3 files changed, 67 insertions(+), 61 deletions(-) >=20 > @@ -467,16 +468,16 @@ out: > * For a given offset of the disk image, find the cluster offset in > * qcow2 file. The offset is stored in *cluster_offset. > * > - * on entry, *num is the number of contiguous sectors we'd like to > + * on entry, *bytes is the number of contiguous bytes we'd like to maybe s/number/maximum number/ > * access following offset. > * > - * on exit, *num is the number of contiguous sectors we can read. > + * on exit, *bytes is the number of contiguous bytes we can read. maybe s/we can read/with the same cluster type/ > * > * Returns the cluster type (QCOW2_CLUSTER_*) on success, -errno in er= ror > * cases. > */ > int qcow2_get_cluster_offset(BlockDriverState *bs, uint64_t offset, > - int *num, uint64_t *cluster_offset) > + unsigned int *bytes, uint64_t *cluster_of= fset) > { > BDRVQcow2State *s =3D bs->opaque; > unsigned int l2_index; > @@ -485,12 +486,9 @@ int qcow2_get_cluster_offset(BlockDriverState *bs,= uint64_t offset, > unsigned int offset_in_cluster, nb_clusters; > uint64_t bytes_available, bytes_needed; > int ret; > - unsigned int bytes; > - > - bytes =3D *num * BDRV_SECTOR_SIZE; One potential overflow gone... > =20 > offset_in_cluster =3D offset_into_cluster(s, offset); > - bytes_needed =3D bytes + offset_in_cluster; > + bytes_needed =3D *bytes + offset_in_cluster; =2E..but not the other. Looks like your callers limit their input 'bytes= ' to at most INT_MAX, and therefore it happens to not overflow unsigned int in practice, but you may want an assertion? > +++ b/block/qcow2.c > @@ -975,6 +975,9 @@ static int qcow2_open(BlockDriverState *bs, QDict *= options, int flags, > } > =20 > bs->encrypted =3D 1; > + > + /* Encryption works on a sector granularity */ > + bs->request_alignment =3D BDRV_SECTOR_SIZE; Trivial conflict with my patch 5/5 that moves request_alignment into BlockLimits (if we even want that, since I still have to find why my patch makes qemu-iotests 77 hang) > =20 > -static coroutine_fn int qcow2_co_readv(BlockDriverState *bs, int64_t s= ector_num, > - int remaining_sectors, QEMUIOVector *qiov) > +static coroutine_fn int qcow2_co_preadv(BlockDriverState *bs, uint64_t= offset, > + uint64_t bytes, QEMUIOVector *= qiov, > + int flags) Wait a minute. .bdrv_co_preadv() takes uint64_t bytes, while bdrv_co_preadv() takes only unsigned int bytes? Eww. We've got some more scrubbing work to do. At least it is going to get easier to universally turn on full 64-bit byte interfaces everywhere, especially once my patches for auto-fragmenting at max_transfer_length land (which in turn won't be posted before your conversion of bdrv_aligned_preadv() to a byte interface). So no impact to this patch. > { > BDRVQcow2State *s =3D bs->opaque; > - int index_in_cluster, n1; > + int offset_in_cluster, n1; > int ret; > - int cur_nr_sectors; /* number of sectors in current iteration */ > + unsigned int cur_bytes; /* number of sectors in current iteration = */ comment is stale now > uint64_t cluster_offset =3D 0; > uint64_t bytes_done =3D 0; > QEMUIOVector hd_qiov; > @@ -1389,26 +1402,24 @@ static coroutine_fn int qcow2_co_readv(BlockDri= verState *bs, int64_t sector_num, > =20 > qemu_co_mutex_lock(&s->lock); > =20 > - while (remaining_sectors !=3D 0) { > + while (bytes !=3D 0) { > =20 > /* prepare next request */ > - cur_nr_sectors =3D remaining_sectors; > + cur_bytes =3D MIN(bytes, INT_MAX); > if (s->cipher) { > - cur_nr_sectors =3D MIN(cur_nr_sectors, > - QCOW_MAX_CRYPT_CLUSTERS * s->cluster_sectors); > + cur_bytes =3D MIN(cur_bytes, > + QCOW_MAX_CRYPT_CLUSTERS * s->cluster_size)= ; Again, my work on auto-fragmenting at the block layer should make it so that we can eventually further simplify this part to just assert that bytes doesn't exceed max_transfer_length, rather than having to fragment it at INT_MAX ourselves. Couple of tweaks to fix as pointed out above, but mostly looks sane. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --mTe9PttbtdTkPBP3HJjnnxIqFrTGFmlIm 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/ iQEcBAEBCAAGBQJXUdf/AAoJEKeha0olJ0NqHbkH/25PrzcW3Zt6dwT8qB9UlEwZ vpuhTn+KA7XHU5NrnPXNj0oSXUjmjcHXgN9KDVvYeord8E2uKwG1snnfb5uaoIWo oA4ZuAAolP66o1Cw6U1HsmU5/2aC91oCZgoVfBO9Olui5hnkN0c2uxcLQp9fiUzH zDfXxRjcY97KSwK6cIZXCnUN03PTExnUiJonLBFDl6TZQwz8LtoocBjsnIQHUbma X7eOSiGjdyc+YnlIoiPXZtBU0Wy1OVjMKmjoKTzijAky2v6W2K9A+wtTBw1ElYYL 6sVD/QAKPRgRaQRc7wPju0tfXS/ZrUBL+r+8jTznbcp6dnln6Z0UbHV8zGZAUjo= =SjLh -----END PGP SIGNATURE----- --mTe9PttbtdTkPBP3HJjnnxIqFrTGFmlIm--