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 <ebl...@redhat.com> > >> Reviewed-by: John Snow <js...@redhat.com> > >> Reviewed-by: Jeff Cody <jc...@redhat.com> > > > >> - /* 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
pgpauBOhz1QHh.pgp
Description: PGP signature
