From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57543) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Vz25L-0003dc-WE for qemu-devel@nongnu.org; Fri, 03 Jan 2014 05:34:41 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Vz25G-00027e-Sd for qemu-devel@nongnu.org; Fri, 03 Jan 2014 05:34:35 -0500 Received: from mx.ipv6.kamp.de ([2a02:248:0:51::16]:36178 helo=mx01.kamp.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Vz25G-00027W-I1 for qemu-devel@nongnu.org; Fri, 03 Jan 2014 05:34:30 -0500 Message-ID: <52C6928E.1060200@kamp.de> Date: Fri, 03 Jan 2014 11:35:58 +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> <52B468D5.5010204@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 17:27, Stefan Hajnoczi wrote: > On Fri, Dec 20, 2013 at 4:57 PM, Peter Lieven wrote: >> 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 ;-) > Happy holidays! I already sent out the block pull request earlier > today but your patch will be included in the first January pull > request. If you pick this up please take v5. Peter