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

[Stefan Hajnoczi]

On Fri, Dec 20, 2013 at 3:43 PM, Peter Lieven <address@hidden> wrote:
> On 20.12.2013 15:38, Stefan Hajnoczi wrote:
>>
>> On Fri, Dec 20, 2013 at 3:07 PM, Peter Lieven <address@hidden> wrote:
>>>
>>> On 20.12.2013 14:57, Stefan Hajnoczi wrote:
>>>>
>>>> On Fri, Dec 20, 2013 at 1:53 PM, Peter Lieven <address@hidden> 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?

Stefan