[PATCH 1/3] nvme-multipath: simplify loop in nvme_update_ana_state()

[Sagi Grimberg]

On 07/10/2024 13:01, Hannes Reinecke wrote:
> Currently nvme_update_ana_state() iterates over all namespaces
> with the srcu lock held, and calls the 'cb' function if a
> match on the list of namespaces from the ANA log is found.
> This assumes that the iteration itself is relatively quick,
> and the namespace state itself doesn't change during iteration.
> For more complex functions (eg if the 'cb' function triggers
> mpath_set_live() which then kicks off a partition scan) the
> namespace state might change while iterating, and the rcu-protected
> area becomes really long.
> So change the loop to iterated over the entries in the ANA log,
> and call nvme_find_get_ns() on each entry.
> With that the 'cb' function is executed outside of the RCU
> protected area, and normal reference counting rules apply.
>
> Signed-off-by: Hannes Reinecke <hare at kernel.org>
> ---
>   drivers/nvme/host/multipath.c | 18 ++++++------------
>   1 file changed, 6 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
> index f72c5a6a2d8e..61f8ae199288 100644
> --- a/drivers/nvme/host/multipath.c
> +++ b/drivers/nvme/host/multipath.c
> @@ -745,7 +745,6 @@ static int nvme_update_ana_state(struct nvme_ctrl *ctrl,
>   	u32 nr_nsids = le32_to_cpu(desc->nnsids), n = 0;
>   	unsigned *nr_change_groups = data;
>   	struct nvme_ns *ns;
> -	int srcu_idx;
>   
>   	dev_dbg(ctrl->device, "ANA group %d: %s.\n",
>   			le32_to_cpu(desc->grpid),
> @@ -757,21 +756,16 @@ static int nvme_update_ana_state(struct nvme_ctrl *ctrl,
>   	if (!nr_nsids)
>   		return 0;
>   
> -	srcu_idx = srcu_read_lock(&ctrl->srcu);
> -	list_for_each_entry_rcu(ns, &ctrl->namespaces, list) {
> +	for(n = 0; n < nr_nsids; n++) {
>   		unsigned nsid;
> -again:
> +
>   		nsid = le32_to_cpu(desc->nsids[n]);
> -		if (ns->head->ns_id < nsid)
> -			continue;
> -		if (ns->head->ns_id == nsid)
> +		ns = nvme_find_get_ns(ctrl, nsid);
> +		if (ns) {
>   			nvme_update_ns_ana_state(desc, ns);
> -		if (++n == nr_nsids)
> -			break;
> -		if (ns->head->ns_id > nsid)
> -			goto again;
> +			nvme_put_ns(ns);
> +		}
>   	}
> -	srcu_read_unlock(&ctrl->srcu, srcu_idx);

You also made the loop now n^2... which *may* be an issue for an 
environment with
a lot of namespaces. It is simpler though...

Reviewed-by: Sagi Grimberg <sagi at grimberg.me>