From: Willy Tarreau Date: Wed, 24 Oct 2018 15:17:56 +0000 (+0200) Subject: MEDIUM: channel: merge back flags CF_WRITE_PARTIAL and CF_WRITE_EVENT X-Git-Tag: v1.9-dev5~38 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=ede3d884fc34a600ab0b450002bba1a33d873aa6;p=thirdparty%2Fhaproxy.git MEDIUM: channel: merge back flags CF_WRITE_PARTIAL and CF_WRITE_EVENT The behaviour of the flag CF_WRITE_PARTIAL was modified by commit 95fad5ba4 ("BUG/MAJOR: stream-int: don't re-arm recv if send fails") due to a situation where it could trigger an immediate wake up of the other side, both acting in loops via the FD cache. This loss has caused the need to introduce CF_WRITE_EVENT as commit c5a9d5bf, to replace it, but both flags express more or less the same thing and this distinction creates a lot of confusion and complexity in the code. Since the FD cache now acts via tasklets, the issue worked around in the first patch no longer exists, so it's more than time to kill this hack and to restore CF_WRITE_PARTIAL's semantics (i.e.: there has been some write activity since we last left process_stream). This patch mostly reverts the two commits above. Only the part making use of CF_WROTE_DATA instead of CF_WRITE_PARTIAL to detect the loss of data upon connection setup was kept because it's more accurate and better suited. --- diff --git a/contrib/debug/flags.c b/contrib/debug/flags.c index a9e09096a7..ae885d6d85 100644 --- a/contrib/debug/flags.c +++ b/contrib/debug/flags.c @@ -72,7 +72,6 @@ void show_chn_flags(unsigned int f) SHOW_FLAG(f, CF_ISRESP); SHOW_FLAG(f, CF_FLT_ANALYZE); - SHOW_FLAG(f, CF_WRITE_EVENT); SHOW_FLAG(f, CF_WAKE_ONCE); SHOW_FLAG(f, CF_NEVER_WAIT); SHOW_FLAG(f, CF_SEND_DONTWAIT); diff --git a/include/proto/channel.h b/include/proto/channel.h index bf84a16bbd..56bf5cf61c 100644 --- a/include/proto/channel.h +++ b/include/proto/channel.h @@ -459,7 +459,7 @@ static inline void channel_check_timeouts(struct channel *chn) unlikely(tick_is_expired(chn->rex, now_ms))) chn->flags |= CF_READ_TIMEOUT; - if (likely(!(chn->flags & (CF_SHUTW|CF_WRITE_TIMEOUT|CF_WRITE_ACTIVITY|CF_WRITE_EVENT))) && + if (likely(!(chn->flags & (CF_SHUTW|CF_WRITE_TIMEOUT|CF_WRITE_ACTIVITY))) && unlikely(tick_is_expired(chn->wex, now_ms))) chn->flags |= CF_WRITE_TIMEOUT; @@ -772,7 +772,7 @@ static inline void co_skip(struct channel *chn, int len) c_realign_if_empty(chn); /* notify that some data was written to the SI from the buffer */ - chn->flags |= CF_WRITE_PARTIAL | CF_WRITE_EVENT; + chn->flags |= CF_WRITE_PARTIAL; } /* Tries to copy chunk into the channel's buffer after length controls. diff --git a/include/types/channel.h b/include/types/channel.h index 7879b1258b..f7ddd1a576 100644 --- a/include/types/channel.h +++ b/include/types/channel.h @@ -117,11 +117,11 @@ #define CF_WAKE_ONCE 0x10000000 /* pretend there is activity on this channel (one-shoot) */ #define CF_FLT_ANALYZE 0x20000000 /* at least one filter is still analyzing this channel */ -#define CF_WRITE_EVENT 0x40000000 /* write activity not processed yet by the stream */ +/* unused: 0x40000000 */ #define CF_ISRESP 0x80000000 /* 0 = request channel, 1 = response channel */ /* Masks which define input events for stream analysers */ -#define CF_MASK_ANALYSER (CF_READ_ATTACHED|CF_READ_ACTIVITY|CF_READ_TIMEOUT|CF_ANA_TIMEOUT|CF_WRITE_ACTIVITY|CF_WAKE_ONCE|CF_WRITE_EVENT) +#define CF_MASK_ANALYSER (CF_READ_ATTACHED|CF_READ_ACTIVITY|CF_READ_TIMEOUT|CF_ANA_TIMEOUT|CF_WRITE_ACTIVITY|CF_WAKE_ONCE) /* Mask for static flags which cause analysers to be woken up when they change */ #define CF_MASK_STATIC (CF_SHUTR|CF_SHUTW|CF_SHUTR_NOW|CF_SHUTW_NOW) diff --git a/src/proto_http.c b/src/proto_http.c index e0ae82ee90..8ac4c018ba 100644 --- a/src/proto_http.c +++ b/src/proto_http.c @@ -3694,7 +3694,7 @@ void http_end_txn_clean_session(struct stream *s) s->si[1].exp = TICK_ETERNITY; s->si[1].flags &= SI_FL_ISBACK | SI_FL_DONT_WAKE; /* we're in the context of process_stream */ s->req.flags &= ~(CF_SHUTW|CF_SHUTW_NOW|CF_AUTO_CONNECT|CF_WRITE_ERROR|CF_STREAMER|CF_STREAMER_FAST|CF_NEVER_WAIT|CF_WAKE_CONNECT|CF_WROTE_DATA); - s->res.flags &= ~(CF_SHUTR|CF_SHUTR_NOW|CF_READ_ATTACHED|CF_READ_ERROR|CF_READ_NOEXP|CF_STREAMER|CF_STREAMER_FAST|CF_WRITE_PARTIAL|CF_NEVER_WAIT|CF_WROTE_DATA|CF_WRITE_EVENT); + s->res.flags &= ~(CF_SHUTR|CF_SHUTR_NOW|CF_READ_ATTACHED|CF_READ_ERROR|CF_READ_NOEXP|CF_STREAMER|CF_STREAMER_FAST|CF_WRITE_PARTIAL|CF_NEVER_WAIT|CF_WROTE_DATA); s->flags &= ~(SF_DIRECT|SF_ASSIGNED|SF_ADDR_SET|SF_BE_ASSIGNED|SF_FORCE_PRST|SF_IGNORE_PRST); s->flags &= ~(SF_CURR_SESS|SF_REDIRECTABLE|SF_SRV_REUSED); s->flags &= ~(SF_ERR_MASK|SF_FINST_MASK|SF_REDISP); diff --git a/src/stream.c b/src/stream.c index 8df5c75639..ad1ccbee40 100644 --- a/src/stream.c +++ b/src/stream.c @@ -628,7 +628,7 @@ static int sess_update_st_con_tcp(struct stream *s) if (!(req->flags & CF_WROTE_DATA) && unlikely((rep->flags & CF_SHUTW) || ((req->flags & CF_SHUTW_NOW) && /* FIXME: this should not prevent a connection from establishing */ - ((!(req->flags & (CF_WRITE_ACTIVITY|CF_WRITE_EVENT)) && channel_is_empty(req)) || + ((!(req->flags & CF_WRITE_ACTIVITY) && channel_is_empty(req)) || ((s->be->options & PR_O_ABRT_CLOSE) && !(s->si[0].flags & SI_FL_CLEAN_ABRT)))))) { /* give up */ si_shutw(si); @@ -639,7 +639,7 @@ static int sess_update_st_con_tcp(struct stream *s) } /* we need to wait a bit more if there was no activity either */ - if (!(req->flags & (CF_WRITE_ACTIVITY|CF_WRITE_EVENT))) + if (!(req->flags & CF_WRITE_ACTIVITY)) return 1; /* OK, this means that a connection succeeded. The caller will be @@ -1759,7 +1759,7 @@ redo: */ if (!((req->flags | res->flags) & (CF_SHUTR|CF_READ_ACTIVITY|CF_READ_TIMEOUT|CF_SHUTW| - CF_WRITE_ACTIVITY|CF_WRITE_EVENT|CF_WRITE_TIMEOUT|CF_ANA_TIMEOUT)) && + CF_WRITE_ACTIVITY|CF_WRITE_TIMEOUT|CF_ANA_TIMEOUT)) && !((si_f->flags | si_b->flags) & (SI_FL_EXP|SI_FL_ERR)) && ((s->pending_events & TASK_WOKEN_ANY) == TASK_WOKEN_TIMER)) { si_f->flags &= ~SI_FL_DONT_WAKE; @@ -2477,8 +2477,8 @@ redo: if (si_b->state == SI_ST_EST) si_update(si_b); - req->flags &= ~(CF_READ_NULL|CF_READ_PARTIAL|CF_WRITE_NULL|CF_WRITE_PARTIAL|CF_READ_ATTACHED|CF_WRITE_EVENT); - res->flags &= ~(CF_READ_NULL|CF_READ_PARTIAL|CF_WRITE_NULL|CF_WRITE_PARTIAL|CF_READ_ATTACHED|CF_WRITE_EVENT); + req->flags &= ~(CF_READ_NULL|CF_READ_PARTIAL|CF_WRITE_NULL|CF_WRITE_PARTIAL|CF_READ_ATTACHED); + res->flags &= ~(CF_READ_NULL|CF_READ_PARTIAL|CF_WRITE_NULL|CF_WRITE_PARTIAL|CF_READ_ATTACHED); si_f->prev_state = si_f->state; si_b->prev_state = si_b->state; si_f->flags &= ~(SI_FL_ERR|SI_FL_EXP); diff --git a/src/stream_interface.c b/src/stream_interface.c index 59827ecadc..e585b1fe4f 100644 --- a/src/stream_interface.c +++ b/src/stream_interface.c @@ -496,10 +496,6 @@ void stream_int_notify(struct stream_interface *si) * the buffer is full. We must not stop based on input data alone because * an HTTP parser might need more data to complete the parsing. */ - - /* ensure it's only set if a write attempt has succeeded */ - ic->flags &= ~CF_WRITE_PARTIAL; - if (!channel_is_empty(ic) && (si_opposite(si)->flags & SI_FL_WAIT_DATA) && (ci_data(ic) == 0 || ic->pipe)) { @@ -544,7 +540,7 @@ void stream_int_notify(struct stream_interface *si) /* changes on the consumption side */ (oc->flags & (CF_WRITE_NULL|CF_WRITE_ERROR)) || - ((oc->flags & (CF_WRITE_ACTIVITY|CF_WRITE_EVENT)) && + ((oc->flags & CF_WRITE_ACTIVITY) && ((oc->flags & CF_SHUTW) || ((oc->flags & CF_WAKE_WRITE) && (si_opposite(si)->state != SI_ST_EST || @@ -641,13 +637,10 @@ int si_cs_send(struct conn_stream *cs) if (si_oc(si)->flags & CF_SHUTW) return 1; - /* ensure it's only set if a write attempt has succeeded */ - oc->flags &= ~CF_WRITE_PARTIAL; - if (oc->pipe && conn->xprt->snd_pipe && conn->mux->snd_pipe) { ret = conn->mux->snd_pipe(cs, oc->pipe); if (ret > 0) { - oc->flags |= CF_WRITE_PARTIAL | CF_WROTE_DATA | CF_WRITE_EVENT; + oc->flags |= CF_WRITE_PARTIAL | CF_WROTE_DATA; did_send = 1; } @@ -696,7 +689,7 @@ int si_cs_send(struct conn_stream *cs) ret = cs->conn->mux->snd_buf(cs, &oc->buf, co_data(oc), send_flag); if (ret > 0) { did_send = 1; - oc->flags |= CF_WRITE_PARTIAL | CF_WROTE_DATA | CF_WRITE_EVENT; + oc->flags |= CF_WRITE_PARTIAL | CF_WROTE_DATA; co_set_data(oc, co_data(oc) - ret); c_realign_if_empty(oc); @@ -965,9 +958,6 @@ static void stream_int_chk_snd_conn(struct stream_interface *si) struct channel *oc = si_oc(si); struct conn_stream *cs = __objt_cs(si->end); - /* ensure it's only set if a write attempt has succeeded */ - oc->flags &= ~CF_WRITE_PARTIAL; - if (unlikely(si->state > SI_ST_EST || (oc->flags & CF_SHUTW))) return;