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

osd: FileJournal: support batch peak and pop from writeq #6701

Merged
merged 1 commit into from Dec 18, 2015

Conversation

XinzeChi
Copy link
Contributor

This would reduce writeq_lock contentions a lot.
Signed-off-by: Xinze Chi xinze@xsky.com

@XinzeChi
Copy link
Contributor Author

@liewegas , is it make sense? It perform better in ssd.

@XinzeChi
Copy link
Contributor Author

@jack-changtao , There are too many places where we use writeq. If do as you said, I think this would be a little complicated. @liewegas

@yuyuyu101
Copy link
Member

we need a benchmark result refer to reviewers.

@XinzeChi
Copy link
Contributor Author

XinzeChi commented Dec 9, 2015

image

Above is the benchmark based on XinzeChi@cde544e.
The hardware environment is:
CPU: Intel(R) Xeon(R) CPU E5-2620 v2 @ 2.10GHz
SSD: Intel P3710

assert(write_lock.is_locked());
Mutex::Locker locker(writeq_lock);
writeq.swap(items);
}
Copy link
Member

Choose a reason for hiding this comment

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

batch_pop_write (it's not peek, since we're removing the items)

@liewegas
Copy link
Member

Hmm, was that test looking just at latency for the journal or from the client?

@XinzeChi
Copy link
Contributor Author

the test is base on XinzeChi@cde544e. The latency is for the journal. I think XinzeChi@cde544e is good benckmark tool for filejournal?
If you like this benchmark tool, I would pull a request for it. The benchmark is written by haomai.

@liewegas
Copy link
Member

Ah. My concern if this buys us only a few % for just the journal and the journal is only a fraction of the overall write latency it may not be worth the risk of breaking journal replay or compatibility with other versions. Note that the new BlueStore backend won't use any of this code, so Jewel is likely the last release where FileStore will be the default.

@XinzeChi
Copy link
Contributor Author

I think you mention #6856 instead of this patch. Tthis patch is clean and safe.

@liewegas
Copy link
Member

Ah, yes, mixed them up. This is simple and clean.

Signed-off-by: Xinze Chi <xinze@xsky.com>
@liewegas
Copy link
Member

But, please do keep in mind that this code isn't going to be used for too much longer. You might want to focus efforts on optimizations with the new backend(s), or in the layers above ObjectStore.

liewegas added a commit that referenced this pull request Dec 18, 2015
osd: FileJournal: support batch peak and pop from writeq

Reviewed-by: Sage Weil <sage@redhat.com>
@liewegas liewegas merged commit 04e2908 into ceph:master Dec 18, 2015
@ghost ghost changed the title FileJournal: Support batch peak and pop from writeq osd: FileJournal: support batch peak and pop from writeq Feb 10, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants