Re: [PATCH v2 2/3] block: make BlockBackend->disable_request_queuing atomic

[Stefan Hajnoczi <stefanha@redhat.com>]

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

On Thu, Mar 09, 2023 at 12:18:00PM +0100, Paolo Bonzini wrote:
> On 3/7/23 22:04, Stefan Hajnoczi wrote:
> >   static int coroutine_fn GRAPH_RDLOCK
> > @@ -1271,7 +1271,8 @@ static void coroutine_fn blk_wait_while_drained(BlockBackend *blk)
> >   {
> >       assert(blk->in_flight > 0);
> > -    if (qatomic_read(&blk->quiesce_counter) && !blk->disable_request_queuing) {
> > +    if (qatomic_read(&blk->quiesce_counter) &&
> > +        !qatomic_read(&blk->disable_request_queuing)) {
> 
> The qatomic_inc in blk_inc_in_flight() made me a bit nervous that
> smp_mb__after_rmw() was needed there, but it's okay.

Yes. I wrote it under the assumption that sequentially consistent
operations like qatomic_inc() are implicit barriers.

> First, anyway blk_wait_while_drained() has to _eventually_ pause the device,
> not immediately.  Even if it misses that blk->quiesce_counter == 1, the I/O
> will proceed and it'll just take a little more polling before
> bdrv_drained_begin() exits.
> 
> Second, I checked with CPPMEM the barriers in AIO_WAIT_WHILE() and
> aio_wait_kick() save the day, even if loading blk->quiesce_counter is
> reordered before the incremented value (1) is stored to blk->in_flight.
> 
> The CPPMEM model here uses mo_relaxed to force all possible kinds of havoc:
> 
> int main() {
>   atomic_int quiesce_counter = 0;
>   atomic_int waiters = 0;
>   atomic_int in_flight = 0;
> 
>   {{{ { quiesce_counter.store(1, mo_relaxed);
>         waiters.store(1, mo_relaxed);    // AIO_WAIT_WHILE starts here
>         atomic_thread_fence(mo_seq_cst);
>         in_flight.load(mo_relaxed).readsvalue(1); } // if 1, sleep
> 
>   ||| { in_flight.store(1, mo_relaxed);  // bdrv_inc_in_flight
>         quiesce_counter.load(mo_relaxed).readsvalue(1); // go down "if"
>         in_flight.store(0, mo_release);  // bdrv_dec_in_flight
>         atomic_thread_fence(mo_seq_cst); // aio_wait_kick starts here
>         waiters.load(mo_relaxed).readsvalue(0); }   // if 0, do not wake
>   }}};
> 
>   return 0;
> }
> 
> 
> Because CPPMEM shows no execution consistent with the buggy .readsvalue(),
> either AIO_WAIT_WHILE will not go to sleep or it will be woken up with
> in_flight == 0.  The polling loop ends and drained_end restarts the
> coroutine from blk->queued_requests.

Okay.

Stefan

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