| Message ID | 20230630181407.14302-1-mwilck@suse.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | christophe varoqui |
| Headers | show |
| Series | libmultipath: ignore nvme devices if nvme native multipath is enabled | expand |
On Fri, Jun 30, 2023 at 08:14:07PM +0200, mwilck@suse.com wrote: > From: Martin Wilck <mwilck@suse.com> > > If the nvme native multipath driver is enabled, blacklist nvme devices > for dm-multipath by default. This is particularly useful with > "find_multipaths greedy". > > Signed-off-by: Martin Wilck <mwilck@suse.com> This looks good. I have two small questions below. > --- > libmultipath/blacklist.c | 35 ++++++++++++++++++++++++++++++++--- > tests/blacklist.c | 30 ++++++++++++++++++++++++++++-- > 2 files changed, 60 insertions(+), 5 deletions(-) > > diff --git a/libmultipath/blacklist.c b/libmultipath/blacklist.c > index 8d15d2e..75100b2 100644 > --- a/libmultipath/blacklist.c > +++ b/libmultipath/blacklist.c > @@ -2,6 +2,8 @@ > * Copyright (c) 2004, 2005 Christophe Varoqui > */ > #include <stdio.h> > +#include <unistd.h> > +#include <fcntl.h> > #include <libudev.h> > > #include "checkers.h" > @@ -191,6 +193,27 @@ find_blacklist_device (const struct _vector *blist, const char *vendor, > return 0; > } > > +/* > + * Test if nvme native multipath is enabled. If the sysfs file can't > + * be accessed, multipath is either disabled at compile time, or no > + * nvme driver is loaded at all. Thus treat errors as "no". > + */ > +static bool nvme_multipath_enabled(void) > +{ > + static const char fn[] = "/sys/module/nvme_core/parameters/multipath"; Is there some benefit that I don't understand to using "static const", instead of just "const"? Obviously, the amount of memory necessary to keep storing this string outside of the function is very small, but I don't see why we need to at all. > + int fd, len; > + char buf[2]; > + > + fd = open(fn, O_RDONLY); > + if (fd == -1) > + return false; > + > + len = read(fd, buf, sizeof(buf)); > + close(fd); > + > + return (len >= 1 && buf[0] == 'Y'); > +} > + > int > setup_default_blist (struct config * conf) > { > @@ -198,9 +221,15 @@ setup_default_blist (struct config * conf) > struct hwentry *hwe; > int i; > > - if (store_ble(conf->blist_devnode, "!^(sd[a-z]|dasd[a-z]|nvme[0-9])", ORIGIN_DEFAULT)) > - return 1; > - > + if (nvme_multipath_enabled()) { > + if (store_ble(conf->blist_devnode, "!^(sd[a-z]|dasd[a-z])", > + ORIGIN_DEFAULT)) > + return 1; > + } else { > + if (store_ble(conf->blist_devnode, "!^(sd[a-z]|dasd[a-z]|nvme[0-9])", > + ORIGIN_DEFAULT)) > + return 1; > + } > if (store_ble(conf->elist_property, "(SCSI_IDENT_|ID_WWN)", ORIGIN_DEFAULT)) > return 1; > > diff --git a/tests/blacklist.c b/tests/blacklist.c > index 882aa3a..65002eb 100644 > --- a/tests/blacklist.c > +++ b/tests/blacklist.c > @@ -17,6 +17,8 @@ > */ > #include <stdarg.h> > #include <stddef.h> > +#include <unistd.h> > +#include <fcntl.h> > #include <setjmp.h> > #include <cmocka.h> > #include "globals.c" > @@ -220,12 +222,36 @@ static void test_devnode_missing(void **state) > MATCH_NOTHING); > } Perhaps we should just use #include "../libmultipath/blacklist.c" like we do for tests where we need get at a file's private variables/functions ("../libmultipath/alias.c" in alias.c for instance). -Ben > +/* copy of the same function in libmultipath/blacklist.c */ > +static bool nvme_multipath_enabled(void) > +{ > + static const char fn[] = "/sys/module/nvme_core/parameters/multipath"; > + int fd, len; > + char buf[2]; > + > + fd = open(fn, O_RDONLY); > + if (fd == -1) > + return false; > + > + len = read(fd, buf, sizeof(buf)); > + close(fd); > + > + return (len >= 1 && buf[0] == 'Y'); > +} > + > static void test_devnode_default(void **state) > { > assert_int_equal(filter_devnode(blist_devnode_default, NULL, "sdaa"), > MATCH_NOTHING); > - assert_int_equal(filter_devnode(blist_devnode_default, NULL, "nvme0n1"), > - MATCH_NOTHING); > + if (nvme_multipath_enabled()) { > + expect_condlog(3, "nvme0n1: device node name blacklisted\n"); > + assert_int_equal(filter_devnode(blist_devnode_default, NULL, > + "nvme0n1"), > + MATCH_DEVNODE_BLIST); > + } else > + assert_int_equal(filter_devnode(blist_devnode_default, NULL, > + "nvme0n1"), > + MATCH_NOTHING); > assert_int_equal(filter_devnode(blist_devnode_default, NULL, "dasda"), > MATCH_NOTHING); > expect_condlog(3, "hda: device node name blacklisted\n"); > -- > 2.41.0 -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
On Fri, 2023-06-30 at 17:16 -0500, Benjamin Marzinski wrote: > On Fri, Jun 30, 2023 at 08:14:07PM +0200, mwilck@suse.com wrote: > > From: Martin Wilck <mwilck@suse.com> > > > > If the nvme native multipath driver is enabled, blacklist nvme > > devices > > for dm-multipath by default. This is particularly useful with > > "find_multipaths greedy". > > > > Signed-off-by: Martin Wilck <mwilck@suse.com> > > This looks good. I have two small questions below. > > > --- > > libmultipath/blacklist.c | 35 ++++++++++++++++++++++++++++++++--- > > tests/blacklist.c | 30 ++++++++++++++++++++++++++++-- > > 2 files changed, 60 insertions(+), 5 deletions(-) > > > > diff --git a/libmultipath/blacklist.c b/libmultipath/blacklist.c > > index 8d15d2e..75100b2 100644 > > --- a/libmultipath/blacklist.c > > +++ b/libmultipath/blacklist.c > > @@ -2,6 +2,8 @@ > > * Copyright (c) 2004, 2005 Christophe Varoqui > > */ > > #include <stdio.h> > > +#include <unistd.h> > > +#include <fcntl.h> > > #include <libudev.h> > > > > #include "checkers.h" > > @@ -191,6 +193,27 @@ find_blacklist_device (const struct _vector > > *blist, const char *vendor, > > return 0; > > } > > > > +/* > > + * Test if nvme native multipath is enabled. If the sysfs file > > can't > > + * be accessed, multipath is either disabled at compile time, or > > no > > + * nvme driver is loaded at all. Thus treat errors as "no". > > + */ > > +static bool nvme_multipath_enabled(void) > > +{ > > + static const char fn[] = > > "/sys/module/nvme_core/parameters/multipath"; > > Is there some benefit that I don't understand to using "static > const", > instead of just "const"? Obviously, the amount of memory necessary > to > keep storing this string outside of the function is very small, but I > don't see why we need to at all. "static const" and "const" aren't the same thing. "static const" makes sure the variable's life time is the run time of the program, hence the compiler can place it in the .rodata section of the output. A "const" variable in function scope without "static" has "automatic" storage class, and is thus allocated and initialized on the stack. https://www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf (§6.2.4 storage durations of objects) https://stackoverflow.com/questions/3709207/c-semantics-of-static-const-vs-const You can see this if you compile a program like this int check(const char *); int fn(void) { /* static */ const char t[] = "0123456789abcdef"; return check(t); } with and without "static". > > diff --git a/tests/blacklist.c b/tests/blacklist.c > > index 882aa3a..65002eb 100644 > > --- a/tests/blacklist.c > > +++ b/tests/blacklist.c > > @@ -17,6 +17,8 @@ > > */ > > #include <stdarg.h> > > #include <stddef.h> > > +#include <unistd.h> > > +#include <fcntl.h> > > #include <setjmp.h> > > #include <cmocka.h> > > #include "globals.c" > > @@ -220,12 +222,36 @@ static void test_devnode_missing(void > > **state) > > MATCH_NOTHING); > > } > > Perhaps we should just use #include "../libmultipath/blacklist.c" > like > we do for tests where we need get at a file's private > variables/functions ("../libmultipath/alias.c" in alias.c for > instance). Ok. Regards Martin -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
diff --git a/libmultipath/blacklist.c b/libmultipath/blacklist.c index 8d15d2e..75100b2 100644 --- a/libmultipath/blacklist.c +++ b/libmultipath/blacklist.c @@ -2,6 +2,8 @@ * Copyright (c) 2004, 2005 Christophe Varoqui */ #include <stdio.h> +#include <unistd.h> +#include <fcntl.h> #include <libudev.h> #include "checkers.h" @@ -191,6 +193,27 @@ find_blacklist_device (const struct _vector *blist, const char *vendor, return 0; } +/* + * Test if nvme native multipath is enabled. If the sysfs file can't + * be accessed, multipath is either disabled at compile time, or no + * nvme driver is loaded at all. Thus treat errors as "no". + */ +static bool nvme_multipath_enabled(void) +{ + static const char fn[] = "/sys/module/nvme_core/parameters/multipath"; + int fd, len; + char buf[2]; + + fd = open(fn, O_RDONLY); + if (fd == -1) + return false; + + len = read(fd, buf, sizeof(buf)); + close(fd); + + return (len >= 1 && buf[0] == 'Y'); +} + int setup_default_blist (struct config * conf) { @@ -198,9 +221,15 @@ setup_default_blist (struct config * conf) struct hwentry *hwe; int i; - if (store_ble(conf->blist_devnode, "!^(sd[a-z]|dasd[a-z]|nvme[0-9])", ORIGIN_DEFAULT)) - return 1; - + if (nvme_multipath_enabled()) { + if (store_ble(conf->blist_devnode, "!^(sd[a-z]|dasd[a-z])", + ORIGIN_DEFAULT)) + return 1; + } else { + if (store_ble(conf->blist_devnode, "!^(sd[a-z]|dasd[a-z]|nvme[0-9])", + ORIGIN_DEFAULT)) + return 1; + } if (store_ble(conf->elist_property, "(SCSI_IDENT_|ID_WWN)", ORIGIN_DEFAULT)) return 1; diff --git a/tests/blacklist.c b/tests/blacklist.c index 882aa3a..65002eb 100644 --- a/tests/blacklist.c +++ b/tests/blacklist.c @@ -17,6 +17,8 @@ */ #include <stdarg.h> #include <stddef.h> +#include <unistd.h> +#include <fcntl.h> #include <setjmp.h> #include <cmocka.h> #include "globals.c" @@ -220,12 +222,36 @@ static void test_devnode_missing(void **state) MATCH_NOTHING); } +/* copy of the same function in libmultipath/blacklist.c */ +static bool nvme_multipath_enabled(void) +{ + static const char fn[] = "/sys/module/nvme_core/parameters/multipath"; + int fd, len; + char buf[2]; + + fd = open(fn, O_RDONLY); + if (fd == -1) + return false; + + len = read(fd, buf, sizeof(buf)); + close(fd); + + return (len >= 1 && buf[0] == 'Y'); +} + static void test_devnode_default(void **state) { assert_int_equal(filter_devnode(blist_devnode_default, NULL, "sdaa"), MATCH_NOTHING); - assert_int_equal(filter_devnode(blist_devnode_default, NULL, "nvme0n1"), - MATCH_NOTHING); + if (nvme_multipath_enabled()) { + expect_condlog(3, "nvme0n1: device node name blacklisted\n"); + assert_int_equal(filter_devnode(blist_devnode_default, NULL, + "nvme0n1"), + MATCH_DEVNODE_BLIST); + } else + assert_int_equal(filter_devnode(blist_devnode_default, NULL, + "nvme0n1"), + MATCH_NOTHING); assert_int_equal(filter_devnode(blist_devnode_default, NULL, "dasda"), MATCH_NOTHING); expect_condlog(3, "hda: device node name blacklisted\n");