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

[Benjamin Marzinski <bmarzins@redhat.com>]

On Thu, Sep 07, 2023 at 02:14:04PM -0500, Benjamin Marzinski wrote:
> 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.

So, thinking about this a little more, assuming we would be writing out
the bindings file in full alias sorted order, it's stupid to sort it
alphabetically, and then resort it back to alias order.  We should
obviously read it in using the same method as when allocating a binding.
Where you search backwards till you find your prefix group, and then
find the proper spot in the prefix group using alias sorting.  Aliases
that aren't valid for any configured prefix (these could have been added
by hand to the bindings file, or possibly they were created with a
different multipath configuration) will just get sorted alphabetically
into the bindings list, between the prefix groups.

Also, it's not enough to just ignore any device config prefix that is a
starting substring of another prefix. The device config prefixes also
cannot be superstrings of the default prefix, so if the default prefix
is "mpath", both of the device config prefixes "mp" and "mpathdev" would
need to be ignored.

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