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

[Max Reitz]

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.

>> +    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.

Max

signature.asc
Description: OpenPGP digital signature