You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by GitBox <gi...@apache.org> on 2020/07/22 18:37:21 UTC
[GitHub] [trafficserver] gtenev opened a new pull request #7028: Fixed problem with all "forced" volumes cache
gtenev opened a new pull request #7028:
URL: https://github.com/apache/trafficserver/pull/7028
Fixed a problem with cache volume configurations where all volumes are
forced to a span (disk) in `storage.config` and percentage size is used
in `volume.config`. Sample configuration:
```
storage.config:
var/trafficserver/disks/disk1 512M volume=1
var/trafficserver/disks/disk2 512M volume=1
var/trafficserver/disks/disk3 512M volume=2
volume.config:
volume=1 scheme=http size=100%
volume=2 scheme=http size=512
```
Since "forced" volumes are now excluded from "total cache space" used
for volume size percentage calculations when all spans (disks) are
"forced" to volumes the "total cache space" becomes 0 and then volume
sizes calculated as percentage become 0. This patch fixes the issue and
also handles the case when a single volume is forced to multiple spans.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] zwoop merged pull request #7028: Fixed problem with all "forced" volumes cache
Posted by GitBox <gi...@apache.org>.
zwoop merged pull request #7028:
URL: https://github.com/apache/trafficserver/pull/7028
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] SolidWallOfCode commented on a change in pull request #7028: Fixed problem with all "forced" volumes cache
Posted by GitBox <gi...@apache.org>.
SolidWallOfCode commented on a change in pull request #7028:
URL: https://github.com/apache/trafficserver/pull/7028#discussion_r459657293
##########
File path: iocore/cache/Cache.cc
##########
@@ -2596,21 +2596,26 @@ cplist_update()
if (nullptr == config_vol->cachep) {
// Find out if this is a forced volume assigned exclusively to a span which was cleared (hence not referenced in cp_list).
// Note: non-exclusive cleared spans are not handled here, only the "exclusive"
+ bool forced_volume = false;
for (int d_no = 0; d_no < gndisks; d_no++) {
if (gdisks[d_no]->forced_volume_num == config_vol->number) {
- CacheVol *new_cp = new CacheVol();
- if (nullptr != new_cp) {
- new_cp->disk_vols = (DiskVol **)ats_malloc(gndisks * sizeof(DiskVol *));
- if (nullptr != new_cp->disk_vols) {
- memset(new_cp->disk_vols, 0, gndisks * sizeof(DiskVol *));
- new_cp->vol_number = config_vol->number;
- new_cp->scheme = config_vol->scheme;
- config_vol->cachep = new_cp;
- fillExclusiveDisks(config_vol->cachep);
- cp_list.enqueue(new_cp);
- } else {
- delete new_cp;
- }
+ forced_volume = true;
+ }
+ }
+
+ if (forced_volume) {
+ CacheVol *new_cp = new CacheVol();
+ if (nullptr != new_cp) {
Review comment:
OK, not really needed - it was "Outside Pull Request Scope".
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] gtenev commented on pull request #7028: Fixed problem with all "forced" volumes cache
Posted by GitBox <gi...@apache.org>.
gtenev commented on pull request #7028:
URL: https://github.com/apache/trafficserver/pull/7028#issuecomment-662720362
@SolidWallOfCode, addressed your feedback please review again.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] SolidWallOfCode commented on a change in pull request #7028: Fixed problem with all "forced" volumes cache
Posted by GitBox <gi...@apache.org>.
SolidWallOfCode commented on a change in pull request #7028:
URL: https://github.com/apache/trafficserver/pull/7028#discussion_r459078764
##########
File path: iocore/cache/Cache.cc
##########
@@ -2754,14 +2759,36 @@ cplist_reconfigure()
Warning("no volumes created");
return -1;
}
- int64_t space_in_blks = static_cast<int64_t>(((config_vol->percent / percent_remaining)) * tot_space_in_blks);
+
+ // Find if the volume is forced and if it is then calculate the total forced volume size.
+ // Forced volumes take the whole span (disk) also sum all disk space this volume is forced to.
+ bool forcedVolume = false;
Review comment:
Why bother with this `bool`? Isn't `tot_forced_space_in_blks == 0` a reasonable substitute?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] SolidWallOfCode commented on a change in pull request #7028: Fixed problem with all "forced" volumes cache
Posted by GitBox <gi...@apache.org>.
SolidWallOfCode commented on a change in pull request #7028:
URL: https://github.com/apache/trafficserver/pull/7028#discussion_r459525926
##########
File path: iocore/cache/Cache.cc
##########
@@ -2596,21 +2596,26 @@ cplist_update()
if (nullptr == config_vol->cachep) {
// Find out if this is a forced volume assigned exclusively to a span which was cleared (hence not referenced in cp_list).
// Note: non-exclusive cleared spans are not handled here, only the "exclusive"
+ bool forced_volume = false;
for (int d_no = 0; d_no < gndisks; d_no++) {
if (gdisks[d_no]->forced_volume_num == config_vol->number) {
- CacheVol *new_cp = new CacheVol();
- if (nullptr != new_cp) {
- new_cp->disk_vols = (DiskVol **)ats_malloc(gndisks * sizeof(DiskVol *));
- if (nullptr != new_cp->disk_vols) {
- memset(new_cp->disk_vols, 0, gndisks * sizeof(DiskVol *));
- new_cp->vol_number = config_vol->number;
- new_cp->scheme = config_vol->scheme;
- config_vol->cachep = new_cp;
- fillExclusiveDisks(config_vol->cachep);
- cp_list.enqueue(new_cp);
- } else {
- delete new_cp;
- }
+ forced_volume = true;
+ }
+ }
+
+ if (forced_volume) {
+ CacheVol *new_cp = new CacheVol();
+ if (nullptr != new_cp) {
Review comment:
I've always wondered about this - if that's `nullptr` it would probably be better to just crash on the dereference. OPRS.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] gtenev commented on a change in pull request #7028: Fixed problem with all "forced" volumes cache
Posted by GitBox <gi...@apache.org>.
gtenev commented on a change in pull request #7028:
URL: https://github.com/apache/trafficserver/pull/7028#discussion_r459634557
##########
File path: iocore/cache/Cache.cc
##########
@@ -2596,21 +2596,26 @@ cplist_update()
if (nullptr == config_vol->cachep) {
// Find out if this is a forced volume assigned exclusively to a span which was cleared (hence not referenced in cp_list).
// Note: non-exclusive cleared spans are not handled here, only the "exclusive"
+ bool forced_volume = false;
for (int d_no = 0; d_no < gndisks; d_no++) {
if (gdisks[d_no]->forced_volume_num == config_vol->number) {
- CacheVol *new_cp = new CacheVol();
- if (nullptr != new_cp) {
- new_cp->disk_vols = (DiskVol **)ats_malloc(gndisks * sizeof(DiskVol *));
- if (nullptr != new_cp->disk_vols) {
- memset(new_cp->disk_vols, 0, gndisks * sizeof(DiskVol *));
- new_cp->vol_number = config_vol->number;
- new_cp->scheme = config_vol->scheme;
- config_vol->cachep = new_cp;
- fillExclusiveDisks(config_vol->cachep);
- cp_list.enqueue(new_cp);
- } else {
- delete new_cp;
- }
+ forced_volume = true;
+ }
+ }
+
+ if (forced_volume) {
+ CacheVol *new_cp = new CacheVol();
+ if (nullptr != new_cp) {
Review comment:
ok, removed both `nullptr` checks.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] zwoop commented on pull request #7028: Fixed problem with all "forced" volumes cache
Posted by GitBox <gi...@apache.org>.
zwoop commented on pull request #7028:
URL: https://github.com/apache/trafficserver/pull/7028#issuecomment-662624759
[approve ci freebsd]
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] zwoop removed a comment on pull request #7028: Fixed problem with all "forced" volumes cache
Posted by GitBox <gi...@apache.org>.
zwoop removed a comment on pull request #7028:
URL: https://github.com/apache/trafficserver/pull/7028#issuecomment-662624759
[approve ci freebsd]
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] zwoop commented on pull request #7028: Fixed problem with all "forced" volumes cache
Posted by GitBox <gi...@apache.org>.
zwoop commented on pull request #7028:
URL: https://github.com/apache/trafficserver/pull/7028#issuecomment-663193418
Cherry-picked to v9.0.x branch.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] gtenev commented on a change in pull request #7028: Fixed problem with all "forced" volumes cache
Posted by GitBox <gi...@apache.org>.
gtenev commented on a change in pull request #7028:
URL: https://github.com/apache/trafficserver/pull/7028#discussion_r459109200
##########
File path: iocore/cache/Cache.cc
##########
@@ -2754,14 +2759,36 @@ cplist_reconfigure()
Warning("no volumes created");
return -1;
}
- int64_t space_in_blks = static_cast<int64_t>(((config_vol->percent / percent_remaining)) * tot_space_in_blks);
+
+ // Find if the volume is forced and if it is then calculate the total forced volume size.
+ // Forced volumes take the whole span (disk) also sum all disk space this volume is forced to.
+ bool forcedVolume = false;
Review comment:
OK, fixed. Although I thought it is more human-readable with the `bool` (also it is a stack variable in a non-performance-critical path, possibly also will be optimized out).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org