From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54010) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Vu2Qt-0001IC-Di for qemu-devel@nongnu.org; Fri, 20 Dec 2013 10:56:16 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Vu2Qn-0003mi-W6 for qemu-devel@nongnu.org; Fri, 20 Dec 2013 10:56:11 -0500 Received: from mx.ipv6.kamp.de ([2a02:248:0:51::16]:52035 helo=mx01.kamp.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Vu2Qn-0003mP-MX for qemu-devel@nongnu.org; Fri, 20 Dec 2013 10:56:05 -0500 Message-ID: <52B468D5.5010204@kamp.de> Date: Fri, 20 Dec 2013 16:57:09 +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> <52B46707.8080305@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:54, Stefan Hajnoczi wrote: > On Fri, Dec 20, 2013 at 4:49 PM, Peter Lieven wrote: >> 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); >> } > Yes, that's exactly what I had in mind. > > Yes, other callbacks will get called and requests will complete in > this event loop. That can be a problem in some scenarios but should > be okay with bdrv_get_allocated_file_size(). ok I will send v4 and then prepare for the holidays ;-) Peter