Skip to content
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: Add sparse read for sync image #11005

Merged
merged 1 commit into from Sep 30, 2016

Conversation

jazeltq
Copy link

@jazeltq jazeltq commented Sep 7, 2016

Currently, the image sync do full read, and we shall add sparse read
to let the sync more efficiently.

Feature: http://tracker.ceph.com/issues/16780
Signed-off-by: tianqing tianqing@unitedstack.com

@jazeltq
Copy link
Author

jazeltq commented Sep 7, 2016

@dillaman

@dillaman dillaman self-assigned this Sep 7, 2016
@jazeltq jazeltq force-pushed the add-sparse-read branch 2 times, most recently from 8b6db5c to 2561b2e Compare September 8, 2016 07:37
@dillaman
Copy link

dillaman commented Sep 9, 2016

@jazeltq unit tests are failing

@jazeltq
Copy link
Author

jazeltq commented Sep 9, 2016

@dillaman ok, i will check....

@@ -103,6 +103,8 @@ void ObjectCopyRequest<I>::send_read_object() {

bool read_required = false;
librados::ObjectReadOperation op;
std::map<uint64_t, uint64_t> extents;
bufferlist xbl;
Copy link

@dillaman dillaman Sep 15, 2016

Choose a reason for hiding this comment

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

Local variable -- will be destructed before async operation completes. Update the tuple to provide storage for the extents and bufferlist.

Copy link
Author

Choose a reason for hiding this comment

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

Ok. Thanks dillaman.

for(auto &it : extents){
bufferlist tmpbl;
len = it.second - it.first;
tmpbl.copy_in(begin, len, databl);

Choose a reason for hiding this comment

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

Use bufferlist::substr_of to quickly build a new zero-copy write bl

Copy link
Author

Choose a reason for hiding this comment

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

Ok. Thanks dillaman.

RWLock::RLocker l(file->lock);
len = clip_io(off, len, file->data.length());

if (m != NULL) {
m->clear();
if (len > 0) {
(*m)[off] = len;
Copy link
Author

Choose a reason for hiding this comment

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

@dillaman The sparse_read here is not right. It just put the length to the extents. How to use bufferlist to emulate sparse_read here ?

Choose a reason for hiding this comment

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

For the unit tests, it's probably safe enough to just populate the entire requested extent even if it doesn't exist. This test stub doesn't track holes, so you wouldn't get a true implementation.

@jazeltq jazeltq force-pushed the add-sparse-read branch 5 times, most recently from e60ca08 to d476fce Compare September 18, 2016 10:35
@jazeltq
Copy link
Author

jazeltq commented Sep 18, 2016

ping @dillaman

@@ -173,14 +172,19 @@ void ObjectCopyRequest<I>::send_write_object() {

auto &sync_ops = m_snap_sync_ops.begin()->second;
assert(!sync_ops.empty());

uint64_t begin;

Choose a reason for hiding this comment

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

Minor: can you use a more descriptive variable name? Perhaps something like buffer_offset

Currently, the image sync do full read, and we shall add sparse read
to let the sync more efficiently.

Feature: http://tracker.ceph.com/issues/16780
Signed-off-by: tianqing <tianqing@unitedstack.com>
@jazeltq
Copy link
Author

jazeltq commented Sep 26, 2016

ping @dillaman

@dillaman dillaman merged commit 42d5111 into ceph:master Sep 30, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants