From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:40141)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <stefanha@gmail.com>) id 1VAIjl-0007KP-3j
	for qemu-devel@nongnu.org; Fri, 16 Aug 2013 08:02:45 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <stefanha@gmail.com>) id 1VAIjc-0003QT-KV
	for qemu-devel@nongnu.org; Fri, 16 Aug 2013 08:02:37 -0400
Received: from mail-wg0-x22c.google.com ([2a00:1450:400c:c00::22c]:61836)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <stefanha@gmail.com>) id 1VAIjc-0003QH-EX
	for qemu-devel@nongnu.org; Fri, 16 Aug 2013 08:02:28 -0400
Received: by mail-wg0-f44.google.com with SMTP id l18so1455451wgh.11
	for <qemu-devel@nongnu.org>; Fri, 16 Aug 2013 05:02:27 -0700 (PDT)
Date: Fri, 16 Aug 2013 14:02:24 +0200
From: Stefan Hajnoczi <stefanha@gmail.com>
Message-ID: <20130816120224.GB22193@stefanha-thinkpad.redhat.com>
References: <1376326396-7676-1-git-send-email-benoit@irqsave.net>
	<1376326396-7676-4-git-send-email-benoit@irqsave.net>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <1376326396-7676-4-git-send-email-benoit@irqsave.net>
Subject: Re: [Qemu-devel] [PATCH V5 3/5] block: Enable the new throttling
 code in the block layer.
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: =?iso-8859-1?Q?Beno=EEt?= Canet <benoit@irqsave.net>
Cc: kwolf@redhat.com, pbonzini@redhat.com, qemu-devel@nongnu.org, stefanha@redhat.com

On Mon, Aug 12, 2013 at 06:53:14PM +0200, Benoît Canet wrote:
> +/* this function drain all the throttled IOs */
> +static bool bdrv_drain_throttled(BlockDriverState *bs)
> +{
> +    bool drained = false;
> +    bool enabled = bs->io_limits_enabled;
> +    int i;
>  
> -    do {} while (qemu_co_enter_next(&bs->throttled_reqs));
> +    bs->io_limits_enabled = false;
>  
> -    if (bs->block_timer) {
> -        qemu_del_timer(bs->block_timer);
> -        qemu_free_timer(bs->block_timer);
> -        bs->block_timer = NULL;
> +    for (i = 0; i < 2; i++) {
> +        while (qemu_co_enter_next(&bs->throttled_reqs[i])) {
> +            drained = true;
> +        }
>      }

This function submits throttled requests but it doesn't drain them -
they might still be executing when this function returns!

> +/* should be called before bdrv_set_io_limits if a limit is set */
>  void bdrv_io_limits_enable(BlockDriverState *bs)
>  {
> -    qemu_co_queue_init(&bs->throttled_reqs);
> -    bs->block_timer = qemu_new_timer_ns(vm_clock, bdrv_block_timer, bs);

Make sure we never double-initialized ->throttle_state:

assert(!bs->io_limits enabled);

> @@ -2512,11 +2552,6 @@ static int coroutine_fn bdrv_co_do_readv(BlockDriverState *bs,
>          return -EIO;
>      }
>  
> -    /* throttling disk read I/O */
> -    if (bs->io_limits_enabled) {
> -        bdrv_io_limits_intercept(bs, false, nb_sectors);
> -    }
> -

Why move bdrv_io_limits_intercept() into tracked_request_begin()?  IMO
tracked_request_begin() should only create the tracked request, it
shouldn't do unrelated stuff like I/O throttling and yielding.  If it
does then it must be declared coroutine_fn.