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

[Keith Busch]

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.