qemu-devel
[Top][All Lists]
Advanced

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

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


From: Peter Lieven
Subject: Re: [Qemu-devel] [PATCHv2] block: add native support for NFS
Date: Tue, 17 Dec 2013 18:03:48 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.2.0

On 17.12.2013 17:47, Stefan Hajnoczi wrote:
On Tue, Dec 17, 2013 at 10:15:25AM +0100, Peter Lieven wrote:
This patch adds native support for accessing images on NFS shares without
the requirement to actually mount the entire NFS share on the host.

NFS Images can simply be specified by an url of the form:
nfs://<host>/<export>/<filename>

For example:
qemu-img create -f qcow2 nfs://10.0.0.1/qemu-images/test.qcow2

You need libnfs from Ronnie Sahlberg available at:
    git://github.com/sahlberg/libnfs.git
for this to work.

During configure it is automatically probed for libnfs and support
is enabled on-the-fly. You can forbid or enforce libnfs support
with --disable-libnfs or --enable-libnfs respectively.

Due to NFS restrictions you might need to execute your binaries
as root, allow them to open priviledged ports (<1024) or specify
insecure option on the NFS server.

Signed-off-by: Peter Lieven <address@hidden>
---
v1->v2:
  - fixed block/Makefile.objs [Ronnie]
  - do not always register a read handler [Ronnie]
  - add support for reading beyond EOF [Fam]
  - fixed struct and paramter naming [Fam]
  - fixed overlong lines and whitespace errors [Fam]
  - return return status from libnfs whereever possible [Fam]
  - added comment why we set allocated_file_size to -ENOTSUP after write [Fam]
  - avoid segfault when parsing filname [Fam]
  - remove unused close_bh from NFSClient [Fam]
  - avoid dividing and mutliplying total_size by BDRV_SECTOR_SIZE in 
nfs_file_create [Fam]

  MAINTAINERS         |    5 +
  block/Makefile.objs |    1 +
  block/nfs.c         |  419 +++++++++++++++++++++++++++++++++++++++++++++++++++
  configure           |   38 +++++
  4 files changed, 463 insertions(+)
  create mode 100644 block/nfs.c
Which NFS protocol versions are supported by current libnfs?
Will check that out. Ronnie?

+#include <poll.h>
Why is this header included?
leftover.

+typedef struct nfsclient {
Please either drop the struct tag or use "NFSClient".
ok

+static void
+nfs_co_generic_cb(int status, struct nfs_context *nfs, void *data,
+                  void *private_data)
+{
+    NFSTask *Task = private_data;
lowercase "task" local variable name please.
ok

+static int coroutine_fn nfs_co_writev(BlockDriverState *bs,
+                                        int64_t sector_num, int nb_sectors,
+                                        QEMUIOVector *iov)
+{
+    NFSClient *client = bs->opaque;
+    NFSTask task;
+    char *buf = NULL;
+
+    nfs_co_init_task(client, &task);
+
+    buf = g_malloc(nb_sectors * BDRV_SECTOR_SIZE);
+    qemu_iovec_to_buf(iov, 0, buf, nb_sectors * BDRV_SECTOR_SIZE);
+
+    if (nfs_pwrite_async(client->context, client->fh,
+                         sector_num * BDRV_SECTOR_SIZE,
+                         nb_sectors * BDRV_SECTOR_SIZE,
+                         buf, nfs_co_generic_cb, &task) != 0) {
+        g_free(buf);
+        return -EIO;
Can we get a more detailed errno here?  (e.g. ENOSPC)
libnfs only returns 0 or -1 if the setup of the call
fails. the status code from the RPC is more detailed
and available in task.status.

+    }
+
+    while (!task.complete) {
+        nfs_set_events(client);
+        qemu_coroutine_yield();
+    }
+
+    g_free(buf);
+
+    if (task.status != nb_sectors * BDRV_SECTOR_SIZE) {
+        return task.status < 0 ? task.status : -EIO;
+    }
+
+    bs->total_sectors = MAX(bs->total_sectors, sector_num + nb_sectors);
Why is this necessary?  block.c will update bs->total_sectors if the
file is growable.
Ok, didn't know ;-)

+    /* set to -ENOTSUP since bdrv_allocated_file_size is only used
+     * in qemu-img open. So we can use the cached value for allocate
+     * filesize obtained from fstat at open time */
+    client->allocated_file_size = -ENOTSUP;
Can you implement this fully?  By stubbing it out like this we won't be
able to call get_allocated_file_size() at runtime in the future without
updating the nfs block driver code.  It's just an fstat call, shouldn't
be too hard to implement properly :).
Will do. I will also add bdrv_truncate as its needed for VMDK Create.

+    if (client->context == NULL) {
+        error_setg(errp, "Failed to init NFS context");
+        ret = -EINVAL;
+        goto fail;
+    }
+
+    if (strlen(filename) <= 6) {
+        error_setg(errp, "Invalid server in URL");
+        ret = -EINVAL;
+        goto fail;
+    }
+
+    server = g_strdup(filename + 6);
+    if (server[0] == '/' || server[0] == '\0') {
+        error_setg(errp, "Invalid server in URL");
+        ret = -EINVAL;
+        goto fail;
+    }
+    strp = strchr(server, '/');
+    if (strp == NULL) {
+        error_setg(errp, "Invalid URL specified.\n");
+        ret = -EINVAL;
+        goto fail;
+    }
+    path = g_strdup(strp);
+    *strp = 0;
+    strp = strrchr(path, '/');
+    if (strp == NULL) {
+        error_setg(errp, "Invalid URL specified.\n");
+        ret = -EINVAL;
+        goto fail;
+    }
+    file = g_strdup(strp);
+    *strp = 0;
Can you use util/uri.c to avoid the string manipulation?  It can extract
the different parts for validation: scheme, server, port, and path.
Thanks for the pointer. This is actually the parsing code from
libnfs examples.

+static int nfs_file_create(const char *filename, QEMUOptionParameter *options,
+                         Error **errp)
+{
+    int ret = 0;
+    int64_t total_size = 0;
+    BlockDriverState *bs;
+    NFSClient *client = NULL;
+    QDict *bs_options;
+
+    bs = bdrv_new("");
This approach seems a little risky to me.  bs is a fake
BlockDriverState with many fields not initialized.  bdrv_*() calls would
crash.

How about a static nfs_open() function that operates on NFSClient and is
shared by nfs_file_create() and nfs_file_open()?
Good idea, will look into this.

+##########################################
+# Do we have libnfs
+if test "$libnfs" != "no" ; then
+  cat > $TMPC << EOF
+#include <nfsc/libnfs-zdr.h>
+#include <nfsc/libnfs.h>
+#include <nfsc/libnfs-raw.h>
+#include <nfsc/libnfs-raw-mount.h>
+int main(void) {
+    nfs_init_context();
+    nfs_pread_async(0,0,0,0,0,0);
+    nfs_pwrite_async(0,0,0,0,0,0,0);
+    nfs_fstat(0,0,0);
+    return 0;
+    }
+EOF
+  if compile_prog "-Werror" "-lnfs" ; then
+    libnfs="yes"
+    LIBS="$LIBS -lnfs"
pkg-config is usually better than hardcoding names.  Is pkg-config
available for libnfs?
it is, but until today it was buggy. we can use it from libnfs 1.9.0
onwards.

Thanks for reviewing.

Peter



reply via email to

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