On Tue, Feb 26, 2019 at 11:47:29AM +0100, Martin Wilck wrote:
> On Fri, 2019-02-22 at 10:58 -0600, Benjamin Marzinski wrote:
> > reload_map() doesn't do the work to sync the state after reloading
> > the
> > map. Instead of calling it directly, cli_reload() and
> > uev_update_path()
> > should call update_path_groups(), which calls reload_map() with all
> > the
> > necessary syncing.
> >
> > Signed-off-by: Benjamin Marzinski <bmarzins@xxxxxxxxxx>
> > ---
> > multipathd/cli_handlers.c | 2 +-
> > multipathd/main.c | 13 ++++++++-----
> > multipathd/main.h | 2 ++
> > 3 files changed, 11 insertions(+), 6 deletions(-)
>
> I can see that this makes some sense for the cli_reload() path (see
> below). But for uev_update_path(), I'm not sure. This is the code path
> where a paths's "ro" attribute changes. With this change, you'll call
> update_multipath_strings() for every uevent that arrives, causing a lot
> of extra work on every uevent. This may hurt, in particular if lots of
> uevents arrive at the same time. I fail to see what you gain by doing
> the extra work. If path states change, either dm events or uevents for
> the other paths in the map will likely arrive soon and cause the other
> path's states to be fixed, or check_path() will detect the state
> changes and fix the DM states eventually. No?
Fair enough. The one issue is that whenever we reload a map, all path
states in the kernel are reset to active. We need to call
sync_map_state() to reset them to the correct state. Obviously, an
dm event will come in soonish, but if there is a backlog of stuff for
multipathd to do, those down paths might be reactivated for a while
before we get that. And if the kernel uses them, it will fail them and
generate even more events for multipathd to deal with.
Also, we wouldn't call update_path_groups() in uev_update_path() very
often. Every time that we do, you will see this message
condlog(2, "%s: update path write_protect to '%d' (uevent)",
uev->kernel, ro);
That certainly doesn't happen on every change event for a path. right?
> For cli_reload() it'd actually be the question if we should re-
> calculate priorities and path groups in multipathd before calling
> update_path_groups() / reload_map().
Sure. This isn't a horribly important patch. It isn't fixing any issue
that I've seen. I just noticed that these are the only code paths I
could see were we reload the map, but don't call setup_multipath() and
sync_map_state(), to make sure all the states are in sync.
> Regards,
> Martin
>
>
>
> >
> > diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c
> > index f95813e..60e17d6 100644
> > --- a/multipathd/cli_handlers.c
> > +++ b/multipathd/cli_handlers.c
> > @@ -877,7 +877,7 @@ cli_reload(void *v, char **reply, int *len, void
> > *data)
> > return 1;
> > }
> >
> > - return reload_map(vecs, mpp, 0, 1);
> > + return update_path_groups(mpp, vecs, 0);
> > }
> >
> > int resize_map(struct multipath *mpp, unsigned long long size,
> > diff --git a/multipathd/main.c b/multipathd/main.c
> > index 81ad6c0..27ff186 100644
> > --- a/multipathd/main.c
> > +++ b/multipathd/main.c
> > @@ -1272,10 +1272,13 @@ uev_update_path (struct uevent *uev, struct
> > vectors * vecs)
> > else {
> > if (ro == 1)
> > pp->mpp->force_readonly = 1;
> > - retval = reload_map(vecs, mpp, 0, 1);
> > - pp->mpp->force_readonly = 0;
> > - condlog(2, "%s: map %s reloaded (retval
> > %d)",
> > - uev->kernel, mpp->alias,
> > retval);
> > + retval = update_path_groups(mpp, vecs,
> > 0);
> > + if (retval == 2)
> > + condlog(2, "%s: map removed
> > during reload", pp->dev);
> > + else {
> > + pp->mpp->force_readonly = 0;
> > + condlog(2, "%s: map %s reloaded
> > (retval %d)", uev->kernel, mpp->alias, retval);
> > + }
> > }
> > }
> > }
> > @@ -1831,7 +1834,7 @@ int update_path_groups(struct multipath *mpp,
> > struct vectors *vecs, int refresh)
> >
> > dm_lib_release();
> > if (setup_multipath(vecs, mpp) != 0)
> > - return 1;
> > + return 2;
> > sync_map_state(mpp);
> >
> > return 0;
> > diff --git a/multipathd/main.h b/multipathd/main.h
> > index 8fd426b..e5c1398 100644
> > --- a/multipathd/main.h
> > +++ b/multipathd/main.h
> > @@ -43,5 +43,7 @@ int __setup_multipath (struct vectors * vecs,
> > struct multipath * mpp,
> > int reset);
> > #define setup_multipath(vecs, mpp) __setup_multipath(vecs, mpp, 1)
> > int update_multipath (struct vectors *vecs, char *mapname, int
> > reset);
> > +int update_path_groups(struct multipath *mpp, struct vectors *vecs,
> > + int refresh);
> >
> > #endif /* MAIN_H */
>
--
dm-devel mailing list
dm-devel@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/dm-devel
