On Fri, 2023-12-15 at 21:49 -0500, Benjamin Marzinski wrote: > On Thu, Dec 14, 2023 at 05:15:20PM +0100, Martin Wilck wrote: > > The term "restorequeuing" suggests that it's somehow reset to a > > previously saved value, which is not the case. We should mention in > > the > > man page that "restorequeuing" actually just enables queueing (for > > maps > > that are configured to queue). Perhaps we should also add > > "enablequeueing" as an alias for "restorequeueing". > > I don't like the name "enablequeueing". "disablequeueing" turns > queueing > off regardless of the device's setting. "enablequeueing" sound like > it > could turn queueing on regardless of the device's setting. If you'd > like, > we could add code to restore queueing to stop queueing if we > shouldn't > be, but I'm not sure how that could happen, so it seems unnecessary, > unless there's some way that this could happen that I'm not thinking > about. I'm fine with updating the man page to mention that > "restorequeueing" will just reset no_path_retry to the configured > setting and reenable queueing if necessary. Ok, forget about "enablequeueing", then. Let's just make sure the man page accurately describes what "restorequeuing" does. > > > @@ -962,10 +955,10 @@ cli_restore_all_queueing(void *v, struct > > > strbuf > > > *reply, void *data) > > > pthread_cleanup_push(put_multipath_config, > > > conf); > > > select_no_path_retry(conf, mpp); > > > pthread_cleanup_pop(1); > > > - /* See comment in cli_restore_queueing() */ > > > - if (mpp->no_path_retry != NO_PATH_RETRY_UNDEF && > > > - mpp->no_path_retry != NO_PATH_RETRY_FAIL) > > > - set_no_path_retry(mpp); > > > + > > > + if (mpp->no_path_retry == NO_PATH_RETRY_QUEUE || > > > + (mpp->no_path_retry > 0 && > > > count_active_paths(mpp) > 0)) > > > + dm_queue_if_no_path(mpp->alias, 1); > > > > What about the case when there are no active paths, no pending > > paths, > > mpp->in_recovery and mpp->retry_tick > 0? > > I guess I always sort of assumed the the restore queueing commands > would > get called after the disablequeueing commands. In that case, > retry_tick > can't be greater than 0. But if the plan is to keep mpp->features in > sync, and we go back to calling set_no_path_retry() here, we still > won't > enable queueing it the state you mention. I would argue that there's > not much point in doing so. We don't have paths, and we already were > failing back IO, so anything that was sending IO will already have > seen > failures. There doesn't seem much point in turning it back on now. That makes some sense, yet IMO a user would expect that "restorequeueing" does just that, whether or not it's reasonable to do so. If it does not, we need to document it, I think. I wonder if we can / should somehow enforce that "restorequeueing" can only be called on a map on which "disablequeueing" had been called previously. > Besides how would we even get into this state? I suppose something > outside of multipathd could have turned off queueing. > > I was planning on just going back to set_no_path_retry() if we keep > mpp->features in sync, unless you object. No objections. Regards, Martin
