[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Eric Blake
Subject: Re: [Qemu-block] [PATCH v4 12/21] mirror: Switch mirror_do_read() to byte-based
Date: Thu, 6 Jul 2017 09:25:14 -0500
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.1

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

>> -    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: signature.asc
Description: OpenPGP digital signature

reply via email to

[Prev in Thread] Current Thread [Next in Thread]