Re: [Qemu-devel] [PATCH v6 09/11] qcow2: Clean up after refcount rebuild

[Max Reitz <mreitz@redhat.com>]

On 2014-10-21 at 11:59, Kevin Wolf wrote:
> Am 20.10.2014 um 16:35 hat Max Reitz geschrieben:
>> Because the old refcount structure will be leaked after having rebuilt
>> it, we need to recalculate the refcounts and run a leak-fixing operation
>> afterwards (if leaks should be fixed at all).
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> Reviewed-by: Benoît Canet <benoit.canet@nodalink.com>
>> ---
>>   block/qcow2-refcount.c | 35 +++++++++++++++++++++++++++++++++++
>>   1 file changed, 35 insertions(+)
>>
>> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
>> index 75e726b..3730be2 100644
>> --- a/block/qcow2-refcount.c
>> +++ b/block/qcow2-refcount.c
>> @@ -1956,12 +1956,47 @@ int qcow2_check_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
>>                         nb_clusters);
>>   
>>       if (rebuild && (fix & BDRV_FIX_ERRORS)) {
>> +        BdrvCheckResult old_res = *res;
>> +
>>           fprintf(stderr, "Rebuilding refcount structure\n");
>>           ret = rebuild_refcount_structure(bs, res, &refcount_table,
>>                                            &nb_clusters);
>>           if (ret < 0) {
>>               goto fail;
>>           }
>> +
>> +        res->corruptions = 0;
>> +        res->leaks = 0;
>> +
>> +        /* Because the old reftable has been exchanged for a new one the
>> +         * references have to be recalculated */
>> +        rebuild = false;
>> +        memset(refcount_table, 0, nb_clusters * sizeof(uint16_t));
>> +        ret = calculate_refcounts(bs, res, 0, &rebuild, &refcount_table,
>> +                                  &nb_clusters);
>> +        if (ret < 0) {
>> +            goto fail;
>> +        }
>> +
>> +        if (fix & BDRV_FIX_LEAKS) {
>> +            /* The old refcount structures are now leaked, fix it; the result
>> +             * can be ignored */
>> +            pre_compare_res = *res;
> I would prefer using another local variable here. At the first sight
> it's not quite clear which references to pre_compare_res correspond to
> which state.

Why not.

>> +            compare_refcounts(bs, res, BDRV_FIX_LEAKS, &rebuild,
>> +                              &highest_cluster, refcount_table, nb_clusters);
>> +            if (rebuild) {
>> +                fprintf(stderr, "ERROR rebuilt refcount structure is still "
>> +                        "broken\n");
>> +            }
>> +            *res = pre_compare_res;
>> +        }
>> +
>> +        if (res->corruptions < old_res.corruptions) {
>> +            res->corruptions_fixed += old_res.corruptions - res->corruptions;
>> +        }
>> +        if (res->leaks < old_res.leaks) {
>> +            res->leaks_fixed += old_res.leaks - res->leaks;
>> +        }
> For these numbers to be accurate, don't we need to run
> compare_refcounts() unconditionally and only make BDRV_FIX_LEAKS
> conditional?

Actually, there is no difference, because at the point of this patch, 
you cannot use BDRV_FIX_ERRORS without BDRV_FIX_LEAKS. But it'd be more 
correct, right.

Thanks,

Max

> Now that I've read the rest of the series, comparing res and old_res
> actually makes sense, so maybe it's not necessary to introduce a
> Qcow2CheckResult.
>
>>       } else if (fix) {
>>           if (rebuild) {
>>               fprintf(stderr, "ERROR need to rebuild refcount structures\n");
>> -- 
>> 2.1.2
> Kevin