storage: use segment fallocate size from config #3288

ZeDRoman · 2021-12-16T15:53:08Z

Cover letter

"Today, redpanda fallocates 32M whenever necessary. We currently inject one raft message into every partition when a topic is created. If I create a topic with 1000 partitions, 32G of my disk is eaten.
The fallocate was chosen to be 32M, and large sizes are beneficial, but it may be useful to have this size be a configuration option" (#2876)

Fixes #2876

Release notes

Features

The minimum disk space allocation size is now configurable via the segment_fallocation_step property. The default is unchanged from the previous behavior (32MB). You may wish to decrease this property if creating large numbers of partitions on systems with limited disk space.

dotnwat

This needs some unit tests that run and validate a workload against the segment appender while varying the fallocation step.

emaxerrno · 2021-12-16T23:06:45Z

src/v/storage/segment_appender.h

@@ -49,7 +51,8 @@ class segment_appender {

        ss::io_priority_class priority;
        size_t number_of_chunks;
-        size_t falloc_step{fallocation_step};
+        size_t falloc_step{
+          config::shard_local_cfg().segment_fallocation_step()};


we need to stop using the thread-local for this stuff but pass it as an option on the caller, all the way to main.

testing core-local configs are super hard. generally not a good practice to use it for tests in particular.

dotnwat · 2021-12-21T00:55:47Z

This needs some unit tests that run and validate a workload against the segment appender while varying the fallocation step.

to clarify @ZeDRoman we should run some workload (e.g. appends/reads in the simple case, or generally a simple fuzzer like opfuzz) that checks that the segment appender appears to be working ok.

dotnwat

looks great thanks for adding the tests. let's expand the tests just a little bit.

dotnwat · 2022-01-04T00:02:02Z

src/v/storage/tests/log_segment_appender_test.cc

@@ -31,7 +31,7 @@ SEASTAR_THREAD_TEST_CASE(test_can_append_multiple_flushes) {
                 | ss::open_flags::truncate)
               .get0();
    auto appender = segment_appender(
-      f, segment_appender::options(ss::default_priority_class(), 1));
+      f, segment_appender::options(ss::default_priority_class(), 1, 16_KiB));


this looks good, but can we parameter all the existing tests, not just a new test that is added, because presumably all of the existing functionality should continue to be preserved independent of the allocation size.

I think factoring out the body of the tests into helper functions will make this straight forward. Something like:

