From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:36419) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VtzZF-0006yd-DZ for qemu-devel@nongnu.org; Fri, 20 Dec 2013 07:52:42 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VtzZA-0005Wl-9D for qemu-devel@nongnu.org; Fri, 20 Dec 2013 07:52:37 -0500 Received: from mx.ipv6.kamp.de ([2a02:248:0:51::16]:36903 helo=mx01.kamp.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VtzZ9-0005Sp-UK for qemu-devel@nongnu.org; Fri, 20 Dec 2013 07:52:32 -0500 Message-ID: <52B43DC7.90007@kamp.de> Date: Fri, 20 Dec 2013 13:53:27 +0100 From: Peter Lieven MIME-Version: 1.0 References: <1387271725-17060-1-git-send-email-pl@kamp.de> <20131217164730.GD2708@stefanha-thinkpad.redhat.com> <52B41279.8060304@kamp.de> <20131220121935.GA5905@stefanha-thinkpad.redhat.com> In-Reply-To: <20131220121935.GA5905@stefanha-thinkpad.redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCHv2] block: add native support for NFS List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: kwolf@redhat.com, Stefan Hajnoczi , famz@redhat.com, qemu-devel@nongnu.org, pbonzini@redhat.com On 20.12.2013 13:19, Stefan Hajnoczi wrote: > On Fri, Dec 20, 2013 at 10:48:41AM +0100, Peter Lieven wrote: >> 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://// >>>> >>>> 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 >>>> --- >>>> 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? >>> >>>> +#include >>> Why is this header included? >>> >>>> +typedef struct nfsclient { >>> Please either drop the struct tag or use "NFSClient". >>> >>>> +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. >>> >>>> +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) >>> >>>> + } >>>> + >>>> + 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. >>> >>>> + /* 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 :). >> It seems I have to leave it as is currently. bdrv_get_allocated_file_size >> is not in a coroutine context. I get coroutine yields to no one. > Create a coroutine and pump the event loop until it has reached > completion: > > co = qemu_coroutine_create(my_coroutine_fn, ...); > qemu_coroutine_enter(co, foo); > while (!complete) { > qemu_aio_wait(); > } > > See block.c for similar examples. Wouldn't it make sense to make this modification to bdrv_get_allocated_file_size in block.c rather than in client/nfs.c and in the future potentially other drivers? If yes, I would ask you to take v3 of the NFS protocol patch and I promise to send a follow up early next year to make this modification to block.c and change block/nfs.c and other implementations to be a coroutine_fn. Thanks Peter