[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [Qemu-devel] [PATCH v2 3/5] vvfat: Replace bdrv_{read,
From: |
Eric Blake |
Subject: |
Re: [Qemu-block] [Qemu-devel] [PATCH v2 3/5] vvfat: Replace bdrv_{read, write}() with bdrv_{pread, pwrite}() |
Date: |
Mon, 6 May 2019 12:39:30 -0500 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.6.1 |
On 5/6/19 12:19 PM, Vladimir Sementsov-Ogievskiy wrote:
>>>> +++ b/block/vvfat.c
>>>> @@ -1494,8 +1494,8 @@ static int vvfat_read(BlockDriverState *bs, int64_t
>>>> sector_num,
>>>> DLOG(fprintf(stderr, "sectors %" PRId64 "+%" PRId64
>>>> " allocated\n", sector_num,
>>>> n >> BDRV_SECTOR_BITS));
>>>> - if (bdrv_read(s->qcow, sector_num, buf + i * 0x200,
>>>> - n >> BDRV_SECTOR_BITS)) {
>>>> + if (bdrv_pread(s->qcow, sector_num * BDRV_SECTOR_SIZE,
>>>> + buf + i * 0x200, n) < 0) {
>>>
>>> Shouldn't we use QEMU_ALIGN_DOWN(n, BDRV_SECTOR_SIZE) ?
>>
>> No, n should already be aligned, which makes align_down a no-op.
>>
>>> Could bdrv_is_allocated give unaligned n?
>>>
>>
>> Yes, bdrv_is_allocated can return unaligned n in some situations; I had
>> a patch that didn't make 4.0 that would add bdrv_block_status_aligned
>> for cases where we need to guarantee that different alignment of a
>> backing chain doesn't bleed through to the specified alignment of the
>> current layer. But those situations are rare, and I need to revisit
>> those and send a v2; so I don't see a problem with this one going in
>> during the meantime as-is.
>>
>
> Than, n is not already aligned, as it comes from bdrv_is_allocated.
Note that whether bdrv_is_allocated can return data not aligned to 512
depends on the driver. It is possible when querying file-posix.c, but
only for a POSIX file that encounters EOF mid-sector. However, it is not
possible for the qcow2 driver. The patches I need to rework are worried
more about cases where a block device with request_alignment of 4k can
still see 512-alignment leak through from a backing file. But since
vvfat is grabbing alignment from a qcow2 image, and not a raw POSIX
file, we should never see sub-sector alignment.
So my answers above were terse but correct: bdrv_is_allocated can return
unaligned data in some cases, but vvfat should not be one of those
cases. If you'd like to add an assert instead of a QEMU_ALIGN_DOWN, that
should be reasonable.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature
- Re: [Qemu-block] [Qemu-devel] [PATCH v2 4/5] block: Remove bdrv_read() and bdrv_write(), (continued)
- [Qemu-block] [PATCH v2 5/5] qcow2: Remove BDRVQcow2State.cluster_sectors, Alberto Garcia, 2019/05/01
- [Qemu-block] [PATCH v2 2/5] vdi: Replace bdrv_{read, write}() with bdrv_{pread, pwrite}(), Alberto Garcia, 2019/05/01
- [Qemu-block] [PATCH v2 3/5] vvfat: Replace bdrv_{read, write}() with bdrv_{pread, pwrite}(), Alberto Garcia, 2019/05/01
- Re: [Qemu-block] [Qemu-devel] [PATCH v2 3/5] vvfat: Replace bdrv_{read, write}() with bdrv_{pread, pwrite}(), Vladimir Sementsov-Ogievskiy, 2019/05/06
- Re: [Qemu-block] [Qemu-devel] [PATCH v2 3/5] vvfat: Replace bdrv_{read, write}() with bdrv_{pread, pwrite}(), Eric Blake, 2019/05/06
- Re: [Qemu-block] [Qemu-devel] [PATCH v2 3/5] vvfat: Replace bdrv_{read, write}() with bdrv_{pread, pwrite}(), Vladimir Sementsov-Ogievskiy, 2019/05/06
- Re: [Qemu-block] [Qemu-devel] [PATCH v2 3/5] vvfat: Replace bdrv_{read, write}() with bdrv_{pread, pwrite}(),
Eric Blake <=
Re: [Qemu-block] [PATCH v2 0/5] Remove bdrv_read() and bdrv_write(), Kevin Wolf, 2019/05/08