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

[Paolo Bonzini <pbonzini@redhat.com>]

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