Re: [Qemu-devel] [PATCH v2] qcow2: Patch for shrinking qcow2 disk image

[Jun Li <junmuzi@gmail.com>]

On 05/15/2014 08:36 AM, Max Reitz wrote:
> On 09.05.2014 17:20, Jun Li wrote:
>> As the realization of raw shrinking, so when do qcow2 shrinking, do not
>
> *when doing
>
>> check l1 entries. When resize to size1(size1 < "disk size"), the 
>> custemer
>
> *customer
Sorry ~ :-)
>
>> knows this will destory the data. So no need to check the l1 entries 
>> which
>> is used or not.
>
> I'd somehow like to disagree, but you're correct. raw-posix truncates 
> the file regardless of whether there is data or not as well. Maybe we 
> should later add support for reporting potential data loss and asking 
> the user what to do (I'm thinking of some "force" or "accept_loss" 
> boolean for bdrv_truncate()).
ok, I will try to realize it in the update version of patch.
>
>> This is v2 of the original patch. thx.
>
> This should not be part of the commit message, but rather follow the 
> "---" under your Signed-off-by.
sorry, thx, :-)
>
>> Signed-off-by: Jun Li <junmuzi@gmail.com>
>> ---
>>   block/qcow2-cluster.c  | 17 ++++++++++++-----
>>   block/qcow2-snapshot.c |  2 +-
>>   block/qcow2.c          | 10 ++--------
>>   block/qcow2.h          |  4 ++--
>>   4 files changed, 17 insertions(+), 16 deletions(-)
>>
>> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
>> index 76d2bcf..8fbbf7f 100644
>> --- a/block/qcow2-cluster.c
>> +++ b/block/qcow2-cluster.c
>> @@ -29,8 +29,8 @@
>>   #include "block/qcow2.h"
>>   #include "trace.h"
>>   -int qcow2_grow_l1_table(BlockDriverState *bs, uint64_t min_size,
>> -                        bool exact_size)
>> +int qcow2_truncate_l1_table(BlockDriverState *bs, uint64_t min_size,
>> +                            bool exact_size)
>>   {
>>       BDRVQcowState *s = bs->opaque;
>>       int new_l1_size2, ret, i;
>> @@ -39,8 +39,9 @@ int qcow2_grow_l1_table(BlockDriverState *bs, 
>> uint64_t min_size,
>>       int64_t new_l1_table_offset, new_l1_size;
>>       uint8_t data[12];
>>   -    if (min_size <= s->l1_size)
>> +    if (min_size == s->l1_size) {
>>           return 0;
>> +    }
>>         /* Do a sanity check on min_size before trying to calculate 
>> new_l1_size
>>        * (this prevents overflows during the while loop for the 
>> calculation of
>> @@ -73,7 +74,13 @@ int qcow2_grow_l1_table(BlockDriverState *bs, 
>> uint64_t min_size,
>>         new_l1_size2 = sizeof(uint64_t) * new_l1_size;
>>       new_l1_table = g_malloc0(align_offset(new_l1_size2, 512));
>> -    memcpy(new_l1_table, s->l1_table, s->l1_size * sizeof(uint64_t));
>> +
>> +    /* shrinking or growing l1 table */
>> +    if (min_size < s->l1_size) {
>> +        memcpy(new_l1_table, s->l1_table, new_l1_size2);
>> +    } else {
>> +        memcpy(new_l1_table, s->l1_table, s->l1_size * 
>> sizeof(uint64_t));
>> +    }
>>         /* write new table (align to cluster) */
>>       BLKDBG_EVENT(bs->file, BLKDBG_L1_GROW_ALLOC_TABLE);
>
> So, as far as I understand the new logic of qcow2_truncate_l1_table(), 
> it will always grow the table unless exact_size == true, in which case 
> it may be shrunk as well. This should probably be reflected in the if 
> condition ("if (exact_size ? min_size == s->l1_size : min_size <= 
> s->l1_size)" or something like that). But on the other hand, I don't 
> like a function which suggests being usable for both shrinking and 
> growing, which then can be used for shrinking only in a special case 
> (exact_size == true). You should at least add a comment which states 
> that this function is generally intended for growing the L1 table with 
> min_size being the new minimal size, but may also be used for 
> shrinking if exact_size is true.
ok, checking...
>
> Apart from that, if you're shrinking the L1 table, you should in my 
> opinion free all clusters referenced from the truncated area. It is 
> true that it's the user's responsibility to make sure no data is lost, 
> but if you just shrink the L1 table without doing anything about the 
> lost data references, clusters will be leaked. This can easily be 
> fixed by qemu-img check, but there are two problems with that: First, 
> data should be leaked only if it cannot be avoided. It can easily be 
> repaired, but unless there are errors during some operation, that 
> should not be necessary. Second, qemu-img check actually does not work 
> for all image sizes that qemu itself supports. This is even more 
> reason to avoid leaks: Normally, it can easily be repaired, but 
> sometimes, it cannot.
ok, thx~ Agree to free all clusters referenced from the truncated area. 
Checking... Will realize this in v3 of patch.
>
>> @@ -558,7 +565,7 @@ static int get_cluster_table(BlockDriverState 
>> *bs, uint64_t offset,
>>         l1_index = offset >> (s->l2_bits + s->cluster_bits);
>>       if (l1_index >= s->l1_size) {
>> -        ret = qcow2_grow_l1_table(bs, l1_index + 1, false);
>> +        ret = qcow2_truncate_l1_table(bs, l1_index + 1, false);
>>           if (ret < 0) {
>>               return ret;
>>           }
>> diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
>> index 0aa9def..6ba460e 100644
>> --- a/block/qcow2-snapshot.c
>> +++ b/block/qcow2-snapshot.c
>> @@ -483,7 +483,7 @@ int qcow2_snapshot_goto(BlockDriverState *bs, 
>> const char *snapshot_id)
>>        * L1 table of the snapshot. If the snapshot L1 table is 
>> smaller, the
>>        * current one must be padded with zeros.
>>        */
>> -    ret = qcow2_grow_l1_table(bs, sn->l1_size, true);
>> +    ret = qcow2_truncate_l1_table(bs, sn->l1_size, true);
>>       if (ret < 0) {
>>           goto fail;
>>       }
>> diff --git a/block/qcow2.c b/block/qcow2.c
>> index a4b97e8..70f951c 100644
>> --- a/block/qcow2.c
>> +++ b/block/qcow2.c
>> @@ -1877,7 +1877,7 @@ static int qcow2_truncate(BlockDriverState *bs, 
>> int64_t offset)
>>       int64_t new_l1_size;
>>       int ret;
>>   -    if (offset & 511) {
>> +    if (offset <= 0 || offset & 511) {
>>           error_report("The new size must be a multiple of 512");
>>           return -EINVAL;
>>       }
>> @@ -1888,14 +1888,8 @@ static int qcow2_truncate(BlockDriverState 
>> *bs, int64_t offset)
>>           return -ENOTSUP;
>>       }
>>   -    /* shrinking is currently not supported */
>> -    if (offset < bs->total_sectors * 512) {
>> -        error_report("qcow2 doesn't support shrinking images yet");
>> -        return -ENOTSUP;
>> -    }
>> -
>>       new_l1_size = size_to_l1(s, offset);
>> -    ret = qcow2_grow_l1_table(bs, new_l1_size, true);
>> +    ret = qcow2_truncate_l1_table(bs, new_l1_size, true);
>>       if (ret < 0) {
>>           return ret;
>>       }
>> diff --git a/block/qcow2.h b/block/qcow2.h
>> index b49424b..fa36930 100644
>> --- a/block/qcow2.h
>> +++ b/block/qcow2.h
>> @@ -499,8 +499,8 @@ int 
>> qcow2_pre_write_overlap_check(BlockDriverState *bs, int ign, int64_t 
>> offset,
>>                                     int64_t size);
>>     /* qcow2-cluster.c functions */
>> -int qcow2_grow_l1_table(BlockDriverState *bs, uint64_t min_size,
>> -                        bool exact_size);
>> +int qcow2_truncate_l1_table(BlockDriverState *bs, uint64_t min_size,
>> +                            bool exact_size);
>>   int qcow2_write_l1_entry(BlockDriverState *bs, int l1_index);
>>   void qcow2_l2_cache_reset(BlockDriverState *bs);
>>   int qcow2_decompress_cluster(BlockDriverState *bs, uint64_t 
>> cluster_offset);
>
> The rest of the code looks surprisingly simple, but I looks correct to 
> me.
>
> I'd really like a test case for qemu-iotests, though. :-)
ok, I will try.

Best Regards,
Jun Li