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

[Martin Wilck <martin.wilck@suse.com>]

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