Re: [dm-devel] [PATCH v4 10/20] libmultipath: indicate wwid failure in dm_addmap_create()

2018-04-12 Thread Martin Wilck
On Thu, 2018-04-12 at 16:32 -0500, Benjamin Marzinski wrote:
> On Thu, Apr 12, 2018 at 10:22:19PM +0200, Martin Wilck wrote:
> > 
> > But I suppose I'm still missing your point.
> 
> Any failure in ev_add_path() before the call to domap() could cause
> multipath device creation to fail, without triggering this. For
> instance
> add_map_with_path() or setup_map() can fail for a host of reasons.

OK, got it, finally. The semantics of my failed marker were intended to
be exactly what they are now: we tried DM_DEVICE_CREATE for this WWID,
and failed. Other semantics are certainly possible, but much harder to
define cleanly.

I believe that catches a large portion of the real-world failures. And
in many cases, this failure means that one or more devices are busy.
(Oh, it could also mean that we tried to set 'hwhandler "1 alua"' for a
device that doesn't support ALUA, but I digress ;-)

Martin

-- 
Dr. Martin Wilck , Tel. +49 (0)911 74053 2107
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH v4 10/20] libmultipath: indicate wwid failure in dm_addmap_create()

2018-04-12 Thread Benjamin Marzinski
On Thu, Apr 12, 2018 at 10:22:19PM +0200, Martin Wilck wrote:
> On Thu, 2018-04-12 at 22:16 +0200, Martin Wilck wrote:
> > On Thu, 2018-04-12 at 13:33 -0500, Benjamin Marzinski wrote:
> > > On Wed, Apr 04, 2018 at 06:16:17PM +0200, Martin Wilck wrote:
> > > > dm_addmap_create() is where we actually try to set up a new
> > > > multipath map. Depending on the result, mark the wwid as
> > > > failed (or not), and re-trigger an uevent if necessary.
> > > > If a path changes from multipath to non-multipath, use an "add"
> > > > event to make sure LVM2 rules pick it up. Increase log level
> > > > of this event to 3.
> > > > 
> > > 
> > > By only looking at domap, we will miss instances of multipathd
> > > failing
> > > to create maps earlier in the process. This isn't necessarily
> > > wrong.
> > > It
> > > just means that we can't rely on checking
> > > /dev/shm/multipath/failed_wwids to definitively tell us whether
> > > multipathd has tried and failed to create the device.
> > 
> > Sorry, I can't follow you. Where else except in the 
> > domap->dm_addmap_create->dm_addmap() code path do we create maps?
> > I'm feeling stupid, I really can't see it.
> 
> If you were referring to other instances of multipathd which have
> already terminated (e.g. multipathd which ran in initramfs): these
> leave the failed markers under /dev/shm when they quit. That's the
> whole point of this patch. A failed marker will only be removed if a)
> the system is rebooted, or b) another attempt to create the map
> succeeds, or c) a user removes the marker manually.
> 
> But I suppose I'm still missing your point.

Any failure in ev_add_path() before the call to domap() could cause
multipath device creation to fail, without triggering this. For instance
add_map_with_path() or setup_map() can fail for a host of reasons.

> 
> Cheers,
> Martin
> 
> -- 
> Dr. Martin Wilck , Tel. +49 (0)911 74053 2107
> SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
> HRB 21284 (AG Nürnberg)

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel


Re: [dm-devel] [PATCH v4 10/20] libmultipath: indicate wwid failure in dm_addmap_create()

2018-04-12 Thread Martin Wilck
On Thu, 2018-04-12 at 22:16 +0200, Martin Wilck wrote:
> On Thu, 2018-04-12 at 13:33 -0500, Benjamin Marzinski wrote:
> > On Wed, Apr 04, 2018 at 06:16:17PM +0200, Martin Wilck wrote:
> > > dm_addmap_create() is where we actually try to set up a new
> > > multipath map. Depending on the result, mark the wwid as
> > > failed (or not), and re-trigger an uevent if necessary.
> > > If a path changes from multipath to non-multipath, use an "add"
> > > event to make sure LVM2 rules pick it up. Increase log level
> > > of this event to 3.
> > > 
> > 
> > By only looking at domap, we will miss instances of multipathd
> > failing
> > to create maps earlier in the process. This isn't necessarily
> > wrong.
> > It
> > just means that we can't rely on checking
> > /dev/shm/multipath/failed_wwids to definitively tell us whether
> > multipathd has tried and failed to create the device.
> 
> Sorry, I can't follow you. Where else except in the 
> domap->dm_addmap_create->dm_addmap() code path do we create maps?
> I'm feeling stupid, I really can't see it.

If you were referring to other instances of multipathd which have
already terminated (e.g. multipathd which ran in initramfs): these
leave the failed markers under /dev/shm when they quit. That's the
whole point of this patch. A failed marker will only be removed if a)
the system is rebooted, or b) another attempt to create the map
succeeds, or c) a user removes the marker manually.

But I suppose I'm still missing your point.

Cheers,
Martin

-- 
Dr. Martin Wilck , Tel. +49 (0)911 74053 2107
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH v4 10/20] libmultipath: indicate wwid failure in dm_addmap_create()

2018-04-12 Thread Martin Wilck
On Thu, 2018-04-12 at 13:33 -0500, Benjamin Marzinski wrote:
> On Wed, Apr 04, 2018 at 06:16:17PM +0200, Martin Wilck wrote:
> > dm_addmap_create() is where we actually try to set up a new
> > multipath map. Depending on the result, mark the wwid as
> > failed (or not), and re-trigger an uevent if necessary.
> > If a path changes from multipath to non-multipath, use an "add"
> > event to make sure LVM2 rules pick it up. Increase log level
> > of this event to 3.
> > 
> 
> By only looking at domap, we will miss instances of multipathd
> failing
> to create maps earlier in the process. This isn't necessarily wrong.
> It
> just means that we can't rely on checking
> /dev/shm/multipath/failed_wwids to definitively tell us whether
> multipathd has tried and failed to create the device.

