Re: [PATCH] multipath-tools: improve processing efficiency for addition and deletion of multipath devices

["Benjamin Marzinski" <bmarzins@redhat.com>]

On Thu, Feb 16, 2017 at 11:38:36PM -0600, Benjamin Marzinski wrote:

Quite possibly, I shouldn't try responding to email at 11:30 PM. It
appears that my brain has already given up for the day.

> On Thu, Feb 16, 2017 at 10:17:35PM +0100, Martin Wilck wrote:
> > Hello Tang,
> > 
> > I'm sorry to reply so late. Thanks a lot for your work, I agree with
> > Ben that the patch is in pretty good shape now. But I have some
> > remarks left, please see below. 
> >  
> > > +bool
> > > +uevent_can_discard(struct uevent *uev)
> > > +{
> > > +	char *tmp;
> > > +	char a[11], b[11];
> > > +	struct config * conf;
> > > +
> > > +	/*
> > > +	 * keep only block devices, discard partitions
> > > +	 */
> > > +	tmp = strstr(uev->devpath, "/block/");
> > > +	if (tmp == NULL){
> > > +		condlog(4, "no /block/ in '%s'", uev->devpath);
> > > +		return true;
> > > +	}
> > > +	if (sscanf(tmp, "/block/%10s", a) != 1 ||
> > > +	    sscanf(tmp, "/block/%10[^/]/%10s", a, b) == 2) {
> > > +		condlog(4, "discard event on %s", uev->devpath);
> > > +		return true;
> > > +	}
> > 
> > I'd like the following better for this check. It uses much less cycles.
> > 
> > static bool
> > can_discard_by_devpath(const char *devpath)
> > {
> > 	static const char BLOCK[] = "/block/";
> > 	const char *p;
> > 
> > 	p = strstr(pathstr, BLOCK);
> > 	if (p == NULL)
> > 		/* not a block device */
> > 		return true;
> > 	p += sizeof(BLOCK) - 1;
> > 	p = strchr(p, '/');
> > 	if (p == NULL)
> > 		/* exactly one path element after "/block/" */
> > 		return false;
> > 	/* If there are more path elements, it's a partition */
> > 	return true;
> > }
> > 
> > > +bool
> > > +uevent_can_filter(struct uevent *earlier, struct uevent *later)
> > > +{
> > > +
> > > +	/*
> > > +	 * filter earlier uvents if path has removed later. Eg:
> > > +	 * "add path1 |chang path1 |add path2 |remove path1"
> > > +	 * can filter as:
> > > +	 * "add path2 |remove path1"
> > > +	 * uevents "add path1" and "chang path1" are filtered out
> > > +	 */
> > > +	if (!strcmp(earlier->kernel, later->kernel) &&
> > > +		!strcmp(later->action, "remove") &&
> > > +		strncmp(later->kernel, "dm-", 3)) {
> > > +		return true;
> > > +	}
> > > +
> > > +	/*
> > > +	 * filter change uvents if add uevents exist. Eg:
> > > +	 * "change path1| add path1 |add path2"
> > > +	 * can filter as:
> > > +	 * "add path1 |add path2"
> > > +	 * uevent "chang path1" is filtered out
> > > +	 */
> > > +	if (!strcmp(earlier->kernel, later->kernel) &&
> > > +		!strcmp(earlier->action, "change") &&
> > > +		!strcmp(later->action, "add") &&
> > > +		strncmp(later->kernel, "dm-", 3)) {
> > > +		return true;
> > > +	}
> > > +
> > > +	return false;
> > > +}
> > 
> > This would be better readable and faster if you'd put the "kernel"
> > tests first so that you need to check only "action" later:
> > 
> > 	if (!strncmp(later->kernel, "dm-", 3) ||
> > 	    strcmp(earlier->kernel, later->kernel))
> > 		return false;
> > 
> > > +
> > > +bool
> > > +merge_need_stop(struct uevent *earlier, struct uevent *later)
> > > +{
> > > +	/*
> > > +	 * dm uevent do not try to merge with left uevents
> > > +	 */
> > > +	if (!strncmp(later->kernel, "dm-", 3))
> > > +		return true;
> > > +
> > > +	/*
> > > +	 * we can not make a jugement without wwid,
> > > +	 * so it is sensible to stop merging
> > > +	 */
> > > +	if (!earlier->wwid || !later->wwid)
> > > +		return true;
> > > +	/*
> > > +	 * uevents merging stoped
> > > +	 * when we meet an opposite action uevent from the same LUN
> > > to AVOID
> > > +	 * "add path1 |remove path1 |add path2 |remove path2 |add
> > > path3"
> > > +	 * to merge as "remove path1, path2" and "add path1, path2,
> > > path3"
> > > +	 * OR
> > > +	 * "remove path1 |add path1 |remove path2 |add path2 |remove
> > > path3"
> > > +	 * to merge as "add path1, path2" and "remove path1, path2,
> > > path3"
> > > +	 * SO
> > > +	 * when we meet a non-change uevent from the same LUN
> > > +	 * with the same wwid and different action
> > > +	 * it would be better to stop merging.
> > > +	 */
> > > +	if (!strcmp(earlier->wwid, later->wwid) &&
> > > +	    strcmp(earlier->action, later->action) &&
> > > +	    strcmp(earlier->action, "change") &&
> > > +	    strcmp(later->action, "change"))
> > > +		return true;
> > > +
> > > +	return false;
> > > +}
> > 
> > I know you discussed this with Ben before, but still have some trouble
> > with it. 
> > 
> > The first case should have been reduced to "remove path1 | remove path2
> > | add path3" by filtering beforehand. I suppose you want to avoid this
> > sequence because it could leave us without paths temporarily, causing
> > multipathd to destroy the map. But I don't understand what "stop
> > merging" buys you here - if you process the events one-by-one, you may
> > also have 0 paths at some point. 
> 
> Well, because of the filtering , you will never actually stop merging
> in this case, like you mentioned.

Or, if I actually read better, I would see that you will stop merging.
I can't think of any problems in theory with merging adds after removes
in the simple case at least.

> > In the second case, we know all events in the sequence have the same
> > WWID; in this case I think it would be safe to filter away "remove"
> > events by subsequent "add" events, ending up with "add path1| add
> > path2| remove path3". But I may be overlooking something here.
> 
> We can't filter out the remove path events in this case. If you have
> 
> add sdb | remove sdb
> 
> You know that sdb in the remove event is referring to the same device as
> sdb in the add event. If you have
> 
> remove sdb | add sdb
> 
> There is no guarantee that sdb in the add event is referring to the same
> LUN as sdb in the remove event. Once the device gets removed that name
> can get reused for anything.

Or you could change the filtering code to verify that the wwids matched.
However, while the device would be for the same LUN, it might be a
different I_T Nexus, so you still don't want to filter the remove of that
specific device. 
 
-Ben

> > 
> > The dangerous thing if you have simultaneous remove and add events for
> > the same LUN is that processing the "add" events is likely to fail in
> > domap(). If you get "add path1 | remove path2", once you process "add
> > path1", "path2" may not exist in the kernel any more, and "domap" will
> > fail if you try to set up both; you may end up removing the map
> > completely. IMHO the only safe way to process events in this situation
> > is to merge the events into a single domap() call.
> > 
> > I know you want to avoid that in this patch, but I think it will be a
> > logical further improvement.
> > 
> > Anyway, AFAICS your patch doesn't introduce a regression wrt the
> > current code here; unless I'm overlooking something, my arguments would
> > apply to sequential event processing as well.
> > 
> > Regards
> > Martin
> > 
> > -- 
> > Dr. Martin Wilck <mwilck@suse.com>, 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