Bug #10117
closedOSD crashes if xattr "_" is absent for the file when doing backfill scanning (ReplicatedPG::scan_range)
0%
Description
We observed a OSD crash pattern which is due to xattr "_" is absent for the file (on filesystem) which result in an assertion failure, corresponding code snippet is:
void ReplicatedPG::scan_range(...) {
for (vector<hobject_t>::iterator p = ls.begin(); p != ls.end(); ++p) {
handle.reset_tp_timeout();
ObjectContextRef obc;
if (is_primary())
obc = object_contexts.lookup(*p);
if (obc) {
bi->objects[*p] = obc->obs.oi.version;
dout(20) << " " << *p << " " << obc->obs.oi.version << dendl;
} else {
bufferlist bl;
int r = pgbackend->objects_get_attr(*p, OI_ATTR, &bl);
assert(r >= 0);
...
}
...
}
I am not sure if there is a better way to handle such data corruption, but a better way might be to tolerate such corruption and use redundancy to construct the list.
Ceph version: v0.80.4
Platform: RHEL6.5
Updated by Samuel Just over 9 years ago
This should probably be a feature request for the backlog. We need a test reproducing it and some code to tolerate it. I'm not sure what the correct solution is, it might be to delete the object, add it to the divergent_priors set in the log (so that it remains missing), notify the primary, and recover the object. Such machinery is probably how we want to handle corrupt objects in general.
Updated by Wenjun Huang over 9 years ago
Samuel Just wrote:
This should probably be a feature request for the backlog. We need a test reproducing it and some code to tolerate it. I'm not sure what the correct solution is, it might be to delete the object, add it to the divergent_priors set in the log (so that it remains missing), notify the primary, and recover the object. Such machinery is probably how we want to handle corrupt objects in general.
Hi, Samuel:
I think there is apparent flaw of this code snippet, as data corruption is common case in commodity hardware. If the object's "_" attr corrupts, and when backfilling occurs, the corresponding osd will crash. I reproduce the issue in my cluster when take the below steps:
1. Use XFS'a attr command to remove an object's "_" attr.(The corresponding physical file's attr is "ceph._")
2. Trigger an backfilling of the cluster.
3. There will be osd crash for the attr absence.
In my opinion, there might be some ways to enhance the code:
1. When the "_" attr absence detected, we can repair the object instantly through its replicated peer.
2. We can delete the object, and it will recover in next self-healing triggers(scrub or deep scrub).
Updated by Wenjun Huang over 9 years ago
Wenjun Huang wrote:
Samuel Just wrote:
This should probably be a feature request for the backlog. We need a test reproducing it and some code to tolerate it. I'm not sure what the correct solution is, it might be to delete the object, add it to the divergent_priors set in the log (so that it remains missing), notify the primary, and recover the object. Such machinery is probably how we want to handle corrupt objects in general.
Hi, Samuel:
I think there is apparent flaw of this code snippet, as data corruption is common case in commodity hardware. If the object's "_" attr corrupts, and when backfilling occurs, the corresponding osd will crash. I reproduce the issue in my cluster when take the below steps:
1. Use XFS'a attr command to remove an object's "_" attr.(The corresponding physical file's attr is "ceph._")
2. Trigger an backfilling of the cluster.
3. There will be osd crash for the attr absence.In my opinion, there might be some ways to enhance the code:
1. When the "_" attr absence detected, we can repair the object instantly through its replicated peer.
2. We can delete the object, and it will recover in next self-healing triggers(scrub or deep scrub).
I prefer the second way, because the first way seems very complicated, which may bring much effort and hidden more bugs.
Updated by Samuel Just over 9 years ago
Right, the deleting the object portion is what I was talking about. I think that's the right way.
Updated by Wenjun Huang over 9 years ago
Samuel Just wrote:
Right, the deleting the object portion is what I was talking about. I think that's the right way.
Hi, Samuel
I have gone through you fix about this bug. [[http://tracker.ceph.com/projects/ceph/repository/revisions/643c173377f9c489c6c035516967c710dc15b8a4/diff/src/osd/ReplicatedPG.cc]]
But I think it is not enough for this issue. Diving into the flow of pgbackend->objects_get_attr(*p, OI_ATTR, &bl), I think ceph can handle two failures itself:
1) ENOENT : may be returned when opening the file. (The fix handled)
2) ENOATTR : may be returned when invoke fgetxattr. (The fix missed, and I think it is the cause of this issue)
So, in my opinion, if we can change the fix like this:
if (r -ENOENT || r -ENOATTR)
continue;
Thank you!
Updated by Wenjun Huang over 9 years ago
Sorry for my carelessness, I meant:if (r -ENOENT || r -ENOATTR)
continue;
Updated by Wenjun Huang over 9 years ago
Wenjun Huang wrote:
Sorry for my carelessness, I meant:
if (r -ENOENT || r -ENOATTR)
continue;
Sorry for my carelessness, I meant:
if (r -ENOENT || r -ENOATTR)
continue;
Updated by Wenjun Huang over 9 years ago
I'm sorry, that I forgot use correct format before, so it omitted some character:
if (r == -ENOENT || r == -ENOATTR)
continue;
Updated by Guang Yang over 9 years ago
Wenjun Huang wrote:
Just had a quick chat with Sam:I'm sorry, that I forgot use correct format before, so it omitted some character:
[...]
- We should not handle ENOATTR the same way as ENOENT, Sam's fix was to fix the general race.
- In order to fix this issue (data corruption), we will need a way to general repair the object, Loic is working on this and we can further discuss.