Am 10.01.2015 um 19:58 schrieb Christoph Hellwig:
>> +struct ubiblock_pdu {
>> + struct request *req;
>
> No need to store the request, you can trivially get at it using
> blk_mq_rq_from_pdu().
Very handy, I was not aware of blk_mq_rq_from_pdu().
>> + struct ubiblock *dev;
>
> Why do you need the dev pointer? You can always trivially get
> it using req->queuedata.
Same here.
>> +static void ubiblock_do_work(struct work_struct *work)
>> +{
>> + int ret;
>> + struct ubiblock_pdu *pdu = container_of(work, struct ubiblock_pdu,
>> work);
>> +
>> + ret = ubiblock_read(pdu);
>> + blk_mq_end_request(pdu->req, ret ?: 0);
>
> Why not just pass ret as-is?
Obviously a brain fart. :-\
>> + if (blk_rq_pos(req) + blk_rq_cur_sectors(req) >
>> + get_capacity(req->rq_disk))
>> + return BLK_MQ_RQ_QUEUE_ERROR;
>
> The upper layers take are of this check.
Ok.
>> + pdu->usgl.list_pos = 0;
>> + pdu->usgl.page_pos = 0;
>
> Having a helper to initialize a ubi_sgl would be nicer than having
> to open code it ike here.
Ok.
>> +
>> + blk_mq_start_request(req);
>> + ret = blk_rq_map_sg(hctx->queue, req, pdu->usgl.sg);
>> +
>> + queue_work(dev->wq, &pdu->work);
>
> Why don't you move these calls into the work queue as well? The
> queue_rq call would literally just become a queue_work call.
I did not know that I'm allowed to get hctx->queue also via req->q.
> And given that this is a fairly common patter this should allow
> us to refactor / optimize this case a bit later on.
Will send a v2 in a jiffy!
Thanks,
//richard
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
