This is an automated email from the ASF dual-hosted git repository.
jvanderzee pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/trafficserver.git
The following commit(s) were added to refs/heads/master by this push:
new 4b0ffc2b5b Clarify cache_config_agg_write_backlog (#10991)
4b0ffc2b5b is described below
commit 4b0ffc2b5b4d89af5c088194c99baca7d9bb562f
Author: JosiahWI <[email protected]>
AuthorDate: Mon Jun 17 17:03:02 2024 -0500
Clarify cache_config_agg_write_backlog (#10991)
* Add comments to explain cache_config_agg_backlog
This notes down my understanding of the discrepency in usages of this
configuration parameter after inspection of the relevant continuations.
* Implement changes requested by Bryan Call
* Remove my name from the comment.
---
src/iocore/cache/P_CacheVol.h | 5 +++--
src/iocore/cache/Stripe.cc | 9 +++++++++
2 files changed, 12 insertions(+), 2 deletions(-)
diff --git a/src/iocore/cache/P_CacheVol.h b/src/iocore/cache/P_CacheVol.h
index 9ef1397be7..aa3a5a8614 100644
--- a/src/iocore/cache/P_CacheVol.h
+++ b/src/iocore/cache/P_CacheVol.h
@@ -288,8 +288,9 @@ public:
* - Adding a Doc to the virtual connection header would exceed the
* maximum fragment size.
* - vc->f.readers is not set (this virtual connection is not an
evacuator),
- * the writes waiting to be aggregated exceed the maximum backlog,
- * and the virtual connection has a non-zero write length.
+ * is full, the writes waiting to be aggregated exceed the maximum
+ * backlog plus the space in the aggregatation buffer, and the virtual
+ * connection has a non-zero write length.
*
* @param vc: The virtual connection.
* @return: Returns true if the operation was successfull, otherwise false.
diff --git a/src/iocore/cache/Stripe.cc b/src/iocore/cache/Stripe.cc
index 79c3d0a35f..e9feed52b3 100644
--- a/src/iocore/cache/Stripe.cc
+++ b/src/iocore/cache/Stripe.cc
@@ -934,6 +934,15 @@ Stripe::add_writer(CacheVC *vc)
{
ink_assert(vc);
this->_write_buffer.add_bytes_pending_aggregation(vc->agg_len);
+ // An extra AGG_SIZE is added to the backlog here, but not in
+ // open_write, at the time I'm writing this comment. I venture to
+ // guess that because the stripe lock may be released between
+ // open_write and add_writer (I have checked this), the number of
+ // bytes pending aggregation lags and is inaccurate. Therefore the
+ // check in open_write is too permissive, and once we get to add_writer
+ // and update our bytes pending, we may discover we have more backlog
+ // than we thought we did. The solution to the problem was to permit
+ // an aggregation buffer extra of backlog here. That's my analysis.
bool agg_error =
(vc->agg_len > AGG_SIZE || vc->header_len + sizeof(Doc) > MAX_FRAG_SIZE ||
(!vc->f.readers && (this->_write_buffer.get_bytes_pending_aggregation() >
cache_config_agg_write_backlog + AGG_SIZE) &&
