Re: [PATCH v7 3/9] nvme-multipath: pass I/O type to nvme_find_path()

[John Garry <john.g.garry@oracle.com>]

> @@ -741,6 +752,7 @@ int nvme_ns_head_ioctl(struct block_device *bdev, blk_mode_t mode,
>    long nvme_ns_head_chr_ioctl(struct file *file, unsigned int cmd,
>    		unsigned long arg)
>    {
> +	u8 opcode;

why declared at the top?

>    	bool open_for_write = file->f_mode & FMODE_WRITE;
>    	struct cdev *cdev = file_inode(file)->i_cdev;
>    	struct nvme_ns_head *head =
> @@ -748,9 +760,19 @@ long nvme_ns_head_chr_ioctl(struct file *file, unsigned int cmd,
>    	void __user *argp = (void __user *)arg;
>    	struct nvme_ns *ns;
>    	int srcu_idx, ret = -EWOULDBLOCK;
> +	unsigned int op_type = NVME_STAT_OTHER;
> +
> +	if (cmd == NVME_IOCTL_SUBMIT_IO) {
> +		if (get_user(opcode, (u8 *)argp))
> +			return -EFAULT;
> +		if (opcode == nvme_cmd_write)
> +			op_type = NVME_STAT_WRITE;
> +		else if (opcode == nvme_cmd_read)
> +			op_type = NVME_STAT_READ;
> +	}
>    
>    	srcu_idx = srcu_read_lock(&head->srcu);
> -	ns = nvme_find_path(head);
> +	ns = nvme_find_path(head, op_type);
>    	if (!ns)
>    		goto out_unlock;
>    
> @@ -770,9 +792,19 @@ int nvme_ns_head_chr_uring_cmd(struct io_uring_cmd *ioucmd,
>    	struct cdev *cdev = file_inode(ioucmd->file)->i_cdev;
>    	struct nvme_ns_head *head = container_of(cdev, struct nvme_ns_head, cdev);
>    	int srcu_idx = srcu_read_lock(&head->srcu);
> -	struct nvme_ns *ns = nvme_find_path(head);
> +	struct nvme_ns *ns;
>    	int ret = -EINVAL;
> +	const struct nvme_uring_cmd *cmd = io_uring_sqe128_cmd(ioucmd->sqe,
> +						struct nvme_uring_cmd);
> +	unsigned int op_type = NVME_STAT_OTHER;
> +	__u8 opcode = READ_ONCE(cmd->opcode);
> +
> +	if (opcode == nvme_cmd_write)
> +		op_type = NVME_STAT_WRITE;
> +	else if (opcode == nvme_cmd_read)
> +		op_type = NVME_STAT_READ;

nit: I think that having a final else leg to set op_type is nicer than 
setting to NVME_STAT_OTHER at init time (and overwriting in some cases).

>    
> +	ns = nvme_find_path(head, op_type);
>    	if (ns)
>    		ret = nvme_ns_uring_cmd(ns, ioucmd, issue_flags);
>    	srcu_read_unlock(&head->srcu, srcu_idx);
> diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
> index 9b9a657fa330..8c20ff516e61 100644
> --- a/drivers/nvme/host/multipath.c
> +++ b/drivers/nvme/host/multipath.c
> @@ -460,7 +460,8 @@ static struct nvme_ns *nvme_numa_path(struct nvme_ns_head *head)
>    	return ns;
>    }
>    
> -inline struct nvme_ns *nvme_find_path(struct nvme_ns_head *head)
> +inline struct nvme_ns *nvme_find_path(struct nvme_ns_head *head,
> +		unsigned int op_type)
>    {
>    	switch (READ_ONCE(head->subsys->iopolicy)) {
>    	case NVME_IOPOLICY_QD:
> @@ -522,7 +523,7 @@ static void nvme_ns_head_submit_bio(struct bio *bio)
>    		return;
>    
>    	srcu_idx = srcu_read_lock(&head->srcu);
> -	ns = nvme_find_path(head);
> +	ns = nvme_find_path(head, __nvme_data_dir(bio_op(bio)));

It's a but unfortunate that we have to find op_type even for when not 
using the latency iopolicy.

>    	if (likely(ns)) {
>    		bio_set_dev(bio, ns->disk->part0);
>    		/*
> @@ -572,7 +573,7 @@ static int nvme_ns_head_get_unique_id(struct gendisk *disk, u8 id[16],
>    	int srcu_idx, ret = -EWOULDBLOCK;
>    
>    	srcu_idx = srcu_read_lock(&head->srcu);
> -	ns = nvme_find_path(head);
> +	ns = nvme_find_path(head, NVME_STAT_OTHER);
>    	if (ns)
>    		ret = nvme_ns_get_unique_id(ns, id, type);
>    	srcu_read_unlock(&head->srcu, srcu_idx);
> @@ -588,7 +589,7 @@ static int nvme_ns_head_report_zones(struct gendisk *disk, sector_t sector,
>    	int srcu_idx, ret = -EWOULDBLOCK;
>    
>    	srcu_idx = srcu_read_lock(&head->srcu);
> -	ns = nvme_find_path(head);
> +	ns = nvme_find_path(head, NVME_STAT_OTHER);
>    	if (ns)
>    		ret = nvme_ns_report_zones(ns, sector, nr_zones, args);
>    	srcu_read_unlock(&head->srcu, srcu_idx);
> diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
> index 824651cc898d..8a9ec502912d 100644
> --- a/drivers/nvme/host/nvme.h
> +++ b/drivers/nvme/host/nvme.h
> @@ -520,6 +520,13 @@ struct nvme_ns_ids {
>    	u8	csi;
>    };
>    
> +enum nvme_stat_group {
> +	NVME_STAT_READ,
> +	NVME_STAT_WRITE,
> +	NVME_STAT_OTHER,

Would NVME_STAT_OTHER ever be used in high frequency scenarios such that 
it is worth having its own type? If not, could NVME_STAT_READ be reused?

> +	NVME_NUM_STAT_GROUPS

Can these ever be used for non-mulitpath? I just wonder why multipath or 
similar is not in the name

> +};
> +
>    /*
>     * Anchor structure for namespaces.  There is one for each namespace in a
>     * NVMe subsystem that any of our controllers can see, and the namespace
> @@ -1032,7 +1039,39 @@ extern const struct attribute_group *nvme_dev_attr_groups[];
>    extern const struct block_device_operations nvme_bdev_ops;
>    
>    void nvme_delete_ctrl_sync(struct nvme_ctrl *ctrl);
> -struct nvme_ns *nvme_find_path(struct nvme_ns_head *head);
> +struct nvme_ns *nvme_find_path(struct nvme_ns_head *head, unsigned int op_type);
> +
> +static inline int __nvme_data_dir(const enum req_op op)
> +{

This returns an int (so not strongly typed), which is going to be 
NVME_STAT_READ, NVME_STAT_WRITE, or NVME_STAT_OTHER. From the function 
name, I am not sure if that it expected. Some might expect READ or WRITE 
returned. 'stat' should be in the name, or similar.

> +	if (op == REQ_OP_READ)
> +		return NVME_STAT_READ;
> +	else if (op == REQ_OP_WRITE)
> +		return NVME_STAT_WRITE;
> +	else
 > +		return NVME_STAT_OTHER;> +}
> +
> +static inline int __nvme_data_dir_passthru(enum nvme_opcode op)
> +{

As __nvme_data_dir

> +	if (op == nvme_cmd_read)
> +		return NVME_STAT_READ;
> +	else if (op == nvme_cmd_write)
> +		return NVME_STAT_WRITE;
> +	else
> +		return NVME_STAT_OTHER;
> +}
> +
> +static inline int nvme_data_dir(struct request *req)

As __nvme_data_dir

> +{
> +	if (blk_rq_is_passthrough(req)) {
> +		struct nvme_request *nr = nvme_req(req);
> +
> +		return __nvme_data_dir_passthru(nr->cmd->common.opcode);
> +	}
> +
> +	return __nvme_data_dir(req_op(req));
> +}
> +
>    #ifdef CONFIG_NVME_MULTIPATH