enable_group Segmentation fault #105
Comments
|
Thanks for the report.
I've reviewed the code, and while it's true that |
|
Hello, I'm not at the office right now, I will provide the information later. This issue occurs with very low probability in a Active-Standby mode when you modify the LUN ID and then change it back. When multipathd detects a change in pp wwid, update_pathvec_from_dm will remove the path and also remove the empty pg, which can lead to this issue. |
|
No problem, take your time. |
|
I've sent a patch to [email protected]. Subject is "libmultipath: fix handling of pp->pgindex". You can inspect it here, too. |
|
Hello. Could the following code in the update_pathvec_from_dm function also lead to remove paths and empty path groups? It could cause the same issue ? the logs related to my previous question: |
pp->pgindex is set in disassemble_map() when a map is parsed. There are various possiblities for this index to become invalid. pp->pgindex is only used in enable_group() and followover_should_fallback(), and both callers take no action if it is 0, which is the right thing to do if we don't know the path's pathgroup. Make sure pp->pgindex is reset to 0 in various places: - when it's orphaned, - before (re)grouping paths, - when we detect a bad mpp assignment in update_pathvec_from_dm(). - when a pathgroup is deleted in update_pathvec_from_dm(). In this case, pgindex needs to be invalidated for all paths in all pathgroups after the one that was deleted. The hunk in group_paths is mostly redundant with the hunk in free_pgvec(), but because we're looping over pg->paths in the former and over pg->pgp in the latter, I think it's better too play safe. Fixes: 99db1bd ("[multipathd] re-enable disabled PG when at least one path is up") Fixes: opensvc#105 Signed-off-by: Martin Wilck <[email protected]>
|
Yes. @bmarzins pointed out the same thing on dm-devel. I have posted another patch series to the dm-devel mailing list ("[PATCH v2 0/8] multipath-tools fixes") with an updated fix that should cover your case. The set is also on my tip branch. |
You (or we) should investigate how it comes to pass that this path with a wrong WWID is in this map. The log history should provide some clue about it. multipathd tries to work around this, but it represents some rather evil problem that has probably external reasons (a path has changed its WWID without being deleted and re-added). |
Okay. |
|
What storage type is it? You can't just have unmapped the LUN, you must have unmapped and re-mapped it, otherwise it wouldn't show up in the host any more, or am I missing something? Anyway, testing the current patch set is more important now. |
And there are no uevents on the host when this happens? |
Yes, the storage LUN is not mounted, but it is mapped to the host. I modified the LUN ID on the storage side, and then executed the command: udevadm monitor --kernel --property, but there were no uevent events. The storage is HUAWEI XSG1. |
|
Are there any kernel messages about changed LUNs or the like? It is highly dangerous to swap a SCSI device in this way while a host is accessing it. It it isn't mounted, there's no immediate threat of data corruption, but still, I would strongly discourage doing it. |
|
Yes, this issue occurred when I modified the LUN ID or umap LUN and rollback at that time. If the kernel issues the scsi command(tur) to the storage while the LUN ID is being modified, the kernel can receive the return code from the storage and detect that the LUN data has changed. In this scenario, a uevent will be triggered, but this depends on probability. I tested many times, and I only occasionally received the uevent event; in most cases, I did not receive it. |
|
Ok, I understand. Do you still observe the crash with the current patch set? |
Ok. I am testing. I have a small question: if this issue occurs and the pgindex is set to invalid, where can we ensure that the reload map will be triggered? |
update_pathvec_from_dm() may set mpp->need_reload if it finds inconsistent settings. In this case, the map should be reloaded, but so far we don't do this reliably. Add a call to reload_and_sync_map() to do_sync_mpp() to clear this kind of inconsistency. In order to avoid endless reload loops, limit the number of retries to 1. Fixes: opensvc#105 Signed-off-by: Martin Wilck <[email protected]>
That question isn't small. We currently don't. It's a subtle matter because we must avoid spurious map reloads, and endless reload loops. The difficult part is where we can safely reload the map. I've double-checked, and I think that I have just pushed another commit (ab60145) to my "tip" branch. I'm curious to see if it works for your case (i.e. causes a map reload for the broken map). @bmarzins, your opinion about that commit would also be highly appreciated, as you've made lots of changes around the checkerloop recently. |
|
Do you observe kernel messages like this? Also, could you run |
|
Note that the fact that multipathd receives no notifications about SCSI Unit Attention (UA)events is a long-standing problem. We have missing links un multiple levels here. |
|
@linzhanglong, can you describe your test procedure in detail? You change a LUN assignment on the storage side, and then what do you do on the host side? |
Hello, I just tested a single LUN, and the reason I previously mentioned not receiving the uevent event was because I was filtering with sdx. In fact, the uevent events were received, and they are as follows: multipathd.log, 36b46e08100526565029487c700000108 is the WWID that the LUN changes to after I unmap it. |
|
I retested by unmapping a LUN, and this is the uevent event for the unmapping. My previous environment had 256 LUNs.
1.1 before unmap LUN: 1.2 after unmap LUN: 1.3 remap LUN to host
2.1 change back |
|
I will merge the patch later today and test it in the environment with 256 LUNs. Looking at the code can solve the problem I noticed that there are related operations for reload_and_sync_map at the end of the do_check_path function. Would it be more appropriate to reload the map there? |
pp->pgindex is set in disassemble_map() when a map is parsed. There are various possiblities for this index to become invalid. pp->pgindex is only used in enable_group() and followover_should_fallback(), and both callers take no action if it is 0, which is the right thing to do if we don't know the path's pathgroup. Make sure pp->pgindex is reset to 0 in various places: - when it's orphaned, - before (re)grouping paths, - when we detect a bad mpp assignment in update_pathvec_from_dm(). - when a pathgroup is deleted in update_pathvec_from_dm(). In this case, pgindex needs to be invalidated for all paths in all pathgroups after the one that was deleted. The hunk in group_paths is mostly redundant with the hunk in free_pgvec(), but because we're looping over pg->paths in the former and over pg->pgp in the latter, I think it's better too play safe. Fixes: 99db1bd ("[multipathd] re-enable disabled PG when at least one path is up") Fixes: opensvc#105 Signed-off-by: Martin Wilck <[email protected]>
update_pathvec_from_dm() may set mpp->need_reload if it finds inconsistent settings. In this case, the map should be reloaded, but so far we don't do this reliably. Add a call to reload_and_sync_map() to do_sync_mpp() to clear this kind of inconsistency. In order to avoid endless reload loops, limit the number of retries to 1. Fixes: opensvc#105 Signed-off-by: Martin Wilck <[email protected]>
pp->pgindex is set in disassemble_map() when a map is parsed. There are various possiblities for this index to become invalid. pp->pgindex is only used in enable_group() and followover_should_fallback(), and both callers take no action if it is 0, which is the right thing to do if we don't know the path's pathgroup. Make sure pp->pgindex is reset to 0 in various places: - when it's orphaned, - before (re)grouping paths, - when we detect a bad mpp assignment in update_pathvec_from_dm(). - when a pathgroup is deleted in update_pathvec_from_dm(). In this case, pgindex needs to be invalidated for all paths in all pathgroups after the one that was deleted. The hunk in group_paths is mostly redundant with the hunk in free_pgvec(), but because we're looping over pg->paths in the former and over pg->pgp in the latter, I think it's better too play safe. Fixes: 99db1bd ("[multipathd] re-enable disabled PG when at least one path is up") Fixes: opensvc#105 Signed-off-by: Martin Wilck <[email protected]> Reviewed-by: Benjamin Marzinski <[email protected]>
update_pathvec_from_dm() may set mpp->need_reload if it finds inconsistent settings. In this case, the map should be reloaded, but so far we don't do this reliably. Add a call to reload_and_sync_map() to do_sync_mpp() to clear this kind of inconsistency. In order to avoid endless reload loops, limit the number of retries to 1. Fixes: opensvc#105 Signed-off-by: Martin Wilck <[email protected]>
The However, I think now that calling Footnotes
|
update_pathvec_from_dm() may set mpp->need_reload if it finds inconsistent settings. In this case, the map should be reloaded, but so far we don't do this reliably. Add a call to reload_map() to do_sync_mpp() to clear this kind of inconsistency. In order to avoid endless reload loops, limit the number of retries to 1. Fixes: opensvc#105 Signed-off-by: Martin Wilck <[email protected]>
Indeed, this was wrong. I've pushed a new commit, 01ec4fa @bmarzins, your feedback would be appreciated. |
update_pathvec_from_dm() may set mpp->need_reload if it finds inconsistent settings. In this case, the map should be reloaded, but so far we don't do this reliably. Add a call to reload_map() to do_sync_mpp() to clear this kind of inconsistency. In order to avoid endless reload loops, limit the number of retries to 1. Fixes: opensvc#105 Signed-off-by: Martin Wilck <[email protected]>
|
Sorry for posting the wrong commit. 01ec4fa is correct. |
Okay, I will test the new patch. |
|
@mwilck, Is there a big benefit to retrying immediately that I'm overlooking? Is this to deal with the case where reload_map() fails, or are you worried about successfully reloading the map, but having update_multipath_strings() still flag it as needing to be reloaded again? I seems to me that instead of immediately retrying, we could just wait till the next path check to try again. Also, I think we do want to call reload_and_sync_map(). Right now, every call to domap() in multipathd will call setup_multipath() and sync_map_state() afterwards. I think we want to keep that for all cases. I posted a patchset that contains an alternate version of this patch. |
|
In my mind, |
|
@linzhanglong, the patches are available here: |
Oops. I misread your code. I though that you did one retry reload_map() immediately, buy you just loop to call update_multipath_strings() again. I still think that reload_and_sync_map() makes more sense, but you can ignore my retrys question. My code should do the reload_and_sync_map() as often as yours. I just moved it after the prio refresh so that if we're going to do a reload because of that anyways, we won't reload the device twice (unless the device gets set to need_reload when we are syncing after the prio changed reload). In my version of the patch, perhaps we could store a flag when That would make multipathd respond to a new |
|
I'd also prefer to do this kind of reload no more than once per tick. The idea of the retry was to check if update_pathvec_from_dm() still reports an inconsistency (which includes the case in which the reload failed, because in that case we'd have failed to fix the situation). I thought one immediate retry was warranted, given that this is a rare but serious error condition. But I've no idea about the likelihood that such an immediate retry would succeed. I'll respond in the dm-devel thread. |
|
Another thought: we could attempt a single reload in |
But your code checks |
|
Right, now I misinterpreted my own code :-) Indeed the idea was just to retry reading the kernel parameters. I didn't intend to reload multiple times. |
pp->pgindex is set in disassemble_map() when a map is parsed. There are various possiblities for this index to become invalid. pp->pgindex is only used in enable_group() and followover_should_fallback(), and both callers take no action if it is 0, which is the right thing to do if we don't know the path's pathgroup. Make sure pp->pgindex is reset to 0 in various places: - when it's orphaned, - before (re)grouping paths, - when we detect a bad mpp assignment in update_pathvec_from_dm(). - when a pathgroup is deleted in update_pathvec_from_dm(). In this case, pgindex needs to be invalidated for all paths in all pathgroups after the one that was deleted. The hunk in group_paths is mostly redundant with the hunk in free_pgvec(), but because we're looping over pg->paths in the former and over pg->pgp in the latter, I think it's better too play safe. Fixes: 99db1bd ("[multipathd] re-enable disabled PG when at least one path is up") Fixes: opensvc#105 Signed-off-by: Martin Wilck <[email protected]> Reviewed-by: Benjamin Marzinski <[email protected]>
update_pathvec_from_dm() may set mpp->need_reload if it finds inconsistent settings. In this case, the map should be reloaded, but so far we don't do this reliably. A previous patch added a call to reload_and_sync_map() in the CHECKER_FINISHED state, but in the mean time the checker may have waited for checker threads to finish, and may have dropped and re-acquired the vecs lock. As mpp->need_reload is a serious but rare condition, also try to fix it early in the checker loop. Because of the previous patch, we can call reload_and_sync_map() here. Fixes: opensvc#105 Signed-off-by: Martin Wilck <[email protected]>
pp->pgindex is set in disassemble_map() when a map is parsed. There are various possiblities for this index to become invalid. pp->pgindex is only used in enable_group() and followover_should_fallback(), and both callers take no action if it is 0, which is the right thing to do if we don't know the path's pathgroup. Make sure pp->pgindex is reset to 0 in various places: - when it's orphaned, - before (re)grouping paths, - when we detect a bad mpp assignment in update_pathvec_from_dm(). - when a pathgroup is deleted in update_pathvec_from_dm(). In this case, pgindex needs to be invalidated for all paths in all pathgroups after the one that was deleted. The hunk in group_paths is mostly redundant with the hunk in free_pgvec(), but because we're looping over pg->paths in the former and over pg->pgp in the latter, I think it's better too play safe. Fixes: 99db1bd ("[multipathd] re-enable disabled PG when at least one path is up") Fixes: opensvc#105 Signed-off-by: Martin Wilck <[email protected]> Reviewed-by: Benjamin Marzinski <[email protected]>
update_pathvec_from_dm() may set mpp->need_reload if it finds inconsistent settings. In this case, the map should be reloaded, but so far we don't do this reliably. A previous patch added a call to reload_and_sync_map() in the CHECKER_FINISHED state, but in the mean time the checker may have waited for checker threads to finish, and may have dropped and re-acquired the vecs lock. As mpp->need_reload is a serious but rare condition, also try to fix it early in the checker loop. Because of the previous patch, we can call reload_and_sync_map() here. Fixes: opensvc#105 Signed-off-by: Martin Wilck <[email protected]>
|
My tip branch now contains Ben's fixes plus an improved version of mine above, plus some cleanup. @linzhanglong, Please provide feedback. |
I have merged the changes from this patch into the test environment without any issues. If there are any new changes, I will merge and test them today. |
|
Test Okay |
|
Hello, when will the new version be released? Is the branch at https://github.com/openSUSE/multipath-tools/tree/tip the next version? |
Hello. In the do_check_path function, the following code:
should be changed to ?
Otherwise, the subsequent code in the enable_group function may access the pgindex array out of bounds:
The text was updated successfully, but these errors were encountered: