[PATCH 0/8] various multipath-tools patches

[PATCH 0/8] various multipath-tools patches

[Martin Wilck]

Hi Christophe,

this series contains some patches I had on my list for
some time,  plus some stuff that came up during the discussion
about Ben's last large patch series.

This goes on top of the "[PATCH 0/6] More misc multipath patches"
series that Ben sent today.

Regards,
Martin

Martin Wilck (8):
  test-kpartx: add test for invocation on image file
  libmultipath: scsi_ioctl_pathinfo doesn't fail
  libmultipath: cciss_ioctl_pathinfo doesn't fail
  multipath.rules: add comment about DM_MULTIPATH_DEVICE_PATH=2
  multipath.8: man page update
  libmultipath: test code for "zombie" TUR threads
  libmultipath: handle TUR threads that can't be cancelled
  multipathd: handle repeated udev retrigger failure

 kpartx/test-kpartx          |  24 +++-
 libmultipath/checkers/tur.c |  69 ++++++++-
 libmultipath/discovery.c    |  33 ++---
 multipath/multipath.8       | 273 ++++++++++++++++++++++--------------
 multipath/multipath.rules   |   4 +
 multipathd/main.c           |  32 +++--
 6 files changed, 297 insertions(+), 138 deletions(-)

-- 
2.19.0

[PATCH 1/8] test-kpartx: add test for invocation on image file

[Martin Wilck]

Add a test for kpartx invocation on an image file that is not
yet part of a loop device, passing a relative path to kpartx:

        # kpartx -av disk.img
        ...
        # kpartx -d disk.img

The recent patches 95c4189b "kpartx: Use absolute paths to create mappings"
and c1adcc5b "kpartx: Use absolute paths to create mappings" both
independently fixed this behavior, which had been broken previously by
9bdfa3eb "kpartx: use absolute path for regular files".

Cc: Stef Walter <stefw@redhat.com>
Cc: Yuval Turgeman <yturgema@redhat.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 kpartx/test-kpartx | 24 +++++++++++++++++++++++-
 1 file changed, 23 insertions(+), 1 deletion(-)

diff --git a/kpartx/test-kpartx b/kpartx/test-kpartx
index d2001dce..d3c9aef5 100755
--- a/kpartx/test-kpartx
+++ b/kpartx/test-kpartx
@@ -87,6 +87,7 @@ step preparation
 FILE1=kpartx1
 FILE2=kpartx2
 FILE3=kpartx3
+FILE4=kpartx4
 SIZE=$((1024*1024*1024))  # use bytes as units here
 SECTSIZ=512
 OFFS=32                # offset of linear mapping into dev, sectors
@@ -108,6 +109,7 @@ step "create loop devices"
 truncate -s $SIZE $FILE1
 truncate -s $SIZE $FILE2
 truncate -s $SIZE $FILE3
+truncate -s $SIZE $FILE4
 
 LO1=$(losetup -f $FILE1 --show)
 push_cleanup 'losetup -d $LO1'
@@ -115,8 +117,10 @@ LO2=$(losetup -f $FILE2 --show)
 push_cleanup 'losetup -d $LO2'
 LO3=$(losetup -f $FILE3 --show)
 push_cleanup 'losetup -d $LO3'
+LO4=$(losetup -f $FILE4 --show)
+push_cleanup 'losetup -d $LO4'
 
-[[ $LO1 && $LO2 && $LO3 && -b $LO1 && -b $LO2 && -b $LO3 ]]
+[[ $LO1 && $LO2 && $LO3 && $LO4 && -b $LO1 && -b $LO2 && -b $LO3 && -b $LO4 ]]
 DEV1=$(stat -c "%t:%T" $LO1)
 DEV2=$(stat -c "%t:%T" $LO2)
 DEV3=$(stat -c "%t:%T" $LO3)
@@ -173,6 +177,7 @@ step "create partitions on loop devices"
 
 mk_partitions $LO1
 mk_partitions $LO2
+mk_partitions $LO4
 
 # Test invocation of kpartx with regular file here
 LO2P1=/dev/mapper/$(basename $LO2)-foo1
@@ -310,4 +315,21 @@ $KPARTX $KPARTX_OPTS -f -d $LO3
 [[ -b /dev/mapper/$SPAN1 ]]
 [[ -b /dev/mapper/$SPAN2 ]]
 
+step "test kpartx creation/deletion on an image file with no existing loopdev"
+losetup -d $LO4
+
+OUTPUT=$($KPARTX $KPARTX_OPTS -v -a $FILE4 2>&1)
+read loop dm < \
+     <(sed -n  's/^add map \(loop[0-9]*\)p1 ([0-9]*:\([0-9]*\)).*$/\1 dm-\2/p' \
+	   <<<$OUTPUT)
+[[ $dm && $loop ]]
+push_cleanup "dmsetup remove -f /dev/$dm"
+push_cleanup "losetup -d /dev/$loop"
+
+[[ -b /dev/mapper/${loop}p1 ]]
+$KPARTX -d $KPARTX_OPTS $FILE4
+[[ ! -b /dev/mapper/${loop}p1 ]]
+# /dev/$loop is _not_ automatically deleted
+[[ -b /dev/${loop} ]]
+
 OK=yes
-- 
2.19.0

Re: [PATCH 1/8] test-kpartx: add test for invocation on image file

[Benjamin Marzinski]

On Wed, Oct 10, 2018 at 10:04:59PM +0200, Martin Wilck wrote:
> Add a test for kpartx invocation on an image file that is not
> yet part of a loop device, passing a relative path to kpartx:
> 
>         # kpartx -av disk.img
>         ...
>         # kpartx -d disk.img
> 
> The recent patches 95c4189b "kpartx: Use absolute paths to create mappings"
> and c1adcc5b "kpartx: Use absolute paths to create mappings" both
> independently fixed this behavior, which had been broken previously by
> 9bdfa3eb "kpartx: use absolute path for regular files".
> 

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>

> Cc: Stef Walter <stefw@redhat.com>
> Cc: Yuval Turgeman <yturgema@redhat.com>
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  kpartx/test-kpartx | 24 +++++++++++++++++++++++-
>  1 file changed, 23 insertions(+), 1 deletion(-)
> 
> diff --git a/kpartx/test-kpartx b/kpartx/test-kpartx
> index d2001dce..d3c9aef5 100755
> --- a/kpartx/test-kpartx
> +++ b/kpartx/test-kpartx
> @@ -87,6 +87,7 @@ step preparation
>  FILE1=kpartx1
>  FILE2=kpartx2
>  FILE3=kpartx3
> +FILE4=kpartx4
>  SIZE=$((1024*1024*1024))  # use bytes as units here
>  SECTSIZ=512
>  OFFS=32                # offset of linear mapping into dev, sectors
> @@ -108,6 +109,7 @@ step "create loop devices"
>  truncate -s $SIZE $FILE1
>  truncate -s $SIZE $FILE2
>  truncate -s $SIZE $FILE3
> +truncate -s $SIZE $FILE4
>  
>  LO1=$(losetup -f $FILE1 --show)
>  push_cleanup 'losetup -d $LO1'
> @@ -115,8 +117,10 @@ LO2=$(losetup -f $FILE2 --show)
>  push_cleanup 'losetup -d $LO2'
>  LO3=$(losetup -f $FILE3 --show)
>  push_cleanup 'losetup -d $LO3'
> +LO4=$(losetup -f $FILE4 --show)
> +push_cleanup 'losetup -d $LO4'
>  
> -[[ $LO1 && $LO2 && $LO3 && -b $LO1 && -b $LO2 && -b $LO3 ]]
> +[[ $LO1 && $LO2 && $LO3 && $LO4 && -b $LO1 && -b $LO2 && -b $LO3 && -b $LO4 ]]
>  DEV1=$(stat -c "%t:%T" $LO1)
>  DEV2=$(stat -c "%t:%T" $LO2)
>  DEV3=$(stat -c "%t:%T" $LO3)
> @@ -173,6 +177,7 @@ step "create partitions on loop devices"
>  
>  mk_partitions $LO1
>  mk_partitions $LO2
> +mk_partitions $LO4
>  
>  # Test invocation of kpartx with regular file here
>  LO2P1=/dev/mapper/$(basename $LO2)-foo1
> @@ -310,4 +315,21 @@ $KPARTX $KPARTX_OPTS -f -d $LO3
>  [[ -b /dev/mapper/$SPAN1 ]]
>  [[ -b /dev/mapper/$SPAN2 ]]
>  
> +step "test kpartx creation/deletion on an image file with no existing loopdev"
> +losetup -d $LO4
> +
> +OUTPUT=$($KPARTX $KPARTX_OPTS -v -a $FILE4 2>&1)
> +read loop dm < \
> +     <(sed -n  's/^add map \(loop[0-9]*\)p1 ([0-9]*:\([0-9]*\)).*$/\1 dm-\2/p' \
> +	   <<<$OUTPUT)
> +[[ $dm && $loop ]]
> +push_cleanup "dmsetup remove -f /dev/$dm"
> +push_cleanup "losetup -d /dev/$loop"
> +
> +[[ -b /dev/mapper/${loop}p1 ]]
> +$KPARTX -d $KPARTX_OPTS $FILE4
> +[[ ! -b /dev/mapper/${loop}p1 ]]
> +# /dev/$loop is _not_ automatically deleted
> +[[ -b /dev/${loop} ]]
> +
>  OK=yes
> -- 
> 2.19.0

