osd/PrimaryLogPG: refresh local last-complete iter for async_recovery_targets properly#21598
Closed
xiexingguo wants to merge 2 commits intoceph:masterfrom
Closed
osd/PrimaryLogPG: refresh local last-complete iter for async_recovery_targets properly#21598xiexingguo wants to merge 2 commits intoceph:masterfrom
xiexingguo wants to merge 2 commits intoceph:masterfrom
Conversation
Member
Author
4e8f33c to
218caca
Compare
…_targets properly Upon overwritting an already-missing object on async_recovery_targets, besides updating the **missing** set async_recovery_targets should call reset_complete_to() simultaneously to reset **complete-to** iter and advance **last_complete** properly, as those old log entries which we used to rely on for recovery are now getting replaced by new **committed** ones. Since the recovery process of an async_recovery_target can be arbitrarily delayed until we hit the osd_max_pg_log_entries limit, the main befenit of an extra call to reset_complete_to() here is to make sure primary perform log-trim in time. Otherwise accumulating of huge amounts of **obsolete** log entries can slow the system down obviously. Signed-off-by: xiexingguo <xie.xingguo@zte.com.cn>
neha-ojha
approved these changes
Apr 23, 2018
Member
neha-ojha
left a comment
There was a problem hiding this comment.
This will probably be useful. Needs to be run against the rados suite to ensure that there are no side effects.
Member
Author
|
This change is currently triggering crashes. Please drop it from your test branch. :-( |
Member
Author
|
I appended another fix to address the crashes from @tchaikov 's test branch and the newest test results seem to be positive: http://pulpito.ceph.com/xxg-2018-04-25_06:14:51-rados:thrash-wip-refresh-lc-distro-basic-smithi/ http://pulpito.ceph.com/xxg-2018-04-25_04:38:44-rados-wip-refresh-lc-distro-basic-smithi/ Mind taking another glance at? |
neha-ojha
reviewed
Apr 25, 2018
| } | ||
| clear_temp_objs(op.temp_removed); | ||
| dout(30) << __func__ << " missing before " << get_parent()->get_log().get_missing().get_items() << dendl; | ||
| get_parent()->log_operation( |
Member
There was a problem hiding this comment.
Is there a reason why we want to move the dout of "missing before"?
Member
Author
There was a problem hiding this comment.
dropped that cosmetic change ( I was trying to wrap that line into 80 chars, but I think it is fine to leave as it is...)
01c628c to
bddf91e
Compare
neha-ojha
reviewed
Apr 26, 2018
src/osd/ReplicatedBackend.cc
Outdated
| update_snaps = true; | ||
| } | ||
|
|
||
| parent->update_stats(m->pg_stats); |
Member
There was a problem hiding this comment.
update_stats() may not be required to move as well? looks good overall.
bddf91e to
2125ee3
Compare
The problem is that if we update pg_missing first, **reset_complete_to()** will probably try to move **complete-to** iter to the newest log entry which we haven't persisted into local pg_log list (we haven't called **log_operation()** yet) and hence trigger the coredump below: ``` 0> 2018-04-24 15:42:47.270 7fb4694d5700 -1 /build/ceph-13.0.2-1706-g01c4f53/src/osd/PGLog.h: In function 'void PGLog::reset_complete_to(pg_info_t*)' thread 7fb4694d5700 time 2018-04-24 15:42:47.270648 /build/ceph-13.0.2-1706-g01c4f53/src/osd/PGLog.h: 778: FAILED assert(log.complete_to != log.log.end()) ceph version 13.0.2-1706-g01c4f53 (01c4f53802f46966433ab1d453afac6db7e5b707) mimic (dev) 1: (ceph::__ceph_assert_fail(char const*, char const*, int, char const*)+0x102) [0x7fb494648f82] 2: (()+0x2e5157) [0x7fb494649157] 3: (non-virtual thunk to PrimaryLogPG::add_local_next_event(pg_log_entry_t const&)+0x478) [0x55d70493aff8] 4: (ReplicatedBackend::do_repop(boost::intrusive_ptr<OpRequest>)+0x855) [0x55d704a3ead5] ``` See also: http://qa-proxy.ceph.com/teuthology/kchai-2018-04-24_14:54:28-rados-wip-kefu-testing-2018-04-24-1145-distro-basic-mira/2434437/ Signed-off-by: xie xingguo <xie.xingguo@zte.com.cn>
Contributor
Member
|
This looks like itw as causing scrub errors in my run |
Member
|
yep, still causing scrub errors http://pulpito.ceph.com/sage-2018-05-18_18:17:59-rados-wip-sage3-testing-2018-05-18-1124-distro-basic-smithi/ |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Upon overwritting an already-missing object on async_recovery_targets,
besides updating the missing set async_recovery_targets should
call reset_complete_to() simultaneously to reset complete-to iter and
advance last_complete properly, as those old log entries which we used
to rely on for recovery are now getting replaced by new committed ones.
Since the recovery process of an async_recovery_target can be arbitrarily
delayed until we hit the osd_max_pg_log_entries limit, the main befenit
of an extra call to reset_complete_to() here is to make sure primary perform
log-trim in time. Otherwise accumulating of huge amounts of obsolete log
entries can slow the system down obviously.
Signed-off-by: xiexingguo xie.xingguo@zte.com.cn