[dm-devel] [PATCH] libmultipath: ignore nvme devices if nvme native multipath is enabled

[Benjamin Marzinski]

On Fri, Jun 30, 2023 at 08:14:07PM +0200, mwilck at suse.com wrote:
> From: Martin Wilck <mwilck at 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 at 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