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/10 01:45:52 UTC
[GitHub] [trafficserver] gtenev opened a new pull request #6995: Fix volume/stripe calcs when using forced volumes
gtenev opened a new pull request #6995:
URL: https://github.com/apache/trafficserver/pull/6995
Fixed problems with initialization of cache volumes when at least
one volume is being forced to a specific "exclusive" span.
Problem description
--------------------
Disks are cleared in the following configuration where volume sizes are
specified using percentages and also one of the volumes is forced to a
specific span (disk):
```
storage.config:
/dev/disk1
/dev/disk2 volume=3 # <- exclusive span forced to a specific volume
volume.config:
volume=1 scheme=http size=50%
volume=2 scheme=http size=50%
volume=3 scheme=http size=512 # <- volume forced to an exclusive span
```
During the first start ATS identifies the clears disks and does the following:
(1) creates and spreads new volume 1 and 2 blocks across disk1 and disk2
(2) deletes all volume 1 and 2 blocks from disk2 to make space for volume 3
(3) creates new volume 3 that takes over the whole disk2.
In step (1) volumes are caclulated larger and spread to disk2 only to be
deleted in step (2) to make space for the forced volume 3.
During the initial start the global volume list cp_list would end up
containing "zombie" `CacheVol` instances which correspond to the volume 1
and 2 blocks deleted from disk2 to make space for the volume 3 and the
mapping of domains to volumes (hosting.config) could end up mapping
to any of the deleted volume blocks.
This problem disappears after restart since cp_list will be initialized
from the disks and cp_list will contain only valid CacheVol instances.
The fix
-------
This fix prevents this from happening by making sure all volumes meant
to have "exclusive" disks are created first to make sure span free
spaces are updated correctly and by excluding the size of
the "exclusive" disks from the total cache size used for volume size
calculations when sizes are specified in percentages (volume.config).
----------------------------------------------------------------
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 #6995: Fix volume/stripe calcs when using forced volumes
Posted by GitBox <gi...@apache.org>.
zwoop merged pull request #6995:
URL: https://github.com/apache/trafficserver/pull/6995
----------------------------------------------------------------
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] vmamidi commented on pull request #6995: Fix volume/stripe calcs when using forced volumes
Posted by GitBox <gi...@apache.org>.
vmamidi commented on pull request #6995:
URL: https://github.com/apache/trafficserver/pull/6995#issuecomment-657764741
@scw00 @SolidWallOfCode did you guys get a chance to look at this PR?
----------------------------------------------------------------
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] scw00 commented on pull request #6995: Fix volume/stripe calcs when using forced volumes
Posted by GitBox <gi...@apache.org>.
scw00 commented on pull request #6995:
URL: https://github.com/apache/trafficserver/pull/6995#issuecomment-658062080
Is this a feature that adding exclusively disk without clearing all disk ?
----------------------------------------------------------------
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] scw00 commented on a change in pull request #6995: Fix volume/stripe calcs when using forced volumes
Posted by GitBox <gi...@apache.org>.
scw00 commented on a change in pull request #6995:
URL: https://github.com/apache/trafficserver/pull/6995#discussion_r454208227
##########
File path: iocore/cache/Cache.cc
##########
@@ -2599,20 +2632,32 @@ fillExclusiveDisks(CacheVol *cp)
if (gdisks[i]->forced_volume_num != volume_number) {
continue;
}
- /* The user had created several volumes before - clear the disk
- and create one volume for http */
- for (int j = 0; j < static_cast<int>(gdisks[i]->header->num_volumes); j++) {
+
+ /* OK, this should be an "exclusive" disk (span). */
+ diskCount++;
+
+ /* There should be a single "forced" volume and no other volumes should exist on this "exclusive" disk (span) */
+ bool found_nonforced_volumes = false;
+ for (int j = 0; j < (int)gdisks[i]->header->num_volumes; j++) {
Review comment:
`static_cast` please
----------------------------------------------------------------
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 #6995: Fix volume/stripe calcs when using forced volumes
Posted by GitBox <gi...@apache.org>.
SolidWallOfCode commented on a change in pull request #6995:
URL: https://github.com/apache/trafficserver/pull/6995#discussion_r454467261
##########
File path: iocore/cache/Cache.cc
##########
@@ -2586,6 +2588,37 @@ cplist_update()
cp = cp->link.next;
}
}
+
+ // Look for (exclusive) spans forced to a specific volume but not yet referenced by any volumes in cp_list,
+ // if found then create a new volume. This also makes sure new exclusive disk volumes are created first
+ // before any other new volumes to assure proper span free space calculation and proper volume block distribution.
+ for (config_vol = config_volumes.cp_queue.head; config_vol; config_vol = config_vol->link.next) {
+ 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"
+ 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 *));
Review comment:
No C-style casts. Because it's a `void *`, `static_cast` should work. You could try
```
new_cp->disk_vols = static_cast<decltype(new_cp->disk_vols)>(ats_malloc(...));
```
to be extra fancy :-P
----------------------------------------------------------------
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 #6995: Fix volume/stripe calcs when using forced volumes
Posted by GitBox <gi...@apache.org>.
SolidWallOfCode commented on a change in pull request #6995:
URL: https://github.com/apache/trafficserver/pull/6995#discussion_r454467261
##########
File path: iocore/cache/Cache.cc
##########
@@ -2586,6 +2588,37 @@ cplist_update()
cp = cp->link.next;
}
}
+
+ // Look for (exclusive) spans forced to a specific volume but not yet referenced by any volumes in cp_list,
+ // if found then create a new volume. This also makes sure new exclusive disk volumes are created first
+ // before any other new volumes to assure proper span free space calculation and proper volume block distribution.
+ for (config_vol = config_volumes.cp_queue.head; config_vol; config_vol = config_vol->link.next) {
+ 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"
+ 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 *));
Review comment:
No C-style casts.
----------------------------------------------------------------
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 #6995: Fix volume/stripe calcs when using forced volumes
Posted by GitBox <gi...@apache.org>.
gtenev commented on pull request #6995:
URL: https://github.com/apache/trafficserver/pull/6995#issuecomment-658399523
@scw00 and @SolidWallOfCode thanks for the review!
The C-style casts were remnants of old code, C-style casts are all over the place in Cache.cc. Since this PR got merged before I pushed the fix, in the interest of time I may address your "stylistic request" in a following PR.
----------------------------------------------------------------
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 #6995: Fix volume/stripe calcs when using forced volumes
Posted by GitBox <gi...@apache.org>.
SolidWallOfCode commented on a change in pull request #6995:
URL: https://github.com/apache/trafficserver/pull/6995#discussion_r454469366
##########
File path: iocore/cache/Cache.cc
##########
@@ -2599,20 +2632,32 @@ fillExclusiveDisks(CacheVol *cp)
if (gdisks[i]->forced_volume_num != volume_number) {
continue;
}
- /* The user had created several volumes before - clear the disk
- and create one volume for http */
- for (int j = 0; j < static_cast<int>(gdisks[i]->header->num_volumes); j++) {
+
+ /* OK, this should be an "exclusive" disk (span). */
+ diskCount++;
+
+ /* There should be a single "forced" volume and no other volumes should exist on this "exclusive" disk (span) */
+ bool found_nonforced_volumes = false;
+ for (int j = 0; j < (int)gdisks[i]->header->num_volumes; j++) {
if (volume_number != gdisks[i]->disk_vols[j]->vol_number) {
- Note("Clearing Disk: %s", gdisks[i]->path);
- gdisks[i]->delete_all_volumes();
+ found_nonforced_volumes = true;
break;
}
}
- diskCount++;
+
+ if (found_nonforced_volumes) {
+ /* The user had created several volumes before - clear the disk and create one volume for http */
+ Note("Clearing Disk: %s", gdisks[i]->path);
+ gdisks[i]->delete_all_volumes();
+ } else if (1 == gdisks[i]->header->num_volumes) {
+ /* "Forced" volumes take the whole disk (span) hence nothing more to do for this span. */
+ continue;
+ }
+
+ /* Now, volumes have been either deleted or did not existing to begin with so we need to create them. */
Review comment:
"or did not exist to begin with"
----------------------------------------------------------------
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 #6995: Fix volume/stripe calcs when using forced volumes
Posted by GitBox <gi...@apache.org>.
gtenev commented on pull request #6995:
URL: https://github.com/apache/trafficserver/pull/6995#issuecomment-658416215
@zwoop here is the 8.1.x back port PR #7001
----------------------------------------------------------------
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 #6995: Fix volume/stripe calcs when using forced volumes
Posted by GitBox <gi...@apache.org>.
zwoop commented on pull request #6995:
URL: https://github.com/apache/trafficserver/pull/6995#issuecomment-658391563
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