diff mbox

[v3] xen_disk: split discard input to match internal representation

Message ID 20161123103912.26699-1-olaf@aepfle.de (mailing list archive)
State New, archived
Headers show

Commit Message

Olaf Hering Nov. 23, 2016, 10:39 a.m. UTC
The guest sends discard requests as u64 sector/count pairs, but the
block layer operates internally with s64/s32 pairs. The conversion
leads to IO errors in the guest, the discard request is not processed.

  domU.cfg:
  'vdev=xvda, format=qcow2, backendtype=qdisk, target=/x.qcow2'
  domU:
  mkfs.ext4 -F /dev/xvda
  Discarding device blocks: failed - Input/output error

Fix this by splitting the request into chunks of BDRV_REQUEST_MAX_SECTORS.
Add input range checking to avoid overflow.

Fixes f313520 ("xen_disk: add discard support")

Signed-off-by: Olaf Hering <olaf@aepfle.de>
---
v3:
 turn tab into spaces to fix checkpatch warning
v2:
 adjust overflow check
 add Fixes revspec because the initial commit also failed to convert u64 to s32
 adjust summary

 hw/block/xen_disk.c | 42 ++++++++++++++++++++++++++++++++++++------
 1 file changed, 36 insertions(+), 6 deletions(-)

Comments

Eric Blake Nov. 23, 2016, 11:40 a.m. UTC | #1
On 11/23/2016 04:39 AM, Olaf Hering wrote:
> The guest sends discard requests as u64 sector/count pairs, but the
> block layer operates internally with s64/s32 pairs. The conversion
> leads to IO errors in the guest, the discard request is not processed.
> 
>   domU.cfg:
>   'vdev=xvda, format=qcow2, backendtype=qdisk, target=/x.qcow2'
>   domU:
>   mkfs.ext4 -F /dev/xvda
>   Discarding device blocks: failed - Input/output error
> 
> Fix this by splitting the request into chunks of BDRV_REQUEST_MAX_SECTORS.
> Add input range checking to avoid overflow.
> 
> Fixes f313520 ("xen_disk: add discard support")
> 
> Signed-off-by: Olaf Hering <olaf@aepfle.de>
> ---

Qualifies as a bug fix, so requesting 2.8 inclusion.
Reviewed-by: Eric Blake <eblake@redhat.com>
Kevin Wolf Nov. 23, 2016, 12:27 p.m. UTC | #2
Am 23.11.2016 um 12:40 hat Eric Blake geschrieben:
> On 11/23/2016 04:39 AM, Olaf Hering wrote:
> > The guest sends discard requests as u64 sector/count pairs, but the
> > block layer operates internally with s64/s32 pairs. The conversion
> > leads to IO errors in the guest, the discard request is not processed.
> > 
> >   domU.cfg:
> >   'vdev=xvda, format=qcow2, backendtype=qdisk, target=/x.qcow2'
> >   domU:
> >   mkfs.ext4 -F /dev/xvda
> >   Discarding device blocks: failed - Input/output error
> > 
> > Fix this by splitting the request into chunks of BDRV_REQUEST_MAX_SECTORS.
> > Add input range checking to avoid overflow.
> > 
> > Fixes f313520 ("xen_disk: add discard support")
> > 
> > Signed-off-by: Olaf Hering <olaf@aepfle.de>
> > ---
> 
> Qualifies as a bug fix, so requesting 2.8 inclusion.
> Reviewed-by: Eric Blake <eblake@redhat.com>

Stefano, are you going to merge this or should I take a look?

Kevin
Anthony PERARD Nov. 23, 2016, 3:50 p.m. UTC | #3
On Wed, Nov 23, 2016 at 10:39:12AM +0000, Olaf Hering wrote:
> The guest sends discard requests as u64 sector/count pairs, but the
> block layer operates internally with s64/s32 pairs. The conversion
> leads to IO errors in the guest, the discard request is not processed.
> 
>   domU.cfg:
>   'vdev=xvda, format=qcow2, backendtype=qdisk, target=/x.qcow2'
>   domU:
>   mkfs.ext4 -F /dev/xvda
>   Discarding device blocks: failed - Input/output error
> 
> Fix this by splitting the request into chunks of BDRV_REQUEST_MAX_SECTORS.
> Add input range checking to avoid overflow.
> 
> Fixes f313520 ("xen_disk: add discard support")
> 
> Signed-off-by: Olaf Hering <olaf@aepfle.de>

