On Wed, Apr 30, 2025 at 09:55:43AM +0200, Martin Wilck wrote:
> On Tue, 2025-04-29 at 16:27 -0400, Benjamin Marzinski wrote:
> > On Tue, Apr 29, 2025 at 09:59:40PM +0200, Martin Wilck wrote:
> > > On Mon, 2025-04-28 at 17:45 -0400, Benjamin Marzinski wrote:
> > > > If the multipath configuration is changed to blacklist existing
> > > > devices,
> > > > and multipathd is reloaded but the blacklisted multipaths device
> > > > can't
> > > > be removed, multipathd was marking the paths as INIT_PARTIAL,
> > > > causing
> > > > them to stay in the multipath device, at least until the
> > > > partial_retrigger_delay timeout elapsed. Instead, mark them as
> > > > INIT_REMOVED and set mpp->need_reload, so the device is reloaded
> > > > and
> > > > the
> > > > paths are removed. To make sure the blacklisted paths are deleted
> > > > when
> > > > the multipath device is removed in coalesce_maps(), set their pp-
> > > > >mpp
> > > > to point to map before removing it.
> > > >
> > > > Fixes d9c61332 ("multipathd: trigger uevents for blacklisted
> > > > paths in
> > > > reconfigure")
> > >
> > > The patch looks good to me, but I only vaguely understand why the
> > > bug
> > > is introduced in d9c61332. Are you positive that before d9c61332,
> > > the
> > > hang didn't occur?
> >
> > Well, I'm pretty sure these blacklisted paths stopped getting deleted
> > during reconfigure by d9c61332. Before d9c61332, blacklisted paths
> > were
> > immediately deleted in update_pathvec_from_dm().
>
> Right, thanks.
>
> I've taken the liberty to fix the typo ("libmutipath") and make some
> minor coding style adjustments in my queue branch. You can inspect the
> result there.
>
> The coding style stuff is work in progress, I've added a clang-format
> based workflow on my tip branch.
>
> I'd like to avoid major reformatting of past code while enforcing
> consistent formatting for present and future submissions which is more-
> or-less in line with what we used to do "sub-conciously" during the
> last years. clang-format has a lot of options [1]. I'm still struggling
> to find style settings that match ours [2].
>
> Let me know if you dislike this, or if you have suggestions for
> improving the settings.
I'm fine with your changes and with using clang-format in general. And I
do prefer just formatting our new changes, like you mentioned in your
tip commit, but if someone wants to make the case for a mass reformat,
I'm willing to consider it.
-Ben
>
> Regards
> Martin
>
> [1] https://clang.llvm.org/docs/ClangFormatStyleOptions.html
> [2]
> https://github.com/openSUSE/multipath-tools/blob/refs/heads/tip/.clang-format
>
> > After this change
> >
> > @@ -193,7 +196,8 @@ static void update_pathvec_from_dm(vector
> > pathvec, struct multipath *mpp,
> > rc = pathinfo(pp, conf,
> >
> > DI_SYSFS|DI_WWID|DI_BLACKLIST|DI_NOFALLBACK|pathinfo_flags);
> > pthread_cleanup_pop(1);
> > - if (rc != PATHINFO_OK) {
> > + if (rc == PATHINFO_FAILED ||
> > + (rc == PATHINFO_SKIPPED && !map_discovery)) {
> > condlog(1, "%s: error %d in pathinfo,
> > discarding path",
> > pp->dev, rc);
> > vector_del_slot(pgp->paths, j--);
> >
> > they started hanging around, so that uevents could be generated for
> > them
> > by trigger_paths_udev_change(). However, since coalesce_paths() will
> > usually clear pp->mpp, they won't get removed when orphan_paths() is
> > later called by remove_maps() to remove the old maps. I admit I
> > didn't
> > test if the paths got removed before that commit, but the commit
> > message
> > says that they did.
> >
> > -Ben
> >
> > >
> > > >
> > > > Signed-off-by: Benjamin Marzinski <bmarz...@redhat.com>
> > >
> > > Reviewed-by: Martin Wilck <mwi...@suse.com>
