On Mon, 2025-01-20 at 18:51 -0500, Benjamin Marzinski wrote: > On Fri, Jan 17, 2025 at 09:27:38PM +0100, Martin Wilck wrote: > > multipathd calls enable_group() from update_path_state() if a path > > in a > > previously disabled pathgroup is reinstated. This call may be > > mistakenly > > skipped if the path group status wasn't up-to-date while > > update_path_state() > > was executed. This can happen after applying the previous patch > > "multipathd: > > sync maps at end of checkerloop", if the kernel has disabled the > > group during > > the last checker interval. > > > > Therefore add another check in checker_finished() after calling > > sync_mpp(), > > and enable groups if necessary. This step can be skipped if the map > > was > > reloaded, because after a reload, all pathgroups are enabled by > > default. > > The logic running in checker_finished() isn't the same as the logic > running in update_path_state(). It update_path_state(), we only > enable a > pathgroup when a path switches to PATH_UP. In checker_finished(), we > enable it whenever a path is in PATH_UP. I worry that this might not > always be correct. Perhaps instead of just checking if the path is in > PATH_UP, we should also make sure that pp->is_checked isn't in > CHECK_PATH_SKIPPED, which would mean that the checker is pending or > messed up, and the path is using its old state. This still assumes > that > the path switched to some other state before the pathgroup got > delayed > in re-initializing again, but that seems pretty safe. I just don't > want > to go re-enabling pathgroups where the controller is actually busy, > since I'm pretty sure that the kernel usually does the right thing > without multipathd's help. OK. I'll see if I can add a test for the checker state. > Alternatively, we could make the check_finished() logic match the > update_path_state() logic exactly by just setting a flag in the > path's > pathgroup during enable_group() (and possibly not call > dm_enablegroup() > at all, That was my first thought, but it doesn't work, because we call free_pgvec(mpp->pg, KEEP_PATHS) update_multipath_strings(), and reallocate the vector in disassemble_map. I guess we could change that, but it would be another intrusive patch set. Currently we just can't store any persistent properties in struct pathgroup. > but I suppose that there could be a benefit to re-enabling the > group as soon as possible. I'm still kinda fuzzy on whether the > kernel's > own pathgroup re-enabling code makes all this redundant). The kernel will even try "bypassed" PGs if it finds no other. But that means that e.g. the marginal pathgroup would be tried before disabled PGs [1]. In general, I don't think we can rely on the kernel in this respect. > Then, in > enable_pathrgoups(), instead of checking each path, we just need to > check if pgp->need_reenable is set for the pathgroup(). As I said, as long as we discard the PGs, I guess we'll have to test the path flags. It's difficult to come up with a decent test case for this, in particular if checking for PATH_UP is not enough... > [...] > > But perhaps the best answer is to just to say that this is a corner > case > where we skip a multipathd action that might be totally unnecessary. > And > even if it is a problem, it will be fixed if multipathd ever decides > that the kernel is using the wrong pathgroup and should switch, or > whenever the table gets reloaded. Maybe the whole patch is > unnecessary. I'm going to submit a v4 that adds a test for the checker flags. Btw, I could only find one place in the kernel code where the kernel actively sets the "bypassed" flag, when the SCSI device handler returns SCSL_DH_DEV_TEMP_BUSY [2]. This seems rather questionable to me. First of all, this seems to assume that PGs are mapped to "controllers", i.e. something like "group_by_tpg", which isn't necessarily the case. Second, while the EMC device handler uses this error code in this way, in the ALUA handler it rather means a memory allocation failure, and the other device handlers don't support it at all. I think we can conclude that the kernel setting "bypassed" on a path group by itself is indeed a corner case, except for EMC Clariion, which is quite dated by now. > Thoughts? I'm clearly thinking too much. > No, you are not. I strongly appreciate these comments. Regards, Martin [1] Unrelated wild thought: in view of the logic the kernel applies for "bypassed" PGs, it might actually make sense to use this flag for marginal pathgroups. They would be tried if all else fails, which is more or less the behavior we want for them. What do you think? [2] https://elixir.bootlin.com/linux/v6.12.6/source/drivers/md/dm-mpath.c#L1564