[PATCH 2/8] libmultipath: scsi_ioctl_pathinfo doesn't fail

[Martin Wilck]

This function never returns error, so make a void.
Move the mask test to pathinfo() where it belongs.
The error message about the serial number isn't important
any more, reduce its level to v3.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/discovery.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
index b267f07c..8381d982 100644
--- a/libmultipath/discovery.c
+++ b/libmultipath/discovery.c
@@ -1496,7 +1496,7 @@ sysfs_pathinfo(struct path * pp, vector hwtable)
 	}
 }
 
-static int
+static void
 scsi_ioctl_pathinfo (struct path * pp, struct config *conf, int mask)
 {
 	struct udev_device *parent;
@@ -1506,7 +1506,7 @@ scsi_ioctl_pathinfo (struct path * pp, struct config *conf, int mask)
 		detect_alua(pp, conf);
 
 	if (!(mask & DI_SERIAL))
-		return 0;
+		return;
 
 	parent = pp->udev;
 	while (parent) {
@@ -1525,17 +1525,17 @@ scsi_ioctl_pathinfo (struct path * pp, struct config *conf, int mask)
 		parent = udev_device_get_parent(parent);
 	}
 	if (!attr_path || pp->sg_id.host_no == -1)
-		return 0;
+		return;
 
 	if (get_vpd_sysfs(parent, 0x80, pp->serial, SERIAL_SIZE) <= 0) {
 		if (get_serial(pp->serial, SERIAL_SIZE, pp->fd)) {
-			condlog(2, "%s: fail to get serial", pp->dev);
-			return 0;
+			condlog(3, "%s: fail to get serial", pp->dev);
+			return;
 		}
 	}
 
 	condlog(3, "%s: serial = %s", pp->dev, pp->serial);
-	return 0;
+	return;
 }
 
 static int
@@ -1937,9 +1937,8 @@ int pathinfo(struct path *pp, struct config *conf, int mask)
 	if (mask & DI_SERIAL)
 		get_geometry(pp);
 
-	if (path_state == PATH_UP && pp->bus == SYSFS_BUS_SCSI &&
-	    scsi_ioctl_pathinfo(pp, conf, mask))
-		goto blank;
+	if (path_state == PATH_UP && pp->bus == SYSFS_BUS_SCSI)
+		scsi_ioctl_pathinfo(pp, conf, mask);
 
 	if (pp->bus == SYSFS_BUS_CCISS &&
 	    cciss_ioctl_pathinfo(pp, mask))
-- 
2.19.0

Re: [PATCH 2/8] libmultipath: scsi_ioctl_pathinfo doesn't fail

[Benjamin Marzinski]

On Wed, Oct 10, 2018 at 10:05:00PM +0200, Martin Wilck wrote:
> This function never returns error, so make a void.
> Move the mask test to pathinfo() where it belongs.
> The error message about the serial number isn't important
> any more, reduce its level to v3.
> 

Didn't you intend to move the DI_SERIAL check to pathinfo()

regardless,

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>

> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/discovery.c | 17 ++++++++---------
>  1 file changed, 8 insertions(+), 9 deletions(-)
> 
> diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
> index b267f07c..8381d982 100644
> --- a/libmultipath/discovery.c
> +++ b/libmultipath/discovery.c
> @@ -1496,7 +1496,7 @@ sysfs_pathinfo(struct path * pp, vector hwtable)
>  	}
>  }
>  
> -static int
> +static void
>  scsi_ioctl_pathinfo (struct path * pp, struct config *conf, int mask)
>  {
>  	struct udev_device *parent;
> @@ -1506,7 +1506,7 @@ scsi_ioctl_pathinfo (struct path * pp, struct config *conf, int mask)
>  		detect_alua(pp, conf);
>  
>  	if (!(mask & DI_SERIAL))
> -		return 0;
> +		return;
>  
>  	parent = pp->udev;
>  	while (parent) {
> @@ -1525,17 +1525,17 @@ scsi_ioctl_pathinfo (struct path * pp, struct config *conf, int mask)
>  		parent = udev_device_get_parent(parent);
>  	}
>  	if (!attr_path || pp->sg_id.host_no == -1)
> -		return 0;
> +		return;
>  
>  	if (get_vpd_sysfs(parent, 0x80, pp->serial, SERIAL_SIZE) <= 0) {
>  		if (get_serial(pp->serial, SERIAL_SIZE, pp->fd)) {
> -			condlog(2, "%s: fail to get serial", pp->dev);
> -			return 0;
> +			condlog(3, "%s: fail to get serial", pp->dev);
> +			return;
>  		}
>  	}
>  
>  	condlog(3, "%s: serial = %s", pp->dev, pp->serial);
> -	return 0;
> +	return;
>  }
>  
>  static int
> @@ -1937,9 +1937,8 @@ int pathinfo(struct path *pp, struct config *conf, int mask)
>  	if (mask & DI_SERIAL)
>  		get_geometry(pp);
>  
> -	if (path_state == PATH_UP && pp->bus == SYSFS_BUS_SCSI &&
> -	    scsi_ioctl_pathinfo(pp, conf, mask))
> -		goto blank;
> +	if (path_state == PATH_UP && pp->bus == SYSFS_BUS_SCSI)
> +		scsi_ioctl_pathinfo(pp, conf, mask);
>  
>  	if (pp->bus == SYSFS_BUS_CCISS &&
>  	    cciss_ioctl_pathinfo(pp, mask))
> -- 
> 2.19.0

[PATCH 3/8] libmultipath: cciss_ioctl_pathinfo doesn't fail

[Martin Wilck]

Make it a void function and move the DI_SERIAL test to pathinfo().

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/discovery.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
index 8381d982..873035e5 100644
--- a/libmultipath/discovery.c
+++ b/libmultipath/discovery.c
@@ -1538,14 +1538,11 @@ scsi_ioctl_pathinfo (struct path * pp, struct config *conf, int mask)
 	return;
 }
 
-static int
-cciss_ioctl_pathinfo (struct path * pp, int mask)
+static void
+cciss_ioctl_pathinfo(struct path *pp)
 {
-	if (mask & DI_SERIAL) {
-		get_serial(pp->serial, SERIAL_SIZE, pp->fd);
-		condlog(3, "%s: serial = %s", pp->dev, pp->serial);
-	}
-	return 0;
+	get_serial(pp->serial, SERIAL_SIZE, pp->fd);
+	condlog(3, "%s: serial = %s", pp->dev, pp->serial);
 }
 
 int
@@ -1940,9 +1937,8 @@ int pathinfo(struct path *pp, struct config *conf, int mask)
 	if (path_state == PATH_UP && pp->bus == SYSFS_BUS_SCSI)
 		scsi_ioctl_pathinfo(pp, conf, mask);
 
