Skip to content

Commit

Permalink
Harmonize MaxRequestSize with minimum value (#2082) (#2104)
Browse files Browse the repository at this point in the history 
* Limit request size with 4_MB

* Harmonize maxRequestSize on disk-agent side

* Change one more const
  • Loading branch information
@drbasic
drbasic authored Sep 23, 2024
1 parent 0fada1c commit a423d6b
Show file tree
Hide file tree
Showing 21 changed files with 100 additions and 71 deletions.
13 changes: 8 additions & 5 deletions cloud/blockstore/libs/endpoints_grpc/socket_endpoint_listener.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -73,13 +73,16 @@ class TEndpointService final
const std::shared_ptr<NProto::TMountVolumeResponse> LastMountResponsePtr;

public:
TEndpointService(
TString diskId,
ui32 blockSize,
ISessionPtr session)
TEndpointService(TString diskId, ui32 blockSize, ISessionPtr session)
: DiskId(std::move(diskId))
, BlockSize(blockSize)
, StorageAdapter(session, blockSize, true)
, StorageAdapter(
session,
blockSize,
true, // normalize,
TDuration::Zero(), // maxRequestDuration
TDuration::Zero() // shutdownTimeout
)
, Session(std::move(session))
, LastMountResponsePtr(std::make_shared<NProto::TMountVolumeResponse>())
{
Expand Down
1 change: 0 additions & 1 deletion cloud/blockstore/libs/nbd/server_handler.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -939,7 +939,6 @@ IServerHandlerFactoryPtr CreateServerHandlerFactory(
std::move(storage),
options.ClientId,
options.BlockSize,
NBD_MAX_BUFFER_SIZE / options.BlockSize,
options.UnalignedRequestsDisabled);

return std::make_shared<TServerHandlerFactory>(
Expand Down
8 changes: 7 additions & 1 deletion cloud/blockstore/libs/rdma/iface/client.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,13 @@ namespace NCloud::NBlockStore::NRdma {
struct TClientConfig
{
ui32 QueueSize = 10;
ui32 MaxBufferSize = 1024*1024;
// Keep the value greater then MaxSubRequestSize, ProcessingRangeSize,
// ResyncRangeSize in cloud/blockstore/libs/service/device_handler.cpp
// cloud/blockstore/libs/storage/partition_nonrepl/model/processing_blocks.h
// cloud/blockstore/libs/storage/partition_nonrepl/part_mirror_resync_util.h
// Keep sync with MaxBufferSize in cloud/blockstore/vhost-server/options.h
// and cloud/blockstore/libs/rdma/iface/server.h
ui32 MaxBufferSize = 4_MB + 4_KB;
EWaitMode WaitMode = EWaitMode::Poll;
ui32 PollerThreads = 1;
TDuration MaxReconnectDelay = TDuration::Seconds(60);
Expand Down
3 changes: 2 additions & 1 deletion cloud/blockstore/libs/rdma/iface/server.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,8 @@ struct TServerConfig
{
ui32 Backlog = 10;
ui32 QueueSize = 10;
ui32 MaxBufferSize = 1024*1024;
// Keep sync with MaxBufferSize in cloud/blockstore/libs/rdma/iface/client.h
ui32 MaxBufferSize = 4_MB + 4_KB;
TDuration KeepAliveTimeout = TDuration::Seconds(10);
EWaitMode WaitMode = EWaitMode::Poll;
ui32 PollerThreads = 1;
Expand Down
9 changes: 7 additions & 2 deletions cloud/blockstore/libs/rdma_test/rdma_test_environment.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,13 @@ TRdmaTestEnvironment::TRdmaTestEnvironment(size_t deviceSize, ui32 poolSize)
: Storage(std::make_shared<TMemoryTestStorage>(deviceSize))
{
THashMap<TString, TStorageAdapterPtr> devices;
devices[Device_1] =
std::make_shared<TStorageAdapter>(Storage, 4_KB, true, 0);
devices[Device_1] = std::make_shared<TStorageAdapter>(
Storage,
4_KB, // storageBlockSize
true, // normalize,
TDuration::Zero(), // maxRequestDuration
TDuration::Zero() // shutdownTimeout
);
TVector<TString> uuids;
for (const auto& [key, value]: devices) {
uuids.push_back(key);
Expand Down
4 changes: 2 additions & 2 deletions cloud/blockstore/libs/service/aligned_device_handler.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -132,11 +132,11 @@ TAlignedDeviceHandler::TAlignedDeviceHandler(
IStoragePtr storage,
TString clientId,
ui32 blockSize,
ui32 maxBlockCount)
ui32 maxSubRequestSize)
: Storage(std::move(storage))
, ClientId(std::move(clientId))
, BlockSize(blockSize)
, MaxBlockCount(maxBlockCount)
, MaxBlockCount(maxSubRequestSize / BlockSize)
{
Y_ABORT_UNLESS(MaxBlockCount > 0);
}
Expand Down
2 changes: 1 addition & 1 deletion cloud/blockstore/libs/service/aligned_device_handler.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ class TAlignedDeviceHandler final
IStoragePtr storage,
TString clientId,
ui32 blockSize,
ui32 maxBlockCount);
ui32 maxSubRequestSize);

// implements IDeviceHandler
NThreading::TFuture<NProto::TReadBlocksLocalResponse> Read(
Expand Down
22 changes: 18 additions & 4 deletions cloud/blockstore/libs/service/device_handler.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,31 +13,40 @@ namespace {

constexpr ui32 MaxUnalignedRequestSize = 32_MB;

// Keep the value less than MaxBufferSize in
// cloud/blockstore/libs/rdma/iface/client.h
constexpr ui32 MaxSubRequestSize = 4_MB;

////////////////////////////////////////////////////////////////////////////////

struct TDefaultDeviceHandlerFactory final
: public IDeviceHandlerFactory
{
const ui32 MaxSubRequestSize;

explicit TDefaultDeviceHandlerFactory(ui32 maxSubRequestSize)
: MaxSubRequestSize(maxSubRequestSize)
{}

IDeviceHandlerPtr CreateDeviceHandler(
IStoragePtr storage,
TString clientId,
ui32 blockSize,
ui32 maxBlockCount,
bool unalignedRequestsDisabled) override
{
if (unalignedRequestsDisabled) {
return std::make_shared<TAlignedDeviceHandler>(
std::move(storage),
std::move(clientId),
blockSize,
maxBlockCount);
MaxSubRequestSize);
}

return std::make_shared<TUnalignedDeviceHandler>(
std::move(storage),
std::move(clientId),
blockSize,
maxBlockCount,
MaxSubRequestSize,
MaxUnalignedRequestSize);
}
};
Expand All @@ -48,7 +57,12 @@ struct TDefaultDeviceHandlerFactory final

IDeviceHandlerFactoryPtr CreateDefaultDeviceHandlerFactory()
{
return std::make_shared<TDefaultDeviceHandlerFactory>();
return std::make_shared<TDefaultDeviceHandlerFactory>(MaxSubRequestSize);
}

IDeviceHandlerFactoryPtr CreateDeviceHandlerFactory(ui32 maxSubRequestSize)
{
return std::make_shared<TDefaultDeviceHandlerFactory>(maxSubRequestSize);
}

} // namespace NCloud::NBlockStore
2 changes: 1 addition & 1 deletion cloud/blockstore/libs/service/device_handler.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,12 +45,12 @@ struct IDeviceHandlerFactory
IStoragePtr storage,
TString clientId,
ui32 blockSize,
ui32 maxBlockCount,
bool unalignedRequestsDisabled) = 0;
};

////////////////////////////////////////////////////////////////////////////////

IDeviceHandlerFactoryPtr CreateDefaultDeviceHandlerFactory();
IDeviceHandlerFactoryPtr CreateDeviceHandlerFactory(ui32 maxSubRequestSize);

} // namespace NCloud::NBlockStore
16 changes: 6 additions & 10 deletions cloud/blockstore/libs/service/device_handler_ut.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -123,11 +123,11 @@ class TTestEnvironment
});
};

