[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] qcow2: Support BDRV_REQ_MAY_UNMAP
From: |
Fam Zheng |
Subject: |
Re: [Qemu-devel] [PATCH] qcow2: Support BDRV_REQ_MAY_UNMAP |
Date: |
Thu, 29 Sep 2016 10:21:47 +0800 |
User-agent: |
Mutt/1.7.0 (2016-08-17) |
On Wed, 09/28 18:11, Max Reitz wrote:
> 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.
> >
> > Signed-off-by: Fam Zheng <address@hidden>
> > ---
> > block/qcow2-cluster.c | 9 +++++----
> > block/qcow2.c | 3 ++-
> > block/qcow2.h | 3 ++-
> > 3 files changed, 9 insertions(+), 6 deletions(-)
> >
> > 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 = bs->opaque;
> > uint64_t *l2_table;
> > @@ -1582,7 +1582,7 @@ static int zero_single_l2(BlockDriverState *bs,
> > uint64_t offset,
> >
> > /* 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] = cpu_to_be64(QCOW_OFLAG_ZERO);
> > qcow2_free_any_clusters(bs, old_offset, 1,
> > QCOW2_DISCARD_REQUEST);
>
> 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 anything?
It's more efficient in disk space. I didn't mention because it is so not
specific to this, but: what virt-sparsify does is creating an overlay -> fstrim
it -> qemu-img commit. This flow revealed to me that BDRV_REQ_MAY_UNMAP should
have been honored this way (after a hint of "how" by Kevin).
>
> 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).
This flag doesn't make sense to me, if the protocol doesn't know how to unmap,
it can ignore BDRV_REQ_MAY_UNMAP, but not BDRV_REQ_SHOULD_UNMAP. It just
complicates things a little.
Fam
- [Qemu-devel] [PATCH] qcow2: Support BDRV_REQ_MAY_UNMAP, Fam Zheng, 2016/09/28
- Re: [Qemu-devel] [PATCH] qcow2: Support BDRV_REQ_MAY_UNMAP, Kevin Wolf, 2016/09/29
- Re: [Qemu-devel] [PATCH] qcow2: Support BDRV_REQ_MAY_UNMAP, Fam Zheng, 2016/09/29
- Re: [Qemu-devel] [PATCH] qcow2: Support BDRV_REQ_MAY_UNMAP, Kevin Wolf, 2016/09/29
- Re: [Qemu-devel] [PATCH] qcow2: Support BDRV_REQ_MAY_UNMAP, Paolo Bonzini, 2016/09/29
- Re: [Qemu-devel] [PATCH] qcow2: Support BDRV_REQ_MAY_UNMAP, Kevin Wolf, 2016/09/29
- Re: [Qemu-devel] [PATCH] qcow2: Support BDRV_REQ_MAY_UNMAP, Paolo Bonzini, 2016/09/29
- Re: [Qemu-devel] [PATCH] qcow2: Support BDRV_REQ_MAY_UNMAP, Paolo Bonzini, 2016/09/29
- Re: [Qemu-devel] [PATCH] qcow2: Support BDRV_REQ_MAY_UNMAP, Fam Zheng, 2016/09/29