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

[Paolo Bonzini]

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.

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.

Paolo

>           blk_dec_in_flight(blk);
>           qemu_co_queue_wait(&blk->queued_requests, NULL);
>           blk_inc_in_flight(blk);
> -- 2.39.2