DeviceHandler = CreateDefaultDeviceHandlerFactory()->CreateDeviceHandler(
auto factory = CreateDeviceHandlerFactory(maxBlockCount * BlockSize);
DeviceHandler = factory->CreateDeviceHandler(
std::move(testStorage),
"testClientId",
BlockSize,
maxBlockCount,
unalignedRequestsDisabled);
}

Expand Down Expand Up @@ -335,11 +335,11 @@ Y_UNIT_TEST_SUITE(TDeviceHandlerTest)

auto storage = std::make_shared<TTestStorage>();

auto deviceHandler = CreateDefaultDeviceHandlerFactory()->CreateDeviceHandler(
auto factory = CreateDeviceHandlerFactory(blocksCountLimit * blockSize);
auto deviceHandler = factory->CreateDeviceHandler(
storage,
clientId,
blockSize,
blocksCountLimit,
false);

std::array<bool, deviceBlocksCount> zeroBlocks;
Expand Down Expand Up @@ -402,7 +402,6 @@ Y_UNIT_TEST_SUITE(TDeviceHandlerTest)
storage,
clientId,
blockSize,
1024,
true);

ui32 startIndex = 42;
Expand Down Expand Up @@ -489,7 +488,6 @@ Y_UNIT_TEST_SUITE(TDeviceHandlerTest)
storage,
clientId,
blockSize,
1024,
true);

