Re: [PATCH v4 05/25] block/block-backend.c: assertions for block-backend

[Hanna Reitz]

On 16.11.21 11:15, Emanuele Giuseppe Esposito wrote:
> On 12/11/2021 12:01, Hanna Reitz wrote:
> > On 25.10.21 12:17, Emanuele Giuseppe Esposito wrote:
> > > All the global state (GS) API functions will check that
> > > qemu_in_main_thread() returns true. If not, it means
> > > that the safety of BQL cannot be guaranteed, and
> > > they need to be moved to I/O.
> > >
> > > Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> > > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> > > ---
> > >   block/block-backend.c  | 90 
> > > +++++++++++++++++++++++++++++++++++++++++-
> > >   softmmu/qdev-monitor.c |  2 +
> > >   2 files changed, 91 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/block/block-backend.c b/block/block-backend.c
> > > index 0afc03fd66..ed45576007 100644
> > > --- a/block/block-backend.c
> > > +++ b/block/block-backend.c
> >
> > [...]
> >
> > > @@ -1550,6 +1596,7 @@ BlockAIOCB *blk_aio_pwritev(BlockBackend *blk, 
> > > int64_t offset,
> > >   void blk_aio_cancel(BlockAIOCB *acb)
> > >   {
> > > +    assert(qemu_in_main_thread());
> > >       bdrv_aio_cancel(acb);
> > >   }
> >
> > This function is in block-backend-io.h, though.
>
> I am confused a little on the {blk/bdrv}_aio functions, namely
> blk_aio_cancel
> bdrv_aio_cancel
> blk_aio_cancel_async
> bdrv_aio_cancel_async
>
> Do you think they should be I/O? The assertion seems to hold though.

Hm, semantically I would have classified them as I/O because they don’t 
modify state.  I don’t have a strong opinion, though, because they don’t 
actually do I/O.  They just cancel other I/O requests.

Most importantly though now I see there’s a comment in bdrv_aio_cancel() 
that states that “thread-safe code should use bdrv_aio_cancel_async 
exclusively”, which to me implies that bdrv_aio_cancel() (and 
blk_aio_cancel()) must be classified as GS, and it sounds like 
bdrv_aio_cancel_async() (and blk_aio_cancel_async()) should be 
classified as I/O.  Looking at the AIOCBInfo.cancel_async 
implementations (called by bdrv_aio_cancel_async()) I’m not sure they’re 
all really thread-safe, though...?  But at least bdrv_aio_cancel() 
claims they should be, so...

It seems to me like the intended separation is that bdrv_aio_cancel() 
should be GS and bdrv_aio_cancel_async() should be I/O.  I can’t verify 
that the .cancel_async implementations are really thread-safe, but 
neither can I verify that blk_aio_cancel_async() is only called by BQL 
callers.  That the assertions hold during testing isn’t too convincing 
for me, because we never wrote tests specifically to exercise these paths.

Hanna