| 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 |
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
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
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 --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);