From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40992) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1c9cYw-0007hm-T5 for qemu-devel@nongnu.org; Wed, 23 Nov 2016 13:46:31 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1c9cYv-0002ph-WB for qemu-devel@nongnu.org; Wed, 23 Nov 2016 13:46:30 -0500 Date: Wed, 23 Nov 2016 10:46:18 -0800 (PST) From: Stefano Stabellini In-Reply-To: <20161123103912.26699-1-olaf@aepfle.de> Message-ID: References: <20161123103912.26699-1-olaf@aepfle.de> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Subject: Re: [Qemu-devel] [PATCH v3] xen_disk: split discard input to match internal representation List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Olaf Hering Cc: qemu-block@nongnu.org, Stefano Stabellini , Anthony Perard , Kevin Wolf , Max Reitz , "open list:X86" , "open list:All patches CC here" 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 Reviewed-by: Stefano Stabellini > 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: > From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stefano Stabellini Subject: Re: [PATCH v3] xen_disk: split discard input to match internal representation Date: Wed, 23 Nov 2016 10:46:18 -0800 (PST) Message-ID: References: <20161123103912.26699-1-olaf@aepfle.de> Mime-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Return-path: In-Reply-To: <20161123103912.26699-1-olaf@aepfle.de> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-block-bounces+gceqb-qemu-block=m.gmane.org@nongnu.org Sender: "Qemu-block" To: Olaf Hering Cc: Kevin Wolf , Stefano Stabellini , qemu-block@nongnu.org, "open list:All patches CC here" , Max Reitz , "open list:X86" , Anthony Perard List-Id: xen-devel@lists.xenproject.org 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 Reviewed-by: Stefano Stabellini > 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: >