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

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2611 bytes --]

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 <eblake@redhat.com>
>> Reviewed-by: John Snow <jsnow@redhat.com>
>> Reviewed-by: Jeff Cody <jcody@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)).

>> -    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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]