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
Handle EC recovery read errors #9304
Conversation
@dzafman is this supposed to be a (yet completed) fix for http://tracker.ceph.com/issues/13937? |
Fixes: ceph#9304 Signed-off-by: David Zafman <dzafman@redhat.com>
@tchaikov I updated the proper commit to show that this pull request does indeed try to address 13937. |
@athanatos Any suggestions on how to move forward is appreciated. |
Fixes: ceph#9304 Signed-off-by: David Zafman <dzafman@redhat.com>
@athanatos I updated to make failed_push() common. I did the following: created scenario There are 9 removes "waiting for scrub" some removes completed even on the pg going through this |
@dzafman I'm not sure what you mean by the last part. Do you mean that the requests stayed hung? |
@athanatos Yes, they are coming out of the dump_ops_in_flight |
@dzafman Sounds like a bug in master to me, probably worth tracking down. |
Fixes: ceph#9304 Signed-off-by: David Zafman <dzafman@redhat.com>
Fixes: #9304 Signed-off-by: David Zafman <dzafman@redhat.com>
3ae9b08
to
48e034b
Compare
@@ -500,6 +500,8 @@ void ECBackend::continue_recovery_op( | |||
assert(!op.recovery_progress.first); | |||
dout(10) << __func__ << ": canceling recovery op for obj " << op.hoid | |||
<< dendl; | |||
// XXX: Doing this on the read error handling just loops in recovery | |||
// How would this case ever actually happen? |
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.
You can't just use cancel_pull() in the read error handling because you know something the parent doesn't: that some of the sources are faulty. Here, I think we are failing because a source went down (not in the up or acting set, or it would have triggered peering to restart) and we no longer have enough copies. I think it's ok to just cancel the recovery op here because the parent would already have updated its missing_loc etc when it got the log update. Either the object later becomes recoverable again and recovery will retry or not in which case it will remain unfound.
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.
You added this code in d9106ce to fix tracker 8161. I tried to use an equivalent version in OnRecoveryReadComplete::finish() only to discovery that it didn't work. I'll have to look into whether I can do something like my new _failed_push() to handle this case too.
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.
Yep, that's what I was talking about. Like I said, here, it's ok because ReplicatedPG/PG already knows that the source died and won't retry until the object is found again. You need to call something which updates missing_loc (which I think you do earlier in this series with _failed_push).
Is this code causing a bug?
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 get it now. I didn't see this code fail in case. I'll remove the comment.
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.
K. It probably wouldn't hurt to add an explanatory comment if you want to. I had to spend 20 minutes trying to figure out why it didn't get handled by check_recovery_sources...
reviewed. |
9b0191f
to
82cdb93
Compare
{ | ||
assert(recovering.count(soid)); | ||
recovering.erase(soid); | ||
missing_loc.remove_location(soid, from); | ||
for (list<pg_shard_t>::const_iterator i = from.begin(); i != from.end() ; ++i) |
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.
could use range-based loop.
recovery_ops.erase(hoid); | ||
|
||
list<pg_shard_t> fl; | ||
for (map<pg_shard_t, int>::iterator i = res.errors.begin(); |
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.
could use range-based loop.
I think this looks ok to me |
This reverts commit 5bc5533. Conflicts: src/test/Makefile.am (no longer exists) src/test/erasure-code/Makefile.am (no longer exists) Signed-off-by: David Zafman <dzafman@redhat.com>
Remove unused struct Signed-off-by: David Zafman <dzafman@redhat.com>
Signed-off-by: David Zafman <dzafman@redhat.com>
Fixes: http://tracker.ceph.com/issues/13937 Signed-off-by: David Zafman <dzafman@redhat.com>
Signed-off-by: David Zafman <dzafman@redhat.com>
Signed-off-by: David Zafman <dzafman@redhat.com>
Caused by: 70e000a Signed-off-by: David Zafman <dzafman@redhat.com>
Low space broke test, saw "flags nearfull,pauserd,pausewr...." Signed-off-by: David Zafman <dzafman@redhat.com>
Fix for broken test-erasure-code.sh and test-erasure-eio.sh Signed-off-by: David Zafman <dzafman@redhat.com>
Reviewed-by: Loic Dachary <ldachary@redhat.com> |
lgtm also. |
@dzafman shall we backport this to jewel? |
@dzafman Ping re: jewel backport. Users are asking about it. |
I end up with active+degraded pg which doesn't repair if I remove an object on all nodes in a way where it gets recognized as missing. With this fix the OSD doesn't crash. Need to figure out how to get recovery to finish. I assume I can have an error which will have to be discovered later by scrubbing.