Re: [dm-devel] [PATCH v2 6/6] multipathd: use symbolic returns for ev_remove_path()

[Benjamin Marzinski <bmarzins@redhat.com>]

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@redhat.com>
> > ---
> >  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@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel