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

os/bluestore/NVMEDevice: Add multiple thread support for SPDK I/O thread #14420

Merged
merged 1 commit into from Apr 13, 2017

Conversation

Projects
None yet
6 participants
@optimistyzy
Contributor

optimistyzy commented Apr 10, 2017

Previously, we only have one thread to do SPDK I/O, and this patch
adds the multiple support

Signed-off-by: Ziye Yang optimistyzy@gmail.com

@optimistyzy

This comment has been minimized.

Show comment
Hide comment
@optimistyzy

optimistyzy Apr 10, 2017

Contributor

@yuyuyu101 I re-opened this PR. @liuhongtong helped me a lot to test the functionality of multiple threads. And this patch will not affect the usage of the previous single thread. Also, we will dig more later for the performance of multiple I/O threads.

Contributor

optimistyzy commented Apr 10, 2017

@yuyuyu101 I re-opened this PR. @liuhongtong helped me a lot to test the functionality of multiple threads. And this patch will not affect the usage of the previous single thread. Also, we will dig more later for the performance of multiple I/O threads.

@liupan1111

This comment has been minimized.

Show comment
Hide comment
@liupan1111

liupan1111 Apr 10, 2017

Contributor

@optimistyzy could you say something about the performance enhancement of this multi thread?

Contributor

liupan1111 commented Apr 10, 2017

@optimistyzy could you say something about the performance enhancement of this multi thread?

@liewegas liewegas added the bluestore label Apr 10, 2017

Show outdated Hide outdated src/os/bluestore/NVMEDevice.cc
@@ -16,6 +16,8 @@
*/
#include <unistd.h>
#include <sys/syscall.h>
#define gettid() syscall(SYS_gettid)

This comment has been minimized.

@yuyuyu101

yuyuyu101 Apr 11, 2017

Member

why gettid instead pthread_self()

@yuyuyu101

yuyuyu101 Apr 11, 2017

Member

why gettid instead pthread_self()

This comment has been minimized.

@yuyuyu101

yuyuyu101 Apr 11, 2017

Member

common/io_priority.h
20:extern pid_t ceph_gettid();

@yuyuyu101

yuyuyu101 Apr 11, 2017

Member

common/io_priority.h
20:extern pid_t ceph_gettid();

@optimistyzy

This comment has been minimized.

Show comment
Hide comment
@optimistyzy

optimistyzy Apr 11, 2017

Contributor
Contributor

optimistyzy commented Apr 11, 2017

@yuyuyu101

This comment has been minimized.

Show comment
Hide comment
@yuyuyu101

yuyuyu101 Apr 11, 2017

Member

I think the sharding way should follow bluestore hash instead of thread id.

Member

yuyuyu101 commented Apr 11, 2017

I think the sharding way should follow bluestore hash instead of thread id.

@yuyuyu101

This comment has been minimized.

Show comment
Hide comment
@yuyuyu101

yuyuyu101 Apr 11, 2017

Member

@liewegas any idea on future sharding model?

Member

yuyuyu101 commented Apr 11, 2017

@liewegas any idea on future sharding model?

@optimistyzy

This comment has been minimized.

Show comment
Hide comment
@optimistyzy

optimistyzy Apr 12, 2017

Contributor

@yuyuyu101 can we firstly use thread_id. Currently, We use fio in the client for test. Via thread id, we can distribute all the I/Os from one thread to the dedicated SPDK I/O thread, which seems a natural way. Also a new version of patch is uploaded

Contributor

optimistyzy commented Apr 12, 2017

@yuyuyu101 can we firstly use thread_id. Currently, We use fio in the client for test. Via thread id, we can distribute all the I/Os from one thread to the dedicated SPDK I/O thread, which seems a natural way. Also a new version of patch is uploaded

@liewegas

This comment has been minimized.

Show comment
Hide comment
@liewegas

liewegas Apr 12, 2017

Member

Seems okay for now. Agree that eventually we should use the bluestore/osd sharding model though!

Member

liewegas commented Apr 12, 2017

Seems okay for now. Agree that eventually we should use the bluestore/osd sharding model though!

Show outdated Hide outdated src/os/bluestore/NVMEDevice.cc
@@ -1058,7 +1133,9 @@ int NVMEDevice::read_random(uint64_t off, uint64_t len, char *buf, bool buffered
t->copy_to_buf(buf, off-t->offset, len);
};
++ioc.num_reading;
driver->queue_task(t);
if(queue_id == -1)
queue_id = ceph_gettid();

This comment has been minimized.

@liewegas

liewegas Apr 12, 2017

Member

indentation and whitespace

@liewegas

liewegas Apr 12, 2017

Member

indentation and whitespace

Show outdated Hide outdated src/os/bluestore/NVMEDevice.cc
@@ -992,7 +1067,9 @@ int NVMEDevice::read(uint64_t off, uint64_t len, bufferlist *pbl,
t->copy_to_buf(buf, 0, t->len);
};
++ioc->num_reading;
driver->queue_task(t);
if(queue_id == -1)
queue_id = ceph_gettid();

This comment has been minimized.

@liewegas

liewegas Apr 12, 2017

Member

indentation and whitespace

@liewegas

liewegas Apr 12, 2017

Member

indentation and whitespace

