[PATCH v2 0/3] Ignore path devices in multipath setups

[PATCH v2 0/3] Ignore path devices in multipath setups

[Nikolay Borisov]

This patch set improves the scanning behavior of btrfs-progs such that devices
which represent paths to a multipath device will be ignored when scanning. There
was an internal report that this causes problems with libstorage-ng. The behavior of
showing path devices diverges from the kernel, where on mounted filesystem only
the actual multipath device will be shown.

Changes in V2:

* Split build system related changes to the first patch of the series.

* Eliminated function names starting with __

* Instead of defining an 'if' always implement sane behavior of is_path_device
based on the state of the systems

* Eliminated special constant and started using PATH_MAX.


Nikolay Borisov (3):
  btrfs-progs: Add optional dependency on libudev
  btrfs-progs: Ignore devices representing paths in multipath
  btrfs-progs: Add fallback code for path device ignore for static build

 Makefile             |  2 +-
 Makefile.inc.in      |  2 +-
 common/device-scan.c | 84 ++++++++++++++++++++++++++++++++++++++++++++
 configure.ac         |  9 +++++
 4 files changed, 95 insertions(+), 2 deletions(-)

--
2.17.1

[PATCH v2 1/3] btrfs-progs: Add optional dependency on libudev

[Nikolay Borisov]

This is needed for future code which will make btrfs-progs' device
scanning logic a little smarter by filtering out path device in
multipath setups. libudev is added as an optional dependency since the
library doesn't have a static version so making it a hard dependency
means forfeiting static build support. To alleviate this a fallback code
will be added for the static build case which doesn't rely on libudev.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 Makefile        | 2 +-
 Makefile.inc.in | 2 +-
 configure.ac    | 9 +++++++++
 3 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/Makefile b/Makefile
index 93fe4c2b3e08..e96f66a36b46 100644
--- a/Makefile
+++ b/Makefile
@@ -129,7 +129,7 @@ LIBS = $(LIBS_BASE) $(LIBS_CRYPTO)
 LIBBTRFS_LIBS = $(LIBS_BASE) $(LIBS_CRYPTO)
 
 # Static compilation flags
-STATIC_CFLAGS = $(CFLAGS) -ffunction-sections -fdata-sections
+STATIC_CFLAGS = $(CFLAGS) -ffunction-sections -fdata-sections -DSTATIC_BUILD
 STATIC_LDFLAGS = -static -Wl,--gc-sections
 STATIC_LIBS = $(STATIC_LIBS_BASE)
 
diff --git a/Makefile.inc.in b/Makefile.inc.in
index 9f49337147b8..c995aef97219 100644
--- a/Makefile.inc.in
+++ b/Makefile.inc.in
@@ -27,7 +27,7 @@ CRYPTO_CFLAGS = @GCRYPT_CFLAGS@ @SODIUM_CFLAGS@ @KCAPI_CFLAGS@
 SUBST_CFLAGS = @CFLAGS@
 SUBST_LDFLAGS = @LDFLAGS@
 
-LIBS_BASE = @UUID_LIBS@ @BLKID_LIBS@ -L. -pthread
+LIBS_BASE = @UUID_LIBS@ @BLKID_LIBS@ @LIBUDEV_LIBS@ -L. -pthread
 LIBS_COMP = @ZLIB_LIBS@ @LZO2_LIBS@ @ZSTD_LIBS@
 LIBS_PYTHON = @PYTHON_LIBS@
 LIBS_CRYPTO = @GCRYPT_LIBS@ @SODIUM_LIBS@ @KCAPI_LIBS@
diff --git a/configure.ac b/configure.ac
index 038c2688421c..d0ceb0d70d16 100644
--- a/configure.ac
+++ b/configure.ac
@@ -304,6 +304,15 @@ PKG_STATIC(UUID_LIBS_STATIC, [uuid])
 PKG_CHECK_MODULES(ZLIB, [zlib])
 PKG_STATIC(ZLIB_LIBS_STATIC, [zlib])
 
+PKG_CHECK_EXISTS([libudev], [pkg_config_libudev=yes], [pkg_config_libudev=no])
+if test "x$pkg_config_libudev" = xyes; then
+	PKG_CHECK_MODULES([LIBUDEV], [libudev])
+	AC_DEFINE([HAVE_LIBUDEV], [1], [Define to 1 if libudev is available])
+else
+	AC_MSG_CHECKING([for LIBUDEV])
+	AC_MSG_RESULT([no])
+fi
+
 AC_ARG_ENABLE([zstd],
 	AS_HELP_STRING([--disable-zstd], [build without zstd support]),
 	[], [enable_zstd=yes]
-- 
2.17.1

[PATCH v2 2/3] btrfs-progs: Ignore devices representing paths in multipath

[Nikolay Borisov]

Currently btrfs-progs will happily enumerate any device which has a
btrfs filesystem on it irrespective of its type. For the majority of
use cases that's fine and there haven't been any problems with that.
However, there was a recent report that in multipath scenario when
running "btrfs fi show" after a path flap (path going down and then
coming back up) instead of the multipath device being show the device
which represents the flapped path is shown. So a multipath filesystem
might look like:

Label: none  uuid: d3c1261f-18be-4015-9fef-6b35759dfdba
        Total devices 1 FS bytes used 192.00KiB
        devid    1 size 10.00GiB used 536.00MiB path /dev/mapper/3600140501cc1f49e5364f0093869c763

/dev/mapper/xxx is actually backed by an arbitrary number of paths,
which in turn are presented to the system as ordinary scsi devices i.e
/dev/sdX. If a path flaps and a user re-runs 'btrfs fi show' the output
would look like:

Label: none  uuid: d3c1261f-18be-4015-9fef-6b35759dfdba
        Total devices 1 FS bytes used 192.00KiB
        devid    1 size 10.00GiB used 536.00MiB path /dev/sdd

This only occurs on unmounted filesystems as those are enumerated by
btrfs-progs, for mounted filesystem the kernel properly deals only with
the actual multipath device.

Turns out the output of this command is consumed by libraries and the
presence of a path device rather than the actual multipath causes
issues.

Fix this by checking for the presence of DM_MULTIPATH_DEVICE_PATH
udev attribute as multipath path devices are tagged with this attribute
by the multipath udev scripts.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---

David,

I haven't renamed is_path_device to is_multipath_device as per our chat offline,
in my mind multipath is the actual /dev/mapper/XXXXX device which precisely the
device we want scanned and individual paths are what I call path devices (don't
know if this is official nomenclature) and is what we want to ignore.

 common/device-scan.c | 47 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 47 insertions(+)

diff --git a/common/device-scan.c b/common/device-scan.c
index b5bfe844104b..a1fd9f38b9df 100644
--- a/common/device-scan.c
+++ b/common/device-scan.c
@@ -14,6 +14,10 @@
  * Boston, MA 021110-1307, USA.
  */

+#ifdef STATIC_BUILD
+#undef HAVE_LIBUDEV
+#endif
+
 #include "kerncompat.h"
 #include <sys/ioctl.h>
 #include <stdlib.h>
@@ -25,6 +29,10 @@
 #include <dirent.h>
 #include <blkid/blkid.h>
 #include <uuid/uuid.h>
+#ifdef HAVE_LIBUDEV
+#include <sys/stat.h>
+#include <libudev.h>
+#endif
 #include "kernel-lib/overflow.h"
 #include "common/path-utils.h"
 #include "common/device-scan.h"
@@ -364,6 +372,42 @@ void free_seen_fsid(struct seen_fsid *seen_fsid_hash[])
 	}
 }

+#ifdef HAVE_LIBUDEV
+static bool is_path_device(char *device_path)
+{
+	struct udev *udev = NULL;
+	struct udev_device *dev = NULL;
+	struct stat dev_stat;
+	const char *val;
+	bool ret = false;
+
+	if (stat(device_path, &dev_stat) < 0)
+		return false;
+
+	udev = udev_new();
+	if (!udev)
+		goto out;
+
+	dev = udev_device_new_from_devnum(udev, 'b', dev_stat.st_rdev);
+	if (!dev)
+		goto out;
+
+	val = udev_device_get_property_value(dev, "DM_MULTIPATH_DEVICE_PATH");
+	if (val && atoi(val) > 0)
+		ret = true;
+out:
+	udev_device_unref(dev);
+	udev_unref(udev);
+
+	return ret;
+}
+#else
+static bool is_path_device(char *device_path)
+{
+	return false;
+}
+#endif
+
 int btrfs_scan_devices(int verbose)
 {
 	int fd = -1;
@@ -394,6 +438,9 @@ int btrfs_scan_devices(int verbose)
 		/* if we are here its definitely a btrfs disk*/
 		strncpy_null(path, blkid_dev_devname(dev));

+		if (is_path_device(path))
+			continue;
+
 		fd = open(path, O_RDONLY);
 		if (fd < 0) {
 			error("cannot open %s: %m", path);
--
2.17.1

[PATCH v2 3/3] btrfs-progs: Add fallback code for path device ignore for static build

[Nikolay Borisov]

Since libudev doesn't provide a static version of the library for static
build btrfs-progs will have to provide manual fallback. This change does
this by parsing the udev database files hosted at /run/udev/data/.
Under that directory every block device should have a file with the
following name: bMAJ:MIN. So implement the bare minimum code necessary
to parse this file and search for the presence of DM_MULTIPATH_DEVICE_PATH
udev attribute. This could likely be racy since access to the udev
database is done outside of libudev but that's the best that can be
done when implementing this manually.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 common/device-scan.c | 55 ++++++++++++++++++++++++++++++++++++--------
 1 file changed, 46 insertions(+), 9 deletions(-)

diff --git a/common/device-scan.c b/common/device-scan.c
index a1fd9f38b9df..bcfd36fade2c 100644
--- a/common/device-scan.c
+++ b/common/device-scan.c
@@ -29,6 +29,7 @@
 #include <dirent.h>
 #include <blkid/blkid.h>
 #include <uuid/uuid.h>
+#include <sys/sysmacros.h>
 #ifdef HAVE_LIBUDEV
 #include <sys/stat.h>
 #include <libudev.h>
@@ -372,23 +373,54 @@ void free_seen_fsid(struct seen_fsid *seen_fsid_hash[])
 	}
 }
 
-#ifdef HAVE_LIBUDEV
-static bool is_path_device(char *device_path)
+#ifdef STATIC_BUILD
+static bool is_path_device(dev_t device)
+{
+	FILE *file;
+	char *line = NULL;
+	size_t len = 0;
+	ssize_t nread;
+	bool ret = false;
+	int ret2;
+	char path[PATH_MAX];
+
+	ret2 = snprintf(path, 100, "/run/udev/data/b%u:%u", major(device),
+			minor(device));
+
+	if (ret2 < 0)
+		return false;
+
+	file = fopen(path, "r");
+	if (file == NULL)
+		return false;
+
+	while ((nread = getline(&line, &len, file)) != -1) {
+		if (strstr(line, "DM_MULTIPATH_DEVICE_PATH=1")) {
+			ret = true;
+			break;
+		}
+	}
+
+	if (line)
+		free(line);
+
+	fclose(file);
+
+	return ret;
+}
+#elif defined(HAVE_LIBUDEV)
+static bool is_path_device(dev_t device)
 {
 	struct udev *udev = NULL;
 	struct udev_device *dev = NULL;
-	struct stat dev_stat;
 	const char *val;
 	bool ret = false;
 
-	if (stat(device_path, &dev_stat) < 0)
-		return false;
-
 	udev = udev_new();
 	if (!udev)
 		goto out;
 
-	dev = udev_device_new_from_devnum(udev, 'b', dev_stat.st_rdev);
+	dev = udev_device_new_from_devnum(udev, 'b', device);
 	if (!dev)
 		goto out;
 
@@ -402,7 +434,7 @@ static bool is_path_device(char *device_path)
 	return ret;
 }
 #else
-static bool is_path_device(char *device_path)
+static bool is_path_device(dev_t device)
 {
 	return false;
 }
@@ -432,13 +464,18 @@ int btrfs_scan_devices(int verbose)
 	iter = blkid_dev_iterate_begin(cache);
 	blkid_dev_set_search(iter, "TYPE", "btrfs");
 	while (blkid_dev_next(iter, &dev) == 0) {
+		struct stat dev_stat;
+
 		dev = blkid_verify(cache, dev);
 		if (!dev)
 			continue;
 		/* if we are here its definitely a btrfs disk*/
 		strncpy_null(path, blkid_dev_devname(dev));
 
-		if (is_path_device(path))
+		if (stat(path, &dev_stat) < 0)
+			continue;
+
+		if (is_path_device(dev_stat.st_rdev))
 			continue;
 
 		fd = open(path, O_RDONLY);
-- 
2.17.1

Re: [PATCH v2 0/3] Ignore path devices in multipath setups

[David Sterba]

On Thu, Sep 30, 2021 at 03:06:31PM +0300, Nikolay Borisov wrote:
> This patch set improves the scanning behavior of btrfs-progs such that devices
> which represent paths to a multipath device will be ignored when scanning. There
> was an internal report that this causes problems with libstorage-ng. The behavior of
> showing path devices diverges from the kernel, where on mounted filesystem only
> the actual multipath device will be shown.
> 
> Changes in V2:
> 
> * Split build system related changes to the first patch of the series.
> 
> * Eliminated function names starting with __
> 
> * Instead of defining an 'if' always implement sane behavior of is_path_device
> based on the state of the systems
> 
> * Eliminated special constant and started using PATH_MAX.
> 
> 
> Nikolay Borisov (3):
>   btrfs-progs: Add optional dependency on libudev
>   btrfs-progs: Ignore devices representing paths in multipath
>   btrfs-progs: Add fallback code for path device ignore for static build

Added to devel, thanks. I slightly changed how the libudev dependency is
done, required by default with possible --diable-libudev. Though it's
introducing a dependency in a minor release, it's required to have the
multipath support and that I belive is case for most distros anyway.