[Filestore] issue-2189: support O_DIRECT in local filestore #2188
Conversation
budevg
added
large-tests
budevg
changed the base branch from
users/evgenybud/local-filestore-o-direct-base
to
main
budevg
added
asan
budevg
force-pushed
the
users/evgenybud/local-filestore-o-direct
branch
from
eb6f31f
to
2082bdd
Compare
budevg
changed the title
budevg
changed the base branch from
main
to
users/evgenybud/local-filestore-o-direct-base
budevg
changed the title
budevg
changed the title
| @@ -192,12 +197,13 @@ void TFileSystem::Read( | |||
| const auto& response = future.GetValue(); | |||
| if (auto self = ptr.lock(); CheckResponse(self, *callContext, req, response)) { | |||
| const auto& buffer = response.GetBuffer(); | |||
| auto alignOffset = response.GetAlignOffset(); | |||
There was a problem hiding this comment.
а почему в ответе не отдать выровненный буфер?
There was a problem hiding this comment.
Изначально проблема была в том, что у нас буфер передаётся в protobuf, а там bytes это TString, у которого нельзя контролировать выравнивание. Поэтому все эти хитрости. Сейчас я сделал, что буфер, как обычно, возвращается, а потом в нём можно найти правильное выравнивание.
|
Note This is an automated comment that will be appended during run. 🔴 linux-x86_64-relwithdebinfo: some tests FAILED for commit 2082bdd.
🔴 linux-x86_64-release-asan: some tests FAILED for commit 2082bdd.
|
|
Note This is an automated comment that will be appended during run. 🔴 linux-x86_64-relwithdebinfo: some tests FAILED for commit 2082bdd.
🔴 linux-x86_64-release-asan: some tests FAILED for commit 2082bdd.
🔴 linux-x86_64-release-tsan: some tests FAILED for commit 2082bdd.
|
budevg
force-pushed
the
users/evgenybud/local-filestore-o-direct
branch
from
fedda35
to
2082bdd
Compare
|
|
||
| TArrayRef<char> data(b.begin(), b.vend()); | ||
| TArrayRef<char> data((char*)b->Begin(), (char*)b->End()); |
There was a problem hiding this comment.
давай лучше без c-style cast
давай сделаем static_cast тут или вообще сделаем в TAlignedBuffer методы, возвращающие char*
| @@ -29,6 +29,9 @@ namespace { | |||
| \ | |||
| xxx(AsyncDestroyHandleEnabled, bool, false )\ | |||
| xxx(AsyncHandleOperationPeriod, TDuration, TDuration::MilliSeconds(50) )\ | |||
| \ | |||
| xxx(DirectIoEnabled, bool, false )\ | |||
| xxx(DirectIoAlign, ui32, 4096 )\ | |||
There was a problem hiding this comment.
давай лучше использовать size_literals.h для такого
то есть в данном случае это 4_KB
| @@ -0,0 +1 @@ | |||
| #include "aligned_buffer.h" | |||
There was a problem hiding this comment.
давай все-таки в cpp имплементацию двинем? мы в хедерах стараемся держать все-таки в основном имплементацию темплейтов, т.к. ее в общем случае в cpp не передвинешь, но тут же не темплейт
| const char* AlignedData; | ||
|
|
||
| public: | ||
| static std::pair<const char*, size_t> ExtractAlignedData(const TString& buffer, ui32 align) |
There was a problem hiding this comment.
ой, у нас же есть структура со смыслом "указатель + размер" - это TStringBuf, давай вернем его
| if (alignedData > buffer.end()) { | ||
| ythrow TServiceError(E_ARGUMENT) | ||
| << "Extracting unaligned buffer " << (void*)buffer.begin() | ||
| << " with size " << buffer.size(); |
There was a problem hiding this comment.
еще align надо в текст ошибки добавить
| , AlignedData(Buffer.begin()) | ||
| { | ||
| if (align) { | ||
| Y_ASSERT(IsPowerOf2(align)); // align should be power of 2 |
There was a problem hiding this comment.
Y_DEBUG_ABORT_UNLESS обычно делаем
| ythrow TServiceError(E_ARGUMENT) | ||
| << "Initializing from unaligned buffer " | ||
| << (void*)Buffer.begin() | ||
| << " with size " << Buffer.size(); |
|
|
||
| const char* End() const | ||
| { | ||
| if (!Buffer) { |
There was a problem hiding this comment.
этот if выглядит ненужным
|
|
||
| const char* Begin() const | ||
| { | ||
| if (!Buffer) { |
There was a problem hiding this comment.
этот if выглядит ненужным - ты же для пустого буфера все равно инициализируешь AlignedData значением Buffer.begin()
| Buffer.resize(AlignedDataOffset() + size); | ||
| } | ||
|
|
||
| TString& GetBuffer() |
There was a problem hiding this comment.
я non-const геттеры стараюсь называть AccessXXX, а не GetXXX, чтоб в месте вызова было сразу видно, что ты планируешь изменять то, что получил
давай назовем этот метод AccessBuffer()
а то я в месте вызова немного задумался, увидев геттер
| ythrow TServiceError(E_ARGUMENT) | ||
| << "Extracting unaligned buffer " << (void*)buffer.begin() | ||
| << " with size " << buffer.size(); | ||
| } |
There was a problem hiding this comment.
наверное, стоит также проверить, что длина aligned-куска >= тому, что запросили при чтении
т.к. если в реализации storage-слоя есть баг (или мисконфиг - флаг про поддержку o direct забыли), который приводит к тому, что мы отдаем буфер без выравнивания, получится, что мы отрежем голову буфера, хотя в ней есть нужные нам данные, и после этого останемся с буфером меньшего размера, чем хотели прочитать
| @@ -191,13 +193,16 @@ void TFileSystem::Read( | |||
| .Subscribe([=, ptr = weak_from_this()] (const auto& future) { | |||
| const auto& response = future.GetValue(); | |||
| if (auto self = ptr.lock(); CheckResponse(self, *callContext, req, response)) { | |||
| const auto& buffer = response.GetBuffer(); | |||
| auto align = Config->GetDirectIoEnabled() ? Config->GetDirectIoAlign() : 0; | |||
| auto [alignedData, size] = TAlignedBuffer::ExtractAlignedData( | |||
There was a problem hiding this comment.
нам бы как-то вместе с ответом доставить информацию о том, у нас есть в буфере выравнивание или нет
иначе получается хрупко - легко допустить ошибку, из-за которой сторадж-слой будет отдавать буферы без пустого места в голове, нужного для выравнивания, а этот код будет считать, что такое пустое место там есть, и будет отрезать у буфера невыровненную голову, хотя на самом деле в ней лежат данные
There was a problem hiding this comment.
Можно прямо в ответе отдавать инфу про alignment - в смысле, не тут по месту вычислять align, а брать из ответа
#2189