-	if (pp->bus == SYSFS_BUS_CCISS &&
-	    cciss_ioctl_pathinfo(pp, mask))
-		goto blank;
+	if (pp->bus == SYSFS_BUS_CCISS && mask & DI_SERIAL)
+		cciss_ioctl_pathinfo(pp);
 
 	if (mask & DI_CHECKER) {
 		if (path_state == PATH_UP) {
-- 
2.19.0

Re: [PATCH 3/8] libmultipath: cciss_ioctl_pathinfo doesn't fail

[Benjamin Marzinski]

On Wed, Oct 10, 2018 at 10:05:01PM +0200, Martin Wilck wrote:
> Make it a void function and move the DI_SERIAL test to pathinfo().
> 

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>

> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/discovery.c | 16 ++++++----------
>  1 file changed, 6 insertions(+), 10 deletions(-)
> 
> diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
> index 8381d982..873035e5 100644
> --- a/libmultipath/discovery.c
> +++ b/libmultipath/discovery.c
> @@ -1538,14 +1538,11 @@ scsi_ioctl_pathinfo (struct path * pp, struct config *conf, int mask)
>  	return;
>  }
>  
> -static int
> -cciss_ioctl_pathinfo (struct path * pp, int mask)
> +static void
> +cciss_ioctl_pathinfo(struct path *pp)
>  {
> -	if (mask & DI_SERIAL) {
> -		get_serial(pp->serial, SERIAL_SIZE, pp->fd);
> -		condlog(3, "%s: serial = %s", pp->dev, pp->serial);
> -	}
> -	return 0;
> +	get_serial(pp->serial, SERIAL_SIZE, pp->fd);
> +	condlog(3, "%s: serial = %s", pp->dev, pp->serial);
>  }
>  
>  int
> @@ -1940,9 +1937,8 @@ int pathinfo(struct path *pp, struct config *conf, int mask)
>  	if (path_state == PATH_UP && pp->bus == SYSFS_BUS_SCSI)
>  		scsi_ioctl_pathinfo(pp, conf, mask);
>  
> -	if (pp->bus == SYSFS_BUS_CCISS &&
> -	    cciss_ioctl_pathinfo(pp, mask))
> -		goto blank;
> +	if (pp->bus == SYSFS_BUS_CCISS && mask & DI_SERIAL)
> +		cciss_ioctl_pathinfo(pp);
>  
>  	if (mask & DI_CHECKER) {
>  		if (path_state == PATH_UP) {
> -- 
> 2.19.0

[PATCH 4/8] multipath.rules: add comment about DM_MULTIPATH_DEVICE_PATH=2

[Martin Wilck]

See dm-devel discussion "Changes in DM_MULTIPATH_DEVICE_PATH in
multipath-tools 0.7.7" (Sep. 2018) for rationale.

Cc: Adam Williamson <awilliam@redhat.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 multipath/multipath.rules | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/multipath/multipath.rules b/multipath/multipath.rules
index d658073f..9df11a95 100644
--- a/multipath/multipath.rules
+++ b/multipath/multipath.rules
@@ -45,6 +45,10 @@ ENV{DM_MULTIPATH_DEVICE_PATH}!="2", \
 # Code below here is only run in "smart" mode.
 # multipath -u has indicated this is "maybe" multipath.
 
+# Note that DM_MULTIPATH_DEVICE_PATH has the value 2 at this point.
+# This value will never propagate to other rules files, because
+# it will be reset to 1 in the "pretend_multipath" section below.
+
 # This shouldn't happen, just in case.
 ENV{FIND_MULTIPATHS_WAIT_UNTIL}!="?*", GOTO="end_mpath"
 
-- 
2.19.0

Re: [PATCH 4/8] multipath.rules: add comment about DM_MULTIPATH_DEVICE_PATH=2

[Benjamin Marzinski]

On Wed, Oct 10, 2018 at 10:05:02PM +0200, Martin Wilck wrote:
> See dm-devel discussion "Changes in DM_MULTIPATH_DEVICE_PATH in
> multipath-tools 0.7.7" (Sep. 2018) for rationale.
> 

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>

> Cc: Adam Williamson <awilliam@redhat.com>
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  multipath/multipath.rules | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/multipath/multipath.rules b/multipath/multipath.rules
> index d658073f..9df11a95 100644
> --- a/multipath/multipath.rules
> +++ b/multipath/multipath.rules
> @@ -45,6 +45,10 @@ ENV{DM_MULTIPATH_DEVICE_PATH}!="2", \
>  # Code below here is only run in "smart" mode.
>  # multipath -u has indicated this is "maybe" multipath.
>  
> +# Note that DM_MULTIPATH_DEVICE_PATH has the value 2 at this point.
> +# This value will never propagate to other rules files, because
> +# it will be reset to 1 in the "pretend_multipath" section below.
> +
>  # This shouldn't happen, just in case.
>  ENV{FIND_MULTIPATHS_WAIT_UNTIL}!="?*", GOTO="end_mpath"
>  
> -- 
> 2.19.0

[PATCH 5/8] multipath.8: man page update

[Martin Wilck]

Clean up the synopsis, listing only combinations of command line switches
that work and make sense. Split the switches between "operation modes"
and options. Fix the documentation of the -v switch, which was wrong.
Move the description of the "device" argument to the top. Link to
multipath.conf(5) for the description of the path grouping policy rather
than repeating the content here. Various minor improvements and clarifications.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 multipath/multipath.8 | 273 ++++++++++++++++++++++++++----------------
 1 file changed, 167 insertions(+), 106 deletions(-)

diff --git a/multipath/multipath.8 b/multipath/multipath.8
index b5e5292f..c9bd23aa 100644
--- a/multipath/multipath.8
+++ b/multipath/multipath.8
@@ -5,7 +5,7 @@
 .\"
 .\" ----------------------------------------------------------------------------
 .
-.TH MULTIPATH 8 2016-10-26 "Linux"
+.TH MULTIPATH 8 2018-10-10 "Linux"
 .
 .
 .\" ----------------------------------------------------------------------------
@@ -21,17 +21,68 @@ multipath \- Device mapper target autoconfig.
 .
 .B multipath
 .RB [\| \-v\ \c
-.IR verbosity \|]
+.IR level \|]
+.RB [\| \-B | \-d | \-i | \-q | \-r \|]
 .RB [\| \-b\ \c
-.IR bindings_file \|]
-.RB [\| \-d \|]
-.RB [\| \-h | \-l | \-ll | \-f | \-t | \-T | \-F | \-B | \-c | \-C | \-q | \-r | \-i | \-a | \-u | \-U | \-w | \-W \|]
+.IR file \|]
 .RB [\| \-p\ \c
-.IR failover | multibus | group_by_serial | group_by_prio | group_by_node_name \|]
+.IR policy \|]
+.RB [\| device \|]
+.
+.LP
+.B multipath
+.RB [\| \-v\ \c
+.IR level \|]
 .RB [\| \-R\ \c
 .IR retries \|]
+.B \-f device
+.
+.LP
+.B multipath
+.RB [\| \-v\ \c
+.IR level \|]
+.RB [\| \-R\ \c
+.IR retries \|]
+.B \-F
+.
+.LP
+.B multipath
+.RB [\| \-v\ \c
+.IR level \|]
+.RB [\| \-l | \-ll \|]
 .RB [\| device \|]
 .
+.LP
+.B multipath
+.RB [\| \-v\ \c
+.IR level \|]
+.RB [\| \-a | \-w \|]
+.B device
+.
+.LP
+.B multipath
+.RB [\| \-v\ \c
+.IR level \|]
+.B -W
+.
+.LP
+.B multipath
+.RB [\| \-v\ \c
+.IR level \|]
+.RB [\| \-i \|]
+.RB [\| \-c | \-C \|]
+.B device
+.
+.LP
+.B multipath
+.RB [\| \-v\ \c
+.IR level \|]
+.RB [\| \-i \|]
+.RB [\| \-u | \-U \|]
+.
+.LP
+.B multipath
+.RB [\| \-h | \-t | \-T \|]
 .
 .\" ----------------------------------------------------------------------------
 .SH DESCRIPTION
@@ -40,83 +91,62 @@ multipath \- Device mapper target autoconfig.
 .B multipath
 is used to detect and coalesce multiple paths to devices, for fail-over or performance reasons.
 .
-.
 .\" ----------------------------------------------------------------------------
-.SH OPTIONS
+.SH ARGUMENTS
 .\" ----------------------------------------------------------------------------
 .
 .TP
-.BI \-v " level"
-Verbosity, print all paths and multipaths:
+.BI device
+Act only on the multipath map specified by
+.IR device ,
+which is either:
 .RS 1.2i
-.TP 1.2i
-.I 0
-No output.
-.TP
-.I 1
-Print the created or updated multipath names only, for use to feed other tools like kpartx.
-.TP
-.I 2 +
-Print all info: detected paths, coalesced paths (ie multipaths) and device maps.
-.RE
-.
-.TP
-.B \-h
-Print usage text.
-.
-.TP
-.B \-d
-Dry run, do not create or update devmaps.
+.IP \[bu]
+A multipath map name.
+.IP \[bu]
+A path (low-level device) associated with the desired multipath map; the path may be in one of the following formats:
+.RS 1.2i
+.IP \[bu]
+.B /dev/sdX
+.IP \[bu]
+.B major:minor
 .
