Re: [Qemu-block] [PATCH 1/4] io/channel: add qio_channel_get_attached_aio_context()

[Vladimir Sementsov-Ogievskiy]

12.02.2019 13:33, Daniel P. Berrangé wrote:
> On Mon, Feb 11, 2019 at 03:55:58PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>> Expose attached aio context. It will be used in nbd code, to
>> understand, in which aio context negotiation should be done.
> 
> I'm not especially objecting to the idea of adding the API to the
> QIOChannel class, but I'm surprised that NBD needs this. Surely it
> already knows what AIO context it assigned to the channel in the
> first place.

Actually not, nbd/client.c don't know it, it gets @ioc as a paramter of 
nbd_receive_negotiate
and nbd_receive_export_list, which may be called from block/nbd-client.c and 
qemu-nbd.c..

for example, nbd-client.c do

nbd_client_attach_aio_context(bs, bdrv_get_aio_context(bs));

where nbd_client_attach_aio_context is

void nbd_client_attach_aio_context(BlockDriverState *bs,
                                    AioContext *new_context)
{
     NBDClientSession *client = nbd_get_client_session(bs);
     qio_channel_attach_aio_context(QIO_CHANNEL(client->ioc), new_context);
     aio_co_schedule(new_context, client->connection_co);
}

It does it after nbd_receive_negotiate(), but I think, it should be split in 
these series in nbd_client_connect(),
so that we attach aio context before negotiation and schedule connection_co 
after it.

and NBDClientSession is block/* related object. So, nbd/client.c sees only 
resulting ioc.
And it seems better to get aio context from ioc, than pass additional parameter 
to nbd_receive_negotiate()
together with ioc.

  IOW, it feels like this is papering over a limitation
> in NBD not keeping track of what AIO context it should be using.
> 
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
>> ---
>>   include/io/channel.h | 9 +++++++++
>>   io/channel.c         | 5 +++++
>>   2 files changed, 14 insertions(+)
>>
>> diff --git a/include/io/channel.h b/include/io/channel.h
>> index da2f138200..1a1e4a01b0 100644
>> --- a/include/io/channel.h
>> +++ b/include/io/channel.h
>> @@ -718,6 +718,15 @@ GSource *qio_channel_add_watch_source(QIOChannel *ioc,
>>   void qio_channel_attach_aio_context(QIOChannel *ioc,
>>                                       AioContext *ctx);
>>   
>> +/*
>> + * qio_channel_get_aio_context
>> + * @ioc: the channel object
>> + *
>> + * Returns channel AioContext if any attached by
>> + * qio_channel_attach_aio_context(), otherwise NULL.
>> + */
>> +AioContext *qio_channel_get_attached_aio_context(QIOChannel *ioc);
>> +
>>   /**
>>    * qio_channel_detach_aio_context:
>>    * @ioc: the channel object
>> diff --git a/io/channel.c b/io/channel.c
>> index 8dd0684f5d..a1b937bb6b 100644
>> --- a/io/channel.c
>> +++ b/io/channel.c
>> @@ -454,6 +454,11 @@ void qio_channel_detach_aio_context(QIOChannel *ioc)
>>       ioc->ctx = NULL;
>>   }
>>   
>> +AioContext *qio_channel_get_attached_aio_context(QIOChannel *ioc)
>> +{
>> +    return ioc->ctx;
>> +}
>> +
>>   void coroutine_fn qio_channel_yield(QIOChannel *ioc,
>>                                       GIOCondition condition)
>>   {
>> -- 
>> 2.18.0
>>
> 
> Regards,
> Daniel
> 


-- 
Best regards,
Vladimir