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

[Stefan Hajnoczi]

On Fri, Dec 20, 2013 at 4:57 PM, Peter Lieven <address@hidden> wrote:
> On 20.12.2013 16:54, Stefan Hajnoczi wrote:
>>
>> On Fri, Dec 20, 2013 at 4:49 PM, Peter Lieven <address@hidden> wrote:
>>>
>>> On 20.12.2013 16:30, Stefan Hajnoczi wrote:
>>>>
>>>> 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?
>>>
>>> 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.