multipath-tools 0.9.7 #77
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
"multipath -d" might change sysfs timeouts of SCSI devices. Make sure it doesn't. Signed-off-by: Martin Wilck <mwilck@suse.com> Cc: Jehan Singh <jehan.singh@suse.com> Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
Factor out a trivial helper function. Signed-off-by: Martin Wilck <mwilck@suse.com> Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
These functions are only called from select_alias(). The logic is more obvious when unified in a single function. Signed-off-by: Martin Wilck <mwilck@suse.com> Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
If the bindings file is changed in a way that multipathd can't handle (e.g. by swapping the aliases of two maps), multipathd must not try to re-use an alias that is already used by another map. Creating or renaming a map with such an alias will fail. We already avoid this for some cases, but not for all. Fix it. Signed-off-by: Martin Wilck <mwilck@suse.com> Cc: David Bond <dbond@suse.com> Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
When I read this code, I always get confused. Adding comments to explain the algorithm. Signed-off-by: Martin Wilck <mwilck@suse.com> Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
If there's a mismatch between expected and actual log message, print both messages. Signed-off-by: Martin Wilck <mwilck@suse.com> Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
Signed-off-by: Martin Wilck <mwilck@suse.com> Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
Use the macros introduced with the tests for get_user_friendly_alias() also in the previously existing tests. Signed-off-by: Martin Wilck <mwilck@suse.com> Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
This way we can further improve readability of the individual test cases. Signed-off-by: Martin Wilck <mwilck@suse.com> Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
Further improve test readablity. Signed-off-by: Martin Wilck <mwilck@suse.com> Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
Add a variable global_bindings that holds the currently active vector of bindings. This variable is freed at program exit. Signed-off-by: Martin Wilck <mwilck@suse.com> Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
We will use this function in a more generic way, give it a more generic name. Signed-off-by: Martin Wilck <mwilck@suse.com> Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
No code changes, just moving code. Signed-off-by: Martin Wilck <mwilck@suse.com> Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
This function just uses the file name, no other configuration parameters. Also, pass the Bindings argument first to use the same convention as the other functions in this file. Signed-off-by: Martin Wilck <mwilck@suse.com> Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
Save code and syscalls by assembling the content in memory first. write() may return less bytes written than expected. Deal with it. Signed-off-by: Martin Wilck <mwilck@suse.com> Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
The name of the temp file is unlikely to be helpful for users, and hard to predict in the unit test. Omit it. Signed-off-by: Martin Wilck <mwilck@suse.com> Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
This way we can test the parsing of input lines from the bindings file more easily. Signed-off-by: Martin Wilck <mwilck@suse.com> Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
Rather than opening the bindings file every time we must retrieve a binding, keep the contents in memory and write the file only if additions have been made. This simplifies the code, and should speed up alias lookups significantly. As a side effect, the aliases will be stored sorted by alias, which changes the way aliases are allocated if there are unused "holes" in the sequence of aliases. For example, if the bindings file contains mpathb, mpathy, and mpatha, in this order, the next new alias used to be mpathz and is now mpathc. Another side effect is that multipathd will not automatically pick up changes to the bindings file at runtime without a reconfigure operation. It is questionable whether these on-the-fly changes were a good idea in the first place, as inconsistent configurations may easily come to pass. It desired, it would be feasible to implement automatic update of the bindings using the existing inotify approach. The new implementation of get_user_friendly_alias() is slightly different than before. The logic is summarized in a comment in the code. Unit tests will be provided that illustrate the changes. Signed-off-by: Martin Wilck <mwilck@suse.com> Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
The different implementation of get_user_friendly_alias() and its helpers
necessitates changes in the unit tests. It would be nice if it didn't, but the
unit tests are too closely bound to the implementation to make this possible.
- The bindings table is held in memory in alphabetically sorted order, which
may change the result of looking for free alias IDs if the entries in the
bindings file were unordered initially. In particular, if only a small
number of bindings exists, "holes" in the file will be detected more easily.
But because the sort order of the aliases differs from simple alphabetic
sorting ("mpathz" precedes "mpathaa"), a bindings file that contains all
bindings from "a" to "aa" (or more) will appear unsorted.
As an extra check, some of the unit tests deliberately use a different
implementation of add_binding() that does not order the bindings
table.
- Broken lines in the bindings file never make it to the in-memory
representation. An alias that appeard "used" because it occurred in a broken
line will not appear used any more. Warnings about malformed lines will only
be printed while the bindings file is read, not from get_user_friendly_alias().
- The match_line argument of mock_bindings_file() is obsolete.
- lookup_binding() and rlookup_binding() have been removed from
libmultipath. They are now emulated in the unit test code.
- lookup_binding() didn't check for used alias in all cases previously, but it does now.
- prefix != NULL and check_if_taken == false is not supported any more
in lookup_binding().
- allocate_binding() uses a very different sequence of systems calls now, as
it's implemented using update_bindings_file(). In particular, it's now more
difficult to predict the content of the write() call that creates the
bindings file. See comments for __wrap_write().
- some unit tests for get_user_friendly_alias() had to call
mock_bindings_file() twice, because the old implementation would read the
file twice (first rlookup_binding() and then lookup_binding()). This is not
necessary any more.
- The unit tests need a teardown function to clear the bindings table in memory.
- Minor changes are necessary because of changed ordering of the log messages.
Previously, lookup_binding() combined check for an existing entry and the
search for a new ID. The new algorithm does this in two separate steps and
tests for used aliases in between, which causes a change in the order in which
log messages are emitted.
Signed-off-by: Martin Wilck <mwilck@suse.com>
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
libdevmapper will most probably not return a UUID for non-existing maps anyway. But it's cheap to double-check here. Signed-off-by: Martin Wilck <mwilck@suse.com> Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
dm_get_uuid() will return 1 for non-existing maps. Thus we don't need to call dm_map_present() any more in alias_already_taken(). This changes our semantics: previously we'd avoid using an alias for which dm_get_uuid() had failed. Now we treat failure in dm_get_uuid() as indication that the map doesn't exist. This is not dangerous because dm_task_get_uuid() cannot fail, and thus the modified dm_get_uuid() will fail if and only if dm_map_present() would return false. This makes the "failed alias" test mostly obsolete, as "failed" is now treated as "unused". Signed-off-by: Martin Wilck <mwilck@suse.com> Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
The current sort order of aliases is alphabetical, which is does not match
the actual order of aliases, where "mpathaa" > "mpathz". Change the ordering as
follows: first sort by string length, then alphabetically. This will make
sure that for aliases with the same prefix, alias order is correct ("mpathaaa"
will be sorted after "mpathzz", etc). Even for mixed prefixes, the alias
order will be correct for every individual prefix, even though aliases with
different prefixes may alternate in the file.
Signed-off-by: Martin Wilck <mwilck@suse.com>
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
Signed-off-by: Martin Wilck <mwilck@suse.com> Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
If we can assume that the bindings array is totally ordered for every prefix, which the previous patch guarantees, the search for a free ID can be simplified. Signed-off-by: Martin Wilck <mwilck@suse.com> Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
The "unsorted" test fail now, and are removed. The algorithm is now better at finding "gaps". Signed-off-by: Martin Wilck <mwilck@suse.com> Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
As the assignment of free aliases now relies on the bindings being properly sorted, add some unit tests to make sure the sorting algorithm works. Signed-off-by: Martin Wilck <mwilck@suse.com> Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
Since "libmultipath: keep bindings in memory", we don't re-read the bindings file after every modification. Add a notification mechanism that makes multipathd aware of changes to the bindings file. Because multipathd itself will change the bindings file, it must compare timestamps in order to avoid reading the file repeatedly. Because select_alias() can be called from multiple thread contexts (uxlsnr, uevent handler), we need to add locking for the bindings file. The timestamp must also be protected by a lock, because it can't be read or written atomically. Note: The notification mechanism expects the bindings file to be atomically replaced by rename(2). Changes must be made in a temporary file and applied using rename(2), as in update_bindings_file(). The inotify mechanism deliberately does not listen to close-after-write events that would be generated by editing the bindings file directly. This Note also: new bindings will only be read from add_map_with_path(), i.e. either during reconfigure(), or when a new map is created during runtime. Existing maps will not be renamed if the binding file changes, unless the user runs "multipathd reconfigure". This is not a change wrt the previous code, but it should be mentioned anyway. Signed-off-by: Martin Wilck <mwilck@suse.com> Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
If some test fails with a lock held, cmocka doesn't deal well with pthread_cleanup_pop(). Such tests can cause deadlock with the locking primitives in the alias code, because locks don't get properly unlocked. Just mock the lock/unlock functions and generate an error if they weren't paired at the end of the test. Signed-off-by: Martin Wilck <mwilck@suse.com> Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
Make the path to multipath.conf configurable, and use the same prefix
by default for multipath.conf and multipath/conf.d. For "usr-merged"
distributions with immutable /usr, we'll want to have the configuration
under a different prefix. This can be achieved by using e.g.
make prefix=/usr etc_prefix=""
Note that with prefix=/usr, before this patch the code would use
/usr/etc/multipath/conf.d, but /etc/multipath.conf. If this (rather
inconsistent) behavior is desired, use the following command line:
make prefix=/usr configfile=/etc/multipath.conf
Signed-off-by: Martin Wilck <mwilck@suse.com>
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
Instead of hard-conding "/etc/multipath" as the path for the state files "bindings", "prkeys", and "wwids", make this path configurable via the "statedir" compile-time option. The default is currently still /etc, it might change to /var/lib or similar in the future. Signed-off-by: Martin Wilck <mwilck@suse.com> Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
This patch adds the support to handle FPIN-Li for FC-NVMe.
On receiving the FPIN-Li events this patch moves the devices paths
which are affected due to link integrity to marginal path groups.
The paths which are set to marginal path group will be unset
on receiving the RSCN events
(mwilck: minor compile fix for 32-bit architectures)
Signed-off-by: Muneendra <muneendra.kumar@broadcom.com>
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
Reviewed-by: Martin Wilck <mwilck@suse.com>
It was wrongly listed in libmpathutil's version script when it was split off from libmultipath in commit 4e1f20c ("libmultipath: split off libmpathutil"), but was never part of libmpathutil LLD 17 failed to link libmpathutil due to this: ``` ld.lld: error: version script assignment of 'LIBMPATHUTIL_1.0' to symbol 'parse_prkey' failed: symbol not defined ``` Remove the `parse_prkey` symbol to fix this Reviewed-by: Martin Wilck <mwilck@suse.com>
The directio checker logs too much at verbosity 3. Signed-off-by: Martin Wilck <mwilck@suse.com> Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
io_cancel() never succeeds, and even if it did, io_getevents() must still be called to reclaim the IO resources from the kernel. Don't pretend the opposite by resetting ct->running, or freeing the memory, before io_getevents() has indicated that the request is finished. In the test code, don't bother about the return value of __wrap_io_cancel(). Signed-off-by: Martin Wilck <mwilck@suse.com> Reviewed-by: Hannes Reinecke <hare@suse.de> Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
libaio uses a different error return convention than glibc. The error code is not returned in errno, but as the negated return value of the function. Adapt the directio checker code. Signed-off-by: Martin Wilck <mwilck@suse.com> Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
It is wrong to assume that aio data structures can be reused or freed after io_cancel(). io_cancel() will almost always return -EINPROGRESS, anyway. Use the io_starttime field to indicate whether an io event has been completed by the kernel. Make sure no in-flight buffers are freed. Fixes opensvc#73. Signed-off-by: Martin Wilck <mwilck@suse.com> Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com> Cc: Li Xiao Keng <lixiaokeng@huawei.com> Cc: Miao Guanqin <miaoguanqin@huawei.com> Cc: Guan Junxiong <guanjunxiong@huawei.com>
…ec() Memory used by aio iocbs must not be freed before either all iocbs have finished, or io_destroy() has been called (which will wait until all iocbs have finished). Don't call cancel_inflight_io() from free_io_err_stat_path(). Rather, cancel directly from free_io_err_pathvec(). This way we make sure that we can't call io_cancel() with an already destroyed ioctx, and that we don't free memory of in-flight requests. The other callers of free_io_err_stat_path() also don't need to call cancel_inflight_io(). In service_paths(), where free_io_err_stat_path() it is called for paths on which all IO has either finished or timed out, hanging requests will already have been cancelled in the try_to_cancel_timeout_io() code path (note that total_time is at least 2 * IO_TIMEOUT_SEC). In the failure case of enqueue_io_err_stat_by_path(), no IO has been submitted yet. Signed-off-by: Martin Wilck <mwilck@suse.com> Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com> Cc: Li Xiao Keng <lixiaokeng@huawei.com> Cc: Miao Guanqin <miaoguanqin@huawei.com> Cc: Guan Junxiong <guanjunxiong@huawei.com>
Currently the number of iocbs per path to test is the same as the total number of iocbs in the ioctx. This can easily cause iocb starvation, in particular if some IOs are hanging. In that case io_submit() will fail, and some paths under test will use much less IOs as intended, or in the worst case, none at all. The total number of iocbs reserved in the kernel should be higher. With this patch, we will be able to run the marginal path test for at least NR_IOSTAT_PATHS=32 paths at the same time. This is not an upper limit, because kernel IOCBs can be reused between paths. Increase the log levels of io_setup and io_submit to make it sure we catch problems with this approach. Signed-off-by: Martin Wilck <mwilck@suse.com> Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com> Cc: Li Xiao Keng <lixiaokeng@huawei.com> Cc: Miao Guanqin <miaoguanqin@huawei.com> Cc: Guan Junxiong <guanjunxiong@huawei.com>
libaio uses a different error return convention than glibc. The error code is not returned in errno, but as the negated return value of the function. Adapt the error handling code in io_err_stat.c. Don't print an error message for failure of io_cancel(), which always returns -EINPRPOGRESS. Signed-off-by: Martin Wilck <mwilck@suse.com> Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
systemd v245 and newer provides the modprobe@.service unit, which can be used to pull in modules via systemd. This is cleaner than loading dm-multipath unconditionally via modules-load.d. Use it if the detected systemd version support it. Signed-off-by: Martin Wilck <mwilck@suse.com> Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
Since c203929 ("multipathd.service: add dependency on systemd-udevd-kernel.socket"), multipathd will start early during boot. Moreover, we recommend using socket activation for multipathd, and if multipathd.socket is enabled, the __mpath_connect() check will succeed anyway. The systemd_service_enabled() test was just "good enough" for standard situations but never robust; it checked for multipathd.wants in "some" directory, which might or might not be the current target, and it would return true even of multipathd.service was masked. Remove this test. Signed-off-by: Martin Wilck <mwilck@suse.com> Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
Signed-off-by: Martin Wilck <mwilck@suse.com> Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
systemd with "split-usr=true" uses rootprefixdir as prefix for
units and for udev libraries and rules ("udevlibexecdir" in
systemd). Fix the documentation for this case. Also, slightly
improve the paragraph about SCSI module loading.
Signed-off-by: Martin Wilck <mwilck@suse.com>
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
Cc: Xose Vazquez Perez <xose.vazquez@gmail.com>
Create a "Contributing" section. Move the the current sections related to contributing into that section. Signed-off-by: Martin Wilck <mwilck@suse.com> Acked-by: Benjamin Marzinski <bmarzins@redhat.com> Cc: Xose Vazquez Perez <xose.vazquez@gmail.com>
Signed-off-by: Martin Wilck <mwilck@suse.com> Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com> Cc: Xose Vazquez Perez <xose.vazquez@gmail.com>
Signed-off-by: Martin Wilck <mwilck@suse.com> Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com> Cc: Xose Vazquez Perez <xose.vazquez@gmail.com>
This option lets multipath set a scsi disk's max_retries sysfs value. Setting this can be helpful for cases where the path checker succeeds, but IO commands hang and timeout. By default, the SCSI layer will retry IOs 5 times. Reducing this value will allow multipath to retry the IO down another path sooner. Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com> Reviewed-by: Martin Wilck <mwilck@suse.com>
When paths are allocated their size is initialized to 0. If they've already set a size, and a future call to sysfs_get_size() fails during the parsing, assume that the size hasn't changed, instead of setting it to 0. All other failures in sysfs_get_size() already retain the existing size. Reviewed-by: Martin Wilck <mwilck@suse.com> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
When resizing a multipath device, make sure that all the paths have been updated to the new size first. Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com> Reviewed-by: Martin Wilck <mwilck@suse.com>
In preparation for reusing resize_map() in other code, move all code necessary to resize the map to the resize_map() function. Also track if map was removed in the function. Reviewed-by: Martin Wilck <mwilck@suse.com> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
No functional changes. Reviewed-by: Martin Wilck <mwilck@suse.com> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
This option gives multipathd the ability to automatically resize a device when it detects that all of the path devices have changed. By default it is set to never, and multipathd will continue to work like it always has, where a users must manually resize a multipath device. Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com> Reviewed-by: Martin Wilck <mwilck@suse.com>
Commits ("libmultipath: Add max_retries config option") and
("multipathd: Add auto_resize config option") changed the size and
member offsets of multiple structs.
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
Reviewed-by: Martin Wilck <mwilck@suse.com>
In the default configuration (both prefix and usr_prefix unset), we'd install man pages and headers under /share/man and /include, respectively, which is very unusual. Have usr_prefix default to /usr in (the default) case where prefix is empty, and set it equal to /prefix otherwise. Signed-off-by: Martin Wilck <mwilck@suse.com> Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
bmarzins
approved these changes
Nov 20, 2023
arnout
pushed a commit
to buildroot/buildroot
that referenced
this pull request
Nov 27, 2023
Change log: - opensvc/multipath-tools#77 Signed-off-by: Alexander Egorenkov <egorenar-dev@posteo.net> Signed-off-by: Yann E. MORIN <yann.morin.1998@free.fr>
Hancock33
pushed a commit
to Hancock33/buildroot
that referenced
this pull request
Nov 30, 2023
Change log: - opensvc/multipath-tools#77 Signed-off-by: Alexander Egorenkov <egorenar-dev@posteo.net> Signed-off-by: Yann E. MORIN <yann.morin.1998@free.fr>
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.
Summary: Important changes in 0.9.7
bindings_file,wwids_fileandprkeys_file, which were deprecated since 0.8.8, have been removed. The path to these files is now hard-coded to$(statedir)(see below).max_retriesconfig option to limit SCSI retriesauto_resizeconfig option to enable resizing multipath maps automaticallymultipath -dchanges sysfs settingsstatedirwhich defaults to/etc/multipathetc_prefixas prefix for config file and config dirusr_prefixnow defaults to/usrifprefixis empty..wantsdirectoriesShortlog
@bmarzins (7):
@mwilck (54):
@marv (1):
(fixes #74)
@muneendramandala (1):
@xosevp (1):