Re: [PATCH v2 12/12] block/nvme: Use per-queue AIO context

[Philippe Mathieu-Daudé]

On 7/1/20 6:03 PM, Stefan Hajnoczi wrote:
> On Tue, Jun 30, 2020 at 09:13:18PM +0200, Philippe Mathieu-Daudé wrote:
>> To be able to use multiple queues on the same hardware,
>> we need to have each queue able to receive IRQ notifications
>> in the correct AIO context.
>> The AIO context and the notification handler have to be proper
>> to each queue, not to the block driver. Move aio_context and
>> irq_notifier from BDRVNVMeState to NVMeQueuePair.
> 
> If I understand correctly, after this patch:
> 
> 1. Each queue pair has an EventNotifier irq_notifier but only the admin
>    queuepair's irq_notifier is hooked up to VFIO. This means all
>    interrupts come via the admin queuepair.

Yes, but this was the behavior before this patch too.

> 
>    (This also means all queuepairs still need to be polled for
>    completions when the interrupt fires because we don't know which
>    queuepair had a completion event.)

Yes.

> 
> 2. AioContexts must be identical across all queuepairs and
>    BlockDriverStates. Although they all have their own AioContext
>    pointer there is no true support for different AioContexts yet.

I'm not sure. Maybe v3 will sort that out.

> 
>    (For example, nvme_cmd_sync() is called with a bs argument but
>    AIO_WAIT_WHILE(q->aio_context, ...) uses the queuepair aio_context so
>    the assumption is that they match.)
> 
> Please confirm and add something equivalent into the commit description
> so the assumptions/limitations are clear.

I'll do my best!

Thanks for your reviews,

Phil.