Skip to content

[Filestore] Move copy of fresh blocks to TFlushActor - #7467

Open
antonmyagkov wants to merge 7 commits into
mainfrom
users/myagkov/eliminate-copy-of-fresh-blocks
Open

[Filestore] Move copy of fresh blocks to TFlushActor#7467
antonmyagkov wants to merge 7 commits into
mainfrom
users/myagkov/eliminate-copy-of-fresh-blocks

Conversation

@antonmyagkov

@antonmyagkov antonmyagkov commented Oct 7, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

issue: #7468

  1. Keep block index in BlockBuffer and shared pointer to BlockBuffer instead of allocating fresh blocks

struct TBlockDataHolder { size_t BufferBlockIndex; IBlockBufferPtr BlockBuffer; }

  1. Move blob allocation to TFlushActor

Also I removed allocator for fresh blocks, so it will not report statistics. However fresh blocks can be additionally allocated for unaligned tail and after tablet restart.

@github-actions

github-actions Bot commented Oct 7, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Note

This is an automated comment that will be appended during run.

Note

All workloads for linux-x86_64-relwithdebinfo have completed.

Tip

Planned checks for linux-x86_64-relwithdebinfo.

  • ⚫ tasks + storage + fastshard (job skipped)
  • ⚫ blockstore (job skipped)
  • ⚫ disk + manager (job skipped)
  • ⚫ filestore (job skipped)

@antonmyagkov
antonmyagkov force-pushed the users/myagkov/eliminate-copy-of-fresh-blocks branch from acd1644 to 84cc23f Compare October 7, 2026 11:39
@github-actions

github-actions Bot commented Oct 7, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Note

This is an automated comment that will be appended during run.

Note

All workloads for linux-x86_64-relwithdebinfo have completed.

Tip

Planned checks for linux-x86_64-relwithdebinfo.

@antonmyagkov
antonmyagkov force-pushed the users/myagkov/eliminate-copy-of-fresh-blocks branch from 84cc23f to d7313b3 Compare October 7, 2026 11:47
@github-actions

github-actions Bot commented Oct 7, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Note

This is an automated comment that will be appended during run.

Note

All workloads for linux-x86_64-relwithdebinfo have completed.

Tip

Planned checks for linux-x86_64-relwithdebinfo.

🟢 linux-x86_64-relwithdebinfo target: cloud/tasks/,cloud/storage/,cloud/fastshard/ (test time: 279s): all tests PASSED for commit d7313b3.

TESTS PASSED ERRORS FAILED FAILED BUILD SKIPPED MUTED?
1733 1733 0 0 0 0 0

🟢 linux-x86_64-relwithdebinfo target: cloud/disk_manager/ (test time: 364s): all tests PASSED for commit d7313b3.

TESTS PASSED ERRORS FAILED FAILED BUILD SKIPPED MUTED?
1798 1798 0 0 0 0 0

🟢 linux-x86_64-relwithdebinfo target: cloud/blockstore/ (test time: 1528s): all tests PASSED for commit d7313b3.

TESTS PASSED ERRORS FAILED FAILED BUILD SKIPPED MUTED?
7611 7610 0 0 0 1 0

@antonmyagkov
antonmyagkov force-pushed the users/myagkov/eliminate-copy-of-fresh-blocks branch from d7313b3 to 7fefcd2 Compare October 7, 2026 12:36
@github-actions

github-actions Bot commented Oct 7, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Note

This is an automated comment that will be appended during run.

Note

All workloads for linux-x86_64-relwithdebinfo have completed.

Tip

Planned checks for linux-x86_64-relwithdebinfo.

  • ⚫ tasks + storage + fastshard (job skipped)
  • ⚫ blockstore (job skipped)
  • ⚫ disk + manager (job skipped)
  • ⚫ filestore (job skipped)

@antonmyagkov
antonmyagkov force-pushed the users/myagkov/eliminate-copy-of-fresh-blocks branch from 7fefcd2 to 702213d Compare October 7, 2026 12:46
@github-actions

github-actions Bot commented Oct 7, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Note

This is an automated comment that will be appended during run.

Note

All workloads for linux-x86_64-relwithdebinfo have completed.

Tip

Planned checks for linux-x86_64-relwithdebinfo.

🟢 linux-x86_64-relwithdebinfo target: cloud/tasks/,cloud/storage/,cloud/fastshard/ (test time: 279s): all tests PASSED for commit 702213d.

TESTS PASSED ERRORS FAILED FAILED BUILD SKIPPED MUTED?
1733 1733 0 0 0 0 0

🟢 linux-x86_64-relwithdebinfo target: cloud/disk_manager/ (test time: 332s): all tests PASSED for commit 702213d.

TESTS PASSED ERRORS FAILED FAILED BUILD SKIPPED MUTED?
1798 1798 0 0 0 0 0

🟢 linux-x86_64-relwithdebinfo target: cloud/blockstore/ (test time: 1491s): all tests PASSED for commit 702213d.

TESTS PASSED ERRORS FAILED FAILED BUILD SKIPPED MUTED?
7611 7610 0 0 0 1 0

