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().