Re: [Qemu-devel] [PATCH 4/4] block/nbd-client: use non-blocking io channel for nbd negotiation
19.02.2019 16:18, Vladimir Sementsov-Ogievskiy wrote:
> 12.02.2019 1:02, Eric Blake wrote:
>> On 2/11/19 6:56 AM, Vladimir Sementsov-Ogievskiy wrote:
>>> Now negotiation is done in coroutine, so to take benefit of it let's
>>> use non-blocking model.
>>>
>>> Note that QIOChannel handle synchronous io calls correctly anyway, so
>>
>> s/handle/handles/
>>
>>> it's not a problem to send final NBD_CMD_DISC to non-blocking channel
>>> but using sync qio interface, even not in coroutine.
>>
>> s/not in/when not in a/
>>
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy
>>> ---
>>> block/nbd-client.c | 10 +++---
>>> 1 file changed, 3 insertions(+), 7 deletions(-)
>>
>> This fixes qemu as NBD client for use in block devices, but what about
>> qemu as NBD client in qemu-nbd? Does that need any updates? Then
>> again, your observation about QIO doing the right thing for both
>> blocking (qemu-nbd) and non-blocking (block layer) channels seems to
>> cover that.
>
> In qemu-nbd client works in a separate thread, so, I think, we don't need
> nonblocking: we have a thread anyway.
>
> On the other hand, is my code in nbd_receive_common safe, keeping in mind
> that we call it from separate thread?
>
> Are qio_channel_attach_aio_context, qemu_aio_coroutine_enter, AIO_WAIT_WHILE
> thread-safe?
Could somebody shed a bit of light here, please?
>
>>
>>> @@ -1072,9 +1072,6 @@ static int nbd_client_connect(BlockDriverState *bs,
>>> object_ref(OBJECT(client->ioc));
>>> }
>>> - /* Now that we're connected, set the socket to be non-blocking and
>>> - * kick the reply mechanism. */
>>> - qio_channel_set_blocking(QIO_CHANNEL(sioc), false, NULL);
>>> client->connection_co = qemu_coroutine_create(nbd_connection_entry,
>>> client);
>>> nbd_client_attach_aio_context(bs, bdrv_get_aio_context(bs));
>>> @@ -1083,9 +1080,8 @@ static int nbd_client_connect(BlockDriverState *bs,
>>> fail:
>>> /*
>>> - * We have connected, but must fail for other reasons. The
>>> - * connection is still blocking; send NBD_CMD_DISC as a courtesy
>>> - * to the server.
>>> + * We have connected, but must fail for other reasons.
>>> + * Send NBD_CMD_DISC as a courtesy to the server.
>>> */
>>> {
>>> NBDRequest request = { .type = NBD_CMD_DISC };
>>>
>>
>
>
--
Best regards,
Vladimir
Re: [Qemu-devel] [PATCH 4/4] block/nbd-client: use non-blocking io channel for nbd negotiation
12.02.2019 1:02, Eric Blake wrote:
> On 2/11/19 6:56 AM, Vladimir Sementsov-Ogievskiy wrote:
>> Now negotiation is done in coroutine, so to take benefit of it let's
>> use non-blocking model.
>>
>> Note that QIOChannel handle synchronous io calls correctly anyway, so
>
> s/handle/handles/
>
>> it's not a problem to send final NBD_CMD_DISC to non-blocking channel
>> but using sync qio interface, even not in coroutine.
>
> s/not in/when not in a/
>
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy
>> ---
>> block/nbd-client.c | 10 +++---
>> 1 file changed, 3 insertions(+), 7 deletions(-)
>
> This fixes qemu as NBD client for use in block devices, but what about
> qemu as NBD client in qemu-nbd? Does that need any updates? Then
> again, your observation about QIO doing the right thing for both
> blocking (qemu-nbd) and non-blocking (block layer) channels seems to
> cover that.
In qemu-nbd client works in a separate thread, so, I think, we don't need
nonblocking: we have a thread anyway.
On the other hand, is my code in nbd_reaceive_common safe, keeping in mind
that we call it from separate thread?
Are qio_channel_attach_aio_context, qemu_aio_coroutine_enter, AIO_WAIT_WHILE
thread-safe?
>
>> @@ -1072,9 +1072,6 @@ static int nbd_client_connect(BlockDriverState *bs,
>> object_ref(OBJECT(client->ioc));
>> }
>>
>> -/* Now that we're connected, set the socket to be non-blocking and
>> - * kick the reply mechanism. */
>> -qio_channel_set_blocking(QIO_CHANNEL(sioc), false, NULL);
>> client->connection_co = qemu_coroutine_create(nbd_connection_entry,
>> client);
>> nbd_client_attach_aio_context(bs, bdrv_get_aio_context(bs));
>>
>> @@ -1083,9 +1080,8 @@ static int nbd_client_connect(BlockDriverState *bs,
>>
>>fail:
>> /*
>> - * We have connected, but must fail for other reasons. The
>> - * connection is still blocking; send NBD_CMD_DISC as a courtesy
>> - * to the server.
>> + * We have connected, but must fail for other reasons.
>> + * Send NBD_CMD_DISC as a courtesy to the server.
>>*/
>> {
>> NBDRequest request = { .type = NBD_CMD_DISC };
>>
>
--
Best regards,
Vladimir
Re: [Qemu-devel] [PATCH 4/4] block/nbd-client: use non-blocking io channel for nbd negotiation
On 2/11/19 6:56 AM, Vladimir Sementsov-Ogievskiy wrote:
> Now negotiation is done in coroutine, so to take benefit of it let's
> use non-blocking model.
>
> Note that QIOChannel handle synchronous io calls correctly anyway, so
s/handle/handles/
> it's not a problem to send final NBD_CMD_DISC to non-blocking channel
> but using sync qio interface, even not in coroutine.
s/not in/when not in a/
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy
> ---
> block/nbd-client.c | 10 +++---
> 1 file changed, 3 insertions(+), 7 deletions(-)
This fixes qemu as NBD client for use in block devices, but what about
qemu as NBD client in qemu-nbd? Does that need any updates? Then
again, your observation about QIO doing the right thing for both
blocking (qemu-nbd) and non-blocking (block layer) channels seems to
cover that.
> @@ -1072,9 +1072,6 @@ static int nbd_client_connect(BlockDriverState *bs,
> object_ref(OBJECT(client->ioc));
> }
>
> -/* Now that we're connected, set the socket to be non-blocking and
> - * kick the reply mechanism. */
> -qio_channel_set_blocking(QIO_CHANNEL(sioc), false, NULL);
> client->connection_co = qemu_coroutine_create(nbd_connection_entry,
> client);
> nbd_client_attach_aio_context(bs, bdrv_get_aio_context(bs));
>
> @@ -1083,9 +1080,8 @@ static int nbd_client_connect(BlockDriverState *bs,
>
> fail:
> /*
> - * We have connected, but must fail for other reasons. The
> - * connection is still blocking; send NBD_CMD_DISC as a courtesy
> - * to the server.
> + * We have connected, but must fail for other reasons.
> + * Send NBD_CMD_DISC as a courtesy to the server.
> */
> {
> NBDRequest request = { .type = NBD_CMD_DISC };
>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature
[Qemu-devel] [PATCH 4/4] block/nbd-client: use non-blocking io channel for nbd negotiation
Now negotiation is done in coroutine, so to take benefit of it let's
use non-blocking model.
Note that QIOChannel handle synchronous io calls correctly anyway, so
it's not a problem to send final NBD_CMD_DISC to non-blocking channel
but using sync qio interface, even not in coroutine.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
block/nbd-client.c | 10 +++---
1 file changed, 3 insertions(+), 7 deletions(-)
diff --git a/block/nbd-client.c b/block/nbd-client.c
index f0ad54ce21..79bc6b7e29 100644
--- a/block/nbd-client.c
+++ b/block/nbd-client.c
@@ -1029,7 +1029,7 @@ static int nbd_client_connect(BlockDriverState *bs,
/* NBD handshake */
logout("session init %s\n", export);
-qio_channel_set_blocking(QIO_CHANNEL(sioc), true, NULL);
+qio_channel_set_blocking(QIO_CHANNEL(sioc), false, NULL);
client->info.request_sizes = true;
client->info.structured_reply = true;
@@ -1072,9 +1072,6 @@ static int nbd_client_connect(BlockDriverState *bs,
object_ref(OBJECT(client->ioc));
}
-/* Now that we're connected, set the socket to be non-blocking and
- * kick the reply mechanism. */
-qio_channel_set_blocking(QIO_CHANNEL(sioc), false, NULL);
client->connection_co = qemu_coroutine_create(nbd_connection_entry,
client);
nbd_client_attach_aio_context(bs, bdrv_get_aio_context(bs));
@@ -1083,9 +1080,8 @@ static int nbd_client_connect(BlockDriverState *bs,
fail:
/*
- * We have connected, but must fail for other reasons. The
- * connection is still blocking; send NBD_CMD_DISC as a courtesy
- * to the server.
+ * We have connected, but must fail for other reasons.
+ * Send NBD_CMD_DISC as a courtesy to the server.
*/
{
NBDRequest request = { .type = NBD_CMD_DISC };
--
2.18.0
