qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Peter Lieven
Subject: Re: [Qemu-devel] [PATCHv5] block: add native support for NFS
Date: Fri, 10 Jan 2014 13:12:06 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.2.0

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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]