{
Expand Down Expand Up @@ -598,11 +596,11 @@ Y_UNIT_TEST_SUITE(TDeviceHandlerTest)

auto storage = std::make_shared<TTestStorage>();

auto deviceHandler = CreateDefaultDeviceHandlerFactory()->CreateDeviceHandler(
auto factory = CreateDeviceHandlerFactory(blocksCountLimit * blockSize);
auto deviceHandler = factory->CreateDeviceHandler(
storage,
clientId,
blockSize,
blocksCountLimit,
unalignedRequestDisabled);

storage->ZeroBlocksHandler = [&] (
Expand Down Expand Up @@ -717,7 +715,6 @@ Y_UNIT_TEST_SUITE(TDeviceHandlerTest)
storage,
clientId,
blockSize,
1024,
false);

storage->WriteBlocksLocalHandler = [&] (
Expand Down Expand Up @@ -781,7 +778,6 @@ Y_UNIT_TEST_SUITE(TDeviceHandlerTest)
storage,
clientId,
blockSize,
1024,
false);

storage->WriteBlocksLocalHandler = [&] (
Expand Down
10 changes: 4 additions & 6 deletions cloud/blockstore/libs/service/storage.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,10 @@ namespace {

////////////////////////////////////////////////////////////////////////////////

constexpr ui32 MaxRequestSize = 32_MB;

////////////////////////////////////////////////////////////////////////////////

class TStorageStub final
: public IStorage
{
Expand Down Expand Up @@ -173,7 +177,6 @@ class TStorageAdapter::TImpl
const IStoragePtr Storage;
const ui32 StorageBlockSize;
const bool Normalize;
const ui32 MaxRequestSize;
const TDuration MaxRequestDuration;

std::shared_ptr<TInflightReads> InflightReads{
Expand All @@ -190,7 +193,6 @@ class TStorageAdapter::TImpl
IStoragePtr storage,
ui32 storageBlockSize,
bool normalize,
ui32 maxRequestSize,
TDuration maxRequestDuration);

TFuture<NProto::TReadBlocksResponse> ReadBlocks(
Expand Down Expand Up @@ -240,12 +242,10 @@ TStorageAdapter::TImpl::TImpl(
IStoragePtr storage,
ui32 storageBlockSize,
bool normalize,
ui32 maxRequestSize,
TDuration maxRequestDuration)
: Storage(std::move(storage))
, StorageBlockSize(storageBlockSize)
, Normalize(normalize)
, MaxRequestSize(maxRequestSize)
, MaxRequestDuration(maxRequestDuration)
{}

Expand Down Expand Up @@ -666,14 +666,12 @@ TStorageAdapter::TStorageAdapter(
IStoragePtr storage,
ui32 storageBlockSize,
bool normalize,
ui32 maxRequestSize,
TDuration maxRequestDuration,
TDuration shutdownTimeout)
: Impl(std::make_unique<TImpl>(
std::move(storage),
storageBlockSize,
normalize,
maxRequestSize,
maxRequestDuration))
, ShutdownTimeout(shutdownTimeout)
{}
Expand Down
5 changes: 2 additions & 3 deletions cloud/blockstore/libs/service/storage.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,9 +57,8 @@ class TStorageAdapter
ui32 storageBlockSize,
bool normalize, // When true, we use StorageBlockSize when making
// requests to the underlying storage
ui32 maxRequestSize = 0,
TDuration maxRequestDuration = TDuration::Zero(),
TDuration shutdownTimeout = TDuration::Zero());
TDuration maxRequestDuration,
TDuration shutdownTimeout);

~TStorageAdapter();

Expand Down
37 changes: 22 additions & 15 deletions cloud/blockstore/libs/service/storage_ut.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -74,9 +74,11 @@ Y_UNIT_TEST_SUITE(TStorageTest)

auto adapter = std::make_shared<TStorageAdapter>(
storage,
4_KB, // storageBlockSize
false, // normalize
32_MB); // maxRequestSize
4_KB, // storageBlockSize
false, // normalize,
TDuration::Zero(), // maxRequestDuration
TDuration::Zero() // shutdownTimeout
);

auto request = std::make_shared<NProto::TWriteBlocksRequest>();
request->SetStartIndex(1000);
Expand Down Expand Up @@ -143,9 +145,11 @@ Y_UNIT_TEST_SUITE(TStorageTest)

auto adapter = std::make_shared<TStorageAdapter>(
storage,
4_KB, // storageBlockSize
true, // normalize
32_MB); // maxRequestSize
4_KB, // storageBlockSize
true, // normalize,
TDuration::Zero(), // maxRequestDuration
TDuration::Zero() // shutdownTimeout
);

auto request = std::make_shared<NProto::TWriteBlocksRequest>();
request->SetStartIndex(1000);
Expand Down Expand Up @@ -215,10 +219,10 @@ Y_UNIT_TEST_SUITE(TStorageTest)

auto adapter = std::make_shared<TStorageAdapter>(
storage,
4_KB, // storageBlockSize
false, // normalize
32_MB, // maxRequestSize
TDuration::Seconds(1) // timeout
4_KB, // storageBlockSize
false, // normalize
TDuration::Seconds(1), // maxRequestDuration
TDuration::Zero() // shutdownTimeout
);

auto now = TInstant::Seconds(1);
Expand Down Expand Up @@ -337,7 +341,6 @@ Y_UNIT_TEST_SUITE(TStorageTest)
storage,
4_KB, // storageBlockSize
false, // normalize
32_MB, // maxRequestSize
TDuration::Seconds(1), // request timeout
shutdownTimeout // shutdown timeout
);
Expand Down Expand Up @@ -463,9 +466,11 @@ Y_UNIT_TEST_SUITE(TStorageTest)

auto adapter = std::make_shared<TStorageAdapter>(
storage,
4096, // storageBlockSize
4_KB, // storageBlockSize
normalize,
32_MB); // maxRequestSize
TDuration::Zero(), // request timeout
TDuration::Zero() // shutdown timeout
);

{ // Requesting the reading of two blocks. Since the first block will
// be filled with zeros, it should be optimized and will return an
Expand Down Expand Up @@ -542,9 +547,11 @@ Y_UNIT_TEST_SUITE(TStorageTest)

auto adapter = std::make_shared<TStorageAdapter>(
storage,
4096, // storageBlockSize
4_KB, // storageBlockSize
normalize,
32_MB); // maxRequestSize
TDuration::Zero(), // request timeout
TDuration::Zero() // shutdown timeout
);

{
auto request = std::make_shared<NProto::TReadBlocksRequest>();
Expand Down
Loading

0 comments on commit a423d6b

Please sign in to comment.