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

[Stefan Hajnoczi]

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?

Stefan