| Message ID | 20211118225840.19810-33-mwilck@suse.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | christophe varoqui |
| Headers | show |
| Series | multipathd: uxlsnr overhaul | expand |
On Thu, Nov 18, 2021 at 11:58:24PM +0100, mwilck@suse.com wrote: > From: Martin Wilck <mwilck@suse.com> > > send_packet() may busy-loop. By polling for POLLOUT, we can > avoid that, even if it's very unlikely in practice. > The last time I reviewed this, I mentioned that when we fall through from CLT_WORK to CLT_SEND, the client hasn't checked for a POLLOUT revent, so it's possible to block here. I'm not sure that we care. If not Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com> > Signed-off-by: Martin Wilck <mwilck@suse.com> > --- > multipathd/uxlsnr.c | 35 ++++++++++++++++++++++++++--------- > 1 file changed, 26 insertions(+), 9 deletions(-) > > diff --git a/multipathd/uxlsnr.c b/multipathd/uxlsnr.c > index 45fe7b5..6213454 100644 > --- a/multipathd/uxlsnr.c > +++ b/multipathd/uxlsnr.c > @@ -447,7 +447,6 @@ static int client_state_machine(struct client *c, struct vectors *vecs, > short revents) > { > ssize_t n; > - const char *buf; > > 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)); > @@ -544,16 +543,31 @@ static int client_state_machine(struct client *c, struct vectors *vecs, > if (get_strbuf_len(&c->reply) == 0) > default_reply(c, c->error); > > - buf = get_strbuf_str(&c->reply); > + if (c->cmd_len == 0) { > + size_t len = get_strbuf_len(&c->reply) + 1; > > - if (send_packet(c->fd, buf) != 0) > - dead_client(c); > - else > - condlog(4, "cli[%d]: Reply [%zu bytes]", c->fd, > - get_strbuf_len(&c->reply) + 1); > - reset_strbuf(&c->reply); > + if (send(c->fd, &len, sizeof(len), MSG_NOSIGNAL) > + != sizeof(len)) > + c->error = -ECONNRESET; > + c->cmd_len = len; > + return STM_BREAK; > + } > > - set_client_state(c, CLT_RECV); > + if (c->len < c->cmd_len) { > + const char *buf = get_strbuf_str(&c->reply); > + > + n = send(c->fd, buf + c->len, c->cmd_len, MSG_NOSIGNAL); > + if (n == -1) { > + if (!(errno == EAGAIN || errno == EINTR)) > + c->error = -ECONNRESET; > + } else > + c->len += n; > + } > + > + if (c->len >= c->cmd_len) { > + condlog(4, "cli[%d]: Reply [%zu bytes]", c->fd, c->cmd_len); > + set_client_state(c, CLT_RECV); > + } > return STM_BREAK; > > default: > @@ -686,6 +700,9 @@ void *uxsock_listen(long ux_sock, void *trigger_data) > case CLT_RECV: > polls[i].events = POLLIN; > break; > + case CLT_SEND: > + polls[i].events = POLLOUT; > + break; > default: > /* don't poll for this client */ > continue; > -- > 2.33.1 -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
On Wed, 2021-11-24 at 19:43 -0600, Benjamin Marzinski wrote: > On Thu, Nov 18, 2021 at 11:58:24PM +0100, mwilck@suse.com wrote: > > From: Martin Wilck <mwilck@suse.com> > > > > send_packet() may busy-loop. By polling for POLLOUT, we can > > avoid that, even if it's very unlikely in practice. > > > > The last time I reviewed this, I mentioned that when we fall through > from CLT_WORK to CLT_SEND, the client hasn't checked for a POLLOUT > revent, so it's possible to block here. I'm not sure that we care. Fixing that will be as easy as entering as returning once from the state machine. I'll add the fix. Martin -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
Hello Ben, On Wed, 2021-11-24 at 19:43 -0600, Benjamin Marzinski wrote: > On Thu, Nov 18, 2021 at 11:58:24PM +0100, mwilck@suse.com wrote: > > From: Martin Wilck <mwilck@suse.com> > > > > send_packet() may busy-loop. By polling for POLLOUT, we can > > avoid that, even if it's very unlikely in practice. > > > > The last time I reviewed this, I mentioned that when we fall through > from CLT_WORK to CLT_SEND, the client hasn't checked for a POLLOUT > revent, so it's possible to block here. I'm not sure that we care. > If not > > Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com> Unlike the other patches, I did remove your "Reviewed-by:" on this patch in the v3 series, because I made a non-trivial change in order to fix the issue you mentioned. Now it's the only patch in the v3 series that doesn't have your Reviewed-by: tag. Will you review this patch again? If you're fine with the patch, and you have no other objections, I'll push the v4 of the series (with the whitespace issues fixed) to my "queue2 branch and start preparing a submission for Christophe. Regards 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 45fe7b5..6213454 100644 --- a/multipathd/uxlsnr.c +++ b/multipathd/uxlsnr.c @@ -447,7 +447,6 @@ static int client_state_machine(struct client *c, struct vectors *vecs, short revents) { ssize_t n; - const char *buf; 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)); @@ -544,16 +543,31 @@ static int client_state_machine(struct client *c, struct vectors *vecs, if (get_strbuf_len(&c->reply) == 0) default_reply(c, c->error); - buf = get_strbuf_str(&c->reply); + if (c->cmd_len == 0) { + size_t len = get_strbuf_len(&c->reply) + 1; - if (send_packet(c->fd, buf) != 0) - dead_client(c); - else - condlog(4, "cli[%d]: Reply [%zu bytes]", c->fd, - get_strbuf_len(&c->reply) + 1); - reset_strbuf(&c->reply); + if (send(c->fd, &len, sizeof(len), MSG_NOSIGNAL) + != sizeof(len)) + c->error = -ECONNRESET; + c->cmd_len = len; + return STM_BREAK; + } - set_client_state(c, CLT_RECV); + if (c->len < c->cmd_len) { + const char *buf = get_strbuf_str(&c->reply); + + n = send(c->fd, buf + c->len, c->cmd_len, MSG_NOSIGNAL); + if (n == -1) { + if (!(errno == EAGAIN || errno == EINTR)) + c->error = -ECONNRESET; + } else + c->len += n; + } + + if (c->len >= c->cmd_len) { + condlog(4, "cli[%d]: Reply [%zu bytes]", c->fd, c->cmd_len); + set_client_state(c, CLT_RECV); + } return STM_BREAK; default: @@ -686,6 +700,9 @@ void *uxsock_listen(long ux_sock, void *trigger_data) case CLT_RECV: polls[i].events = POLLIN; break; + case CLT_SEND: + polls[i].events = POLLOUT; + break; default: /* don't poll for this client */ continue;