Re: [PATCH REPOST] rbd: be picky about osd request status type

[Alex Elder <elder@inktank.com>]

On 01/04/2013 11:07 PM, Dan Mick wrote:
> I personally dislike "spaces after cast", but I haven't checked the
> kernel style guide.  Otherwise:
> 
> Reviewed-by: Dan Mick <dan.mick@inktank.com>

I think I'm probably in violation of the kernel style
guide too.

In fact, I just checked, and the indent(1) options for the
kernel specify that there should be no space after a cast.

I'll fix that, and will try hard to adjust my habits...

					-Alex
> 
> On 01/03/2013 02:40 PM, Alex Elder wrote:
>> The result field in a ceph osd reply header is a signed 32-bit type,
>> but rbd code often casually uses int to represent it.
>>
>> The following changes the types of variables that handle this result
>> value to be "s32" instead of "int" to be completely explicit about
>> it.  Only at the point we pass that result to __blk_end_request()
>> does the type get converted to the plain old int defined for that
>> interface.
>>
>> There is almost certainly no binary impact of this change, but I
>> prefer to show the exact size and signedness of the value since we
>> know it.
>>
>> Signed-off-by: Alex Elder <elder@inktank.com>
>> ---
>>   drivers/block/rbd.c |   23 ++++++++++++-----------
>>   1 file changed, 12 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
>> index 85131de..8b79a5b 100644
>> --- a/drivers/block/rbd.c
>> +++ b/drivers/block/rbd.c
>> @@ -171,7 +171,7 @@ struct rbd_client {
>>    */
>>   struct rbd_req_status {
>>       int done;
>> -    int rc;
>> +    s32 rc;
>>       u64 bytes;
>>   };
>>
>> @@ -1053,13 +1053,13 @@ static void rbd_destroy_ops(struct
>> ceph_osd_req_op *ops)
>>   static void rbd_coll_end_req_index(struct request *rq,
>>                      struct rbd_req_coll *coll,
>>                      int index,
>> -                   int ret, u64 len)
>> +                   s32 ret, u64 len)
>>   {
>>       struct request_queue *q;
>>       int min, max, i;
>>
>>       dout("rbd_coll_end_req_index %p index %d ret %d len %llu\n",
>> -         coll, index, ret, (unsigned long long) len);
>> +         coll, index, (int) ret, (unsigned long long) len);
>>
>>       if (!rq)
>>           return;
>> @@ -1080,7 +1080,7 @@ static void rbd_coll_end_req_index(struct
>> request *rq,
>>           max++;
>>
>>       for (i = min; i<max; i++) {
>> -        __blk_end_request(rq, coll->status[i].rc,
>> +        __blk_end_request(rq, (int) coll->status[i].rc,
>>                     coll->status[i].bytes);
>>           coll->num_done++;
>>           kref_put(&coll->kref, rbd_coll_release);
>> @@ -1089,7 +1089,7 @@ static void rbd_coll_end_req_index(struct
>> request *rq,
>>   }
>>
>>   static void rbd_coll_end_req(struct rbd_request *rbd_req,
>> -                 int ret, u64 len)
>> +                 s32 ret, u64 len)
>>   {
>>       rbd_coll_end_req_index(rbd_req->rq,
>>                   rbd_req->coll, rbd_req->coll_index,
>> @@ -1129,7 +1129,7 @@ static int rbd_do_request(struct request *rq,
>>       if (!rbd_req) {
>>           if (coll)
>>               rbd_coll_end_req_index(rq, coll, coll_index,
>> -                           -ENOMEM, len);
>> +                           (s32) -ENOMEM, len);
>>           return -ENOMEM;
>>       }
>>
>> @@ -1206,7 +1206,7 @@ done_err:
>>       bio_chain_put(rbd_req->bio);
>>       ceph_osdc_put_request(osd_req);
>>   done_pages:
>> -    rbd_coll_end_req(rbd_req, ret, len);
>> +    rbd_coll_end_req(rbd_req, (s32) ret, len);
>>       kfree(rbd_req);
>>       return ret;
>>   }
>> @@ -1219,7 +1219,7 @@ static void rbd_req_cb(struct ceph_osd_request
>> *osd_req, struct ceph_msg *msg)
>>       struct rbd_request *rbd_req = osd_req->r_priv;
>>       struct ceph_osd_reply_head *replyhead;
>>       struct ceph_osd_op *op;
>> -    __s32 rc;
>> +    s32 rc;
>>       u64 bytes;
>>       int read_op;
>>
>> @@ -1227,14 +1227,14 @@ static void rbd_req_cb(struct ceph_osd_request
>> *osd_req, struct ceph_msg *msg)
>>       replyhead = msg->front.iov_base;
>>       WARN_ON(le32_to_cpu(replyhead->num_ops) == 0);
>>       op = (void *)(replyhead + 1);
>> -    rc = le32_to_cpu(replyhead->result);
>> +    rc = (s32) le32_to_cpu(replyhead->result);
>>       bytes = le64_to_cpu(op->extent.length);
>>       read_op = (le16_to_cpu(op->op) == CEPH_OSD_OP_READ);
>>
>>       dout("rbd_req_cb bytes=%llu readop=%d rc=%d\n",
>>           (unsigned long long) bytes, read_op, (int) rc);
>>
>> -    if (rc == -ENOENT && read_op) {
>> +    if (rc == (s32) -ENOENT && read_op) {
>>           zero_bio_chain(rbd_req->bio, 0);
>>           rc = 0;
>>       } else if (rc == 0 && read_op && bytes < rbd_req->len) {
>> @@ -1679,7 +1679,8 @@ static void rbd_rq_fn(struct request_queue *q)
>>                           bio_chain, coll, cur_seg);
>>               else
>>                   rbd_coll_end_req_index(rq, coll, cur_seg,
>> -                               -ENOMEM, chain_size);
>> +                               (s32) -ENOMEM,
>> +                               chain_size);
>>               size -= chain_size;
>>               ofs += chain_size;
>>