-.TP
-.B \-l
-Show the current multipath topology from information fetched in sysfs and the device mapper.
+.\" ----------------------------------------------------------------------------
+.SH OPERATION MODES
+.\" ----------------------------------------------------------------------------
 .
-.TP
-.B \-ll
-Show the current multipath topology from all available information (sysfs, the device mapper, path checkers ...).
+The default operation mode is to detect and set up multipath maps from the devices found in
+the system.
 .
+Other operation modes are chosen by using one of the following command line switches:
 .TP
 .B \-f
-Flush a multipath device map specified as parameter, if unused.
+Flush (remove) a multipath device map specified as parameter, if unused.
 .
 .TP
 .B \-F
-Flush all unused multipath device maps.
-.
-.TP
-.B \-t
-Display the currently used multipathd configuration.
+Flush (remove) all unused multipath device maps.
 .
 .TP
-.B \-T
-Display the currently used multipathd configuration, limiting the output to
-those devices actually present in the system. This can be used a template for
-creating \fImultipath.conf\fR.
+.B \-l
+Show ("list") the current multipath topology from information fetched in sysfs and the device mapper.
 .
 .TP
-.B \-r
-Force devmap reload.
+.B \-ll
+Show ("list") the current multipath topology from all available information (sysfs, the
+device mapper, path checkers ...).
 .
 .TP
-.B \-i
-Ignore WWIDs file when processing devices. If
-\fIfind_multipaths strict\fR or \fIfind_multipaths no\fR is set in
-\fImultipath.conf\fR, multipath only considers devices that are
-listed in the WWIDs file. This option overrides that behavior. For other values
-of \fIfind_multipaths\fR, this option has no effect. See the description of
-\fIfind_multipaths\fR in
-.BR multipath.conf (5).
-This option should only be used in rare circumstances.
+.B \-a
+Add the WWID for the specified device to the WWIDs file.
 .
 .TP
-.B \-B
-Treat the bindings file as read only.
+.B \-w
+Remove the WWID for the specified device from the WWIDs file.
 .
 .TP
-.BI \-b " bindings_file"
-Set user_friendly_names bindings file location.  The default is
-\fI/etc/multipath/bindings\fR.
+.B \-W
+Reset the WWIDs file to only include the current multipath devices.
 .
 .TP
 .B \-c
@@ -129,14 +159,6 @@ test whether or not I/O on this device is likely to succeed. The command
 itself doesn't attempt to do I/O on the device.
 .
 .TP
-.B \-q
-Allow device tables with \fIqueue_if_no_path\fR when multipathd is not running.
-.
-.TP
-.B \-a
-Add the WWID for the specified device to the WWIDs file.
-.
-.TP
 .B \-u
 Check if the device specified in the program environment should be
 a path in a multipath device.
@@ -147,60 +169,99 @@ Check if the device specified in the program environment is a multipath device
 with usable paths. See \fB-C\fB.
 .
 .TP
-.B \-w
-Remove the WWID for the specified device from the WWIDs file.
+.B \-h
+Print usage text.
 .
 .TP
-.B \-W
-Reset the WWIDs file to only include the current multipath devices.
+.B \-t
+Display the currently used multipathd configuration.
 .
 .TP
-.BI \-p " policy"
-Force new maps to use the specified policy:
+.B \-T
+Display the currently used multipathd configuration, limiting the output to
+those devices actually present in the system. This can be used a template for
+creating \fImultipath.conf\fR.
+.
+.\" ----------------------------------------------------------------------------
+.SH OPTIONS
+.\" ----------------------------------------------------------------------------
+.
+.TP
+.BI \-v " level"
+Verbosity of information printed to stdout in default and "list" operation
+modes. The default level is \fI-v 2\fR.
 .RS 1.2i
 .TP 1.2i
-.I failover
-One path per priority group.
+.I 0
+Nothing is printed.
 .TP
-.I multibus
-All paths in one priority group.
+.I 1
+In default mode, Names/WWIDs of created or modified multipath maps are
+printed. In list mode, WWIDs of all multipath maps are printed.
 .TP
-.I group_by_serial
-One priority group per serial number.
+.I 2
+In default mode,
+Topology of created or modified multipath maps is printed.
+In list mode, topology of all multipath maps is printed.
 .TP
-.I group_by_prio
-One priority group per priority value. Priorities are determined by
-callout programs specified as a global, per-controller or
-per-multipath option in the configuration file.
+.I 3
+All detected paths and the topology of all multipath maps are printed.
+.
+.LP
+.
+The verbosity level also controls the level of log and debug messages printed to
+\fIstderr\fR. The default level corresponds to \fILOG_NOTICE\fR
+(important messages that shouldn't be missed in normal operation).
+.
+.RE
 .TP
-.I group_by_node_name
-One priority group per target node name. Target node names are fetched
-in \fI/sys/class/fc_transport/target*/node_name\fR.
+.B \-d
+Dry run, do not create or update devmaps.
+.
 .TP
-.RE
-Existing maps are not modified.
+.B \-i
+Ignore WWIDs file when processing devices. If
+\fIfind_multipaths strict\fR or \fIfind_multipaths no\fR is set in
+\fImultipath.conf\fR, multipath only considers devices that are
+listed in the WWIDs file. This option overrides that behavior. For other values
+of \fIfind_multipaths\fR, this option has no effect. See the description of
+\fIfind_multipaths\fR in
+.BR multipath.conf (5).
+This option should only be used in rare circumstances.
 .
 .TP
-.BI \-R " retries"
-Number of times to retry flushing multipath devices that are in-use. The default
-is \fI0\fR.
+.B \-B
+Treat the bindings file as read only.
 .
 .TP
-.BI device
-Update only the devmap specified by
-.IR device ,
-which is either:
-.RS 1.2i
-.IP \[bu]
-A devmap name.
-.IP \[bu]
-A path associated with the desired devmap; the path may be in one of the following formats:
-.RS 1.2i
-.IP \[bu]
-.B /dev/sdX
-.IP \[bu]
-.B major:minor
+.BI \-b " file"
+Set \fIuser_friendly_names\fR bindings file location.  The default is
+\fI/etc/multipath/bindings\fR.
+.
+.TP
+.B \-q
+Don't unset the device mapper feature \fIqueue_if_no_path\fR for multipath
+maps. Normally, \fBmultipath\fR would do so if \fBmultipathd\fR is not
+running, because only a running multipath daemon guarantees that unusable
+paths are reinstated when they become usable again.
+.
+.TP
+.BI \-p " policy"
+Force new maps to use the specified policy, overriding the configuration in
+\fBmultipath.conf(5)\fR. The possible values for
+\fIpolicy\fR are the same as the values for \fIpath_grouping_policy\fR in
+\fBmultipath.conf(5)\fR. Existing maps are not modified.
+.
+.TP
+.B \-r
+Force a reload of all existing multipath maps. This command is delegated to
+the multipathd daemon if it's running. In this case, other command line
+switches of the \fImultipath\fR command have no effect.
 .
+.TP
+.BI \-R " retries"
+Number of times to retry flushing multipath devices that are in use. The default
+is \fI0\fR.
 .
 .\" ----------------------------------------------------------------------------
 .SH "SEE ALSO"
-- 
2.19.0

Re: [PATCH 5/8] multipath.8: man page update

[Benjamin Marzinski]

On Wed, Oct 10, 2018 at 10:05:03PM +0200, Martin Wilck wrote:
> Clean up the synopsis, listing only combinations of command line switches
> that work and make sense. Split the switches between "operation modes"
> and options. Fix the documentation of the -v switch, which was wrong.
> Move the description of the "device" argument to the top. Link to
> multipath.conf(5) for the description of the path grouping policy rather
> than repeating the content here. Various minor improvements and clarifications.
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  multipath/multipath.8 | 273 ++++++++++++++++++++++++++----------------
>  1 file changed, 167 insertions(+), 106 deletions(-)
> 
> diff --git a/multipath/multipath.8 b/multipath/multipath.8
> index b5e5292f..c9bd23aa 100644
> --- a/multipath/multipath.8
> +++ b/multipath/multipath.8
> @@ -5,7 +5,7 @@
>  .\"
>  .\" ----------------------------------------------------------------------------
>  .
> -.TH MULTIPATH 8 2016-10-26 "Linux"
> +.TH MULTIPATH 8 2018-10-10 "Linux"
>  .
>  .
>  .\" ----------------------------------------------------------------------------
> @@ -21,17 +21,68 @@ multipath \- Device mapper target autoconfig.
>  .
>  .B multipath
>  .RB [\| \-v\ \c
> -.IR verbosity \|]
> +.IR level \|]
> +.RB [\| \-B | \-d | \-i | \-q | \-r \|]
>  .RB [\| \-b\ \c
> -.IR bindings_file \|]
> -.RB [\| \-d \|]
> -.RB [\| \-h | \-l | \-ll | \-f | \-t | \-T | \-F | \-B | \-c | \-C | \-q | \-r | \-i | \-a | \-u | \-U | \-w | \-W \|]
> +.IR file \|]
>  .RB [\| \-p\ \c
> -.IR failover | multibus | group_by_serial | group_by_prio | group_by_node_name \|]
> +.IR policy \|]
> +.RB [\| device \|]
> +.
> +.LP
> +.B multipath
> +.RB [\| \-v\ \c
> +.IR level \|]
>  .RB [\| \-R\ \c
>  .IR retries \|]
> +.B \-f device
> +.
> +.LP
> +.B multipath
> +.RB [\| \-v\ \c
> +.IR level \|]
> +.RB [\| \-R\ \c
> +.IR retries \|]
> +.B \-F
> +.
> +.LP
> +.B multipath
> +.RB [\| \-v\ \c
> +.IR level \|]
> +.RB [\| \-l | \-ll \|]
>  .RB [\| device \|]
>  .
> +.LP
> +.B multipath
> +.RB [\| \-v\ \c
> +.IR level \|]
> +.RB [\| \-a | \-w \|]
> +.B device
> +.
> +.LP
> +.B multipath
> +.RB [\| \-v\ \c
> +.IR level \|]
> +.B -W
> +.
> +.LP
> +.B multipath
> +.RB [\| \-v\ \c
> +.IR level \|]
> +.RB [\| \-i \|]
> +.RB [\| \-c | \-C \|]
> +.B device
> +.
> +.LP
> +.B multipath
> +.RB [\| \-v\ \c
> +.IR level \|]
> +.RB [\| \-i \|]
> +.RB [\| \-u | \-U \|]
> +.
> +.LP
> +.B multipath
> +.RB [\| \-h | \-t | \-T \|]
>  .
>  .\" ----------------------------------------------------------------------------
>  .SH DESCRIPTION
> @@ -40,83 +91,62 @@ multipath \- Device mapper target autoconfig.
>  .B multipath
>  is used to detect and coalesce multiple paths to devices, for fail-over or performance reasons.
>  .
> -.
>  .\" ----------------------------------------------------------------------------
> -.SH OPTIONS
> +.SH ARGUMENTS
>  .\" ----------------------------------------------------------------------------
>  .
>  .TP
> -.BI \-v " level"
> -Verbosity, print all paths and multipaths:
> +.BI device
> +Act only on the multipath map specified by
> +.IR device ,
> +which is either:
>  .RS 1.2i
> -.TP 1.2i
> -.I 0
> -No output.
> -.TP
> -.I 1
> -Print the created or updated multipath names only, for use to feed other tools like kpartx.
> -.TP
> -.I 2 +
> -Print all info: detected paths, coalesced paths (ie multipaths) and device maps.
> -.RE
> -.
> -.TP
> -.B \-h
> -Print usage text.
> -.
> -.TP
> -.B \-d
> -Dry run, do not create or update devmaps.
> +.IP \[bu]
> +A multipath map name.
> +.IP \[bu]
> +A path (low-level device) associated with the desired multipath map; the path may be in one of the following formats:
> +.RS 1.2i
> +.IP \[bu]
> +.B /dev/sdX
> +.IP \[bu]
> +.B major:minor

The device can also be a WWID. Otherwise this looks great.

-Ben

>  .
> -.TP
> -.B \-l
> -Show the current multipath topology from information fetched in sysfs and the device mapper.
> +.\" ----------------------------------------------------------------------------
> +.SH OPERATION MODES
> +.\" ----------------------------------------------------------------------------
>  .
> -.TP
> -.B \-ll
> -Show the current multipath topology from all available information (sysfs, the device mapper, path checkers ...).
> +The default operation mode is to detect and set up multipath maps from the devices found in
> +the system.
>  .
> +Other operation modes are chosen by using one of the following command line switches:
>  .TP
>  .B \-f
> -Flush a multipath device map specified as parameter, if unused.
> +Flush (remove) a multipath device map specified as parameter, if unused.
>  .
>  .TP
>  .B \-F
> -Flush all unused multipath device maps.
> -.
> -.TP
> -.B \-t
> -Display the currently used multipathd configuration.
> +Flush (remove) all unused multipath device maps.
>  .
>  .TP
> -.B \-T
> -Display the currently used multipathd configuration, limiting the output to
> -those devices actually present in the system. This can be used a template for
> -creating \fImultipath.conf\fR.
> +.B \-l
> +Show ("list") the current multipath topology from information fetched in sysfs and the device mapper.
>  .
>  .TP
> -.B \-r
> -Force devmap reload.
> +.B \-ll
> +Show ("list") the current multipath topology from all available information (sysfs, the
> +device mapper, path checkers ...).
>  .
>  .TP
> -.B \-i
> -Ignore WWIDs file when processing devices. If
> -\fIfind_multipaths strict\fR or \fIfind_multipaths no\fR is set in
> -\fImultipath.conf\fR, multipath only considers devices that are
> -listed in the WWIDs file. This option overrides that behavior. For other values
> -of \fIfind_multipaths\fR, this option has no effect. See the description of
> -\fIfind_multipaths\fR in
> -.BR multipath.conf (5).
> -This option should only be used in rare circumstances.
> +.B \-a
> +Add the WWID for the specified device to the WWIDs file.
>  .
>  .TP
> -.B \-B
> -Treat the bindings file as read only.
> +.B \-w
> +Remove the WWID for the specified device from the WWIDs file.
>  .
>  .TP
> -.BI \-b " bindings_file"
> -Set user_friendly_names bindings file location.  The default is
> -\fI/etc/multipath/bindings\fR.
> +.B \-W
> +Reset the WWIDs file to only include the current multipath devices.
>  .
>  .TP
>  .B \-c
> @@ -129,14 +159,6 @@ test whether or not I/O on this device is likely to succeed. The command
>  itself doesn't attempt to do I/O on the device.
>  .
>  .TP
> -.B \-q
> -Allow device tables with \fIqueue_if_no_path\fR when multipathd is not running.
> -.
> -.TP
> -.B \-a
> -Add the WWID for the specified device to the WWIDs file.
> -.
> -.TP
>  .B \-u
>  Check if the device specified in the program environment should be
>  a path in a multipath device.
> @@ -147,60 +169,99 @@ Check if the device specified in the program environment is a multipath device
>  with usable paths. See \fB-C\fB.
>  .
>  .TP
> -.B \-w
> -Remove the WWID for the specified device from the WWIDs file.
> +.B \-h
> +Print usage text.
>  .
>  .TP
> -.B \-W
> -Reset the WWIDs file to only include the current multipath devices.
> +.B \-t
> +Display the currently used multipathd configuration.
>  .
>  .TP
> -.BI \-p " policy"
> -Force new maps to use the specified policy:
> +.B \-T
> +Display the currently used multipathd configuration, limiting the output to
> +those devices actually present in the system. This can be used a template for
> +creating \fImultipath.conf\fR.
> +.
> +.\" ----------------------------------------------------------------------------
> +.SH OPTIONS
> +.\" ----------------------------------------------------------------------------
> +.
> +.TP
> +.BI \-v " level"
> +Verbosity of information printed to stdout in default and "list" operation
> +modes. The default level is \fI-v 2\fR.
>  .RS 1.2i
>  .TP 1.2i
> -.I failover
> -One path per priority group.
> +.I 0
> +Nothing is printed.
>  .TP
> -.I multibus
> -All paths in one priority group.
> +.I 1
> +In default mode, Names/WWIDs of created or modified multipath maps are
> +printed. In list mode, WWIDs of all multipath maps are printed.
>  .TP
> -.I group_by_serial
> -One priority group per serial number.
> +.I 2
> +In default mode,
> +Topology of created or modified multipath maps is printed.
> +In list mode, topology of all multipath maps is printed.
>  .TP
> -.I group_by_prio
> -One priority group per priority value. Priorities are determined by
> -callout programs specified as a global, per-controller or
> -per-multipath option in the configuration file.
> +.I 3
> +All detected paths and the topology of all multipath maps are printed.
> +.
> +.LP
> +.
> +The verbosity level also controls the level of log and debug messages printed to
> +\fIstderr\fR. The default level corresponds to \fILOG_NOTICE\fR
> +(important messages that shouldn't be missed in normal operation).
> +.
> +.RE
>  .TP
> -.I group_by_node_name
> -One priority group per target node name. Target node names are fetched
> -in \fI/sys/class/fc_transport/target*/node_name\fR.
> +.B \-d
> +Dry run, do not create or update devmaps.
> +.
>  .TP
> -.RE
> -Existing maps are not modified.
> +.B \-i
> +Ignore WWIDs file when processing devices. If
> +\fIfind_multipaths strict\fR or \fIfind_multipaths no\fR is set in
> +\fImultipath.conf\fR, multipath only considers devices that are
> +listed in the WWIDs file. This option overrides that behavior. For other values
> +of \fIfind_multipaths\fR, this option has no effect. See the description of
> +\fIfind_multipaths\fR in
> +.BR multipath.conf (5).
> +This option should only be used in rare circumstances.
>  .
>  .TP
> -.BI \-R " retries"
> -Number of times to retry flushing multipath devices that are in-use. The default
> -is \fI0\fR.
> +.B \-B
> +Treat the bindings file as read only.
>  .
>  .TP
> -.BI device
> -Update only the devmap specified by
> -.IR device ,
> -which is either:
> -.RS 1.2i
> -.IP \[bu]
> -A devmap name.
> -.IP \[bu]
> -A path associated with the desired devmap; the path may be in one of the following formats:
> -.RS 1.2i
> -.IP \[bu]
> -.B /dev/sdX
> -.IP \[bu]
> -.B major:minor
> +.BI \-b " file"
> +Set \fIuser_friendly_names\fR bindings file location.  The default is
> +\fI/etc/multipath/bindings\fR.
> +.
> +.TP
> +.B \-q
> +Don't unset the device mapper feature \fIqueue_if_no_path\fR for multipath
> +maps. Normally, \fBmultipath\fR would do so if \fBmultipathd\fR is not
> +running, because only a running multipath daemon guarantees that unusable
> +paths are reinstated when they become usable again.
> +.
> +.TP
> +.BI \-p " policy"
> +Force new maps to use the specified policy, overriding the configuration in
> +\fBmultipath.conf(5)\fR. The possible values for
> +\fIpolicy\fR are the same as the values for \fIpath_grouping_policy\fR in
> +\fBmultipath.conf(5)\fR. Existing maps are not modified.
> +.
> +.TP
> +.B \-r
> +Force a reload of all existing multipath maps. This command is delegated to
> +the multipathd daemon if it's running. In this case, other command line
> +switches of the \fImultipath\fR command have no effect.
>  .
> +.TP
> +.BI \-R " retries"
> +Number of times to retry flushing multipath devices that are in use. The default
> +is \fI0\fR.
>  .
>  .\" ----------------------------------------------------------------------------
>  .SH "SEE ALSO"
> -- 
> 2.19.0

[PATCH 6/8] libmultipath: test code for "zombie" TUR threads

[Martin Wilck]

This patch adds test code that simulates hanging, non-cancellable
TUR threads, for testing multipathd's behavior in that situation.
Compile libmultipath with CFLAGS=-DTUR_TEST_MAJOR=8 to activate
the test code. Without that flag, the patch has no effect.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/checkers/tur.c | 45 +++++++++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

diff --git a/libmultipath/checkers/tur.c b/libmultipath/checkers/tur.c
index b2a21707..a986a244 100644
--- a/libmultipath/checkers/tur.c
+++ b/libmultipath/checkers/tur.c
@@ -196,6 +196,50 @@ static void cleanup_func(void *data)
 	rcu_unregister_thread();
 }
 
