[fs_connector][feat]: Add file management module (file_io) for fs connector #176
Conversation
kfirtoledo
requested review from
dannyharnik,
elevran and
vMaroon
as code owners
kfirtoledo
changed the title
dannyharnik
left a comment
dannyharnik
left a comment
There was a problem hiding this comment.
Looks good but need to ensure we don't hit the race condition I pointed out
kfirtoledo
force-pushed
the
connector_file_io
branch
from
717d535 to
ad87dbc
Compare
…nector Signed-off-by: Kfir Toledo <kfir.toledo@ibm.com>
kfirtoledo
force-pushed
the
connector_file_io
branch
from
ad87dbc to
3492df9
Compare
|
|
||
| // Write to a temporary file to ensure atomic replace on rename | ||
| // Include thread_stream_idx so each thread uses a unique temporary file | ||
| std::string tmp_path = target_path + std::to_string(thread_stream_idx) + ".tmp."; |
There was a problem hiding this comment.
What if you have multiple vllm instances writing to the same path?
I think it's thus safer to generate a temporary file name instead of target_path + std::to_string(thread_stream_idx) + ".tmp.".
There was a problem hiding this comment.
The stream_idx was added to avoid such a case (assuming it is unique).
Could use a random string as well...
There was a problem hiding this comment.
I will change it to random to be on the safe side.
| ifs.seekg(0, std::ios::beg); | ||
|
|
||
| // Acquire pinned buffer of the required size | ||
| auto [pinned_ptr, pinned_size] = get_thread_local_pinned(file_size); |
There was a problem hiding this comment.
where is get_thread_local_pinned defined?
There was a problem hiding this comment.
in buffer.cpp https://github.com/llm-d/llm-d-kv-cache-manager/pull/177/files#:~:text=std%3A%3Apair%3Cvoid*%2C%20size_t%3E-,get_thread_local_pinned(,-size_t%20required_bytes%2C%20int%20numa_node)%20%7B
I split it just to make the review easier
| } | ||
|
|
||
| // Read file into pinned memory | ||
| ifs.read(reinterpret_cast<char*>(pinned_ptr), file_size); |
There was a problem hiding this comment.
What about setting a (1MB?) read buffer like you did on the other function?
e.g.
ifs.rdbuf()->pubsetbuf(...);
There was a problem hiding this comment.
For ifstream, the data is read in one large call directly into the pinned staging buffer, so there is no need for an extra buffer.
For ofstream, the internal buffer is small and leads to many write syscalls, so we use a 1MB thread-local buffer to speed up writes.
In the next version, I plan to check more deeply whether fstream gives the best read and write performance.
Signed-off-by: Kfir Toledo <kfir.toledo@ibm.com>
|
@orozery and @dannyharnik PTAL |
3 participants
This PR introduces the file management module used by the filesystem offloading connector.
Included in this PR: