qemu-commits
[Top][All Lists]
Advanced

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

[Qemu-commits] [qemu/qemu] 632a77: block: all I/O should be completed be


From: GitHub
Subject: [Qemu-commits] [qemu/qemu] 632a77: block: all I/O should be completed before removing...
Date: Tue, 14 Nov 2017 08:50:25 -0800

  Branch: refs/heads/master
  Home:   https://github.com/qemu/qemu
  Commit: 632a77354317df32c7ff2d23424f0559c23fee51
      
https://github.com/qemu/qemu/commit/632a77354317df32c7ff2d23424f0559c23fee51
  Author: Zhengui <address@hidden>
  Date:   2017-11-13 (Mon, 13 Nov 2017)

  Changed paths:
    M block/block-backend.c

  Log Message:
  -----------
  block: all I/O should be completed before removing throttle timers.

In blk_remove_bs, all I/O should be completed before removing throttle
timers. If there has inflight I/O, removing throttle timers here will
cause the inflight I/O never return.
This patch add bdrv_drained_begin before throttle_timers_detach_aio_context
to let all I/O completed before removing throttle timers.

[Moved declaration of bs as suggested by Alberto Garcia
<address@hidden>.
--Stefan]

Signed-off-by: Zhengui <address@hidden>
Reviewed-by: Stefan Hajnoczi <address@hidden>
Reviewed-by: Alberto Garcia <address@hidden>
Message-id: address@hidden
Signed-off-by: Stefan Hajnoczi <address@hidden>


  Commit: dc868fb03b9b829ed9d2ecdae0fcc12f3fe19b4f
      
https://github.com/qemu/qemu/commit/dc868fb03b9b829ed9d2ecdae0fcc12f3fe19b4f
  Author: Stefan Hajnoczi <address@hidden>
  Date:   2017-11-13 (Mon, 13 Nov 2017)

  Changed paths:
    M block/block-backend.c
    M block/throttle-groups.c

  Log Message:
  -----------
  throttle-groups: drain before detaching ThrottleState

I/O requests hang after stop/cont commands at least since QEMU 2.10.0
with -drive iops=100:

  (guest)$ dd if=/dev/zero of=/dev/vdb oflag=direct count=1000
  (qemu) stop
  (qemu) cont
  ...I/O is stuck...

This happens because blk_set_aio_context() detaches the ThrottleState
while requests may still be in flight:

  if (tgm->throttle_state) {
      throttle_group_detach_aio_context(tgm);
      throttle_group_attach_aio_context(tgm, new_context);
  }

This patch encloses the detach/attach calls in a drained region so no
I/O request is left hanging.  Also add assertions so we don't make the
same mistake again in the future.

Reported-by: Yongxue Hong <address@hidden>
Signed-off-by: Stefan Hajnoczi <address@hidden>
Reviewed-by: Alberto Garcia <address@hidden>
Message-id: address@hidden
Signed-off-by: Stefan Hajnoczi <address@hidden>


  Commit: 48bf7ea81aa848027bad24f7e7791b503dff727d
      
https://github.com/qemu/qemu/commit/48bf7ea81aa848027bad24f7e7791b503dff727d
  Author: Alberto Garcia <address@hidden>
  Date:   2017-11-13 (Mon, 13 Nov 2017)

  Changed paths:
    M block/block-backend.c

  Log Message:
  -----------
  block: Check for inserted BlockDriverState in blk_io_limits_disable()

When you set I/O limits using block_set_io_throttle or the command
line throttling.* options they are kept in the BlockBackend regardless
of whether a BlockDriverState is attached to the backend or not.

Therefore when removing the limits using blk_io_limits_disable() we
need to check if there's a BDS before attempting to drain it, else it
will crash QEMU. This can be reproduced very easily using HMP:

     (qemu) drive_add 0 if=none,throttling.iops-total=5000
     (qemu) drive_del none0

Reported-by: sochin jiang <address@hidden>
Signed-off-by: Alberto Garcia <address@hidden>
Reviewed-by: Max Reitz <address@hidden>
Message-id: address@hidden
Signed-off-by: Stefan Hajnoczi <address@hidden>


  Commit: c89bcf3af01e7a8834cca5344e098bf879e99999
      
https://github.com/qemu/qemu/commit/c89bcf3af01e7a8834cca5344e098bf879e99999
  Author: Alberto Garcia <address@hidden>
  Date:   2017-11-13 (Mon, 13 Nov 2017)

  Changed paths:
    M block/block-backend.c

  Log Message:
  -----------
  block: Leave valid throttle timers when removing a BDS from a backend

