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

[Peter Lieven <pl@kamp.de>]

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 <pl@kamp.de>
>> ---
>> 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