From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:38685) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b9vrP-0002Oh-2I for qemu-devel@nongnu.org; Mon, 06 Jun 2016 10:50:36 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1b9vrK-0001CJ-ID for qemu-devel@nongnu.org; Mon, 06 Jun 2016 10:50:34 -0400 Date: Mon, 6 Jun 2016 16:50:17 +0200 From: Kevin Wolf Message-ID: <20160606145017.GB4885@noname.redhat.com> References: <1464974478-23598-1-git-send-email-kwolf@redhat.com> <1464974478-23598-6-git-send-email-kwolf@redhat.com> <5751E4CE.10806@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="gKMricLos+KVdGMg" Content-Disposition: inline In-Reply-To: <5751E4CE.10806@redhat.com> Subject: Re: [Qemu-devel] [PATCH 5/5] qcow2: Implement .bdrv_co_pwritev List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: qemu-block@nongnu.org, jsnow@redhat.com, qemu-devel@nongnu.org, mreitz@redhat.com --gKMricLos+KVdGMg Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Am 03.06.2016 um 22:13 hat Eric Blake geschrieben: > On 06/03/2016 11:21 AM, Kevin Wolf wrote: > > This changes qcow2 to implement the byte-based .bdrv_co_pwritev > > interface rather than the sector-based old one. > >=20 > > As preallocation uses the same allocation function as normal writes, and > > the interface of that function needs to be changed, it is converted in > > the same patch. > >=20 > > Signed-off-by: Kevin Wolf > > --- > > block/qcow2-cluster.c | 13 ++++---- > > block/qcow2.c | 82 ++++++++++++++++++++++++-------------------= -------- > > block/qcow2.h | 3 +- > > trace-events | 6 ++-- > > 4 files changed, 49 insertions(+), 55 deletions(-) > >=20 >=20 > > +++ b/block/qcow2-cluster.c > > @@ -1261,7 +1261,8 @@ fail: > > * Return 0 on success and -errno in error cases > > */ > > int qcow2_alloc_cluster_offset(BlockDriverState *bs, uint64_t offset, > > - int *num, uint64_t *host_offset, QCowL2Meta **m) > > + unsigned int *bytes, uint64_t *host_off= set, > > + QCowL2Meta **m) > > { >=20 > > =20 > > - *num -=3D remaining >> BDRV_SECTOR_BITS; > > - assert(*num > 0); > > + *bytes -=3D remaining; > > + assert(*bytes > 0); >=20 > The assertion is now weaker. Beforehand, num could go negative. But > bytes cannot, all it can do is go to 0. If we wrap around, the > assertion won't catch it. Do you want to strengthen it by also > asserting that bytes < INT_MAX? Is it useful to assert this, though? We have tons of such loops and we never assert that cur_bytes <=3D bytes (which is what you would be checking effectively) because it's quite obvious. This assertion was meant to document the less obvious fact that we always make some progress and never return 0 bytes. > > +++ b/block/qcow2.c > > @@ -1544,23 +1544,20 @@ fail: > > return ret; > > } > > =20 > > -static coroutine_fn int qcow2_co_writev(BlockDriverState *bs, > > - int64_t sector_num, > > - int remaining_sectors, > > - QEMUIOVector *qiov) > > +static coroutine_fn int qcow2_co_pwritev(BlockDriverState *bs, > > + uint64_t offset, uint64_t bytes, QEMUIOVector *qiov, int flags) >=20 > Worth consistent indentation, while touching it? Will fix. > > + while (bytes !=3D 0) { > > =20 > > l2meta =3D NULL; > > =20 > > trace_qcow2_writev_start_part(qemu_coroutine_self()); > > - index_in_cluster =3D sector_num & (s->cluster_sectors - 1); > > - cur_nr_sectors =3D remaining_sectors; > > - if (bs->encrypted && > > - cur_nr_sectors > > > - QCOW_MAX_CRYPT_CLUSTERS * s->cluster_sectors - index_in_cl= uster) { > > - cur_nr_sectors =3D > > - QCOW_MAX_CRYPT_CLUSTERS * s->cluster_sectors - index_i= n_cluster; > > + offset_in_cluster =3D offset_into_cluster(s, offset); > > + cur_bytes =3D MIN(bytes, INT_MAX); > > + if (bs->encrypted) { > > + cur_bytes =3D MIN(cur_bytes, > > + QCOW_MAX_CRYPT_CLUSTERS * s->cluster_size > > + - offset_in_cluster); >=20 > Again, if the block layer learns to auto-fragment, then we should just > inform the block layer about QCOW_MAX_CRYPT_CLUSTERS as our max_transfer > limit. >=20 >=20 > > @@ -1634,10 +1628,10 @@ static coroutine_fn int qcow2_co_writev(BlockDr= iverState *bs, > > qemu_co_mutex_unlock(&s->lock); > > BLKDBG_EVENT(bs->file, BLKDBG_WRITE_AIO); > > trace_qcow2_writev_data(qemu_coroutine_self(), > > - (cluster_offset >> 9) + index_in_clust= er); > > - ret =3D bdrv_co_writev(bs->file->bs, > > - (cluster_offset >> 9) + index_in_cluster, > > - cur_nr_sectors, &hd_qiov); > > + cluster_offset + offset_in_cluster); > > + ret =3D bdrv_co_pwritev(bs->file->bs, > > + cluster_offset + offset_in_cluster, > > + cur_bytes, &hd_qiov, 0); >=20 > s/0/flags/, even if .supported_write_flags stays 0 for now. I don't agree. That might make sense for a passthrough driver like raw, but I don't see a reason to guess for qcow2 that passing flags down is the right thing to do. The only flag that we currently have (FUA) isn't correctly implemented by passing it to the lower layer because qcow2 metadata must be considered. So I would leave changing flags here for the patch that adds some flag to .supported_write_flags. > > @@ -2059,7 +2053,7 @@ static int preallocate(BlockDriverState *bs) > > uint8_t buf[BDRV_SECTOR_SIZE]; > > memset(buf, 0, BDRV_SECTOR_SIZE); > > ret =3D bdrv_write(bs->file->bs, > > - (host_offset >> BDRV_SECTOR_BITS) + num - 1, > > + ((host_offset + cur_bytes) >> BDRV_SECTOR_BIT= S) - 1, > > buf, 1); >=20 > Do we still have to call bdrv_write(), or can we convert this to a > byte-based interface call? Okay, I'll make it a one-byte bdrv_pwrite(). Kevin --gKMricLos+KVdGMg Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.22 (GNU/Linux) iQIcBAEBAgAGBQJXVY2pAAoJEH8JsnLIjy/WlxgQAIeod7m1s+ySLWk1DHfDV4P+ VFp/AyzWZ9ELOcXfDtc4oK4JC+HGYXNyWgHCA+pz7+PrEzyHEWKZGw/rfPL/KqYU TubQPz5I8VlLlHu2tvk7Hycdo0Z+/jBFEWXjdc3DQKHKhaicHlkLYYK2P1HGDYm4 sa/oYU83YT/vYmTkakzO4cw6aWzZeWoDwt4n5FuqB4VlvwvdmzavIk64lHnfdb7b c9JyY3Y4pfKOvxtk1p0YTETAk46grarizHj4/cgyWs+sA3UQtmoh2NKC9C0GpROx s5hCMeXjhiHWvWpfGi1nTY6lH1shdDy8zv1ndRkgcl6ETA+QTETud/ftrlPuhla4 zuMz6t5yXd/kUiSvIJPN4LU1J3uBTs+0IGl7BOKg9dC7n39H+5ucEoL6cg+UcG5b Lv8WVuS5NeoBL9V27Y/KcNV02ukoEuSvQeI6Q/SjTbzmDnTwfEKVhjGjdQPaKejh cuU6BOvzGUN+Vkku7Wa0yo/AdVApFwlW9lpcEsblzeTtNgKok9jnSLWasX90B0wJ IEMr1vHlNb239DhLYi27lb3rBXJtvXM7C0CGEN2Orx4ChfZGNp17nWpEV7dJIm2v FlusX7VPvtJFFDcZRCC/hY5LzABzQncIn/6T/n3T2pLQS6e7uxLp+4NE9LrVZvkn rEHdp9ko4YNmMIDWR5Fx =EkvL -----END PGP SIGNATURE----- --gKMricLos+KVdGMg--