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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240620065641.GA22113@lst.de>
Date: Thu, 20 Jun 2024 08:56:41 +0200
From: Christoph Hellwig <hch@....de>
To: John Meneghini <jmeneghi@...hat.com>
Cc: kbusch@...nel.org, hch@....de, sagi@...mberg.me,
	linux-nvme@...ts.infradead.org, linux-kernel@...r.kernel.org,
	emilne@...hat.com, jrani@...estorage.com, randyj@...estorage.com,
	chaitanyak@...dia.com, hare@...nel.org
Subject: Re: [PATCH v7 1/1] nvme-multipath: implement "queue-depth" iopolicy

> [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]);