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

[Vladimir Sementsov-Ogievskiy]

06.05.2019 20:06, Eric Blake wrote:
> On 5/6/19 11:47 AM, Vladimir Sementsov-Ogievskiy wrote:
>> 01.05.2019 21:13, Alberto Garcia wrote:
>>> There's only a couple of bdrv_read() and bdrv_write() calls left in
>>> the vvfat code, and they can be trivially replaced with the byte-based
>>> bdrv_pread() and bdrv_pwrite().
>>>
>>> Signed-off-by: Alberto Garcia <address@hidden>
>>> ---
>>>    block/vvfat.c | 12 +++++++-----
>>>    1 file changed, 7 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/block/vvfat.c b/block/vvfat.c
>>> index 5f66787890..253cc716dd 100644
>>> --- a/block/vvfat.c
>>> +++ 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.

-- 
Best regards,
Vladimir