Sorry, I can't follow you. Where else except in the 
domap->dm_addmap_create->dm_addmap() code path do we create maps?
I'm feeling stupid, I really can't see it.

Martin

-- 
Dr. Martin Wilck , Tel. +49 (0)911 74053 2107
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH v4 10/20] libmultipath: indicate wwid failure in dm_addmap_create()

2018-04-12 Thread Benjamin Marzinski
On Wed, Apr 04, 2018 at 06:16:17PM +0200, Martin Wilck wrote:
> dm_addmap_create() is where we actually try to set up a new
> multipath map. Depending on the result, mark the wwid as
> failed (or not), and re-trigger an uevent if necessary.
> If a path changes from multipath to non-multipath, use an "add"
> event to make sure LVM2 rules pick it up. Increase log level
> of this event to 3.
> 

By only looking at domap, we will miss instances of multipathd failing
to create maps earlier in the process. This isn't necessarily wrong. It
just means that we can't rely on checking
/dev/shm/multipath/failed_wwids to definitively tell us whether
multipathd has tried and failed to create the device.

Reviewed-by: Benjamin Marzinski 
> Signed-off-by: Martin Wilck 
> ---
>  libmultipath/configure.c | 37 -
>  libmultipath/configure.h |  2 +-
>  libmultipath/devmapper.c |  9 -
>  libmultipath/structs.h   |  1 +
>  multipathd/main.c|  2 +-
>  5 files changed, 39 insertions(+), 12 deletions(-)
> 
> diff --git a/libmultipath/configure.c b/libmultipath/configure.c
> index c1a50e4..9aa3d21 100644
> --- a/libmultipath/configure.c
> +++ b/libmultipath/configure.c
> @@ -445,11 +445,18 @@ trigger_udev_change(const struct multipath *mpp)
>  }
>  
>  void
> -trigger_paths_udev_change(const struct multipath *mpp)
> +trigger_paths_udev_change(struct multipath *mpp, bool is_mpath)
>  {
>   struct pathgroup * pgp;
>   struct path * pp;
>   int i, j;
> + /*
> +  * If a path changes from multipath to non-multipath, we must
> +  * synthesize an artificial "add" event, otherwise the LVM2 rules
> +  * (69-lvm2-lvmetad.rules) won't pick it up. Otherwise, we'd just
> +  * irritate ourselves with an "add", so use "change".
> +  */
> + const char *action = is_mpath ? "change" : "add";
>  
>   if (!mpp || !mpp->pg)
>   return;
> @@ -468,14 +475,21 @@ trigger_paths_udev_change(const struct multipath *mpp)
>*/
>   env = udev_device_get_property_value(
>   pp->udev, "DM_MULTIPATH_DEVICE_PATH");
> - if (env != NULL && !strcmp(env, "1"))
> - continue;
>  
> - condlog(4, "triggering change uevent for %s", pp->dev);
> - sysfs_attr_set_value(pp->udev, "uevent", "change",
> -  strlen("change"));
> + if (is_mpath && env != NULL && !strcmp(env, "1"))
> + continue;
> + else if (!is_mpath &&
> +(env == NULL || !strcmp(env, "0")))
> + continue;
> +
> + condlog(3, "triggering %s uevent for %s (is %smultipath 
> member)",
> + action, pp->dev, is_mpath ? "" : "no ");
> + sysfs_attr_set_value(pp->udev, "uevent",
> +  action, strlen(action));
>   }
>   }
> +
> + mpp->needs_paths_uevent = 0;
>  }
>  
>  static int
> @@ -876,8 +890,10 @@ int domap(struct multipath *mpp, char *params, int 
> is_daemon)
>* succeeded
>*/
>   mpp->force_udev_reload = 0;
> - if (mpp->action == ACT_CREATE && remember_wwid(mpp->wwid) == 1)
> - trigger_paths_udev_change(mpp);
> + if (mpp->action == ACT_CREATE &&
> + (remember_wwid(mpp->wwid) == 1 ||
> +  mpp->needs_paths_uevent))
> + trigger_paths_udev_change(mpp, true);
>   if (!is_daemon) {
>   /* multipath client mode */
>   dm_switchgroup(mpp->alias, mpp->bestpg);
> @@ -902,7 +918,10 @@ int domap(struct multipath *mpp, char *params, int 
> is_daemon)
>   }
>   dm_setgeometry(mpp);
>   return DOMAP_OK;
> - }
> + } else if (r == DOMAP_FAIL && mpp->action == ACT_CREATE &&
> +mpp->needs_paths_uevent)
> + trigger_paths_udev_change(mpp, false);
> +
>   return DOMAP_FAIL;
>  }
>  
> diff --git a/libmultipath/configure.h b/libmultipath/configure.h
> index 545cbc2..8b56d33 100644
> --- a/libmultipath/configure.h
> +++ b/libmultipath/configure.h
> @@ -37,4 +37,4 @@ int get_refwwid (enum mpath_cmds cmd, char * dev, enum 
> devtypes dev_type,
>vector pathvec, char **wwid);
>  int reload_map(struct vectors *vecs, struct multipath *mpp, int refresh, int 
> is_daemon);
>  struct udev_device *get_udev_device(const char *dev, enum devtypes dev_type);
> -void trigger_paths_udev_change(const struct multipath *mpp);
> +void trigger_paths_udev_change(struct multipath *mpp, bool is_mpath);
> diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
> index 2a92105..f2befad 100644
> --- a/libmultipath/devmapper.