From 600e88758d5dbc6b8e101ed29ff73277e1bf81fb Mon Sep 17 00:00:00 2001 From: Maxim Deb Natkh Date: Fri, 12 Apr 2024 15:45:32 +0000 Subject: [PATCH 1/3] issue-539: add stats for BS requests for clients --- cloud/filestore/libs/service/auth_scheme.cpp | 2 + cloud/filestore/libs/service/request.cpp | 2 + cloud/filestore/libs/service/request.h | 2 + .../service/service_actor_readdata.cpp | 39 +++++++++++++++---- .../service/service_actor_writedata.cpp | 28 +++++++++++++ .../libs/storage/service/service_ut.cpp | 19 ++++++++- .../libs/storage/tablet/tablet_ut_data.cpp | 1 + .../analytics/libs/event-log/dump_ut.cpp | 2 + 8 files changed, 85 insertions(+), 10 deletions(-) diff --git a/cloud/filestore/libs/service/auth_scheme.cpp b/cloud/filestore/libs/service/auth_scheme.cpp index 9bd80fe077..140f220c6d 100644 --- a/cloud/filestore/libs/service/auth_scheme.cpp +++ b/cloud/filestore/libs/service/auth_scheme.cpp @@ -42,6 +42,8 @@ TPermissionList GetRequestPermissions(EFileStoreRequest requestType) case EFileStoreRequest::TestLock: case EFileStoreRequest::DescribeData: case EFileStoreRequest::GenerateBlobIds: + case EFileStoreRequest::ReadBlob: + case EFileStoreRequest::WriteBlob: case EFileStoreRequest::AddData: return CreatePermissionList({}); diff --git a/cloud/filestore/libs/service/request.cpp b/cloud/filestore/libs/service/request.cpp index ea2ca4a074..6bf427b5f6 100644 --- a/cloud/filestore/libs/service/request.cpp +++ b/cloud/filestore/libs/service/request.cpp @@ -118,6 +118,8 @@ static const TString RequestNames[] = { "DescribeData", "GenerateBlobIds", "AddData", + "ReadBlob", + "WriteBlob", }; static_assert( diff --git a/cloud/filestore/libs/service/request.h b/cloud/filestore/libs/service/request.h index 3b793d75dc..e248603d1b 100644 --- a/cloud/filestore/libs/service/request.h +++ b/cloud/filestore/libs/service/request.h @@ -130,6 +130,8 @@ enum class EFileStoreRequest DescribeData, GenerateBlobIds, AddData, + ReadBlob, + WriteBlob, MAX }; diff --git a/cloud/filestore/libs/storage/service/service_actor_readdata.cpp b/cloud/filestore/libs/storage/service/service_actor_readdata.cpp index 852ce1e104..68c2cbb797 100644 --- a/cloud/filestore/libs/storage/service/service_actor_readdata.cpp +++ b/cloud/filestore/libs/storage/service/service_actor_readdata.cpp @@ -5,6 +5,7 @@ #include #include #include +#include #include #include @@ -44,6 +45,7 @@ class TReadDataActor final: public TActorBootstrapped IRequestStatsPtr RequestStats; IProfileLogPtr ProfileLog; TMaybe InFlightRequest; + TVector> InFlightBSRequests; const NCloud::NProto::EStorageMediaKind MediaKind; public: @@ -254,7 +256,7 @@ void TReadDataActor::HandleDescribeDataResponse( { const auto* msg = ev->Get(); - Y_ABORT_UNLESS(InFlightRequest); + TABLET_VERIFY(InFlightRequest); InFlightRequest->Complete(ctx.Now(), msg->GetError()); FinalizeProfileLogRequestInfo( @@ -285,8 +287,24 @@ void TReadDataActor::HandleDescribeDataResponse( void TReadDataActor::ReadBlobIfNeeded(const TActorContext& ctx) { RemainingBlobsToRead = DescribeResponse.GetBlobPieces().size(); + if (RemainingBlobsToRead == 0) { + ReplyAndDie(ctx); + return; + } + + RequestInfo->CallContext->RequestType = EFileStoreRequest::ReadBlob; ui32 blobPieceId = 0; + InFlightBSRequests.reserve(RemainingBlobsToRead); for (const auto& blobPiece: DescribeResponse.GetBlobPieces()) { + InFlightBSRequests.emplace_back(std::make_unique( + TRequestInfo( + RequestInfo->Sender, + RequestInfo->Cookie, + RequestInfo->CallContext), + ProfileLog, + MediaKind, + RequestStats)); + InFlightBSRequests.back()->Start(ctx.Now()); NKikimr::TLogoBlobID blobId = LogoBlobIDFromLogoBlobID(blobPiece.GetBlobId()); LOG_DEBUG( @@ -330,10 +348,6 @@ void TReadDataActor::ReadBlobIfNeeded(const TActorContext& ctx) SendToBSProxy(ctx, proxy, request.release(), blobPieceId++); } - - if (RemainingBlobsToRead == 0) { - ReplyAndDie(ctx); - } } void TReadDataActor::HandleReadBlobResponse( @@ -369,11 +383,18 @@ void TReadDataActor::HandleReadBlobResponse( (ui64)(msg->Status), ev->Cookie); - Y_ABORT_UNLESS(ev->Cookie < DescribeResponse.BlobPiecesSize()); + TABLET_VERIFY(ev->Cookie < DescribeResponse.BlobPiecesSize()); const auto& blobPiece = DescribeResponse.GetBlobPieces(ev->Cookie); + uint64_t blobIdx = ev->Cookie; + TABLET_VERIFY( + 0 <= blobIdx && blobIdx < InFlightBSRequests.size() && + InFlightBSRequests[blobIdx] && + !InFlightBSRequests[blobIdx]->IsCompleted()); + InFlightBSRequests[blobIdx]->Complete(ctx.Now(), {}); + for (size_t i = 0; i < msg->ResponseSz; ++i) { - Y_ABORT_UNLESS(i < blobPiece.RangesSize()); + TABLET_VERIFY(i < blobPiece.RangesSize()); const auto& blobPiece = DescribeResponse.GetBlobPieces(ev->Cookie); const auto& blobRange = blobPiece.GetRanges(i); @@ -427,7 +448,7 @@ void TReadDataActor::HandleReadBlobResponse( DescribeResponse.DebugString().c_str(), i, response.Buffer.size()); - Y_ABORT_UNLESS(blobRange.GetOffset() >= AlignedByteRange.Offset); + TABLET_VERIFY(blobRange.GetOffset() >= AlignedByteRange.Offset); char* targetData = GetDataPtr( blobRange.GetOffset(), @@ -440,6 +461,7 @@ void TReadDataActor::HandleReadBlobResponse( --RemainingBlobsToRead; if (RemainingBlobsToRead == 0) { + RequestInfo->CallContext->RequestType = EFileStoreRequest::ReadData; ReplyAndDie(ctx); } } @@ -461,6 +483,7 @@ void TReadDataActor::ReadData( const TString& fallbackReason) { ReadDataFallbackEnabled = true; + RequestInfo->CallContext->RequestType = EFileStoreRequest::ReadData; LOG_WARN( ctx, diff --git a/cloud/filestore/libs/storage/service/service_actor_writedata.cpp b/cloud/filestore/libs/storage/service/service_actor_writedata.cpp index d31d977f76..e6197de031 100644 --- a/cloud/filestore/libs/storage/service/service_actor_writedata.cpp +++ b/cloud/filestore/libs/storage/service/service_actor_writedata.cpp @@ -43,7 +43,10 @@ class TWriteDataActor final: public TActorBootstrapped // Stats for reporting IRequestStatsPtr RequestStats; IProfileLogPtr ProfileLog; + // Refers to GenerateBlobIds or AddData request, depending on which one is + // in flight TMaybe InFlightRequest; + TVector> InFlightBSRequests; const NCloud::NProto::EStorageMediaKind MediaKind; @@ -160,9 +163,21 @@ class TWriteDataActor final: public TActorBootstrapped RemainingBlobsToWrite = GenerateBlobIdsResponse.BlobsSize(); ui64 offset = 0; + RequestInfo->CallContext->RequestType = EFileStoreRequest::WriteBlob; + InFlightBSRequests.reserve(RemainingBlobsToWrite); for (const auto& blob: GenerateBlobIdsResponse.GetBlobs()) { NKikimr::TLogoBlobID blobId = LogoBlobIDFromLogoBlobID(blob.GetBlobId()); + InFlightBSRequests.emplace_back(std::make_unique( + TRequestInfo( + RequestInfo->Sender, + RequestInfo->Cookie, + RequestInfo->CallContext), + ProfileLog, + MediaKind, + RequestStats)); + InFlightBSRequests.back()->Start(ctx.Now()); + std::unique_ptr request; if (GenerateBlobIdsResponse.BlobsSize() == 1) { // do not copy the buffer if there is only one blob @@ -219,8 +234,19 @@ class TWriteDataActor final: public TActorBootstrapped "TEvPutResult response received: %s", msg->ToString().c_str()); + auto blobIdx = msg->Id.Cookie(); + // It is implicitly expected that cookies are generated in increasing + // order starting from 0. + TABLET_VERIFY( + 0 <= blobIdx && blobIdx < InFlightBSRequests.size() && + InFlightBSRequests[blobIdx] && + !InFlightBSRequests[blobIdx]->IsCompleted()); + InFlightBSRequests[blobIdx]->Complete(ctx.Now(), {}); + --RemainingBlobsToWrite; if (RemainingBlobsToWrite == 0) { + RequestInfo->CallContext->RequestType = + EFileStoreRequest::WriteData; AddData(ctx); } } @@ -292,6 +318,8 @@ class TWriteDataActor final: public TActorBootstrapped void WriteData(const TActorContext& ctx, const NProto::TError& error) { WriteDataFallbackEnabled = true; + RequestInfo->CallContext->RequestType = EFileStoreRequest::WriteData; + LOG_WARN( ctx, TFileStoreComponents::SERVICE, diff --git a/cloud/filestore/libs/storage/service/service_ut.cpp b/cloud/filestore/libs/storage/service/service_ut.cpp index 6c58174fb2..06687e9a5a 100644 --- a/cloud/filestore/libs/storage/service/service_ut.cpp +++ b/cloud/filestore/libs/storage/service/service_ut.cpp @@ -1815,6 +1815,14 @@ Y_UNIT_TEST_SUITE(TStorageServiceTest) 4, subgroup->GetCounter("Count")->GetAtomic()); } + { + auto subgroup = counters->FindSubgroup("request", "ReadBlob"); + UNIT_ASSERT(subgroup); + // 1MB = 4 blobs of 256KB. Read is performed twice. + UNIT_ASSERT_VALUES_EQUAL( + 8, + subgroup->GetCounter("Count")->GetAtomic()); + } } Y_UNIT_TEST(ShouldFallbackToReadDataIfDescribeDataFails) @@ -2176,7 +2184,6 @@ Y_UNIT_TEST_SUITE(TStorageServiceTest) UNIT_ASSERT_VALUES_EQUAL( 1, subgroup->GetCounter("Errors")->GetAtomic()); - } { auto subgroup = counters->FindSubgroup("request", "WriteData"); @@ -2185,7 +2192,15 @@ Y_UNIT_TEST_SUITE(TStorageServiceTest) 7, subgroup->GetCounter("Count")->GetAtomic()); } - + { + auto subgroup = counters->FindSubgroup("request", "WriteBlob"); + UNIT_ASSERT(subgroup); + // Total number of put requests should have been 1 + 1 + 1 + 2 + 11 + // + 3 + ceil(360 / 64) = 25 + UNIT_ASSERT_VALUES_EQUAL( + 25, + subgroup->GetCounter("Count")->GetAtomic()); + } } Y_UNIT_TEST(ShouldNotUseThreeStageWriteForSmallOrUnalignedRequests) diff --git a/cloud/filestore/libs/storage/tablet/tablet_ut_data.cpp b/cloud/filestore/libs/storage/tablet/tablet_ut_data.cpp index 0d3e79f56e..5ca614a294 100644 --- a/cloud/filestore/libs/storage/tablet/tablet_ut_data.cpp +++ b/cloud/filestore/libs/storage/tablet/tablet_ut_data.cpp @@ -4339,6 +4339,7 @@ Y_UNIT_TEST_SUITE(TIndexTabletTest_Data) UNIT_ASSERT_VALUES_EQUAL(blob.BlobSize(), expectedSizes[i]); \ UNIT_ASSERT_VALUES_EQUAL(blob.Generation(), generation); \ UNIT_ASSERT_VALUES_EQUAL(blob.Step(), step); \ + UNIT_ASSERT_VALUES_EQUAL(blob.Cookie(), i); \ UNIT_ASSERT_VALUES_EQUAL( \ generatedBlob.GetOffset(), \ currentOffset); \ diff --git a/cloud/filestore/tools/analytics/libs/event-log/dump_ut.cpp b/cloud/filestore/tools/analytics/libs/event-log/dump_ut.cpp index 43c3db25ee..418747547c 100644 --- a/cloud/filestore/tools/analytics/libs/event-log/dump_ut.cpp +++ b/cloud/filestore/tools/analytics/libs/event-log/dump_ut.cpp @@ -159,6 +159,8 @@ Y_UNIT_TEST_SUITE(TDumpTest) TEST_REQUEST_TYPE(51, DescribeData); TEST_REQUEST_TYPE(52, GenerateBlobIds); TEST_REQUEST_TYPE(53, AddData); + TEST_REQUEST_TYPE(54, ReadBlob); + TEST_REQUEST_TYPE(55, WriteBlob); // Fuse TEST_REQUEST_TYPE(1001, Flush); From 6776da2105c22b662b1851fcccf824130ad38417 Mon Sep 17 00:00:00 2001 From: Maxim Deb Natkh Date: Fri, 12 Apr 2024 17:07:08 +0000 Subject: [PATCH 2/3] fix review issues --- .../libs/storage/service/service_actor_readdata.cpp | 5 ++--- .../libs/storage/service/service_actor_writedata.cpp | 4 ++-- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/cloud/filestore/libs/storage/service/service_actor_readdata.cpp b/cloud/filestore/libs/storage/service/service_actor_readdata.cpp index 68c2cbb797..d6410f9285 100644 --- a/cloud/filestore/libs/storage/service/service_actor_readdata.cpp +++ b/cloud/filestore/libs/storage/service/service_actor_readdata.cpp @@ -386,10 +386,9 @@ void TReadDataActor::HandleReadBlobResponse( TABLET_VERIFY(ev->Cookie < DescribeResponse.BlobPiecesSize()); const auto& blobPiece = DescribeResponse.GetBlobPieces(ev->Cookie); - uint64_t blobIdx = ev->Cookie; + ui64 blobIdx = ev->Cookie; TABLET_VERIFY( - 0 <= blobIdx && blobIdx < InFlightBSRequests.size() && - InFlightBSRequests[blobIdx] && + blobIdx < InFlightBSRequests.size() && InFlightBSRequests[blobIdx] && !InFlightBSRequests[blobIdx]->IsCompleted()); InFlightBSRequests[blobIdx]->Complete(ctx.Now(), {}); diff --git a/cloud/filestore/libs/storage/service/service_actor_writedata.cpp b/cloud/filestore/libs/storage/service/service_actor_writedata.cpp index e6197de031..85ddf6008a 100644 --- a/cloud/filestore/libs/storage/service/service_actor_writedata.cpp +++ b/cloud/filestore/libs/storage/service/service_actor_writedata.cpp @@ -234,11 +234,11 @@ class TWriteDataActor final: public TActorBootstrapped "TEvPutResult response received: %s", msg->ToString().c_str()); - auto blobIdx = msg->Id.Cookie(); + ui64 blobIdx = msg->Id.Cookie(); // It is implicitly expected that cookies are generated in increasing // order starting from 0. TABLET_VERIFY( - 0 <= blobIdx && blobIdx < InFlightBSRequests.size() && + blobIdx < InFlightBSRequests.size() && InFlightBSRequests[blobIdx] && !InFlightBSRequests[blobIdx]->IsCompleted()); InFlightBSRequests[blobIdx]->Complete(ctx.Now(), {}); From 13eb5992254c096762596b57912e2b927352fa26 Mon Sep 17 00:00:00 2001 From: Maxim Deb Natkh Date: Fri, 12 Apr 2024 18:12:53 +0000 Subject: [PATCH 3/3] fix test --- cloud/filestore/tools/analytics/libs/event-log/dump_ut.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cloud/filestore/tools/analytics/libs/event-log/dump_ut.cpp b/cloud/filestore/tools/analytics/libs/event-log/dump_ut.cpp index 418747547c..1556b89273 100644 --- a/cloud/filestore/tools/analytics/libs/event-log/dump_ut.cpp +++ b/cloud/filestore/tools/analytics/libs/event-log/dump_ut.cpp @@ -94,7 +94,7 @@ Y_UNIT_TEST_SUITE(TDumpTest) { const auto requests = GetRequestTypes(); - UNIT_ASSERT_VALUES_EQUAL(68, requests.size()); + UNIT_ASSERT_VALUES_EQUAL(70, requests.size()); ui32 index = 0; #define TEST_REQUEST_TYPE(id, name) \