Re: [Qemu-devel] [PATCHv2] block: add native support for NFS

[Peter Lieven <pl@kamp.de>]

On 20.12.2013 17:27, Stefan Hajnoczi wrote:
> On Fri, Dec 20, 2013 at 4:57 PM, Peter Lieven <pl@kamp.de> wrote:
>> On 20.12.2013 16:54, Stefan Hajnoczi wrote:
>>> On Fri, Dec 20, 2013 at 4:49 PM, Peter Lieven <pl@kamp.de> wrote:
>>>> On 20.12.2013 16:30, Stefan Hajnoczi wrote:
>>>>> On Fri, Dec 20, 2013 at 3:43 PM, Peter Lieven <pl@kamp.de> wrote:
>>>>>> On 20.12.2013 15:38, Stefan Hajnoczi wrote:
>>>>>>> On Fri, Dec 20, 2013 at 3:07 PM, Peter Lieven <pl@kamp.de> wrote:
>>>>>>>> On 20.12.2013 14:57, Stefan Hajnoczi wrote:
>>>>>>>>> On Fri, Dec 20, 2013 at 1:53 PM, Peter Lieven <pl@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.
If you pick this up please take v5.

Peter