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 cab8fb02b4 Add a constructor to `Stripe` to impose invariants early
(#11811)
cab8fb02b4 is described below
commit cab8fb02b46650132b5e54b8ef6a5607800ac216
Author: JosiahWI <[email protected]>
AuthorDate: Tue Oct 15 21:32:57 2024 -0500
Add a constructor to `Stripe` to impose invariants early (#11811)
* Add Stripe::Stripe and use it in unit tests
* Initialize disk related metadata in constructor
* Update cache regression test for new constructor
* Start write head at correct buffer position
* Free all allocated buffers
One of the test cases that didn't previously allocate a Stripe directory
now has to free the one allocated in the constructor. Also, we have to free
the disk path that we now allocate in the constructor.
---
src/iocore/cache/Cache.cc | 8 +-
src/iocore/cache/CacheTest.cc | 16 ++--
src/iocore/cache/Stripe.cc | 140 +++++++++++++++++++----------
src/iocore/cache/Stripe.h | 18 +++-
src/iocore/cache/StripeSM.cc | 36 +++-----
src/iocore/cache/StripeSM.h | 20 +++--
src/iocore/cache/unit_tests/test_Stripe.cc | 52 +++++++----
7 files changed, 181 insertions(+), 109 deletions(-)
diff --git a/src/iocore/cache/Cache.cc b/src/iocore/cache/Cache.cc
index a53afbf849..c9461d5b4f 100644
--- a/src/iocore/cache/Cache.cc
+++ b/src/iocore/cache/Cache.cc
@@ -269,16 +269,14 @@ Cache::open(bool clear, bool /* fix ATS_UNUSED */)
if (cp->disk_stripes[i] && !DISK_BAD(cp->disk_stripes[i]->disk)) {
DiskStripeBlockQueue *q = cp->disk_stripes[i]->dpb_queue.head;
for (; q; q = q->link.next) {
- cp->stripes[vol_no] = new StripeSM();
+ blocks = q->b->len;
CacheDisk *d = cp->disk_stripes[i]->disk;
- cp->stripes[vol_no]->disk = d;
- cp->stripes[vol_no]->fd = d->fd;
+ cp->stripes[vol_no] = new StripeSM(d, blocks,
q->b->offset);
cp->stripes[vol_no]->cache = this;
cp->stripes[vol_no]->cache_vol = cp;
- blocks = q->b->len;
bool vol_clear = clear || d->cleared || q->new_block;
- cp->stripes[vol_no]->init(d->path, blocks, q->b->offset,
vol_clear);
+ cp->stripes[vol_no]->init(vol_clear);
vol_no++;
cache_size += blocks;
}
diff --git a/src/iocore/cache/CacheTest.cc b/src/iocore/cache/CacheTest.cc
index 32d6c44942..3572b2f059 100644
--- a/src/iocore/cache/CacheTest.cc
+++ b/src/iocore/cache/CacheTest.cc
@@ -435,7 +435,7 @@
REGRESSION_TEST(cache_disk_replacement_stability)(RegressionTest *t, int level,
CacheHostRecord hr1, hr2;
StripeSM *sample;
static int const sample_idx = 16;
- StripeSM stripes[MAX_VOLS];
+ StripeSM *stripes[MAX_VOLS];
StripeSM *stripe_ptrs[MAX_VOLS]; // array of pointers.
char buff[2048];
@@ -450,11 +450,10 @@
REGRESSION_TEST(cache_disk_replacement_stability)(RegressionTest *t, int level,
disk.num_errors = 0;
for (int i = 0; i < MAX_VOLS; ++i) {
- stripe_ptrs[i] = stripes + i;
- stripes[i].disk = &disk;
- stripes[i].len = DEFAULT_STRIPE_SIZE;
- snprintf(buff, sizeof(buff), "/dev/sd%c %" PRIu64 ":%" PRIu64, 'a' + i,
DEFAULT_SKIP, stripes[i].len);
- CryptoContext().hash_immediate(stripes[i].hash_id, buff, strlen(buff));
+ stripes[i] = new StripeSM{&disk,
static_cast<off_t>(DEFAULT_STRIPE_SIZE / STORE_BLOCK_SIZE), 0};
+ stripe_ptrs[i] = stripes[i];
+ snprintf(buff, sizeof(buff), "/dev/sd%c %" PRIu64 ":%" PRIu64, 'a' + i,
DEFAULT_SKIP, stripes[i]->len);
+ CryptoContext().hash_immediate(stripes[i]->hash_id, buff, strlen(buff));
}
hr1.vol_hash_table = nullptr;
@@ -466,7 +465,7 @@
REGRESSION_TEST(cache_disk_replacement_stability)(RegressionTest *t, int level,
hr2.stripes = stripe_ptrs;
hr2.num_vols = MAX_VOLS;
- sample = stripes + sample_idx;
+ sample = stripes[sample_idx];
sample->len = 1024ULL * 1024 * 1024 * (1024 + 128); // 1.1 TB
snprintf(buff, sizeof(buff), "/dev/sd%c %" PRIu64 ":%" PRIu64, 'a' +
sample_idx, DEFAULT_SKIP, sample->len);
CryptoContext().hash_immediate(sample->hash_id, buff, strlen(buff));
@@ -498,6 +497,9 @@
REGRESSION_TEST(cache_disk_replacement_stability)(RegressionTest *t, int level,
hr1.stripes = nullptr;
hr2.stripes = nullptr;
+ for (StripeSM *stripe : stripes) {
+ delete stripe;
+ }
}
static double zipf_alpha = 1.2;
diff --git a/src/iocore/cache/Stripe.cc b/src/iocore/cache/Stripe.cc
index 334c28083a..628249643e 100644
--- a/src/iocore/cache/Stripe.cc
+++ b/src/iocore/cache/Stripe.cc
@@ -21,13 +21,17 @@
limitations under the License.
*/
+#include "P_CacheDisk.h"
#include "P_CacheInternal.h"
#include "StripeSM.h"
+#include "tsutil/DbgCtl.h"
+
#include "tscore/hugepages.h"
#include "tscore/ink_assert.h"
#include "tscore/ink_memory.h"
+#include <cstddef>
#include <cstring>
using CacheHTTPInfo = HTTPInfo;
@@ -35,12 +39,23 @@ using CacheHTTPInfo = HTTPInfo;
namespace
{
+DbgCtl dbg_ctl_cache_init{"cache_init"};
+
+constexpr int
DIRECTORY_FOOTER_SIZE{ROUND_TO_STORE_BLOCK(sizeof(StripteHeaderFooter))};
+
int
compare_ushort(void const *a, void const *b)
{
return *static_cast<unsigned short const *>(a) - *static_cast<unsigned short
const *>(b);
}
+template <typename T, typename U>
+constexpr double
+percent(T part, U whole)
+{
+ return static_cast<double>(part) / static_cast<double>(whole) * 100.0;
+}
+
} // namespace
struct StripeInitInfo {
@@ -69,6 +84,84 @@ struct StripeInitInfo {
// Stripe
//
+Stripe::Stripe(CacheDisk *disk, off_t blocks, off_t dir_skip)
+ : path{ats_strdup(disk->path)},
+ fd{disk->fd},
+ skip{ROUND_TO_STORE_BLOCK((dir_skip < START_POS ? START_POS : dir_skip))},
+ start{skip},
+ len{blocks * STORE_BLOCK_SIZE},
+ disk{disk}
+{
+ ink_assert(this->len < MAX_STRIPE_SIZE);
+
+ this->_init_hash_text(disk->path, blocks, dir_skip);
+ this->_init_data(STORE_BLOCK_SIZE);
+ this->_init_directory(this->dirlen(), this->headerlen(),
DIRECTORY_FOOTER_SIZE);
+}
+
+void
+Stripe::_init_hash_text(char const *seed, off_t blocks, off_t dir_skip)
+{
+ char const *seed_str = this->disk->hash_base_string ?
this->disk->hash_base_string : seed;
+ const size_t hash_seed_size = strlen(seed_str);
+ const size_t hash_text_size = hash_seed_size + 32;
+
+ this->hash_text = static_cast<char *>(ats_malloc(hash_text_size));
+ ink_strlcpy(hash_text, seed_str, hash_text_size);
+ snprintf(hash_text + hash_seed_size, (hash_text_size - hash_seed_size), " %"
PRIu64 ":%" PRIu64 "",
+ static_cast<uint64_t>(dir_skip), static_cast<uint64_t>(blocks));
+}
+
+void
+Stripe::_init_data(off_t store_block_size)
+{
+ // iteratively calculate start + buckets; updates this->start
+ this->_init_data_internal();
+ this->_init_data_internal();
+ this->_init_data_internal();
+
+ this->data_blocks = (this->len - (this->start - this->skip)) /
store_block_size;
+}
+
+void
+Stripe::_init_data_internal()
+{
+ // step1: calculate the number of entries.
+ off_t total_entries = (this->len - (this->start - this->skip)) /
cache_config_min_average_object_size;
+ // step2: calculate the number of buckets
+ off_t total_buckets = total_entries / DIR_DEPTH;
+ // step3: calculate the number of segments, no segment has more than 16384
buckets
+ this->segments = (total_buckets + (((1 << 16) - 1) / DIR_DEPTH)) / ((1 <<
16) / DIR_DEPTH);
+ // step4: divide total_buckets into segments on average.
+ this->buckets = (total_buckets + this->segments - 1) / this->segments;
+ // step5: set the start pointer.
+ this->start = this->skip + 2 * this->dirlen();
+}
+
+void
+Stripe::_init_directory(std::size_t directory_size, int header_size, int
footer_size)
+{
+ ink_assert(directory_size <= static_cast<std::size_t>(this->len));
+ // It's probably invalid for the directory to be this small, but at least we
+ // know we will allocate sufficient space for the data we initialize
+ // pointers to, and we can't corrupt our dir pointer by writing to the
+ // footer.
+ ink_release_assert(directory_size >= sizeof(Dir) + header_size +
footer_size);
+
+ Dbg(dbg_ctl_cache_init, "Stripe %s: allocating %zu directory bytes for a
%lld byte volume (%lf%%)", hash_text.get(),
+ directory_size, (long long)this->len, percent(directory_size,
this->len));
+ if (ats_hugepage_enabled()) {
+ this->raw_dir = static_cast<char *>(ats_alloc_hugepage(directory_size));
+ }
+ if (nullptr == this->raw_dir) {
+ this->raw_dir = static_cast<char *>(ats_memalign(ats_pagesize(),
directory_size));
+ }
+ this->dir = reinterpret_cast<Dir *>(this->raw_dir + header_size);
+ this->header = reinterpret_cast<StripteHeaderFooter *>(this->raw_dir);
+ std::size_t const footer_offset{directory_size -
static_cast<std::size_t>(footer_size)};
+ this->footer = reinterpret_cast<StripteHeaderFooter *>(this->raw_dir +
footer_offset);
+}
+
int
Stripe::dir_check()
{
@@ -261,53 +354,6 @@ Stripe::_init_dir()
}
}
-void
-Stripe::_init_data_internal()
-{
- // step1: calculate the number of entries.
- off_t total_entries = (this->len - (this->start - this->skip)) /
cache_config_min_average_object_size;
- // step2: calculate the number of buckets
- off_t total_buckets = total_entries / DIR_DEPTH;
- // step3: calculate the number of segments, no segment has more than 16384
buckets
- this->segments = (total_buckets + (((1 << 16) - 1) / DIR_DEPTH)) / ((1 <<
16) / DIR_DEPTH);
- // step4: divide total_buckets into segments on average.
- this->buckets = (total_buckets + this->segments - 1) / this->segments;
- // step5: set the start pointer.
- this->start = this->skip + 2 * this->dirlen();
-}
-
-void
-Stripe::_init_data(off_t blocks, off_t dir_skip)
-{
- len = blocks * STORE_BLOCK_SIZE;
- ink_assert(len <= MAX_STRIPE_SIZE);
-
- skip = ROUND_TO_STORE_BLOCK((dir_skip < START_POS ? START_POS : dir_skip));
-
- // successive approximation, directory/meta data eats up some storage
- start = skip;
-
- // iteratively calculate start + buckets
- this->_init_data_internal();
- this->_init_data_internal();
- this->_init_data_internal();
-
- data_blocks = (len - (start - skip)) / STORE_BLOCK_SIZE;
-
- // raw_dir
- raw_dir = nullptr;
- if (ats_hugepage_enabled()) {
- raw_dir = static_cast<char *>(ats_alloc_hugepage(this->dirlen()));
- }
- if (raw_dir == nullptr) {
- raw_dir = static_cast<char *>(ats_memalign(ats_pagesize(),
this->dirlen()));
- }
-
- dir = reinterpret_cast<Dir *>(raw_dir + this->headerlen());
- header = reinterpret_cast<StripteHeaderFooter *>(raw_dir);
- footer = reinterpret_cast<StripteHeaderFooter *>(raw_dir + this->dirlen() -
ROUND_TO_STORE_BLOCK(sizeof(StripteHeaderFooter)));
-}
-
bool
Stripe::flush_aggregate_write_buffer()
{
diff --git a/src/iocore/cache/Stripe.h b/src/iocore/cache/Stripe.h
index 6f9f877a58..ebf6e46dba 100644
--- a/src/iocore/cache/Stripe.h
+++ b/src/iocore/cache/Stripe.h
@@ -33,6 +33,7 @@
#include "tscore/ink_align.h"
#include "tscore/ink_memory.h"
+#include <cstddef>
#include <cstdint>
#define CACHE_BLOCK_SHIFT 9
@@ -102,6 +103,19 @@ public:
CacheVol *cache_vol{};
+ /**
+ * Stripe constructor.
+ *
+ * @param disk: The disk object to associate with this stripe.
+ * The disk path must be non-null.
+ * @param blocks: Number of blocks. Must be at least 10.
+ * @param dir_skip: Offset into the disk at which to start the stripe.
+ * If this value is less than START_POS, START_POS will be used instead.
+ *
+ * @see START_POS
+ */
+ Stripe(CacheDisk *disk, off_t blocks, off_t dir_skip);
+
int dir_check();
uint32_t round_to_approx_size(uint32_t l) const;
@@ -158,11 +172,13 @@ protected:
void _clear_init();
void _init_dir();
- void _init_data(off_t blocks, off_t dir_skip);
bool flush_aggregate_write_buffer();
private:
+ void _init_hash_text(char const *seed, off_t blocks, off_t dir_skip);
+ void _init_data(off_t store_block_size);
void _init_data_internal();
+ void _init_directory(std::size_t directory_size, int header_size, int
footer_size);
};
inline uint32_t
diff --git a/src/iocore/cache/StripeSM.cc b/src/iocore/cache/StripeSM.cc
index f5861df320..d3c901ec59 100644
--- a/src/iocore/cache/StripeSM.cc
+++ b/src/iocore/cache/StripeSM.cc
@@ -109,6 +109,17 @@ struct StripeInitInfo {
}
};
+StripeSM::StripeSM(CacheDisk *disk, off_t blocks, off_t dir_skip) :
Continuation(new_ProxyMutex()), Stripe{disk, blocks, dir_skip}
+{
+ this->_preserved_dirs.evacuate_size = static_cast<int>(len /
EVACUATION_BUCKET_SIZE) + 2;
+ int evac_len = this->_preserved_dirs.evacuate_size *
sizeof(DLL<EvacuationBlock>);
+ this->_preserved_dirs.evacuate = static_cast<DLL<EvacuationBlock>
*>(ats_malloc(evac_len));
+ memset(static_cast<void *>(this->_preserved_dirs.evacuate), 0, evac_len);
+
+ open_dir.mutex = this->mutex;
+ SET_HANDLER(&StripeSM::aggWrite);
+}
+
int
StripeSM::begin_read(CacheVC *cont) const
{
@@ -151,36 +162,13 @@ StripeSM::clear_dir()
}
int
-StripeSM::init(char *s, off_t blocks, off_t dir_skip, bool clear)
+StripeSM::init(bool clear)
{
- // Hash
- char *seed_str = disk->hash_base_string ?
disk->hash_base_string : s;
- const size_t hash_seed_size = strlen(seed_str);
- const size_t hash_text_size = hash_seed_size + 32;
-
- hash_text = static_cast<char *>(ats_malloc(hash_text_size));
- ink_strlcpy(hash_text, seed_str, hash_text_size);
- snprintf(hash_text + hash_seed_size, (hash_text_size - hash_seed_size), " %"
PRIu64 ":%" PRIu64 "",
- static_cast<uint64_t>(dir_skip), static_cast<uint64_t>(blocks));
CryptoContext().hash_immediate(hash_id, hash_text, strlen(hash_text));
- path = ats_strdup(s);
-
- // Stripe
- this->_init_data(blocks, dir_skip);
-
// Evacuation
this->hit_evacuate_window = (this->data_blocks *
cache_config_hit_evacuate_percent) / 100;
- // PreservationTable
- this->_preserved_dirs.evacuate_size = static_cast<int>(len /
EVACUATION_BUCKET_SIZE) + 2;
- int evac_len = this->_preserved_dirs.evacuate_size *
sizeof(DLL<EvacuationBlock>);
- this->_preserved_dirs.evacuate = static_cast<DLL<EvacuationBlock>
*>(ats_malloc(evac_len));
- memset(static_cast<void *>(this->_preserved_dirs.evacuate), 0, evac_len);
-
- Dbg(dbg_ctl_cache_init, "Vol %s: allocating %zu directory bytes for a %lld
byte volume (%lf%%)", hash_text.get(), dirlen(),
- static_cast<long long>(this->len), static_cast<double>(dirlen()) /
static_cast<double>(this->len) * 100.0);
-
// AIO
if (clear) {
Note("clearing cache directory '%s'", hash_text.get());
diff --git a/src/iocore/cache/StripeSM.h b/src/iocore/cache/StripeSM.h
index 7ee9515392..685942c49d 100644
--- a/src/iocore/cache/StripeSM.h
+++ b/src/iocore/cache/StripeSM.h
@@ -24,6 +24,7 @@
#pragma once
#include "P_CacheDir.h"
+#include "P_CacheDisk.h"
#include "P_RamCache.h"
#include "AggregateWriteBuffer.h"
#include "PreservationTable.h"
@@ -111,7 +112,7 @@ public:
int clear_dir_aio();
int clear_dir();
- int init(char *s, off_t blocks, off_t dir_skip, bool clear);
+ int init(bool clear);
int handle_dir_clear(int event, void *data);
int handle_dir_read(int event, void *data);
@@ -156,11 +157,18 @@ public:
int within_hit_evacuate_window(Dir const *dir) const;
- StripeSM() : Continuation(new_ProxyMutex())
- {
- open_dir.mutex = mutex;
- SET_HANDLER(&StripeSM::aggWrite);
- }
+ /**
+ * StripeSM constructor.
+ *
+ * @param disk: The disk object to associate with this stripe.
+ * The disk path must be non-null.
+ * @param blocks: Number of blocks. Must be at least 10.
+ * @param dir_skip: Offset into the disk at which to start the stripe.
+ * If this value is less than START_POS, START_POS will be used instead.
+ *
+ * @see START_POS
+ */
+ StripeSM(CacheDisk *disk, off_t blocks, off_t dir_skip);
Queue<CacheVC, Continuation::Link_link> &get_pending_writers();
diff --git a/src/iocore/cache/unit_tests/test_Stripe.cc
b/src/iocore/cache/unit_tests/test_Stripe.cc
index 34baca524e..6c09b1af00 100644
--- a/src/iocore/cache/unit_tests/test_Stripe.cc
+++ b/src/iocore/cache/unit_tests/test_Stripe.cc
@@ -81,6 +81,17 @@ std::array<AddWriterBranchTest, 32>
add_writer_branch_test_cases = {
}
};
+static void
+init_disk(CacheDisk &disk)
+{
+ disk.path = static_cast<char *>(ats_malloc(1));
+ disk.path[0] = '\0';
+ disk.disk_stripes = static_cast<DiskStripe
**>(ats_malloc(sizeof(DiskStripe *)));
+ disk.disk_stripes[0] = nullptr;
+ disk.header = static_cast<DiskHeader
*>(ats_malloc(sizeof(DiskHeader)));
+ disk.header->num_volumes = 0;
+}
+
/* Catch test helper to provide a StripeSM with a valid file descriptor.
*
* The file will be deleted automatically when the application ends normally.
@@ -113,22 +124,13 @@ init_stripe_for_writing(StripeSM &stripe,
StripteHeaderFooter &header, CacheVol
cache_rsb.gc_frags_evacuated =
Metrics::Counter::createPtr("unit_test.gc.frags.evacuated");
stripe.cache_vol->vol_rsb.gc_frags_evacuated =
Metrics::Counter::createPtr("unit_test.gc.frags.evacuated");
- // A number of things must be initialized in a certain way for Stripe
- // not to segfault, hit an assertion, or exhibit zero-division.
- // I just picked values that happen to work.
stripe.sector_size = 256;
- stripe.skip = 0;
- stripe.len = 600000000000000;
- stripe.segments = 1;
- stripe.buckets = 4;
- stripe.start = stripe.skip + 2 * stripe.dirlen();
- stripe.raw_dir = static_cast<char *>(ats_memalign(ats_pagesize(),
stripe.dirlen()));
- stripe.dir = reinterpret_cast<Dir *>(stripe.raw_dir +
stripe.headerlen());
-
- stripe.get_preserved_dirs().evacuate = static_cast<DLL<EvacuationBlock>
*>(ats_malloc(2024));
- memset(static_cast<void *>(stripe.get_preserved_dirs().evacuate), 0, 2024);
-
- header.write_pos = 50000;
+
+ // This is the minimum value for header.write_pos. Offsets are calculated
+ // based on the distance of the write_pos from this point. If we ever move
+ // the write head before the start of the stripe data section, we will
+ // underflow offset calculations and end up in big trouble.
+ header.write_pos = stripe.start;
header.agg_pos = 1;
header.phase = 0;
stripe.header = &header;
@@ -137,8 +139,10 @@ init_stripe_for_writing(StripeSM &stripe,
StripteHeaderFooter &header, CacheVol
TEST_CASE("The behavior of StripeSM::add_writer.")
{
- FakeVC vc;
- StripeSM stripe;
+ FakeVC vc;
+ CacheDisk disk;
+ init_disk(disk);
+ StripeSM stripe{&disk, 10, 0};
SECTION("Branch tests.")
{
@@ -187,6 +191,10 @@ TEST_CASE("The behavior of StripeSM::add_writer.")
CHECK(true == result);
}
}
+
+ ats_free(stripe.raw_dir);
+ ats_free(stripe.get_preserved_dirs().evacuate);
+ ats_free(stripe.path);
}
// This test case demonstrates how to set up a StripeSM and make
@@ -194,7 +202,9 @@ TEST_CASE("The behavior of StripeSM::add_writer.")
// tmpfile for the StripeSM to write to.
TEST_CASE("aggWrite behavior with f.evacuator unset")
{
- StripeSM stripe;
+ CacheDisk disk;
+ init_disk(disk);
+ StripeSM stripe{&disk, 10, 0};
StripteHeaderFooter header;
CacheVol cache_vol;
auto *file{init_stripe_for_writing(stripe, header, cache_vol)};
@@ -296,6 +306,7 @@ TEST_CASE("aggWrite behavior with f.evacuator unset")
ats_free(stripe.raw_dir);
ats_free(stripe.get_preserved_dirs().evacuate);
+ ats_free(stripe.path);
}
// When f.evacuator is set, vc.buf must contain a Doc object including headers
@@ -304,7 +315,9 @@ TEST_CASE("aggWrite behavior with f.evacuator unset")
// only on the presence of the f.evacuator flag.
TEST_CASE("aggWrite behavior with f.evacuator set")
{
- StripeSM stripe;
+ CacheDisk disk;
+ init_disk(disk);
+ StripeSM stripe{&disk, 10, 0};
StripteHeaderFooter header;
CacheVol cache_vol;
auto *file{init_stripe_for_writing(stripe, header, cache_vol)};
@@ -393,4 +406,5 @@ TEST_CASE("aggWrite behavior with f.evacuator set")
delete[] source;
ats_free(stripe.raw_dir);
ats_free(stripe.get_preserved_dirs().evacuate);
+ ats_free(stripe.path);
}
