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!
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
