[dm-devel] [PATCH 4/8] libmultipath: fix queue_mode feature handling

[Martin Wilck]

On Fri, 2022-10-21 at 12:48 -0500, Benjamin Marzinski wrote:
> 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 at redhat.com>
> > 
> > Reviewed-by: Martin Wilck <mwilck at suse.com>
> > 
> > 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,

This was the part I was missing. The kernel separates args using
isspace(), but of course it won't insert any weird characters
when it returns the status line.

Fine, then.

> 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.

Right, figured that in the meantime.

>  
> > > +
> > > +       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".

Ok.

Martin