tool: separating ceph-dedup-tool into tool and daemon #56067
tool: separating ceph-dedup-tool into tool and daemon #56067
Conversation
There was a problem hiding this comment.
I stopped reviewing for now after the above comments. Before re-requesting review please do the following:
- Add a commit adding comment to every member or variable accessed in more than one thread
// Accessed in the main thread and in the worker threads under <lock_name> - Ensure that for every one of those variables, the same lock is held for all accesses -- both reads and writes.
- Ensure that there are as few of the above as possible
Also, do not complicate the locking structure by adding more locks or by adding shared-exclusive locks for now -- let's focus on getting the simple version correct first.
| ceph::mutex glock = ceph::make_mutex("glock"); | ||
| class SampleDedupWorkerThread; | ||
| std::list<SampleDedupWorkerThread> threads; | ||
| bool all_stop = false; |
There was a problem hiding this comment.
Before moving onto your next work item, please submit a PR refactoring these globals into an automatic variable declared in main().
There was a problem hiding this comment.
OK. I'll submit another PR to refactor the globals after this PR.
| register_async_signal_handler_oneshot(SIGINT, handle_signal); | ||
| register_async_signal_handler_oneshot(SIGTERM, handle_signal); | ||
|
|
||
| int ret = make_crawling_daemon(opts); |
There was a problem hiding this comment.
Before your next work item, submit a PR renaming make_crawling_daemon to run_crawling_daemon.
| << "Chunk Size : " << chunk_size << std::endl | ||
| << std::endl; | ||
|
|
||
| while (!all_stop) { |
There was a problem hiding this comment.
Reading all_stop here is undefined behavior as glock is not held.
| cout << "new iteration" << std::endl; | ||
|
|
||
| ObjectCursor current_object = begin; | ||
| while (!stop && current_object < end) { |
There was a problem hiding this comment.
Reading stop here is undefined behavior as m_lock is not held. In a larger sense -- why bother checking stop here and not all_stop? Why add m_lock/stop in addition to g_lock/all_stop?
…dedup_daemon Signed-off-by: Myoungwon Oh <myoungwon.oh@samsung.com>
Signed-off-by: Myoungwon Oh <myoungwon.oh@samsung.com>
fd8f6aa to
cc2856d
Compare
|
@athanatos Can you take a look at 4d903e1 and 8129d82 to see if I have addressed your comments? |
…rrupt signal Signed-off-by: Myoungwon Oh <myoungwon.oh@samsung.com>
Signed-off-by: Myoungwon Oh <myoungwon.oh@samsung.com>
Since the ceph-dedup-daemon exclusively uses set-chunk op, dedup options in pool_info_t is not necessary. These options are relevant only when handling tier-flush op. Plus, this commit include the removal of unused code that is no longer in use. Signed-off-by: Myoungwon Oh <myoungwon.oh@samsung.com>
cc2856d to
31a98c2
Compare
|
I've made some extra changes to the updated commits to address your comments. |
|
Can we drop "tool/ceph_dedup: rework deduplication procedure in the daemon" and introduce it in its own PR? |
Signed-off-by: Myoungwon Oh <myoungwon.oh@samsung.com>
Signed-off-by: Myoungwon Oh <myoungwon.oh@samsung.com>
31a98c2 to
5b06232
Compare
@athanatos OK. Done. |
|
jenkins test signed |
athanatos
left a comment
There was a problem hiding this comment.
Ok, lgtm once it goes through testing. Once this is scheduled for a run and the run completes, please confirm that the run includes your modified test and that the test passes. Ping me again once that happens and I'll take care of doing the merge.
This review identified 2-3 small changes that should be their own PRs, let's do those next (one at a time, please).
#53992
PR 1 : separating dedup tool and daemon (commit 1-3)
PR 2 : renaming and refactoring global variable
PR 3 : rework deduplication procedure in a async. manner
PR 4 : handling dedup-daemon's parameter and config + tests and subconscious bug fixes (commit 4-18)
PR 5 : Depending on the progress, integration to cephadm? or unittest framework (commit 19~)
Signed-off-by: Myoungwon Oh myoungwon.oh@samsung.com
Contribution Guidelines
To sign and title your commits, please refer to Submitting Patches to Ceph.
If you are submitting a fix for a stable branch (e.g. "quincy"), please refer to Submitting Patches to Ceph - Backports for the proper workflow.
When filling out the below checklist, you may click boxes directly in the GitHub web UI. When entering or editing the entire PR message in the GitHub web UI editor, you may also select a checklist item by adding an
xbetween the brackets:[x]. Spaces and capitalization matter when checking off items this way.Show available Jenkins commands
jenkins retest this pleasejenkins test classic perfjenkins test crimson perfjenkins test signedjenkins test make checkjenkins test make check arm64jenkins test submodulesjenkins test dashboardjenkins test dashboard cephadmjenkins test apijenkins test docsjenkins render docsjenkins test ceph-volume alljenkins test ceph-volume toxjenkins test windowsjenkins test rook e2e