On Thu, May 13, 2021 at 08:11:13PM +0000, Martin Wilck wrote:
> On Thu, 2021-05-13 at 12:23 -0500, Benjamin Marzinski wrote:
> > There are many possible outcomes of calling ev_remove_path(), and not
> > all callers agree on which outcomes are a success and which are a
> > failure. So ev_remove_path() should simply return a different value
> > for
> > each outcome, and the callers can decide how to deal with them.
> >
> > Signed-off-by: Benjamin Marzinski <bmarzins@xxxxxxxxxx>
> > ---
> > multipathd/cli_handlers.c | 14 ++++++++++++--
> > multipathd/main.c | 35 +++++++++++++++++++----------------
> > multipathd/main.h | 9 +++++++++
> > 3 files changed, 40 insertions(+), 18 deletions(-)
> >
> > diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c
> > index 1de6ad8e..1462ea84 100644
> > --- a/multipathd/cli_handlers.c
> > +++ b/multipathd/cli_handlers.c
> > @@ -752,7 +752,8 @@ cli_add_path (void * v, char ** reply, int * len,
> > void * data)
> > /* Have the checker reinstate this
> > path asap */
> > pp->tick = 1;
> > return 0;
> > - } else if (!ev_remove_path(pp, vecs, true))
> > + } else if (ev_remove_path(pp, vecs, true) &
> > + REMOVE_PATH_SUCCESS)
> > /* Path removed in ev_remove_path()
> > */
> > pp = NULL;
> > else {
> > @@ -813,6 +814,7 @@ cli_del_path (void * v, char ** reply, int * len,
> > void * data)
> > struct vectors * vecs = (struct vectors *)data;
> > char * param = get_keyparam(v, PATH);
> > struct path *pp;
> > + int ret;
> >
> > param = convert_dev(param, 1);
> > condlog(2, "%s: remove path (operator)", param);
> > @@ -821,7 +823,15 @@ cli_del_path (void * v, char ** reply, int *
> > len, void * data)
> > condlog(0, "%s: path already removed", param);
> > return 1;
> > }
> > - return ev_remove_path(pp, vecs, 1);
> > + ret = ev_remove_path(pp, vecs, 1);
> > + if (ret == REMOVE_PATH_DELAY) {
> > + *reply = strdup("delayed\n");
> > + *len = strlen(*reply) + 1;
> > + } else if (ret == REMOVE_PATH_MAP_ERROR) {
> > + *reply = strdup("map reload error. removed\n");
> > + *len = strlen(*reply) + 1;
> > + }
> > + return (ret == REMOVE_PATH_FAILURE);
> > }
> >
> > int
> > diff --git a/multipathd/main.c b/multipathd/main.c
> > index 4bdf14bd..72fb7e38 100644
> > --- a/multipathd/main.c
> > +++ b/multipathd/main.c
> > @@ -838,7 +838,7 @@ handle_path_wwid_change(struct path *pp, struct
> > vectors *vecs)
> > return;
> >
> > udd = udev_device_ref(pp->udev);
> > - if (ev_remove_path(pp, vecs, 1) != 0 && pp->mpp) {
> > + if (!(ev_remove_path(pp, vecs, 1) & REMOVE_PATH_SUCCESS) &&
> > pp->mpp) {
> > pp->dmstate = PSTATE_FAILED;
> > dm_fail_path(pp->mpp->alias, pp->dev_t);
> > }
> > @@ -948,8 +948,8 @@ uev_add_path (struct uevent *uev, struct vectors
> > * vecs, int need_do_map)
> > * Make another attempt to remove the
> > path
> > */
> > pp->mpp = prev_mpp;
> > - ret = ev_remove_path(pp, vecs, true);
> > - if (ret != 0) {
> > + if (!(ev_remove_path(pp, vecs, true)
> > &
> > + REMOVE_PATH_SUCCESS)) {
> > /*
> > * Failure in ev_remove_path
> > will keep
> > * path in pathvec in
> > INIT_REMOVED state
> > @@ -960,6 +960,7 @@ uev_add_path (struct uevent *uev, struct vectors
> > * vecs, int need_do_map)
> > dm_fail_path(pp->mpp->alias,
> > pp->dev_t);
> > condlog(1, "%s: failed to re-
> > add path still mapped in %s",
> > pp->dev, pp->mpp-
> > >alias);
> > + ret = 1;
> > } else if (r == PATHINFO_OK)
> > /*
> > * Path successfully freed,
> > move on to
> > @@ -1167,7 +1168,7 @@ static int
> > uev_remove_path (struct uevent *uev, struct vectors * vecs, int
> > need_do_map)
> > {
> > struct path *pp;
> > - int ret;
> > + int ret = 0;
> >
> > condlog(3, "%s: remove path (uevent)", uev->kernel);
> > delete_foreign(uev->udev);
> > @@ -1176,8 +1177,8 @@ uev_remove_path (struct uevent *uev, struct
> > vectors * vecs, int need_do_map)
> > lock(&vecs->lock);
> > pthread_testcancel();
> > pp = find_path_by_dev(vecs->pathvec, uev->kernel);
> > - if (pp)
> > - ret = ev_remove_path(pp, vecs, need_do_map);
> > + if (pp && ev_remove_path(pp, vecs, need_do_map) ==
> > REMOVE_PATH_FAILURE)
> > + ret = 1;
> > lock_cleanup_pop(vecs->lock);
> > if (!pp) {
> > /* Not an error; path might have been purged earlier
> > */
> > @@ -1191,7 +1192,7 @@ int
> > ev_remove_path (struct path *pp, struct vectors * vecs, int
> > need_do_map)
> > {
> > struct multipath * mpp;
> > - int i, retval = 0;
> > + int i, retval = REMOVE_PATH_SUCCESS;
> > char params[PARAMS_SIZE] = {0};
> >
> > /*
> > @@ -1245,7 +1246,6 @@ ev_remove_path (struct path *pp, struct vectors
> > * vecs, int need_do_map)
> > condlog(2, "%s: removed map after"
> > " removing all paths",
> > alias);
> > - retval = 0;
> > /* flush_map() has freed the path */
> > goto out;
> > }
> > @@ -1262,11 +1262,14 @@ ev_remove_path (struct path *pp, struct
> > vectors * vecs, int need_do_map)
> >
> > if (mpp->wait_for_udev) {
> > mpp->wait_for_udev = 2;
> > + retval = REMOVE_PATH_DELAY;
> > goto out;
> > }
> >
> > - if (!need_do_map)
> > + if (!need_do_map) {
> > + retval = REMOVE_PATH_DELAY;
> > goto out;
> > + }
> > /*
> > * reload the map
> > */
> > @@ -1275,7 +1278,7 @@ ev_remove_path (struct path *pp, struct vectors
> > * vecs, int need_do_map)
> > condlog(0, "%s: failed in domap for "
> > "removal of path %s",
> > mpp->alias, pp->dev);
> > - retval = 1;
> > + retval = REMOVE_PATH_FAILURE;
>
> Hm. With the introduction of INIT_REMOVED, this failure isn't fatal any
> more. As far as multipathd is concerned, the path is removed and will
> be deleted from the map as soon as the reason for the domap() failure
> (likely a problem with some other device in the map) is resolved.
> There's no difference from the REMOVE_PATH_DELAY case wrt how the path
> will be treated in the future.
>
> So while I agree it's reasonable to distinguish this case from the
> "delay without failure" cases above, I'm unsure if we should treat it
> as an error in uev_remove_path() (or uev_trigger(), for that matter).
Sure. All that a failure does is print an error message anyway, and a
message already gets printed if domap fails, so another message won't
help with debugging problems.
>
> > } else {
> > /*
> > * update our state from kernel
> > @@ -1283,12 +1286,12 @@ ev_remove_path (struct path *pp, struct
> > vectors * vecs, int need_do_map)
> > char devt[BLK_DEV_SIZE];
> >
> > strlcpy(devt, pp->dev_t, sizeof(devt));
> > +
> > + /* setup_multipath will free the path
> > + * regardless of whether it succeeds or
> > + * fails */
> > if (setup_multipath(vecs, mpp))
> > - return 0;
> > - /*
> > - * Successful map reload without this path:
> > - * sync_map_state() will free it.
> > - */
> > + return REMOVE_PATH_MAP_ERROR;
> > sync_map_state(mpp);
> >
> > condlog(2, "%s: path removed from map %s",
> > @@ -1307,7 +1310,7 @@ fail:
> > condlog(0, "%s: error removing path. removing map %s", pp-
> > >dev,
> > mpp->alias);
> > remove_map_and_stop_waiter(mpp, vecs);
> > - return 0;
> > + return REMOVE_PATH_MAP_ERROR;
> > }
> >
> > static int
> > diff --git a/multipathd/main.h b/multipathd/main.h
> > index ddd953f9..24d050c8 100644
> > --- a/multipathd/main.h
> > +++ b/multipathd/main.h
> > @@ -13,6 +13,15 @@ enum daemon_status {
> > DAEMON_STATUS_SIZE,
> > };
> >
> > +#define REMOVE_PATH_FAILURE 0x0 /* path was not removed */
>
> We should add a remark that this is normally non-fatal, it's init state
> is set to INIT_REMOVED, and that it will be removed at the next
> possible occasion. The only thing that should be avoided is to try and
> add another path with the same major/minor number.
Sure.
> Use an enum, maybe?
I can do that.
-Ben
> Regards,
> Martin
>
>
> > +#define REMOVE_PATH_SUCCESS 0x1 /* path was removed */
> > +#define REMOVE_PATH_DELAY 0x2 /* path is set to be removed later. it
> > + * currently still exists and is part
> > of the
> > + * kernel map */
> > +#define REMOVE_PATH_MAP_ERROR 0x5 /* map was removed because of
> > error. value
> > + * includes REMOVE_PATH_SUCCESS bit
> > + * because the path was also
> > removed */
>
>
>
> > +
> > struct prout_param_descriptor;
> > struct prin_resp;
> >
--
dm-devel mailing list
dm-devel@xxxxxxxxxx
https://listman.redhat.com/mailman/listinfo/dm-devel
