Re: [PATCH 09/12] multipathd: merge uevents before proccessing

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hello Ben,

Yes, a *_safe list traversal method can meet the needs,
I will modify it and simplify the codes.

Thanks,
Tang Junhui




发件人:         "Benjamin Marzinski" <bmarzins@xxxxxxxxxx>
收件人:         tang.junhui@xxxxxxxxxx,
抄送:        tang.wenjun3@xxxxxxxxxx, zhang.kai16@xxxxxxxxxx, dm-devel@xxxxxxxxxx, bart.vanassche@xxxxxxxxxxx, mwilck@xxxxxxxx
日期:         2017/01/04 08:39
主题:        Re: [dm-devel] [PATCH 09/12] multipathd: merge uevents before        proccessing
发件人:        dm-devel-bounces@xxxxxxxxxx




On Tue, Dec 27, 2016 at 04:03:26PM +0800, tang.junhui@xxxxxxxxxx wrote:
> From: tang.junhui <tang.junhui@xxxxxxxxxx>
>
> These uevents are going to be merged:
> 1) uevents come from paths and
> 2) uevents type is same and
> 3) uevents type is addition or deletion and
> 4) uevents wwid is same.

This is just a nit, and I might be missing something subtle here, but it
seems like instead of adding list_for_some_entry_reverse, and then
breaking the abstraction to manually get previous entries, you could
have just added list_for_some_entry_reverse_safe in your earlier patch,
and hid the work of traversing a list while removing elements behind the
well understood abstraction of a *_safe list traversal method.

-Ben

>
> Change-Id: I05ee057391c092aa0c5f989b7a4f9cb550bb4d98
> Signed-off-by: tang.junhui <tang.junhui@xxxxxxxxxx>
> ---
>  libmultipath/uevent.c | 125 +++++++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 114 insertions(+), 11 deletions(-)
>
> diff --git a/libmultipath/uevent.c b/libmultipath/uevent.c
> index b0b05e9..114068c 100644
> --- a/libmultipath/uevent.c
> +++ b/libmultipath/uevent.c
> @@ -85,6 +85,20 @@ struct uevent * alloc_uevent (void)
>                   return uev;
>  }
>  
> +void
> +uevq_cleanup(struct list_head *tmpq)
> +{
> +                 struct uevent *uev, *tmp;
> +
> +                 list_for_each_entry_safe(uev, tmp, tmpq, node) {
> +                                  list_del_init(&uev->node);
> +
> +                                  if (uev->udev)
> +                                                   udev_device_unref(uev->udev);
> +                                  FREE(uev);
> +                 }
> +}
> +
>  bool
>  uevent_can_discard(char *devpath, char *kernel)
>  {
> @@ -125,6 +139,103 @@ uevent_can_discard(char *devpath, char *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;
> +}
> +
> +bool
> +uevent_can_merge(struct uevent *earlier, struct uevent *later)
> +{
> +                 /* merge paths uevents
> +                  * whose wwids exsit and are same
> +                  * and actions are same,
> +                  * and actions are addition or deletion
> +                  */
> +                 if (earlier->wwid && later->wwid &&
> +                     !strcmp(earlier->wwid, later->wwid) &&
> +                     !strcmp(earlier->action, later->action) &&
> +                     strncmp(earlier->action, "change", 6) &&
> +                     strncmp(earlier->kernel, "dm-", 3)) {
> +                                  return true;
> +                 }
> +
> +                 return false;
> +}
> +
> +void
> +uevent_merge(struct uevent *later, struct list_head *tmpq)
> +{
> +                 struct uevent *earlier, *temp;
> +                 /*
> +                  * compare the uevent with earlier uevents
> +                  */
> +                 list_for_some_entry_reverse(earlier, &later->node, tmpq, node) {
> +next_earlier_node:
> +                                  if (merge_need_stop(earlier, later))
> +                                                   break;
> +                                  /*
> +                                   * try to merge earlier uevents to the later uevent
> +                                   */
> +                                  if (uevent_can_merge(earlier, later)) {
> +                                                   condlog(3, "merged uevent: %s-%s-%s with uevent: %s-%s-%s",
> +                                                                    earlier->action, earlier->kernel, earlier->wwid,
> +                                                                    later->action, later->kernel, later->wwid);
> +                                                   temp = earlier;
> +
> +                                                   earlier = list_entry(earlier->node.prev, typeof(struct uevent), node);
> +                                                   list_move(&temp->node, &later->merge_node);
> +
> +                                                   if (earlier ==  list_entry(tmpq, typeof(struct uevent), node))
> +                                                                    break;
> +                                                   else
> +                                                                    goto next_earlier_node;
> +                                  }
> +                 }
> +}
> +
> +void
> +merge_uevq(struct list_head *tmpq)
> +{
> +                 struct uevent *later;
> +
> +                 list_for_each_entry_reverse(later, tmpq, node) {
> +                                  uevent_merge(later, tmpq);
> +                 }
> +}
> +
>  void
>  service_uevq(struct list_head *tmpq)
>  {
> @@ -136,6 +247,8 @@ service_uevq(struct list_head *tmpq)
>                                    if (my_uev_trigger && my_uev_trigger(uev, my_trigger_data))
>                                                     condlog(0, "uevent trigger error");
>  
> +                                  uevq_cleanup(&uev->merge_node);
> +
>                                    if (uev->udev)
>                                                     udev_device_unref(uev->udev);
>                                    FREE(uev);
> @@ -150,17 +263,6 @@ static void uevent_cleanup(void *arg)
>                   udev_unref(udev);
>  }
>  
> -void
> -uevq_cleanup(struct list_head *tmpq)
> -{
> -                 struct uevent *uev, *tmp;
> -
> -                 list_for_each_entry_safe(uev, tmp, tmpq, node) {
> -                                  list_del_init(&uev->node);
> -                                  FREE(uev);
> -                 }
> -}
> -
>  /*
>   * Service the uevent queue.
>   */
> @@ -189,6 +291,7 @@ int uevent_dispatch(int (*uev_trigger)(struct uevent *, void * trigger_data),
>                                    pthread_mutex_unlock(uevq_lockp);
>                                    if (!my_uev_trigger)
>                                                     break;
> +                                  merge_uevq(&uevq_tmp);
>                                    service_uevq(&uevq_tmp);
>                   }
>                   condlog(3, "Terminating uev service queue");
> --
> 2.8.1.windows.1
>

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


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

[Index of Archives]     [DM Crypt]     [Fedora Desktop]     [ATA RAID]     [Fedora Marketing]     [Fedora Packaging]     [Fedora SELinux]     [Yosemite Discussion]     [KDE Users]     [Fedora Docs]

  Powered by Linux