[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 2/4] nbd/client: do negotiation in coroutine
From: |
Vladimir Sementsov-Ogievskiy |
Subject: |
Re: [Qemu-devel] [PATCH 2/4] nbd/client: do negotiation in coroutine |
Date: |
Tue, 19 Feb 2019 10:37:16 +0000 |
12.02.2019 0:38, Eric Blake wrote:
> On 2/11/19 6:55 AM, Vladimir Sementsov-Ogievskiy wrote:
>> As a first step to non-blocking negotiation, move it to coroutine.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
>> ---
>> nbd/client.c | 123 +++++++++++++++++++++++++++++++++++++++++++++------
>> 1 file changed, 109 insertions(+), 14 deletions(-)
>>
>> diff --git a/nbd/client.c b/nbd/client.c
>> index 10a52ad7d0..2ba2220a4a 100644
>> --- a/nbd/client.c
>> +++ b/nbd/client.c
>> @@ -859,13 +859,14 @@ static int nbd_list_meta_contexts(QIOChannel *ioc,
>> * 2: server is newstyle, but lacks structured replies
>> * 3: server is newstyle and set up for structured replies
>> */
>> -static int nbd_start_negotiate(QIOChannel *ioc, QCryptoTLSCreds *tlscreds,
>> - const char *hostname, QIOChannel **outioc,
>> - bool structured_reply, bool *zeroes,
>> - Error **errp)
>> +static int coroutine_fn nbd_co_start_negotiate(
>> + QIOChannel *ioc, QCryptoTLSCreds *tlscreds, const char *hostname,
>> + QIOChannel **outioc, bool structured_reply, bool *zeroes, Error
>> **errp)
>> {
>> uint64_t magic;
>>
>> + assert(qemu_in_coroutine());
>> +
>
> Do we need this assert, since this function is static, and only called by:
OK, I think this can be dropped
>
>> trace_nbd_start_negotiate(tlscreds, hostname ? hostname : "<null>");
>>
>> if (zeroes) {
>> @@ -990,19 +991,20 @@ static int nbd_negotiate_finish_oldstyle(QIOChannel
>> *ioc, NBDExportInfo *info,
>> * Returns: negative errno: failure talking to server
>> * 0: server is connected
>> */
>> -int nbd_receive_negotiate(QIOChannel *ioc, QCryptoTLSCreds *tlscreds,
>> - const char *hostname, QIOChannel **outioc,
>> - NBDExportInfo *info, Error **errp)
>> +static int coroutine_fn nbd_co_receive_negotiate(
>> + QIOChannel *ioc, QCryptoTLSCreds *tlscreds, const char *hostname,
>> + QIOChannel **outioc, NBDExportInfo *info, Error **errp)
>> {
>> int result;
>> bool zeroes;
>> bool base_allocation = info->base_allocation;
>>
>> + assert(qemu_in_coroutine());
>> assert(info->name);
>> trace_nbd_receive_negotiate_name(info->name);
>>
>> - result = nbd_start_negotiate(ioc, tlscreds, hostname, outioc,
>> - info->structured_reply, &zeroes, errp);
>> + result = nbd_co_start_negotiate(ioc, tlscreds, hostname, outioc,
>> + info->structured_reply, &zeroes, errp);
>
> other functions in the same file that have also made the same assertion?
> For that matter, does the assert() add any value over the fact that we
> already annotated things as coroutine_fn?
>
> I know that at some point, there was a proposal on the list for having
> the compiler enforce coroutine_fn annotations (ensuring that a
> coroutine_fn only calls other coroutine_fns, and that coroutines can
> only be started on an entry point properly marked), but that's a
> different task for another day.
>
I thought, that converting function to be "coroutine_fn" (not creating a new
one)
is a good reason to add an assertion.
>
>> +static int nbd_receive_common(CoroutineEntry *entry,
>> + NbdReceiveNegotiateCo *data)
>> +{
>> + data->ret = -EINPROGRESS;
>> +
>> + if (qemu_in_coroutine()) {
>> + entry(data);
>> + } else {
>> + AioContext *ctx = qio_channel_get_attached_aio_context(data->ioc);
>> + bool attach = !ctx;
>
> There's where you used the function added in 1/4.
>
>> +
>> + if (attach) {
>> + ctx = qemu_get_current_aio_context();
>> + qio_channel_attach_aio_context(data->ioc, ctx);
>> + }
>> +
>> + qemu_aio_coroutine_enter(ctx, qemu_coroutine_create(entry, data));
>> + AIO_WAIT_WHILE(ctx, data->ret == -EINPROGRESS);
>> +
>> + if (attach) {
>> + qio_channel_detach_aio_context(data->ioc);
>> + }
>> + }
>> +
>> + return data->ret;
>> +}
>
> Looks reasonable.
>
>> +
>> +int nbd_receive_negotiate(
>> + QIOChannel *ioc, QCryptoTLSCreds *tlscreds, const char *hostname,
>> + QIOChannel **outioc, NBDExportInfo *info, Error **errp)
>> +{
>
> Why the reflowed parameter list, compared to its existing listing (as
> shown by the - lines where you added nbd_receive_co_negotiate)? That's
> only cosmetic, so I can live with it, but it seems like it makes the
> diff slightly larger.
>
Hmm, maybe, will change it back.
--
Best regards,
Vladimir
- Re: [Qemu-devel] [PATCH 4/4] block/nbd-client: use non-blocking io channel for nbd negotiation, (continued)