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.
signature.asc
Description: PGP signature
