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 <[email protected]> >> Reviewed-by: John Snow <[email protected]> >> Reviewed-by: Jeff Cody <[email protected]> > >> - /* 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)). >> - 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. 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. 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). > > The rest of the patch looks okay. > > Kevin > -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature
