[Top][All Lists]

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

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

From: Eric Blake
Subject: Re: [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

Attachment: signature.asc
Description: OpenPGP digital signature

reply via email to

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