lkml.org 
[lkml]   [2014]   [Jun]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v4] NVMe: basic conversion to blk-mq
On 06/02/2014 12:08 PM, Christoph Hellwig wrote:
>> +static int nvme_map_rq(struct nvme_queue *nvmeq, struct nvme_iod *iod,
>> + struct request *req, enum dma_data_direction dma_dir,
>> + int psegs)
>> {
>> sg_init_table(iod->sg, psegs);
>> + iod->nents = blk_rq_map_sg(req->q, req, iod->sg);
>>
>> + if (!dma_map_sg(nvmeq->q_dmadev, iod->sg, iod->nents, dma_dir))
>> return -ENOMEM;
>>
>> + return iod->nents;
>
> Given how simple I'd suggest merging this into the only caller.

Ok

>
>> +static int nvme_submit_iod(struct nvme_queue *nvmeq, struct nvme_iod *iod,
>> + struct nvme_ns *ns)
>> {
>> + struct request *req = iod->private;
>> struct nvme_command *cmnd;
>> + u16 control = 0;
>> + u32 dsmgmt = 0;
>>
>> + spin_lock_irq(&nvmeq->q_lock);
>> + if (nvmeq->q_suspended) {
>> + spin_unlock_irq(&nvmeq->q_lock);
>> + return -EBUSY;
>> + }
>>
>> + if (req->cmd_flags & REQ_DISCARD) {
>> + nvme_submit_discard(nvmeq, ns, req, iod);
>> + goto end_submit;
>> + }
>> + if (req->cmd_flags & REQ_FLUSH) {
>> + nvme_submit_flush(nvmeq, ns, req->tag);
>> + goto end_submit;
>> + }
>
> It would be nicer to have the locking and the the suspend check
> in the caller, and then branch out to one function for each type
> of request, especially as the caller already has special cases for
> discard and zero-payload requests anyway.
>

Ok, good idea.

>> +static int nvme_queue_request(struct blk_mq_hw_ctx *hctx, struct request *req)
>> +{
>
> Can you call this nvme_queue_rq to match the method name? Makes
> grepping so much easier.. (ditto for the admin queue).
>

Yes

>> + struct nvme_ns *ns = hctx->queue->queuedata;
>> + struct nvme_queue *nvmeq = hctx->driver_data;
>>
>> + return nvme_submit_req_queue(nvmeq, ns, req);
>
> What's the point of the serparate nvme_submit_req_queue function?
>

Removed

>> spin_lock(&nvmeq->q_lock);
>> - nvme_process_cq(nvmeq);
>> - result = nvmeq->cqe_seen ? IRQ_HANDLED : IRQ_NONE;
>> - nvmeq->cqe_seen = 0;
>> + result = nvme_process_cq(nvmeq) ? IRQ_HANDLED : IRQ_NONE;
>
> No other caller checks the nvme_process_cq return value, so it might
> as well return the IRQ_ values directly.

Ok (it's been changed as cqe_seen had been mistakenly removed.)
>
>> +static struct blk_mq_ops nvme_mq_admin_ops = {
>> + .queue_rq = nvme_queue_admin_request,
>> + .map_queue = blk_mq_map_queue,
>> + .init_hctx = nvme_init_admin_hctx,
>> + .init_request = nvme_init_admin_request,
>> + .timeout = nvme_timeout,
>
> Care to name these nvme_admin_<methodname> for easier grep-ability?

Yes

>
>> +static int nvme_alloc_admin_tags(struct nvme_dev *dev)
>> +{
>> + if (!dev->admin_rq) {
>
> Why do you need the NULL check here?

the nvme_alloc_admin_tags is called both in nvme_dev_start and
nvme_dev_resume. To make sure we don't double allocated it check if its
already been allocated.

>
>> + dev->admin_tagset.reserved_tags = 1;
>
> What is the reserved tag for?

It was for flush. However, this way to do it has been removed in the
later series.

>
>> + dev->admin_rq = blk_mq_init_queue(&dev->admin_tagset);
>> + if (!dev->admin_rq) {
>> + memset(&dev->admin_tagset, 0,
>> + sizeof(dev->admin_tagset));
>> + blk_mq_free_tag_set(&dev->admin_tagset);
>
> Why do you zero the tagset here before freeing it?
>

Removed.

Thanks!



\
 
 \ /
  Last update: 2014-06-02 14:41    [W:2.025 / U:0.004 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site