From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50890) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Vu2Ja-0006Me-Ht for qemu-devel@nongnu.org; Fri, 20 Dec 2013 10:48:43 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Vu2JV-00017T-EE for qemu-devel@nongnu.org; Fri, 20 Dec 2013 10:48:38 -0500 Received: from mx.ipv6.kamp.de ([2a02:248:0:51::16]:58984 helo=mx01.kamp.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Vu2JV-00017L-3e for qemu-devel@nongnu.org; Fri, 20 Dec 2013 10:48:33 -0500 Message-ID: <52B46707.8080305@kamp.de> Date: Fri, 20 Dec 2013 16:49: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> <52B43DC7.90007@kamp.de> <52B44F0E.5070403@kamp.de> <52B4579B.7000205@kamp.de> In-Reply-To: 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: Kevin Wolf , Paolo Bonzini , Fam Zheng , qemu-devel , Stefan Hajnoczi On 20.12.2013 16:30, Stefan Hajnoczi wrote: > On Fri, Dec 20, 2013 at 3:43 PM, Peter Lieven wrote: >> On 20.12.2013 15:38, Stefan Hajnoczi wrote: >>> On Fri, Dec 20, 2013 at 3:07 PM, Peter Lieven wrote: >>>> On 20.12.2013 14:57, Stefan Hajnoczi wrote: >>>>> On Fri, Dec 20, 2013 at 1:53 PM, Peter Lieven wrote: >>>>>> 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: >>>>>>>>>> + /* 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. >>>>> .bdrv_get_allocated_file_size() implementations in other block drivers >>>>> are synchronous. Making the block driver interface use coroutines >>>>> would be wrong unless all the block drivers were updated to use >>>>> coroutines too. >>>> I can do that. I think its not too complicated because all those >>>> implementations do not rely on callbacks. It should be possible >>>> to just rename the existing implemenations to lets say >>>> .bdrv_co_get_allocated_file_size and call them inside a coroutine. >>> No, that would be wrong because coroutine functions should not block. >>> The point of coroutines is that if they cannot proceed they must yield >>> so the event loop regains control. If you simply rename the function >>> to _co_ then they will block the event loop and not be true coroutine >>> functions. >>> >>>>> Can you just call nfs_fstat() (the sync libnfs interface)? >>>> I can only do that if its guaranteed that no other requests are in flight >>>> otherwise it will mess up. >>> How will it mess up? >> The sync calls into libnfs are just wrappers around the async calls. >> The problem is that this wrapper will handle all the callbacks for the >> in-flight requests and they will never return. > So back to my original suggestion to use a qemu_aio_wait() loop in block/nfs.c? sorry, I cannot follow you. but maybe this here is a short solution. question is, what will happen when there are pending requests which invoke callbacks. will we end up returning from qemu_aio_wait? in the qemu-img info case this here works: static int64_t nfs_get_allocated_file_size(BlockDriverState *bs) { NFSClient *client = bs->opaque; NFSRPC task = {0}; struct stat st; task.st = &st; if (nfs_fstat_async(client->context, client->fh, nfs_co_generic_cb, &task) != 0) { return -ENOMEM; } while (!task.complete) { nfs_set_events(client); qemu_aio_wait(); } return (task.status < 0 ? task.status : st.st_blocks * st.st_blksize); } in theory we could also leave .bdrv_get_allocated_file_size completly out. Its just a nice to have so that qemu-img info does not show "unavailable" for disk size. Peter