Re: [Qemu-devel] [PATCH] xen_disk: convert discard input to byte ranges

2016-11-23 Thread Stefano Stabellini
On Wed, 23 Nov 2016, Olaf Hering wrote: > On Wed, Nov 23, Olaf Hering wrote: > > > > > +if (!blk_split_discard(ioreq, req->sector_number, > > > > req->nr_sectors)) { > > > > +goto err; > > > How is error handling supposed to work here? > > In the guest the cmd is stuck,

Re: [Qemu-devel] [PATCH] xen_disk: convert discard input to byte ranges

2016-11-23 Thread Olaf Hering
Ping. On Fri, Nov 18, Olaf Hering wrote: > On Fri, Nov 18, Olaf Hering wrote: > > > @@ -708,12 +743,10 @@ static int ioreq_runio_qemu_aio(struct ioreq *ioreq) > > +if (!blk_split_discard(ioreq, req->sector_number, > > req->nr_sectors)) { > > +goto err; > > How is error

Re: [Qemu-devel] [PATCH] xen_disk: convert discard input to byte ranges

2016-11-23 Thread Olaf Hering
On Wed, Nov 23, Olaf Hering wrote: > > > +if (!blk_split_discard(ioreq, req->sector_number, > > > req->nr_sectors)) { > > > +goto err; > > How is error handling supposed to work here? In the guest the cmd is stuck, instead of getting an IO error: [ 91.966404] mkfs.ext4

Re: [Qemu-devel] [PATCH] xen_disk: convert discard input to byte ranges

2016-11-22 Thread Eric Blake
On 11/22/2016 11:00 AM, Olaf Hering wrote: > On Tue, Nov 22, Eric Blake wrote: > >> if (sec_start + sec_count < sec_count || >> sec_start > (INT64_MAX >> BDRV_SECTOR_BITS) - sec_count) { >> return false; >> } > > My point was: how does this handle sec_start=0,sec_count=UINT64_MAX or >

Re: [Qemu-devel] [PATCH] xen_disk: convert discard input to byte ranges

2016-11-22 Thread Olaf Hering
On Tue, Nov 22, Eric Blake wrote: > if (sec_start + sec_count < sec_count || > sec_start > (INT64_MAX >> BDRV_SECTOR_BITS) - sec_count) { > return false; > } My point was: how does this handle sec_start=0,sec_count=UINT64_MAX or sec_start=INT64_MAX,sec_count=INT64_MAX? For me this gets

Re: [Qemu-devel] [PATCH] xen_disk: convert discard input to byte ranges

2016-11-22 Thread Eric Blake
On 11/22/2016 10:12 AM, Olaf Hering wrote: > On Fri, Nov 18, Eric Blake wrote: > >> if (sec_start > (INT64_MAX >> BDRV_SECTOR_BITS) - sec_count) > > I have looked at this for a while now and cant spot how this would cover > all cases. Are you saying there should be just a single overflow check,

Re: [Qemu-devel] [PATCH] xen_disk: convert discard input to byte ranges

2016-11-22 Thread Olaf Hering
On Fri, Nov 18, Eric Blake wrote: > if (sec_start > (INT64_MAX >> BDRV_SECTOR_BITS) - sec_count) I have looked at this for a while now and cant spot how this would cover all cases. Are you saying there should be just a single overflow check, yours? My change has two: one to check for wrap around

Re: [Qemu-devel] [PATCH] xen_disk: convert discard input to byte ranges

2016-11-18 Thread Eric Blake
On 11/18/2016 11:41 AM, Olaf Hering wrote: > On Fri, Nov 18, Eric Blake wrote: > >> On 11/18/2016 04:24 AM, Olaf Hering wrote: >>> +/* Overflowing byte limit? */ >>> +if ((sec_start + sec_count) > ((INT64_MAX + INT_MAX) >> >>> BDRV_SECTOR_BITS)) { >> This is undefined. INT64_MAX +

Re: [Qemu-devel] [PATCH] xen_disk: convert discard input to byte ranges

2016-11-18 Thread Olaf Hering
On Fri, Nov 18, Eric Blake wrote: > On 11/18/2016 04:24 AM, Olaf Hering wrote: > > +/* 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, The expanded

Re: [Qemu-devel] [PATCH] xen_disk: convert discard input to byte ranges

2016-11-18 Thread Eric Blake
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,

Re: [Qemu-devel] [PATCH] xen_disk: convert discard input to byte ranges

2016-11-18 Thread Kevin Wolf
Am 18.11.2016 um 15:35 hat Eric Blake geschrieben: > On 11/18/2016 08:19 AM, Olaf Hering wrote: > > Am 18. November 2016 14:43:18 MEZ, schrieb Eric Blake : > >> On 11/18/2016 04:24 AM, Olaf Hering wrote: > >>> The guest sends discard requests as u64 sector/count pairs, but the >

Re: [Qemu-devel] [PATCH] xen_disk: convert discard input to byte ranges

2016-11-18 Thread Eric Blake
On 11/18/2016 08:19 AM, Olaf Hering wrote: > Am 18. November 2016 14:43:18 MEZ, schrieb Eric Blake : >> 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

Re: [Qemu-devel] [PATCH] xen_disk: convert discard input to byte ranges

2016-11-18 Thread Olaf Hering
Am 18. November 2016 14:43:18 MEZ, schrieb Eric Blake : >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,

Re: [Qemu-devel] [PATCH] xen_disk: convert discard input to byte ranges

2016-11-18 Thread Eric Blake
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. Doesn't the block layer already split

Re: [Qemu-devel] [PATCH] xen_disk: convert discard input to byte ranges

2016-11-18 Thread Olaf Hering
On Fri, Nov 18, Olaf Hering wrote: > @@ -708,12 +743,10 @@ static int ioreq_runio_qemu_aio(struct ioreq *ioreq) > +if (!blk_split_discard(ioreq, req->sector_number, req->nr_sectors)) { > +goto err; How is error handling supposed to work here? Initially I forgot the "!", which