[06/15] libmultipath: split get_state into two functions

Message ID	20240828221757.4060548-7-bmarzins@redhat.com (mailing list archive)
State	Not Applicable, archived
Delegated to:	Benjamin Marzinski
Headers	show Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 060E21AD9C2 for <dm-devel@lists.linux.dev>; Wed, 28 Aug 2024 22:18:05 +0000 (UTC) From: Benjamin Marzinski <bmarzins@redhat.com> To: Christophe Varoqui <christophe.varoqui@opensvc.com> Cc: device-mapper development <dm-devel@lists.linux.dev>, Martin Wilck <Martin.Wilck@suse.com> Subject: [PATCH 06/15] libmultipath: split get_state into two functions Date: Wed, 28 Aug 2024 18:17:48 -0400 Message-ID: <20240828221757.4060548-7-bmarzins@redhat.com> In-Reply-To: <20240828221757.4060548-1-bmarzins@redhat.com> References: <20240828221757.4060548-1-bmarzins@redhat.com> Precedence: bulk MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Content-Type: text/plain; charset="US-ASCII"; x-default=true
Series	Yet Another path checker refactor \| expand [00/15] Yet Another path checker refactor [01/15] libmultipath: store checker_check() result in checker struct [02/15] libmultipath: add missing checker function prototypes [03/15] libmultipath: split out the code to wait for pending checkers [04/15] libmultipath: remove pending wait code from libcheck_check calls [05/15] multipath-tools tests: fix up directio tests [06/15] libmultipath: split get_state into two functions [07/15] libmultipath: change path_offline to path_sysfs_state [08/15] multipathd: split check_path_state into two functions [09/15] multipathd: split do_checker_path [10/15] multipathd: split check_path into two functions [11/15] multipathd: split handle_uninitialized_path into two functions [12/15] multipathd: split check_paths into two functions [13/15] multipathd: update priority once after updating all paths [14/15] multipathd: remove pointless check [15/15] multipathd: fix deferred_failback_tick for reload removes

Message ID

20240828221757.4060548-7-bmarzins@redhat.com (mailing list archive)

State

Not Applicable, archived

Delegated to:

Benjamin Marzinski

Headers

From: Benjamin Marzinski <bmarzins@redhat.com>
To: Christophe Varoqui <christophe.varoqui@opensvc.com>
Cc: device-mapper development <dm-devel@lists.linux.dev>,
        Martin Wilck <Martin.Wilck@suse.com>
Subject: [PATCH 06/15] libmultipath: split get_state into two functions
Date: Wed, 28 Aug 2024 18:17:48 -0400
Message-ID: <20240828221757.4060548-7-bmarzins@redhat.com>
In-Reply-To: <20240828221757.4060548-1-bmarzins@redhat.com>
References: <20240828221757.4060548-1-bmarzins@redhat.com>
Precedence: bulk
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Content-Type: text/plain; charset="US-ASCII"; x-default=true

Series

Yet Another path checker refactor | expand

Commit Message

Benjamin Marzinski Aug. 28, 2024, 10:17 p.m. UTC

get_state() is now split into start_checker(), which runs the checker
but doesn't wait for async checkers, and get_state(), which returns the
state from the checker, after waiting for async checkers if necessary.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/discovery.c | 22 ++++++++++++++++------
 libmultipath/discovery.h |  4 +++-
 libmultipath/propsel.c   |  1 +
 multipathd/main.c        |  4 +++-
 4 files changed, 23 insertions(+), 8 deletions(-)

Comments

Martin Wilck Sept. 4, 2024, 3:14 p.m. UTC | #1

