Re: [PATCH v7 1/1] nvme-multipath: implement "queue-depth" iopolicy

[Christoph Hellwig <hch@lst.de>]

> [jmeneghi: vairious changes and improvements, addressed review comments]

s/vairious/various/ 

> +	if ((nvme_req(rq)->flags & NVME_MPATH_CNT_ACTIVE))

No need for the double braces here.

> +		WARN_ON_ONCE((atomic_dec_if_positive(&ns->ctrl->nr_active)) < 0);

Overly long line.

But I don't understand why you need the WARN_ON anyway.  If the value
must always be positive there is no point in atomic_dec_if_positive.
If misaccounting is fine there WARN_ON is counterproductive.

> -static struct nvme_ns *nvme_round_robin_path(struct nvme_ns_head *head,
> -		int node, struct nvme_ns *old)
> +static struct nvme_ns *nvme_round_robin_path(struct nvme_ns_head *head)
>  {
> -	struct nvme_ns *ns, *found = NULL;
> +	struct nvme_ns *ns, *old, *found = NULL;
> +	int node = numa_node_id();
> +
> +	old = srcu_dereference(head->current_path[node], &head->srcu);
> +	if (unlikely(!old))
> +		return __nvme_find_path(head, node);

Can you split the refactoring of the existing path selectors into a
prep patch, please?

> +static void nvme_subsys_iopolicy_update(struct nvme_subsystem *subsys,
> +		int iopolicy)
> +{
> +	struct nvme_ctrl *ctrl;
> +	int old_iopolicy = READ_ONCE(subsys->iopolicy);
> +
> +	if (old_iopolicy == iopolicy)
> +		return;
> +
> +	WRITE_ONCE(subsys->iopolicy, iopolicy);

What is the atomicy model here?  There doesn't seem to be any
global lock protecting it?  Maybe move it into the
nvme_subsystems_lock critical section?

> +	pr_notice("%s: changed from %s to %s for subsysnqn %s\n", __func__,
> +			nvme_iopolicy_names[old_iopolicy], nvme_iopolicy_names[iopolicy],
> +			subsys->subnqn);

The function is not really relevant here,  this should become something
like:

	pr_notice("%s: changing iopolicy from %s to %s\n",
		subsys->subnqn,
		nvme_iopolicy_names[old_iopolicy],
		nvme_iopolicy_names[iopolicy]);

or maybe:

	dev_notice(changing iopolicy from %s to %s\n",
		&subsys->dev,
		nvme_iopolicy_names[old_iopolicy],
		nvme_iopolicy_names[iopolicy]);