Copilot commented on code in PR #12717:
URL: https://github.com/apache/trafficserver/pull/12717#discussion_r2582871738
##########
src/iocore/cache/CacheHosting.cc:
##########
@@ -765,6 +775,29 @@ ConfigVolumes::BuildListFromString(char *config_file_path,
char *file_buf)
err = "Unexpected end of line";
break;
}
+ } else if (strcasecmp(tmp, "ram_cache_size") == 0) { // match
ram_cache_size
+ tmp += 15;
+ ram_cache_size = ink_atoi64(tmp);
+
+ if (ram_cache_size < 0) {
+ err = "Invalid ram_cache_size value (must be >= 0)";
+ break;
+ }
Review Comment:
The parsing logic uses `ink_atoi64` which may return 0 for invalid input,
but the validation only checks `< 0`. This means malformed input like
"ram_cache_size=abc" would silently set the value to 0 (disabling RAM cache)
rather than reporting an error. Consider checking if the input string starts
with a digit before calling `ink_atoi64`, or validate that the result is
reasonable given the input.
##########
src/iocore/cache/CacheHosting.cc:
##########
@@ -765,6 +775,29 @@ ConfigVolumes::BuildListFromString(char *config_file_path,
char *file_buf)
err = "Unexpected end of line";
break;
}
+ } else if (strcasecmp(tmp, "ram_cache_size") == 0) { // match
ram_cache_size
+ tmp += 15;
+ ram_cache_size = ink_atoi64(tmp);
+
+ if (ram_cache_size < 0) {
+ err = "Invalid ram_cache_size value (must be >= 0)";
+ break;
+ }
+ // Note: ram_cache_size=0 disables RAM cache for this volume, same as
ramcache=false
+ while (*tmp && (ParseRules::is_digit(*tmp) || strchr("kmgtKMGT",
*tmp))) {
+ tmp++;
+ }
+ } else if (strcasecmp(tmp, "ram_cache_cutoff") == 0) { // match
ram_cache_cutoff
+ tmp += 17;
+ ram_cache_cutoff = ink_atoi64(tmp);
+
+ if (ram_cache_cutoff < 0) {
+ err = "Invalid ram_cache_cutoff value (must be >= 0)";
+ break;
+ }
Review Comment:
The parsing logic uses `ink_atoi64` which may return 0 for invalid input,
but the validation only checks `< 0`. This means malformed input like
"ram_cache_cutoff=xyz" would silently set the value to 0 rather than reporting
an error. Consider checking if the input string starts with a digit before
calling `ink_atoi64`, or validate that the result is reasonable given the input.
##########
src/iocore/cache/CacheHosting.cc:
##########
@@ -741,16 +743,24 @@ ConfigVolumes::BuildListFromString(char
*config_file_path, char *file_buf)
}
} else if (strcasecmp(tmp, "avg_obj_size") == 0) { // match avg_obj_size
tmp += 13;
- avg_obj_size = atoi(tmp);
+ avg_obj_size = static_cast<int>(ink_atoi64(tmp));
- while (ParseRules::is_digit(*tmp)) {
+ if (avg_obj_size < 0) {
+ err = "Invalid avg_obj_size value (must be >= 0)";
+ break;
+ }
+ while (*tmp && (ParseRules::is_digit(*tmp) || strchr("kmgtKMGT",
*tmp))) {
tmp++;
}
} else if (strcasecmp(tmp, "fragment_size") == 0) { // match
fragment_size
tmp += 14;
- fragment_size = atoi(tmp);
+ fragment_size = static_cast<int>(ink_atoi64(tmp));
- while (ParseRules::is_digit(*tmp)) {
+ if (fragment_size < 0) {
+ err = "Invalid fragment_size value (must be >= 0)";
+ break;
+ }
Review Comment:
Similar to other size parameters, the parsing logic uses `ink_atoi64` which
may return 0 for invalid input, but the validation only checks `< 0`. This
means malformed input like "fragment_size=abc" would silently set the value to
0 rather than reporting an error. Consider checking if the input string starts
with a digit before calling `ink_atoi64`.
##########
src/iocore/cache/Cache.cc:
##########
@@ -745,41 +745,70 @@ CacheVConnection::CacheVConnection() :
VConnection(nullptr) {}
// if generic_host_rec.stripes == nullptr, what do we do???
StripeSM *
-Cache::key_to_stripe(const CacheKey *key, std::string_view hostname) const
+Cache::key_to_stripe(const CacheKey *key, std::string_view hostname, int
volume_override) const
{
ReplaceablePtr<CacheHostTable>::ScopedReader hosttable(&this->hosttable);
- uint32_t h = (key->slice32(2) >> DIR_TAG_WIDTH) %
STRIPE_HASH_TABLE_SIZE;
- unsigned short *hash_table = hosttable->gen_host_rec.vol_hash_table;
- const CacheHostRecord *host_rec = &hosttable->gen_host_rec;
+ uint32_t h = (key->slice32(2) >> DIR_TAG_WIDTH)
% STRIPE_HASH_TABLE_SIZE;
+ unsigned short *hash_table =
hosttable->gen_host_rec.vol_hash_table;
+ const CacheHostRecord *host_rec = &hosttable->gen_host_rec;
+ StripeSM *selected_stripe = nullptr;
+ bool remap_selection = false;
+
+ if (volume_override > 0) {
+ for (int i = 0; i < host_rec->num_cachevols; i++) {
+ if (host_rec->cp[i] && host_rec->cp[i]->vol_number == volume_override) {
+ for (int j = 0; j < host_rec->num_vols; j++) {
+ if (host_rec->stripes[j] && host_rec->stripes[j]->cache_vol &&
+ host_rec->stripes[j]->cache_vol->vol_number == volume_override) {
+ selected_stripe = host_rec->stripes[j];
+ remap_selection = true;
+ break;
+ }
+ }
+ break;
+ }
+ }
+
+ if (!selected_stripe) {
+ Warning("Invalid volume override %d, volume configured but no stripes
available. Falling back to hostname-based selection.",
+ volume_override);
Review Comment:
The warning message states "volume configured but no stripes available", but
this warning can also occur when the volume number doesn't exist at all (not
just when it has no stripes). The error message should distinguish between
"volume not found" and "volume found but has no stripes" to make debugging
easier.
##########
src/iocore/cache/Cache.cc:
##########
@@ -745,41 +745,70 @@ CacheVConnection::CacheVConnection() :
VConnection(nullptr) {}
// if generic_host_rec.stripes == nullptr, what do we do???
StripeSM *
-Cache::key_to_stripe(const CacheKey *key, std::string_view hostname) const
+Cache::key_to_stripe(const CacheKey *key, std::string_view hostname, int
volume_override) const
{
ReplaceablePtr<CacheHostTable>::ScopedReader hosttable(&this->hosttable);
- uint32_t h = (key->slice32(2) >> DIR_TAG_WIDTH) %
STRIPE_HASH_TABLE_SIZE;
- unsigned short *hash_table = hosttable->gen_host_rec.vol_hash_table;
- const CacheHostRecord *host_rec = &hosttable->gen_host_rec;
+ uint32_t h = (key->slice32(2) >> DIR_TAG_WIDTH)
% STRIPE_HASH_TABLE_SIZE;
+ unsigned short *hash_table =
hosttable->gen_host_rec.vol_hash_table;
+ const CacheHostRecord *host_rec = &hosttable->gen_host_rec;
+ StripeSM *selected_stripe = nullptr;
+ bool remap_selection = false;
+
+ if (volume_override > 0) {
+ for (int i = 0; i < host_rec->num_cachevols; i++) {
+ if (host_rec->cp[i] && host_rec->cp[i]->vol_number == volume_override) {
+ for (int j = 0; j < host_rec->num_vols; j++) {
+ if (host_rec->stripes[j] && host_rec->stripes[j]->cache_vol &&
+ host_rec->stripes[j]->cache_vol->vol_number == volume_override) {
+ selected_stripe = host_rec->stripes[j];
+ remap_selection = true;
+ break;
+ }
+ }
+ break;
+ }
+ }
Review Comment:
The volume override selection logic has inefficient nested loops. When a
matching cache volume is found in the outer loop (line 760), the inner loop
searches through all stripes. However, once the outer loop finds a match and
breaks (line 769), it might not have found any stripes. The algorithm should
select the first stripe of the matching volume using a hash-based or
round-robin approach rather than always selecting the first stripe found. This
could lead to uneven distribution of requests across stripes within a volume.
##########
doc/admin-guide/files/volume.config.en.rst:
##########
@@ -73,21 +73,66 @@ Optional directory entry sizing
You can also add an option ``avg_obj_size=<size>`` to the volume configuration
line. This overrides the global
:ts:cv:`proxy.config.cache.min_average_object_size`
-configuration for this volume. This is useful if you have a volume that is
dedicated
-for say very small objects, and you need a lot of directory entries to store
them.
+configuration for this volume. The size supports multipliers (K, M, G, T) for
+convenience (e.g., ``avg_obj_size=64K`` or ``avg_obj_size=1M``). This is useful
+if you have a volume that is dedicated for say very small objects, and you need
+a lot of directory entries to store them.
Optional fragment size setting
------------------------------
You can also add an option ``fragment_size=<size>`` to the volume configuration
line. This overrides the global
:ts:cv:`proxy.config.cache.target_fragment_size`
-configuration for this volume. This allows for a smaller, or larger, fragment
size
-for a particular volume. This may be useful together with ``avg_obj_size`` as
well,
-since a larger fragment size could reduce the number of directory entries
needed
-for a large object.
+configuration for this volume. The size supports multipliers (K, M, G, T) for
+convenience (e.g., ``fragment_size=512K`` or ``fragment_size=2M``). This allows
+for a smaller, or larger, fragment size for a particular volume. This may be
+useful together with ``avg_obj_size`` as well, since a larger fragment size
could
+reduce the number of directory entries needed for a large object.
Note that this setting has a maximmum value of 4MB.
Review Comment:
Spelling error: "maximmum" should be "maximum".
```suggestion
Note that this setting has a maximum value of 4MB.
```
##########
src/proxy/http/remap/RemapConfig.cc:
##########
@@ -1197,12 +1205,37 @@ remap_parse_config_bti(const char *path,
BUILD_TABLE_INFO *bti)
if ((bti->remap_optflg & REMAP_OPTFLG_MAP_ID) != 0) {
int idx = 0;
int ret = remap_check_option(bti->argv, bti->argc, REMAP_OPTFLG_MAP_ID,
&idx);
+
if (ret & REMAP_OPTFLG_MAP_ID) {
- char *c = strchr(bti->argv[idx], static_cast<int>('='));
+ char *c = strchr(bti->argv[idx], static_cast<int>('='));
+
new_mapping->map_id = static_cast<unsigned int>(atoi(++c));
}
}
+ // Parse @volume= option
+ new_mapping->cache_volume = -1;
+ if ((bti->remap_optflg & REMAP_OPTFLG_VOLUME) != 0) {
+ int idx = 0;
+ int ret = remap_check_option(bti->argv, bti->argc, REMAP_OPTFLG_VOLUME,
&idx);
+
+ if (ret & REMAP_OPTFLG_VOLUME) {
+ char *c = strchr(bti->argv[idx], static_cast<int>('='));
+ int volume_num = atoi(++c);
+
+ new_mapping->cache_volume = volume_num;
+
+ // Validate volume number (basic validation - detailed validation
later)
+ if (volume_num < 1) {
+ snprintf(errStrBuf, sizeof(errStrBuf), "Invalid cache volume number
%d at line %d (must be >= 1)", volume_num, cln + 1);
Review Comment:
The documentation states that volume numbers must be between 1 and 255, but
the code validation at line 1229 only checks `volume_num < 1`. There's no upper
bound check for 255, which could allow invalid volume numbers to be configured.
An upper bound validation should be added to match the documented constraint.
```suggestion
if (volume_num < 1 || volume_num > 255) {
snprintf(errStrBuf, sizeof(errStrBuf), "Invalid cache volume
number %d at line %d (must be between 1 and 255)", volume_num, cln + 1);
```
##########
src/iocore/cache/CacheProcessor.cc:
##########
@@ -1470,22 +1504,49 @@ CacheProcessor::cacheInitialized()
uint64_t total_cache_bytes = 0; // bytes that can used in total_size
uint64_t total_direntries = 0; // all the direntries in the cache
uint64_t used_direntries = 0; // and used
- uint64_t total_ram_cache_bytes = 0;
+ uint64_t total_ram_cache_bytes = 0; // Total RAM cache size across all
volumes
+ uint64_t shared_cache_size = 0; // Total cache size of volumes
without explicit RAM allocations
+
+ // Calculate total cache size of volumes without explicit RAM allocations
+ if (http_ram_cache_size > 0) {
+ for (int i = 0; i < gnstripes; i++) {
+ if (gstripes[i]->cache_vol->ram_cache_size <= 0) {
+ shared_cache_size += (gstripes[i]->len >> STORE_BLOCK_SHIFT);
+ }
+ }
+ Dbg(dbg_ctl_cache_init, "Shared cache size (for RAM pool
distribution): %" PRId64 " blocks", shared_cache_size);
+ }
for (int i = 0; i < gnstripes; i++) {
StripeSM *stripe = gstripes[i];
int64_t ram_cache_bytes = 0;
- if (stripe->cache_vol->ramcache_enabled) {
- if (http_ram_cache_size == 0) {
+ // If RAM cache enabled, check if this volume has a private RAM cache
allocation
+ if (stripe->cache_vol->ramcache_enabled &&
stripe->cache_vol->ram_cache_size != 0) {
+ if (stripe->cache_vol->ram_cache_size > 0) {
+ int64_t volume_stripe_count = 0;
+
+ for (int j = 0; j < gnstripes; j++) {
+ if (gstripes[j]->cache_vol == stripe->cache_vol) {
+ volume_stripe_count++;
+ }
+ }
+
+ if (volume_stripe_count > 0) {
+ ram_cache_bytes = stripe->cache_vol->ram_cache_size /
volume_stripe_count;
+ Dbg(dbg_ctl_cache_init, "Volume %d stripe %d using private RAM
allocation: %" PRId64 " bytes (%" PRId64 " MB)",
+ stripe->cache_vol->vol_number, i, ram_cache_bytes,
ram_cache_bytes / (1024 * 1024));
+ }
+ } else if (http_ram_cache_size == 0) {
// AUTO_SIZE_RAM_CACHE
ram_cache_bytes = stripe->dirlen() * DEFAULT_RAM_CACHE_MULTIPLIER;
} else {
+ // Use shared pool allocation - distribute only among volumes
without explicit allocations
ink_assert(stripe->cache != nullptr);
+ int64_t divisor = (shared_cache_size > 0) ? shared_cache_size :
theCache->cache_size;
+ double factor =
static_cast<double>(static_cast<int64_t>(stripe->len >> STORE_BLOCK_SHIFT)) /
divisor;
- double factor =
static_cast<double>(static_cast<int64_t>(stripe->len >> STORE_BLOCK_SHIFT)) /
theCache->cache_size;
- Dbg(dbg_ctl_cache_init, "factor = %f", factor);
-
+ Dbg(dbg_ctl_cache_init, "factor = %f (divisor = %" PRId64 ")",
factor, divisor);
ram_cache_bytes = static_cast<int64_t>(http_ram_cache_size *
factor);
Review Comment:
The shared_cache_size calculation at line 1546 uses a fallback to
`theCache->cache_size` when `shared_cache_size` is 0, but this can lead to
incorrect RAM cache allocation. If all volumes have explicit RAM allocations
(making shared_cache_size = 0), stripes would still be allocated RAM from
http_ram_cache_size using the full cache size as divisor. This should either
skip allocation or log a warning since there should be no shared RAM pool
available in this case.
##########
src/iocore/cache/CacheProcessor.cc:
##########
@@ -1470,22 +1504,49 @@ CacheProcessor::cacheInitialized()
uint64_t total_cache_bytes = 0; // bytes that can used in total_size
uint64_t total_direntries = 0; // all the direntries in the cache
uint64_t used_direntries = 0; // and used
- uint64_t total_ram_cache_bytes = 0;
+ uint64_t total_ram_cache_bytes = 0; // Total RAM cache size across all
volumes
+ uint64_t shared_cache_size = 0; // Total cache size of volumes
without explicit RAM allocations
+
+ // Calculate total cache size of volumes without explicit RAM allocations
+ if (http_ram_cache_size > 0) {
+ for (int i = 0; i < gnstripes; i++) {
+ if (gstripes[i]->cache_vol->ram_cache_size <= 0) {
+ shared_cache_size += (gstripes[i]->len >> STORE_BLOCK_SHIFT);
+ }
+ }
+ Dbg(dbg_ctl_cache_init, "Shared cache size (for RAM pool
distribution): %" PRId64 " blocks", shared_cache_size);
+ }
for (int i = 0; i < gnstripes; i++) {
StripeSM *stripe = gstripes[i];
int64_t ram_cache_bytes = 0;
- if (stripe->cache_vol->ramcache_enabled) {
- if (http_ram_cache_size == 0) {
+ // If RAM cache enabled, check if this volume has a private RAM cache
allocation
+ if (stripe->cache_vol->ramcache_enabled &&
stripe->cache_vol->ram_cache_size != 0) {
+ if (stripe->cache_vol->ram_cache_size > 0) {
+ int64_t volume_stripe_count = 0;
+
+ for (int j = 0; j < gnstripes; j++) {
+ if (gstripes[j]->cache_vol == stripe->cache_vol) {
+ volume_stripe_count++;
+ }
+ }
+
+ if (volume_stripe_count > 0) {
+ ram_cache_bytes = stripe->cache_vol->ram_cache_size /
volume_stripe_count;
+ Dbg(dbg_ctl_cache_init, "Volume %d stripe %d using private RAM
allocation: %" PRId64 " bytes (%" PRId64 " MB)",
+ stripe->cache_vol->vol_number, i, ram_cache_bytes,
ram_cache_bytes / (1024 * 1024));
+ }
Review Comment:
The volume stripe count calculation (lines 1527-1533) is inefficient as it
iterates through all stripes for each stripe being processed, resulting in
O(n²) complexity. This count should be pre-calculated once for each volume
before the main loop to improve performance, especially for systems with many
stripes.
##########
src/iocore/cache/CacheProcessor.cc:
##########
@@ -1442,22 +1444,54 @@ CacheProcessor::cacheInitialized()
}
}
+ // Calculate total private RAM allocations from per-volume configurations
int64_t http_ram_cache_size = 0;
+ int64_t total_private_ram = 0;
+
+ if (cache_config_ram_cache_size != AUTO_SIZE_RAM_CACHE) {
+ CacheVol *cp = cp_list.head;
+
+ for (; cp; cp = cp->link.next) {
+ if (cp->ram_cache_size > 0) {
+ total_private_ram += cp->ram_cache_size;
+ Dbg(dbg_ctl_cache_init, "Volume %d has private RAM allocation: %"
PRId64 " bytes (%" PRId64 " MB)", cp->vol_number,
+ cp->ram_cache_size, cp->ram_cache_size / (1024 * 1024));
+ }
+ }
+
+ if (total_private_ram > 0) {
+ Dbg(dbg_ctl_cache_init, "Total private RAM allocations: %" PRId64 "
bytes (%" PRId64 " MB)", total_private_ram,
+ total_private_ram / (1024 * 1024));
+ }
+ }
- // let us calculate the Size
if (cache_config_ram_cache_size == AUTO_SIZE_RAM_CACHE) {
Dbg(dbg_ctl_cache_init, "cache_config_ram_cache_size ==
AUTO_SIZE_RAM_CACHE");
} else {
// we got configured memory size
// TODO, should we check the available system memories, or you will
// OOM or swapout, that is not a good situation for the server
Dbg(dbg_ctl_cache_init, "%" PRId64 " != AUTO_SIZE_RAM_CACHE",
cache_config_ram_cache_size);
- http_ram_cache_size =
- static_cast<int64_t>((static_cast<double>(theCache->cache_size) /
total_size) * cache_config_ram_cache_size);
+
+ // Calculate shared pool: global RAM cache size minus private
allocations
+ int64_t shared_pool = cache_config_ram_cache_size - total_private_ram;
+
+ if (shared_pool < 0) {
+ Warning("Total private RAM cache allocations (%" PRId64 " bytes)
exceed global ram_cache.size (%" PRId64 " bytes). "
+ "Using global limit. Consider increasing
proxy.config.cache.ram_cache.size.",
+ total_private_ram, cache_config_ram_cache_size);
+ shared_pool = cache_config_ram_cache_size; // Fall back to
using the global pool for all
+ total_private_ram = 0;
Review Comment:
The documentation states that when private RAM allocations exceed the global
limit, "the private allocations are disabled, falling back to the standard
shared allocation" (lines 114-116). However, the code at lines 1483-1484 sets
`shared_pool = cache_config_ram_cache_size` and `total_private_ram = 0`, which
doesn't actually disable private allocations. The volumes with `ram_cache_size
> 0` will still use their private allocations in the loop starting at line
1520. The code should either honor the private allocations up to the limit or
truly disable them as documented.
```suggestion
total_private_ram = 0;
// Disable private RAM allocations for all volumes
for (int vol_idx = 0; vol_idx < gvols->n_volumes; ++vol_idx) {
gvols->volumes[vol_idx]->ram_cache_size = 0;
}
```
##########
src/proxy/http/remap/RemapConfig.cc:
##########
@@ -1197,12 +1205,37 @@ remap_parse_config_bti(const char *path,
BUILD_TABLE_INFO *bti)
if ((bti->remap_optflg & REMAP_OPTFLG_MAP_ID) != 0) {
int idx = 0;
int ret = remap_check_option(bti->argv, bti->argc, REMAP_OPTFLG_MAP_ID,
&idx);
+
if (ret & REMAP_OPTFLG_MAP_ID) {
- char *c = strchr(bti->argv[idx], static_cast<int>('='));
+ char *c = strchr(bti->argv[idx], static_cast<int>('='));
+
new_mapping->map_id = static_cast<unsigned int>(atoi(++c));
}
}
+ // Parse @volume= option
+ new_mapping->cache_volume = -1;
+ if ((bti->remap_optflg & REMAP_OPTFLG_VOLUME) != 0) {
+ int idx = 0;
+ int ret = remap_check_option(bti->argv, bti->argc, REMAP_OPTFLG_VOLUME,
&idx);
+
+ if (ret & REMAP_OPTFLG_VOLUME) {
+ char *c = strchr(bti->argv[idx], static_cast<int>('='));
+ int volume_num = atoi(++c);
Review Comment:
The use of `strchr` to find '=' is redundant since `remap_check_option`
already validated that the option starts with "volume=" (line 832). The value
can be obtained directly using `&bti->argv[idx][7]` (similar to how it's done
in `remap_check_option` line 837), eliminating the unnecessary `strchr` call
and improving code clarity and efficiency.
```suggestion
int volume_num = atoi(&bti->argv[idx][7]);
```
##########
src/iocore/cache/CacheHosting.cc:
##########
@@ -741,16 +743,24 @@ ConfigVolumes::BuildListFromString(char
*config_file_path, char *file_buf)
}
} else if (strcasecmp(tmp, "avg_obj_size") == 0) { // match avg_obj_size
tmp += 13;
- avg_obj_size = atoi(tmp);
+ avg_obj_size = static_cast<int>(ink_atoi64(tmp));
- while (ParseRules::is_digit(*tmp)) {
+ if (avg_obj_size < 0) {
+ err = "Invalid avg_obj_size value (must be >= 0)";
+ break;
+ }
Review Comment:
Similar to ram_cache_size and ram_cache_cutoff, the parsing logic uses
`ink_atoi64` which may return 0 for invalid input, but the validation only
checks `< 0`. This means malformed input like "avg_obj_size=xyz" would silently
set the value to 0 rather than reporting an error. Consider checking if the
input string starts with a digit before calling `ink_atoi64`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
