On Fri, Nov 23, 2012 at 3:15 PM, Peter Maydell <peter.maydell@xxxxxxxxxx> wrote:
> On 23 November 2012 14:11, Stefan Hajnoczi <stefanha@xxxxxxxxx> wrote:
>> On Thu, Nov 22, 2012 at 10:07 AM, Stefan Priebe <s.priebe@xxxxxxxxxxxx> wrote:
>>> diff --git a/block/rbd.c b/block/rbd.c
>>> index 5a0f79f..0384c6c 100644
>>> --- a/block/rbd.c
>>> +++ b/block/rbd.c
>>> @@ -69,7 +69,7 @@ typedef enum {
>>> typedef struct RBDAIOCB {
>>> BlockDriverAIOCB common;
>>> QEMUBH *bh;
>>> - int ret;
>>> + ssize_t ret;
>>> QEMUIOVector *qiov;
>>> char *bounce;
>>> RBDAIOCmd cmd;
>>> @@ -86,7 +86,7 @@ typedef struct RADOSCB {
>>> int done;
>>> int64_t size;
>>> char *buf;
>>> - int ret;
>>> + ssize_t ret;
>>> } RADOSCB;
>>>
>>> #define RBD_FD_READ 0
>>
>> I preferred your previous patch:
>>
>> ssize_t on 32-bit hosts has sizeof(ssize_t) == 4. In
>> qemu_rbd_complete_aio() we may assign acb->ret = rcb->size. Here the
>> size field is int64_t, so ssize_t ret would truncate the value.
>
> The rcb size field should be a size_t: it is used for calling
> rbd_aio_write and rbd_aio_read so if we've overflowed 32 bits
> then we've already got a problem there.
You are right that size_t is fine for read/write.
But size_t is not fine for discard:
1. librbd takes uint64_t for discard.
2. Completing a discard request uses size.
3. BlockDriverCompletionFunc only passes int ret value <--- broken for
large discard
Stefan Priebe has been discarding large regions (e.g. >4 GB must be
possible on a 32-bit host).
The previous int64_t patch didn't clean up types completely but it
made things better. AFAICT we need to be able to discard >4 GB so
ssize_t/size_t doesn't cut it on 32-bit hosts.
Stefan
--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
