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

[Max Reitz]

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