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

[Stefan Hajnoczi]

On Fri, Jun 04, 2021 at 08:52:26AM +0200, 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'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. 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.

This approach looks fine to me. Vladimir mentioned coroutines, which
have simpler code for sequential I/O, but don't support cancellation.
Since cancellation is the point of this series I think sticking to the
aio approach makes sense.

Regarding coroutine cancellation, it's a hard to add since there is
already a lot of coroutine code that's not written with cancellation in
mind.

I think I would approach it by adding a .cancel_cb() field to Coroutine
that does nothing by default (because existing code doesn't support
cancellation and we must wait for the operation to complete). Cases the
do support cancellation would install a .cancel_cb() across yield that
causes the operation that coroutine is waiting on to complete early.

An alternative approach is to re-enter the coroutine, but this requires
all yield points in QEMU to check for cancellation. I don't think this
is practical because converting all the code would be hard.

Anyway, the aio approach looks fine.

Stefan

signature.asc
Description: PGP signature