[PATCH v2] multipath -u: test socket connection in non-blocking mode

[PATCH v2] multipath -u: test socket connection in non-blocking mode

[Martin Wilck]

Since commit d7188fcd "multipathd: start daemon after udev trigger",
multipathd startup is delayed during boot until after "udev settle"
terminates. But "multipath -u" is run by udev workers for storage devices,
and attempts to connect to the multipathd socket. This causes a start job
for multipathd to be scheduled by systemd, but that job won't be started
until "udev settle" finishes. This is not a problem on systems with 129 or
less storage units, because the connect() call of "multipath -u" will
succeed anyway. But on larger systems, the listen backlog of the systemd
socket can be exceeded, which causes connect() calls for the socket to
block until multipathd starts up and begins calling accept(). This creates
a deadlock situation, because "multipath -u" (called by udev workers)
blocks, and thus "udev settle" doesn't finish, delaying multipathd
startup. This situation then persists until either the workers or "udev
settle" time out. In the former case, path devices might be misclassified
as non-multipath devices by "multipath -u".

Fix this by using a non-blocking socket fd for connect() and interpret the
errno appropriately.

This patch reverts most of the changes from commit 8cdf6661 "multipath:
check on multipathd without starting it". Instead, "multipath -u" does
access the socket and start multipath again (which is what we want IMO),
but it is now able to detect and handle the "full backlog" situation.

Signed-off-by: Martin Wilck <mwilck@suse.com>

V2:

Use same error reporting convention in __mpath_connect() as in
mpath_connect() (Hannes Reinecke). We can't easily change the latter,
because it's part of the "public" libmpathcmd API. 

---
 libmpathcmd/mpath_cmd.c | 24 +++++++++++++-
 libmpathcmd/mpath_cmd.h | 15 +++++++++
 multipath/main.c        | 70 +++++++++++++----------------------------
 3 files changed, 60 insertions(+), 49 deletions(-)

diff --git a/libmpathcmd/mpath_cmd.c b/libmpathcmd/mpath_cmd.c
index df4ca541..b681311b 100644
--- a/libmpathcmd/mpath_cmd.c
+++ b/libmpathcmd/mpath_cmd.c
@@ -26,6 +26,7 @@
 #include <poll.h>
 #include <string.h>
 #include <errno.h>
+#include <fcntl.h>
 
 #include "mpath_cmd.h"
 
@@ -93,10 +94,11 @@ static size_t write_all(int fd, const void *buf, size_t len)
 /*
  * connect to a unix domain socket
  */
-int mpath_connect(void)
+int __mpath_connect(int nonblocking)
 {
 	int fd, len;
 	struct sockaddr_un addr;
+	int flags = 0;
 
 	memset(&addr, 0, sizeof(addr));
 	addr.sun_family = AF_LOCAL;
@@ -108,14 +110,34 @@ int mpath_connect(void)
 	if (fd == -1)
 		return -1;
 
+	if (nonblocking) {
+		flags = fcntl(fd, F_GETFL, 0);
+		if (flags != -1)
+			(void)fcntl(fd, F_SETFL, flags|O_NONBLOCK);
+	}
+
 	if (connect(fd, (struct sockaddr *)&addr, len) == -1) {
+		int err = errno;
+
 		close(fd);
+		errno = err;
 		return -1;
 	}
 
+	if (nonblocking && flags != -1)
+		(void)fcntl(fd, F_SETFL, flags);
+
 	return fd;
 }
 
+/*
+ * connect to a unix domain socket
+ */
+int mpath_connect(void)
+{
+	return __mpath_connect(0);
+}
+
 int mpath_disconnect(int fd)
 {
 	return close(fd);
diff --git a/libmpathcmd/mpath_cmd.h b/libmpathcmd/mpath_cmd.h
index 15aeb067..ccfd35f2 100644
--- a/libmpathcmd/mpath_cmd.h
+++ b/libmpathcmd/mpath_cmd.h
@@ -34,6 +34,21 @@ extern "C" {
 #define DEFAULT_REPLY_TIMEOUT	4000
 
 
+/*
+ * DESCRIPTION:
+ *	Same as mpath_connect() (see below) except for the "nonblocking"
+ *	parameter.
+ *	If "nonblocking" is set, connects in non-blocking mode. This is
+ *	useful to avoid blocking if the listening socket's backlog is
+ *	exceeded. In this case, errno will be set to EAGAIN.
+ *	In case of success, the returned file descriptor is in in blocking
+ *	mode, even if "nonblocking" was true.
+ *
+ * RETURNS:
+ *	A file descriptor on success. -1 on failure (with errno set).
+ */
+int __mpath_connect(int nonblocking);
+
 /*
  * DESCRIPTION:
  *	Connect to the running multipathd daemon. On systems with the
diff --git a/multipath/main.c b/multipath/main.c
index 008e3d3f..7933b07f 100644
--- a/multipath/main.c
+++ b/multipath/main.c
@@ -850,55 +850,29 @@ out:
 	return r;
 }
 
-int is_multipathd_running(void)
+static int test_multipathd_socket(void)
 {
-	FILE *f = NULL;
-	char buf[16];
-	char path[PATH_MAX];
-	int pid;
-	char *end;
+	int fd;
+	/*
+	 * "multipath -u" may be run before the daemon is started. In this
+	 * case, systemd might own the socket but might delay multipathd
+	 * startup until some other unit (udev settle!)  has finished
+	 * starting. With many LUNs, the listen backlog may be exceeded, which
+	 * would cause connect() to block. This causes udev workers calling
+	 * "multipath -u" to hang, and thus creates a deadlock, until "udev
+	 * settle" times out.  To avoid this, call connect() in non-blocking
+	 * mode here, and take EAGAIN as indication for a filled-up systemd
+	 * backlog.
+	 */
 
-	f = fopen(DEFAULT_PIDFILE, "r");
-	if (!f) {
-		if (errno != ENOENT)
-			condlog(4, "can't open " DEFAULT_PIDFILE ": %s",
-				strerror(errno));
-		return 0;
-	}
-	if (!fgets(buf, sizeof(buf), f)) {
-		if (ferror(f))
-			condlog(4, "read of " DEFAULT_PIDFILE " failed: %s",
-				strerror(errno));
-		fclose(f);
-		return 0;
-	}
-	fclose(f);
-	errno = 0;
-	strchop(buf);
-	pid = strtol(buf, &end, 10);
-	if (errno != 0 || pid <= 0 || *end != '\0') {
-		condlog(4, "invalid contents in " DEFAULT_PIDFILE ": '%s'",
-			buf);
-		return 0;
-	}
-	snprintf(path, sizeof(path), "/proc/%d/comm", pid);
-	f = fopen(path, "r");
-	if (!f) {
-		if (errno != ENOENT)
-			condlog(4, "can't open %s: %s", path, strerror(errno));
-		return 0;
-	}
-	if (!fgets(buf, sizeof(buf), f)) {
-		if (ferror(f))
-			condlog(4, "read of %s failed: %s", path,
-				strerror(errno));
-		fclose(f);
-		return 0;
-	}
-	fclose(f);
-	strchop(buf);
-	if (strcmp(buf, "multipathd") != 0)
-		return 0;
+	fd = __mpath_connect(1);
+	if (fd == -1) {
+		if (errno == EAGAIN)
+			condlog(3, "daemon backlog exceeded");
+		else
+			return 0;
+	} else
+		close(fd);
 	return 1;
 }
 
@@ -1080,7 +1054,7 @@ main (int argc, char *argv[])
 	}
 	if (cmd == CMD_VALID_PATH &&
 	    dev_type == DEV_UEVENT) {
-		if (!is_multipathd_running()) {
+		if (!test_multipathd_socket()) {
 			condlog(3, "%s: daemon is not running", dev);
 			if (!systemd_service_enabled(dev)) {
 				r = print_cmd_valid(RTVL_NO, NULL, conf);
-- 
2.21.0

Re: [PATCH v2] multipath -u: test socket connection in non-blocking mode

[Hannes Reinecke]

On 4/24/19 11:07 AM, Martin Wilck wrote:
> Since commit d7188fcd "multipathd: start daemon after udev trigger",
> multipathd startup is delayed during boot until after "udev settle"
> terminates. But "multipath -u" is run by udev workers for storage devices,
> and attempts to connect to the multipathd socket. This causes a start job
> for multipathd to be scheduled by systemd, but that job won't be started
> until "udev settle" finishes. This is not a problem on systems with 129 or
> less storage units, because the connect() call of "multipath -u" will
> succeed anyway. But on larger systems, the listen backlog of the systemd
> socket can be exceeded, which causes connect() calls for the socket to
> block until multipathd starts up and begins calling accept(). This creates
> a deadlock situation, because "multipath -u" (called by udev workers)
> blocks, and thus "udev settle" doesn't finish, delaying multipathd
> startup. This situation then persists until either the workers or "udev
> settle" time out. In the former case, path devices might be misclassified
> as non-multipath devices by "multipath -u".
> 
> Fix this by using a non-blocking socket fd for connect() and interpret the
> errno appropriately.
> 
> This patch reverts most of the changes from commit 8cdf6661 "multipath:
> check on multipathd without starting it". Instead, "multipath -u" does
> access the socket and start multipath again (which is what we want IMO),
> but it is now able to detect and handle the "full backlog" situation.
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> 
> V2:
> 
> Use same error reporting convention in __mpath_connect() as in
> mpath_connect() (Hannes Reinecke). We can't easily change the latter,
> because it's part of the "public" libmpathcmd API.
> 
> ---
>   libmpathcmd/mpath_cmd.c | 24 +++++++++++++-
>   libmpathcmd/mpath_cmd.h | 15 +++++++++
>   multipath/main.c        | 70 +++++++++++++----------------------------
>   3 files changed, 60 insertions(+), 49 deletions(-)
> 
> diff --git a/libmpathcmd/mpath_cmd.c b/libmpathcmd/mpath_cmd.c
> index df4ca541..b681311b 100644
> --- a/libmpathcmd/mpath_cmd.c
> +++ b/libmpathcmd/mpath_cmd.c
> @@ -26,6 +26,7 @@
>   #include <poll.h>
>   #include <string.h>
>   #include <errno.h>
> +#include <fcntl.h>
>   
>   #include "mpath_cmd.h"
>   
> @@ -93,10 +94,11 @@ static size_t write_all(int fd, const void *buf, size_t len)
>   /*
>    * connect to a unix domain socket
>    */
> -int mpath_connect(void)
> +int __mpath_connect(int nonblocking)
>   {
>   	int fd, len;
>   	struct sockaddr_un addr;
> +	int flags = 0;
>   
>   	memset(&addr, 0, sizeof(addr));
>   	addr.sun_family = AF_LOCAL;
> @@ -108,14 +110,34 @@ int mpath_connect(void)
>   	if (fd == -1)
>   		return -1;
>   
> +	if (nonblocking) {
> +		flags = fcntl(fd, F_GETFL, 0);
> +		if (flags != -1)
> +			(void)fcntl(fd, F_SETFL, flags|O_NONBLOCK);
> +	}
> +
>   	if (connect(fd, (struct sockaddr *)&addr, len) == -1) {
> +		int err = errno;
> +
>   		close(fd);
> +		errno = err;
>   		return -1;
>   	}
>   
> +	if (nonblocking && flags != -1)
> +		(void)fcntl(fd, F_SETFL, flags);
> +
>   	return fd;
>   }
>   
> +/*
> + * connect to a unix domain socket
> + */
> +int mpath_connect(void)
> +{
> +	return __mpath_connect(0);
> +}
> +
>   int mpath_disconnect(int fd)
>   {
>   	return close(fd);
> diff --git a/libmpathcmd/mpath_cmd.h b/libmpathcmd/mpath_cmd.h
> index 15aeb067..ccfd35f2 100644
> --- a/libmpathcmd/mpath_cmd.h
> +++ b/libmpathcmd/mpath_cmd.h
> @@ -34,6 +34,21 @@ extern "C" {
>   #define DEFAULT_REPLY_TIMEOUT	4000
>   
>   
> +/*
> + * DESCRIPTION:
> + *	Same as mpath_connect() (see below) except for the "nonblocking"
> + *	parameter.
> + *	If "nonblocking" is set, connects in non-blocking mode. This is
> + *	useful to avoid blocking if the listening socket's backlog is
> + *	exceeded. In this case, errno will be set to EAGAIN.
> + *	In case of success, the returned file descriptor is in in blocking
> + *	mode, even if "nonblocking" was true.
> + *
> + * RETURNS:
> + *	A file descriptor on success. -1 on failure (with errno set).
> + */
> +int __mpath_connect(int nonblocking);
> +
>   /*
>    * DESCRIPTION:
>    *	Connect to the running multipathd daemon. On systems with the
> diff --git a/multipath/main.c b/multipath/main.c
> index 008e3d3f..7933b07f 100644
> --- a/multipath/main.c
> +++ b/multipath/main.c
> @@ -850,55 +850,29 @@ out:
>   	return r;
>   }
>   
> -int is_multipathd_running(void)
> +static int test_multipathd_socket(void)
>   {
> -	FILE *f = NULL;
> -	char buf[16];
> -	char path[PATH_MAX];
> -	int pid;
> -	char *end;
> +	int fd;
> +	/*
> +	 * "multipath -u" may be run before the daemon is started. In this
> +	 * case, systemd might own the socket but might delay multipathd
> +	 * startup until some other unit (udev settle!)  has finished
> +	 * starting. With many LUNs, the listen backlog may be exceeded, which
> +	 * would cause connect() to block. This causes udev workers calling
> +	 * "multipath -u" to hang, and thus creates a deadlock, until "udev
> +	 * settle" times out.  To avoid this, call connect() in non-blocking
> +	 * mode here, and take EAGAIN as indication for a filled-up systemd
> +	 * backlog.
> +	 */
>   
> -	f = fopen(DEFAULT_PIDFILE, "r");
> -	if (!f) {
> -		if (errno != ENOENT)
> -			condlog(4, "can't open " DEFAULT_PIDFILE ": %s",
> -				strerror(errno));
> -		return 0;
> -	}
> -	if (!fgets(buf, sizeof(buf), f)) {
> -		if (ferror(f))
> -			condlog(4, "read of " DEFAULT_PIDFILE " failed: %s",
> -				strerror(errno));
> -		fclose(f);
> -		return 0;
> -	}
> -	fclose(f);
> -	errno = 0;
> -	strchop(buf);
> -	pid = strtol(buf, &end, 10);
> -	if (errno != 0 || pid <= 0 || *end != '\0') {
> -		condlog(4, "invalid contents in " DEFAULT_PIDFILE ": '%s'",
> -			buf);
> -		return 0;
> -	}
> -	snprintf(path, sizeof(path), "/proc/%d/comm", pid);
> -	f = fopen(path, "r");
> -	if (!f) {
> -		if (errno != ENOENT)
> -			condlog(4, "can't open %s: %s", path, strerror(errno));
> -		return 0;
> -	}
> -	if (!fgets(buf, sizeof(buf), f)) {
> -		if (ferror(f))
> -			condlog(4, "read of %s failed: %s", path,
> -				strerror(errno));
> -		fclose(f);
> -		return 0;
> -	}
> -	fclose(f);
> -	strchop(buf);
> -	if (strcmp(buf, "multipathd") != 0)
> -		return 0;
> +	fd = __mpath_connect(1);
> +	if (fd == -1) {
> +		if (errno == EAGAIN)
> +			condlog(3, "daemon backlog exceeded");
> +		else
> +			return 0;
> +	} else
> +		close(fd);
>   	return 1;
>   }
>   
> @@ -1080,7 +1054,7 @@ main (int argc, char *argv[])
>   	}
>   	if (cmd == CMD_VALID_PATH &&
>   	    dev_type == DEV_UEVENT) {
> -		if (!is_multipathd_running()) {
> +		if (!test_multipathd_socket()) {
>   			condlog(3, "%s: daemon is not running", dev);
>   			if (!systemd_service_enabled(dev)) {
>   				r = print_cmd_valid(RTVL_NO, NULL, conf);
> 
I would have preferred a 'boolean' as argument for __mpath_connect(), 
but that's a style question I guess.

Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah
HRB 21284 (AG Nürnberg)

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Re: [PATCH v2] multipath -u: test socket connection in non-blocking mode

[Martin Wilck]

On Wed, 2019-04-24 at 11:07 +0200, Martin Wilck wrote:
> Since commit d7188fcd "multipathd: start daemon after udev trigger",
> multipathd startup is delayed during boot until after "udev settle"
> terminates. But "multipath -u" is run by udev workers for storage
> devices,
> and attempts to connect to the multipathd socket. This causes a start
> job
> for multipathd to be scheduled by systemd, but that job won't be
> started
> until "udev settle" finishes. This is not a problem on systems with
> 129 or
> less storage units, because the connect() call of "multipath -u" will
> succeed anyway. But on larger systems, the listen backlog of the
> systemd
> socket can be exceeded, which causes connect() calls for the socket
> to
> block until multipathd starts up and begins calling accept(). This
> creates
> a deadlock situation, because "multipath -u" (called by udev workers)
> blocks, and thus "udev settle" doesn't finish, delaying multipathd
> startup. This situation then persists until either the workers or
> "udev
> settle" time out. In the former case, path devices might be
> misclassified
> as non-multipath devices by "multipath -u".
> 
> Fix this by using a non-blocking socket fd for connect() and
> interpret the
> errno appropriately.
> 
> This patch reverts most of the changes from commit 8cdf6661
> "multipath:
> check on multipathd without starting it". Instead, "multipath -u"
> does
> access the socket and start multipath again (which is what we want
> IMO),
> but it is now able to detect and handle the "full backlog" situation.
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> 
> V2:
> 
> Use same error reporting convention in __mpath_connect() as in
> mpath_connect() (Hannes Reinecke). We can't easily change the latter,
> because it's part of the "public" libmpathcmd API. 

FTR, our customer reported that this patch fixed his problem.

@Ben, I'd be grateful if you could try it (or have the user try it)
in your problem case as well.

-- 
Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)


--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Re: [PATCH v2] multipath -u: test socket connection in non-blocking mode

[Benjamin Marzinski]

On Thu, Apr 25, 2019 at 09:33:03PM +0200, Martin Wilck wrote:
> On Wed, 2019-04-24 at 11:07 +0200, Martin Wilck wrote:
> > Since commit d7188fcd "multipathd: start daemon after udev trigger",
> > multipathd startup is delayed during boot until after "udev settle"
> > terminates. But "multipath -u" is run by udev workers for storage
> > devices,
> > and attempts to connect to the multipathd socket. This causes a start
> > job
> > for multipathd to be scheduled by systemd, but that job won't be
> > started
> > until "udev settle" finishes. This is not a problem on systems with
> > 129 or
> > less storage units, because the connect() call of "multipath -u" will
> > succeed anyway. But on larger systems, the listen backlog of the
> > systemd
> > socket can be exceeded, which causes connect() calls for the socket
> > to
> > block until multipathd starts up and begins calling accept(). This
> > creates
> > a deadlock situation, because "multipath -u" (called by udev workers)
> > blocks, and thus "udev settle" doesn't finish, delaying multipathd
> > startup. This situation then persists until either the workers or
> > "udev
> > settle" time out. In the former case, path devices might be
> > misclassified
> > as non-multipath devices by "multipath -u".
> > 
> > Fix this by using a non-blocking socket fd for connect() and
> > interpret the
> > errno appropriately.
> > 
> > This patch reverts most of the changes from commit 8cdf6661
> > "multipath:
> > check on multipathd without starting it". Instead, "multipath -u"
> > does
> > access the socket and start multipath again (which is what we want
> > IMO),
> > but it is now able to detect and handle the "full backlog" situation.
> > 
> > Signed-off-by: Martin Wilck <mwilck@suse.com>
> > 
> > V2:
> > 
> > Use same error reporting convention in __mpath_connect() as in
> > mpath_connect() (Hannes Reinecke). We can't easily change the latter,
> > because it's part of the "public" libmpathcmd API. 
> 
> FTR, our customer reported that this patch fixed his problem.
> 
> @Ben, I'd be grateful if you could try it (or have the user try it)
> in your problem case as well.

Unfortunately, I don't have a 129+ path system handy that the person who
does isn't around right now.  The code makes sense, and assuming that I
can verify that it fixes the problem I'm seeing, I'm fine with going
this route.

-Ben

> 
> -- 
> Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107
> SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
> HRB 21284 (AG Nürnberg)
> 

Re: [PATCH v2] multipath -u: test socket connection in non-blocking mode

[Martin Wilck]

On Mon, 2019-04-29 at 16:38 -0500, Benjamin Marzinski wrote:
> On Thu, Apr 25, 2019 at 09:33:03PM +0200, Martin Wilck wrote:
> > 
> > @Ben, I'd be grateful if you could try it (or have the user try it)
> > in your problem case as well.
> 
> Unfortunately, I don't have a 129+ path system handy that the person
> who
> does isn't around right now.  The code makes sense, and assuming that
> I
> can verify that it fixes the problem I'm seeing, I'm fine with going
> this route.

Thanks, Ben. As noted earlier, we've had several customers who reported
this, and they're all happy with this patch. 

Martin

Re: [PATCH v2] multipath -u: test socket connection in non-blocking mode

[Benjamin Marzinski]

On Thu, May 16, 2019 at 08:44:56AM +0200, Martin Wilck wrote:
> On Mon, 2019-04-29 at 16:38 -0500, Benjamin Marzinski wrote:
> > On Thu, Apr 25, 2019 at 09:33:03PM +0200, Martin Wilck wrote:
> > > 
> > > @Ben, I'd be grateful if you could try it (or have the user try it)
> > > in your problem case as well.
> > 
> > Unfortunately, I don't have a 129+ path system handy that the person
> > who
> > does isn't around right now.  The code makes sense, and assuming that
> > I
> > can verify that it fixes the problem I'm seeing, I'm fine with going
> > this route.
> 
> Thanks, Ben. As noted earlier, we've had several customers who reported
> this, and they're all happy with this patch. 

I did get around to testing this, and it fixes our reproducer too.

-Ben

> 
> Martin
>