Skip to content

Fixed problem with all "forced" volumes cache #7028

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
zwoop merged 1 commit into from
Jul 23, 2020
Merged

Fixed problem with all "forced" volumes cache #7028

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 43 additions & 18 deletions iocore/cache/Cache.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Copy Markdown
Member

@SolidWallOfCode SolidWallOfCode Jul 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've always wondered about this - if that's nullptr it would probably be better to just crash on the dereference. OPRS.

Copy link
Copy Markdown
Contributor Author

@gtenev gtenev Jul 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, removed both nullptr checks.

Copy link
Copy Markdown
Member

@SolidWallOfCode SolidWallOfCode Jul 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, not really needed - it was "Outside Pull Request Scope".

new_cp->disk_vols = static_cast<decltype(new_cp->disk_vols)>(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;
}
}
}
Expand Down Expand Up @@ -2638,7 +2643,7 @@ fillExclusiveDisks(CacheVol *cp)

/* 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++) {
for (int j = 0; j < static_cast<int>(gdisks[i]->header->num_volumes); j++) {
if (volume_number != gdisks[i]->disk_vols[j]->vol_number) {
found_nonforced_volumes = true;
break;
Expand All @@ -2654,7 +2659,7 @@ fillExclusiveDisks(CacheVol *cp)
continue;
}

/* Now, volumes have been either deleted or did not existing to begin with so we need to create them. */
/* Now, volumes have been either deleted or did not exist to begin with so we need to create them. */

int64_t size_diff = gdisks[i]->num_usable_blocks;
DiskVolBlock *dpb;
Expand Down Expand Up @@ -2754,14 +2759,34 @@ 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.
int64_t tot_forced_space_in_blks = 0;
for (int i = 0; i < gndisks; i++) {
if (config_vol->number == gdisks[i]->forced_volume_num) {
tot_forced_space_in_blks += (gdisks[i]->num_usable_blocks / blocks_per_vol) * blocks_per_vol;
}
}

int64_t space_in_blks = 0;
if (0 == tot_forced_space_in_blks) {
// Calculate the space as percentage of total space in blocks.
space_in_blks = static_cast<int64_t>(((double)(config_vol->percent / percent_remaining)) * tot_space_in_blks);
} else {
// Forced volumes take all disk space, so no percentage calculations here.
space_in_blks = tot_forced_space_in_blks;
}

space_in_blks = space_in_blks >> (20 - STORE_BLOCK_SHIFT);
/* round down to 128 megabyte multiple */
space_in_blks = (space_in_blks >> 7) << 7;
config_vol->size = space_in_blks;
tot_space_in_blks -= space_in_blks << (20 - STORE_BLOCK_SHIFT);
percent_remaining -= (config_vol->size < 128) ? 0 : config_vol->percent;

if (0 == tot_forced_space_in_blks) {
tot_space_in_blks -= space_in_blks << (20 - STORE_BLOCK_SHIFT);
percent_remaining -= (config_vol->size < 128) ? 0 : config_vol->percent;
}
}
if (config_vol->size < 128) {
Warning("the size of volume %d (%" PRId64 ") is less than the minimum required volume size %d", config_vol->number,
Expand Down