Re: [PATCH v2 1/4] qemu-img: implement compare --stat

[Vladimir Sementsov-Ogievskiy]

25.10.2021 19:40, Hanna Reitz wrote:
> On 21.10.21 12:12, Vladimir Sementsov-Ogievskiy wrote:
> > With new option qemu-img compare will not stop at first mismatch, but
> > instead calculate statistics: how many clusters with different data,
> > how many clusters with equal data, how many clusters were unallocated
> > but become data and so on.
> >
> > We compare images chunk by chunk. Chunk size depends on what
> > block_status returns for both images. It may return less than cluster
> > (remember about qcow2 subclusters), it may return more than cluster (if
> > several consecutive clusters share same status). Finally images may
> > have different cluster sizes. This all leads to ambiguity in how to
> > finally compare the data.
> >
> > What we can say for sure is that, when we compare two qcow2 images with
> > same cluster size, we should compare clusters with data separately.
> > Otherwise, if we for example compare 10 consecutive clusters of data
> > where only one byte differs we'll report 10 different clusters.
> > Expected result in this case is 1 different cluster and 9 equal ones.
> >
> > So, to serve this case and just to have some defined rule let's do the
> > following:
> >
> > 1. Select some block-size for compare procedure. In this commit it must
> >     be specified by user, next commit will add some automatic logic and
> >     make --block-size optional.
> >
> > 2. Go chunk-by-chunk using block_status as we do now with only one
> >     differency:
> >     If block_status() returns DATA region that intersects block-size
> >     aligned boundary, crop this region at this boundary.
> >
> > This way it's still possible to compare less than cluster and report
> > subcluster-level accuracy, but we newer compare more than one cluster
> > of data.
> >
> > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> > ---
> >   docs/tools/qemu-img.rst |  18 +++-
> >   qemu-img.c              | 206 +++++++++++++++++++++++++++++++++++++---
> >   qemu-img-cmds.hx        |   4 +-
> >   3 files changed, 212 insertions(+), 16 deletions(-)
>
> Looks good to me overall!  Just some technical comments below.
>
> > diff --git a/docs/tools/qemu-img.rst b/docs/tools/qemu-img.rst
> > index d58980aef8..21164253d4 100644
> > --- a/docs/tools/qemu-img.rst
> > +++ b/docs/tools/qemu-img.rst
> > @@ -159,6 +159,18 @@ Parameters to compare subcommand:
> >     Strict mode - fail on different image size or sector allocation
> > +.. option:: --stat
> > +
> > +  Instead of exit on first mismatch compare the whole images and print
> > +  statistics on amount of different pairs of clusters, based on their
> > +  block-status and are they equal or not.
>
> I’d phrase this as:
>
> Instead of exiting on the first mismatch, compare the whole images and print 
> statistics on how much they differ in terms of block status (i.e. are blocks 
> allocated or not, do they contain data, are they marked as containing only 
> zeroes) and block content (a block of data that contains only zero still has 
> the same content as a marked-zero block).

For me the rest starting from "and block content" sounds unclear, seems doesn't 
add any information to previous (i.e. are blocks allocated ...)

> > +
> > +.. option:: --block-size BLOCK_SIZE
> > +
> > +  Block size for comparing with ``--stat``. This doesn't guarantee exact
> > +  size of comparing chunks, but that guarantee that data chunks being
> > +  compared will never cross aligned block-size boundary.
>
> I’d do just some minor tweaks to the second sentence:
>
> This doesn't guarantee an exact size for comparing chunks, but it does 
> guarantee that those chunks will never cross a block-size-aligned boundary.

OK, sounds good

> > +
> >   Parameters to convert subcommand:
> >   .. program:: qemu-img-convert
>
> [...]
>
> > diff --git a/qemu-img.c b/qemu-img.c
> > index f036a1d428..79a0589167 100644
> > --- a/qemu-img.c
> > +++ b/qemu-img.c
> > @@ -83,6 +83,8 @@ enum {
> >       OPTION_BITMAPS = 275,
> >       OPTION_FORCE = 276,
> >       OPTION_SKIP_BROKEN = 277,
> > +    OPTION_STAT = 277,
>
> That doesn’t look ideal, I believe `OPTION_STAT` should have a different value 
> than `OPTION_SKIP_BROKEN`.  (I guess a rebase is to blame?)

Oops

> > +    OPTION_BLOCK_SIZE = 278,
> >   };
> >   typedef enum OutputFormat {
> > @@ -1304,6 +1306,107 @@ static int check_empty_sectors(BlockBackend *blk, 
> > int64_t offset,
> >       return 0;
> >   }
> > +#define IMG_CMP_STATUS_MASK (BDRV_BLOCK_DATA | BDRV_BLOCK_ZERO | \
> > +                             BDRV_BLOCK_ALLOCATED)
> > +#define IMG_CMP_STATUS_MAX (IMG_CMP_STATUS_MASK | BDRV_BLOCK_EOF)
> > +
> > +typedef struct ImgCmpStat {
> > +    /* stat: [ret: 0 is equal, 1 is not][status1][status2] -> n_bytes */
> > +    uint64_t stat[2][IMG_CMP_STATUS_MAX + 1][IMG_CMP_STATUS_MAX + 1];
>
> `IMG_CMP_STATUS_MAX` isn’t packed tightly because it only has four bits set 
> (0x33).  That in itself isn’t a problem, but it means that `IMG_CMP_STATUS_MAX 
> + 1` is 52, and so this array’s size is 52 * 52 * 2 * sizeof(uint64_t) = 43264. 
>  Again, that isn’t a problem in itself (although it is a bit sad that this 
> could fit into 16 * 16 * 2 * 8 = 4 kB), but in `img_compare()` [1], you put 
> this structure on the stack, and I believe it’s too big for that.

Hmm. May be, it's better just use GHashTables and don't bother with these 
sparse arrays

> > +} ImgCmpStat;
>
> [...]
>
> > +static void cmp_stat_print_agenda(void)
> > +{
> > +    printf("Compare stats:\n"
> > +           "Agenda\n"
>
> I’m more used to the term “Key” for this, but my experience is mostly limited 
> to the git-backport-diff output, so it’s not that strong.

I don't care, "key" is OK for me.

> > +           "D: DATA\n"
> > +           "Z: ZERO\n"
> > +           "A: ALLOCATED\n"
> > +           "E: after end of file\n\n");
> > +}
>
> [...]
>
> > @@ -1331,6 +1436,9 @@ static int img_compare(int argc, char **argv)
> >       uint64_t progress_base;
> >       bool image_opts = false;
> >       bool force_share = false;
> > +    ImgCmpStat stat = {0};
>
> [1] (Here is where `ImgCmpStat` goes on the stack, which it shouldn’t, 
> considering it’s over 40 kB in size.)
>
> > +    bool do_stat;
> > +    int64_t block_size = 0;
> >       cache = BDRV_DEFAULT_CACHE;
> >       for (;;) {
>
> [...]
>
> > @@ -1395,6 +1505,15 @@ static int img_compare(int argc, char **argv)
> >           case OPTION_IMAGE_OPTS:
> >               image_opts = true;
> >               break;
> > +        case OPTION_STAT:
> > +            do_stat = true;
> > +            break;
> > +        case OPTION_BLOCK_SIZE:
> > +            block_size = cvtnum_full("block size", optarg, 1, INT64_MAX);
> > +            if (block_size < 0) {
> > +                exit(EXIT_SUCCESS);
>
> Shouldn’t this be 2 instead of `EXIT_SUCCESS`?

Oops, right, it should be an error

> (Above for --object we use `EXIT_SUCCESS`, but only for the case where a help text 
> was printed.  When parsing fails, we exit with 2, which is documented as the error 
> code (“>1”).)
>
> > +            }
> > +            break;
> >           }
> >       }
> > @@ -1410,6 +1529,20 @@ static int img_compare(int argc, char **argv)
> >       filename1 = argv[optind++];
> >       filename2 = argv[optind++];
> > +    if (!do_stat && block_size) {
> > +        error_report("--block-size can be used only together with --stat");
> > +        ret = 1;
>
> 2 is the error code, 1 means “success, but images differ”.

Will fix

> > +        goto out;
>
> The `out` label frees `buf1` and `buf2`, and unrefs `blk1` and `blk2`. My gcc 
> complains that `blk1` and `blk2` have not been initialized by this point, 
> though.  I believe we should go to `out3` here.
>
> > +    }
> > +
> > +    if (do_stat && !block_size) {
> > +        /* TODO: make block-size optional */
> > +        error_report("You must specify --block-size together with --stat");
> > +        ret = 1;
> > +        goto out;
>
> Same here.
>
> > +    }
> > +
> > +
> >       /* Initialize before goto out */
> >       qemu_progress_init(progress, 2.0);
>
> [...]
>
> > @@ -1486,11 +1623,15 @@ static int img_compare(int argc, char **argv)
> >               goto out;
> >           }
> >           allocated2 = status2 & BDRV_BLOCK_ALLOCATED;
> > +        if (do_stat && (status2 & BDRV_BLOCK_DATA)) {
> > +            /* Don't compare cross-block data */
> > +            pnum2 = MIN(block_end, offset + pnum2) - offset;
> > +        }
> >           assert(pnum1 && pnum2);
> >           chunk = MIN(pnum1, pnum2);
> > -        if (strict) {
> > +        if (strict && !do_stat) {
>
> Question is, do we want to allow strict mode together with --stat at all?  I 
> think I’d prefer making it an outright error.

Hmm. I think, I tend to agree, will do.

> >               if (status1 != status2) {
> >                   ret = 1;
> >                   qprintf(quiet, "Strict mode: Offset %" PRId64


Thanks!

--
Best regards,
Vladimir