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.