[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH v6 09/20] parallels: Switch to .bdrv_co_block_st
From: |
Eric Blake |
Subject: |
Re: [Qemu-block] [PATCH v6 09/20] parallels: Switch to .bdrv_co_block_status() |
Date: |
Sat, 9 Dec 2017 10:39:45 -0600 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 |
On 12/09/2017 06:31 AM, Vladimir Sementsov-Ogievskiy wrote:
> 07.12.2017 23:30, Eric Blake wrote:
>> We are gradually moving away from sector-based interfaces, towards
>> byte-based. Update the parallels driver accordingly. Note that
>> the internal function block_status() is still sector-based, because
>> it is still in use by other sector-based functions; but that's okay
>> because request_alignment is 512 as a result of those functions.
>> For now, no optimizations are added based on the mapping hint.
>>
>> Signed-off-by: Eric Blake <address@hidden>
>>
>> {
>> BDRVParallelsState *s = bs->opaque;
>> - int64_t offset;
>> + int count;
>>
>> + assert(QEMU_IS_ALIGNED(offset | bytes, BDRV_SECTOR_SIZE));
>> qemu_co_mutex_lock(&s->lock);
>> - offset = block_status(s, sector_num, nb_sectors, pnum);
>> + offset = block_status(s, offset >> BDRV_SECTOR_BITS,
>> + bytes >> BDRV_SECTOR_BITS, &count);
>> qemu_co_mutex_unlock(&s->lock);
>>
>> + *pnum = count * BDRV_SECTOR_SIZE;
>> if (offset < 0) {
>> return 0;
>> }
>>
>> + *map = offset;
>
> oh, didn't notice previous time :(, should be
>
> *map = offset * BDRV_SECTOR_SIZE
>
Oh, right.
> (also, if you already use ">> BDRV_SECTOR_BITS" in this function,
> would not it be more consistent to use "<< BDRV_SECTOR_BITS"
> for sector-to-byte conversion?)
>
> with that fixed (with "<<" or "*", up to you),
'>> BDRV_SECTOR_BITS' always works. You could also write '/
BDRV_SECTOR_SIZE' (the compiler should figure out that you are dividing
by a power of 2, and use the shift instruction under the hood), but I
find that a bit harder to reason about.
On the other hand, '<< BDRV_SECTOR_BITS' only produces the same size
output as the input; if the left side is 32 bits, it risks overflowing.
But '* BDRV_SECTOR_SIZE' always produces a 64-bit value. So I've
learned (from past mistakes in other byte-conversion series) that the
multiply form is less likely to introduce unintended truncation bugs.
> Reviewed-by: Vladimir Sementsov-Ogievskiy <address@hidden>
>
--
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 v6 05/20] iscsi: Switch cluster_sectors to byte-based, (continued)
- [Qemu-block] [PATCH v6 05/20] iscsi: Switch cluster_sectors to byte-based, Eric Blake, 2017/12/07
- [Qemu-block] [PATCH v6 07/20] iscsi: Switch to .bdrv_co_block_status(), Eric Blake, 2017/12/07
- [Qemu-block] [PATCH v6 06/20] iscsi: Switch iscsi_allocmap_update() to byte-based, Eric Blake, 2017/12/07
- [Qemu-block] [PATCH v6 08/20] null: Switch to .bdrv_co_block_status(), Eric Blake, 2017/12/07
- [Qemu-block] [PATCH v6 09/20] parallels: Switch to .bdrv_co_block_status(), Eric Blake, 2017/12/07
[Qemu-block] [PATCH v6 10/20] qcow: Switch to .bdrv_co_block_status(), Eric Blake, 2017/12/07
[Qemu-block] [PATCH v6 11/20] qcow2: Switch to .bdrv_co_block_status(), Eric Blake, 2017/12/07
[Qemu-block] [PATCH v6 12/20] qed: Switch to .bdrv_co_block_status(), Eric Blake, 2017/12/07