| Message ID | 1600923569-17412-1-git-send-email-bmarzins@redhat.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | add library to check if device is a valid path | expand |
On Wed, 2020-09-23 at 23:59 -0500, Benjamin Marzinski wrote: > The main part of the this patchset is the first patch, which adds a > new library interface to check whether devices are valid paths. This > was designed for use in the Storage Instantiation Daemon (SID). > > https://github.com/sid-project > > Hopefully, I've removed all the controvertial bits from the last time > I > proposed this library. > > The second patch adds get_uid fallback code for dasd devices. The > third > patch just changes the get_uid log level for devices configured with > uid_attribute "". This is because it is currently necessary to > configure > multipath with > > overrides { > uid_attribute "" > } > > to claim multipath devices with SID (instead of using > multipath.rules), > since SID doesn't currently get the UID information itself, and it is > called by udev before this information is added to the udev database. > This makes me wonder how SID and multipathd are supposed to coexist. We wouldn't want this overrides directive for multipathd itself, would we? Actually that "overrides" begs for allowing a custom configuration file for libmultipath to be used with SID. OTOH, that would allow the configurations to diverge, which might cause issues, too (in particular if blacklisting options were different). I think what we should do is allow applications to set overrides like this at runtime, modifying the configuration. Perhaps we could support an application-specific, additional config_dir, from which items like the above could be read in addition to the regular configuration files. This additional configuration would not be used by multipathd and multipath. Does that make sense? Regards Martin
On Thu, Sep 24, 2020 at 08:18:02AM +0000, Martin Wilck wrote: > On Wed, 2020-09-23 at 23:59 -0500, Benjamin Marzinski wrote: > > The main part of the this patchset is the first patch, which adds a > > new library interface to check whether devices are valid paths. This > > was designed for use in the Storage Instantiation Daemon (SID). > > > > https://github.com/sid-project > > > > Hopefully, I've removed all the controvertial bits from the last time > > I > > proposed this library. > > > > The second patch adds get_uid fallback code for dasd devices. The > > third > > patch just changes the get_uid log level for devices configured with > > uid_attribute "". This is because it is currently necessary to > > configure > > multipath with > > > > overrides { > > uid_attribute "" > > } > > > > to claim multipath devices with SID (instead of using > > multipath.rules), > > since SID doesn't currently get the UID information itself, and it is > > called by udev before this information is added to the udev database. > > > > This makes me wonder how SID and multipathd are supposed to coexist. > We wouldn't want this overrides directive for multipathd itself, would > we? Don't we need it to be used by everything? We certainly don't want multipathd to get a different value for the wwid than SID has. It seems like all programs that access the multipath devices should use the same method to get the WWIDs. The long term solution is that SID will call out to the devices, and grab these uid attributes, just like udev currently does. It already does this for the blkid values. When SID is running, these udev rules will be disabled, and SID will provide udev with this data. So eventually, multipath won't need any configuration changes to work with SID. setting this overrides line is just a stop-gap, so that people can test SID and multipath now. -Ben > Actually that "overrides" begs for allowing a custom configuration file > for libmultipath to be used with SID. OTOH, that would allow the > configurations to diverge, which might cause issues, too (in particular > if blacklisting options were different). > > I think what we should do is allow applications to set overrides like > this at runtime, modifying the configuration. Perhaps we could support > an application-specific, additional config_dir, from which items like > the above could be read in addition to the regular configuration files. > This additional configuration would not be used by multipathd and > multipath. Does that make sense? > > Regards > Martin > > -- > Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107 > SUSE Software Solutions Germany GmbH > HRB 36809, AG Nürnberg GF: Felix > Imendörffer > -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
On Thu, 2020-09-24 at 11:30 -0500, Benjamin Marzinski wrote: > On Thu, Sep 24, 2020 at 08:18:02AM +0000, Martin Wilck wrote: > > On Wed, 2020-09-23 at 23:59 -0500, Benjamin Marzinski wrote: > > > > > > The second patch adds get_uid fallback code for dasd devices. The > > > third > > > patch just changes the get_uid log level for devices configured > > > with > > > uid_attribute "". This is because it is currently necessary to > > > configure > > > multipath with > > > > > > overrides { > > > uid_attribute "" > > > } > > > > > > to claim multipath devices with SID (instead of using > > > multipath.rules), > > > since SID doesn't currently get the UID information itself, and > > > it is > > > called by udev before this information is added to the udev > > > database. > > > > > > > This makes me wonder how SID and multipathd are supposed to > > coexist. > > We wouldn't want this overrides directive for multipathd itself, > > would > > we? > > Don't we need it to be used by everything? We certainly don't want > multipathd to get a different value for the wwid than SID has. It > seems > like all programs that access the multipath devices should use the > same > method to get the WWIDs. Well, as soon as SID is authoritative for the question whether or not a device should be multipathed, this is certainly true. That would mean, then, that multipath-tools would abandon the current philosophy of relying on udev, and attempt to derive device properties directly from sysfs instead. That makes a certain amount of sense to me, even though it's contrary to what we've been doing the last years. Relying on udev is not without issues, as we both know. uevents being delayed or never delivered is one problem, the other is the fact that udev rules can be customized without limits, leading to a profliferation of variables and configuration options. All this has historical reasons, today we could very well obtain almost all device attributes we need directly from sysfs. ... but I gather that this is a temporary scenario ... > The long term solution is that SID will call out to the devices, and > grab these uid attributes, just like udev currently does. It already > does this for the blkid values. When SID is running, these udev rules > will be disabled, and SID will provide udev with this data. So > eventually, multipath won't need any configuration changes to work > with > SID. setting this overrides line is just a stop-gap, so that people > can > test SID and multipath now. So, SID will call into libmultipath via libmpathvalid, udev will obtain the properties from SID, and multipathd will fetch them from udev in turn? Or will multipathd talk directly to SID? I seem to be missing the overall picture. Anyway, if you can live with simply adding an "overrides" statement to multipath.conf for SID at the current stage, fine with me. Forgot to say: ACK for this series from me, with the exception of the minor nit I had on #3. Regards, Martin
On Thu, Sep 24, 2020 at 07:22:21PM +0000, Martin Wilck wrote: > On Thu, 2020-09-24 at 11:30 -0500, Benjamin Marzinski wrote: > > On Thu, Sep 24, 2020 at 08:18:02AM +0000, Martin Wilck wrote: > > > On Wed, 2020-09-23 at 23:59 -0500, Benjamin Marzinski wrote: > > > > > > > > The second patch adds get_uid fallback code for dasd devices. The > > > > third > > > > patch just changes the get_uid log level for devices configured > > > > with > > > > uid_attribute "". This is because it is currently necessary to > > > > configure > > > > multipath with > > > > > > > > overrides { > > > > uid_attribute "" > > > > } > > > > > > > > to claim multipath devices with SID (instead of using > > > > multipath.rules), > > > > since SID doesn't currently get the UID information itself, and > > > > it is > > > > called by udev before this information is added to the udev > > > > database. > > > > > > > > > > This makes me wonder how SID and multipathd are supposed to > > > coexist. > > > We wouldn't want this overrides directive for multipathd itself, > > > would > > > we? > > > > Don't we need it to be used by everything? We certainly don't want > > multipathd to get a different value for the wwid than SID has. It > > seems > > like all programs that access the multipath devices should use the > > same > > method to get the WWIDs. > > Well, as soon as SID is authoritative for the question whether or not a > device should be multipathed, this is certainly true. > > That would mean, then, that multipath-tools would abandon the current > philosophy of relying on udev, and attempt to derive device properties > directly from sysfs instead. That makes a certain amount of sense to > me, even though it's contrary to what we've been doing the last years. > Relying on udev is not without issues, as we both know. uevents being > delayed or never delivered is one problem, the other is the fact that > udev rules can be customized without limits, leading to a > profliferation of variables and configuration options. All this has > historical reasons, today we could very well obtain almost all device > attributes we need directly from sysfs. > > ... but I gather that this is a temporary scenario ... > > > The long term solution is that SID will call out to the devices, and > > grab these uid attributes, just like udev currently does. It already > > does this for the blkid values. When SID is running, these udev rules > > will be disabled, and SID will provide udev with this data. So > > eventually, multipath won't need any configuration changes to work > > with > > SID. setting this overrides line is just a stop-gap, so that people > > can > > test SID and multipath now. > > So, SID will call into libmultipath via libmpathvalid, udev will obtain > the properties from SID, and multipathd will fetch them from udev in > turn? Or will multipathd talk directly to SID? I seem to be missing the > overall picture. Yeah. SID will populate the udev database with the necessary udev properties, and multipathd will get those udev properties just like it always does. There is no change necessary to multipathd. Right now, the only thing that SID's multipath module replaces is multipath.rules, and it doesn't currently handle smart mode, since the part of SID that's necssary to trigger and respond to a timeout doesn't exist yet. SID sets the udev properties just like anything else called by udev. There's a SID udev rule like this: # cat /lib/udev/rules.d/00-sid.rules SUBSYSTEM=="block", ACTION=="add|change|remove", IMPORT{program}="usid scan" > Anyway, if you can live with simply adding an "overrides" statement to > multipath.conf for SID at the current stage, fine with me. > > Forgot to say: ACK for this series from me, with the exception of the > minor nit I had on #3. > > Regards, > Martin > > -- > Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107 > SUSE Software Solutions Germany GmbH > HRB 36809, AG Nürnberg GF: Felix > Imendörffer > -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
On Thu, 2020-09-24 at 20:08 -0500, Benjamin Marzinski wrote: > On Thu, Sep 24, 2020 at 07:22:21PM +0000, Martin Wilck wrote: > > > > So, SID will call into libmultipath via libmpathvalid, udev will > > obtain > > the properties from SID, and multipathd will fetch them from udev > > in > > turn? Or will multipathd talk directly to SID? I seem to be missing > > the > > overall picture. > > Yeah. SID will populate the udev database with the necessary udev > properties, and multipathd will get those udev properties just like > it > always does. But then I'm not getting how you'll get along with a SID-specific configuration for libmultipath's behavior. You want get_uid() to use direct sysfs access for SID, and use udev for multipath(d). How else would you achieve that? More generally, I'm not quite convinced of the the design yet. The information flow kernel -> (sysfs or ioctl) -> libmultipath(sid mode) -> libmpathvalid -> SID -> udev -> udev db -> libudev -> libmultipath (udev mode) -> multipathd is more complex than it needs to be. It might actually increase the lags experienced by multipathd, which will still have to wait for uevent workers to finish until it can be certain about device properties. Not to mention that SID must be rock stable and always available during boot, initrd processing, etc. Why don't we rather write a common library for determining WWIDs and the "should be multipathed" predicate, to be used by udev (with a plugin), multipath-tools, SID, and possibly other tools like systemd and LVM, with common, simple configuration, guaranteed to always provide the same results? I mean, libmultipath already has all the "intelligence" built-in to do this. We'd "just" need to cut down configuration options drastically to get more reprocucible results, and refactor things to obtain a minimalistic API. Unlike the current libmpathvalid design, this wouldn't be built on top of current libmultipath, rather vice-versa. multipath-tools would also benefit a lot from such work. Regards Martin
On Fri, Sep 25, 2020 at 10:01:01AM +0000, Martin Wilck wrote: > On Thu, 2020-09-24 at 20:08 -0500, Benjamin Marzinski wrote: > > On Thu, Sep 24, 2020 at 07:22:21PM +0000, Martin Wilck wrote: > > > > > > So, SID will call into libmultipath via libmpathvalid, udev will > > > obtain > > > the properties from SID, and multipathd will fetch them from udev > > > in > > > turn? Or will multipathd talk directly to SID? I seem to be missing > > > the > > > overall picture. > > > > Yeah. SID will populate the udev database with the necessary udev > > properties, and multipathd will get those udev properties just like > > it > > always does. > > But then I'm not getting how you'll get along with a SID-specific > configuration for libmultipath's behavior. You want get_uid() to use > direct sysfs access for SID, and use udev for multipath(d). How else > would you achieve that? In the future, libmapathvalid will have access to the udev properties that SID will be passing back to udev, so it can use the same data. Those values just aren't there yet. I admit, how exactly this will work is not completely nailed down. > More generally, I'm not quite convinced of the the design yet. The > information flow kernel -> (sysfs or ioctl) -> libmultipath(sid mode) > -> libmpathvalid -> SID -> udev -> udev db -> libudev -> libmultipath > (udev mode) -> multipathd is more complex than it needs to be. It might > actually increase the lags experienced by multipathd, which will still > have to wait for uevent workers to finish until it can be certain about > device properties. Not to mention that SID must be rock stable and > always available during boot, initrd processing, etc. Yes, clearly SID will need to be just as robust as udev. > Why don't we rather write a common library for determining WWIDs and > the "should be multipathed" predicate, to be used by udev (with a > plugin), multipath-tools, SID, and possibly other tools like systemd > and LVM, with common, simple configuration, guaranteed to always > provide the same results? I mean, libmultipath already has all the > "intelligence" built-in to do this. We'd "just" need to cut down > configuration options drastically to get more reprocucible results, and > refactor things to obtain a minimalistic API. Unlike the current > libmpathvalid design, this wouldn't be built on top of current > libmultipath, rather vice-versa. multipath-tools would also benefit a > lot from such work. Right now a lot of this infomation is being gathered by libraries. For udev's builtin commands, those libraries are already being called directly. SID will just be calling all those libraries directly, instead of having to exec a program that bascially just calles the library. Obviously not everything is in a library, however. But the idea of WWID library sounds great. libmultipath probably doesn't have all the intelligence built-in, because I assume people would want this library would handle more device types than multipath does. Although you are correct that just with what libmultipath does now, it would still have a use. -Ben > Regards > Martin > > -- > Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107 > SUSE Software Solutions Germany GmbH > HRB 36809, AG Nürnberg GF: Felix > Imendörffer > -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
The main part of the this patchset is the first patch, which adds a new library interface to check whether devices are valid paths. This was designed for use in the Storage Instantiation Daemon (SID). https://github.com/sid-project Hopefully, I've removed all the controvertial bits from the last time I proposed this library. The second patch adds get_uid fallback code for dasd devices. The third patch just changes the get_uid log level for devices configured with uid_attribute "". This is because it is currently necessary to configure multipath with overrides { uid_attribute "" } to claim multipath devices with SID (instead of using multipath.rules), since SID doesn't currently get the UID information itself, and it is called by udev before this information is added to the udev database. I'm resubmitting this as a RFC patchset. Assuming they are ACKed, I will resend these patches again, along with a man page, and unit tests, on top of Martin's revised version of his "multipath-tools: shutdown, libdevmapper races, globals" patchset. changes from v1 --------------- 0001: This patch is now rebased on top of, and makes use of Martin's patches that provide a default *_multipath_config, udev, and logsink. Because of this, mpathvalid_init() now has a parameter used to set logsink. There is also a new API function, mpathvalid_reload_config(). 0003: This is completely new, since Martin pointed out that adding a new config option to always use the fallback getuid code was unnecessary. It just makes a uid_attribute of "" log at normal levels. Benjamin Marzinski (3): multipath: add libmpathvalid library libmultipath: add uid failback for dasd devices libmultipath: change log level for null uid_attribute Makefile | 3 +- libmpathvalid/Makefile | 38 ++++++ libmpathvalid/libmpathvalid.version | 10 ++ libmpathvalid/mpath_valid.c | 199 ++++++++++++++++++++++++++++ libmpathvalid/mpath_valid.h | 61 +++++++++ libmultipath/defaults.h | 1 + libmultipath/discovery.c | 40 +++++- 7 files changed, 349 insertions(+), 3 deletions(-) create mode 100644 libmpathvalid/Makefile create mode 100644 libmpathvalid/libmpathvalid.version create mode 100644 libmpathvalid/mpath_valid.c create mode 100644 libmpathvalid/mpath_valid.h