From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42440) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bpHSS-00059k-Jh for qemu-devel@nongnu.org; Wed, 28 Sep 2016 12:11:45 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bpHSQ-0008Q8-6S for qemu-devel@nongnu.org; Wed, 28 Sep 2016 12:11:43 -0400 References: <1475046261-15679-1-git-send-email-famz@redhat.com> From: Max Reitz Message-ID: Date: Wed, 28 Sep 2016 18:11:31 +0200 MIME-Version: 1.0 In-Reply-To: <1475046261-15679-1-git-send-email-famz@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="XnoSfoXx8r4hqhdQFuKIOD61IJPnCxAT9" Subject: Re: [Qemu-devel] [PATCH] qcow2: Support BDRV_REQ_MAY_UNMAP List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Fam Zheng , qemu-devel@nongnu.org Cc: Kevin Wolf , qemu-block@nongnu.org This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --XnoSfoXx8r4hqhdQFuKIOD61IJPnCxAT9 From: Max Reitz To: Fam Zheng , qemu-devel@nongnu.org Cc: Kevin Wolf , qemu-block@nongnu.org Message-ID: Subject: Re: [PATCH] qcow2: Support BDRV_REQ_MAY_UNMAP References: <1475046261-15679-1-git-send-email-famz@redhat.com> In-Reply-To: <1475046261-15679-1-git-send-email-famz@redhat.com> Content-Type: text/plain; charset=iso-8859-15 Content-Transfer-Encoding: quoted-printable On 28.09.2016 09:04, Fam Zheng wrote: > Handling this is similar to what is done to the L2 entry in the case of= > compressed clusters. >=20 > Signed-off-by: Fam Zheng > --- > block/qcow2-cluster.c | 9 +++++---- > block/qcow2.c | 3 ++- > block/qcow2.h | 3 ++- > 3 files changed, 9 insertions(+), 6 deletions(-) >=20 > diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c > index 61d1ffd..928c1e2 100644 > --- a/block/qcow2-cluster.c > +++ b/block/qcow2-cluster.c > @@ -1558,7 +1558,7 @@ fail: > * clusters. > */ > static int zero_single_l2(BlockDriverState *bs, uint64_t offset, > - uint64_t nb_clusters) > + uint64_t nb_clusters, int flags) > { > BDRVQcow2State *s =3D bs->opaque; > uint64_t *l2_table; > @@ -1582,7 +1582,7 @@ static int zero_single_l2(BlockDriverState *bs, u= int64_t offset, > =20 > /* Update L2 entries */ > qcow2_cache_entry_mark_dirty(bs, s->l2_table_cache, l2_table);= > - if (old_offset & QCOW_OFLAG_COMPRESSED) { > + if (old_offset & QCOW_OFLAG_COMPRESSED || flags & BDRV_REQ_MAY= _UNMAP) { > l2_table[l2_index + i] =3D cpu_to_be64(QCOW_OFLAG_ZERO); > qcow2_free_any_clusters(bs, old_offset, 1, QCOW2_DISCARD_R= EQUEST); I don't quite understand the reasoning behind this. How is this more efficient than just using the existing path where we don't discard anythi= ng? Note that BDRV_REQ_MAY_UNMAP does not mean "Yes, please discard" but just "You may discard if it's easier for you". But it's actually not easier for us, so I don't see why we're doing it. As far as I can guess you actually want some way to tell a block driver to actually make an effort to discard clusters as long they then read back as zero (which is why you cannot simply use bdrv_pdiscard()). However, I think this would require a new flag called BDRV_REQ_SHOULD_UNMAP (which should imply BDRV_REQ_MAY_UNMAP). Note that there is actually a case where qcow2 should support BDRV_REQ_MAY_UNMAP, which is for v2 images. Currently, we just return -ENOTSUP for them, but if we don't have a backing file and BDRV_REQ_MAY_UNMAP is set, we could go on and make qcow2_zero_clusters() work for them. Max > } else { > @@ -1595,7 +1595,8 @@ static int zero_single_l2(BlockDriverState *bs, u= int64_t offset, > return nb_clusters; > } > =20 > -int qcow2_zero_clusters(BlockDriverState *bs, uint64_t offset, int nb_= sectors) > +int qcow2_zero_clusters(BlockDriverState *bs, uint64_t offset, int nb_= sectors, > + int flags) > { > BDRVQcow2State *s =3D bs->opaque; > uint64_t nb_clusters; > @@ -1612,7 +1613,7 @@ int qcow2_zero_clusters(BlockDriverState *bs, uin= t64_t offset, int nb_sectors) > s->cache_discards =3D true; > =20 > while (nb_clusters > 0) { > - ret =3D zero_single_l2(bs, offset, nb_clusters); > + ret =3D zero_single_l2(bs, offset, nb_clusters, flags); > if (ret < 0) { > goto fail; > } > diff --git a/block/qcow2.c b/block/qcow2.c > index 0e53a4d..474f244 100644 > --- a/block/qcow2.c > +++ b/block/qcow2.c > @@ -1154,6 +1154,7 @@ static int qcow2_open(BlockDriverState *bs, QDict= *options, int flags, > =20 > /* Initialise locks */ > qemu_co_mutex_init(&s->lock); > + bs->supported_zero_flags =3D BDRV_REQ_MAY_UNMAP; > =20 > /* Repair image if dirty */ > if (!(flags & (BDRV_O_CHECK | BDRV_O_INACTIVE)) && !bs->read_only = && > @@ -2476,7 +2477,7 @@ static coroutine_fn int qcow2_co_pwrite_zeroes(Bl= ockDriverState *bs, > trace_qcow2_pwrite_zeroes(qemu_coroutine_self(), offset, count); > =20 > /* Whatever is left can use real zero clusters */ > - ret =3D qcow2_zero_clusters(bs, offset, count >> BDRV_SECTOR_BITS)= ; > + ret =3D qcow2_zero_clusters(bs, offset, count >> BDRV_SECTOR_BITS,= flags); > qemu_co_mutex_unlock(&s->lock); > =20 > return ret; > diff --git a/block/qcow2.h b/block/qcow2.h > index 9ce5a37..92203a8 100644 > --- a/block/qcow2.h > +++ b/block/qcow2.h > @@ -547,7 +547,8 @@ uint64_t qcow2_alloc_compressed_cluster_offset(Bloc= kDriverState *bs, > int qcow2_alloc_cluster_link_l2(BlockDriverState *bs, QCowL2Meta *m); > int qcow2_discard_clusters(BlockDriverState *bs, uint64_t offset, > int nb_sectors, enum qcow2_discard_type type, bool full_discard); > -int qcow2_zero_clusters(BlockDriverState *bs, uint64_t offset, int nb_= sectors); > +int qcow2_zero_clusters(BlockDriverState *bs, uint64_t offset, int nb_= sectors, > + int flags); > =20 > int qcow2_expand_zero_clusters(BlockDriverState *bs, > BlockDriverAmendStatusCB *status_cb, >=20 --XnoSfoXx8r4hqhdQFuKIOD61IJPnCxAT9 Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- iQEvBAEBCAAZBQJX6+uzEhxtcmVpdHpAcmVkaGF0LmNvbQAKCRD0B9sAYdXPQJcl B/9kAJKpCJKKuQiBXvyJAb01Jg2C79x9a/Fr3+JvtOfCTj7cYFJdEOOgANrHlVjl k6OfNsBYZUw09m3cRTKeMhgnDUVOGeLSAFrkAZlbrmVVcC0h8w6g3AIbx3riZG5O UIKTj6UwVo+4I1rWz0w1pZPrT9gdepAp6gZbbxtjFmUnrASjUNtKUBZp98HDExM9 tMSzY9V5pc5koLCh/g3KJZBx6/hs2fd2eIaeUtpKzEs56vq/tGFwoNjFL9M6V2fT dvwRCyHO+E70yHfriewxRUTwQA8WwRWiFIOgTEkGCGpp4OO9clw8iLYS0LNeMpBX v8M8uJKf9Yl2zilO1yKaEleN =3Bdt -----END PGP SIGNATURE----- --XnoSfoXx8r4hqhdQFuKIOD61IJPnCxAT9--