On Tue, 2023-12-05 at 18:57 -0500, Benjamin Marzinski wrote:
> Currently, the interactive commands to disable and restore queueing
> call
> set_no_path_retry() without first re-reading the device table. This
> means that the value of mpp->features might not be current, which
> means
> that set_no_path_retry() might not tell device-mapper to change the
> queue_if_no_path state when it should have.
>
> Instead of making these commands do extra work to re-read the device
> table, simply make them always tell dm to update the queue_if_no_path
> state, regardless of the current state. There is no harm in telling
> dm
> to set the state to its current value.
>
> Signed-off-by: Benjamin Marzinski <bmarzins@xxxxxxxxxx>
I am still not convinced, as this goes against the spirit of keeping
the no_path_retry setting consistently in a single place. Why don't we
rather make sure that our "features" string correctly matches the
kernel settings (ignoring possible attempts to bypass multipathd)?
The code snippet I sent in reply to your 1/5 patch should achieve this
for map reloading. Queueing can only changed by (re)loading, or by
sending dm messages. Thus AFAICS it should be sufficient to add calls
to add_feature("queue_if_no_path") and
remove_feature("queue_if_no_path") to dm_queue_if_no_path(). Am I
overlooking something?
Wrt "there is no harm in telling dm to set the state to its current
value", the kernel doesn't seem to check whether the new and current
value are the same. It always takes a spinlock and set some flags in
the dm device. So the ioctl is not a zero-cost operation.
Apart from these general doubts, I have some more remarks below.
Before 6071176 ("libmultipath: drop mpp->nr_active field"), which
introduced the call to set_no_path_retry() from cli_restore_queueing(),
we also reset mpp->retry_tick, which your new code does not. (That was
actually part of the motivation to use set_no_path_retry() rather than
direct calls to dm_queue_if_no_path(), IIRC).
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".
> ---
> multipathd/cli_handlers.c | 25 +++++++++----------------
> 1 file changed, 9 insertions(+), 16 deletions(-)
>
> diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c
> index b1dff202..758b571b 100644
> --- a/multipathd/cli_handlers.c
> +++ b/multipathd/cli_handlers.c
> @@ -934,16 +934,9 @@ cli_restore_queueing(void *v, struct strbuf
> *reply, void *data)
> select_no_path_retry(conf, mpp);
> pthread_cleanup_pop(1);
>
> - /*
> - * Don't call set_no_path_retry() for the NO_PATH_RETRY_FAIL
> case.
> - * That would disable queueing when "restorequeueing" is
> called,
> - * and the code never behaved that way. Users might not
> expect it.
> - * In almost all cases, queueing will be disabled anyway
> when we
> - * are here.
> - */
> - 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);
>
> return 0;
> }
> @@ -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?
Regards,
Martin
