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

[Kevin Wolf <kwolf@redhat.com>]

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 <den@openvz.org>
> >>>> CC: Kevin Wolf <kwolf@redhat.com>
> >>>> CC: Peter Lieven <pl@kamp.de>
> >>>> ---
> >>>>   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