Re: [Qemu-devel] [PATCHv5] block: add native support for NFS

[Peter Lieven <pl@kamp.de>]

On 10.01.2014 12:40, Kevin Wolf wrote:
> Am 09.01.2014 um 17:08 hat Peter Lieven geschrieben:
>> Am 09.01.2014 15:13, schrieb Kevin Wolf:
>>> Am 26.12.2013 um 13:48 hat Peter Lieven geschrieben:
>>>> v4->v5:
>>>>   - disussed with Ronnie and decided to move URL + Paramter parsing to LibNFS.
>>>>     This allows for URL parameter processing directly in LibNFS without altering
>>>>     the qemu NFS block driver. This bumps the version requirement for LibNFS
>>>>     to 1.9.0 though.
>>> Considering that we'll likely want to add new options in the future, I'm
>>> not sure if this is a good idea. This means that struct nfs_url will
>>> change, and if qemu isn't updated, it might not even notice that some
>>> option was requested in a new field that it doesn't know and provide,
>>> so it will silently ignore it. Or if we have a qemu built against an
>>> older libnfs, this must be marked as an incompatible ABI change, so it
>>> can't run at all with the newer libnfs version.
>> Maybe we are talking about differnt things here. The paramteres/options
>> we were talking about are options to libnfs not to qemu. This could be
>> e.g. the uid or the protocol version. This is nothing qemu has to care about.
>> The nfs_url struct is likely not to change and even if it would change
>> I see no problem as long as we do only extend the struct and do not change
>> to position of the server, export and file.
> The problem is that qemu doesn't treat nfsurl as a black box. It calls
> the libnfs function to parse a URL into the struct, and then it accesses
> the individual fields of struct nfsurl to pass them to several libnfs
> functions.
libnfs feeds itself when the url is parsed. this way qemu does not
need to be recompiled or adjusted any time when there is a new
option to libnfs. the options that are specified here are only meaningful
to the NFS transport itself nothing qemu has to care about.
>
> If struct nfsurl is extended, qemu needs to learn to feed the new fields
> to libnfs functions, otherwise they'll be silently ignored.
>
> Just think it through what happens after adding a new option with the
> combinations of an old qemu binary run with a new libnfs, and a new qemu
> binary run with the old libnfs. You'll come to the conclusion that the
> ABI is incompatible both ways.
we only have a pointer to the struct so the size doesn't matter. as
long as the expected fields are at the right positions I do not see the issue.
>
>>>> +static QemuOptsList runtime_opts = {
>>>> +    .name = "nfs",
>>>> +    .head = QTAILQ_HEAD_INITIALIZER(runtime_opts.head),
>>>> +    .desc = {
>>>> +        {
>>>> +            .name = "filename",
>>>> +            .type = QEMU_OPT_STRING,
>>>> +            .help = "URL to the NFS file",
>>>> +        },
>>>> +        { /* end of list */ }
>>>> +    },
>>>> +};
>>> I think this is the point where I disagree. First of all, if it's a URL,
>>> call it "url" instead of "filename". But more importantly, a URL encodes
>>> options in a string instead of structured options that can be set
>>> separately.
>> Thats exactly what I meant above: The options are encoded in the
>> string, e.g.
>>
>> nfs://server/export/file?uid=1000&gid=1000
>>
>> Thats what libnfs expects. But all the options are not important for qemu.
> This means that we only expose one string option to QMP clients. There
> is no way for them to figure out which options are valid, even with a
> QAPI schema. This will be a real problem for libvirt support for libnfs.
Which option is available is highly depending on the libnfs version.
Currently libnfs parses the options and ignores them if they are unknown.
Like it or not that is how it is designed.
>
> It also means that you can't override single options of a libnfs backing
> file on the command line, though I suppose that is a minor use case.
>
>> "filename" was copied from block/iscsi.c. The semantic is exactly
>> the same. It accepts an URL and all the paramter parsing is
>> done inside libiscsi with iscsi_parse_url_full.
> Yup, and the part that you didn't copy is:
>
>      /* TODO Convert to fine grained options */
>
> The iscsi block driver is still using the legacy interface and pending
> conversion. Doesn't mean that this is a reason for a new block driver to
> not do it right from the beginning.
Sorry, I cannot see from the docs or existing drivers how to do this.
All protocols that use URLs are pending regarding to the conversion.
To be honest it starts to getting a little bit frustrating. I wanted to share
this protocol extension I developed to get around some issues with
disappearing NFS shares we experienced. I heard that anything is fine and then
suddenly it had to run through qemu-iotests. Ok, I looked at it
and found that most of the tests only work properly with the
file protocol altough they are tagged generic. I fixed that or proposed a fix.
Now, I shall start over and change the parsing that was changed
upon the wish of the libnfs author and that works well for me.
Then I shall convert everything to a qapi schema whereby the current
design of libnfs is designed to work with plain URLs.

I currently do not have resources to look further into this. If the driver
does not meet the requirements and this can't be adjusted with little
changes than we have to leave it out for the moment.

Peter