Re: [PATCH 4/8] libmultipath: fix queue_mode feature handling

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Oct 20, 2022 at 02:58:42PM +0000, Martin Wilck wrote:
> 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, &params, 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?

The kernel will never output any whitespace characters other than space,
so that seems like unnecessary complexity.  We also know that the kernel
will never output a feature named something like "enqueue_mode" so we
don't need to check for the space before "queue_mode". But I suppose the
the kernel could add feature named "<something>queue_mode" in the
future. If you think it's important, I send a patch to add a space
before "queue_mode", in the check to deal with that possibility, but I'm
not particularly worried about this.
 
> >  
> >         /*
> >          * 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.
> 

So is fprintf(), so even as a systemd style daemon, condlog() is a
cancellation point.
 
> > +
> > +       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.

I was matching the characters that isspace() checks for, for
consistency, since we used isspace() to check that there was a space
before "queue_mode".

-Ben
 
> > +               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





[Index of Archives]     [DM Crypt]     [Fedora Desktop]     [ATA RAID]     [Fedora Marketing]     [Fedora Packaging]     [Fedora SELinux]     [Yosemite Discussion]     [KDE Users]     [Fedora Docs]

  Powered by Linux