static void test_can_append_multiple_flushes(size_t size) { ... } SEASTAR_THREAD_TEST_CASE(test_can_append_multiple_flushes) { run_test_can_append_multiple_flushes(16_KiB); run_test_can_append_multiple_flushes(32_MiB);

dotnwat · 2022-01-04T00:06:01Z

src/v/storage/tests/log_segment_appender_test.cc

@@ -217,3 +217,40 @@ SEASTAR_THREAD_TEST_CASE(test_can_append_little_data) {
    BOOST_REQUIRE_EQUAL(appender.file_byte_offset(), data.size());
    appender.close().get();
 }
+
+SEASTAR_THREAD_TEST_CASE(test_fallocate_size) {
+    for (const size_t fallocate_size : {1ul, 16_KiB, 32_MiB}) {


what validation is applied to this new parameter? can I use 0 here or non-power-of-two values? I think it is a good idea to have validation (if we don't already have that--perhaps I missed it), and make sure the tests here have a few edge cases in the space of allowed values.

We can use any value > 0. But in segment_appender fallocation_offset will be aligned to 4096.
So I think that here will be better to have 4096 divisor in order to have page aligned fallocations
I'll add validator

You may find the existing storage::internal::chunk_cache::validate_chunk_size useful as an example (currently used for the append_chunk_size setting).

It would be good to have some upper and lower bounds on this to prevent craziness if the user types it wrong: minimum should be the append_chunk_size (if we can't get at the live value from the validator then use the default 16kib) and max should be something arbitrary, maybe the default segment size (1gib). Arbitrary limits are kind of annoying, but it's better than enabling the user to instantly fill their disk by typing too many zeros in a config option.

jcsp

This looks good, just suggestions for making the setting adjustable without restart, and validation.

jcsp · 2022-01-13T12:27:03Z

src/v/config/configuration.cc

+      *this,
+      "segment_fallocation_step",
+      "Size for segments fallocation",
+      {.example = "32768", .visibility = visibility::tunable},


This should be relatively easy to make live-adjustable:

Consume it via the .bind() method and pass around a binding rather than an integer (https://vectorizedio.atlassian.net/wiki/spaces/CORE/pages/78676076/Configuration+Properties)

Once that's done, set ".needs_restart = needs_restart::no" here.

jcsp · 2022-01-13T12:30:07Z

src/v/storage/tests/log_segment_appender_test.cc

@@ -217,3 +217,40 @@ SEASTAR_THREAD_TEST_CASE(test_can_append_little_data) {
    BOOST_REQUIRE_EQUAL(appender.file_byte_offset(), data.size());
    appender.close().get();
 }
+
+SEASTAR_THREAD_TEST_CASE(test_fallocate_size) {
+    for (const size_t fallocate_size : {1ul, 16_KiB, 32_MiB}) {


You may find the existing storage::internal::chunk_cache::validate_chunk_size useful as an example (currently used for the append_chunk_size setting).

It would be good to have some upper and lower bounds on this to prevent craziness if the user types it wrong: minimum should be the append_chunk_size (if we can't get at the live value from the validator then use the default 16kib) and max should be something arbitrary, maybe the default segment size (1gib). Arbitrary limits are kind of annoying, but it's better than enabling the user to instantly fill their disk by typing too many zeros in a config option.

Add segment_fallocate size to config properties and set segment_appender fallocate size from config

jcsp · 2022-01-18T13:08:15Z

LGTM, needs Noah's earlier request for changes to be dismissed or replaced with a +1 before github will let us merge.

dotnwat

nice work!

dotnwat · 2022-01-19T19:42:41Z

src/v/storage/segment_appender.h

@@ -77,6 +77,20 @@ class segment_appender {

    void set_callbacks(callbacks* callbacks) { _callbacks = callbacks; }

+    /** Validator for fallocation step configuration setting */
+    static std::optional<ss::sstring>
+    validate_fallocation_step(const size_t& value) {


nit: there is not any benefit to taking a size_t by const&. I wonder if config:: is making this requirement?

github-actions bot added the area/redpanda label Dec 16, 2021

ZeDRoman marked this pull request as ready for review December 16, 2021 19:34

ZeDRoman requested review from dotnwat, ivotron, LenaAn, mmaslankaprv and VadimPlh as code owners December 16, 2021 19:34

dotnwat reviewed Dec 16, 2021

View reviewed changes

emaxerrno reviewed Dec 16, 2021

View reviewed changes

ZeDRoman force-pushed the issues-2876 branch 2 times, most recently from 3b80e85 to bff4782 Compare December 21, 2021 14:31

ZeDRoman requested review from dotnwat and emaxerrno December 22, 2021 09:01

dotnwat requested changes Jan 4, 2022

View reviewed changes

jcsp reviewed Jan 13, 2022

View reviewed changes

ZeDRoman force-pushed the issues-2876 branch from bff4782 to 3c4cbbf Compare January 14, 2022 13:31

ZeDRoman requested review from NyaliaLui, rystsov and ztlpn as code owners January 14, 2022 13:31

ZeDRoman force-pushed the issues-2876 branch from 3c4cbbf to f7ca85c Compare January 14, 2022 14:08

storage: use segment fallocate size from config

c47478e

Add segment_fallocate size to config properties and set segment_appender fallocate size from config

ZeDRoman force-pushed the issues-2876 branch from f7ca85c to c47478e Compare January 14, 2022 14:12

ZeDRoman requested review from jcsp and dotnwat January 14, 2022 14:16

jcsp approved these changes Jan 18, 2022

View reviewed changes

dotnwat approved these changes Jan 19, 2022

View reviewed changes

dotnwat merged commit 4a10444 into redpanda-data:dev Jan 19, 2022

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

storage: use segment fallocate size from config #3288

storage: use segment fallocate size from config #3288

ZeDRoman commented Dec 16, 2021 •

edited by jcsp

Loading

dotnwat left a comment

emaxerrno Dec 16, 2021

dotnwat commented Dec 21, 2021

dotnwat left a comment

dotnwat Jan 4, 2022

dotnwat Jan 4, 2022

ZeDRoman Jan 12, 2022

jcsp Jan 13, 2022

jcsp left a comment

jcsp Jan 13, 2022

jcsp Jan 13, 2022

jcsp commented Jan 18, 2022

dotnwat left a comment

dotnwat Jan 19, 2022

storage: use segment fallocate size from config #3288

storage: use segment fallocate size from config #3288

Conversation

ZeDRoman commented Dec 16, 2021 • edited by jcsp Loading

Cover letter

Release notes

Features

dotnwat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dotnwat commented Dec 21, 2021

dotnwat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcsp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcsp commented Jan 18, 2022

dotnwat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZeDRoman commented Dec 16, 2021 •

edited by jcsp

Loading