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

[Klaus Jensen <its@irrelevant.dk>]

[-- Attachment #1: Type: text/plain, Size: 2792 bytes --]

On Mar  9 16:03, Stefan Hajnoczi wrote:
> 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.)
> 

Understood! I'll fix that up.

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

Yeah, I understand that there is a risk of dead-lock due to "weird"
scheduling if one is not careful.

Thanks Stefan, these kinds of comments are super helpful when trying to
wrap ones head around this!

I'll give it another spin and post a v2 taking your comments into
account :)

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]