If a BlockBackend has I/O limits set then its ThrottleGroupMember
structure uses the AioContext from its attached BlockDriverState.
Those two contexts must be kept in sync manually. This is not
ideal and will be fixed in the future by removing the throttling
configuration from the BlockBackend and storing it in an implicit
filter node instead, but for now we have to live with this.

When you remove the BlockDriverState from the backend then the
throttle timers are destroyed. If a new BlockDriverState is later
inserted then they are created again using the new AioContext.

There are a couple of problems with this:

   a) The code manipulates the timers directly, leaving the
      ThrottleGroupMember.aio_context field in an inconsisent state.

   b) If you remove the I/O limits (e.g by destroying the backend)
      when the timers are gone then throttle_group_unregister_tgm()
      will attempt to destroy them again, crashing QEMU.

While b) could be fixed easily by allowing the timers to be freed
twice, this would result in a situation in which we can no longer
guarantee that a valid ThrottleState has a valid AioContext and
timers.

This patch ensures that the timers and AioContext are always valid
when I/O limits are set, regardless of whether the BlockBackend has a
BlockDriverState inserted or not.

[Fixed "There'a" typo as suggested by Max Reitz <address@hidden>
--Stefan]

Reported-by: sochin jiang <address@hidden>
Signed-off-by: Alberto Garcia <address@hidden>
Reviewed-by: Max Reitz <address@hidden>
Message-id: address@hidden
Signed-off-by: Stefan Hajnoczi <address@hidden>


  Commit: 0761562687e0d8135310a94b1d3e08376387c027
      
https://github.com/qemu/qemu/commit/0761562687e0d8135310a94b1d3e08376387c027
  Author: Alberto Garcia <address@hidden>
  Date:   2017-11-13 (Mon, 13 Nov 2017)

  Changed paths:
    M tests/qemu-iotests/093
    M tests/qemu-iotests/093.out

  Log Message:
  -----------
  qemu-iotests: Test I/O limits with removable media

This test hotplugs a CD drive to a VM and checks that I/O limits can
be set only when the drive has media inserted and that they are kept
when the media is replaced.

This also tests the removal of a device with valid I/O limits set but
no media inserted. This involves deleting and disabling the limits
of a BlockBackend without BlockDriverState, a scenario that has been
crashing until the fixes from the last couple of patches.

[Python PEP8 fixup: "Don't use spaces are the = sign when used to
indicate a keyword argument or a default parameter value"
--Stefan]

Signed-off-by: Alberto Garcia <address@hidden>
Reviewed-by: Max Reitz <address@hidden>
Message-id: address@hidden
Signed-off-by: Stefan Hajnoczi <address@hidden>


  Commit: 191b5fbfa66e5b23e2150f3c6981d30eb84418a9
      
https://github.com/qemu/qemu/commit/191b5fbfa66e5b23e2150f3c6981d30eb84418a9
  Author: Peter Maydell <address@hidden>
  Date:   2017-11-14 (Tue, 14 Nov 2017)

  Changed paths:
    M block/block-backend.c
    M block/throttle-groups.c
    M tests/qemu-iotests/093
    M tests/qemu-iotests/093.out

  Log Message:
  -----------
  Merge remote-tracking branch 'remotes/stefanha/tags/block-pull-request' into 
staging

Pull request

The following disk I/O throttling fixes solve recent bugs.

# gpg: Signature made Tue 14 Nov 2017 10:37:12 GMT
# gpg:                using RSA key 0x9CA4ABB381AB73C8
# gpg: Good signature from "Stefan Hajnoczi <address@hidden>"
# gpg:                 aka "Stefan Hajnoczi <address@hidden>"
# Primary key fingerprint: 8695 A8BF D3F9 7CDA AC35  775A 9CA4 ABB3 81AB 73C8

* remotes/stefanha/tags/block-pull-request:
  qemu-iotests: Test I/O limits with removable media
  block: Leave valid throttle timers when removing a BDS from a backend
  block: Check for inserted BlockDriverState in blk_io_limits_disable()
  throttle-groups: drain before detaching ThrottleState
  block: all I/O should be completed before removing throttle timers.

Signed-off-by: Peter Maydell <address@hidden>


Compare: https://github.com/qemu/qemu/compare/0dc8874aded7...191b5fbfa66e

reply via email to

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