New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
rbd-mirror: fix sparse read optimization in image sync #12368
Conversation
@dillaman It is not tested well yet and I am going to make it more readable. Right now I just want to know your opinion if that agrees with what you proposed to fix this issue. |
@@ -353,6 +377,7 @@ void ObjectCopyRequest<I>::compute_diffs() { | |||
bufferlist(), | |||
std::map<uint64_t, uint64_t>()); | |||
} | |||
m_snap_object_sizes[end_remote_snap_id] = end_size; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what this is protecting against. The same end_size is encoded in the trunc op above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... never mind, I see how it's updated in the WRITE op case.
buffer_offset += it.second; | ||
dout(20) << ": write op: " << it.first<< "~" << it.second << dendl; | ||
sparse_read_offset = std::get<1>(sync_op); | ||
for (auto it : std::get<4>(sync_op)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking you could just use the interval_set, initialize one to the requested offset + length, subtract out the extents within the sparse read result, and issue a zero / truncate ops for the remaining extents. The truncate would only (possibly) apply to the last extent if its offset + length == the object size.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using interval_set was my initial thought too, but with my current approach I preserve operations order (i.e. the next op is for larger offset), while with interval_set approach it looks I would generate write ops first, then zero/truncate ops, i.e. the offset order would be lost. Does it make any difference for the store how ops are ordered?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While it shouldn't matter, I suppose it's possible that sending them out-of-order could expose some edge condition within the OSDs that is not normally exercised. Probably best to just keep your current implementation to be safe.
Issue truncate or zero ops for the subtracted extents between the diff and the sparse read. Fixes: http://tracker.ceph.com/issues/18146 Signed-off-by: Mykola Golub <mgolub@mirantis.com>
@dillaman updated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Issue truncate or zero ops for the subtracted extents between the
diff and the sparse read.
Fixes: http://tracker.ceph.com/issues/18146
Signed-off-by: Mykola Golub mgolub@mirantis.com