On Wed, 2024-06-05 at 19:22 -0400, Benjamin Marzinski wrote:
> Make sure that all callers of set_no_path_retry() update the
> multipath->features string from the kernel before calling it, so that
> they have the current queueing state to check.
>
> Signed-off-by: Benjamin Marzinski <bmarzins@xxxxxxxxxx>
I am not sure about this one.
refresh_multipath() is not a cheap operation, requiring at least 3
device-mapper ioctls, and we hold the multipath lock while we call it,
meaning that multipathd can't take care of other important things while
it's handling this CLI request.
OTOH, the kernel only changes the features string when the map is
reloaded. Nobody should be reloading the map under normal conditions
anyway, and if they do, we would see an uevent shortly afterwards. IOW,
I think it's safe to assume that multipathd has the correct features
string cached, except in pathological situations.
Am I overlooking something? Do you have evidence of errors occuring
because multipathd has a wrong features string cached?
Regards
Martin
> ---
> multipathd/cli_handlers.c | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
> diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c
> index 117570e1..3dc5e690 100644
> --- a/multipathd/cli_handlers.c
> +++ b/multipathd/cli_handlers.c
> @@ -934,6 +934,8 @@ cli_restore_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);
> + if (refresh_multipath(vecs, mpp))
> + return -ENODEV;
> set_no_path_retry(mpp);
> } else
> condlog(2, "%s: queueing not disabled. Nothing to
> do", mapname);
> @@ -956,6 +958,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);
> + if (refresh_multipath(vecs, mpp)) {
> + i--;
> + continue;
> + }
> set_no_path_retry(mpp);
> } else
> condlog(2, "%s: queueing not disabled.
> Nothing to do",
> @@ -983,6 +989,8 @@ cli_disable_queueing(void *v, struct strbuf
> *reply, void *data)
> mpp->retry_tick = 0;
> mpp->no_path_retry = NO_PATH_RETRY_FAIL;
> mpp->disable_queueing = 1;
> + if (refresh_multipath(vecs, mpp))
> + return -ENODEV;
> set_no_path_retry(mpp);
> return 0;
> }
> @@ -1001,6 +1009,10 @@ cli_disable_all_queueing(void *v, struct
> strbuf *reply, void *data)
> mpp->retry_tick = 0;
> mpp->no_path_retry = NO_PATH_RETRY_FAIL;
> mpp->disable_queueing = 1;
> + if (refresh_multipath(vecs, mpp)) {
> + i--;
> + continue;
> + }
> set_no_path_retry(mpp);
> }
> return 0;
