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).
> } 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.
Use an enum, maybe?
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
