Re: [Qemu-block] [PATCH v4 12/21] mirror: Switch mirror_do_read() to byte-based

[Kevin Wolf]

Am 05.07.2017 um 23:08 hat Eric Blake geschrieben:
> We are gradually converting to byte-based interfaces, as they are
> easier to reason about than sector-based.  Convert another internal
> function (no semantic change).
> 
> Signed-off-by: Eric Blake <address@hidden>
> Reviewed-by: John Snow <address@hidden>
> Reviewed-by: Jeff Cody <address@hidden>

> -    /* The sector range must meet granularity because:
> +    assert(bytes <= s->buf_size);
> +    /* The range will be sector-aligned because:
>       * 1) Caller passes in aligned values;
> -     * 2) mirror_cow_align is used only when target cluster is larger. */
> -    assert(!(sector_num % sectors_per_chunk));
> -    nb_chunks = DIV_ROUND_UP(nb_sectors, sectors_per_chunk);
> +     * 2) mirror_cow_align is used only when target cluster is larger.
> +     * But it might not be cluster-aligned at end-of-file. */
> +    assert(QEMU_IS_ALIGNED(bytes, BDRV_SECTOR_SIZE));
> +    nb_chunks = DIV_ROUND_UP(bytes, s->granularity);

The assertion in the old code was about sector_num (i.e.  that the start
of the range is cluster-aligned), not about nb_sectors, so while you add
a new assertion, it is asserting something different. This explains
why you had to switch to sector aligned even though the semantics
shouldn't be changed by this patch.

Is this intentional or did you confuse sector_num and nb_sectors? I
think we can just have both assertions.

The rest of the patch looks okay.

Kevin