Re: [Qemu-devel] [RFC] Using BlockdevRef in the block layer

[Max Reitz <mreitz@redhat.com>]

On 06.12.2013 12:14, Max Reitz wrote:
> On 06.12.2013 12:00, Max Reitz wrote:
>> On 06.12.2013 11:43, Kevin Wolf wrote:
>>> Am 05.12.2013 um 18:41 hat Max Reitz geschrieben:
>>>> When trying to implement this, I hit the problem that BlockdevRef
>>>> allows you to reference an existing block device; however, this
>>>> seems currently unimplemented. This is further hindered by the fact
>>>> how this reference is done: If you want to give a file to some
>>>> driver such as qcow2 (or blkdebug) which references an existing
>>>> block device, the option dict should look something like this:
>>>>
>>>> {
>>>> 'driver': 'blkdebug',
>>>> 'id': 'drive0-debug',
>>>> 'file': 'drive0'
>>>> }
>>>>
>>>> So, as you can see, the “file” value now is simply a string (this is
>>>> what distinguishes a reference from a "real" file, in which case it
>>>> would be a dict). The problem is that blockdev_init() already uses
>>>> this case for just specifying a filename without any further
>>>> options.
>>> Yes, -drive uses 'file' for the filename, but blockdev-add doesn't.
>>> Inside bdrv_open(), there is a pretty clear distinction between the 
>>> two:
>>> The legacy filename option of -drive is passed as a separate parameter
>>> and never as an options QDict entry. If the option comes from QMP,
>>> though, it will be in the QDict.
>>>
>>> /me checks the source code...
>>>
>>> Okay, that's not true today, even if it comes from QMP we treat it like
>>> the legacy option. We need to move the parsing of the 'file' option 
>>> from
>>> blockdev_init() to drive_init() to make it work this way. It's the 
>>> right
>>> thing to do anyway.
>>>
>>>> My current solution is to ignore the “file” value in case
>>>> blockdev_init() is called from qmp_blockdev_add(), but this doesn't
>>>> solve the general legacy issue. So, did I miss anything or is
>>>> referencing an existing block device really not supported so far and
>>>> the only meaning to the "file" option containing a pure string is
>>>> specifying a filename?
>>> It's not supported so far, but we want to have it.
>>>
>>> Ideally legacy option handling would be moved to drive_init() by
>>> conversion to the new data structures. This might not be entirely
>>> trivial with file names, though, so I think for now changing things
>>> within block_open() and friends is okay.
>>
>> Okay, this makes sense. I'll try my best. ;-)
>>
>>>> Second, if specifying a reference to an existing device should
>>>> really be supported, bdrv_open() should ideally not call
>>>> bdrv_file_open() anymore, but a function bdrv_find_ref() instead
>>>> which resolves a BlockdevRef structure (for simplicity, it appears
>>>> to be easier to use a QDict equivalent to a BlockdevRef instead of
>>>> the latter itself (since that results in many effectively redundant
>>>> conversions to and from those representations)).
>>> Agreed. Not sure about the function name (perhaps bdrv_open_ref is
>>> clearer?), but otherwise I like this design; perhaps the reason is that
>>> I suggested it myself earlier. ;-)
>>>
>>>> However,
>>>> bdrv_file_open() supports parsing protocol filenames, which
>>>> bdrv_find_ref() would not. As a result, it is probably best to call
>>>> bdrv_find_ref() from bdrv_file_open() instead and leave bdrv_open()
>>>> generally the way it is right now – yes, this is a question. ;-)
>>>> (“Do you agree?”)
>>> Perhaps what we need to do is to call bdrv_file_open() for the legacy
>>> case (filename passed as a separate parameter to bdrv_open()), and call
>>> bdrv_find_ref() when we have a 'file' QDict entry?
>>
>> Yes, that is what I referred to in my reply to the original RFC. 
>> However, it seems easier to just do everything in bdrv_file_open().
>>
>>>> Third, I planned to implement the blkdebug and blkverify QMP
>>>> interface by just making them BlockdevOptionsGenericFormat (with the
>>>> addition of a "test" BlockdevRef for blkverify). This will give them
>>>> a “file” automatically. However, this makes them “drivers for the
>>>> protocol level” (or however this is properly called), i.e., they
>>>> need to specify bdrv_open() instead of bdrv_file_open() to work. But
>>>> blkdebug and blkverify are their own protocols with the current
>>>> interface: Making them require an underlying file will break the
>>>> current interface with the filename specifying the required options.
>>>> To resolve this, I added two new “interfaces”, blkdebug-qmp and
>>>> blkverify-qmp, which reference the same functions as blkdebug and
>>>> blkverify, respectively, however, they offer bdrv_open() instead of
>>>> bdrv_file_open(). These new block drivers will thus not support the
>>>> current interface, but they will be properly supported through the
>>>> QMP interface.
>>> Hm... This is ugly.
>>>
>>> I'm not entirely sure I understand why we can't keep using
>>> bdrv_file_open() when inheriting from BlockdevOptionsGenericFormat.
>>> Granted, we wouldn't get bs->file automatially opened by
>>> bdrv_open_common() - but we don't today, and calling bdrv_file_open()
>>> manually from blkdebug_open() works just fine.
>>
>> Yes, we wouldn't get it opened automatically. I liked it being opened 
>> automatically. ;-)
>
> Ah, and what's worse, bdrv_file_open() attempts a bdrv_swap() which is 
> bound to fail. That means, we either have to disable bdrv_swap() on 
> some protocols – or we just don't call it "file" (i.e., don't let 
> BlockdevOptionsBlkdebug be based on BlockdevOptionsGenericFormat). I'd 
> prefer the second option, since then I don't even have to care about 
> nesting "file"s anymore. ;-)

Er, make that bdrv_open_common() instead of bdrv_file_open(), obviously.

Max