Re: [dm-devel] [PATCH 18/21] libmultipath: keep bindings in memory

[Benjamin Marzinski <bmarzins@redhat.com>]

On Thu, Sep 07, 2023 at 12:30:53PM +0200, Martin Wilck wrote:
> On Wed, 2023-09-06 at 17:47 -0500, Benjamin Marzinski wrote:
> > On Fri, Sep 01, 2023 at 08:02:31PM +0200, mwilck@suse.com wrote:
> > > From: Martin Wilck <mwilck@suse.com>
> > > 

> > > +       vector_foreach_slot(bindings, bdg, i) {
> > > +               int curr_id = scan_devname(bdg->alias, prefix);
> > 
> > If we know that the bindings will be sorted by alias order, we don't
> > need to do all this. Something like this should work:
> 
> That's true. Unfortunately, we can't ensure ordering "by alias order".
> The reason is that our configuration allows using different alias
> prefixes for different devices. The current sorting is simply
> alphabetical. It detects duplicates, but it ensures "alias order" only
> between "mpatha" and "mpathz".
> 
> I've thought of just removing the "different aliases for different
> devices" feature. I don't know if any users out there actually set
> device-specific alias_prefix values in the devices section of
> multipath.conf. I don't recall having seen such a configuration so far;
> almost every config I have seen simply uses "mpath" everywhere. But I
> recognize that it may feel tempting in some cases. One could use the
> "NETAPP" prefix for some arrays and the "EMC" prefix for others, for
> example, making it easier to see which is what. And we simply don't
> know if we'd break existing user setups with such a change. So if at
> all, we can't do it in a minor release without deprecating it first.
> 
> When we add a binding in add_binding(), we don't know which
> alias_prefix is configured for a given WWID, and we have no easy way to
> figure it out. We know the alias, but we don't know which portion of it
> is the prefix and which is the ID (it's not forbidden to use "aaaa" as
> alias_prefix). I wouldn't want to start guessing.
> 
> If you can think of a way to keep the bindings cleanly sorted, please
> let me know.

Doh! You even mentioned this issue in your "fix alias tests" commit. I
just didn't pay enough attention to it. But I believe there is a way to
make sure that we are dealing with a correctly sorted list of potential
bindings when calling get_free_id(), if we decide it's worth the extra
work.

The global_bindings isn't guaranteed to have all the bindings for our
prefix correctly sorted, but they will all be in a group.  If we wanted,
we could create a new vector that just included these bindings, and sort
it using the current prefix and a comparison function to do alias-sytle
sorting. the msort_r() function takes an extra argument to pass to the
compar() function, which could be the prefix.

It would be great if we could just sort part of the global_bindings
vector in place, once we knew the prefix.  Unfortunately, that would
only work if we could know that no prefix can ever match a starting
substring of another prefix. Otherwise, sorting using one can make the
other not be in a single continuous group.  For instance, if some device
configs used "mpatha" as a prefix, they would expect "mpathaaa" to
follow immediately after "mapthaz", but if the bindings file had already
been sorted using the "mpath" prefix, then "mpathba" would follow
"mpathaz", and "mpathaaa" would come much later. Keeping the global file
alphabetically sorted guarantees that no matter the prefix that devices
were added under, all device aliases that are valid with that prefix
will be in a group together.
 
So, is it worth it to make another vector, copy the alises which are
valid with the current prefix into it, and then sort it? get_free_id()
will be cleaner, and gap detection won't break down after you get past
mpathaa, which it currently does with an alpahbetically sorted bindings
list.

Alternatively, we could just decide that setting a prefix in a device
config that matches the starting substring of a another prefix is a
config error (obviously using the exact same prefix is fine), and ignore
that prefix from the device config when we parse the configuration. Then
we could read in the bindings alphabetically like we currently do, find
the prefix groups in them, and sort them once, in-place. When allocating
a binding, we would need to search backwards through the bindings till
we found the end of the group matching our prefix (or an alias that
comes alphabetically before our prefix). Then we would have to search
backwards through our prefix group using the id, till we found a binding
with a smaller id.

Thoughts?

-Ben

> > 
> >                 if (curr_id <= 0)
> >                         continue;
> > 
> >                 while (id < curr_id) {
> >                         if (!id_already_taken(id, prefix, map_wwid))
> >                                 return id;
> >                         id++;
> >                 }
> >                 if (id == INT_MAX)
> >                         break;
> >                 id++;
> >         }
> >         if (id == INT_MAX)
> >                 condlog(0, "no more available user_friendly_names");
> >         return id < INT_MAX ? id : -1;
> > }
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel