Re: [Qemu-devel] [PATCH 1/2] glusterfs: fix max_discard

[Kevin Wolf]

Am 03.02.2015 um 12:30 hat Peter Lieven geschrieben:
> Am 03.02.2015 um 08:31 schrieb Denis V. Lunev:
> > On 02/02/15 23:46, Denis V. Lunev wrote:
> >> On 02/02/15 23:40, Peter Lieven wrote:
> >>> Am 02.02.2015 um 21:09 schrieb Denis V. Lunev:
> >>>> qemu_gluster_co_discard calculates size to discard as follows
> >>>>      size_t size = nb_sectors * BDRV_SECTOR_SIZE;
> >>>>      ret = glfs_discard_async(s->fd, offset, size, 
> >>>> &gluster_finish_aiocb, acb);
> >>>>
> >>>> glfs_discard_async is declared as follows:
> >>>>    int glfs_discard_async (glfs_fd_t *fd, off_t length, size_t lent,
> >>>>                            glfs_io_cbk fn, void *data) __THROW
> >>>> This is problematic on i686 as sizeof(size_t) == 4.
> >>>>
> >>>> Set bl_max_discard to SIZE_MAX >> BDRV_SECTOR_BITS to avoid overflow
> >>>> on i386.
> >>>>
> >>>> Signed-off-by: Denis V. Lunev <address@hidden>
> >>>> CC: Kevin Wolf <address@hidden>
> >>>> CC: Peter Lieven <address@hidden>
> >>>> ---
> >>>>   block/gluster.c | 9 +++++++++
> >>>>   1 file changed, 9 insertions(+)
> >>>>
> >>>> diff --git a/block/gluster.c b/block/gluster.c
> >>>> index 1eb3a8c..8a8c153 100644
> >>>> --- a/block/gluster.c
> >>>> +++ b/block/gluster.c
> >>>> @@ -622,6 +622,11 @@ out:
> >>>>       return ret;
> >>>>   }
> >>>>   +static void qemu_gluster_refresh_limits(BlockDriverState *bs, Error 
> >>>> **errp)
> >>>> +{
> >>>> +    bs->bl.max_discard = MIN(SIZE_MAX >> BDRV_SECTOR_BITS, INT_MAX);
> >>>> +}
> >>>> +
> >>> Looking at the gluster code bl.max_transfer_length should have the same 
> >>> limit, but thats a different patch.
> >> ha, the same applies to nbd code too.
> >>
> >> I'll do this stuff tomorrow and also I think that some
> >> audit in other drivers could reveal something interesting.
> >>
> >> Den
> > ok. The situation is well rotten here on i686.
> >
> > The problem comes from the fact that QEMUIOVector
> > and iovec uses size_t as length. All API calls use
> > this abstraction. Thus all conversion operations
> > from nr_sectors to size could bang at any moment.
> >
> > Putting dirty hands here is problematic from my point
> > of view. Should we really care about this? 32bit
> > applications are becoming old good history of IT...
> 
> The host has to be 32bit to be in trouble. And at least if we have KVM the 
> host
> has to support long mode.
> 
> I have on my todo to add generic code for honouring bl.max_transfer_length
> in block.c. We could change default maximum from INT_MAX to SIZE_MAX >> 
> BDRV_SECTOR_BITS
> for bl.max_transfer_length.

So the conclusion is that we'll apply this series as it is and you'll
take care of the rest later?

Kevin