[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Paolo Bonzini
Subject: Re: [PATCH v2 2/3] block: make BlockBackend->disable_request_queuing atomic
Date: Thu, 9 Mar 2023 12:18:00 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.7.1

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


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

reply via email to

[Prev in Thread] Current Thread [Next in Thread]