multipath-tools 0.9.9 #85
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Fixes: b3582da ("11-dm-mpath.rules: use import logic like 13-dm-disk.rules") Signed-off-by: Martin Wilck <[email protected]> Reviewed-by: Benjamin Marzinski <[email protected]>
condlog uses its own log levels, so LOG_WARNING makes no sense. It actually translates to a message sent at the LOG_DEBUG level. Signed-off-by: Benjamin Marzinski <[email protected]> Reviewed-by: Martin Wilck <[email protected]>
Currently multipathd always tries to run as a realtime process with a priority of 99. This is excessive. As a first step towards fixing this, make it possible at compile time to lower the priority or keep multipathd from making itself a realtime process. Signed-off-by: Benjamin Marzinski <[email protected]> Reviewed-by: Martin Wilck <[email protected]>
Despite 03a5456 ("libmultipath: always use glibc basename()"), we still call the system library's basename() from libmultipath. musl libc until 1.24 provided a prototype for basename() in string.h, which was not correct and was resolved to the destructive POSIX basename(). musl libc 1.25 removed this prototype. While the remaining code path doesn't strictly depend on the non-destructive behavior of glibc's basename(), it's cleaner and safer to use the same implementation everywhere. Fixes: 03a5456 ("libmultipath: always use glibc basename()") Fixes: opensvc#84 Signed-off-by: Martin Wilck <[email protected]> Reviewed-by: Khem Raj <[email protected]> Reviewed-by: Benjamin Marzinski <[email protected]>
With this change, if the SCHED_RT_PRIO compiler flag has been removed. Instead multipathd will call getrlimit(RLIMIT_RTPRIO, ...) and look at the hard limit. It it's 0, multipath will do nothing. Otherwise it will change its scheduling policy to SCHED_RR and its priority to the hard limit. This allows users to change the priority of that multipathd runs with by adding LimitRTPRIO=<prio> to the [Service] section of the multipathd.service unit file. Setting LimitRTPRIO=0 will make multipathd run as a normal process, while setting LimitRTPRIO=infinity will make it use the maximum SCHED_RR prio, which is 99. To keep the existing behavior, multipathd.service now sets LimitRTPRIO=infinity Signed-off-by: Benjamin Marzinski <[email protected]> Reviewed-by: Martin Wilck <[email protected]>
If multipathd doesn't become a real time process, it was scheduled as a normal process, without any priority increase. Bump up the CPUWeight so that even as a normal process, it will still run with increased priority. If multipathd did become a real time process, it set itself to the highest priority, which is excessive. A priority of 10 is plenty. Signed-off-by: Benjamin Marzinski <[email protected]> Reviewed-by: Martin Wilck <[email protected]>
…ended Add a comment to explain why we must set MPATH_DEVICE_READY=0 when we see a device becoming ready (number of active paths 0 -> 1) while the device is suspended. Signed-off-by: Martin Wilck <[email protected]> Reviewed-by: Benjamin Marzinski <[email protected]>
DM_NOSCAN is our "output" flag for 13-dm-disk.rules, and it should be treated the same way as DM_UDEV_DISABLE_OTHER_RULES_FLAG, which isn't imported from the udev database. The state that we need to remember is MPATH_DEVICE_READY, which we've already imported above, and we will set the "output" flags accordingly in the "force_activation" code path further down. Signed-off-by: Martin Wilck <[email protected]> Reviewed-by: Benjamin Marzinski <[email protected]>
Use the same set of properties to import as 13-dm-disk.rules. ID_FS_VERSION isn't used in any udev rule I am aware of. Signed-off-by: Martin Wilck <[email protected]> Reviewed-by: Benjamin Marzinski <[email protected]>
…date With the late patches for 10-dm.rules, DM_UDEV_DISABLE_OTHER_RULES_FLAG isn't restored from the udev database any more, so we don't need to restore the flag to its original state before it is saved to the db. Signed-off-by: Martin Wilck <[email protected]> Reviewed-by: Benjamin Marzinski <[email protected]>
With late late patches for 10-dm.rules, DM_UDEV_DISABLE_OTHER_RULES_FLAG is never restored from the udev db. Thus we don't need to clear it here any more for coldplug events. Also, we must use .DM_SUSPENDED instead of DM_SUSPENDED as input flag with the v3 rule set (other occurences of DM_SUSPENDED will be replaced in a follow-up patch). Signed-off-by: Martin Wilck <[email protected]> Reviewed-by: Benjamin Marzinski <[email protected]>
With the late changes to 13-dm-disk.rules, we don't need to import any
blkid-generated properties from the udev database, because they will
be imported later.
Except for ID_FS_TYPE, this actually holds since lvm2 commit 94f77a4 ("udev:
import previous results of blkid when in suspended state"), included in lvm2
2.03.19, but we have no simple way to detect the version of the lvm2 rules.
Signed-off-by: Martin Wilck <[email protected]>
Reviewed-by: Benjamin Marzinski <[email protected]>
With the late changes to the device mapper rules, DM_SUSPENDED is not exported any more. Use .DM_SUSPENDED instead. Note that although 11-dm-mpath.rules is not a part of lvm2, it can be considered as part of the device-mapper layer (everything before 13-dm-disk.rules can), and is thus allowed to use .DM_SUSPENDED. In practice .DM_SUSPENDED is equivalent to DM_UDEV_DISABLE_OTHER_RULES_FLAG for multipath devices, but using .DM_SUSPENDED here makes the intention more obvious. Signed-off-by: Martin Wilck <[email protected]> Reviewed-by: Benjamin Marzinski <[email protected]>
We don't need to restore DM_NOSCAN from the db anymore, so we can rename it to .DM_NOSCAN, making it a temporary property. Signed-off-by: Martin Wilck <[email protected]> Reviewed-by: Benjamin Marzinski <[email protected]>
This combination of a GOTO and a simple rule can be combined into a single rule. Signed-off-by: Martin Wilck <[email protected]> Reviewed-by: Benjamin Marzinski <[email protected]>
The labels "dont_activate" and "scan_import" denote the same code line. Remove "dont_activate". Improve two misleading label names. Substitutions: dont_activate -> scan_import force_activation -> check_mpath_ready mpath_action -> check_mpath_unchanged Signed-off-by: Martin Wilck <[email protected]> Reviewed-by: Benjamin Marzinski <[email protected]>
DM_SUSPENDED=1 implies DM_UDEV_DISABLE_OTHER_RULES_FLAG=1, no need to check it again. The DM_NOSCAN check needs to remain in order to keep compatibility with dm rules v2. Signed-off-by: Martin Wilck <[email protected]> Reviewed-by: Benjamin Marzinski <[email protected]>
glibc 2.37 has added several new wrappers around the ioctl and io_getevents system calls on 32 bit systems, to deal with 64bit time_t. These aren't resolved with cmocka's wrapping technique. Fix this with C preprocessor trickery, similar to 7b217f8 ("multipath-tools: Makefile.inc: set _FILE_OFFSET_BITS=64"). Note: the directio test with DIO_TEST_DEV for fails under qemu-linux-user with foreign-arch binfmt, because aio-related system calls are unsupported by qemu-linux-user. See https://gitlab.com/qemu-project/qemu/-/issues/210. Signed-off-by: Martin Wilck <[email protected]> Reviewed-by: Benjamin Marzinski <[email protected]>
Fedora's glibc 2.39 includes the following patch: https://patches.linaro.org/project/libc-alpha/patch/[email protected]/ It causes open("file", O_RDONLY) to resolve to __open64_2(), whereas it resolves to open64() with gcc, causing CI failures because of wrong wrapper substitutions. Signed-off-by: Martin Wilck <[email protected]> Reviewed-by: Benjamin Marzinski <[email protected]>
Signed-off-by: Martin Wilck <[email protected]>
We want to run workflows if the workflow file itself has changed. Fixes: 2cbe81a ("GitHub Workflows: run expensive workflows only on relevant changes") Signed-off-by: Martin Wilck <[email protected]>
TGTDIR is a convenience option for developers for building multipath-tools such that compiled-in paths of the plugins, config files, etc. match an installation under some optional path. See README.md for instructions and examples. Signed-off-by: Martin Wilck <[email protected]> Reviewed-by: Benjamin Marzinski <[email protected]>
No functional changes, just code movement. Signed-off-by: Martin Wilck <[email protected]> Reviewed-by: Benjamin Marzinski <[email protected]>
A helper function that searches a struct multipath by dev_t, and works whether or not mpp->paths is currently available. Signed-off-by: Martin Wilck <[email protected]> Reviewed-by: Benjamin Marzinski <[email protected]>
This reverts commit bbb77f3. Reviewed-by: Benjamin Marzinski <[email protected]>
Changing max_sectors_kb on a live map is dangerous. When I/O occurs while the max_sectors_kb value of a map is larger than that of any of its paths, I/O errors like this may result: blk_insert_cloned_request: over max size limit. (127 > 126) This situation must be strictly avoided. The kernel makes sure that the map's limits match those of the paths when the map is created. But setting max_sectors_kb on path devices before reloading is dangerous, even if we read the value from the map's max_sectors_kb beforehand. The reason for this is that the sysfs value max_sectors_kb is the kernel's max_sectors divided by 2, and user space has no way to figure out if the kernel-internal value is odd or even. Thus by writing back the value just read to max_sectors_kb, one might actually decrease the kernel value by one. The only safe way to set max_sectors_kb on a live map would be to suspend it, flushing all outstanding IO, then the path max_sectors_kb, reload, and resume. Since commit 8fd4868 ("libmultipath: don't set max_sectors_kb on reloads"), we don't set the configured max_sectors_kb any more. But as shown above, this is not safe. So really only set max_sectors_kb when creating a map. Users who have stacked block devices on top of multipath, as described in the commit message of 8fd4868 must make sure that the max_sectors_kb setting is correct when the multipath map is first created. Decreasing the map's max_sectors_kb value (without touching the paths) should actually be possible in this situation, because stacked block devices are usually bio-based, and bio-based IO (in contrast to request-based) can be split if the lower-level device has can't handle the size of the I/O. Signed-off-by: Martin Wilck <[email protected]> Reviewed-by: Benjamin Marzinski <[email protected]>
As explained in the previous commit, setting max_sectors_kb for paths that are members of a live map is dangerous. But as long as a path is not a member of a map, setting max_sectors_kb poses no risk. Set the parameter in adopt_paths(), for paths that aren't members of the map yet. In order to determine whether or not a path is currently member of the map, adopt_paths() needs to passed the current member list. If (and only if) it's called from coalesce_paths(), the passed struct multipath doesn not represent the current state of the map; adopt_paths() must therefore get a new argument current_mpp that represents the kernel state. We must still call sysfs_set_max_sectors_kb() from domap() in the ACT_CREATE case, because when adopt_paths is called from coalesce_paths() for a map that doesn't exist yet, mpp->max_sectors_kb is not set. Signed-off-by: Martin Wilck <[email protected]> Reviewed-by: Benjamin Marzinski <[email protected]>
Signed-off-by: Martin Wilck <[email protected]> Reviewed-by: Benjamin Marzinski <[email protected]>
Signed-off-by: Martin Wilck <[email protected]> Reviewed-by: Benjamin Marzinski <[email protected]>
Instead of adding the new 5300 model, replace them with wildcards. Cc: Martin Wilck <[email protected]> Cc: Benjamin Marzinski <[email protected]> Cc: Christophe Varoqui <[email protected]> Cc: DM-DEVEL ML <[email protected]> Signed-off-by: Xose Vazquez Perez <[email protected]> Reviewed-by: Benjamin Marzinski <[email protected]> Reviewed-by: Martin Wilck <[email protected]>
Add "If enabled, " to the sentence about multipathd.socket, since it is no longer enabled by default. Signed-off-by: Benjamin Marzinski <[email protected]> Reviewed-by: Martin Wilck <[email protected]>
Add the missing output for manual failback and print the defferral time for deferred failbacks, if one isn't currently in progress. Signed-off-by: Benjamin Marzinski <[email protected]> Reviewed-by: Martin Wilck <[email protected]>
If no_path_retry was greater than 0, multipathd was counting a map failure when recovery mode was entered, and again when queueing was disabled. The first one is incorrect, since the map is still queueing. Signed-off-by: Benjamin Marzinski <[email protected]> Reviewed-by: Martin Wilck <[email protected]>
Also, the description for "del map $map" was incorrect. Signed-off-by: Benjamin Marzinski <[email protected]> Reviewed-by: Martin Wilck <[email protected]>
The %s multipath and path wildcards both say they print the device vend/prod/rev string, but neither of them do. The multipath wildcards already provide a way to print the revision string and the %s wildcard is used in the multipath -l output, so leave the wildcard output alone, and change the description to only mention the vendor and product. There is no other way to print the revision by path, and the path %s wildcard is only used in the verbose multipath output, so make it actually print the revision. Also check for unset strings, and print "##" instead of nothing. Signed-off-by: Benjamin Marzinski <[email protected]> Reviewed-by: Martin Wilck <[email protected]>
(mwilck: minor spelling fixes applied). Suggested-by: Nitin Yewale <[email protected]> Signed-off-by: Benjamin Marzinski <[email protected]> Reviewed-by: Martin Wilck <[email protected]>
This comment has been minimized.
This comment has been minimized.
Signed-off-by: Martin Wilck <[email protected]>
Signed-off-by: Martin Wilck <[email protected]>
has been resolved with latest push |
|
@yu-re-ka, forgive my ignorance, I have zero knowledge about NixOS. Can you provide a quick recipe for me to reproduce your issue, preferably in a container? |
|
It's strange because your environment uses the same base versions that I have in mine (openSUSE Tumbleweed). The NixOS issue seems to be here: But if you look at the file and So if you are actually compiling against glibc1, you should be fine because of the If this doesn't work, I fear I have to ask you to do some debugging on your own behalf and see what's going wrong in your environment. Footnotes
|
|
So running with these changes to the nixos-24.05 branch I get this, looks like util.h isn't getting included by my eyes. Diff to the derivation: Built locally via nix build .#legacyPackages.x86_64-linux.multipath-tools -L Not entirely sure whats going on the makefiles are a bit confusing but it looks more to me like the makefiles aren't including what is expected. |
|
Noting from glibc, the import we want is <libgen.h> according to: And rerunning with these changes to those test files like so (and killing the const warning as a const char seems to be being passed into basename): I get to here where util.h is definitely not getting sent to CFLAGS as an include dir: |
|
The problem is in the nix derivation itself, not in the multipath-tools code. Because of the insertion of The insertion of Next time, instead of creating downstream changes or string substitution hacks, please send patches upstream that make the NixOS build succeed without such customizations. We won't reject such patches unless they break something in other distros. Footnotes |
|
This code block in multipath-tools The first two sed commands replace patterns which we don't use any more, and the 3rd one appears totally pointless to me. |
Historically, it was necessary. It has been obsoleted by 02bc889. |
13 tasks
|
Thank you so much for figuring this out! I have not been involved with the multipath-tools package before except updating it to this new version / backporting the fix for the new musl basename compatibility things. These lines are quite old (up to 8 years!). I would like to believe we are a lot more strict in how we patch around code nowadays. Anyways, it's cleaned up now. |
|
while we're discussing this, what's the right way to insert absolute paths to executables e.g. in systemd unit files or udev rules in NixoS? |
|
@cvaroqui, as the NixOS issue is resolved, can we go forward with this PR? |
yu-re-ka
pushed a commit
to NixOS/nixpkgs
that referenced
this pull request
Jun 12, 2024
The upstream devs have pointed out[^1] that these sed commands are obsolete and are responsible for issues with building the 0.9.9 release (not tagged yet) [^1]: opensvc/multipath-tools#85 (comment)
this depends: is it a executable from your own application, or from another
|
|
I think we can do the former (I'm currently working on a patch series for it) but not the latter, which is just too NixOS-specific and up to the Nix developers to care about. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Important note
It is not recommended to use lvm2 2.03.24 and newer with multipath-tools
versions earlier than 0.9.9. See "Other major changes" below.
User-Visible Changes
realtime priority, 99. This has always been excessive, and on some
distributions (e.g. RHEL 8), it hasn't worked at all. It is now possible to
set multipathd's real time scheduling by setting the hard limit for
RLIMIT_RTPRIO(see getrlimit(2)), which corresponds to thertpriosetting in limits.conf and to
LimitRTPRIO=in the systemd unit file. Thedefault in the systemd unit file has been set to 10. If the limit is set to
0, multipathd doesn't attempt to enable real-time scheduling.
Otherwise, it will try to set the scheduling priority to the given value.
Fixes #82.
sufficient priority even if real time scheduling is switched off, the
CPUWeight=setting in the unit file is set to 1000. This is necessarybecause regular nice(2) values have no effect in systems with cgroups enabled.
max_sectors_kbconfiguration: multipathd appliesthe
max_sectors_kbsetting only during map creation, or when a new path isadded to an existing map. The kernel makes sure that the multipath device
never has a larger
max_sectors_kbvalue than any of its configured pathdevices. The reason for this change is that applying
max_sectors_kbonlive maps can cause IO errors and data loss in rare situations.
It can now happen that some path devices have a higher
max_sectors_kbvalue than the map; this is not an error. It is not possible any more to
decrease
max_sectors_kbinmultipath.confand runmultipathd reconfigureto "apply" this setting to the map and its paths. If decreasingthe IO size is necessary, either destroy and recreate the map, or remove one
path with
multipathd del path $PATH, runmultipathd reconfigure, andre-add the path with
multipathd add path $PATH.%kformax_sectors_kbhas been added tothe
multipathd show paths formatandmultipathd show maps formatcommands.
flush_on_last_deloptionnow takes the values
always,unused, ornever.yesandnoare still accepted as aliases for
alwaysandunused, respectively.alwaysmeans that when all paths for a multipath map have been removed,outstanding IO will be failed and the map will be deleted.
unusedmeansthat this will only happen when the map is not mounted or otherwise opened.
nevermeans the map will only be removed if thequeue_if_no_pathfeature is off.
This fixes a problem where multipathd could hang when the last path of
a queueing map was deleted.
$maparguments in multipathd interactive shell: The$mapargument in commands likeresize map $mapnow accepts a WWID,and poorly chosen map aliases that could be mistaken for device names.
show maps formatandshow paths formatcommands are documented in themultipathd(8) man page now.
%swildcard for paths: this wildcard inshow paths formatnow printsthe product revision, too.
Other major changes
device mapper udev rules (
DM_UDEV_RULES_VSN==3, lvm2 >= 2.03.24). They arestill compatible with older versions of the device-mapper udev
rules (lvm2 < 2.03.24). If lvm2 2.03.24 or newer is installed, it is
recommended to update multipath-tools to 0.9.9 or newer.
See also LVM2 2.03.24 release notes.
Bug fixes
glibc_basename()to avoid some issues with MUSL libc.Fixes #84.
no_path_retry > 0.commands and have thus been removed from the
show wildcardscommandoutput.
Other
TGTDIRoption to simplify building for a different targethost (see README.md).
Shortlog
@bmarzins (17):
multipathd: use condlog level for setscheduler error message
multipathd: make multipathd scheduling configurable
multipathd: make multipathd set priority to RLIMIT_RTPRIO
multipathd: Set CPUWeight to 1000 and LimitRTPRIO to 10
libmultipath: export partmap_in_use
libmultipath: change flush_on_last_del to fix a multipathd hang
libmultipath: remove redundant config option from InfiniBox config
libmultipath: pad dev_loss_tmo to avoid race with no_path_retry
libmultipath: fix deferred_remove function arguments
libmultipath: remove redundant config option from InfiniBox config
libmultipath: pad dev_loss_tmo to avoid race with no_path_retry
libmultipath: fix deferred_remove function arguments
libmultipath: accept poorly chosen aliases in find_mp_by_str
libmultipath: accept wwids in find_mp_by_str
multipath-tools man pages: don't assume multipath.socket is enabled
libmultipath: print all values in snprint_failback
multipathd: Stop double counting map failures for no_path_retry > 0
multipath-tools man pages: add missing multipathd commands
libmultipath: change the vend/prod/rev printing
multipath-tools man pages: Add format wildcard descriptions
@mwilck (33):
11-dm-mpath.rules: fix misspelled DM_UDEV_DISABLE_OTHER_RULES_FLAG
libmpathutil: really always use glibc basename()
11-dm-mpath.rules: explain logic for device becoming ready while suspended
11-dm-mpath.rules: don't import DM_NOSCAN from udev db
11-dm-mpath.rules: don't import ID_FS_VERSION from udev db
11-dm-mpath.rules: adapt MPATH_DEVICE_READY=0 logic to 10-dm.rules update
11-dm-mpath.rules: adapt coldplug event handling ro 10-dm.rules update
11-dm-mpath.rules: don't import properties with new 13-dm-disk.rules
11-dm-mpath.rules: replace DM_SUSPENDED by .DM_SUSPENDED
11-dm-mpath.rules: replace DM_NOSCAN by .DM_NOSCAN
11-dm-mpath.rules: simplify PATH_FAILED case
11-dm-mpath.rules: make label names more intuitive
kpartx.rules: ignore DM_SUSPENDED
multipath-tools tests: fix CI failures on arm/v7 with glibc 2.37
multipath-tools tests: fix CI failures with clang on Fedora Rawhide
GitHub actions: fixes for spelling CI
GitHub workflows: run workflows if workflow file has changed
multipath-tools: add TGTDIR option
libmultipath: move get_udev_for_mpp to sysfs.c
libmultipath: add mp_find_path_by_devt()
Revert "libmultipath: fix max_sectors_kb on adding path"
libmultipath: Only set max_sectors_kb on map creation
libmultipath: set max_sectors_kb in adopt_paths()
libmultipath: add wildcard %k for printing max_sectors_kb
multipath.conf(5): update documentation for max_sectors_kb
libmultipath: use bitwise flags for map flushing API
libmultipath: use bitwise flags for dm_simplecmd API
libmultipath: add argument names to some prototypes
libmultipath: bump version to 0.9.9
GitHub Workflows: native.yaml: run for Fedora 40
update NEWS.md
multipath.conf.5.in: fix man page date
More updates to NEWS.md
@NitinYewale (1):
libmultipath: remove pathgroup wildcard options
@xosevp (3):
multipath-tools: simplify comment in hwtable
multipath-tools: unify text in multipath.conf.5
multipath-tools: update man pages dates
As usual, all patches have a
Reviewed-by:trailer, except those that just concernNEWS.md, the workflows, and the version bump, and the trivial commit 9fa3770.Update 2024-06-04
Added some more patches from @bmarzins and @NitinYewale. Mostly man pages and wildcard treatment.