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

[Peter Lieven <pl@kamp.de>]

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);
}

in theory we could also leave .bdrv_get_allocated_file_size completly out. Its just a
nice to have so that qemu-img info does not show "unavailable" for disk size.

Peter