[Qemu-devel] bogus bdrv_check_request in bdrv_co_discard

[Qemu-devel] bogus bdrv_check_request in bdrv_co_discard

[Olaf Hering]

What is the purpose of the bdrv_check_request() call in bdrv_co_discard?

It seems a frontend cant possibly know what the limit is in the
qemu-of-the-day, I found no interface to propagate
BDRV_REQUEST_MAX_SECTORS into the guest.

I think to handle nb_sectors > BDRV_REQUEST_MAX_SECTORS bdrv_co_discard
has to split the request into smaller chunks, just as it does a few
lines down in the function.


Olaf

Re: [Qemu-devel] bogus bdrv_check_request in bdrv_co_discard

[Paolo Bonzini]

On 09/03/2016 13:11, Olaf Hering wrote:
> What is the purpose of the bdrv_check_request() call in bdrv_co_discard?
> 
> It seems a frontend cant possibly know what the limit is in the
> qemu-of-the-day, I found no interface to propagate
> BDRV_REQUEST_MAX_SECTORS into the guest.

It depends on the backend.  For example SCSI uses the block limits VPD
page.  It has a default max-unmap-size of 1 GiB, which happens to be
smaller than BDRV_REQUEST_MAX_SECTORS too.

It probably should range check max_unmap_size and max_io_size against
BDRV_REQUEST_MAX_SECTORS, and reject anything bigger than that, though.

Paolo

> I think to handle nb_sectors > BDRV_REQUEST_MAX_SECTORS bdrv_co_discard
> has to split the request into smaller chunks, just as it does a few
> lines down in the function.

Re: [Qemu-devel] bogus bdrv_check_request in bdrv_co_discard

[Kevin Wolf]

Am 09.03.2016 um 15:45 hat Olaf Hering geschrieben:
> On Wed, Mar 09, Paolo Bonzini wrote:
> 
> > It probably should range check max_unmap_size and max_io_size against
> > BDRV_REQUEST_MAX_SECTORS, and reject anything bigger than that, though.
> 
> Are you sure? Shouldnt the only check be if sect+num is inside the disk
> size? And everything smaller should be automatically split by qemu to
> whatever num.
> 
> I will see what works for me, probably something like this is already enough
> (for 2.0.2):
> 
> --- xen-4.5.2-testing.orig/tools/qemu-xen-dir-remote/block.c
> +++ xen-4.5.2-testing/tools/qemu-xen-dir-remote/block.c
> @@ -4898,7 +4898,8 @@ int coroutine_fn bdrv_co_discard(BlockDr
>  
>      if (!bs->drv) {
>          return -ENOMEDIUM;
> -    } else if (bdrv_check_request(bs, sector_num, nb_sectors)) {
> +    } else if (bdrv_check_byte_request(bs, sector_num * BDRV_SECTOR_SIZE,
> +                                           nb_sectors * BDRV_SECTOR_SIZE)) {
>          return -EIO;

Removing integer overflow checks without removing the potentially
overflowing operation doesn't feel like a particularly good idea,
though.

Kevin

Re: [Qemu-devel] bogus bdrv_check_request in bdrv_co_discard

[Olaf Hering]

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?!

Olaf

Re: [Qemu-devel] bogus bdrv_check_request in bdrv_co_discard

[Olaf Hering]

On Wed, Mar 09, Olaf Hering wrote:

> Why does the code use signed ints anyway for sectors and offset?!

I have to check mainline (next week), at least this fixes mkfs for me:

+++ xen-4.4.3-testing/tools/qemu-xen-dir-remote/block/raw-posix.c
@@ -792,8 +792,8 @@ static BlockDriverAIOCB *paio_submit(Blo
         acb->aio_iov = qiov->iov;
         acb->aio_niov = qiov->niov;
     }
-    acb->aio_nbytes = nb_sectors * 512;
-    acb->aio_offset = sector_num * 512;
+    acb->aio_nbytes = nb_sectors * 512U;
+    acb->aio_offset = sector_num * 512U;
 
     trace_paio_submit(acb, opaque, sector_num, nb_sectors, type);
     pool = aio_get_thread_pool(bdrv_get_aio_context(bs));

Olaf

Re: [Qemu-devel] bogus bdrv_check_request in bdrv_co_discard

[Olaf Hering]

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