[dm-devel] [RFC PATCH 0/4] Make prio timeouts work like checkers

[dm-devel] [RFC PATCH 0/4] Make prio timeouts work like checkers

[Benjamin Marzinski]

This patchset changes how prioritizers set their timeouts, to make them
match how the checker functions work, and also cleans up some minor
timeout issues. I did this to make out timeouts consistent, but if
someone has a good reason to object to that, I don't feel
strongly that it's necessary, and I can resend just the bugfixes.

Benjamin Marzinski (4):
  libmultipath: don't bother to reset default timeout value
  libmultipath: make prioritizer timeouts work like checker timeouts
  libmultipath: fix timeouts for detect_alua()
  libmultipath: fix timeouts for path_latency prioritizer

 libmultipath/discovery.c                 | 13 +++++++------
 libmultipath/prio.c                      | 14 +++++++++-----
 libmultipath/prio.h                      |  3 +--
 libmultipath/prioritizers/alua.c         |  2 +-
 libmultipath/prioritizers/alua_rtpg.c    |  5 ++---
 libmultipath/prioritizers/emc.c          |  5 +++--
 libmultipath/prioritizers/hds.c          |  4 ++--
 libmultipath/prioritizers/hp_sw.c        |  4 ++--
 libmultipath/prioritizers/ontap.c        |  7 +++----
 libmultipath/prioritizers/path_latency.c |  5 +++--
 libmultipath/prioritizers/rdac.c         |  4 ++--
 11 files changed, 35 insertions(+), 31 deletions(-)

-- 
2.17.2

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

[dm-devel] [RFC PATCH 1/4] libmultipath: don't bother to reset default timeout value

[Benjamin Marzinski]

by the time get_state() is rechecking the sysfs timeout value,
c->timeout has already been set.  The only reason why this check exists
is to deal with the possiblity that the sysfs value has changed. If the
sysfs value doesn't exist (which likely means that the device is not a
scsi device), then there's no reason to reset the default value, since
that can't have changed.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/discovery.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
index 5626d48d..2b1a11d5 100644
--- a/libmultipath/discovery.c
+++ b/libmultipath/discovery.c
@@ -1965,9 +1965,8 @@ get_state (struct path * pp, struct config *conf, int daemon, int oldstate)
 		checker_set_async(c);
 	else
 		checker_set_sync(c);
-	if (!conf->checker_timeout &&
-	    sysfs_get_timeout(pp, &(c->timeout)) <= 0)
-		c->timeout = DEF_TIMEOUT;
+	if (!conf->checker_timeout)
+	    sysfs_get_timeout(pp, &(c->timeout));
 	state = checker_check(c, oldstate);
 	condlog(3, "%s: %s state = %s", pp->dev,
 		checker_name(c), checker_state_name(state));
-- 
2.17.2

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

Re: [dm-devel] [RFC PATCH 1/4] libmultipath: don't bother to reset default timeout value

[Martin Wilck]

On Fri, 2023-07-28 at 14:05 -0500, Benjamin Marzinski wrote:
> by the time get_state() is rechecking the sysfs timeout value,
> c->timeout has already been set.  The only reason why this check
> exists
> is to deal with the possiblity that the sysfs value has changed. If
> the
> sysfs value doesn't exist (which likely means that the device is not
> a
> scsi device), then there's no reason to reset the default value,
> since
> that can't have changed.
> 
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>

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

Thinking about it, I am not sure if it's wise to re-read the timeout
from sysfs every time we retrieve a path state. It's inefficient, and I
wonder if we even want to do this if someone modifies SCSI timeouts in
sysfs while multipathd is running. It would be cleaner to set
checker_timeout and reload multipathd.

But that's no reason to reject this patch.

> ---
>  libmultipath/discovery.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
> index 5626d48d..2b1a11d5 100644
> --- a/libmultipath/discovery.c
> +++ b/libmultipath/discovery.c
> @@ -1965,9 +1965,8 @@ get_state (struct path * pp, struct config
> *conf, int daemon, int oldstate)
>                 checker_set_async(c);
>         else
>                 checker_set_sync(c);
> -       if (!conf->checker_timeout &&
> -           sysfs_get_timeout(pp, &(c->timeout)) <= 0)
> -               c->timeout = DEF_TIMEOUT;
> +       if (!conf->checker_timeout)
> +           sysfs_get_timeout(pp, &(c->timeout));
>         state = checker_check(c, oldstate);
>         condlog(3, "%s: %s state = %s", pp->dev,
>                 checker_name(c), checker_state_name(state));

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

[dm-devel] [RFC PATCH 2/4] libmultipath: make prioritizer timeouts work like checker timeouts

[Benjamin Marzinski]

Multipath's prioritizers previously didn't use the sysfs timeout for
scsi devices, and used a prioritizer specific default timeout (although
in practice, all the prioritizers except hds used 60 seconds). Now
prioritizers deal with timeouts the same way as the checkers. They first
use checker_time if set, then use the sysfs tiemout for scsi devices, or
30 seconds for non-scsi devices.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/prio.c                   | 14 +++++++++-----
 libmultipath/prio.h                   |  3 +--
 libmultipath/prioritizers/alua.c      |  2 +-
 libmultipath/prioritizers/alua_rtpg.c |  5 ++---
 libmultipath/prioritizers/emc.c       |  5 +++--
 libmultipath/prioritizers/hds.c       |  4 ++--
 libmultipath/prioritizers/hp_sw.c     |  4 ++--
 libmultipath/prioritizers/ontap.c     |  7 +++----
 libmultipath/prioritizers/rdac.c      |  4 ++--
 9 files changed, 25 insertions(+), 23 deletions(-)

diff --git a/libmultipath/prio.c b/libmultipath/prio.c
index cdd37529..69b71578 100644
--- a/libmultipath/prio.c
+++ b/libmultipath/prio.c
@@ -3,20 +3,24 @@
 #include <stddef.h>
 #include <dlfcn.h>
 #include <sys/stat.h>
+#include <libudev.h>
 
 #include "debug.h"
 #include "util.h"
 #include "prio.h"
+#include "structs.h"
+#include "discovery.h"
 
 static const char * const prio_dir = MULTIPATH_DIR;
 static LIST_HEAD(prioritizers);
 
-unsigned int get_prio_timeout(unsigned int timeout_ms,
-			      unsigned int default_timeout)
+unsigned int get_prio_timeout(struct path *pp, unsigned int timeout)
 {
-	if (timeout_ms)
-		return timeout_ms;
-	return default_timeout;
+	if (timeout)
+		return timeout;
+	timeout = DEF_TIMEOUT;
+	sysfs_get_timeout(pp, &timeout);
+	return timeout * 1000;
 }
 
 int init_prio(void)
diff --git a/libmultipath/prio.h b/libmultipath/prio.h
index 184bf65f..20808fd7 100644
--- a/libmultipath/prio.h
+++ b/libmultipath/prio.h
@@ -52,8 +52,7 @@ struct prio {
 	int (*getprio)(struct path *, char *, unsigned int);
 };
 
-unsigned int get_prio_timeout(unsigned int checker_timeout,
-			      unsigned int default_timeout);
+unsigned int get_prio_timeout(struct path *pp, unsigned int checker_timeout);
 int init_prio(void);
 void cleanup_prio (void);
 struct prio * add_prio (const char *);
diff --git a/libmultipath/prioritizers/alua.c b/libmultipath/prioritizers/alua.c
index a28bca05..d3ba367f 100644
--- a/libmultipath/prioritizers/alua.c
+++ b/libmultipath/prioritizers/alua.c
@@ -109,7 +109,7 @@ int getprio (struct path * pp, char * args, unsigned int timeout)
 		return -ALUA_PRIO_NO_INFORMATION;
 
 	exclusive_pref = get_exclusive_pref_arg(args);
