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

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

[tang.junhui]

[-- Attachment #1.1.1: Type: text/plain, Size: 9493 bytes --]

Hello Martin,




Thanks for your responsible,




＞ I'd like the following better for this check. It uses much less cycles.＞  static bool＞ can_discard_by_devpath(const char *devpath)




＞ 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




Yes, I think these codes can be reworked, but since they have no logical

effect, and this patch takes too long time, can you commit a new patch 

to rework these codes?




＞ 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. 

＞ 

＞ 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.




＞ 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.




"case 2: remove path1 |add path1 |remove path2 |add path2 |remove path3"

Since the mergering is starting from the latest to the eariest uevent,  so the

uevent "remove path3" try to merger other uevents, if we do not stop mergering, 




uevent "remove path3" would merge  uevent "remove path2" and uevent 


"remove path1". Uevent "add  path2" would merge "add  path1". 


The result is that we get two merged uevents:


1) add path2, path1


2) remove path3, path2, path1 


Then we process the merged uevents from the earlier to later, We try to ”add 


path2 path1“, and we would create or reload a map device with path1 and path2. 


Next we process ” remove path3, path2, path1“, so path1, path2 and path3 are 


all removed from the map device, this is wrong,  the correct result is that the path1


and path2 should be still in the map device.






Just as you say, this patch is just a shape, we need more testing, and maybe 


more patches are needed to make it better.






Thank you,


Tang Junhui

















 















原始邮件



发件人： ＜mwilck@suse.com＞
收件人：唐军辉10074136 ＜christophe.varoqui@opensvc.com＞ ＜bmarzins@redhat.com＞ ＜hare@suse.de＞ ＜bart.vanassche@sandisk.com＞
抄送人：张凯10072500 ＜dm-devel@redhat.com＞唐文俊10144149
日 期 ：2017年02月17日 05:22
主 题 ：Re: [dm-devel] [PATCH] multipath-tools: improve processing efficiency for addition and deletion of multipath devices





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. 

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.

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

[-- Attachment #1.1.2: Type: text/html , Size: 23738 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

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

[Benjamin Marzinski]

On Fri, Feb 17, 2017 at 11:24:10AM +0800, tang.junhui@zte.com.cn wrote:
>    Hello Martin,
> 
>    Thanks for your responsible,
> 
>    ＞ I'd like the following better for this check. It uses much less cycles.
> 
>    ＞  static bool
>    ＞ can_discard_by_devpath(const char *devpath)
> 
>    ＞ 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;
> 
>    Yes, I think these codes can be reworked, but since they have no logical
> 
>    effect, and this patch takes too long time, can you commit a new patch 
> 
>    to rework these codes?
> 
>    ＞ 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. 
> 
>    ＞ 
> 
>    ＞ 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.
> 
>    ＞ 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.
> 
>    "case 2: remove path1 |add path1 |remove path2 |add path2 |remove path3"
> 
>    Since the mergering is starting from the latest to the eariest uevent,  so
>    the
> 
>    uevent "remove path3" try to merger other uevents, if we do not stop
>    mergering, 
> 
>    uevent "remove path3" would merge  uevent "remove path2" and uevent 
> 
>    "remove path1". Uevent "add  path2" would merge "add  path1". 
> 
>    The result is that we get two merged uevents:
> 
>    1) add path2, path1
> 
>    2) remove path3, path2, path1 
> 
>    Then we process the merged uevents from the earlier to later, We try to
>    ”add 
> 
>    path2 path1“, and we would create or reload a map device with path1 and
>    path2. 
> 
>    Next we process ” remove path3, path2, path1“, so path1, path2 and path3
>    are 
> 
>    all removed from the map device, this is wrong,  the correct result is
>    that the path1
> 
>    and path2 should be still in the map device.
> 
>    Just as you say, this patch is just a shape, we need more testing, and
>    maybe 
> 
>    more patches are needed to make it better.

But in theory, you could have one merged uevent where you

remove path1, add path1, remove path2, add path2, remove path3, and then
call domap one time.

As long as you keep them in right order, I think you could just merge
add and remove events. But what you have right now handles the common
cases, which are bursts of add/chage events, and bursts of remove events.

-Ben

> 
>    Thank you,
> 
>    Tang Junhui
> 
>     
> 
>                                     原始邮件
>    发件人： ＜mwilck@suse.com＞;
>    收件人：唐军辉10074136; ＜christophe.varoqui@opensvc.com＞;
>    ＜bmarzins@redhat.com＞; ＜hare@suse.de＞; ＜bart.vanassche@sandisk.com＞;
>    抄送人：张凯10072500; ＜dm-devel@redhat.com＞;唐文俊10144149;
>    日 期 ：2017年02月17日 05:22
>    主 题 ：Re: [dm-devel] [PATCH] multipath-tools: improve processing
>    efficiency for addition and deletion of multipath devices
> 
>    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. 
> 
>    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.
> 
>    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


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

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

[Martin Wilck]

On Fri, 2017-02-17 at 11:24 +0800, tang.junhui@zte.com.cn wrote:
> Hello Martin,
> 
> Thanks for your responsible,
> 
> ＞ I'd like the following better for this check. It uses much less cyc
> les.
> 
> ＞  static bool
> ＞ can_discard_by_devpath(const char *devpath)
> 
> ＞ 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;
> 
> Yes, I think these codes can be reworked, but since they have no
> logical
> effect, and this patch takes too long time, can you commit a new
> patch 
> to rework these codes?

Yes, I could do that.

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

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

[Martin Wilck]

Hello Tang,

＞ 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.
> 
> ＞ 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.
> 
> "case 2: remove path1 |add path1 |remove path2 |add path2 |remove
> path3"
> Since the mergering is starting from the latest to the eariest
> uevent,  so the
> uevent "remove path3" try to merger other uevents, if we do not stop
> mergering, 
> uevent "remove path3" would merge  uevent "remove path2" and uevent 
> "remove path1". Uevent "add  path2" would merge "add  path1". 
> The result is that we get two merged uevents:
> 1) add path2, path1
> 2) remove path3, path2, path1 

> Then we process the merged uevents from the earlier to later, We try
> to ”add 
> path2 path1“, and we would create or reload a map device with path1
> and path2. 
> Next we process ” remove path3, path2, path1“, so path1, path2 and
> path3 are 
> all removed from the map device, this is wrong,  the correct result
> is that the path1
> and path2 should be still in the map device.

I don't understand that. This would mean that you'd re-order "remove
path1 | add path1" into "add path1| remove path1", and the same for
path2. That may obviously never happen. Your "stop merging" logic
forbids it, and that's fine for me for now.

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

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

[tang.junhui]

[-- Attachment #1.1.1: Type: text/plain, Size: 2834 bytes --]

Hello Martin,





This exmaple shows why we need  to stop merging,

Yes,  It's fine since we have "stop merging" in current code.




RegardsTang












原始邮件



发件人： ＜mwilck@suse.com＞
收件人： ＜dm-devel@redhat.com＞
日 期 ：2017年02月20日 19:24
主 题 ：Re: [dm-devel] 答复: Re:  [PATCH] multipath-tools: improve processing efficiency for addition and deletion of multipath devices





Hello Tang,

＞ 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.
＞ 
＞ ＞ 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.
＞ 
＞ "case 2: remove path1 |add path1 |remove path2 |add path2 |remove
＞ path3"
＞ Since the mergering is starting from the latest to the eariest
＞ uevent,  so the
＞ uevent "remove path3" try to merger other uevents, if we do not stop
＞ mergering, 
＞ uevent "remove path3" would merge  uevent "remove path2" and uevent 
＞ "remove path1". Uevent "add  path2" would merge "add  path1". 
＞ The result is that we get two merged uevents:
＞ 1) add path2, path1
＞ 2) remove path3, path2, path1 

＞ Then we process the merged uevents from the earlier to later, We try
＞ to ”add 
＞ path2 path1“, and we would create or reload a map device with path1
＞ and path2. 
＞ Next we process ” remove path3, path2, path1“, so path1, path2 and
＞ path3 are 
＞ all removed from the map device, this is wrong,  the correct result
＞ is that the path1
＞ and path2 should be still in the map device.

I don't understand that. This would mean that you'd re-order "remove
path1 | add path1" into "add path1| remove path1", and the same for
path2. That may obviously never happen. Your "stop merging" logic
forbids it, and that's fine for me for now.

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

[-- Attachment #1.1.2: Type: text/html , Size: 6291 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]