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
signature.asc
Description: PGP signature