Re: [Qemu-devel] [PATCH v2 01/12] qcow2: Add new overlap check functions

[Max Reitz <mreitz@redhat.com>]

On 2015-01-21 at 11:53, Stefan Hajnoczi wrote:
> On Mon, Nov 24, 2014 at 04:56:49PM +0100, Max Reitz wrote:
>> +static void build_window_bitmap(Qcow2MetadataList *mdl,
>> +                                Qcow2MetadataWindow *window)
>> +{
>> +    int cache_i, oldest_cache_i = -1, i;
>> +    unsigned oldest_cache_age = 0;
>> +
>> +    for (cache_i = 0; cache_i < mdl->nb_cached_windows; cache_i++) {
>> +        unsigned age;
>> +
>> +        if (mdl->cached_windows[cache_i] < 0) {
>> +            break;
>> +        }
>> +
>> +        age = mdl->current_age - mdl->windows[mdl->cached_windows[cache_i]].age;
>> +        if (age > oldest_cache_age) {
>> +            oldest_cache_age = age;
>> +            oldest_cache_i = cache_i;
>> +        }
>> +    }
>> +
>> +    if (cache_i >= mdl->nb_cached_windows) {
>> +        destroy_window_bitmap(mdl,
>> +            &mdl->windows[mdl->cached_windows[oldest_cache_i]]);
>> +        cache_i = oldest_cache_i;
>> +    }
>> +
>> +    assert(cache_i >= 0);
>> +    mdl->cached_windows[cache_i] = window - mdl->windows;
>> +    window->cached_windows_index = cache_i;
> Is this field ever used?

You're right, it isn't. Great! *g*

Thanks, I'll remove it.

>> +/**
>> + * Removes a range of the given types from the metadata list.
>> + */
>> +void qcow2_metadata_list_remove(BlockDriverState *bs, uint64_t offset,
>> +                                int nb_clusters, QCow2MetadataOverlap types)
>> +{
>> +    BDRVQcowState *s = bs->opaque;
>> +    uint64_t start_cluster = offset >> s->cluster_bits;
>> +    uint64_t end_cluster = start_cluster + nb_clusters;
>> +    uint64_t current_cluster = start_cluster;
>> +
>> +    types &= s->overlap_check;
>> +    if (!types) {
>> +        return;
>> +    }
>> +
>> +    if (offset_into_cluster(s, offset)) {
>> +        /* Try to remove even broken metadata ranges */
>> +        end_cluster++;
> Why does it make sense to go ahead (and add another cluster on to the
> end) when offset is not cluster-aligned?
>
> It seems we might clear out the wrong metadata.

Which I deemed not as bad as keeping stale metadata in and then running 
into corruption wrongly detected. Feel free to convince me otherwise, I 
don't have a very strong opinion (I think we should ideally do as much 
for corruption prevention as possible, but I don't want to force users 
to run qemu-img check if they don't actually have to).

Max