[PATCH 2/3] nvme: multipath: only update ctrl->nr_active when using queue-depth iopolicy

[Keith Busch]

On Tue, Nov 07, 2023 at 04:23:30PM -0500, Ewan D. Milne wrote:
> The atomic updates of ctrl->nr_active are unnecessary when using
> numa or round-robin iopolicy, so avoid that cost on a per-request basis.
> Clear nr_active when changing iopolicy and do not decrement below zero.
> (This handles changing the iopolicy while requests are in flight.)

Oh, here's restricting it to that policy. Any reason not to fold it in
the first one?

> -	atomic_inc(&ns->ctrl->nr_active);
> +	if (READ_ONCE(ns->head->subsys->iopolicy) == NVME_IOPOLICY_QD)
> +		atomic_inc(&ns->ctrl->nr_active);

This all looks racy with the ability to change policies in flight. I
think we need to introduce a new flag and do something like this:

	if (READ_ONCE(ns->head->subsys->iopolicy) == NVME_IOPOLICY_QD) {
		atomic_inc(&ns->ctrl->nr_active);
	  	nvme_req(rq)->flags |= NVME_MPATH_COUNT_ACTIVE;
	}

...

> @@ -147,7 +148,8 @@ void nvme_mpath_end_request(struct request *rq)
>  	if (!(nvme_req(rq)->flags & NVME_MPATH_IO_STATS))
>  		return;
>  
> -	atomic_dec(&ns->ctrl->nr_active);
> +	if (READ_ONCE(ns->head->subsys->iopolicy) == NVME_IOPOLICY_QD)
> +		atomic_dec_if_positive(&ns->ctrl->nr_active);

And then the above becomes:

	if (nvme_req(rq)->flags & NVME_MPATH_COUNT_ACTIVE)
		atomic_dec_if_positive(&ns->ctrl->nr_active);

...

> +void nvme_subsys_iopolicy_update(struct nvme_subsystem *subsys, int iopolicy)
> +{
> +	struct nvme_ctrl *ctrl;
> +
> +	WRITE_ONCE(subsys->iopolicy, iopolicy);
> +
> +	mutex_lock(&nvme_subsystems_lock);
> +	list_for_each_entry(ctrl, &subsys->ctrls, subsys_entry) {
> +		atomic_set(&ctrl->nr_active, 0);
> +	}
> +	mutex_unlock(&nvme_subsystems_lock);

And then you don't need to do the above since atomic_dec's are always
pairs with atomic_inc's. What do you think?