Re: [PATCH RFC 1/4] hw/block/nvme: convert dsm to aiocb

[Stefan Hajnoczi]

On Mon, Mar 08, 2021 at 07:05:40PM +0100, Klaus Jensen wrote:
> On Mar  8 16:37, Stefan Hajnoczi wrote:
> > On Tue, Mar 02, 2021 at 12:10:37PM +0100, Klaus Jensen wrote:
> > > +static void nvme_dsm_cancel(BlockAIOCB *aiocb)
> > > +{
> > > +    NvmeDSMAIOCB *iocb = container_of(aiocb, NvmeDSMAIOCB, common);
> > > +
> > > +    /* break loop */
> > > +    iocb->curr.len = 0;
> > > +    iocb->curr.idx = iocb->nr;
> > > +
> > > +    iocb->ret = -ECANCELED;
> > > +
> > > +    if (iocb->aiocb) {
> > > +        blk_aio_cancel_async(iocb->aiocb);
> > > +        iocb->aiocb = NULL;
> > > +    }
> > > +}
> > 
> > Is the case where iocb->aiocb == NULL just in case nvme_dsm_cancel() is
> > called after the last discard has completed but before the BH runs? I
> > want to make sure there are no other cases because nothing would call
> > iocb->common.cb().
> > 
> 
> Yes - that case *can* happen, right?
> 
> I modeled this after the appoach in the ide trim code (hw/ide/core.c).

Yes, nvme_dsm_bh() may run after other event loop activity. Therefore we
have to take the iocb->aiocb == NULL case into account because some
event loop activity could call nvme_dsm_cancel() before the BH runs.

Another (wild?) possibility is that nvme_dsm_cancel() is called twice.
That's okay, nvme_dsm_cancel() supports that nicely.

But I wasn't sure if there are any other cases where iocb->aiocb can be
NULL? It could be nice to include an assertion or comment to clarify
this. For example:

  if (iocb->aiocb) {
      blk_aio_cancel_async(iocb->aiocb);
      iocb->aiocb = NULL;
  } else {
      /*
       * We only get here if nvme_dsm_cancel() was already called or
       * nvme_dsm_bh() is about to run.
       */
      assert(iocb->curr.idx == iocb->nr);
  }

  /* break loop */
  iocb->curr.len = 0;
  iocb->curr.idx = iocb->nr;

  iocb->ret = -ECANCELED;

(I'm not sure if my assert is correct, but hopefully this explains what
I mean.)

The reason why this assertion is important is because nvme_dsm_cancel()
does not support other iocb->aiocb = NULL cases. The cancelled request
could hang if nothing completes it. The assertion will complain loudly
if this every happens (may not now, but if someone changes the code in
the future).

Stefan

signature.asc
Description: PGP signature