Re: [Qemu-devel] [PATCH v5 3/4] qcow2: add shrink image support

[Pavel Butsykin]

On 13.07.2017 17:36, Max Reitz wrote:
> On 2017-07-13 10:41, Kevin Wolf wrote:
> > Am 12.07.2017 um 18:58 hat Max Reitz geschrieben:
> > > On 2017-07-12 16:52, Kevin Wolf wrote:
> > > > Am 12.07.2017 um 13:46 hat Pavel Butsykin geschrieben:
> > > > > This patch add shrinking of the image file for qcow2. As a result, this allows
> > > > > us to reduce the virtual image size and free up space on the disk without
> > > > > copying the image. Image can be fragmented and shrink is done by punching holes
> > > > > in the image file.
> > > > >
> > > > > Signed-off-by: Pavel Butsykin <address@hidden>
> > > > > Reviewed-by: Max Reitz <address@hidden>
> > > > > ---
> > > > >   block/qcow2-cluster.c  |  40 ++++++++++++++++++
> > > > >   block/qcow2-refcount.c | 110 +++++++++++++++++++++++++++++++++++++++++++++++++
> > > > >   block/qcow2.c          |  43 +++++++++++++++----
> > > > >   block/qcow2.h          |  14 +++++++
> > > > >   qapi/block-core.json   |   3 +-
> > > > >   5 files changed, 200 insertions(+), 10 deletions(-)
> > > > >
> > > > > diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> > > > > index f06c08f64c..518429c64b 100644
> > > > > --- a/block/qcow2-cluster.c
> > > > > +++ b/block/qcow2-cluster.c
> > > > > @@ -32,6 +32,46 @@
> > > > >   #include "qemu/bswap.h"
> > > > >   #include "trace.h"
> > > > >   
> > > > > +int qcow2_shrink_l1_table(BlockDriverState *bs, uint64_t exact_size)
> > > > > +{
> > > > > +    BDRVQcow2State *s = bs->opaque;
> > > > > +    int new_l1_size, i, ret;
> > > > > +
> > > > > +    if (exact_size >= s->l1_size) {
> > > > > +        return 0;
> > > > > +    }
> > > > > +
> > > > > +    new_l1_size = exact_size;
> > > > > +
> > > > > +#ifdef DEBUG_ALLOC2
> > > > > +    fprintf(stderr, "shrink l1_table from %d to %d\n", s->l1_size, 
> > > > > new_l1_size);
> > > > > +#endif
> > > > > +
> > > > > +    BLKDBG_EVENT(bs->file, BLKDBG_L1_SHRINK_WRITE_TABLE);
> > > > > +    ret = bdrv_pwrite_zeroes(bs->file, s->l1_table_offset +
> > > > > +                                       sizeof(uint64_t) * new_l1_size,
> > > > > +                             (s->l1_size - new_l1_size) * sizeof(uint64_t), 0);
> > > > > +    if (ret < 0) {
> > > > > +        return ret;
> > > > > +    }
> > > > > +
> > > > > +    ret = bdrv_flush(bs->file->bs);
> > > > > +    if (ret < 0) {
> > > > > +        return ret;
> > > > > +    }
> > > >
> > > > If we have an error here (or after a partial bdrv_pwrite_zeroes()), we
> > > > have entries zeroed out on disk, but in memory we still have the
> > > > original L1 table.
> > > >
> > > > Should the in-memory L1 table be zeroed first? Then we can't
> > > > accidentally reuse stale entries, but would have to allocate new ones,
> > > > which would get on-disk state and in-memory state back in sync again.
> > >
> > > Yes, I thought of the same.  But this implies that the allocation is
> > > able to modify the L1 table, and I find that unlikely if that
> > > bdrv_flush() failed already...
> > >
> > > So I concluded not to have an opinion on which order is better.
> >
> > Well, let me ask the other way round: Is there any disadvantage in first
> > zeroing the in-memory table and then writing to the image?
>
> I was informed that the code would be harder to write. :-)
>
> > If I have a choice between "always safe" and "not completely safe, but I
> > think it's unlikely to happen", especially in image formats, then I will
> > certainly take the "always safe".
> >
> > > > > +    BLKDBG_EVENT(bs->file, BLKDBG_L1_SHRINK_FREE_L2_CLUSTERS);
> > > > > +    for (i = s->l1_size - 1; i > new_l1_size - 1; i--) {
> > > > > +        if ((s->l1_table[i] & L1E_OFFSET_MASK) == 0) {
> > > > > +            continue;
> > > > > +        }
> > > > > +        qcow2_free_clusters(bs, s->l1_table[i] & L1E_OFFSET_MASK,
> > > > > +                            s->cluster_size, QCOW2_DISCARD_ALWAYS);
> > > > > +        s->l1_table[i] = 0;
> > > > > +    }
> > > > > +    return 0;
> > > > > +}
> > > > > +
> > > > >   int qcow2_grow_l1_table(BlockDriverState *bs, uint64_t min_size,
> > > > >                           bool exact_size)
> > > > >   {
> > > >
> > > > I haven't checked qcow2_shrink_reftable() for similar kinds of problems,
> > > > I hope Max has.
> > >
> > > Well, it's exactly the same there.
> >
> > Ok, so I'll object to this code without really having looked at it.
> I won't object to your objection. O:-)

Kevin,

Can you help me to reduce the number of patch-set versions? :)
And look at the rest part of the series, thanks!

> Max