[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
signature.asc
Description: OpenPGP digital signature
- [Qemu-block] [PATCH v4 02/21] trace: Show blockjob actions via bytes, not sectors, (continued)
- [Qemu-block] [PATCH v4 02/21] trace: Show blockjob actions via bytes, not sectors, Eric Blake, 2017/07/05
- [Qemu-block] [PATCH v4 08/21] mirror: Switch MirrorBlockJob to byte-based, Eric Blake, 2017/07/05
- [Qemu-block] [PATCH v4 09/21] mirror: Switch mirror_do_zero_or_discard() to byte-based, Eric Blake, 2017/07/05
- [Qemu-block] [PATCH v4 10/21] mirror: Update signature of mirror_clip_sectors(), Eric Blake, 2017/07/05
- [Qemu-block] [PATCH v4 11/21] mirror: Switch mirror_cow_align() to byte-based, Eric Blake, 2017/07/05
- [Qemu-block] [PATCH v4 12/21] mirror: Switch mirror_do_read() to byte-based, Eric Blake, 2017/07/05
- [Qemu-block] [PATCH v4 13/21] mirror: Switch mirror_iteration() to byte-based, Eric Blake, 2017/07/05
- [Qemu-block] [PATCH v4 14/21] block: Drop unused bdrv_round_sectors_to_clusters(), Eric Blake, 2017/07/05
- [Qemu-block] [PATCH v4 16/21] backup: Switch block_backup.h to byte-based, Eric Blake, 2017/07/05
- [Qemu-block] [PATCH v4 15/21] backup: Switch BackupBlockJob to byte-based, Eric Blake, 2017/07/05
- [Qemu-block] [PATCH v4 17/21] backup: Switch backup_do_cow() to byte-based, Eric Blake, 2017/07/05