On Fri, 2022-10-07 at 12:35 -0500, Benjamin Marzinski wrote:
> device-mapper is not able to change the queue_mode on a table reload.
> Make sure that when multipath sets up the map, both on regular
> reloads
> and reconfigures, it keeps the queue_mode the same.
>
> Signed-off-by: Benjamin Marzinski <bmarzins@xxxxxxxxxx>
Reviewed-by: Martin Wilck <mwilck@xxxxxxxx>
Some remarks below.
> ---
> libmultipath/configure.c | 4 +++
> libmultipath/dmparser.c | 2 ++
> libmultipath/propsel.c | 55
> ++++++++++++++++++++++++++++++++++++++
> libmultipath/structs.h | 7 +++++
> multipath/multipath.conf.5 | 7 +++--
> 5 files changed, 73 insertions(+), 2 deletions(-)
>
> diff --git a/libmultipath/configure.c b/libmultipath/configure.c
> index 8af7cd79..41641e30 100644
> --- a/libmultipath/configure.c
> +++ b/libmultipath/configure.c
> @@ -1075,6 +1075,7 @@ int coalesce_paths (struct vectors *vecs,
> vector mpvec, char *refwwid,
> struct config *conf = NULL;
> int allow_queueing;
> struct bitfield *size_mismatch_seen;
> + struct multipath * cmpp;
>
> /* ignore refwwid if it's empty */
> if (refwwid && !strlen(refwwid))
> @@ -1184,6 +1185,9 @@ int coalesce_paths (struct vectors *vecs,
> vector mpvec, char *refwwid,
> }
> verify_paths(mpp);
>
> + cmpp = find_mp_by_wwid(curmp, mpp->wwid);
> + if (cmpp)
> + mpp->queue_mode = cmpp->queue_mode;
> if (setup_map(mpp, ¶ms, vecs)) {
> remove_map(mpp, vecs->pathvec, NULL);
> continue;
> diff --git a/libmultipath/dmparser.c b/libmultipath/dmparser.c
> index 50d13c08..3b37a926 100644
> --- a/libmultipath/dmparser.c
> +++ b/libmultipath/dmparser.c
> @@ -151,6 +151,8 @@ int disassemble_map(const struct _vector
> *pathvec,
>
> free(word);
> }
> + mpp->queue_mode = strstr(mpp->features, "queue_mode bio") ?
> + QUEUE_MODE_BIO : QUEUE_MODE_RQ;
Nitpick: You have spent effort to make multipath-tools support any
whitepace characters in the previous patches, but here you don't. I can
see that disassemble_map() generally assumes space characters as word
delimiters, but at least I see some inconsistency here.
Do you intend to generalize the whitespace handling in
disassemble_map(), too? Or am I overlooking something?
>
> /*
> * hwhandler
> diff --git a/libmultipath/propsel.c b/libmultipath/propsel.c
> index 98e3aad1..d4f19897 100644
> --- a/libmultipath/propsel.c
> +++ b/libmultipath/propsel.c
> @@ -26,6 +26,7 @@
> #include "strbuf.h"
> #include <inttypes.h>
> #include <libudev.h>
> +#include <ctype.h>
>
> pgpolicyfn *pgpolicies[] = {
> NULL,
> @@ -413,6 +414,59 @@ void reconcile_features_with_options(const char
> *id, char **features, int* no_pa
> }
> }
>
> +static void reconcile_features_with_queue_mode(struct multipath *mp)
> +{
> + char *space = NULL, *val = NULL, *mode_str = NULL, *feat;
> + int features_mode = QUEUE_MODE_UNDEF;
> +
> + if (!mp->features)
> + return;
> +
> + pthread_cleanup_push(cleanup_free_ptr, &space);
> + pthread_cleanup_push(cleanup_free_ptr, &val);
> + pthread_cleanup_push(cleanup_free_ptr, &mode_str);
I was wondering why we need pthread_cleanup() complexity here, seeing
no cancellation points in this function. I eventually realized that
condlog()->dlog()->log_safe()->pthread_mutex_lock() is a cancellation
point. I suppose we need to clean that up some time.
> +
> + if (!(feat = strstr(mp->features, "queue_mode")) ||
> + feat == mp->features || !isspace(*(feat - 1)) ||
> + sscanf(feat, "queue_mode%m[ \f\n\r\t\v]%ms", &space,
> &val) != 2)
Nit: Given that mp->features comes from the multipath.conf, I'm pretty
sure that it can't contain \n or \r as whitespace characters
(read_line() would remove them()). Not sure about \f and \v; guess they
are allowed but I wouldn't swear that they can be used in
multipath.conf without causing trouble elsewhere.
> + goto sync_mode;
> + if (asprintf(&mode_str, "queue_mode%s%s", space, val) < 0) {
> + condlog(1, "failed to allocate space for queue_mode
> feature string");
> + mode_str = NULL; /* value undefined on failure */
> + goto exit;
> + }
> +
> + if (!strcmp(val, "rq") || !strcmp(val, "mq"))
> + features_mode = QUEUE_MODE_RQ;
> + else if (!strcmp(val, "bio"))
> + features_mode = QUEUE_MODE_BIO;
> + if (features_mode == QUEUE_MODE_UNDEF) {
> + condlog(2, "%s: ignoring invalid feature '%s'",
> + mp->alias, mode_str);
> + goto sync_mode;
> + }
> +
> + if (mp->queue_mode == QUEUE_MODE_UNDEF)
> + mp->queue_mode = features_mode;
> + if (mp->queue_mode == features_mode)
> + goto exit;
> +
> + condlog(2,
> + "%s: ignoring feature '%s' because queue_mode is set
> to '%s'",
> + mp->alias, mode_str,
> + (mp->queue_mode == QUEUE_MODE_RQ)? "rq" : "bio");
> +
> +sync_mode:
> + if (mode_str)
> + remove_feature(&mp->features, mode_str);
> + if (mp->queue_mode == QUEUE_MODE_BIO)
> + add_feature(&mp->features, "queue_mode bio");
> +exit:
> + pthread_cleanup_pop(1);
> + pthread_cleanup_pop(1);
> + pthread_cleanup_pop(1);
> +}
> +
> int select_features(struct config *conf, struct multipath *mp)
> {
> const char *origin;
> @@ -428,6 +482,7 @@ out:
> reconcile_features_with_options(mp->alias, &mp->features,
> &mp->no_path_retry,
> &mp->retain_hwhandler);
> + reconcile_features_with_queue_mode(mp);
> condlog(3, "%s: features = \"%s\" %s", mp->alias, mp-
> >features, origin);
> return 0;
> }
> diff --git a/libmultipath/structs.h b/libmultipath/structs.h
> index 5a713d46..129bdf0e 100644
> --- a/libmultipath/structs.h
> +++ b/libmultipath/structs.h
> @@ -170,6 +170,12 @@ enum max_sectors_kb_states {
> MAX_SECTORS_KB_MIN = 4, /* can't be smaller than page size
> */
> };
>
> +enum queue_mode_states {
> + QUEUE_MODE_UNDEF = 0,
> + QUEUE_MODE_BIO,
> + QUEUE_MODE_RQ,
> +};
> +
> enum scsi_protocol {
> SCSI_PROTOCOL_FCP = 0, /* Fibre Channel */
> SCSI_PROTOCOL_SPI = 1, /* parallel SCSI */
> @@ -396,6 +402,7 @@ struct multipath {
> int needs_paths_uevent;
> int ghost_delay;
> int ghost_delay_tick;
> + int queue_mode;
> uid_t uid;
> gid_t gid;
> mode_t mode;
> diff --git a/multipath/multipath.conf.5 b/multipath/multipath.conf.5
> index e098d555..46a4126c 100644
> --- a/multipath/multipath.conf.5
> +++ b/multipath/multipath.conf.5
> @@ -459,8 +459,11 @@ precedence. See KNOWN ISSUES.
> <mode> can be \fIbio\fR, \fIrq\fR or \fImq\fR, which corresponds to
> bio-based, request-based, and block-multiqueue (blk-mq) request-
> based,
> respectively.
> -The default depends on the kernel parameter \fBdm_mod.use_blk_mq\fR.
> It is
> -\fImq\fR if the latter is set, and \fIrq\fR otherwise.
> +Before kernel 4.20 The default depends on the kernel parameter
> +\fBdm_mod.use_blk_mq\fR. It is \fImq\fR if the latter is set, and
> \fIrq\fR
> +otherwise. Since kernel 4.20, \fIrq\fR and \fImq\fR both correspond
> to
> +block-multiqueue. Once a multipath device has been created, its
> queue_mode
> +cannot be changed.
> .TP
> The default is: \fB<unset>\fR
> .RE
--
dm-devel mailing list
dm-devel@xxxxxxxxxx
https://listman.redhat.com/mailman/listinfo/dm-devel
