Skip to content
  • Stefan Hajnoczi's avatar
    a937f8e8
    virtio-blk: simplify virtio_blk_dma_restart_cb() · a937f8e8
    Stefan Hajnoczi authored
    
    
    virtio_blk_dma_restart_cb() is tricky because the BH must deal with
    virtio_blk_data_plane_start()/virtio_blk_data_plane_stop() being called.
    
    There are two issues with the code:
    
    1. virtio_blk_realize() should use qdev_add_vm_change_state_handler()
       instead of qemu_add_vm_change_state_handler(). This ensures the
       ordering with virtio_init()'s vm change state handler that calls
       virtio_blk_data_plane_start()/virtio_blk_data_plane_stop() is
       well-defined. Then blk's AioContext is guaranteed to be up-to-date in
       virtio_blk_dma_restart_cb() and it's no longer necessary to have a
       special case for virtio_blk_data_plane_start().
    
    2. Only blk_drain() waits for virtio_blk_dma_restart_cb()'s
       blk_inc_in_flight() to be decremented. The bdrv_drain() family of
       functions do not wait for BlockBackend's in_flight counter to reach
       zero. virtio_blk_data_plane_stop() relies on blk_set_aio_context()'s
       implicit drain, but that's a bdrv_drain() and not a blk_drain().
       Note that virtio_blk_reset() already correctly relies on blk_drain().
       If virtio_blk_data_plane_stop() switches to blk_drain() then we can
       properly wait for pending virtio_blk_dma_restart_bh() calls.
    
    Once these issues are taken care of the code becomes simpler. This
    change is in preparation for multiple IOThreads in virtio-blk where we
    need to clean up the multi-threading behavior.
    
    I ran the reproducer from commit 49b44549 ("virtio-blk: On restart,
    process queued requests in the proper context") to check that there is
    no regression.
    
    Cc: Sergio Lopez <slp@redhat.com>
    Cc: Kevin Wolf <kwolf@redhat.com>
    Cc: Emanuele Giuseppe Esposito <eesposit@redhat.com>
    Signed-off-by: default avatarStefan Hajnoczi <stefanha@redhat.com>
    Acked-by: default avatarMichael S. Tsirkin <mst@redhat.com>
    Reviewed-by: default avatarEmanuele Giuseppe Esposito <eesposit@redhat.com>
    Message-id: 20221102182337.252202-1-stefanha@redhat.com
    Signed-off-by: default avatarStefan Hajnoczi <stefanha@redhat.com>
    a937f8e8
    virtio-blk: simplify virtio_blk_dma_restart_cb()
    Stefan Hajnoczi authored
    
    
    virtio_blk_dma_restart_cb() is tricky because the BH must deal with
    virtio_blk_data_plane_start()/virtio_blk_data_plane_stop() being called.
    
    There are two issues with the code:
    
    1. virtio_blk_realize() should use qdev_add_vm_change_state_handler()
       instead of qemu_add_vm_change_state_handler(). This ensures the
       ordering with virtio_init()'s vm change state handler that calls
       virtio_blk_data_plane_start()/virtio_blk_data_plane_stop() is
       well-defined. Then blk's AioContext is guaranteed to be up-to-date in
       virtio_blk_dma_restart_cb() and it's no longer necessary to have a
       special case for virtio_blk_data_plane_start().
    
    2. Only blk_drain() waits for virtio_blk_dma_restart_cb()'s
       blk_inc_in_flight() to be decremented. The bdrv_drain() family of
       functions do not wait for BlockBackend's in_flight counter to reach
       zero. virtio_blk_data_plane_stop() relies on blk_set_aio_context()'s
       implicit drain, but that's a bdrv_drain() and not a blk_drain().
       Note that virtio_blk_reset() already correctly relies on blk_drain().
       If virtio_blk_data_plane_stop() switches to blk_drain() then we can
       properly wait for pending virtio_blk_dma_restart_bh() calls.
    
    Once these issues are taken care of the code becomes simpler. This
    change is in preparation for multiple IOThreads in virtio-blk where we
    need to clean up the multi-threading behavior.
    
    I ran the reproducer from commit 49b44549 ("virtio-blk: On restart,
    process queued requests in the proper context") to check that there is
    no regression.
    
    Cc: Sergio Lopez <slp@redhat.com>
    Cc: Kevin Wolf <kwolf@redhat.com>
    Cc: Emanuele Giuseppe Esposito <eesposit@redhat.com>
    Signed-off-by: default avatarStefan Hajnoczi <stefanha@redhat.com>
    Acked-by: default avatarMichael S. Tsirkin <mst@redhat.com>
    Reviewed-by: default avatarEmanuele Giuseppe Esposito <eesposit@redhat.com>
    Message-id: 20221102182337.252202-1-stefanha@redhat.com
    Signed-off-by: default avatarStefan Hajnoczi <stefanha@redhat.com>
Loading