Re: [Qemu-devel] [PATCH v4 04/11] block: Inline bdrv_co_block_status_from_*()

[Max Reitz <mreitz@redhat.com>]

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

On 21.05.19 10:57, Vladimir Sementsov-Ogievskiy wrote:
> 10.04.2019 23:20, Max Reitz wrote:
>> With bdrv_filtered_rw_bs(), we can easily handle this default filter
>> behavior in bdrv_co_block_status().
>>
>> blkdebug wants to have an additional assertion, so it keeps its own
>> implementation, except bdrv_co_block_status_from_file() needs to be
>> inlined there.
>>
>> Suggested-by: Eric Blake <eblake@redhat.com>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>   include/block/block_int.h | 22 -----------------
>>   block/blkdebug.c          |  7 ++++--
>>   block/blklogwrites.c      |  1 -
>>   block/commit.c            |  1 -
>>   block/copy-on-read.c      |  2 --
>>   block/io.c                | 51 +++++++++++++--------------------------
>>   block/mirror.c            |  1 -
>>   block/throttle.c          |  1 -
>>   8 files changed, 22 insertions(+), 64 deletions(-)

[...]

>> diff --git a/block/io.c b/block/io.c
>> index 5c33ecc080..8d124bae5c 100644
>> --- a/block/io.c
>> +++ b/block/io.c

[...]

>> @@ -2088,7 +2059,8 @@ static int coroutine_fn bdrv_co_block_status(BlockDriverState *bs,
>>   
>>       /* Must be non-NULL or bdrv_getlength() would have failed */
>>       assert(bs->drv);
>> -    if (!bs->drv->bdrv_co_block_status) {
>> +    has_filtered_child = bs->drv->is_filter && bdrv_filtered_rw_child(bs);
>> +    if (!bs->drv->bdrv_co_block_status && !has_filtered_child) {
>>           *pnum = bytes;
>>           ret = BDRV_BLOCK_DATA | BDRV_BLOCK_ALLOCATED;
>>           if (offset + bytes == total_size) {
>> @@ -2109,9 +2081,20 @@ static int coroutine_fn bdrv_co_block_status(BlockDriverState *bs,
>>       aligned_offset = QEMU_ALIGN_DOWN(offset, align);
>>       aligned_bytes = ROUND_UP(offset + bytes, align) - aligned_offset;
>>   
>> -    ret = bs->drv->bdrv_co_block_status(bs, want_zero, aligned_offset,
>> -                                        aligned_bytes, pnum, &local_map,
>> -                                        &local_file);
>> +    if (bs->drv->bdrv_co_block_status) {
>> +        ret = bs->drv->bdrv_co_block_status(bs, want_zero, aligned_offset,
>> +                                            aligned_bytes, pnum, &local_map,
>> +                                            &local_file);
>> +    } else {
>> +        /* Default code for filters */
>> +
>> +        local_file = bdrv_filtered_rw_bs(bs);
>> +        assert(local_file);
>> +
>> +        *pnum = aligned_bytes;
>> +        local_map = aligned_offset;
>> +        ret = BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID;
>> +    }
> 
> 
> preexistent, but why default for filters is aligned and for other nodes is not?

I suppose because the default code for other nodes has been written
before the aligning code was introduced.

I guess there is no good reason to enforce alignment in either case.  It
is important to do so when issuing a request to the driver because the
driver is not required to be able to handle unaligned requests.  If we
completely forgo the driver and just go through to the next layer, it
doesn’t really matter, I think.

Well, I just kept it as it was before. O:-)

Max


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