On Tue, Jan 17, 2017 at 03:28:06PM +0800, tang.junhui@xxxxxxxxxx wrote:
> Hello Ben
>
> Thank you for your patience again.
>
> I'll modify code according to your suggestion as this:
> 1) add configuration in the defaults section
> uid_attrs "sd:ID_SERIAL dasd:ID_UID"
> it would override any other configurations if this
> filed is configured and matched;
>
> 2) In uevent processing thread, we will assign merge_id
> according the label in uevents by this configuration;
>
> 3) this patch will take back:
> [PATCH 12/12] libmultipath: use existing wwid when
> wwid has already been existed in uevent
>
> 4) if this field is not configured, only do filtering and
> no merging works.
>
> Please confirm whether this modification is feasible.
Yes. This is perfectly reasonable solution. Thanks for doing all the
work on this.
-Ben
>
> Regards,
> Tang Junhui
>
> ������: "Benjamin Marzinski" <bmarzins@xxxxxxxxxx>
> �ռ���: tang.junhui@xxxxxxxxxx,
> ����: christophe.varoqui@xxxxxxxxxxx, hare@xxxxxxx,
> mwilck@xxxxxxxx, bart.vanassche@xxxxxxxxxxx, dm-devel@xxxxxxxxxx,
> zhang.kai16@xxxxxxxxxx, tang.wenjun3@xxxxxxxxxx
> ����: 2017/01/17 05:38
> ����: Re: [PATCH 04/11] multipathd: add need_do_map to indicate
> whether need calling domap() in ev_add_path()
>
> --------------------------------------------------------------------------
>
> On Thu, Jan 12, 2017 at 01:52:20PM +0800, tang.junhui@xxxxxxxxxx wrote:
> > From: tang.junhui <tang.junhui@xxxxxxxxxx>
> >
> > Usually calling domap() in ev_add_path() is needed, but only last path
> > need to call domap() in processing for merged uevents to reduce the
> > count of calling domap() and promote efficiency. So add input parameter
> > need_do_map to indicate whether need calling domap() in ev_add_path().
>
> With the addition of checking if the merge_id equals the wwid, you are
> protected against accidentially merging paths that shouldn't be merged,
> which is great. But setting need_do_map for these paths doesn't
> completely make sure that if the wwid and merge_id differ, you will
> always call domap.
>
> A contrived situation where this fails would look like:
>
> add path1, path2, path3
>
> where merge_id equals the wwid for path1 and path2, but there is a
> different wwid for path3. In this case, you would call domap on just
> the multipath device for path3, but since path1 and path2 matched the
> merge_id, they wouldn't force a call to domap.
>
> A less contrived example would be
>
> add path1, path2, path3, path4
>
> Where these were devices that were actually pointing at two different
> LUNs, but all set ID_SERIAL the same. path1 and path2 pointed to one
> LUN, while path3 and path4 pointed to another LUN. In this case the
> wwid of path1 and path2 matched the merge_id, while the wwid of path3
> and path4 was different. In this case you would call domap twice, on
> both path3 and path4, but nothing would call domap to create a multipath
> device for path1 and path2.
>
> In general, the code to set need_do_map if the wwid and merge_id don't
> match only works if either none of the device in a merged set have wwids
> that match the merge_id, or if the last device has a wwid that matches
> the merge_id. If there are devices with wwids that match the merge_id,
> but the last device in the set isn't one of them, then some devices will
> not get a multipath device created for them.
>
> Now, I don't know of any device that works like my above example, so
> your code will likely work fine for all real-world devices. Also,
> fixing this is a pain, as you don't find out until processing the last
> path in a set that things went wrong, and then you would have to go back
> and run the skipped functions on one of the paths you have already
> processed.
>
> The easy way to fix it is to use the other possibility that I mentioned
> before, which is to not have the merge_id, and just use the udev
> provided wwid, instead of fetching it from pathinfo. Like I mentioned,
> if you do this, you want to make sure that you only try to grab the wwid
> from the udev events for devices with the correct kernel name: ID_SERIAL
> only for "sd.*" devices, and ID_UID only for "dasd.*" devices. I also
> think that this should be configurable.
>
> Otherwise, you can either go through the messy work of calling domap
> correctly when the last device of a set has a wwid that doesn't match
> the merge_id, or we can decide that this won't acutally cause problems
> with any known device, and punt fixing it for now. And if it causes
> problems with some future devices, we can deal with it then.
>
> -Ben
>
> >
> > Change-Id: I7f33de49a1d89d91180dcb95cd94e4944ae7ff36
> > Signed-off-by: tang.wenjun <tang.wenjun3@xxxxxxxxxx>
> > ---
> > multipathd/cli_handlers.c | 3 ++-
> > multipathd/main.c | 47
> ++++++++++++++++++++++++++++++++++++-----------
> > multipathd/main.h | 2 +-
> > 3 files changed, 39 insertions(+), 13 deletions(-)
> >
> > diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c
> > index b0eeca6..3a46c09 100644
> > --- a/multipathd/cli_handlers.c
> > +++ b/multipathd/cli_handlers.c
> > @@ -670,7 +670,8 @@ cli_add_path (void * v, char ** reply, int * len,
> void * data)
> > pp->checkint = conf->checkint;
> > }
> > put_multipath_config(conf);
> > - return ev_add_path(pp, vecs);
> > + return ev_add_path(pp, vecs, 1);
> > +
> > blacklisted:
> > *reply = strdup("blacklisted\n");
> > *len = strlen(*reply) + 1;
> > diff --git a/multipathd/main.c b/multipathd/main.c
> > index adc3258..ebd7de1 100644
> > --- a/multipathd/main.c
> > +++ b/multipathd/main.c
> > @@ -608,7 +608,7 @@ ev_remove_map (char * devname, char * alias, int
> minor, struct vectors * vecs)
> > }
> >
> > static int
> > -uev_add_path (struct uevent *uev, struct vectors * vecs)
> > +uev_add_path (struct uevent *uev, struct vectors * vecs, int
> need_do_map)
> > {
> > struct path *pp;
> > int ret = 0, i;
> > @@ -640,9 +640,18 @@ uev_add_path (struct uevent *uev, struct vectors *
> vecs)
> > r = pathinfo(pp,
> conf,
> >
> DI_ALL | DI_BLACKLIST);
> >
> put_multipath_config(conf);
> > - if (r ==
> PATHINFO_OK)
> > - ret
> = ev_add_path(pp, vecs);
> > - else if (r ==
> PATHINFO_SKIPPED) {
> > + if (r ==
> PATHINFO_OK) {
> > + /*
> > + *
> Make sure merging use the correct wwid
> > + *
> Othterwise calling domap()
> > + */
> > + if
> (!need_do_map &&
> > +
> uev->merge_id &&
> > +
> strcmp(uev->merge_id, pp->wwid))
> > +
> need_do_map = 1;
> > +
> > + ret
> = ev_add_path(pp, vecs, need_do_map);
> > + } else if (r ==
> PATHINFO_SKIPPED) {
> >
> condlog(3, "%s: remove blacklisted path",
> >
> uev->kernel);
> > i =
> find_slot(vecs->pathvec, (void *)pp);
> > @@ -681,7 +690,16 @@ uev_add_path (struct uevent *uev, struct vectors *
> vecs)
> > conf = get_multipath_config();
> > pp->checkint = conf->checkint;
> > put_multipath_config(conf);
> > - ret = ev_add_path(pp, vecs);
> > + /*
> > + * Make sure merging use the correct
> wwid
> > + * Othterwise calling domap()
> > + */
> > + if (!need_do_map &&
> > + uev->merge_id &&
> > + strcmp(uev->merge_id, pp->wwid))
> > + need_do_map = 1;
> > +
> > + ret = ev_add_path(pp, vecs,
> need_do_map);
> > } else {
> > condlog(0, "%s: failed to store path
> info, "
> > "dropping event",
> > @@ -699,7 +717,7 @@ uev_add_path (struct uevent *uev, struct vectors *
> vecs)
> > * 1: error
> > */
> > int
> > -ev_add_path (struct path * pp, struct vectors * vecs)
> > +ev_add_path (struct path * pp, struct vectors * vecs, int need_do_map)
> > {
> > struct multipath * mpp;
> > char params[PARAMS_SIZE] = {0};
> > @@ -767,6 +785,13 @@ rescan:
> > /* persistent reservation check*/
> > mpath_pr_event_handle(pp);
> >
> > + if (!need_do_map)
> > + return 0;
> > +
> > + if (!dm_map_present(mpp->alias)) {
> > + mpp->action = ACT_CREATE;
> > + start_waiter = 1;
> > + }
> > /*
> > * push the map to the device-mapper
> > */
> > @@ -995,7 +1020,7 @@ uev_update_path (struct uevent *uev, struct vectors
> * vecs)
> > }
> >
> > if (pp->initialized ==
> INIT_REQUESTED_UDEV)
> > - retval =
> uev_add_path(uev, vecs);
> > + retval =
> uev_add_path(uev, vecs, 1);
> > else if (mpp && ro >= 0) {
> > condlog(2, "%s:
> update path write_protect to '%d' (uevent)", uev->kernel, ro);
> >
> > @@ -1150,7 +1175,7 @@ uev_trigger (struct uevent * uev, void *
> trigger_data)
> > put_multipath_config(conf);
> >
> > if (!strncmp(uev->action, "add", 3)) {
> > - r = uev_add_path(uev, vecs);
> > + r = uev_add_path(uev, vecs, 1);
> > goto out;
> > }
> > if (!strncmp(uev->action, "remove", 6)) {
> > @@ -1570,7 +1595,7 @@ check_path (struct vectors * vecs, struct path *
> pp, int ticks)
> > conf =
> get_multipath_config();
> > ret = pathinfo(pp,
> conf, DI_ALL | DI_BLACKLIST);
> > if (ret ==
> PATHINFO_OK) {
> > -
> ev_add_path(pp, vecs);
> > +
> ev_add_path(pp, vecs, 1);
> >
> pp->tick = 1;
> > } else if (ret ==
> PATHINFO_SKIPPED) {
> >
> put_multipath_config(conf);
> > @@ -1686,7 +1711,7 @@ check_path (struct vectors * vecs, struct path *
> pp, int ticks)
> > }
> > if (!disable_reinstate &&
> reinstate_path(pp, add_active)) {
> > condlog(3, "%s:
> reload map", pp->dev);
> > - ev_add_path(pp,
> vecs);
> > + ev_add_path(pp,
> vecs, 1);
> > pp->tick = 1;
> > return 0;
> > }
> > @@ -1709,7 +1734,7 @@ check_path (struct vectors * vecs, struct path *
> pp, int ticks)
> > /* Clear IO errors
> */
> > if
> (reinstate_path(pp, 0)) {
> >
> condlog(3, "%s: reload map", pp->dev);
> > -
> ev_add_path(pp, vecs);
> > +
> ev_add_path(pp, vecs, 1);
> >
> pp->tick = 1;
> >
> return 0;
> > }
> > diff --git a/multipathd/main.h b/multipathd/main.h
> > index f72580d..f810d41 100644
> > --- a/multipathd/main.h
> > +++ b/multipathd/main.h
> > @@ -22,7 +22,7 @@ void exit_daemon(void);
> > const char * daemon_status(void);
> > int need_to_delay_reconfig (struct vectors *);
> > int reconfigure (struct vectors *);
> > -int ev_add_path (struct path *, struct vectors *);
> > +int ev_add_path (struct path *, struct vectors *, int);
> > int ev_remove_path (struct path *, struct vectors *);
> > int ev_add_map (char *, char *, struct vectors *);
> > int ev_remove_map (char *, char *, int, struct vectors *);
> > --
> > 2.8.1.windows.1
> >
--
dm-devel mailing list
dm-devel@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/dm-devel
