Re: [Qemu-devel] [Qemu-block] [PATCH v2] throttle: fix a qemu crash problem when calling blk_delete

[Stefan Hajnoczi <stefanha@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2398 bytes --]

On Tue, Oct 24, 2017 at 11:33:51AM +0800, sochin jiang wrote:
> commit 7ca7f0 moves the throttling related part of the BDS life cycle
> management to BlockBackend, adds call to
> throttle_timers_detach_aio_context in blk_remove_bs.  commit 1606e
> remove a block device from its throttle group in blk_delete by calling
> blk_io_limits_disable, this fix an easily reproducible qemu crash. But
> delete a BB without a BDS inserted could easily cause a qemu crash too
> by calling bdrv_drained_begin in blk_io_limits_disable. Say, a simply
> drive_add and then a drive_del command.
> 
> This patch removes draining BDS by calling throttle_group_unregister_tgm
> directly instead of blk_io_limits_disable, leaves draining operation to
> blk_remove_bs in case that there is no BDS inserted. Futhermore, make sure
> throttle timers are initialized or attached before throttle_timers_destroy
> is called in throttle_group_unregister_tgm.
> 
> Signed-off-by: sochin jiang <sochin.jiang@huawei.com>
> ---
>  block/block-backend.c   | 2 +-
>  block/throttle-groups.c | 4 +++-
>  2 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/block/block-backend.c b/block/block-backend.c
> index 45d9101..39c7cca 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c
> @@ -341,7 +341,7 @@ static void blk_delete(BlockBackend *blk)
>      assert(!blk->name);
>      assert(!blk->dev);
>      if (blk->public.throttle_group_member.throttle_state) {
> -        blk_io_limits_disable(blk);
> +        throttle_group_unregister_tgm(&blk->public.throttle_group_member);

The following assertions fail without the drain when there are pending
requests:

  void throttle_group_unregister_tgm(ThrottleGroupMember *tgm)
  {
      ThrottleState *ts = tgm->throttle_state;
      ThrottleGroup *tg = container_of(ts, ThrottleGroup, ts);
      ThrottleGroupMember *token;
      int i;

      if (!ts) {
          /* Discard already unregistered tgm */
          return;
      }

      assert(tgm->pending_reqs[0] == 0 && tgm->pending_reqs[1] == 0);
      assert(qemu_co_queue_empty(&tgm->throttled_reqs[0]));
      assert(qemu_co_queue_empty(&tgm->throttled_reqs[1]));

A safer approach is making blk_io_limits_disable(blk) skip the draining
when blk_bs(blk) == NULL.  That is the only case where we are 100% sure
that there are no pending requests.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]