-	rc = get_alua_info(pp, timeout);
+	rc = get_alua_info(pp, get_prio_timeout(pp, timeout));
 	if (rc >= 0) {
 		aas = (rc & 0x0f);
 		priopath = (rc & 0x80);
diff --git a/libmultipath/prioritizers/alua_rtpg.c b/libmultipath/prioritizers/alua_rtpg.c
index 2db91536..49982545 100644
--- a/libmultipath/prioritizers/alua_rtpg.c
+++ b/libmultipath/prioritizers/alua_rtpg.c
@@ -31,7 +31,6 @@
 #include "alua_rtpg.h"
 
 #define SENSE_BUFF_LEN  32
-#define SGIO_TIMEOUT     60000
 
 #define PRINT_DEBUG(f, a...) \
 	condlog(4, "alua: " f, ##a)
@@ -162,7 +161,7 @@ retry:
 	hdr.dxfer_len		= resplen;
 	hdr.sbp			= sense;
 	hdr.mx_sb_len		= sizeof(sense);
-	hdr.timeout		= get_prio_timeout(timeout, SGIO_TIMEOUT);
+	hdr.timeout		= timeout;
 
 	if (ioctl(fd, SG_IO, &hdr) < 0) {
 		PRINT_DEBUG("do_inquiry: IOCTL failed!");
@@ -316,7 +315,7 @@ retry:
 	hdr.dxfer_len		= resplen;
 	hdr.mx_sb_len		= sizeof(sense);
 	hdr.sbp			= sense;
-	hdr.timeout		= get_prio_timeout(timeout, SGIO_TIMEOUT);
+	hdr.timeout		= timeout;
 
 	if (ioctl(fd, SG_IO, &hdr) < 0) {
 		condlog(2, "%s: sg ioctl failed: %s",
diff --git a/libmultipath/prioritizers/emc.c b/libmultipath/prioritizers/emc.c
index 3b63cca0..97fde31b 100644
--- a/libmultipath/prioritizers/emc.c
+++ b/libmultipath/prioritizers/emc.c
@@ -31,7 +31,7 @@ int emc_clariion_prio(const char *dev, int fd, unsigned int timeout)
 	io_hdr.dxferp = sense_buffer;
 	io_hdr.cmdp = inqCmdBlk;
 	io_hdr.sbp = sb;
-	io_hdr.timeout = get_prio_timeout(timeout, 60000);
+	io_hdr.timeout = timeout;
 	io_hdr.pack_id = 0;
 	if (ioctl(fd, SG_IO, &io_hdr) < 0) {
 		pp_emc_log(0, "sending query command failed");
@@ -84,5 +84,6 @@ out:
 int getprio (struct path *pp, __attribute__((unused)) char *args,
 	     unsigned int timeout)
 {
-	return emc_clariion_prio(pp->dev, pp->fd, timeout);
+	return emc_clariion_prio(pp->dev, pp->fd,
+				 get_prio_timeout(pp, timeout));
 }
diff --git a/libmultipath/prioritizers/hds.c b/libmultipath/prioritizers/hds.c
index d569f2d7..13f497cc 100644
--- a/libmultipath/prioritizers/hds.c
+++ b/libmultipath/prioritizers/hds.c
@@ -114,7 +114,7 @@ int hds_modular_prio (const char *dev, int fd, unsigned int timeout)
 	io_hdr.dxferp = inqBuff;
 	io_hdr.cmdp = inqCmdBlk;
 	io_hdr.sbp = sense_buffer;
-	io_hdr.timeout = get_prio_timeout(timeout, 2000); /* TimeOut = 2 seconds */
+	io_hdr.timeout = timeout;
 
 	if (ioctl (fd, SG_IO, &io_hdr) < 0) {
 		pp_hds_log(0, "SG_IO error");
@@ -171,5 +171,5 @@ int hds_modular_prio (const char *dev, int fd, unsigned int timeout)
 int getprio (struct path * pp, __attribute__((unused)) char *args,
 	     unsigned int timeout)
 {
-	return hds_modular_prio(pp->dev, pp->fd, timeout);
+	return hds_modular_prio(pp->dev, pp->fd, get_prio_timeout(pp, timeout));
 }
diff --git a/libmultipath/prioritizers/hp_sw.c b/libmultipath/prioritizers/hp_sw.c
index 5b85ad2e..b4cbc58f 100644
--- a/libmultipath/prioritizers/hp_sw.c
+++ b/libmultipath/prioritizers/hp_sw.c
@@ -46,7 +46,7 @@ int hp_sw_prio(const char *dev, int fd, unsigned int timeout)
 	io_hdr.dxfer_direction = SG_DXFER_NONE;
 	io_hdr.cmdp = turCmdBlk;
 	io_hdr.sbp = sb;
-	io_hdr.timeout = get_prio_timeout(timeout, 60000);
+	io_hdr.timeout = timeout;
 	io_hdr.pack_id = 0;
 retry:
 	if (ioctl(fd, SG_IO, &io_hdr) < 0) {
@@ -98,5 +98,5 @@ out:
 int getprio (struct path *pp, __attribute__((unused)) char *args,
 	     unsigned int timeout)
 {
-	return hp_sw_prio(pp->dev, pp->fd, timeout);
+	return hp_sw_prio(pp->dev, pp->fd, get_prio_timeout(pp, timeout));
 }
diff --git a/libmultipath/prioritizers/ontap.c b/libmultipath/prioritizers/ontap.c
index 262e69d2..b9860974 100644
--- a/libmultipath/prioritizers/ontap.c
+++ b/libmultipath/prioritizers/ontap.c
@@ -28,7 +28,6 @@
 #define INQUIRY_CMDLEN	6
 #define DEFAULT_PRIOVAL	10
 #define RESULTS_MAX	256
-#define SG_TIMEOUT	60000
 
 #define pp_ontap_log(prio, fmt, args...) \
 	condlog(prio, "%s: ontap prio: " fmt, dev, ##args)
@@ -90,7 +89,7 @@ static int send_gva(const char *dev, int fd, unsigned char pg,
 	io_hdr.dxferp = results;
 	io_hdr.cmdp = cdb;
 	io_hdr.sbp = sb;
-	io_hdr.timeout = get_prio_timeout(timeout, SG_TIMEOUT);
+	io_hdr.timeout = timeout;
 	io_hdr.pack_id = 0;
 	if (ioctl(fd, SG_IO, &io_hdr) < 0) {
 		pp_ontap_log(0, "SG_IO ioctl failed, errno=%d", errno);
@@ -142,7 +141,7 @@ static int get_proxy(const char *dev, int fd, unsigned int timeout)
 	io_hdr.dxferp = results;
 	io_hdr.cmdp = cdb;
 	io_hdr.sbp = sb;
-	io_hdr.timeout = get_prio_timeout(timeout, SG_TIMEOUT);
+	io_hdr.timeout = timeout;
 	io_hdr.pack_id = 0;
 	if (ioctl(fd, SG_IO, &io_hdr) < 0) {
 		pp_ontap_log(0, "ioctl sending inquiry command failed, "
@@ -244,5 +243,5 @@ prio_select:
 int getprio (struct path *pp, __attribute__((unused)) char *args,
 	     unsigned int timeout)
 {
-	return ontap_prio(pp->dev, pp->fd, timeout);
+	return ontap_prio(pp->dev, pp->fd, get_prio_timeout(pp, timeout));
 }
diff --git a/libmultipath/prioritizers/rdac.c b/libmultipath/prioritizers/rdac.c
index 92a2fb85..0faa8155 100644
--- a/libmultipath/prioritizers/rdac.c
+++ b/libmultipath/prioritizers/rdac.c
@@ -31,7 +31,7 @@ int rdac_prio(const char *dev, int fd, unsigned int timeout)
 	io_hdr.dxferp = sense_buffer;
 	io_hdr.cmdp = inqCmdBlk;
 	io_hdr.sbp = sb;
-	io_hdr.timeout = get_prio_timeout(timeout, 60000);
+	io_hdr.timeout = timeout;
 	io_hdr.pack_id = 0;
 	if (ioctl(fd, SG_IO, &io_hdr) < 0) {
 		pp_rdac_log(0, "sending inquiry command failed");
@@ -94,5 +94,5 @@ out:
 int getprio (struct path *pp, __attribute__((unused)) char *args,
 	     unsigned int timeout)
 {
-	return rdac_prio(pp->dev, pp->fd, timeout);
+	return rdac_prio(pp->dev, pp->fd, get_prio_timeout(pp, timeout));
 }
-- 
2.17.2

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

[dm-devel] [RFC PATCH 3/4] libmultipath: fix timeouts for detect_alua()

[Benjamin Marzinski]

detect_alua calls prioritizer functions that expect the timeout to be in
milliseconds instead of seconds. Fix this and also respect the
checker_timeout setting.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/discovery.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
index 2b1a11d5..9392134f 100644
--- a/libmultipath/discovery.c
+++ b/libmultipath/discovery.c
@@ -1016,15 +1016,17 @@ detect_alua(struct path * pp)
 	int ret;
 	int tpgs;
 	unsigned int timeout;
+	struct config *conf;
 
 
 	if (pp->bus != SYSFS_BUS_SCSI) {
 		pp->tpgs = TPGS_NONE;
 		return;
 	}
-
-	if (sysfs_get_timeout(pp, &timeout) <= 0)
-		timeout = DEF_TIMEOUT;
+	conf = get_multipath_config();
+	timeout = conf->checker_timeout * 1000;
+	put_multipath_config(conf);
+	timeout = get_prio_timeout(pp, timeout);
 
 	tpgs = get_target_port_group_support(pp, timeout);
 	if (tpgs == -RTPG_INQUIRY_FAILED)
-- 
2.17.2

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

[dm-devel] [RFC PATCH 4/4] libmultipath: fix timeouts for path_latency prioritizer

[Benjamin Marzinski]

The path_latency prioritizer didn't call get_prio_timeout, so that the
timeout could possibly be 0. Also, it was assuming that the timeout was
in seconds, instead of milliseconds.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/prioritizers/path_latency.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/libmultipath/prioritizers/path_latency.c b/libmultipath/prioritizers/path_latency.c
index 2f5be9b9..d48535d2 100644
--- a/libmultipath/prioritizers/path_latency.c
+++ b/libmultipath/prioritizers/path_latency.c
@@ -107,7 +107,7 @@ static void cleanup_directio_read(int fd, char *buf, int restore_flags)
 static int do_directio_read(int fd, unsigned int timeout, char *buf, int sz)
 {
 	fd_set read_fds;
-	struct timeval tm = { .tv_sec = timeout };
+	struct timeval tm = { .tv_sec = timeout / 1000 };
 	int ret;
 	int num_read;
 
@@ -247,7 +247,8 @@ int getprio(struct path *pp, char *args, unsigned int timeout)
 
 		(void)clock_gettime(CLOCK_MONOTONIC, &tv_before);
 
-		if (do_directio_read(pp->fd, timeout, buf, blksize)) {
+		if (do_directio_read(pp->fd, get_prio_timeout(pp, timeout),
+				     buf, blksize)) {
 			pp_pl_log(0, "%s: path down", pp->dev);
 			cleanup_directio_read(pp->fd, buf, restore_flags);
 			return -1;
-- 
2.17.2

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

Re: [dm-devel] [RFC PATCH 0/4] Make prio timeouts work like checkers

[Benjamin Marzinski]

On Fri, Jul 28, 2023 at 02:05:51PM -0500, Benjamin Marzinski wrote:

Ping. Perhaps this patchset got overlooked.

-Ben

> This patchset changes how prioritizers set their timeouts, to make them
> match how the checker functions work, and also cleans up some minor
> timeout issues. I did this to make out timeouts consistent, but if
> someone has a good reason to object to that, I don't feel
> strongly that it's necessary, and I can resend just the bugfixes.
> 
> Benjamin Marzinski (4):
>   libmultipath: don't bother to reset default timeout value
>   libmultipath: make prioritizer timeouts work like checker timeouts
>   libmultipath: fix timeouts for detect_alua()
>   libmultipath: fix timeouts for path_latency prioritizer
> 
>  libmultipath/discovery.c                 | 13 +++++++------
>  libmultipath/prio.c                      | 14 +++++++++-----
>  libmultipath/prio.h                      |  3 +--
>  libmultipath/prioritizers/alua.c         |  2 +-
>  libmultipath/prioritizers/alua_rtpg.c    |  5 ++---
>  libmultipath/prioritizers/emc.c          |  5 +++--
>  libmultipath/prioritizers/hds.c          |  4 ++--
>  libmultipath/prioritizers/hp_sw.c        |  4 ++--
>  libmultipath/prioritizers/ontap.c        |  7 +++----
>  libmultipath/prioritizers/path_latency.c |  5 +++--
>  libmultipath/prioritizers/rdac.c         |  4 ++--
>  11 files changed, 35 insertions(+), 31 deletions(-)
> 
> -- 
> 2.17.2
> 
> --
> dm-devel mailing list
> dm-devel@redhat.com
> https://listman.redhat.com/mailman/listinfo/dm-devel
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [RFC PATCH 0/4] Make prio timeouts work like checkers

[Martin Wilck]

On Fri, 2023-07-28 at 14:05 -0500, Benjamin Marzinski wrote:
> This patchset changes how prioritizers set their timeouts, to make
> them
> match how the checker functions work, and also cleans up some minor
> timeout issues. I did this to make out timeouts consistent, but if
> someone has a good reason to object to that, I don't feel
> strongly that it's necessary, and I can resend just the bugfixes.
> 

I don't object the idea, quite the contrary. But I would prefer a
different solution.

IMO we should treat the "io_timeout" as a path property, and add a
field  in "struct path" for it. It would be initialized from 
conf->checker_timeout, or if that's unset, from sysfs, and finally,
DEF_TIMEOUT. By reading the sysfs value, we'd be able to accomodate
different properties for different devices, but we'd not re-read this
value repeatedly like we're doing now. IMO that would be more
consistent with what we do for other device properties.

We currently pass the timeout value down the call stack all the way
from pathinfo() like this (for alua):

pathinfo()
  get_prio()
     prio_getprio()
       p->getprio()
         get_alua_info()
            get_target_port_group()
               do_inquiry() (*)
                 do_inquiry_sg()
            get_asymmetric_access_state()
               do_rtpg() (*)
               
With the exception of the functions marked by (*), all these functions
obtain a "struct path" argument, too. IIUC, the main reason we're doing
this is to avoid stalled getprio() calls for paths that are down
(bb935d4 ("libmultipath: change failed path prio timeout")).

IMO it would make more sense to remove the "timeout" arguments from
these functions, and just determine the timeout where it's needed. I
don't think that's a layering violation; functions that receive a
"struct path" can also handle PATH_DOWN. Thus we could write

int get_prio_timeout_ms(const struct path *pp) 
{
      if (pp->state == PATH_DOWN)
            return 10;
      else
            return pp->io_timeout * 1000;
}

and use this function as far down the stack as we can.

Furthermore, to improve code readability and avoid issues like in 3/4
and 4, I think we should call all variables and fields that take
millisecond values "timeout_ms".

Thoughts?

Martin

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

Re: [dm-devel] [RFC PATCH 0/4] Make prio timeouts work like checkers

[Benjamin Marzinski]

On Tue, Aug 29, 2023 at 08:34:39PM +0000, Martin Wilck wrote:
> On Fri, 2023-07-28 at 14:05 -0500, Benjamin Marzinski wrote:
> > This patchset changes how prioritizers set their timeouts, to make
> > them
> > match how the checker functions work, and also cleans up some minor
> > timeout issues. I did this to make out timeouts consistent, but if
> > someone has a good reason to object to that, I don't feel
> > strongly that it's necessary, and I can resend just the bugfixes.
> > 
> 
> I don't object the idea, quite the contrary. But I would prefer a
> different solution.
> 
> IMO we should treat the "io_timeout" as a path property, and add a
> field  in "struct path" for it. It would be initialized from 
> conf->checker_timeout, or if that's unset, from sysfs, and finally,
> DEF_TIMEOUT. By reading the sysfs value, we'd be able to accomodate
> different properties for different devices, but we'd not re-read this
> value repeatedly like we're doing now. IMO that would be more
> consistent with what we do for other device properties.
> 
> We currently pass the timeout value down the call stack all the way
> from pathinfo() like this (for alua):
> 
> pathinfo()
>   get_prio()
>      prio_getprio()
>        p->getprio()
>          get_alua_info()
>             get_target_port_group()
>                do_inquiry() (*)
>                  do_inquiry_sg()
>             get_asymmetric_access_state()
>                do_rtpg() (*)
>                
> With the exception of the functions marked by (*), all these functions
> obtain a "struct path" argument, too. IIUC, the main reason we're doing
> this is to avoid stalled getprio() calls for paths that are down
> (bb935d4 ("libmultipath: change failed path prio timeout")).
> 
> IMO it would make more sense to remove the "timeout" arguments from
> these functions, and just determine the timeout where it's needed. I
> don't think that's a layering violation; functions that receive a
> "struct path" can also handle PATH_DOWN. Thus we could write
> 
> int get_prio_timeout_ms(const struct path *pp) 
> {
>       if (pp->state == PATH_DOWN)
>             return 10;
>       else
>             return pp->io_timeout * 1000;
> }
> 
> and use this function as far down the stack as we can.
> 
> Furthermore, to improve code readability and avoid issues like in 3/4
> and 4, I think we should call all variables and fields that take
> millisecond values "timeout_ms".
> 
> Thoughts?

Sure. I can rework this.

-Ben

> 
> Martin
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel