Re: [Qemu-devel] [PATCH v2 3/5] vvfat: Replace bdrv_{read, write}() with bdrv_{pread, pwrite}()

[Eric Blake <eblake@redhat.com>]

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

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


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