[dm-devel] uid_attrs (was Re: [PATCH 04/11] multipathd: add need_do_map to indicate whether need calling domap() in ev_add_path())

[Martin Wilck]

Hi Ben & Tang,

On Tue, 2017-01-17 at 10:14 -0600, Benjamin Marzinski wrote:
> On Tue, Jan 17, 2017 at 03:28:06PM +0800, tang.junhui at zte.com.cn
> wrote:
> >    Hello Ben
> > 
> >    Thank you for your patience again.
> > 
> >    I'll modify code according to your suggestion as this:
> >    1) add configuration in the defaults section
> >       uid_attrs "sd:ID_SERIAL dasd:ID_UID"
> >       it would override any other configurations if this
> >       filed is configured and matched;
> > 
> >    2) In uevent processing thread, we will assign merge_id
> >       according the label in uevents by this configuration;
> > 
> >    3) this patch will take back:
> >       [PATCH 12/12] libmultipath: use existing wwid when
> >       wwid has already been existed in uevent
> > 
> >    4) if this field is not configured, only do filtering and
> >       no merging works.
> > 
> >    Please confirm whether this modification is feasible.
> 
> Yes. This is perfectly reasonable solution.  Thanks for doing all the
> work on this.

I'm sorry to jump upon this old discussion again. I've just recently
realized that with the introduction of "uid_attrs", the section on
"WWID generation" in multipath.conf(5) needs to be corrected. Looking
into that, I find it unfortunate that this new option was introduced.
It makes an already complex configuration exercise even more confusing
(try to rewrite that man page paragraph in an easily understandable way
and I think you'll know what I mean).

>From the end user point of view, "uid_attrs" seems to simply duplicate
the functionality of "uid_attribute", just in more awkward syntax, and
with higher priority. As a side effect, it enables uevent merging,
which is documented in the man page but unexpected from the name of the
option.

I went through Ben's reasoning from Jan 16th again:

> In general, the code to set need_do_map if the wwid and merge_id
> don't
> match only works if either none of the device in a merged set have
> wwids
> that match the merge_id, or if the last device has a wwid that
> matches
> the merge_id. If there are devices with wwids that match the
> merge_id,
> but the last device in the set isn't one of them, then some devices
> will
> not get a multipath device created for them.
> 
[...]
The easy way to fix it is to use the other possibility that I
mentioned
> before, which is to not have the merge_id, and just use the udev
> provided wwid, instead of fetching it from pathinfo.  Like I
> mentioned,
> if you do this, you want to make sure that you only try to grab the
> wwid
> from the udev events for devices with the correct kernel name:
> ID_SERIAL
> only for "sd.*" devices, and ID_UID only for "dasd.*" devices. I also
> think that this should be configurable.
> 
> Otherwise, you can either go through the messy work of calling domap
> correctly when the last device of a set has a wwid that doesn't match
> the merge_id, or we can decide that this won't acutally cause
> problems
> with any known device, and punt fixing it for now. And if it causes
> problems with some future devices, we can deal with it then.

It appears that Tang chose Ben's proposed "easy way" to fix the problem
described. The key point appears to be to "use the udev provided wwid,
instead of fetching it from pathinfo". But the end result may be
confusing for users.

I didn't react at the time, but in hindsight I'd find it much better if
multipath's standard WWID generation procedure had been used for uevent
merging. I admit I don't fully grok why that's harder to implement than
the solution that got merged, probably because I didn't try yet :-) 
Anyway, I wonder if it would still be possible to change this at this
point in time (i.e. after "uid_attrs" was merged).

Cheers,
Martin

PS: And while I'm nitpicking anyway: The description of "uid_attrs" in
the man page talks about "type of device", while what's really matched
against is the kernel devnode name such as /dev/sdX or /dev/dasdY. The
reader is left clueless what to do for devices other than sd or dasd (I
suspect that that's actually unsupported, but the man page doesn't say
anything about that).

I can provide a patches for this (and for "WWID generation") but I'd
like to discuss the main topic of this email first.

-- 
Dr. Martin Wilck <mwilck at suse.com>, Tel. +49 (0)911 74053 2107
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)