+/*
+ * Test code for "zombie tur thread" handling.
+ * Compile e.g. with CFLAGS=-DTUR_TEST_MAJOR=8
+ * Additional parameters can be configure with the macros below.
+ *
+ * Everty nth started TUR thread will hang in non-cancellable state
+ * for given number of seconds, for device given by major/minor.
+ */
+#ifdef TUR_TEST_MAJOR
+
+#ifndef TUR_TEST_MINOR
+#define TUR_TEST_MINOR 0
+#endif
+#ifndef TUR_SLEEP_INTERVAL
+#define TUR_SLEEP_INTERVAL 3
+#endif
+#ifndef TUR_SLEEP_SECS
+#define TUR_SLEEP_SECS 60
+#endif
+
+static void tur_deep_sleep(const struct tur_checker_context *ct)
+{
+	static int sleep_cnt;
+	const struct timespec ts = { .tv_sec = TUR_SLEEP_SECS, .tv_nsec = 0 };
+	int oldstate;
+
+	if (ct->devt != makedev(TUR_TEST_MAJOR, TUR_TEST_MINOR) ||
+	    ++sleep_cnt % TUR_SLEEP_INTERVAL != 0)
+		return;
+
+	condlog(1, "tur thread going to sleep for %ld seconds", ts.tv_sec);
+	if (pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, &oldstate) != 0)
+		condlog(0, "pthread_setcancelstate: %m");
+	if (nanosleep(&ts, NULL) != 0)
+		condlog(0, "nanosleep: %m");
+	condlog(1, "tur zombie thread woke up");
+	if (pthread_setcancelstate(oldstate, NULL) != 0)
+		condlog(0, "pthread_setcancelstate (2): %m");
+	pthread_testcancel();
+}
+#else
+#define tur_deep_sleep(x) do {} while (0)
+#endif /* TUR_TEST_MAJOR */
+
 static void *tur_thread(void *ctx)
 {
 	struct tur_checker_context *ct = ctx;
@@ -209,6 +253,7 @@ static void *tur_thread(void *ctx)
 	condlog(3, "%d:%d : tur checker starting up", major(ct->devt),
 		minor(ct->devt));
 
+	tur_deep_sleep(ct);
 	state = tur_check(ct->fd, ct->timeout, msg);
 	pthread_testcancel();
 
-- 
2.19.0

Re: [PATCH 6/8] libmultipath: test code for "zombie" TUR threads

[Benjamin Marzinski]

On Wed, Oct 10, 2018 at 10:05:04PM +0200, Martin Wilck wrote:
> This patch adds test code that simulates hanging, non-cancellable
> TUR threads, for testing multipathd's behavior in that situation.
> Compile libmultipath with CFLAGS=-DTUR_TEST_MAJOR=8 to activate
> the test code. Without that flag, the patch has no effect.
> 

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>

> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/checkers/tur.c | 45 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 45 insertions(+)
> 
> diff --git a/libmultipath/checkers/tur.c b/libmultipath/checkers/tur.c
> index b2a21707..a986a244 100644
> --- a/libmultipath/checkers/tur.c
> +++ b/libmultipath/checkers/tur.c
> @@ -196,6 +196,50 @@ static void cleanup_func(void *data)
>  	rcu_unregister_thread();
>  }
>  
> +/*
> + * Test code for "zombie tur thread" handling.
> + * Compile e.g. with CFLAGS=-DTUR_TEST_MAJOR=8
> + * Additional parameters can be configure with the macros below.
> + *
> + * Everty nth started TUR thread will hang in non-cancellable state
> + * for given number of seconds, for device given by major/minor.
> + */
> +#ifdef TUR_TEST_MAJOR
> +
> +#ifndef TUR_TEST_MINOR
> +#define TUR_TEST_MINOR 0
> +#endif
> +#ifndef TUR_SLEEP_INTERVAL
> +#define TUR_SLEEP_INTERVAL 3
> +#endif
> +#ifndef TUR_SLEEP_SECS
> +#define TUR_SLEEP_SECS 60
> +#endif
> +
> +static void tur_deep_sleep(const struct tur_checker_context *ct)
> +{
> +	static int sleep_cnt;
> +	const struct timespec ts = { .tv_sec = TUR_SLEEP_SECS, .tv_nsec = 0 };
> +	int oldstate;
> +
> +	if (ct->devt != makedev(TUR_TEST_MAJOR, TUR_TEST_MINOR) ||
> +	    ++sleep_cnt % TUR_SLEEP_INTERVAL != 0)
> +		return;
> +
> +	condlog(1, "tur thread going to sleep for %ld seconds", ts.tv_sec);
> +	if (pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, &oldstate) != 0)
> +		condlog(0, "pthread_setcancelstate: %m");
> +	if (nanosleep(&ts, NULL) != 0)
> +		condlog(0, "nanosleep: %m");
> +	condlog(1, "tur zombie thread woke up");
> +	if (pthread_setcancelstate(oldstate, NULL) != 0)
> +		condlog(0, "pthread_setcancelstate (2): %m");
> +	pthread_testcancel();
> +}
> +#else
> +#define tur_deep_sleep(x) do {} while (0)
> +#endif /* TUR_TEST_MAJOR */
> +
>  static void *tur_thread(void *ctx)
>  {
>  	struct tur_checker_context *ct = ctx;
> @@ -209,6 +253,7 @@ static void *tur_thread(void *ctx)
>  	condlog(3, "%d:%d : tur checker starting up", major(ct->devt),
>  		minor(ct->devt));
>  
> +	tur_deep_sleep(ct);
>  	state = tur_check(ct->fd, ct->timeout, msg);
>  	pthread_testcancel();
>  
> -- 
> 2.19.0

[PATCH 7/8] libmultipath: handle TUR threads that can't be cancelled

[Martin Wilck]

When the tur checker code determines that a hanging TUR thread
couldn't be cancelled, rather than simply returning, reallocate
the checker context and start a new thread. This will leak some
memory if the hanging thread never wakes up again, but well, in
that highly unlikely case we're leaking threads anyway.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/checkers/tur.c | 24 +++++++++++++++++++++---
 1 file changed, 21 insertions(+), 3 deletions(-)

diff --git a/libmultipath/checkers/tur.c b/libmultipath/checkers/tur.c
index a986a244..9ecca5bd 100644
--- a/libmultipath/checkers/tur.c
+++ b/libmultipath/checkers/tur.c
@@ -349,11 +349,29 @@ int libcheck_check(struct checker * c)
 		}
 	} else {
 		if (uatomic_read(&ct->holders) > 1) {
-			/* The thread has been cancelled but hasn't
-			 * quit. exit with timeout. */
+			int holders;
+
+			/*
+			 * The thread has been cancelled but hasn't quit.
+			 * We have to prevent it from interfering with the new
+			 * thread. We create a new context and leave the old
+			 * one with the stale thread, hoping it will clean up
+			 * eventually.
+			 */
 			condlog(3, "%d:%d : tur thread not responding",
 				major(ct->devt), minor(ct->devt));
-			return PATH_TIMEOUT;
+
+			/* libcheck_init will replace c->context */
+			libcheck_init(c);
+
+			holders = uatomic_sub_return(&ct->holders, 1);
+			if (!holders)
+				/* It did terminate, eventually */
+				cleanup_context(ct);
+
+			ct = c->context;
+			if (ct == NULL)
+				return PATH_UNCHECKED;
 		}
 		/* Start new TUR checker */
 		pthread_mutex_lock(&ct->lock);
-- 
2.19.0

Re: [PATCH 7/8] libmultipath: handle TUR threads that can't be cancelled

[Benjamin Marzinski]

On Wed, Oct 10, 2018 at 10:05:05PM +0200, Martin Wilck wrote:
> When the tur checker code determines that a hanging TUR thread
> couldn't be cancelled, rather than simply returning, reallocate
> the checker context and start a new thread. This will leak some
> memory if the hanging thread never wakes up again, but well, in
> that highly unlikely case we're leaking threads anyway.
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/checkers/tur.c | 24 +++++++++++++++++++++---
>  1 file changed, 21 insertions(+), 3 deletions(-)
> 
> diff --git a/libmultipath/checkers/tur.c b/libmultipath/checkers/tur.c
> index a986a244..9ecca5bd 100644
> --- a/libmultipath/checkers/tur.c
> +++ b/libmultipath/checkers/tur.c
> @@ -349,11 +349,29 @@ int libcheck_check(struct checker * c)
>  		}
>  	} else {
>  		if (uatomic_read(&ct->holders) > 1) {
> -			/* The thread has been cancelled but hasn't
> -			 * quit. exit with timeout. */
> +			int holders;
> +
> +			/*
> +			 * The thread has been cancelled but hasn't quit.
> +			 * We have to prevent it from interfering with the new
> +			 * thread. We create a new context and leave the old
> +			 * one with the stale thread, hoping it will clean up
> +			 * eventually.
> +			 */
>  			condlog(3, "%d:%d : tur thread not responding",
>  				major(ct->devt), minor(ct->devt));
> -			return PATH_TIMEOUT;
> +
> +			/* libcheck_init will replace c->context */
> +			libcheck_init(c);
> +
> +			holders = uatomic_sub_return(&ct->holders, 1);
> +			if (!holders)
> +				/* It did terminate, eventually */
> +				cleanup_context(ct);
> +
> +			ct = c->context;
> +			if (ct == NULL)
> +				return PATH_UNCHECKED;

libcheck_init can fail to allocate the checker_context, and return 1.
If it does so, it won't reset c->context.  In this case, you will unhold
the context but keep using it.  Instead we should fail right after the
call to libcheck_init if it returns 1.

Also, returning PATH_UNCHECKED triggers code in check_path that assumes
there is a problem with the path. It recalls pathinfo(), and does not do
all the work that would happen on PATH_DOWN. PATH_TIMEOUT works like
PATH_DOWN and PATH_SHAKY, which seems like the right thing to do here.

-Ben

>  		}
>  		/* Start new TUR checker */
>  		pthread_mutex_lock(&ct->lock);
> -- 
> 2.19.0

Re: [PATCH 7/8] libmultipath: handle TUR threads that can't be cancelled

[Martin Wilck]

On Fri, 2018-10-12 at 17:11 -0500,  Benjamin Marzinski  wrote:
> On Wed, Oct 10, 2018 at 10:05:05PM +0200, Martin Wilck wrote:
> > When the tur checker code determines that a hanging TUR thread
> > couldn't be cancelled, rather than simply returning, reallocate
> > the checker context and start a new thread. This will leak some
> > memory if the hanging thread never wakes up again, but well, in
> > that highly unlikely case we're leaking threads anyway.
> > 
> > Signed-off-by: Martin Wilck <mwilck@suse.com>
> > ---
> >  libmultipath/checkers/tur.c | 24 +++++++++++++++++++++---
> >  1 file changed, 21 insertions(+), 3 deletions(-)
> > 
> > diff --git a/libmultipath/checkers/tur.c
> > b/libmultipath/checkers/tur.c
> > index a986a244..9ecca5bd 100644
> > --- a/libmultipath/checkers/tur.c
> > +++ b/libmultipath/checkers/tur.c
> > @@ -349,11 +349,29 @@ int libcheck_check(struct checker * c)
> >  		}
> >  	} else {
> >  		if (uatomic_read(&ct->holders) > 1) {
> > -			/* The thread has been cancelled but hasn't
> > -			 * quit. exit with timeout. */
> > +			int holders;
> > +
> > +			/*
> > +			 * The thread has been cancelled but hasn't
> > quit.
> > +			 * We have to prevent it from interfering with
> > the new
> > +			 * thread. We create a new context and leave
> > the old
> > +			 * one with the stale thread, hoping it will
> > clean up
> > +			 * eventually.
> > +			 */
> >  			condlog(3, "%d:%d : tur thread not responding",
> >  				major(ct->devt), minor(ct->devt));
> > -			return PATH_TIMEOUT;
> > +
> > +			/* libcheck_init will replace c->context */
> > +			libcheck_init(c);
> > +
> > +			holders = uatomic_sub_return(&ct->holders, 1);
> > +			if (!holders)
> > +				/* It did terminate, eventually */
> > +				cleanup_context(ct);
> > +
> > +			ct = c->context;
> > +			if (ct == NULL)
> > +				return PATH_UNCHECKED;
> 
> libcheck_init can fail to allocate the checker_context, and return 1.
> If it does so, it won't reset c->context.  In this case, you will
> unhold
> the context but keep using it.  Instead we should fail right after
> the
> call to libcheck_init if it returns 1.

OK, I'll resubmit with this change.

> Also, returning PATH_UNCHECKED triggers code in check_path that
> assumes
> there is a problem with the path. It recalls pathinfo(), and does not
> do
> all the work that would happen on PATH_DOWN. PATH_TIMEOUT works like
> PATH_DOWN and PATH_SHAKY, which seems like the right thing to do
> here.

I am unsure about that. if libcheck_init() fails, we are out of memory.
This is a pretty hopeless situation anyway, but it does not indicate
(by itself) that the path is down. Proactively failing the path in DM
is dangerous; it might remove the last working path and cause file
systems to go read-only. We should only do that if we're positive that
the path is bad. Are we positive that a stalled TUR thread can only
happen if a path is broken? Maybe the thread is stalled because system
is thrashing?

Martin

-- 
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

[PATCH 8/8] multipathd: handle repeated udev retrigger failure

[Martin Wilck]

If a path was still not properly initialized after exhausting the
retrigger tries, it used to remain in INIT_MISSING_UDEV state forever.
get_uid() might fall back to non-udev-based methods to determine
the WWID, but it would never be called for a path in this state any more.

This patch changes this behavior by resetting the path back to FAILED
state if udev can't provide the WWID even after retriggering. Now, if
the path ever happens to be in PATH_UP or PATH_GHOST state again,
pathinfo(DI_ALL) will be called from check_path(), and there's at least
some chance to obtain a WWID for it.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 multipathd/main.c | 32 +++++++++++++++++++++++---------
 1 file changed, 23 insertions(+), 9 deletions(-)

diff --git a/multipathd/main.c b/multipathd/main.c
index 2d45d989..a9e1a4bd 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -1828,15 +1828,29 @@ check_path (struct vectors * vecs, struct path * pp, int ticks)
 	retrigger_tries = conf->retrigger_tries;
 	checkint = conf->checkint;
 	put_multipath_config(conf);
-	if (!pp->mpp && pp->initialized == INIT_MISSING_UDEV &&
-	    pp->retriggers < retrigger_tries) {
-		condlog(2, "%s: triggering change event to reinitialize",
-			pp->dev);
-		pp->initialized = INIT_REQUESTED_UDEV;
-		pp->retriggers++;
-		sysfs_attr_set_value(pp->udev, "uevent", "change",
-				     strlen("change"));
-		return 0;
+	if (!pp->mpp && pp->initialized == INIT_MISSING_UDEV) {
+		if (pp->retriggers < retrigger_tries) {
+			condlog(2, "%s: triggering change event to reinitialize",
+				pp->dev);
+			pp->initialized = INIT_REQUESTED_UDEV;
+			pp->retriggers++;
+			sysfs_attr_set_value(pp->udev, "uevent", "change",
+					     strlen("change"));
+			return 0;
+		} else {
+			condlog(1, "%s: not initialized after %d udev retriggers",
+				pp->dev, retrigger_tries);
+			/*
+			 * Make sure that the "add missing path" code path
+			 * below may reinstate the path later, if it ever
+			 * comes up again.
+			 * The WWID needs not be cleared; if it was set, the
+			 * state hadn't been INIT_MISSING_UDEV in the first
+			 * place.
+			 */
+			pp->initialized = INIT_FAILED;
+			return 0;
+		}
 	}
 
 	/*
-- 
2.19.0

Re: [PATCH 8/8] multipathd: handle repeated udev retrigger failure

[Benjamin Marzinski]

On Wed, Oct 10, 2018 at 10:05:06PM +0200, Martin Wilck wrote:
> If a path was still not properly initialized after exhausting the
> retrigger tries, it used to remain in INIT_MISSING_UDEV state forever.
> get_uid() might fall back to non-udev-based methods to determine
> the WWID, but it would never be called for a path in this state any more.
> 
> This patch changes this behavior by resetting the path back to FAILED
> state if udev can't provide the WWID even after retriggering. Now, if
> the path ever happens to be in PATH_UP or PATH_GHOST state again,
> pathinfo(DI_ALL) will be called from check_path(), and there's at least
> some chance to obtain a WWID for it.

What you have seems reasonable, but looking at the code that calls
pathinfo() in this case, I do have some related suggestions.

Since check_path() always returns early if !pp->mpp and
pp->initialized != INIT_FAILED, we don't need the
pp->initialized != INIT_MISSING_UDEV check before doing the pathinfo
(although we might want to check pp->initialized == INIT_FAILED, in case
we add other pp->initialized states in the future).

Also, I don't think that checking if pathinfo returns PATHINFO_OK is the
right thing to do before calling ev_add_path(). pathinfo() will return
PATHINFO_OK even if we fail to get the wwid. I'm pretty sure this should
check if pp->initialized == INIT_OK instead.

Finally, if pp->initialized != INIT_OK after the call to pathinfo(),
would you object to setting pp->tick to max_checkint so that we don't
keep checking the path as often.  It is possible that this really is a
device that shouldn't be multipathed, and for which we will never be
able to get a wwid. It seems like after some many failures, we don't
need to be so proactive about rechecking an active path where we simply
aren't getting the wwid.

-Ben

> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  multipathd/main.c | 32 +++++++++++++++++++++++---------
>  1 file changed, 23 insertions(+), 9 deletions(-)
> 
> diff --git a/multipathd/main.c b/multipathd/main.c
> index 2d45d989..a9e1a4bd 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -1828,15 +1828,29 @@ check_path (struct vectors * vecs, struct path * pp, int ticks)
>  	retrigger_tries = conf->retrigger_tries;
>  	checkint = conf->checkint;
>  	put_multipath_config(conf);
> -	if (!pp->mpp && pp->initialized == INIT_MISSING_UDEV &&
> -	    pp->retriggers < retrigger_tries) {
> -		condlog(2, "%s: triggering change event to reinitialize",
> -			pp->dev);
> -		pp->initialized = INIT_REQUESTED_UDEV;
> -		pp->retriggers++;
> -		sysfs_attr_set_value(pp->udev, "uevent", "change",
> -				     strlen("change"));
> -		return 0;
> +	if (!pp->mpp && pp->initialized == INIT_MISSING_UDEV) {
> +		if (pp->retriggers < retrigger_tries) {
> +			condlog(2, "%s: triggering change event to reinitialize",
> +				pp->dev);
> +			pp->initialized = INIT_REQUESTED_UDEV;
> +			pp->retriggers++;
> +			sysfs_attr_set_value(pp->udev, "uevent", "change",
> +					     strlen("change"));
> +			return 0;
> +		} else {
> +			condlog(1, "%s: not initialized after %d udev retriggers",
> +				pp->dev, retrigger_tries);
> +			/*
> +			 * Make sure that the "add missing path" code path
> +			 * below may reinstate the path later, if it ever
> +			 * comes up again.
> +			 * The WWID needs not be cleared; if it was set, the
> +			 * state hadn't been INIT_MISSING_UDEV in the first
> +			 * place.
> +			 */
> +			pp->initialized = INIT_FAILED;
> +			return 0;
> +		}
>  	}
>  
>  	/*
> -- 
> 2.19.0

Re: [PATCH 0/8] various multipath-tools patches

[Christophe Varoqui]

[-- Attachment #1.1: Type: text/plain, Size: 1267 bytes --]

Martin,

The 01 to 06 patchs of this set are now merged.

Thanks,
Christophe.

On Wed, Oct 10, 2018 at 10:05 PM Martin Wilck <mwilck@suse.com> wrote:

> Hi Christophe,
>
> this series contains some patches I had on my list for
> some time,  plus some stuff that came up during the discussion
> about Ben's last large patch series.
>
> This goes on top of the "[PATCH 0/6] More misc multipath patches"
> series that Ben sent today.
>
> Regards,
> Martin
>
> Martin Wilck (8):
>   test-kpartx: add test for invocation on image file
>   libmultipath: scsi_ioctl_pathinfo doesn't fail
>   libmultipath: cciss_ioctl_pathinfo doesn't fail
>   multipath.rules: add comment about DM_MULTIPATH_DEVICE_PATH=2
>   multipath.8: man page update
>   libmultipath: test code for "zombie" TUR threads
>   libmultipath: handle TUR threads that can't be cancelled
>   multipathd: handle repeated udev retrigger failure
>
>  kpartx/test-kpartx          |  24 +++-
>  libmultipath/checkers/tur.c |  69 ++++++++-
>  libmultipath/discovery.c    |  33 ++---
>  multipath/multipath.8       | 273 ++++++++++++++++++++++--------------
>  multipath/multipath.rules   |   4 +
>  multipathd/main.c           |  32 +++--
>  6 files changed, 297 insertions(+), 138 deletions(-)
>
> --
> 2.19.0
>
>

[-- Attachment #1.2: Type: text/html, Size: 1744 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]