On 11/18/2016 04:24 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. > > Signed-off-by: Olaf Hering <[email protected]> > --- > hw/block/xen_disk.c | 45 +++++++++++++++++++++++++++++++++++++++------ > 1 file changed, 39 insertions(+), 6 deletions(-) > > diff --git a/hw/block/xen_disk.c b/hw/block/xen_disk.c > index 3a7dc19..c3f572f 100644 > --- a/hw/block/xen_disk.c > +++ b/hw/block/xen_disk.c > @@ -660,6 +660,41 @@ 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 sec_start = sector_number; > + uint64_t sec_count = nr_sectors; > + uint64_t byte_remaining; > + uint64_t limit = BDRV_REQUEST_MAX_SECTORS << BDRV_SECTOR_BITS;
[For reference, this limit is the same as rounding INT32_MAX down to the
nearest 512-byte limit, or 0x7ffffe00]
> +
> + /* Wrap around? */
> + if ((sec_start + sec_count) < sec_count) {
> + return false;
> + }
> + /* Overflowing byte limit? */
> + if ((sec_start + sec_count) > ((INT64_MAX + INT_MAX) >>
> BDRV_SECTOR_BITS)) {
This is undefined. INT64_MAX + anything non-negative overflows int64,
and even if you treat overflow as defined by twos-complement
representation (which creates a negative number), shifting a negative
number is also undefined.
If you are trying to detect guests that make a request that would cover
more than INT64_MAX bytes, you can simplify. Besides, for as much
storage as there is out there, I seriously doubt ANYONE will ever have
2^63 bytes addressable through a single device. Why not just write it as:
if ((INT64_MAX >> BDRV_SECTOR_BITS) - sec_count < sec_start) {
> + return false;
> + }
> +
> + 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);
This part looks reasonable.
> +
> + return true;
> +}
> +
> static int ioreq_runio_qemu_aio(struct ioreq *ioreq)
> {
> struct XenBlkDev *blkdev = ioreq->blkdev;
> @@ -708,12 +743,10 @@ static int ioreq_runio_qemu_aio(struct ioreq *ioreq)
> break;
> case BLKIF_OP_DISCARD:
> {
> - struct blkif_request_discard *discard_req = (void *)&ioreq->req;
The old code had it...
> - 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;
...but C doesn't require a cast to void*. As long as you are touching
this, you could remove the cast (unless I'm missing something, and the
cast is also there to cast away const).
> + if (!blk_split_discard(ioreq, req->sector_number, req->nr_sectors)) {
> + goto err;
> + }
> break;
> }
> default:
>
>
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
