Re: [RFC PATCH 00/11] hw/nvme: reimplement all multi-aio commands with custom aiocbs

[Klaus Jensen <its@irrelevant.dk>]

[-- Attachment #1: Type: text/plain, Size: 6240 bytes --]

On Jun  7 10:11, Vladimir Sementsov-Ogievskiy wrote:
>07.06.2021 09:17, Klaus Jensen wrote:
>>On Jun  7 08:14, Vladimir Sementsov-Ogievskiy wrote:
>>>04.06.2021 09:52, Klaus Jensen wrote:
>>>>
>>>>I've kept the RFC since I'm still new to using the block layer like
>>>>this. I was hoping that Stefan could find some time to look over this -
>>>>this is a huge series, so I don't expect non-nvme folks to spend a large
>>>>amount of time on it, but I would really like feedback on my approach in
>>>>the reimplementation of flush and format.
>>>
>>>If I understand your code correctly, you do stat next io operation 
>>>from call-back of a previous. It works, and it is similar to haw 
>>>mirror block-job was operating some time ago (still it maintained 
>>>several in-flight requests simultaneously, but I'm about using _aio_ 
>>>functions). Still, now mirror doesn't use _aio_ functions like this.
>>>
>>>Better approach to call several io functions of block layer 
>>>one-by-one is creating a coroutine. You may just add a coroutine 
>>>function, that does all your linear logic as you want, without any 
>>>callbacks like:
>>>
>>>nvme_co_flush()
>>>{
>>>  for (...) {
>>>     blk_co_flush();
>>>  }
>>>}
>>>
>>>(and you'll need qemu_coroutine_create() and qemu_coroutine_enter() 
>>>to start a coroutine).
>>>
>>
>>So, this is definitely a tempting way to implement this. I must admit 
>>that I did not consider it like this because I thought this was at the 
>>wrong level of abstraction (looked to me like this was something that 
>>belonged in block/, not hw/). Again, I referred to the Trim 
>>implementation in hw/ide as the source of inspiration on the 
>>sequential AIOCB approach.
>
>No, I think it's OK from abstraction point of view. Everybody is 
>welcome to use coroutines if it is appropriate and especially for doing 
>sequential IOs :)
>Actually, it's just more efficient: the way I propose, you create one 
>coroutine, which does all your logic as you want, when blk_aio_ 
>functions actually create a coroutine under the hood each time (I don't 
>think that it noticeably affects performance, but logic becomes more 
>straightforward)
>
>The only problem is that for this way we don't have cancellation API, 
>so you can't use it for cancellation anyway :(
>

Yeah, I'm not really feeling up for adding that :P

>>
>>>Still, I'm not sure that moving from simultaneous issuing several IO 
>>>commands to sequential is good idea..
>>>And this way you of course can't use blk_aio_canel.. This leads to my 
>>>last doubt:
>>>
>>>One more thing I don't understand after fast look at the series: how 
>>>cancelation works? It seems to me, that you just call cancel on 
>>>nested AIOCBs, produced by blk_<io_functions>, but no one of them 
>>>implement cancel.. I see only four implementations of .cancel_async 
>>>callback in the whole Qemu code: in iscsi, in ide/core.c, in 
>>>dma-helpers.c and in thread-pool.c.. Seems no one is related to 
>>>blk_aio_flush() and other blk_* functions you call in the series. Or, 
>>>what I miss?
>>>
>>
>>Right now, cancellation is only initiated by the device when a 
>>submission queue is deleted. This causes blk_aio_cancel() to be called 
>>on each BlockAIOCB (NvmeRequest.aiocb) for outstanding requests. In 
>>most cases this BlockAIOCB is a DMAAIOCB from softmmu/dma-helpers.c, 
>>which implements .cancel_async. Prior to this patchset, Flush, DSM, 
>>Copy and so on, did not have any BlockAIOCB to cancel since we did not 
>>keep references to the ongoing AIOs.
>
>Hmm. Looking at flush for example, I don't see how DMAAIOCB comes.
>
>You do
>
>  iocb->aiocb = blk_aio_flush(ns->blkconf.blk, nvme_flush_ns_cb, iocb);
>
>it calls blk_aio_prwv(), it calls blk_aio_get() with 
>blk_aio_em_aiocb_info, that doesn't implement .cancel_async..
>

I meant that most I/O in the regular path (read/write) are using the dma 
helpers (since they do DMA). We might use the blk_aio_p{read,write} 
directly when we read from/write to memory on the device (the controller 
memory buffer), but it is not the common case.

You are correct that BlkAioEmAIOCB does not implement cancel, but the 
"wrapper" (NvmeFlushAIOCB) *does*. This means that, from the NVMe 
controller perspective, we can cancel the flush in between 
(un-cancellable blk_aio_flush-initiated) flushes to multiple namespaces.

>>
>>The blk_aio_cancel() call is synchronous, but it does call 
>>bdrv_aio_cancel_async() which calls the .cancel_async callback if 
>>implemented. This means that we can now cancel ongoing DSM or Copy 
>>commands while they are processing their individual LBA ranges. So 
>>while blk_aio_cancel() subsequently waits for the AIO to complete this 
>>may cause them to complete earlier than if they had run to full 
>>completion (i.e. if they did not implement .cancel_async).
>>
>>There are two things I'd like to do subsequent to this patch series:
>>
>>   1. Fix the Abort command to actually do something. Currently the 
>> command is a no-op (which is allowed by the spec), but I'd like it to 
>> actually cancel the command that the host specified.
>>
>>   2. Make submission queue deletion asynchronous.
>>
>>The infrastructure provided by this refactor should allow this if I am 
>>not mistaken.
>>
>>Overall, I think this "sequentialization" makes it easier to reason 
>>about cancellation, but that might just be me ;)
>>
>
>I just don't like sequential logic simulated on top of aio-callback 
>async API, which in turn is simulated on top of coroutine-driven 
>sequential API (which is made on top of real async block API 
>(thread-based or linux-aio based, etc)) :)

Ha! Yes, we are not exactly improving on that layering here ;)

> Still I can't suggest now an alternative that supports cancellation. 
>But I still think that cancellation doesn't work for blk_aio_flush and 
>friends either..
>

The aiocb from blk_aio_flush is considered "un-cancellable" I guess (by 
design from the block layer), but the NVMe command "Flush" is 
cancellable from the perspective of the NVMe controller. Or at least, 
that's what I am intending to do here.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]