Add IOTracer reader, writer classes for reading/writing IO operations in a binary file #6958
Conversation
There was a problem hiding this comment.
@akankshamahajan15 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
akankshamahajan15
force-pushed
the
io_tracing
branch
2 times, most recently
from
dc71296
to
228a300
Compare
|
@akankshamahajan15 has updated the pull request. Re-import the pull request |
There was a problem hiding this comment.
@akankshamahajan15 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
|
||
| IOTraceRecord(const uint64_t& _access_timestamp, const TraceType& _trace_type, | ||
| const std::string& _file_operation, | ||
| const std::string& _io_status, const std::string& _file_name, |
There was a problem hiding this comment.
_file_name, _len and _offset are required fields for reads, as indicated by the comment earlier, right? Should we have another constructor with just the minimal information for other operations?
There was a problem hiding this comment.
Yes, I will add another constructors that would be more appropriate than calling with all the arguments.
trace_replay/io_tracer.h
Outdated
| IOTraceWriter(IOTraceWriter&&) = delete; | ||
| IOTraceWriter& operator=(IOTraceWriter&&) = delete; | ||
|
|
||
| Status WriteIOOp(const IOTraceRecord& record, const Slice& file_operation, |
There was a problem hiding this comment.
IOTraceRecord already encapsulates the file_operation, io_status and file_name right? Do we still need the other parameters?
There was a problem hiding this comment.
WriteIOOp uses PutLengthPrefixedSlice so either pass the Slice arguments in WriteIOOp("" in IOTraceRecord constructor) or convert the string to Slice in the WriteIOOp. I will change it to second option as I am adding FileSystem function to call WriteIOOp, there will be more strings added and not all are needed for all operations.
trace_replay/io_tracer.h
Outdated
| void EndIOTrace(); | ||
|
|
||
| bool is_tracing_enabled() const { | ||
| return writer_.load(std::memory_order_relaxed); |
There was a problem hiding this comment.
Does writer_ need to be an atomic variable? Access to writer_ is already protected by trace_writer_mutex_. is_tracing_enabled() seems like it would be more frequently called, so it would be better to find a cheaper way to do the check than an atomic variable.
There was a problem hiding this comment.
@anand1976 writer_ is protected by trace_writer_mutex_ in StartIOTrace and EndIOTrace. So if I change writer_ to not be atomic, can I access the value of writer_ without trace_writer_mutex_ in is_tracing_enabled(), Or is it fine to do so.
There was a problem hiding this comment.
Can we just use a bool to determine if tracing is enabled? writer_ can continue to be an atomic variable, and WriteIOOp anyway checks writer_ and acquires mutex, so the worst that can happen is a caller calls is_tracing_enabled(), and then calls WriteIOOp, which would simply ignore it if tracing has been disabled in the meantime.
akankshamahajan15
force-pushed
the
io_tracing
branch
from
228a300
to
cf8c92c
Compare
|
@akankshamahajan15 has updated the pull request. Re-import the pull request |
There was a problem hiding this comment.
@akankshamahajan15 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
akankshamahajan15
force-pushed
the
io_tracing
branch
from
cf8c92c
to
abefd5e
Compare
|
@akankshamahajan15 has updated the pull request. Re-import the pull request |
There was a problem hiding this comment.
@akankshamahajan15 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
akankshamahajan15
force-pushed
the
io_tracing
branch
from
abefd5e
to
aea7426
Compare
|
@akankshamahajan15 has updated the pull request. Re-import the pull request |
| // A mutex protects the writer_. | ||
| InstrumentedMutex trace_writer_mutex_; | ||
| std::atomic<IOTraceWriter*> writer_; | ||
| bool tracing_enabled; |
There was a problem hiding this comment.
Nit: It would be good to add a comment explaining why this is used instead of writer_ in is_tracing_enabled().
… in a binary file.
Summary: 1. As part of IOTracing project, Add a class IOTracer,
IOTraceReader and IOTracerWriter that writes the file operations
information in a binary file. IOTrace Record contains record information
and right now it contains access_timestamp, file_operation, file_name,
io_status, len, offset and later other options will be added when file
system APIs will be call IOTracer.
2. Add few unit test cases that verify that reading and writing to a IO
Trace file is working properly and before start trace and after ending
trace nothing is added to the binary file.
Test Plan: 1. make check -j64
2. New testcases for IOTracer.
Reviewers:
Subscribers:
Tasks:
Tags:
akankshamahajan15
force-pushed
the
io_tracing
branch
from
aea7426
to
5d4ae5e
Compare
|
@akankshamahajan15 has updated the pull request. Re-import the pull request |
There was a problem hiding this comment.
@akankshamahajan15 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
@akankshamahajan15 merged this pull request in 552fd76. |
… in a binary file (#6958) Summary: 1. As part of IOTracing project, Add a class IOTracer, IOTraceReader and IOTracerWriter that writes the file operations information in a binary file. IOTrace Record contains record information and right now it contains access_timestamp, file_operation, file_name, io_status, len, offset and later other options will be added when file system APIs will be call IOTracer. 2. Add few unit test cases that verify that reading and writing to a IO Trace file is working properly and before start trace and after ending trace nothing is added to the binary file. Pull Request resolved: facebook/rocksdb#6958 Test Plan: 1. make check -j64 2. New testcases for IOTracer. Reviewed By: anand1976 Differential Revision: D21943375 Pulled By: akankshamahajan15 fbshipit-source-id: 3532204e2a3eab0104bf411ab142e3fdd4fbce54 Signed-off-by: Changlong Chen <levisonchen@live.cn>
… in a binary file (#6958) Summary: 1. As part of IOTracing project, Add a class IOTracer, IOTraceReader and IOTracerWriter that writes the file operations information in a binary file. IOTrace Record contains record information and right now it contains access_timestamp, file_operation, file_name, io_status, len, offset and later other options will be added when file system APIs will be call IOTracer. 2. Add few unit test cases that verify that reading and writing to a IO Trace file is working properly and before start trace and after ending trace nothing is added to the binary file. Pull Request resolved: facebook/rocksdb#6958 Test Plan: 1. make check -j64 2. New testcases for IOTracer. Reviewed By: anand1976 Differential Revision: D21943375 Pulled By: akankshamahajan15 fbshipit-source-id: 3532204e2a3eab0104bf411ab142e3fdd4fbce54 Signed-off-by: Changlong Chen <levisonchen@live.cn>
Summary:1. As part of IOTracing project, Add a class IOTracer,
IOTraceReader and IOTracerWriter that writes the file operations
information in a binary file. IOTrace Record contains record information
and right now it contains access_timestamp, file_operation, file_name,
io_status, len, offset and later other options will be added when file
system APIs will be call IOTracer.
Trace file is working properly and before start trace and after ending
trace nothing is added to the binary file.
Test Plan: 1. make check -j64
2. New testcases for IOTracer.
Reviewers:
Subscribers:
Tasks:
Tags: