diff mbox series

[32/35] multipathd: uxlsnr: add timeout handling

Message ID 20210910114120.13665-33-mwilck@suse.com (mailing list archive)
State Not Applicable, archived
Delegated to: christophe varoqui
Headers show
Series multipathd: uxlsnr overhaul | expand

Commit Message

Martin Wilck Sept. 10, 2021, 11:41 a.m. UTC
From: Martin Wilck <mwilck@suse.com>

Our ppoll() call needs to wake up when a client request times out.
This logic can be added by determining the first client that's about
to time out. The logic in handle_client() will then cause a timeout
reply to be sent to the client. This is more client-friendly
as the client timing out without receiving a reply.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 multipathd/uxlsnr.c | 58 +++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 53 insertions(+), 5 deletions(-)

Comments

Benjamin Marzinski Sept. 16, 2021, 4:17 a.m. UTC | #1
On Fri, Sep 10, 2021 at 01:41:17PM +0200, mwilck@suse.com wrote:
> From: Martin Wilck <mwilck@suse.com>
> 
> Our ppoll() call needs to wake up when a client request times out.
> This logic can be added by determining the first client that's about
> to time out. The logic in handle_client() will then cause a timeout
> reply to be sent to the client. This is more client-friendly
> as the client timing out without receiving a reply.
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  multipathd/uxlsnr.c | 58 +++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 53 insertions(+), 5 deletions(-)
> 
> diff --git a/multipathd/uxlsnr.c b/multipathd/uxlsnr.c
> index 4637954..1bf4126 100644
> --- a/multipathd/uxlsnr.c
> +++ b/multipathd/uxlsnr.c
> @@ -320,6 +320,35 @@ static void handle_inotify(int fd, struct watch_descriptors *wds)
>  }
>  
>  static const struct timespec ts_zero = { .tv_sec = 0, };
> +static const struct timespec ts_max = { .tv_sec = LONG_MAX, .tv_nsec = 999999999 };
> +
> +/* call with clients lock held */
> +static struct timespec *__get_soonest_timeout(struct timespec *ts)
> +{
> +	struct timespec ts_min = ts_max, now;
> +	bool any = false;
> +	struct client *c;
> +
> +	list_for_each_entry(c, &clients, node) {
> +		if (timespeccmp(&c->expires, &ts_zero) != 0 &&
> +		    timespeccmp(&c->expires, &ts_min) < 0) {
> +			ts_min = c->expires;
> +			any = true;
> +		}
> +	}
> +
> +	if (!any)
> +		return NULL;
> +
> +	get_monotonic_time(&now);
> +	timespecsub(&ts_min, &now, ts);
> +	if (timespeccmp(ts, &ts_zero) < 0)
> +		*ts = ts_zero;
> +
> +	condlog(4, "%s: next client expires in %ld.%03lds", __func__,
> +		(long)ts->tv_sec, ts->tv_nsec / 1000000);
> +	return ts;
> +}
>  
>  /* call with clients lock held */
>  static bool __need_lock(void)
> @@ -422,6 +451,24 @@ static void set_client_state(struct client *c, int state)
>  	c->state = state;
>  }
>  
> +static void check_timeout(struct client *c)
> +{
> +	struct timespec now;
> +
> +	if (timespeccmp(&c->expires, &ts_zero) == 0)
> +		return;
> +
> +	get_monotonic_time(&now);
> +	if (timespeccmp(&c->expires, &now) > 0)
> +		return;
> +
> +	condlog(2, "%s: cli[%d]: timed out at %ld.%03ld", __func__,
> +		c->fd, (long)c->expires.tv_sec, c->expires.tv_nsec / 1000000);
> +
> +	c->error = -ETIMEDOUT;
> +	set_client_state(c, CLT_SEND);
> +}
> +
>  static void handle_client(struct client *c, struct vectors *vecs, short revents)
>  {
>  	ssize_t n;
> @@ -435,6 +482,8 @@ static void handle_client(struct client *c, struct vectors *vecs, short revents)
>  		return;
>  	}
>  
> +	check_timeout(c);
> +
>  	condlog(4, "%s: cli[%d] poll=%x state=%d cmd=\"%s\" repl \"%s\"", __func__,
>  		c->fd, revents, c->state, c->cmd, get_strbuf_str(&c->reply));
>  
> @@ -594,6 +643,7 @@ void *uxsock_listen(long ux_sock, void *trigger_data)
>  	while (1) {
>  		struct client *c, *tmp;
>  		int i, n_pfds, poll_count, num_clients;
> +		struct timespec __timeout, *timeout;

Maybe it's just too late to be looking at code, but I'm missing why we
need a separate variable that it a pointer to __timeout.

-Ben

>  
>  		/* setup for a poll */
>  		pthread_mutex_lock(&client_lock);
> @@ -661,10 +711,12 @@ void *uxsock_listen(long ux_sock, void *trigger_data)
>                                  break;
>                  }
>                  n_pfds = i;
> +		timeout = __get_soonest_timeout(&__timeout);
> +
>  		pthread_cleanup_pop(1);
>  
>  		/* most of our life is spent in this call */
> -		poll_count = ppoll(polls, n_pfds, NULL, &mask);
> +		poll_count = ppoll(polls, n_pfds, timeout, &mask);
>  
>  		handle_signals(false);
>  		if (poll_count == -1) {
> @@ -679,10 +731,6 @@ void *uxsock_listen(long ux_sock, void *trigger_data)
>  			break;
>  		}
>  
> -		if (poll_count == 0) {
> -			handle_signals(true);
> -			continue;
> -		}
>  		if (polls[POLLFD_IDLE].fd != -1 &&
>  		    polls[POLLFD_IDLE].revents & POLLIN)
>  			drain_idle_fd(idle_fd);
> -- 
> 2.33.0

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
Martin Wilck Sept. 16, 2021, 8:58 a.m. UTC | #2
On Wed, 2021-09-15 at 23:17 -0500, Benjamin Marzinski wrote:
> On Fri, Sep 10, 2021 at 01:41:17PM +0200, mwilck@suse.com wrote:
> > From: Martin Wilck <mwilck@suse.com>
> > 
> > Our ppoll() call needs to wake up when a client request times out.
> > This logic can be added by determining the first client that's
> > about
> > to time out. The logic in handle_client() will then cause a timeout
> > reply to be sent to the client. This is more client-friendly
> > as the client timing out without receiving a reply.
> > 
> > Signed-off-by: Martin Wilck <mwilck@suse.com>
> > ---
> >  multipathd/uxlsnr.c | 58
> > +++++++++++++++++++++++++++++++++++++++++----
> >  1 file changed, 53 insertions(+), 5 deletions(-)
> > 
> >  
> > @@ -594,6 +643,7 @@ void *uxsock_listen(long ux_sock, void
> > *trigger_data)
> >         while (1) {
> >                 struct client *c, *tmp;
> >                 int i, n_pfds, poll_count, num_clients;
> > +               struct timespec __timeout, *timeout;
> 
> Maybe it's just too late to be looking at code, but I'm missing why
> we
> need a separate variable that it a pointer to __timeout.

This way __get_soonest_timeout() can return either NULL or &__timeout,
and we can simply pass the return value to ppoll().

Martin



--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
Benjamin Marzinski Sept. 16, 2021, 3:08 p.m. UTC | #3
On Thu, Sep 16, 2021 at 10:58:36AM +0200, Martin Wilck wrote:
> On Wed, 2021-09-15 at 23:17 -0500, Benjamin Marzinski wrote:
> > On Fri, Sep 10, 2021 at 01:41:17PM +0200, mwilck@suse.com wrote:
> > > From: Martin Wilck <mwilck@suse.com>
> > > 
> > > Our ppoll() call needs to wake up when a client request times out.
> > > This logic can be added by determining the first client that's
> > > about
> > > to time out. The logic in handle_client() will then cause a timeout
> > > reply to be sent to the client. This is more client-friendly
> > > as the client timing out without receiving a reply.
> > > 
> > > Signed-off-by: Martin Wilck <mwilck@suse.com>
> > > ---
> > >  multipathd/uxlsnr.c | 58
> > > +++++++++++++++++++++++++++++++++++++++++----
> > >  1 file changed, 53 insertions(+), 5 deletions(-)
> > > 
> > >  
> > > @@ -594,6 +643,7 @@ void *uxsock_listen(long ux_sock, void
> > > *trigger_data)
> > >         while (1) {
> > >                 struct client *c, *tmp;
> > >                 int i, n_pfds, poll_count, num_clients;
> > > +               struct timespec __timeout, *timeout;
> > 
> > Maybe it's just too late to be looking at code, but I'm missing why
> > we
> > need a separate variable that it a pointer to __timeout.
> 
> This way __get_soonest_timeout() can return either NULL or &__timeout,
> and we can simply pass the return value to ppoll().

Ah. Yep. Too late to be reviewing code.

-Ben

> 
> Martin
> 

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

Patch

diff --git a/multipathd/uxlsnr.c b/multipathd/uxlsnr.c
index 4637954..1bf4126 100644
--- a/multipathd/uxlsnr.c
+++ b/multipathd/uxlsnr.c
@@ -320,6 +320,35 @@  static void handle_inotify(int fd, struct watch_descriptors *wds)
 }
 
 static const struct timespec ts_zero = { .tv_sec = 0, };
+static const struct timespec ts_max = { .tv_sec = LONG_MAX, .tv_nsec = 999999999 };
+
+/* call with clients lock held */
+static struct timespec *__get_soonest_timeout(struct timespec *ts)
+{
+	struct timespec ts_min = ts_max, now;
+	bool any = false;
+	struct client *c;
+
+	list_for_each_entry(c, &clients, node) {
+		if (timespeccmp(&c->expires, &ts_zero) != 0 &&
+		    timespeccmp(&c->expires, &ts_min) < 0) {
+			ts_min = c->expires;
+			any = true;
+		}
+	}
+
+	if (!any)
+		return NULL;
+
+	get_monotonic_time(&now);
+	timespecsub(&ts_min, &now, ts);
+	if (timespeccmp(ts, &ts_zero) < 0)
+		*ts = ts_zero;
+
+	condlog(4, "%s: next client expires in %ld.%03lds", __func__,
+		(long)ts->tv_sec, ts->tv_nsec / 1000000);
+	return ts;
+}
 
 /* call with clients lock held */
 static bool __need_lock(void)
@@ -422,6 +451,24 @@  static void set_client_state(struct client *c, int state)
 	c->state = state;
 }
 
+static void check_timeout(struct client *c)
+{
+	struct timespec now;
+
+	if (timespeccmp(&c->expires, &ts_zero) == 0)
+		return;
+
+	get_monotonic_time(&now);
+	if (timespeccmp(&c->expires, &now) > 0)
+		return;
+
+	condlog(2, "%s: cli[%d]: timed out at %ld.%03ld", __func__,
+		c->fd, (long)c->expires.tv_sec, c->expires.tv_nsec / 1000000);
+
+	c->error = -ETIMEDOUT;
+	set_client_state(c, CLT_SEND);
+}
+
 static void handle_client(struct client *c, struct vectors *vecs, short revents)
 {
 	ssize_t n;
@@ -435,6 +482,8 @@  static void handle_client(struct client *c, struct vectors *vecs, short revents)
 		return;
 	}
 
+	check_timeout(c);
+
 	condlog(4, "%s: cli[%d] poll=%x state=%d cmd=\"%s\" repl \"%s\"", __func__,
 		c->fd, revents, c->state, c->cmd, get_strbuf_str(&c->reply));
 
@@ -594,6 +643,7 @@  void *uxsock_listen(long ux_sock, void *trigger_data)
 	while (1) {
 		struct client *c, *tmp;
 		int i, n_pfds, poll_count, num_clients;
+		struct timespec __timeout, *timeout;
 
 		/* setup for a poll */
 		pthread_mutex_lock(&client_lock);
@@ -661,10 +711,12 @@  void *uxsock_listen(long ux_sock, void *trigger_data)
                                 break;
                 }
                 n_pfds = i;
+		timeout = __get_soonest_timeout(&__timeout);
+
 		pthread_cleanup_pop(1);
 
 		/* most of our life is spent in this call */
-		poll_count = ppoll(polls, n_pfds, NULL, &mask);
+		poll_count = ppoll(polls, n_pfds, timeout, &mask);
 
 		handle_signals(false);
 		if (poll_count == -1) {
@@ -679,10 +731,6 @@  void *uxsock_listen(long ux_sock, void *trigger_data)
 			break;
 		}
 
-		if (poll_count == 0) {
-			handle_signals(true);
-			continue;
-		}
 		if (polls[POLLFD_IDLE].fd != -1 &&
 		    polls[POLLFD_IDLE].revents & POLLIN)
 			drain_idle_fd(idle_fd);