Re: [RFC PATCH v2 0/8] Removal of AioContext lock, bs->parents and ->children: new rwlock

[Stefan Hajnoczi]

On Tue, May 24, 2022 at 09:55:39AM +0200, Paolo Bonzini wrote:
> On 5/22/22 17:06, Stefan Hajnoczi wrote:
> > However, I hit on a problem that I think Emanuele and Paolo have already
> > pointed out: draining is GS & IO. This might have worked under the 1 
> > IOThread
> > model but it does not make sense for multi-queue. It is possible to submit 
> > I/O
> > requests in drained sections. How can multiple threads be in drained 
> > sections
> > simultaneously and possibly submit further I/O requests in their drained
> > sections? Those sections wouldn't be "drained" in any useful sense of the 
> > word.
> 
> Yeah, that works only if the drained sections are well-behaved.
> 
> "External" sources of I/O are fine; they are disabled using is_external, and
> don't drain themselves I think.

I/O requests for a given BDS may be executing in multiple AioContexts,
so how do you call aio_disable_external() on all relevant AioContexts?

We covered this below but I wanted to reply here in case someone else
reads this part without reading the rest.

> Mirror is the only I/O user of drain, and it's fine because it never submits
> I/O to the drained BDS.
>
> Drained sections in the main thread can be special cased to allow I/O
> (wrlock in this series would also allow I/O).
> 
> So I think that the "cooperation from all relevant places" that Kevin
> mentioned is already there, except for coroutine commands in the monitor.
> Those are a bad idea in my opinion and I'd rather revert commit eb94b81a94
> ("block: Convert 'block_resize' to coroutine") until we have a clearer idea
> of how to handle them.
> 
> I agree that it's basically impossible to review the change.  On the other
> hand, there's already a substantial amount of faith involved in the
> correctness of the current code.
> 
> In particular the AioContext lock does absolutely nothing to protect
> corutines in the main thread against graph changes---both from the monitor
> (including BHs as in "block: Fix BB.root changing across bdrv_next()") and
> from BDS coroutines.  The former are unprotected; the latter are protected
> by drain only: using drain to protect against graph writes would be a matter
> of extending *existing* faith to the multi-iothread case.
> 
> Once the deadlock is broken, we can proceed to remove the AioContext lock
> and then introduce actual coroutine-based locking.
> 
> > Possible steps for AioContext removal
> > -------------------------------------
> > I also wanted to share my assumptions about multi-queue and AioContext 
> > removal.
> > Please let me know if anything seems wrong or questionable:
> > 
> > - IO code can execute in any thread that has an AioContext.
> > - Multiple threads may execute a IO code at the same time.
> > - GS code only execute under the BQL.
> > 
> > For AioContext removal this means:
> > 
> > - bdrv_get_aio_context() becomes mostly meaningless since there is no need 
> > for
> >    a special "home" AioContext.
> 
> Correct.  bdrv_set_aio_context() remains useful as a way to set a home
> AioContext for sockets.
> 
> > - bdrv_coroutine_enter() becomes mostly meaningless because there is no 
> > need to
> >    run a coroutine in the BDS's AioContext.
> > - aio_disable_external(bdrv_get_aio_context(bs)) no longer works because 
> > many
> >    threads/AioContexts may submit new I/O requests. 
> > BlockDevOps.drained_begin()
> >    may be used instead (e.g. to temporarily disable ioeventfds on a 
> > multi-queue
> >    virtio-blk device).
> 
> This is a change that can be done independent of this work.
> 
> > - AIO_WAIT_WHILE() simplifies to
> > 
> >      while ((cond)) {
> >          aio_poll(qemu_get_current_aio_context(), true);
> >          ...
> >      }
> > 
> >    and the distinction between home AioContext and non-home context is
> >    eliminated. AioContext unlocking is dropped.
> 
> (I'll reply on this from elsewhere in the thread).
> 
> > Does this make sense? I haven't seen these things in recent patch series.
> 
> I agree, and yeah all these are blocked on protecting graph modifications.
> 
> In parallel to the block layer discussions, it's possible to work on
> introducing a request queue lock in virtio-blk and virtio-scsi.  That's the
> only thing that relies on the AioContext lock outside the block layer.

I'm not sure what the request queue lock protects in virtio-blk? In
virtio-scsi I guess a lock is needed to protect SCSI target emulation
state?

Stefan

signature.asc
Description: PGP signature