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

[Klaus Jensen]

Hi Vladimir,

Thanks for taking the time to look through this!

I'll try to comment on all your observations below.

On Jun  7 08:14, Vladimir Sementsov-Ogievskiy wrote:
> 04.06.2021 09:52, Klaus Jensen wrote:
> > From: Klaus Jensen <k.jensen@samsung.com>
> >
> > This series reimplements flush, dsm, copy, zone reset and format nvm to
> > allow cancellation. I posted an RFC back in March ("hw/block/nvme:
> > convert ad-hoc aio tracking to aiocb") and I've applied some feedback
> > from Stefan and reimplemented the remaining commands.
> >
> > The basic idea is to define custom AIOCBs for these commands. The custom
> > AIOCB takes care of issuing all the "nested" AIOs one by one instead of
> > blindly sending them off simultaneously without tracking the returned
> > aiocbs.
>
> I'm not familiar with nvme. But intuitively, isn't it less efficient to 
> send mutltiple commands one-by-one? Overall, wouldn't it be slower?

No, you are right, it is of course slower overall.

> In block layer we mostly do opposite transition: instead of doing IO 
> operations one-by-one, run them simultaneously to make a non-empty 
> queue on a device.. Even on one device. This way overall performance is 
> increased.

Of these commands, Copy is the only one that I would consider optimizing 
like this. But the most obvious use of the Copy command is host-driven 
garbage collection in the context of zoned namespaces. And I would not 
consider that operation to be performance critical in terms of latency. 
All regular I/O commands are "one aiocb" and doesnt need any of this. 
And we already "parallelize" this heavily.

> If you need to store nested AIOCBs, you may store them in a list for 
> example, and cancel in a loop, keeping simultaneous start for all 
> flushes.. If you send two flushes on two different disks, what's the 
> reason to wait for first flush finish before issuing the second?

Keeping a list of returned aiocbs was my initial approach actually. But 
when I looked at hw/ide I got the impression that the AIOCB approach was 
the right one. My first approach involved adding an aiocblist to the 
core NvmeRequest structure, but I ended up killing that approach because 
I didnt want to deal with it on the normal I/O path.

But you are absolutely correct that waiting for the first flush to 
finish is suboptimal.

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

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

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 ;)

> > Those commands are special in
> > that may issue AIOs to multiple namespaces and thus, to multiple block
> > backends. Since this device does not support iothreads, I've opted for
> > simply always returning the main loop aio context, but I wonder if this
> > is acceptable or not. It might be the case that this should contain an
> > assert of some kind, in case someone starts adding iothread support.
> >
> > Klaus Jensen (11):
> >   hw/nvme: reimplement flush to allow cancellation
> >   hw/nvme: add nvme_block_status_all helper
> >   hw/nvme: reimplement dsm to allow cancellation
> >   hw/nvme: save reftag when generating pi
> >   hw/nvme: remove assert from nvme_get_zone_by_slba
> >   hw/nvme: use prinfo directly in nvme_check_prinfo and nvme_dif_check
> >   hw/nvme: add dw0/1 to the req completion trace event
> >   hw/nvme: reimplement the copy command to allow aio cancellation
> >   hw/nvme: reimplement zone reset to allow cancellation
> >   hw/nvme: reimplement format nvm to allow cancellation
> >   Partially revert "hw/block/nvme: drain namespaces on sq deletion"
> >
> >  hw/nvme/nvme.h       |   10 +-
> >  include/block/nvme.h |    8 +
> >  hw/nvme/ctrl.c       | 1861 ++++++++++++++++++++++++------------------
> >  hw/nvme/dif.c        |   64 +-
> >  hw/nvme/trace-events |   21 +-
> >  5 files changed, 1102 insertions(+), 862 deletions(-)

signature.asc
Description: PGP signature