From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:48898)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <olaf@aepfle.de>) id 1ag8XZ-0007zV-N0
	for qemu-devel@nongnu.org; Wed, 16 Mar 2016 06:18:58 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <olaf@aepfle.de>) id 1ag8XV-00006f-9P
	for qemu-devel@nongnu.org; Wed, 16 Mar 2016 06:18:57 -0400
Date: Wed, 16 Mar 2016 11:18:40 +0100
From: Olaf Hering <olaf@aepfle.de>
Message-ID: <20160316101840.GA9298@aepfle.de>
References: <20160309121139.GA21975@aepfle.de> <56E0173C.70407@redhat.com>
	<20160309144514.GA29027@aepfle.de>
	<20160309145837.GI5205@noname.redhat.com>
	<20160309165026.GA14059@aepfle.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20160309165026.GA14059@aepfle.de>
Subject: Re: [Qemu-devel] bogus bdrv_check_request in bdrv_co_discard
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: Kevin Wolf <kwolf@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>, qemu-block@nongnu.org, qemu-devel@nongnu.org, Stefan Hajnoczi <stefanha@redhat.com>

On Wed, Mar 09, Olaf Hering wrote:

> On Wed, Mar 09, Kevin Wolf wrote:
> 
> > Removing integer overflow checks without removing the potentially
> > overflowing operation doesn't feel like a particularly good idea,
> > though.
> 
> Why does the code use signed ints anyway for sectors and offset?!

Until this underlying bug is fixed a change like this works for me:

diff --git a/block/io.c b/block/io.c
index a69bfc4..df1e383 100644
--- a/block/io.c
+++ b/block/io.c
@@ -2464,7 +2464,7 @@ static void coroutine_fn bdrv_discard_co_entry(void *opaque)
     rwco->ret = bdrv_co_discard(rwco->bs, rwco->sector_num, rwco->nb_sectors);
 }

-int coroutine_fn bdrv_co_discard(BlockDriverState *bs, int64_t sector_num,
+static int __bdrv_co_discard(BlockDriverState *bs, int64_t sector_num,
                                  int nb_sectors)
 {   
     BdrvTrackedRequest req;
@@ -2546,6 +2546,26 @@ out:
     return ret;
 }

+int coroutine_fn bdrv_co_discard(BlockDriverState *bs, int64_t sector_num,
+                                 int nb_sectors)
+{
+    int num, ret;
+    int limit = BDRV_REQUEST_MAX_SECTORS;
+    int remaining = nb_sectors;
+    int64_t sector_offset = sector_num;
+
+    do {
+        num = remaining > limit ? limit : remaining;
+        ret = __bdrv_co_discard(bs, sector_offset, num);
+        if (ret < 0)
+            break;
+        remaining -= num;
+        sector_offset += num;
+    } while (remaining > 0);
+
+    return ret;
+}
+
 int bdrv_discard(BlockDriverState *bs, int64_t sector_num, int nb_sectors)
 {   
     Coroutine *co;

Olaf