| Message ID | 1690571155-8283-2-git-send-email-bmarzins@redhat.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | christophe varoqui |
| Headers | show |
| Series | Make prio timeouts work like checkers | expand |
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
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));
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(-)