Acked-by: Anthony PERARD <anthony.perard@citrix.com>
Stefano Stabellini Nov. 23, 2016, 6:46 p.m. UTC | #4
On Wed, 23 Nov 2016, Olaf Hering wrote:
> The guest sends discard requests as u64 sector/count pairs, but the
> block layer operates internally with s64/s32 pairs. The conversion
> leads to IO errors in the guest, the discard request is not processed.
> 
>   domU.cfg:
>   'vdev=xvda, format=qcow2, backendtype=qdisk, target=/x.qcow2'
>   domU:
>   mkfs.ext4 -F /dev/xvda
>   Discarding device blocks: failed - Input/output error
> 
> Fix this by splitting the request into chunks of BDRV_REQUEST_MAX_SECTORS.
> Add input range checking to avoid overflow.
> 
> Fixes f313520 ("xen_disk: add discard support")
> 
> Signed-off-by: Olaf Hering <olaf@aepfle.de>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> v3:
>  turn tab into spaces to fix checkpatch warning
> v2:
>  adjust overflow check
>  add Fixes revspec because the initial commit also failed to convert u64 to s32
>  adjust summary
> 
>  hw/block/xen_disk.c | 42 ++++++++++++++++++++++++++++++++++++------
>  1 file changed, 36 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/block/xen_disk.c b/hw/block/xen_disk.c
> index 3a7dc19..456a2d5 100644
> --- a/hw/block/xen_disk.c
> +++ b/hw/block/xen_disk.c
> @@ -660,6 +660,38 @@ static void qemu_aio_complete(void *opaque, int ret)
>      qemu_bh_schedule(ioreq->blkdev->bh);
>  }
>  
> +static bool blk_split_discard(struct ioreq *ioreq, blkif_sector_t sector_number,
> +                              uint64_t nr_sectors)
> +{
> +    struct XenBlkDev *blkdev = ioreq->blkdev;
> +    int64_t byte_offset;
> +    int byte_chunk;
> +    uint64_t byte_remaining, limit;
> +    uint64_t sec_start = sector_number;
> +    uint64_t sec_count = nr_sectors;
> +
> +    /* Wrap around, or overflowing byte limit? */
> +    if (sec_start + sec_count < sec_count ||
> +        sec_start + sec_count > INT64_MAX >> BDRV_SECTOR_BITS) {
> +        return false;
> +    }
> +
> +    limit = BDRV_REQUEST_MAX_SECTORS << BDRV_SECTOR_BITS;
> +    byte_offset = sec_start << BDRV_SECTOR_BITS;
> +    byte_remaining = sec_count << BDRV_SECTOR_BITS;
> +
> +    do {
> +        byte_chunk = byte_remaining > limit ? limit : byte_remaining;
> +        ioreq->aio_inflight++;
> +        blk_aio_pdiscard(blkdev->blk, byte_offset, byte_chunk,
> +                         qemu_aio_complete, ioreq);
> +        byte_remaining -= byte_chunk;
> +        byte_offset += byte_chunk;
> +    } while (byte_remaining > 0);
> +
> +    return true;
> +}
> +
>  static int ioreq_runio_qemu_aio(struct ioreq *ioreq)
>  {
>      struct XenBlkDev *blkdev = ioreq->blkdev;
> @@ -708,12 +740,10 @@ static int ioreq_runio_qemu_aio(struct ioreq *ioreq)
>          break;
>      case BLKIF_OP_DISCARD:
>      {
> -        struct blkif_request_discard *discard_req = (void *)&ioreq->req;
> -        ioreq->aio_inflight++;
> -        blk_aio_pdiscard(blkdev->blk,
> -                         discard_req->sector_number << BDRV_SECTOR_BITS,
> -                         discard_req->nr_sectors << BDRV_SECTOR_BITS,
> -                         qemu_aio_complete, ioreq);
> +        struct blkif_request_discard *req = (void *)&ioreq->req;
> +        if (!blk_split_discard(ioreq, req->sector_number, req->nr_sectors)) {
> +            goto err;
> +        }
>          break;
>      }
>      default:
>
Stefano Stabellini Nov. 23, 2016, 6:46 p.m. UTC | #5
On Wed, 23 Nov 2016, Kevin Wolf wrote:
> Am 23.11.2016 um 12:40 hat Eric Blake geschrieben:
> > On 11/23/2016 04:39 AM, Olaf Hering wrote:
> > > The guest sends discard requests as u64 sector/count pairs, but the
> > > block layer operates internally with s64/s32 pairs. The conversion
> > > leads to IO errors in the guest, the discard request is not processed.
> > > 
> > >   domU.cfg:
> > >   'vdev=xvda, format=qcow2, backendtype=qdisk, target=/x.qcow2'
> > >   domU:
> > >   mkfs.ext4 -F /dev/xvda
> > >   Discarding device blocks: failed - Input/output error
> > > 
> > > Fix this by splitting the request into chunks of BDRV_REQUEST_MAX_SECTORS.
> > > Add input range checking to avoid overflow.
> > > 
> > > Fixes f313520 ("xen_disk: add discard support")
> > > 
> > > Signed-off-by: Olaf Hering <olaf@aepfle.de>
> > > ---
> > 
> > Qualifies as a bug fix, so requesting 2.8 inclusion.
> > Reviewed-by: Eric Blake <eblake@redhat.com>
> 
> Stefano, are you going to merge this or should I take a look?

I can merge it.

Cheers,

Stefano
Olaf Hering Nov. 23, 2016, 8:44 p.m. UTC | #6
Am 23. November 2016 13:27:13 MEZ, schrieb Kevin Wolf <kwolf@redhat.com>:
>Am 23.11.2016 um 12:40 hat Eric Blake geschrieben:

>> Qualifies as a bug fix, so requesting 2.8 inclusion.
>> Reviewed-by: Eric Blake <eblake@redhat.com>

Is this a can for 2.x?

Olaf
Olaf Hering Nov. 23, 2016, 10:13 p.m. UTC | #7
Am 23. November 2016 21:44:50 MEZ, schrieb Olaf Hering <olaf@aepfle.de>:

>Is this a can for 2.x?
 candidate 


Olaf
diff mbox

Patch

diff --git a/hw/block/xen_disk.c b/hw/block/xen_disk.c
index 3a7dc19..456a2d5 100644
--- a/hw/block/xen_disk.c
+++ b/hw/block/xen_disk.c
@@ -660,6 +660,38 @@  static void qemu_aio_complete(void *opaque, int ret)
     qemu_bh_schedule(ioreq->blkdev->bh);
 }
 
+static bool blk_split_discard(struct ioreq *ioreq, blkif_sector_t sector_number,
+                              uint64_t nr_sectors)
+{
+    struct XenBlkDev *blkdev = ioreq->blkdev;
+    int64_t byte_offset;
+    int byte_chunk;
+    uint64_t byte_remaining, limit;
+    uint64_t sec_start = sector_number;
+    uint64_t sec_count = nr_sectors;
+
+    /* Wrap around, or overflowing byte limit? */
+    if (sec_start + sec_count < sec_count ||
+        sec_start + sec_count > INT64_MAX >> BDRV_SECTOR_BITS) {
+        return false;
+    }
+
+    limit = BDRV_REQUEST_MAX_SECTORS << BDRV_SECTOR_BITS;
+    byte_offset = sec_start << BDRV_SECTOR_BITS;
+    byte_remaining = sec_count << BDRV_SECTOR_BITS;
+
+    do {
+        byte_chunk = byte_remaining > limit ? limit : byte_remaining;
+        ioreq->aio_inflight++;
+        blk_aio_pdiscard(blkdev->blk, byte_offset, byte_chunk,
+                         qemu_aio_complete, ioreq);
+        byte_remaining -= byte_chunk;
+        byte_offset += byte_chunk;
+    } while (byte_remaining > 0);
+
+    return true;
+}
+
 static int ioreq_runio_qemu_aio(struct ioreq *ioreq)
 {
     struct XenBlkDev *blkdev = ioreq->blkdev;
@@ -708,12 +740,10 @@  static int ioreq_runio_qemu_aio(struct ioreq *ioreq)
         break;
     case BLKIF_OP_DISCARD:
     {
-        struct blkif_request_discard *discard_req = (void *)&ioreq->req;
-        ioreq->aio_inflight++;
-        blk_aio_pdiscard(blkdev->blk,
-                         discard_req->sector_number << BDRV_SECTOR_BITS,
-                         discard_req->nr_sectors << BDRV_SECTOR_BITS,
-                         qemu_aio_complete, ioreq);
+        struct blkif_request_discard *req = (void *)&ioreq->req;
+        if (!blk_split_discard(ioreq, req->sector_number, req->nr_sectors)) {
+            goto err;
+        }
         break;
     }
     default: