Re: [PATCH 4/5] libnvdimm: Adding blk-mq support to the pmem driver

[Ross Zwisler <ross.zwisler@linux.intel.com>]

On Mon, Jul 31, 2017 at 03:24:40PM -0700, Dave Jiang wrote:
> Adding blk-mq support to the pmem driver in addition to the direct bio
> support. This allows for hardware offloading via DMA engines. By default
> the bio method will be enabled. The blk-mq support can be turned on via
> module parameter queue_mode=1.
> 
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
>  drivers/nvdimm/pmem.c |  131 +++++++++++++++++++++++++++++++++++++++++++++----
>  drivers/nvdimm/pmem.h |    3 +
>  2 files changed, 122 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/nvdimm/pmem.c b/drivers/nvdimm/pmem.c
> index c544d46..347835b 100644
> --- a/drivers/nvdimm/pmem.c
> +++ b/drivers/nvdimm/pmem.c
> @@ -31,10 +31,24 @@
>  #include <linux/pmem.h>
>  #include <linux/dax.h>
>  #include <linux/nd.h>
> +#include <linux/blk-mq.h>
>  #include "pmem.h"
>  #include "pfn.h"
>  #include "nd.h"
>  
> +enum {
> +	PMEM_Q_BIO = 0,
> +	PMEM_Q_MQ = 1,
> +};
> +
> +static int queue_mode = PMEM_Q_BIO;
> +module_param(queue_mode, int, 0444);
> +MODULE_PARM_DESC(queue_mode, "Pmem Queue Mode");

Maybe in the parameter description we can give the user a hint as to what the
settings are?  i.e. "PMEM queue mode (0=BIO, 1=MQ+DMA)" or something?

> +
> +struct pmem_cmd {
> +	struct request *rq;
> +};
> +
>  static struct device *to_dev(struct pmem_device *pmem)
>  {
>  	/*
> @@ -239,9 +253,12 @@ static const struct dax_operations pmem_dax_ops = {
>  	.direct_access = pmem_dax_direct_access,
>  };
>  
> -static void pmem_release_queue(void *q)
> +static void pmem_release_queue(void *pmem)
>  {
> -	blk_cleanup_queue(q);
> +

Unneeded whitespace.  Also, a nit, can we maybe add a local pmem with type
"struct pmem_device *" so we don't have to cast twice?

> +	blk_cleanup_queue(((struct pmem_device *)pmem)->q);
> +	if (queue_mode == PMEM_Q_MQ)
> +		blk_mq_free_tag_set(&((struct pmem_device *)pmem)->tag_set);
>  }
>  
>  static void pmem_freeze_queue(void *q)
> @@ -259,6 +276,65 @@ static void pmem_release_disk(void *__pmem)
>  	put_disk(pmem->disk);
>  }
>  
> +static int pmem_handle_cmd(struct pmem_cmd *cmd)
> +{
> +	struct request *req = cmd->rq;
> +	struct request_queue *q = req->q;
> +	struct pmem_device *pmem = q->queuedata;
> +	struct nd_region *nd_region = to_region(pmem);
> +	struct bio_vec bvec;
> +	struct req_iterator iter;
> +	int rc = 0;
> +
> +	if (req->cmd_flags & REQ_FLUSH)
> +		nvdimm_flush(nd_region);
> +
> +	rq_for_each_segment(bvec, req, iter) {
> +		rc = pmem_do_bvec(pmem, bvec.bv_page, bvec.bv_len,
> +				bvec.bv_offset, op_is_write(req_op(req)),
> +				iter.iter.bi_sector);
> +		if (rc < 0)
> +			break;
> +	}
> +
> +	if (req->cmd_flags & REQ_FUA)
> +		nvdimm_flush(nd_region);
> +
> +	blk_mq_end_request(cmd->rq, rc);
> +
> +	return 0;

	return rc;
?

> +}
> +
> +static int pmem_queue_rq(struct blk_mq_hw_ctx *hctx,
> +		const struct blk_mq_queue_data *bd)
> +{
> +	struct pmem_cmd *cmd = blk_mq_rq_to_pdu(bd->rq);
> +	int rc;
> +
> +	cmd->rq = bd->rq;
> +
> +	blk_mq_start_request(bd->rq);
> +
> +	rc = pmem_handle_cmd(cmd);
> +	if (rc < 0)
> +		rc = BLK_MQ_RQ_QUEUE_ERROR;
> +	else
> +		rc = BLK_MQ_RQ_QUEUE_OK;
> +
> +	return rc;
> +}

You can get rid of 'rc' altogether and simplify this to:

static int pmem_queue_rq(struct blk_mq_hw_ctx *hctx,
                const struct blk_mq_queue_data *bd)
{
        struct pmem_cmd *cmd = blk_mq_rq_to_pdu(bd->rq);

        cmd->rq = bd->rq;
        blk_mq_start_request(bd->rq);

        if (pmem_handle_cmd(cmd) < 0)
                return BLK_MQ_RQ_QUEUE_ERROR;
        else
                return BLK_MQ_RQ_QUEUE_OK;
}

> +static int pmem_init_hctx(struct blk_mq_hw_ctx *hctx, void *data,
> +		unsigned int index)
> +{
> +	return 0;
> +}

I'm pretty sure you don't need a dummy implementation of .init_hctx.  All
callers check to make sure it's defined before calling, so if you just leave
the OP NULL it'll do the same thing.  scsi_mq_ops for example does this.

> @@ -303,17 +380,47 @@ static int pmem_attach_disk(struct device *dev,
>  		return -EBUSY;
>  	}
>  
> -	q = blk_alloc_queue_node(GFP_KERNEL, dev_to_node(dev));
> -	if (!q)
> -		return -ENOMEM;
> +	if (queue_mode == PMEM_Q_MQ) {
> +		pmem->tag_set.ops = &pmem_mq_ops;
> +		pmem->tag_set.nr_hw_queues = nr_online_nodes;
> +		pmem->tag_set.queue_depth = 64;
> +		pmem->tag_set.numa_node = dev_to_node(dev);
> +		pmem->tag_set.cmd_size = sizeof(struct pmem_cmd);
> +		pmem->tag_set.flags = BLK_MQ_F_SHOULD_MERGE;
> +		pmem->tag_set.driver_data = pmem;
> +
> +		rc = blk_mq_alloc_tag_set(&pmem->tag_set);
> +		if (rc < 0)
> +			return rc;
> +
> +		pmem->q = blk_mq_init_queue(&pmem->tag_set);
> +		if (IS_ERR(pmem->q)) {
> +			blk_mq_free_tag_set(&pmem->tag_set);
> +			return -ENOMEM;
> +		}
>  
> -	if (devm_add_action_or_reset(dev, pmem_release_queue, q))
> -		return -ENOMEM;
> +		if (devm_add_action_or_reset(dev, pmem_release_queue, pmem)) {
> +			blk_mq_free_tag_set(&pmem->tag_set);
> +			return -ENOMEM;
> +		}
> +	} else if (queue_mode == PMEM_Q_BIO) {
> +		pmem->q = blk_alloc_queue_node(GFP_KERNEL, dev_to_node(dev));
> +		if (!pmem->q)
> +			return -ENOMEM;
> +
> +		if (devm_add_action_or_reset(dev, pmem_release_queue, pmem))
> +			return -ENOMEM;
> +
> +		blk_queue_make_request(pmem->q, pmem_make_request);

Later on in this function there is a big block that is still manipulating 'q'
instead of pmem->q:

	blk_queue_write_cache(q, true, true);
	blk_queue_make_request(q, pmem_make_request);
	blk_queue_physical_block_size(q, PAGE_SIZE);
	blk_queue_max_hw_sectors(q, UINT_MAX);
	blk_queue_bounce_limit(q, BLK_BOUNCE_ANY);
	queue_flag_set_unlocked(QUEUE_FLAG_NONROT, q);
	queue_flag_set_unlocked(QUEUE_FLAG_DAX, q);
	q->queuedata = pmem;

I'm pretty sure this all needs to be looking at pmem->q instead, and we may
just end up killing the local 'q'.

Also, this block duplicates the blk_queue_make_request() call, which we
manually make happen in the PMEM_Q_BIO case above.  Not sure if this needs to
be common for both that and the PMEM_Q_MQ case, but we probably don't want it
in both places.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm