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

[Kevin Wolf]

Am 06.07.2017 um 16:25 hat Eric Blake geschrieben:
> On 07/06/2017 08:30 AM, Kevin Wolf wrote:
> > 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));
> 
> So the strict translation would be assert(!(offset % s->granularity)),
> or rewritten to be assert(QEMU_IS_ALIGNED(offset, s->granularity)).

Right.

> >> -    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));
> 
> and you're right that this appears to be a new assertion.
> 
> >> +    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.
> 
> At this point, I'm not sure if I confused things, or if I hit an actual
> iotest failure later in the series that I traced back to this point.

But if you did, wouldn't this indicate a real bug in the patch?

> If I have a reason to respin, I'll see if both assertions still hold
> (the rewritten alignment check on offset, and the new check on bytes
> being sector-aligned), through all the tests.

Actually, I think this one, while seemingly minor, is a reason to
respin. This kind of assertions is exactly for preventing bugs when
doing changes like this patch does, so removing the assertion in such a
patch looks really suspicious.

Also, with the new assertion, the comment doesn't really make that much
sense any more either.

> Also, while asserting that bytes is sector-aligned is good in the
> short term, it may be overkill by the time dirty-bitmap is changed to
> byte alignment (right now, bdrv_getlength() is always sector-aligned,
> because it rounds up; but if we ever make it byte-accurate, then the
> trailing cluster might not be a sector-aligned bytes length).

This is true, but I think for the time being the assertion is worth it.

Kevin

pgpik0pgVTKiT.pgp
Description: PGP signature