Re: [Qemu-devel] [PATCH v2 4/5] qcow2: More complete consistency check

[Max Reitz]

Am 29.08.2013 13:36, schrieb Kevin Wolf:
> Am 28.08.2013 um 16:55 hat Max Reitz geschrieben:
> > The qcow2_check_refcounts function has been extended to be able to fix
> > OFLAG_COPIED errors and multiple references on refcount blocks.
> >
> > If no corruptions remain after an image repair (and no errors have been
> > encountered), clear the corrupt flag in qcow2_check.
> >
> > Signed-off-by: Max Reitz <address@hidden>
> This would be easier to review if you kept the code changes of the
> actual check (mostly code movement, I guess) and the repair of each type
> of errors in separate patches.
>
> >   block/qcow2-cluster.c  |   4 +-
> >   block/qcow2-refcount.c | 249 ++++++++++++++++++++++++++++++++++++++++++-------
> >   block/qcow2.c          |   6 +-
> >   block/qcow2.h          |   1 +
> >   include/block/block.h  |   1 +
> >   5 files changed, 222 insertions(+), 39 deletions(-)
> > @@ -1240,7 +1243,8 @@ int qcow2_check_refcounts(BlockDriverState *bs, 
> > BdrvCheckResult *res,
> >   {
> >       BDRVQcowState *s = bs->opaque;
> >       int64_t size, i, highest_cluster;
> > -    int nb_clusters, refcount1, refcount2;
> > +    uint64_t *l2_table = NULL;
> > +    int nb_clusters, refcount1, refcount2, j;
> >       QCowSnapshot *sn;
> >       uint16_t *refcount_table;
> >       int ret;
> > @@ -1305,10 +1309,85 @@ int qcow2_check_refcounts(BlockDriverState *bs, 
> > BdrvCheckResult *res,
> >               inc_refcounts(bs, res, refcount_table, nb_clusters,
> >                   offset, s->cluster_size);
> >               if (refcount_table[cluster] != 1) {
> > -                fprintf(stderr, "ERROR refcount block %" PRId64
> > +                fprintf(stderr, "%s refcount block %" PRId64
> >                       " refcount=%d\n",
> > +                    fix & BDRV_FIX_ERRORS ? "Repairing" :
> > +                                            "ERROR",
> >                       i, refcount_table[cluster]);
> > -                res->corruptions++;
> > +                if (fix & BDRV_FIX_ERRORS) {
> This is a quite long if block. Probably worth splitting into its own
> function. (The res->corruptions++ part could be in a common fail: part
> then.)
Seems reasonable.

> > +                    int64_t new_offset;
> > +                    void *refcount_block;
> > +
> > +                    /* allocate new refcount block */
> > +                    new_offset = qcow2_alloc_clusters(bs, s->cluster_size);
> How trustworthy is qcow2_alloc_clusters() when we know that our refcount
> information is corrupted? Couldn't we end up accidentally overwriting
> things?
I personally don't really see any other way to do this than to just fail 
without any attempt on repairing. If the refcounts are completely 
broken, I don't see a reasonable way of allocating a free cluster. 
Furthermore, the following pre-write check should at least prevent this 
from overwriting any metadata.

> > +                    if (new_offset < 0) {
> > +                        fprintf(stderr, "Could not allocate new cluster\n");
> > +                        res->corruptions++;
> > +                        continue;
> > +                    }
> > +                    /* fetch current content */
> > +                    ret = qcow2_cache_get(bs, s->refcount_block_cache, offset,
> > +                            &refcount_block);
> > +                    if (ret < 0) {
> > +                        fprintf(stderr, "Could not fetch refcount block\n");
> strerror(-ret) would be useful information in almost all of the error
> cases.
Yes, right.

> > +                        qcow2_free_clusters(bs, new_offset, s->cluster_size,
> > +                                QCOW2_DISCARD_ALWAYS);
> > +                        res->corruptions++;
> > +                        continue;
> > +                    }
> > +                    /* new block has not yet been entered into refcount table,
> > +                     * therefore it is no refcount block yet (regarding this
> > +                     * check) */
> > +                    ret = qcow2_pre_write_overlap_check(bs, QCOW2_OL_DEFAULT,
> > +                            new_offset, s->cluster_sectors * BDRV_SECTOR_SIZE);
> > +                    if (ret < 0) {
> > +                        fprintf(stderr, "Could not write refcount block (would 
> > "
> > +                                "overlap with existing metadata)\n");
> > +                        /* the image will be marked corrupt here, so don't even
> > +                         * attempt on freeing the cluster */
> > +                        res->corruptions++;
> > +                        goto fail;
> > +                    }
> > +                    /* write to new block */
> > +                    ret = bdrv_write(bs->file, new_offset >> BDRV_SECTOR_BITS,
> > +                            refcount_block, s->cluster_sectors);
> > +                    if (ret < 0) {
> > +                        fprintf(stderr, "Could not write refcount block\n");
> > +                        qcow2_free_clusters(bs, new_offset, s->cluster_size,
> > +                                QCOW2_DISCARD_ALWAYS);
> > +                        res->corruptions++;
> > +                        continue;
> > +                    }
> > +                    /* update refcount table */
> > +                    assert(!(new_offset & (s->cluster_size - 1)));
> > +                    s->refcount_table[i] = new_offset;
> > +                    ret = write_reftable_entry(bs, i);
> > +                    if (ret < 0) {
> > +                        fprintf(stderr, "Could not update refcount table\n");
> > +                        s->refcount_table[i] = offset;
> > +                        qcow2_free_clusters(bs, new_offset, s->cluster_size,
> > +                                QCOW2_DISCARD_ALWAYS);
> > +                        res->corruptions++;
> > +                        continue;
> > +                    }
> > +                    qcow2_cache_put(bs, s->refcount_block_cache,
> > +                            &refcount_block);
> > +                    /* update refcounts */
> > +                    if ((new_offset >> s->cluster_bits) >= nb_clusters) {
> > +                        /* increase refcount_table size if necessary */
> > +                        int old_nb_clusters = nb_clusters;
> > +                        nb_clusters = (new_offset >> s->cluster_bits) + 1;
> > +                        refcount_table = g_realloc(refcount_table,
> > +                                nb_clusters * sizeof(uint16_t));
> > +                        memset(&refcount_table[old_nb_clusters], 0, 
> > (nb_clusters
> > +                                - old_nb_clusters) * sizeof(uint16_t));
> > +                    }
> > +                    refcount_table[cluster]--;
> > +                    inc_refcounts(bs, res, refcount_table, nb_clusters,
> > +                            new_offset, s->cluster_size);
> res->corruptions_fixed++?
Oh, yes, I forgot that.

> > +                } else {
> > +                    res->corruptions++;
> > +                }
> >               }
> >           }
> >       }
> > @@ -1364,10 +1443,108 @@ int qcow2_check_refcounts(BlockDriverState *bs, 
> > BdrvCheckResult *res,
> >           }
> >       }
> >   
> > +    l2_table = g_malloc(s->l2_size * sizeof(uint64_t));
> qemu_blockalign() is better than g_malloc() because it avoids using a
> bounce buffer for O_DIRECT images.
Ah, okay. I'll include that in my other series as well.

> > +
> > +    /* check OFLAG_COPIED */
> > +    for (i = 0; i < s->l1_size; i++) {
> > +        uint64_t l1_entry = s->l1_table[i];
> > +        uint64_t l2_offset = l1_entry & L1E_OFFSET_MASK;
> > +        bool l2_dirty = false;
> > +        int refcount;
> > +
> > +        if (!l2_offset) {
> > +            continue;
> > +        }
> > +
> > +        refcount = get_refcount(bs, l2_offset >> s->cluster_bits);
> > +        if (refcount < 0) {
> > +            /* don't print message nor increment check_errors, since the above
> > +             * loop will have done this already */
> > +            continue;
> > +        }
> > +        if ((refcount == 1) != ((l1_entry & QCOW_OFLAG_COPIED) != 0)) {
> > +            fprintf(stderr, "%s OFLAG_COPIED L2 cluster: l1_entry=%" PRIx64
> > +                    " refcount=%d\n",
> > +                    fix & BDRV_FIX_ERRORS ? "Repairing" :
> > +                                            "ERROR",
> > +                    l1_entry, refcount);
> > +            if (fix & BDRV_FIX_ERRORS) {
> > +                s->l1_table[i] = refcount == 1
> > +                               ? l1_entry |  QCOW_OFLAG_COPIED
> > +                               : l1_entry & ~QCOW_OFLAG_COPIED;
> > +                ret = qcow2_write_l1_entry(bs, i);
> > +                if (ret < 0) {
> > +                    res->check_errors++;
> > +                    goto fail;
> > +                }
> else res->corruptions_fixed++?
>
> > +            } else {
> > +                res->corruptions++;
> > +            }
> > +        }
> > +
> > +        ret = bdrv_pread(bs->file, l2_offset, l2_table,
> > +                s->l2_size * sizeof(uint64_t));
> > +        if (ret != s->l2_size * sizeof(uint64_t)) {
> > +            fprintf(stderr, "ERROR: Could not read L2 table\n");
> > +            res->check_errors++;
> > +            if (ret >= 0) {
> > +                ret = -EIO;
> > +            }
> Doesn't happen. You can just check ret < 0 instead of ret != ... in the
> first place, bdrv_pread() never returns short reads.
Okay, I think I just saw it that way in some other code.

> > +            goto fail;
> > +        }
> > +
> > +        for (j = 0; j < s->l2_size; j++) {
> > +            uint64_t l2_entry = be64_to_cpu(l2_table[j]);
> > +            uint64_t data_offset;
> > +
> > +            if (qcow2_get_cluster_type(l2_entry) != QCOW2_CLUSTER_NORMAL) {
> > +                continue;
> > +            }
> > +
> > +            data_offset = l2_entry & L2E_OFFSET_MASK;
> > +
> > +            refcount = get_refcount(bs, data_offset >> s->cluster_bits);
> > +            if (refcount < 0) {
> > +                /* don't print message nor increment check_errors */
> Why?
I didn't want to duplicate the whole comment from above, but maybe it 
makes sense if it's not obvious:

/* don't print message nor increment check_errors, since the above
 * loop will have done this already */

> > +                continue;
> > +            }
> > +            if ((refcount == 1) != ((l2_entry & QCOW_OFLAG_COPIED) != 0)) {
> > +                fprintf(stderr, "%s OFLAG_COPIED data cluster: l2_entry=%"
> > +                        PRIx64 " refcount=%d\n",
> > +                        fix & BDRV_FIX_ERRORS ? "Repairing" :
> > +                                                "ERROR",
> > +                        l2_entry, refcount);
> > +                if (fix & BDRV_FIX_ERRORS) {
> > +                    l2_table[j] = cpu_to_be64(refcount == 1
> > +                                ? l2_entry |  QCOW_OFLAG_COPIED
> > +                                : l2_entry & ~QCOW_OFLAG_COPIED);
> > +                    l2_dirty = true;
> res->corruptions_fixed++
>
> > +                } else {
> > +                    res->corruptions++;
> > +                }
> > +            }
> > +        }
> > +
> > +        if (l2_dirty) {
> > +            ret = bdrv_pwrite(bs->file, l2_offset, l2_table,
> > +                    s->l2_size * sizeof(uint64_t));
> > +            if (ret != s->l2_size * sizeof(uint64_t)) {
> > +                fprintf(stderr, "ERROR: Could not write L2 table\n");
> > +                res->check_errors++;
> > +                if (ret >= 0) {
> > +                    ret = -EIO;
> > +                }
> bdrv_pwrite() also doesn't return short writes.
>
> > +                goto fail;
> > +            }
> > +        }
> > +    }
> > +
> > +
> >       res->image_end_offset = (highest_cluster + 1) * s->cluster_size;
> >       ret = 0;
> >   
> >   fail:
> > +    g_free(l2_table);
> >       g_free(refcount_table);
> >   
> >       return ret;
> Kevin
Max