Show outdated Hide outdated src/os/bluestore/NVMEDevice.cc
@@ -954,7 +1027,9 @@ int NVMEDevice::aio_write(
if (buffered) {
// Only need to push the first entry
driver->queue_task(t);
if(queue_id == -1)
queue_id = ceph_gettid();

This comment has been minimized.

@liewegas

liewegas Apr 12, 2017

Member

indentation and whitespace

@liewegas

liewegas Apr 12, 2017

Member

indentation and whitespace

Show outdated Hide outdated src/os/bluestore/NVMEDevice.cc
@@ -926,7 +997,9 @@ void NVMEDevice::aio_submit(IOContext *ioc)
ioc->num_pending -= pending;
assert(ioc->num_pending.load() == 0); // we should be only thread doing this
// Only need to push the first entry
driver->queue_task(t, pending);
if(queue_id == -1)
queue_id = ceph_gettid();

This comment has been minimized.

@liewegas

liewegas Apr 12, 2017

Member

indentation and whitespace

@liewegas

liewegas Apr 12, 2017

Member

indentation and whitespace

Show outdated Hide outdated src/os/bluestore/NVMEDevice.cc
driver->flush_wait();
if(queue_id == -1)
queue_id = ceph_gettid();

This comment has been minimized.

@liewegas

liewegas Apr 12, 2017

Member

indentation and whitespace

@liewegas

liewegas Apr 12, 2017

Member

indentation and whitespace

Show outdated Hide outdated src/os/bluestore/NVMEDevice.cc
assert(!spdk_nvme_cpl_is_error(completion));
dout(20) << __func__ << " write/zero op successfully, left "
<< driver->queue_op_seq - driver->completed_op_seq << dendl;
<<queue->queue_op_seq - queue->completed_op_seq << dendl;

This comment has been minimized.

@liewegas

liewegas Apr 12, 2017

Member

whitespace

@liewegas

liewegas Apr 12, 2017

Member

whitespace

Show outdated Hide outdated src/os/bluestore/NVMEDevice.cc
it->reap_ioc();
//be careful, here we need to have a methods,and let each thread reap its own, currently it is done
// by only one dedicatd dpdk thread
if(!queueid) {

This comment has been minimized.

@liewegas

liewegas Apr 12, 2017

Member

whitespace

@liewegas

liewegas Apr 12, 2017

Member

whitespace

bluestore, NVMEDevice: Add multiple thread support for SPDK I/O thread
Previously, we only have one thread to do SPDK I/O, and this patch
adds the multiple thread support.

In this first version, we use the tid of the thread to map the I/Os
of this thread to the corresponding SPDK I/O thread.

Signed-off-by: Ziye Yang <optimistyzy@gmail.com>
@optimistyzy

This comment has been minimized.

Show comment
Hide comment
@optimistyzy

optimistyzy Apr 12, 2017

Contributor

@liewegas updated with your comments.

Contributor

optimistyzy commented Apr 12, 2017

@liewegas updated with your comments.

@liewegas liewegas changed the title from bluestore, NVMEDevice: Add multiple thread support for SPDK I/O thread to os/bluestore/NVMEDevice: Add multiple thread support for SPDK I/O thread Apr 12, 2017

@optimistyzy

This comment has been minimized.

Show comment
Hide comment
@optimistyzy

optimistyzy Apr 13, 2017

Contributor

@yuyuyu101 @liewegas can this patch be merged?

Contributor

optimistyzy commented Apr 13, 2017

@yuyuyu101 @liewegas can this patch be merged?

@yuyuyu101

This comment has been minimized.

Show comment
Hide comment
@yuyuyu101

yuyuyu101 Apr 13, 2017

Member

retest this please

Member

yuyuyu101 commented Apr 13, 2017

retest this please

@optimistyzy

This comment has been minimized.

Show comment
Hide comment
@optimistyzy

optimistyzy Apr 13, 2017

Contributor
Contributor

optimistyzy commented Apr 13, 2017

@tchaikov

This comment has been minimized.

Show comment
Hide comment
@tchaikov

tchaikov Apr 13, 2017

Contributor

@optimistyzy "r3test this please" is a command to instruct jenkins to re-trigger the "make check" test.

Contributor

tchaikov commented Apr 13, 2017

@optimistyzy "r3test this please" is a command to instruct jenkins to re-trigger the "make check" test.

@optimistyzy

This comment has been minimized.

Show comment
Hide comment
@optimistyzy

optimistyzy Apr 13, 2017

Contributor

@tchikov thanks for explaining. It seems that I misunderstand the meaning.

Contributor

optimistyzy commented Apr 13, 2017

@tchikov thanks for explaining. It seems that I misunderstand the meaning.

@liewegas liewegas merged commit 9967fce into ceph:master Apr 13, 2017

3 checks passed

Signed-off-by all commits in this PR are signed
Details
Unmodifed Submodules submodules for project are unmodified
Details
default Build finished.
Details

oritwas pushed a commit to oritwas/ceph that referenced this pull request Apr 21, 2017

rbd-mirror: store replay status in mirroring object
Fixes: #14420
Signed-off-by: Mykola Golub <mgolub@mirantis.com>
(cherry picked from commit 52b2fe1)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment