[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 00/17] block: Convert common I/O path to BdrvChi
From: |
Paolo Bonzini |
Subject: |
Re: [Qemu-devel] [PATCH 00/17] block: Convert common I/O path to BdrvChild |
Date: |
Tue, 21 Jun 2016 13:01:30 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.1.1 |
On 21/06/2016 12:56, Kevin Wolf wrote:
> Am 21.06.2016 um 11:47 hat Paolo Bonzini geschrieben:
>>
>>
>> On 21/06/2016 11:21, Kevin Wolf wrote:
>>> This series converts all I/O function in the core block layer up to
>>> bdrv_co_preadv/pwritev() to taking a BdrvChild as their first parameter
>>> instead of a BlockDriverState.
>>>
>>> The original motivation for this change were op blockers, where one of
>>> the biggest problems is making sure that every user of block devices
>>> actually registers correctly with the op blockers system. If the I/O
>>> functions know which parent a request comes from (BdrvChild basically
>>> corresponds to an edge in our block device graph), it can use assertions
>>> to make sure that that parent has actually registered its activities and
>>> thereby ensured that it doesn't conflict with other users.
>>>
>>> There are, however, more benefits we get from this change. The most
>>> important one is probably that it enforces important aspects of the
>>> block layer design like that external users go through a BlockBackend
>>> and request are internally routed along the edges of the graph. Accesses
>>> to random BDSes are no longer possible, you need to own an actual child
>>> reference so you can make a request.
>>
>> I still fail to understand what is the rationale for this change. The
>> API is weird; you read from a disk, not from an edge, and in fact the
>> first thing all the APIs do is dereference the BdrvChild...
>>
>> The assertions are nice, but I'm not sure it's a good idea to design a
>> whole API around them.
>
> Do you see a problem with such an API, though? If there is no reason not
> to have the advantages, as small as they may seem, why not take them?
I don't see a reason not to take them; I don't see any red flags, but
there are some yellow flags (the kinda weird API) that I don't
understand and I hope you can explain.
Thinking more about it, it's perfectly possible that this is just a
combination of block/io.c's growth by accretion and the well-known fact
"naming pseudo-OOP member functions in C sucks".
In other words, if you sell me this as "let's add some member functions
to BdrvChild and use them", I can buy it. Perhaps the only thing to do
then is to rename functions and design a consistent naming.
Paolo
- Re: [Qemu-devel] [PATCH 16/17] block: Convert bdrv_prwv_co() to BdrvChild, (continued)
- [Qemu-devel] [PATCH 13/17] block: Convert bdrv_pread(v) to BdrvChild, Kevin Wolf, 2016/06/21
- [Qemu-devel] [PATCH 17/17] block: Convert bdrv_co_preadv/pwritev to BdrvChild, Kevin Wolf, 2016/06/21
- [Qemu-devel] [PATCH 15/17] block: Convert bdrv_pwrite_zeroes() to BdrvChild, Kevin Wolf, 2016/06/21
- [Qemu-devel] [PATCH 14/17] block: Convert bdrv_pwrite(v/_sync) to BdrvChild, Kevin Wolf, 2016/06/21
- Re: [Qemu-devel] [PATCH 00/17] block: Convert common I/O path to BdrvChild, Paolo Bonzini, 2016/06/21
Re: [Qemu-devel] [PATCH 00/17] block: Convert common I/O path to BdrvChild, Stefan Hajnoczi, 2016/06/28