On Fri, Dec 20, 2013 at 4:57 PM, Peter Lieven <p...@kamp.de> wrote: > On 20.12.2013 16:54, Stefan Hajnoczi wrote: >> >> On Fri, Dec 20, 2013 at 4:49 PM, Peter Lieven <p...@kamp.de> wrote: >>> >>> On 20.12.2013 16:30, Stefan Hajnoczi wrote: >>>> >>>> On Fri, Dec 20, 2013 at 3:43 PM, Peter Lieven <p...@kamp.de> wrote: >>>>> >>>>> On 20.12.2013 15:38, Stefan Hajnoczi wrote: >>>>>> >>>>>> On Fri, Dec 20, 2013 at 3:07 PM, Peter Lieven <p...@kamp.de> wrote: >>>>>>> >>>>>>> On 20.12.2013 14:57, Stefan Hajnoczi wrote: >>>>>>>> >>>>>>>> On Fri, Dec 20, 2013 at 1:53 PM, Peter Lieven <p...@kamp.de> 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.