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

[Paolo Bonzini]

On 5/24/22 10:08, Stefan Hajnoczi wrote:
> 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?

With multiqueue yeah, we have to replace aio_disable_external() with 
drained_begin/end() callbacks; but I'm not talking about that yet.

> > 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?

Yes, but even in virtio-blk there is this code that runs in the main 
thread and is currently protected by aio_context_acquire/release:

    blk_drain(s->blk);

    /* We drop queued requests after blk_drain() because blk_drain()
     * itself can produce them. */
    while (s->rq) {
        req = s->rq;
        s->rq = req->next;
        virtqueue_detach_element(req->vq, &req->elem, 0);
        virtio_blk_free_request(req);
    }

Maybe it's safe to run it without a lock because it runs after 
virtio_set_status(vdev, 0) but I'd rather play it safe and protect s->rq 
with a lock.

Paolo