linux-kernel - Re: [PATCH v3 1/1] nvme: multipath: Implemented new iopolicy "queue-depth"

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 for Android: free password hash cracker in your pocket

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

Message-ID: <ZkyrSqGQX6kQf7nG@kbusch-mbp.dhcp.thefacebook.com>
Date: Tue, 21 May 2024 08:10:18 -0600
From: Keith Busch <kbusch@...nel.org>
To: John Meneghini <jmeneghi@...hat.com>
Cc: Hannes Reinecke <hare@...e.de>, hch@....de, sagi@...mberg.me,
	emilne@...hat.com, linux-nvme@...ts.infradead.org,
	linux-kernel@...r.kernel.org, jrani@...estorage.com,
	randyj@...estorage.com, hare@...nel.org
Subject: Re: [PATCH v3 1/1] nvme: multipath: Implemented new iopolicy
 "queue-depth"

On Tue, May 21, 2024 at 09:58:31AM -0400, John Meneghini wrote:
> On 5/21/24 02:46, Hannes Reinecke wrote:
> > > +    list_for_each_entry(ctrl, &subsys->ctrls, subsys_entry) {
> > > +        atomic_set(&ctrl->nr_active, 0);
> > > +        nvme_mpath_clear_ctrl_paths(ctrl);
> > 
> > You always reset the variables here, even if specified iopolicy is
> > the same than the currently active one.
> > I'd rather check if the iopolicy is different before changing the settings.
> 
> Yes, Keith pointed this out too.  This is actually a feature not a bug.  In
> situations were we want to "reset" the nr_active counters on all controllers
> the user can simply set the queue-depth iopolicy a second time.  I don't
> expect users to do this very often... they shouldn't be changing IO policies
> back and forth too much... but the ability to "reset" the nr_active counters
> during testing has been very helpful and important to do.  So I'd like to
> keep this.  Moreover, this is NOT the performance path. I don't see the
> point in making performance optimizations in a code path that is run once a
> year.

I missed that you actually want to reset the counters on a live queue.
Wouldn't that just lead to an imbalance? If that is really a feature,
then I retract a previous comment: you do need the atomic_dec_not_zero
(or whatever it was called) since the active count is no longer tied to
the inc's.