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/PGLog: add roll_forward_to(to update crt) to PGLog update_missing #38906
Conversation
3d1536d
to
a7bcb55
Compare
a7bcb55
to
383343e
Compare
383343e
to
4831a50
Compare
66bcf56
to
3e3d14c
Compare
e996e89
to
47d4973
Compare
src/osd/PGLog.h
Outdated
@@ -1257,6 +1257,7 @@ struct PGLog : DoutPrefixProvider { | |||
bool maintain_rollback, | |||
IndexedLog *log, | |||
missing_type &missing, | |||
bool &should_rollforward, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out params like should_rollforward should be passed as a pointer with null allowed as a way for the caller to ignore the output, see CodingStyle.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks, will take into accord.
updated code.
Also, kept whitespace changes for different PR for brevity
Test output:
[100%] Built target unittest_pglog
2021-01-25T22:32:57.574+0000 7fdbb80b5280 10 **merge_log log((1'1,2'1], crt=0'0) from osd.? into log((1'1,2'1], crt=0'0)**
2021-01-25T22:32:57.574+0000 7fdbb80b5280 10 **merge_log result log((1'1,2'1], crt=0'0) missing(0 may_include_deletes = 1) changed=0**
2021-01-25T22:32:57.574+0000 7fdbb80b5280 10 **merge_log log((1'1,2'1], crt=0'0) from osd.? into log((1'1,2'1], crt=0'0)**
2021-01-25T22:32:57.574+0000 7fdbb80b5280 10 **merge_log result log((1'1,2'1], crt=0'0) missing(0 may_include_deletes = 0) changed=0**
2021-01-25T22:32:57.574+0000 7fdbb80b5280 10 **merge_log log((1'1,1'5], crt=0'0) from osd.? into log((1'4,1'5], crt=0'0)**
2021-01-25T22:32:57.574+0000 7fdbb80b5280 10 merge_log extending tail to 1'1
2021-01-25T22:32:57.574+0000 7fdbb80b5280 15 1'1 (0'0) unknown -9223372036854775808:a0000000::::0 by unknown.0.0:0 0.000000 0 ObjectCleanRegions clean_offsets: [0~18446744073709551615], clean_omap: 1, new_object: 0
2021-01-25T22:32:57.574+0000 7fdbb80b5280 10 **merge_log result log((1'1,1'5], crt=0'0) missing(0 may_include_deletes = 0) changed=1**
2021-01-25T22:32:57.574+0000 7fdbb80b5280 10 **merge_log log((1'1,2'4], crt=0'0) from osd.? into log((1'1,1'3], crt=0'0)**
2021-01-25T22:32:57.574+0000 7fdbb80b5280 10 merge_log extending head to 2'4
2021-01-25T22:32:57.574+0000 7fdbb80b5280 20 ? 2'4 (0'0) delete -9223372036854775808:e0000000::::0 by unknown.0.0:0 0.000000 0 ObjectCleanRegions clean_offsets: [0~18446744073709551615], clean_omap: 1, new_object: 0
2021-01-25T22:32:57.574+0000 7fdbb80b5280 20 ? 2'3 (0'0) modify -9223372036854775808:90000000::::0 by unknown.0.0:0 0.000000 0 ObjectCleanRegions clean_offsets: [0~18446744073709551615], clean_omap: 1, new_object: 0
2021-01-25T22:32:57.574+0000 7fdbb80b5280 20 ? 1'2 (0'0) delete -9223372036854775808:c0000000::::0 by unknown.0.0:0 0.000000 0 ObjectCleanRegions clean_offsets: [0~18446744073709551615], clean_omap: 1, new_object: 0
2021-01-25T22:32:57.574+0000 7fdbb80b5280 20 merge_log cut point (usually last shared) is 1'2
2021-01-25T22:32:57.574+0000 7fdbb80b5280 20 merge_log original_crt = 0'0
2021-01-25T22:32:57.574+0000 7fdbb80b5280 10 merge_log divergent 1'3 (0'0) delete -9223372036854775808:90000000::::0 by unknown.0.0:0 0.000000 0 ObjectCleanRegions clean_offsets: [0~18446744073709551615], clean_omap: 1, new_object: 0
2021-01-25T22:32:57.574+0000 7fdbb80b5280 20 update missing, appended 2'3 (0'0) modify -9223372036854775808:90000000::::0 by unknown.0.0:0 0.000000 0 ObjectCleanRegions clean_offsets: [0~18446744073709551615], clean_omap: 1, new_object: 0
2021-01-25T22:32:57.574+0000 7fdbb80b5280 20 update missing, appended 2'4 (0'0) delete -9223372036854775808:e0000000::::0 by unknown.0.0:0 0.000000 0 ObjectCleanRegions clean_offsets: [0~18446744073709551615], clean_omap: 1, new_object: 0
2021-01-25T22:32:57.574+0000 7fdbb80b5280 20 _merge_object_divergent_entries: merging hoid -9223372036854775808:90000000::::0 entries: 1'3 (0'0) delete -9223372036854775808:90000000::::0 by unknown.0.0:0 0.000000 0 ObjectCleanRegions clean_offsets: [0~18446744073709551615], clean_omap: 1, new_object: 0
2021-01-25T22:32:57.574+0000 7fdbb80b5280 20 _merge_object_divergent_entries: keeping 1'3 (0'0) delete -9223372036854775808:90000000::::0 by unknown.0.0:0 0.000000 0 ObjectCleanRegions clean_offsets: [0~18446744073709551615], clean_omap: 1, new_object: 0
2021-01-25T22:32:57.574+0000 7fdbb80b5280 10 _merge_object_divergent_entries: hoid object_not_in_store: 0
2021-01-25T22:32:57.574+0000 7fdbb80b5280 10 _merge_object_divergent_entries: hoid -9223372036854775808:90000000::::0 prior_version: 0'0 first_divergent_update: 1'3 last_divergent_update: 1'3
2021-01-25T22:32:57.574+0000 7fdbb80b5280 10 _merge_object_divergent_entries: more recent entry found: 2'3 (0'0) modify -9223372036854775808:90000000::::0 by unknown.0.0:0 0.000000 0 ObjectCleanRegions clean_offsets: [0~18446744073709551615], clean_omap: 1, new_object: 0, already merged
2021-01-25T22:32:57.574+0000 7fdbb80b5280 10 crt updated after new entries merged to 2'4
2021-01-25T22:32:57.574+0000 7fdbb80b5280 10 **merge_log result log((1'1,2'4], crt=2'4) missing(2 may_include_deletes = 1) changed=1**
2021-01-25T22:32:57.574+0000 7fdbb80b5280 10 **merge_log log((1'1,2'4], crt=0'0) from osd.? into log((1'1,1'3], crt=0'0)**
2021-01-25T22:32:57.574+0000 7fdbb80b5280 10 merge_log extending head to 2'4
2021-01-25T22:32:57.574+0000 7fdbb80b5280 20 ? 2'4 (0'0) delete -9223372036854775808:e0000000::::0 by unknown.0.0:0 0.000000 0 ObjectCleanRegions clean_offsets: [0~18446744073709551615], clean_omap: 1, new_object: 0
2021-01-25T22:32:57.574+0000 7fdbb80b5280 20 ? 2'3 (0'0) modify -9223372036854775808:90000000::::0 by unknown.0.0:0 0.000000 0 ObjectCleanRegions clean_offsets: [0~18446744073709551615], clean_omap: 1, new_object: 0
2021-01-25T22:32:57.574+0000 7fdbb80b5280 20 ? 1'2 (0'0) delete -9223372036854775808:c0000000::::0 by unknown.0.0:0 0.000000 0 ObjectCleanRegions clean_offsets: [0~18446744073709551615], clean_omap: 1, new_object: 0
2021-01-25T22:32:57.574+0000 7fdbb80b5280 20 merge_log cut point (usually last shared) is 1'2
2021-01-25T22:32:57.574+0000 7fdbb80b5280 20 merge_log original_crt = 0'0
2021-01-25T22:32:57.575+0000 7fdbb80b5280 10 merge_log divergent 1'3 (0'0) delete -9223372036854775808:90000000::::0 by unknown.0.0:0 0.000000 0 ObjectCleanRegions clean_offsets: [0~18446744073709551615], clean_omap: 1, new_object: 0
2021-01-25T22:32:57.575+0000 7fdbb80b5280 20 update missing, appended 2'3 (0'0) modify -9223372036854775808:90000000::::0 by unknown.0.0:0 0.000000 0 ObjectCleanRegions clean_offsets: [0~18446744073709551615], clean_omap: 1, new_object: 0
2021-01-25T22:32:57.575+0000 7fdbb80b5280 20 update missing, appended 2'4 (0'0) delete -9223372036854775808:e0000000::::0 by unknown.0.0:0 0.000000 0 ObjectCleanRegions clean_offsets: [0~18446744073709551615], clean_omap: 1, new_object: 0
2021-01-25T22:32:57.575+0000 7fdbb80b5280 20 _merge_object_divergent_entries: merging hoid -9223372036854775808:90000000::::0 entries: 1'3 (0'0) delete -9223372036854775808:90000000::::0 by unknown.0.0:0 0.000000 0 ObjectCleanRegions clean_offsets: [0~18446744073709551615], clean_omap: 1, new_object: 0
2021-01-25T22:32:57.575+0000 7fdbb80b5280 20 _merge_object_divergent_entries: keeping 1'3 (0'0) delete -9223372036854775808:90000000::::0 by unknown.0.0:0 0.000000 0 ObjectCleanRegions clean_offsets: [0~18446744073709551615], clean_omap: 1, new_object: 0
2021-01-25T22:32:57.575+0000 7fdbb80b5280 10 _merge_object_divergent_entries: hoid object_not_in_store: 0
2021-01-25T22:32:57.575+0000 7fdbb80b5280 10 _merge_object_divergent_entries: hoid -9223372036854775808:90000000::::0 prior_version: 0'0 first_divergent_update: 1'3 last_divergent_update: 1'3
2021-01-25T22:32:57.575+0000 7fdbb80b5280 10 _merge_object_divergent_entries: more recent entry found: 2'3 (0'0) modify -9223372036854775808:90000000::::0 by unknown.0.0:0 0.000000 0 ObjectCleanRegions clean_offsets: [0~18446744073709551615], clean_omap: 1, new_object: 0, already merged
2021-01-25T22:32:57.575+0000 7fdbb80b5280 10 crt updated after new entries merged to 2'4
2021-01-25T22:32:57.575+0000 7fdbb80b5280 10 **merge_log result log((1'1,2'4], crt=2'4) missing(1 may_include_deletes = 0) changed=1**
2021-01-25T22:32:57.575+0000 7fdbb80b5280 10 merge_log log((1'1,1'4], crt=0'0) from osd.? into log((1'1,1'5], crt=0'0)
2021-01-25T22:32:57.575+0000 7fdbb80b5280 10 rewind_divergent_log truncate divergent future 1'4
2021-01-25T22:32:57.575+0000 7fdbb80b5280 20 rewind_divergent_log original_crt = 0'0
2021-01-25T22:32:57.575+0000 7fdbb80b5280 10 rewind_divergent_log future divergent 1'5 (0'0) unknown -9223372036854775808:90000000::::0 by unknown.0.0:0 0.000000 0 ObjectCleanRegions clean_offsets: [0~18446744073709551615], clean_omap: 1, new_object: 0
2021-01-25T22:32:57.575+0000 7fdbb80b5280 20 _merge_object_divergent_entries: merging hoid -9223372036854775808:90000000::::0 entries: 1'5 (0'0) unknown -9223372036854775808:90000000::::0 by unknown.0.0:0 0.000000 0 ObjectCleanRegions clean_offsets: [0~18446744073709551615], clean_omap: 1, new_object: 0
2021-01-25T22:32:57.575+0000 7fdbb80b5280 20 _merge_object_divergent_entries: keeping 1'5 (0'0) unknown -9223372036854775808:90000000::::0 by unknown.0.0:0 0.000000 0 ObjectCleanRegions clean_offsets: [0~18446744073709551615], clean_omap: 1, new_object: 0
2021-01-25T22:32:57.575+0000 7fdbb80b5280 10 _merge_object_divergent_entries: hoid object_not_in_store: 0
2021-01-25T22:32:57.575+0000 7fdbb80b5280 10 _merge_object_divergent_entries: hoid -9223372036854775808:90000000::::0 prior_version: 0'0 first_divergent_update: 1'5 last_divergent_update: 1'5
2021-01-25T22:32:57.575+0000 7fdbb80b5280 10 _merge_object_divergent_entries: hoid -9223372036854775808:90000000::::0 has no more recent entries in log
2021-01-25T22:32:57.575+0000 7fdbb80b5280 10 _merge_object_divergent_entries: hoid -9223372036854775808:90000000::::0 prior_version or op type indicates creation, deleting
2021-01-25T22:32:57.575+0000 7fdbb80b5280 10 **merge_log result log((1'1,1'4], crt=0'0) missing(0 may_include_deletes = 0) changed=1**
[ OK ] PGLogTest.merge_log (1 ms)
[----------] 1 test from PGLogTest (1 ms total)
[----------] Global test environment tear-down
[==========] 1 test from 1 test suite ran. (1 ms total)
[ PASSED ] 1 test.
8704541
to
d7ebf64
Compare
d7ebf64
to
3966447
Compare
src/osd/PGLog.h
Outdated
log->add(*p); | ||
log->add(*p); | ||
ldpp_dout(dpp, 20) << "update missing, appended " << *p << dendl; | ||
if (should_rollforward) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like it should be outside of the if(log) check. Also, add braces.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@athanatos but we specifically want to check this itself, since we only want to rollforward, if actually log->add
operation happened or not.
You test changes appear to pass without the changes to merge_log. I'd like at least one test case that doesn't pass on master. |
we need to check the if log if entries were actually appended successfully using append_log_entries_update_missing instead of updating can_rollback_to value unconditionally fixes: https://tracker.ceph.com/issues/48609 Signed-off-by: Deepika Upadhyay <dupadhya@redhat.com>
on merging logs can_rollback_to should only be updated if any new entry has been added. crt >> initial crt >> updated crt 1,2,3 case: 0'0 | 0'0 4 case: head updated & hence crt updated >> 0'0 | 2'4 5 case: divergent entry in log >> crt unchanged 0'0 | 0'0 fixes: https://tracker.ceph.com/issues/48609 Signed-off-by: Deepika Upadhyay <dupadhya@redhat.com>
3966447
to
9fd18b2
Compare
working on it |
the test case for creating a scenario seems non trivial, but to test that crt would have been updated even if log->add didn't happen, @neha-ojha suggested to check with debugging through teuthology: ( 3218028 )
earlier we would have updated crt if not for the check, this should verify behaviour until we can figure out unit test that would exercise this. |
3218028
to
6d7dbe8
Compare
There are a couple ways to make adding unit tests for this simpler:
|
@ideepika I'm not sure the code you have for this is exactly what I meant. I suggested adding a debug statement in the existing code (not with your fix) which could tell us if/when crt was updated without log addition in a teuthology run. This can be achieved by
|
Signed-off-by: Deepika Upadhyay <dupadhya@redhat.com>
6d7dbe8
to
54c54df
Compare
using with dispatch delays with which we can reproduce lrc issue, we enter peering state, whenever we receive log while peering, I get to see log addition without append.
|
@@ -453,7 +454,6 @@ void PGLog::merge_log(pg_info_t &oinfo, pg_log_t&& olog, pg_shard_t fromosd, | |||
for (auto &&oe: divergent) { | |||
dout(10) << "merge_log divergent " << oe << dendl; | |||
} | |||
log.roll_forward_to(log.head, rollbacker); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this PR can be simplified and made more unit-testable by only doing a roll_forward_to()
if new_entries
is not empty, which is essentially what is used for doing log->add()
and setting should_rollforward
.
were added.
fixes: https://tracker.ceph.com/issues/48609
Signed-off-by: Deepika Upadhyay dupadhya@redhat.com
Checklist
Show available Jenkins commands
jenkins retest this please
jenkins test classic perf
jenkins test crimson perf
jenkins test signed
jenkins test make check
jenkins test make check arm64
jenkins test submodules
jenkins test dashboard
jenkins test api
jenkins test docs
jenkins render docs
jenkins test ceph-volume all
jenkins test ceph-volume tox