Re: [Qemu-devel] [PATCH] blk: fix aio context loss on media change

[Kevin Wolf <kwolf@redhat.com>]

Am 14.03.2017 um 18:11 hat Vladimir Sementsov-Ogievskiy geschrieben:
> If we have separate iothread for cdrom, we lose connection to it on
> qmp_blockdev_change_medium, as aio_context is on bds which is dropped
> and switched with new one.
> 
> As an example result, after such media change we have crash on
> virtio_scsi_ctx_check: Assertion `blk_get_aio_context(d->conf.blk) == s->ctx' failed.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
> 
> Hi all!
> 
> We've faced into this assert, and there some kind of fix. I don't sure that
> such fix doesn't break some conceptions, in this case, I hope, someone will
> propose a true-way solution.

The "true way" would be proper AioContext management in the sense that
all users of a BDS can specify a specific AioContext that they need and
if they all agree, callbacks are invoked to change everyone to that
AioContext. If they conflict, attaching the new user would have to error
out.

But we discussed this earlier, and while I'm not completely sure any
more about the details, I seem to remeber that Paolo said something
along the lines that AioContext is going away anyway and building the
code for proper management would be wasted time.

Stefan, Paolo, do you remember the details why we didn't even do a
simple fix like the one below? I think there were some patches on the
list, no?

Kevin

> ======
> Also, on master branch I can't reproduce it as vm crashed earlier, without any
> eject/change, on assert(s->ctx && s->dataplane_started) in
> virtio_scsi_data_plane_handle_ctrl(). It looks like race with
> virtio_scsi_dataplane_start(), and for test (to reproduce assert described above),
> I've "fixed" it with just:
> 
> @@ -63,6 +63,7 @@ static bool virtio_scsi_data_plane_handle_ctrl(VirtIODevice *vdev,
>  {
>      VirtIOSCSI *s = VIRTIO_SCSI(vdev);
>  
> +    sleep(10);
>      assert(s->ctx && s->dataplane_started);
>      return virtio_scsi_handle_ctrl_vq(s, vq);
>  }
> 
> This race is not reproduced for me in our 2.6 based branch.
> 
>  block/block-backend.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/block/block-backend.c b/block/block-backend.c
> index 5742c09c2c..6d5044228e 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c
> @@ -65,6 +65,8 @@ struct BlockBackend {
>      bool allow_write_beyond_eof;
>  
>      NotifierList remove_bs_notifiers, insert_bs_notifiers;
> +
> +    AioContext *aio_context;
>  };
>  
>  typedef struct BlockBackendAIOCB {
> @@ -559,6 +561,10 @@ int blk_insert_bs(BlockBackend *blk, BlockDriverState *bs, Error **errp)
>      }
>      bdrv_ref(bs);
>  
> +    if (blk->aio_context != NULL) {
> +        bdrv_set_aio_context(bs, blk->aio_context);
> +    }
> +
>      notifier_list_notify(&blk->insert_bs_notifiers, blk);
>      if (blk->public.throttle_state) {
>          throttle_timers_attach_aio_context(
> @@ -1607,6 +1613,7 @@ void blk_set_aio_context(BlockBackend *blk, AioContext *new_context)
>  {
>      BlockDriverState *bs = blk_bs(blk);
>  
> +    blk->aio_context = new_context;
>      if (bs) {
>          if (blk->public.throttle_state) {
>              throttle_timers_detach_aio_context(&blk->public.throttle_timers);
> -- 
> 2.11.1
>