diff mbox

[PULL,v2,22/26] qcow2-refcount: Move OFLAG_COPIED checks

Message ID CAJSP0QVgHQdjLzUs3Z8MxNaAP3s2UwyBcmU6Nf5EdDdrb0LEoA@mail.gmail.com
State New
Headers show

Commit Message

Stefan Hajnoczi Sept. 2, 2013, 1:52 p.m. UTC 
On Mon, Sep 2, 2013 at 10:49 AM, Kevin Wolf <kwolf@redhat.com> wrote:
> From: Max Reitz <mreitz@redhat.com>
>
> Move the OFLAG_COPIED checks out of check_refcounts_l1 and
> check_refcounts_l2 and after the actual refcount checks/fixes (since the
> refcounts might actually change there).
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/qcow2-refcount.c | 115 +++++++++++++++++++++++++++++++++++--------------
>  1 file changed, 82 insertions(+), 33 deletions(-)

This patch breaks qemu-iotests 039 as follows:

$ ./check -qcow2 039
QEMU          -- qemu
QEMU_IMG      -- qemu-img
QEMU_IO       -- qemu-io
IMGFMT        -- qcow2 (compat=1.1)
IMGPROTO      -- file
PLATFORM      -- Linux/x86_64 3.10.9-200.fc19.x86_64

039 2s ... - output mismatch (see 039.out.bad)
Failures: 039
Failed 1 of 1 tests

> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
> index 8ee2f13..aa4b98d 100644
> --- a/block/qcow2-refcount.c
> +++ b/block/qcow2-refcount.c
> @@ -1053,7 +1053,7 @@ static int check_refcounts_l2(BlockDriverState *bs, BdrvCheckResult *res,
>      BDRVQcowState *s = bs->opaque;
>      uint64_t *l2_table, l2_entry;
>      uint64_t next_contiguous_offset = 0;
> -    int i, l2_size, nb_csectors, refcount;
> +    int i, l2_size, nb_csectors;
>
>      /* Read L2 table from disk */
>      l2_size = s->l2_size * sizeof(uint64_t);
> @@ -1105,23 +1105,8 @@ static int check_refcounts_l2(BlockDriverState *bs, BdrvCheckResult *res,
>
>          case QCOW2_CLUSTER_NORMAL:
>          {
> -            /* QCOW_OFLAG_COPIED must be set iff refcount == 1 */
>              uint64_t offset = l2_entry & L2E_OFFSET_MASK;
>
> -            if (flags & CHECK_OFLAG_COPIED) {
> -                refcount = get_refcount(bs, offset >> s->cluster_bits);
> -                if (refcount < 0) {
> -                    fprintf(stderr, "Can't get refcount for offset %"
> -                        PRIx64 ": %s\n", l2_entry, strerror(-refcount));
> -                    goto fail;
> -                }
> -                if ((refcount == 1) != ((l2_entry & QCOW_OFLAG_COPIED) != 0)) {
> -                    fprintf(stderr, "ERROR OFLAG_COPIED: offset=%"
> -                        PRIx64 " refcount=%d\n", l2_entry, refcount);
> -                    res->corruptions++;
> -                }
> -            }
> -
>              if (flags & CHECK_FRAG_INFO) {
>                  res->bfi.allocated_clusters++;
>                  if (next_contiguous_offset &&
> @@ -1178,7 +1163,7 @@ static int check_refcounts_l1(BlockDriverState *bs,
>  {
>      BDRVQcowState *s = bs->opaque;
>      uint64_t *l1_table, l2_offset, l1_size2;
> -    int i, refcount, ret;
> +    int i, ret;
>
>      l1_size2 = l1_size * sizeof(uint64_t);
>
> @@ -1202,22 +1187,6 @@ static int check_refcounts_l1(BlockDriverState *bs,
>      for(i = 0; i < l1_size; i++) {
>          l2_offset = l1_table[i];
>          if (l2_offset) {
> -            /* QCOW_OFLAG_COPIED must be set iff refcount == 1 */
> -            if (flags & CHECK_OFLAG_COPIED) {
> -                refcount = get_refcount(bs, (l2_offset & ~QCOW_OFLAG_COPIED)
> -                    >> s->cluster_bits);
> -                if (refcount < 0) {
> -                    fprintf(stderr, "Can't get refcount for l2_offset %"
> -                        PRIx64 ": %s\n", l2_offset, strerror(-refcount));
> -                    goto fail;
> -                }
> -                if ((refcount == 1) != ((l2_offset & QCOW_OFLAG_COPIED) != 0)) {
> -                    fprintf(stderr, "ERROR OFLAG_COPIED: l2_offset=%" PRIx64
> -                        " refcount=%d\n", l2_offset, refcount);
> -                    res->corruptions++;
> -                }
> -            }
> -
>              /* Mark L2 table as used */
>              l2_offset &= L1E_OFFSET_MASK;
>              inc_refcounts(bs, res, refcount_table, refcount_table_size,
> @@ -1249,6 +1218,80 @@ fail:
>  }
>
>  /*
> + * Checks the OFLAG_COPIED flag for all L1 and L2 entries.
> + *
> + * This function does not print an error message nor does it increment
> + * check_errors if get_refcount fails (this is because such an error will have
> + * been already detected and sufficiently signaled by the calling function
> + * (qcow2_check_refcounts) by the time this function is called).
> + */
> +static int check_oflag_copied(BlockDriverState *bs, BdrvCheckResult *res)
> +{
> +    BDRVQcowState *s = bs->opaque;
> +    uint64_t *l2_table = qemu_blockalign(bs, s->cluster_size);
> +    int ret;
> +    int refcount;
> +    int i, j;
> +
> +    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;
> +
> +        if (!l2_offset) {
> +            continue;
> +        }
> +
> +        refcount = get_refcount(bs, l2_offset >> s->cluster_bits);
> +        if (refcount < 0) {
> +            /* don't print message nor increment check_errors */
> +            continue;
> +        }
> +        if ((refcount == 1) != ((l1_entry & QCOW_OFLAG_COPIED) != 0)) {
> +            fprintf(stderr, "ERROR OFLAG_COPIED L2 cluster: l1_index=%d "
> +                    "l1_entry=%" PRIx64 " refcount=%d\n",
> +                    i, l1_entry, refcount);
> +            res->corruptions++;
> +        }
> +
> +        ret = bdrv_pread(bs->file, l2_offset, l2_table,
> +                         s->l2_size * sizeof(uint64_t));
> +        if (ret < 0) {
> +            fprintf(stderr, "ERROR: Could not read L2 table: %s\n",
> +                    strerror(-ret));
> +            res->check_errors++;
> +            goto fail;
> +        }
> +
> +        for (j = 0; j < s->l2_size; j++) {
> +            uint64_t l2_entry = be64_to_cpu(l2_table[j]);
> +            uint64_t data_offset = l2_entry & L2E_OFFSET_MASK;
> +            int cluster_type = qcow2_get_cluster_type(l2_entry);
> +
> +            if ((cluster_type == QCOW2_CLUSTER_NORMAL) ||
> +                ((cluster_type == QCOW2_CLUSTER_ZERO) && (data_offset != 0))) {
> +                refcount = get_refcount(bs, data_offset >> s->cluster_bits);
> +                if (refcount < 0) {
> +                    /* don't print message nor increment check_errors */
> +                    continue;
> +                }
> +                if ((refcount == 1) != ((l2_entry & QCOW_OFLAG_COPIED) != 0)) {
> +                    fprintf(stderr, "ERROR OFLAG_COPIED data cluster: "
> +                            "l2_entry=%" PRIx64 " refcount=%d\n",
> +                            l2_entry, refcount);
> +                    res->corruptions++;
> +                }
> +            }
> +        }
> +    }
> +
> +    ret = 0;
> +
> +fail:
> +    qemu_vfree(l2_table);
> +    return ret;
> +}
> +
> +/*
>   * Checks an image for refcount consistency.
>   *
>   * Returns 0 if no errors are found, the number of errors in case the image is
> @@ -1383,6 +1426,12 @@ int qcow2_check_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
>          }
>      }
>
> +    /* check OFLAG_COPIED */
> +    ret = check_oflag_copied(bs, res);
> +    if (ret < 0) {
> +        goto fail;
> +    }
> +
>      res->image_end_offset = (highest_cluster + 1) * s->cluster_size;
>      ret = 0;
>
> --
> 1.8.1.4
>
>

Comments

Kevin Wolf Sept. 3, 2013, 7:21 a.m. UTC | #1 
Am 02.09.2013 um 15:52 hat Stefan Hajnoczi geschrieben:
> On Mon, Sep 2, 2013 at 10:49 AM, Kevin Wolf <kwolf@redhat.com> wrote:
> > From: Max Reitz <mreitz@redhat.com>
> >
> > Move the OFLAG_COPIED checks out of check_refcounts_l1 and
> > check_refcounts_l2 and after the actual refcount checks/fixes (since the
> > refcounts might actually change there).
> >
> > Signed-off-by: Max Reitz <mreitz@redhat.com>
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  block/qcow2-refcount.c | 115 +++++++++++++++++++++++++++++++++++--------------
> >  1 file changed, 82 insertions(+), 33 deletions(-)
> 
> This patch breaks qemu-iotests 039 as follows:

Anthony, please pull anyway.

This is just a missing update to the reference output of the test case
when run with cache=writethrough. Max sent a follow-up that Stefan will
include in the next pull request.

Kevin
diff mbox

Patch

--- 039.out    2013-09-02 13:31:26.134178859 +0200
+++ 039.out.bad    2013-09-02 15:48:00.103063126 +0200
@@ -12,8 +12,8 @@ 
 wrote 512/512 bytes at offset 0
 512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 incompatible_features     0x1
-ERROR OFLAG_COPIED: offset=8000000000050000 refcount=0
 ERROR cluster 5 refcount=0 reference=1
+ERROR OFLAG_COPIED data cluster: l2_entry=8000000000050000 refcount=0

 2 errors were found on the image.
 Data may be corrupted, or further writes to the image may corrupt it.
@@ -24,7 +24,6 @@ 
 incompatible_features     0x1

 == Repairing the image file must succeed ==
-ERROR OFLAG_COPIED: offset=8000000000050000 refcount=0
 Repairing cluster 5 refcount=0 reference=1
 The following inconsistencies were found and repaired:

@@ -44,7 +43,6 @@ 
 wrote 512/512 bytes at offset 0
 512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 incompatible_features     0x1
-ERROR OFLAG_COPIED: offset=8000000000050000 refcount=0
 Repairing cluster 5 refcount=0 reference=1
 wrote 512/512 bytes at offset 0
 512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)