@antonmyagkov
antonmyagkov marked this pull request as ready for review October 7, 2026 14:00
@github-actions

github-actions Bot commented Oct 7, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Note

This is an automated comment that will be appended during run.

Note

All workloads for linux-x86_64-relwithdebinfo have completed.

Tip

Planned checks for linux-x86_64-relwithdebinfo.

@github-actions

github-actions Bot commented Oct 7, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Note

This is an automated comment that will be appended during run.

Note

All workloads for linux-x86_64-relwithdebinfo have completed.

Tip

Planned checks for linux-x86_64-relwithdebinfo.

🟢 linux-x86_64-relwithdebinfo target: cloud/tasks/,cloud/storage/,cloud/fastshard/ (test time: 273s): all tests PASSED for commit b0136be.

TESTS PASSED ERRORS FAILED FAILED BUILD SKIPPED MUTED?
1733 1733 0 0 0 0 0

🟢 linux-x86_64-relwithdebinfo target: cloud/disk_manager/ (test time: 319s): all tests PASSED for commit b0136be.

TESTS PASSED ERRORS FAILED FAILED BUILD SKIPPED MUTED?
1798 1798 0 0 0 0 0

🔴 linux-x86_64-relwithdebinfo target: cloud/blockstore/ (test time: 1535s): some tests FAILED for commit b0136be.

TESTS PASSED ERRORS FAILED FAILED BUILD SKIPPED MUTED?
7586 7584 0 1 0 1 0

🟢 linux-x86_64-relwithdebinfo target: cloud/blockstore/ (test time: 965s): all tests PASSED for commit b0136be.

TESTS PASSED ERRORS FAILED FAILED BUILD SKIPPED MUTED?
7611 7610 0 0 0 1 0

🔴 linux-x86_64-relwithdebinfo target: cloud/filestore/ (test time: 5460s) Test run completed, no test results found for commit b0136be.
Please check build logs.

🟢 linux-x86_64-relwithdebinfo target: cloud/filestore/ (test time: 5144s): all tests PASSED for commit b0136be.

TESTS PASSED ERRORS FAILED FAILED BUILD SKIPPED MUTED?
4915 4914 0 0 0 1 0

@github-actions

github-actions Bot commented Oct 9, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Note

This is an automated comment that will be appended during run.

Note

All workloads for linux-x86_64-relwithdebinfo have completed.

Tip

Planned checks for linux-x86_64-relwithdebinfo.

🟢 linux-x86_64-relwithdebinfo target: cloud/tasks/,cloud/storage/,cloud/fastshard/ (test time: 266s): all tests PASSED for commit e31a409.

TESTS PASSED ERRORS FAILED FAILED BUILD SKIPPED MUTED?
1733 1733 0 0 0 0 0

🟢 linux-x86_64-relwithdebinfo target: cloud/disk_manager/ (test time: 314s): all tests PASSED for commit e31a409.

TESTS PASSED ERRORS FAILED FAILED BUILD SKIPPED MUTED?
1798 1798 0 0 0 0 0

🟢 linux-x86_64-relwithdebinfo target: cloud/blockstore/ (test time: 1651s): all tests PASSED for commit e31a409.

TESTS PASSED ERRORS FAILED FAILED BUILD SKIPPED MUTED?
7611 7610 0 0 0 1 0

@github-actions

github-actions Bot commented Oct 9, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Note

This is an automated comment that will be appended during run.

Note

All workloads for linux-x86_64-relwithdebinfo have completed.

Tip

Planned checks for linux-x86_64-relwithdebinfo.

@antonmyagkov antonmyagkov changed the title [Filestore] Eliminate copy of fresh blocks [Filestore] Move copy of fresh blocks to TFlushActor Oct 9, 2026
@github-actions

github-actions Bot commented Oct 9, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Note

This is an automated comment that will be appended during run.

Note

All workloads for linux-x86_64-relwithdebinfo have completed.

Tip

Planned checks for linux-x86_64-relwithdebinfo.

@antonmyagkov antonmyagkov added filestore Add this label to run only cloud/filestore build and tests on PR large-tests Launch large tests for PR msan Launch builds with memory sanitizer along with regular build tsan Launch builds with thread sanitizer along with regular build ubsan Launch builds with undefined behaviour sanitizer along with regular build labels Oct 9, 2026
@antonmyagkov antonmyagkov added the asan Launch builds with address sanitizer along with regular build label Oct 9, 2026
@github-actions

github-actions Bot commented Oct 9, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Note

This is an automated comment that will be appended during run.

Note

All workloads for linux-x86_64-relwithdebinfo have completed.

Tip

Planned checks for linux-x86_64-relwithdebinfo.

🔴 linux-x86_64-relwithdebinfo target: cloud/filestore/ (test time: 7934s): some tests FAILED for commit f392ba1.

TESTS PASSED ERRORS FAILED FAILED BUILD SKIPPED MUTED?
4968 4966 0 1 0 1 0

🔴 linux-x86_64-relwithdebinfo target: cloud/filestore/ (test time: 33s): some tests FAILED for commit f392ba1.

TESTS PASSED ERRORS FAILED FAILED BUILD SKIPPED MUTED?
2 1 0 1 0 0 0

🔴 linux-x86_64-relwithdebinfo target: cloud/filestore/ (test time: 19s): some tests FAILED for commit f392ba1.

TESTS PASSED ERRORS FAILED FAILED BUILD SKIPPED MUTED?
2 1 0 1 0 0 0

@github-actions

github-actions Bot commented Oct 9, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Note

This is an automated comment that will be appended during run.

Note

All workloads for linux-x86_64-release-ubsan have completed.

Tip

Planned checks for linux-x86_64-release-ubsan.

🟢 linux-x86_64-release-ubsan target: cloud/filestore/ (test time: 4681s): all tests PASSED for commit f392ba1.

TESTS PASSED ERRORS FAILED FAILED BUILD SKIPPED MUTED?
4258 4258 0 0 0 0 0

@github-actions

github-actions Bot commented Oct 9, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Note

This is an automated comment that will be appended during run.

Note

All workloads for linux-x86_64-release-tsan have completed.

Tip

Planned checks for linux-x86_64-release-tsan.

🔴 linux-x86_64-release-tsan target: cloud/filestore/ (test time: 5430s): some tests FAILED for commit f392ba1.

TESTS PASSED ERRORS FAILED FAILED BUILD SKIPPED MUTED?
4198 4197 0 1 0 0 0

@github-actions

github-actions Bot commented Oct 9, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Note

This is an automated comment that will be appended during run.

Note

All workloads for linux-x86_64-release-asan have completed.

Tip

Planned checks for linux-x86_64-release-asan.

🟢 linux-x86_64-release-asan target: cloud/filestore/ (test time: 4975s): all tests PASSED for commit f392ba1.

TESTS PASSED ERRORS FAILED FAILED BUILD SKIPPED MUTED?
4208 4208 0 0 0 0 0

@github-actions

github-actions Bot commented Oct 9, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Note

This is an automated comment that will be appended during run.

Note

All workloads for linux-x86_64-release-msan have completed.

Tip

Planned checks for linux-x86_64-release-msan.

🟢 linux-x86_64-release-msan target: cloud/filestore/ (test time: 4948s): all tests PASSED for commit f392ba1.

TESTS PASSED ERRORS FAILED FAILED BUILD SKIPPED MUTED?
4208 4208 0 0 0 0 0

SvartMetal
SvartMetal requested changes Oct 12, 2026
View reviewed changes
Comment thread cloud/filestore/libs/storage/tablet/model/blob_builder.cpp
////////////////////////////////////////////////////////////////////////////////

void TMixedBlobBuilder::Accept(const TBlock& block, TStringBuf blockData)
void TMixedBlobBuilder::Accept(const TBlock& block, const TBlockDataHolder& blockData)

@SvartMetal SvartMetal Oct 12, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

80 symbols

Comment thread cloud/filestore/libs/storage/tablet/model/block.h

[[nodiscard]] TStringBuf GetBlockData() const
{
Y_DEBUG_ABORT_UNLESS(BlockBuffer);

@SvartMetal SvartMetal Oct 12, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Y_ABORT_UNLESS

because you will dereference null below anyway

Comment thread cloud/filestore/libs/storage/tablet/model/fresh_blocks.cpp
return inserted;
}

bool TFreshBlocks::AddBlock(

@SvartMetal SvartMetal Oct 12, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Let's add a comment that we use this when we need to write an unaligned tail at the end of the fresh block, or when tablet is restarted and fresh blocks are recovered from FreshBlocks local DB table

Let's emphasize that this function copies fresh block's data

Comment thread cloud/filestore/libs/storage/tablet/model/fresh_blocks.cpp
TString buffer;
buffer.reserve(blockSize);
buffer.append(blockData);
buffer.resize(blockSize, 0);

@SvartMetal SvartMetal Oct 12, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Let's add a comment that it fills the buffer with remaining zeroes

Comment thread cloud/filestore/libs/storage/tablet/tablet_actor_flush.cpp
const auto ok = GenerateBlobId(
commitId,
blob.BlobContent.size(),
blob.BlobContent.size() * GetBlockSize(),

@SvartMetal SvartMetal Oct 12, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Let's double check that we have no similar places in code

For example we can change BlobContent's type and observe compilation errors

Comment thread cloud/filestore/libs/storage/tablet/tablet_actor_writedata.cpp
args.CommitId,
b,
args.Buffer->GetBlock(b - args.ByteRange.FirstAlignedBlock()));
{b - args.ByteRange.FirstAlignedBlock(), args.Buffer});

@SvartMetal SvartMetal Oct 12, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Let's use a full type name here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@SvartMetal SvartMetal SvartMetal requested changes
@qkrorlqr qkrorlqr Awaiting requested review from qkrorlqr

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

asan Launch builds with address sanitizer along with regular build filestore Add this label to run only cloud/filestore build and tests on PR large-tests Launch large tests for PR msan Launch builds with memory sanitizer along with regular build tsan Launch builds with thread sanitizer along with regular build ubsan Launch builds with undefined behaviour sanitizer along with regular build

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@antonmyagkov @SvartMetal