On Wed, 2024-08-28 at 18:17 -0400, Benjamin Marzinski wrote:
> get_state() is now split into start_checker(), which runs the checker
> but doesn't wait for async checkers, and get_state(), which returns
> the
> state from the checker, after waiting for async checkers if
> necessary.
> 
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
>  libmultipath/discovery.c | 22 ++++++++++++++++------
>  libmultipath/discovery.h |  4 +++-
>  libmultipath/propsel.c   |  1 +
>  multipathd/main.c        |  4 +++-
>  4 files changed, 23 insertions(+), 8 deletions(-)
> 
> diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
> index 5648be60..e0f46ff2 100644
> --- a/libmultipath/discovery.c
> +++ b/libmultipath/discovery.c
> @@ -1974,30 +1974,29 @@ cciss_ioctl_pathinfo(struct path *pp)
>  }
>  
>  int
> -get_state (struct path * pp, struct config *conf, int daemon, int
> oldstate)
> +start_checker (struct path * pp, struct config *conf, int daemon,
> int oldstate)
>  {
>  	struct checker * c = &pp->checker;
> -	int state;
>  
>  	if (!checker_selected(c)) {
>  		if (daemon) {
>  			if (pathinfo(pp, conf, DI_SYSFS) !=
> PATHINFO_OK) {
>  				condlog(3, "%s: couldn't get sysfs
> pathinfo",
>  					pp->dev);
> -				return PATH_UNCHECKED;
> +				return -1;
>  			}
>  		}
>  		select_detect_checker(conf, pp);
>  		select_checker(conf, pp);
>  		if (!checker_selected(c)) {
>  			condlog(3, "%s: No checker selected", pp-
> >dev);
> -			return PATH_UNCHECKED;
> +			return -1;
>  		}
>  		checker_set_fd(c, pp->fd);
>  		if (checker_init(c, pp->mpp?&pp->mpp-
> >mpcontext:NULL)) {
>  			checker_clear(c);
>  			condlog(3, "%s: checker init failed", pp-
> >dev);
> -			return PATH_UNCHECKED;
> +			return -1;
>  		}
>  	}
>  	if (pp->mpp && !c->mpcontext)
> @@ -2008,6 +2007,15 @@ get_state (struct path * pp, struct config
> *conf, int daemon, int oldstate)
>  	else
>  		checker_set_sync(c);
>  	checker_check(c, oldstate);
> +	return 0;
> +}
> +
> +int
> +get_state (struct path * pp)
> +{
> +	struct checker * c = &pp->checker;
> +	int state;
> +
>  	state = checker_get_state(c);
>  	condlog(3, "%s: %s state = %s", pp->dev,
>  		checker_name(c), checker_state_name(state));
> @@ -2455,7 +2463,9 @@ int pathinfo(struct path *pp, struct config
> *conf, int mask)
>  
>  	if (mask & DI_CHECKER) {
>  		if (path_state == PATH_UP) {
> -			int newstate = get_state(pp, conf, 0,
> path_state);
> +			int newstate = PATH_UNCHECKED;
> +			if (start_checker(pp, conf, 0, path_state)
> == 0)
> +				newstate = get_state(pp);
>  			if (newstate != PATH_PENDING ||
>  			    pp->state == PATH_UNCHECKED ||
>  			    pp->state == PATH_WILD)
> diff --git a/libmultipath/discovery.h b/libmultipath/discovery.h
> index c93abf1c..f3e0c618 100644
> --- a/libmultipath/discovery.h
> +++ b/libmultipath/discovery.h
> @@ -34,7 +34,9 @@ int path_discovery (vector pathvec, int flag);
>  int path_get_tpgs(struct path *pp); /* This function never returns
> TPGS_UNDEF */
>  int do_tur (char *);
>  int path_offline (struct path *);
> -int get_state (struct path * pp, struct config * conf, int daemon,
> int state);
> +int start_checker(struct path * pp, struct config * conf, int
> daemon,
> +		  int state);
> +int get_state(struct path * pp);
>  int get_vpd_sgio (int fd, int pg, int vend_id, char * str, int
> maxlen);
>  int pathinfo (struct path * pp, struct config * conf, int mask);
>  int alloc_path_with_pathinfo (struct config *conf, struct
> udev_device *udevice,
> diff --git a/libmultipath/propsel.c b/libmultipath/propsel.c
> index a3fce203..ad771d35 100644
> --- a/libmultipath/propsel.c
> +++ b/libmultipath/propsel.c
> @@ -700,6 +700,7 @@ out:
>  	condlog(3, "%s: path_checker = %s %s", pp->dev,
>  		checker_name(c), origin);
>  	c->timeout = pp->checker_timeout;
> +	c->path_state = PATH_UNCHECKED;

Should this be in 01/15 already?

Martin

Benjamin Marzinski Sept. 4, 2024, 6:43 p.m. UTC | #2

On Wed, Sep 04, 2024 at 05:14:32PM +0200, Martin Wilck wrote:
> On Wed, 2024-08-28 at 18:17 -0400, Benjamin Marzinski wrote:
> > get_state() is now split into start_checker(), which runs the checker
> > but doesn't wait for async checkers, and get_state(), which returns
> > the
> > state from the checker, after waiting for async checkers if
> > necessary.
> > 
> > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> > ---
> >  libmultipath/discovery.c | 22 ++++++++++++++++------
> >  libmultipath/discovery.h |  4 +++-
> >  libmultipath/propsel.c   |  1 +
> >  multipathd/main.c        |  4 +++-
> >  4 files changed, 23 insertions(+), 8 deletions(-)
> > 
> > diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
> > index 5648be60..e0f46ff2 100644
> > --- a/libmultipath/discovery.c
> > +++ b/libmultipath/discovery.c
> > @@ -1974,30 +1974,29 @@ cciss_ioctl_pathinfo(struct path *pp)
> >  }
> >  
> >  int
> > -get_state (struct path * pp, struct config *conf, int daemon, int
> > oldstate)
> > +start_checker (struct path * pp, struct config *conf, int daemon,
> > int oldstate)
> >  {
> >  	struct checker * c = &pp->checker;
> > -	int state;
> >  
> >  	if (!checker_selected(c)) {
> >  		if (daemon) {
> >  			if (pathinfo(pp, conf, DI_SYSFS) !=
> > PATHINFO_OK) {
> >  				condlog(3, "%s: couldn't get sysfs
> > pathinfo",
> >  					pp->dev);
> > -				return PATH_UNCHECKED;
> > +				return -1;
> >  			}
> >  		}
> >  		select_detect_checker(conf, pp);
> >  		select_checker(conf, pp);
> >  		if (!checker_selected(c)) {
> >  			condlog(3, "%s: No checker selected", pp-
> > >dev);
> > -			return PATH_UNCHECKED;
> > +			return -1;
> >  		}
> >  		checker_set_fd(c, pp->fd);
> >  		if (checker_init(c, pp->mpp?&pp->mpp-
> > >mpcontext:NULL)) {
> >  			checker_clear(c);
> >  			condlog(3, "%s: checker init failed", pp-
> > >dev);
> > -			return PATH_UNCHECKED;
> > +			return -1;
> >  		}
> >  	}
> >  	if (pp->mpp && !c->mpcontext)
> > @@ -2008,6 +2007,15 @@ get_state (struct path * pp, struct config
> > *conf, int daemon, int oldstate)
> >  	else
> >  		checker_set_sync(c);
> >  	checker_check(c, oldstate);
> > +	return 0;
> > +}
> > +
> > +int
> > +get_state (struct path * pp)
> > +{
> > +	struct checker * c = &pp->checker;
> > +	int state;
> > +
> >  	state = checker_get_state(c);
> >  	condlog(3, "%s: %s state = %s", pp->dev,
> >  		checker_name(c), checker_state_name(state));
> > @@ -2455,7 +2463,9 @@ int pathinfo(struct path *pp, struct config
> > *conf, int mask)
> >  
> >  	if (mask & DI_CHECKER) {
> >  		if (path_state == PATH_UP) {
> > -			int newstate = get_state(pp, conf, 0,
> > path_state);
> > +			int newstate = PATH_UNCHECKED;
> > +			if (start_checker(pp, conf, 0, path_state)
> > == 0)
> > +				newstate = get_state(pp);
> >  			if (newstate != PATH_PENDING ||
> >  			    pp->state == PATH_UNCHECKED ||
> >  			    pp->state == PATH_WILD)
> > diff --git a/libmultipath/discovery.h b/libmultipath/discovery.h
> > index c93abf1c..f3e0c618 100644
> > --- a/libmultipath/discovery.h
> > +++ b/libmultipath/discovery.h
> > @@ -34,7 +34,9 @@ int path_discovery (vector pathvec, int flag);
> >  int path_get_tpgs(struct path *pp); /* This function never returns
> > TPGS_UNDEF */
> >  int do_tur (char *);
> >  int path_offline (struct path *);
> > -int get_state (struct path * pp, struct config * conf, int daemon,
> > int state);
> > +int start_checker(struct path * pp, struct config * conf, int
> > daemon,
> > +		  int state);
> > +int get_state(struct path * pp);
> >  int get_vpd_sgio (int fd, int pg, int vend_id, char * str, int
> > maxlen);
> >  int pathinfo (struct path * pp, struct config * conf, int mask);
> >  int alloc_path_with_pathinfo (struct config *conf, struct
> > udev_device *udevice,
> > diff --git a/libmultipath/propsel.c b/libmultipath/propsel.c
> > index a3fce203..ad771d35 100644
> > --- a/libmultipath/propsel.c
> > +++ b/libmultipath/propsel.c
> > @@ -700,6 +700,7 @@ out:
> >  	condlog(3, "%s: path_checker = %s %s", pp->dev,
> >  		checker_name(c), origin);
> >  	c->timeout = pp->checker_timeout;
> > +	c->path_state = PATH_UNCHECKED;
> 
> Should this be in 01/15 already?

Yeah. That makes sense.

-Ben

> 
> Martin

diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
index 5648be60..e0f46ff2 100644
--- a/libmultipath/discovery.c
+++ b/libmultipath/discovery.c
@@ -1974,30 +1974,29 @@  cciss_ioctl_pathinfo(struct path *pp)
 }
 
 int
-get_state (struct path * pp, struct config *conf, int daemon, int oldstate)
+start_checker (struct path * pp, struct config *conf, int daemon, int oldstate)
 {
 	struct checker * c = &pp->checker;
-	int state;
 
 	if (!checker_selected(c)) {
 		if (daemon) {
 			if (pathinfo(pp, conf, DI_SYSFS) != PATHINFO_OK) {
 				condlog(3, "%s: couldn't get sysfs pathinfo",
 					pp->dev);
-				return PATH_UNCHECKED;
+				return -1;
 			}
 		}
 		select_detect_checker(conf, pp);
 		select_checker(conf, pp);
 		if (!checker_selected(c)) {
 			condlog(3, "%s: No checker selected", pp->dev);
-			return PATH_UNCHECKED;
+			return -1;
 		}
 		checker_set_fd(c, pp->fd);
 		if (checker_init(c, pp->mpp?&pp->mpp->mpcontext:NULL)) {
 			checker_clear(c);
 			condlog(3, "%s: checker init failed", pp->dev);
-			return PATH_UNCHECKED;
+			return -1;
 		}
 	}
 	if (pp->mpp && !c->mpcontext)
@@ -2008,6 +2007,15 @@  get_state (struct path * pp, struct config *conf, int daemon, int oldstate)
 	else
 		checker_set_sync(c);
 	checker_check(c, oldstate);
+	return 0;
+}
+
+int
+get_state (struct path * pp)
+{
+	struct checker * c = &pp->checker;
+	int state;
+
 	state = checker_get_state(c);
 	condlog(3, "%s: %s state = %s", pp->dev,
 		checker_name(c), checker_state_name(state));
@@ -2455,7 +2463,9 @@  int pathinfo(struct path *pp, struct config *conf, int mask)
 
 	if (mask & DI_CHECKER) {
 		if (path_state == PATH_UP) {
-			int newstate = get_state(pp, conf, 0, path_state);
+			int newstate = PATH_UNCHECKED;
+			if (start_checker(pp, conf, 0, path_state) == 0)
+				newstate = get_state(pp);
 			if (newstate != PATH_PENDING ||
 			    pp->state == PATH_UNCHECKED ||
 			    pp->state == PATH_WILD)
diff --git a/libmultipath/discovery.h b/libmultipath/discovery.h
index c93abf1c..f3e0c618 100644
--- a/libmultipath/discovery.h
+++ b/libmultipath/discovery.h
@@ -34,7 +34,9 @@  int path_discovery (vector pathvec, int flag);
 int path_get_tpgs(struct path *pp); /* This function never returns TPGS_UNDEF */
 int do_tur (char *);
 int path_offline (struct path *);
-int get_state (struct path * pp, struct config * conf, int daemon, int state);
+int start_checker(struct path * pp, struct config * conf, int daemon,
+		  int state);
+int get_state(struct path * pp);
 int get_vpd_sgio (int fd, int pg, int vend_id, char * str, int maxlen);
 int pathinfo (struct path * pp, struct config * conf, int mask);
 int alloc_path_with_pathinfo (struct config *conf, struct udev_device *udevice,
diff --git a/libmultipath/propsel.c b/libmultipath/propsel.c
index a3fce203..ad771d35 100644
--- a/libmultipath/propsel.c
+++ b/libmultipath/propsel.c
@@ -700,6 +700,7 @@  out:
 	condlog(3, "%s: path_checker = %s %s", pp->dev,
 		checker_name(c), origin);
 	c->timeout = pp->checker_timeout;
+	c->path_state = PATH_UNCHECKED;
 	return 0;
 }
 
diff --git a/multipathd/main.c b/multipathd/main.c
index 1b7fd04f..4f752adb 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -2324,7 +2324,9 @@  check_path_state(struct path *pp)
 	if (newstate == PATH_UP) {
 		conf = get_multipath_config();
 		pthread_cleanup_push(put_multipath_config, conf);
-		newstate = get_state(pp, conf, 1, newstate);
+		newstate = PATH_UNCHECKED;
+		if (start_checker(pp, conf, 1, newstate) == 0)
+			newstate = get_state(pp);
 		pthread_cleanup_pop(1);
 	} else {
 		checker_clear_message(&pp->checker);

[06/15] libmultipath: split get_state into two functions

Commit Message

Comments

Patch