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.
signature.asc