RE: [PATCH v9 1/3] ndctl, monitor: add ndctl monitor

From: "Qi, Fuli" <qi.fuli@jp.fujitsu.com>
To: 'Dan Williams' <dan.j.williams@intel.com>
Cc: linux-nvdimm <linux-nvdimm@lists.01.org>
Subject: RE: [PATCH v9 1/3] ndctl, monitor: add ndctl monitor
Date: Mon, 9 Jul 2018 06:31:01 +0000	[thread overview]
Message-ID: <0DEDF3B159719A448A49EF0E7B11E3223DA75AE5@g01jpexmbkw24> (raw)
In-Reply-To: <CAPcyv4jis9dsHEhen-n=sYrBJJrmgpZWCJvn3BaieOgkMFNk=Q@mail.gmail.com>

> -----Original Message-----
> From: Dan Williams [mailto:dan.j.williams@intel.com]
> Sent: Monday, July 9, 2018 3:27 PM
> To: Qi, Fuli/斉 福利 <qi.fuli@jp.fujitsu.com>
> Cc: linux-nvdimm <linux-nvdimm@lists.01.org>
> Subject: Re: [PATCH v9 1/3] ndctl, monitor: add ndctl monitor
> 
> On Sun, Jul 8, 2018 at 11:22 PM, Qi, Fuli <qi.fuli@jp.fujitsu.com> wrote:
> >> -----Original Message-----
> >> From: Dan Williams [mailto:dan.j.williams@intel.com]
> >> Sent: Monday, July 9, 2018 3:04 PM
> >> To: Qi, Fuli/斉 福利 <qi.fuli@jp.fujitsu.com>
> >> Cc: linux-nvdimm <linux-nvdimm@lists.01.org>
> >> Subject: Re: [PATCH v9 1/3] ndctl, monitor: add ndctl monitor
> >>
> >> On Sun, Jul 8, 2018 at 9:59 PM, Qi, Fuli <qi.fuli@jp.fujitsu.com> wrote:
> >> >> -----Original Message-----
> >> >> From: Dan Williams [mailto:dan.j.williams@intel.com]
> >> >> Sent: Sunday, July 8, 2018 5:06 AM
> >> >> To: Qi, Fuli/斉 福利 <qi.fuli@jp.fujitsu.com>
> >> >> Cc: linux-nvdimm <linux-nvdimm@lists.01.org>
> >> >> Subject: Re: [PATCH v9 1/3] ndctl, monitor: add ndctl monitor
> >> >>
> >> >> On Thu, Jun 28, 2018 at 7:30 PM, QI Fuli <qi.fuli@jp.fujitsu.com> wrote:
> >> >> > Ndctl monitor is used for monitoring the smart events of nvdimm DIMMs.
> >> >> > When a smart event fires, monitor will output the notifications
> >> >> > which include dimm health status and evnet informations to
> >> >> > syslog or a logfile by setting [--logfile] option. The
> >> >> > notifications follow json format and can be consumed by log collectors like
> Fluentd.
> >> >> >
> >> >> > The objects to monitor can be selected by setting [--dimm]
> >> >> > [--region] [--namespace] [--bus] options and the event type can
> >> >> > be filtered by setting [--dimm-event] option. These options
> >> >> > support multiple space-separated arguments.
> >> >> >
> >> >> > Ndctl monitor can be forked as a daemon by using [--daemon]
> >> >> > option, such as:
> >> >> >    # ndctl monitor --daemon --logfile /var/log/ndctl/monitor.log
> >> >> >
> >> >> > Signed-off-by: QI Fuli <qi.fuli@jp.fujitsu.com>
> >> >> > ---
> >> >> >  builtin.h              |   1 +
> >> >> >  ndctl/Makefile.am      |   3 +-
> >> >> >  ndctl/lib/libndctl.sym |   1 +
> >> >> >  ndctl/lib/smart.c      |  17 ++
> >> >> >  ndctl/libndctl.h       |   6 +
> >> >> >  ndctl/monitor.c        | 531 +++++++++++++++++++++++++++++++++++++++++
> >> >> >  ndctl/ndctl.c          |   1 +
> >> >> >  util/filter.h          |   9 +
> >> >> >  8 files changed, 568 insertions(+), 1 deletion(-)  create mode
> >> >> > 100644 ndctl/monitor.c
> >> >> >
> >> >> > diff --git a/builtin.h b/builtin.h index d3cc723..675a6ce 100644
> >> >> > --- a/builtin.h
> >> >> > +++ b/builtin.h
> >> >> > @@ -39,6 +39,7 @@ int cmd_inject_error(int argc, const char
> >> >> > **argv, void *ctx);  int cmd_wait_scrub(int argc, const char
> >> >> > **argv, void *ctx);  int cmd_start_scrub(int argc, const char
> >> >> > **argv, void *ctx); int cmd_list(int argc, const char **argv,
> >> >> > void *ctx);
> >> >> > +int cmd_monitor(int argc, const char **argv, void *ctx);
> >> >> >  #ifdef ENABLE_TEST
> >> >> >  int cmd_test(int argc, const char **argv, void *ctx);  #endif
> >> >> > diff --git a/ndctl/Makefile.am b/ndctl/Makefile.am index
> >> >> > d22a379..7dbf223
> >> >> > 100644
> >> >> > --- a/ndctl/Makefile.am
> >> >> > +++ b/ndctl/Makefile.am
> >> >> > @@ -16,7 +16,8 @@ ndctl_SOURCES = ndctl.c \
> >> >> >                 util/json-smart.c \
> >> >> >                 util/json-firmware.c \
> >> >> >                 inject-error.c \
> >> >> > -               inject-smart.c
> >> >> > +               inject-smart.c \
> >> >> > +               monitor.c
> >> >> >
> >> >> >  if ENABLE_DESTRUCTIVE
> >> >> >  ndctl_SOURCES += ../test/blk_namespaces.c \ diff --git
> >> >> > a/ndctl/lib/libndctl.sym b/ndctl/lib/libndctl.sym index
> >> >> > e939993..f64df56 100644
> >> >> > --- a/ndctl/lib/libndctl.sym
> >> >> > +++ b/ndctl/lib/libndctl.sym
> >> >> > @@ -366,4 +366,5 @@ global:
> >> >> >         ndctl_namespace_inject_error2;
> >> >> >         ndctl_namespace_uninject_error2;
> >> >> >         ndctl_cmd_ars_stat_get_flag_overflow;
> >> >> > +       ndctl_cmd_smart_get_event_flags;
> >> >> >  } LIBNDCTL_15;
> >> >> > diff --git a/ndctl/lib/smart.c b/ndctl/lib/smart.c index
> >> >> > 0455252..90a65d0 100644
> >> >> > --- a/ndctl/lib/smart.c
> >> >> > +++ b/ndctl/lib/smart.c
> >> >> > @@ -101,6 +101,23 @@ NDCTL_EXPORT unsigned int
> >> >> > ndctl_cmd_smart_threshold_get_temperature(
> >> >> >
> >> >> >  smart_cmd_op(smart_threshold_get_supported_alarms, unsigned
> >> >> > int, 0);
> >> >> >
> >> >> > +NDCTL_EXPORT unsigned int
> >> >> > +ndctl_cmd_smart_get_event_flags(struct
> >> >> > +ndctl_cmd *cmd)
> >> >>
> >> >> My expectation for this ndctl_*_get_event_flags() api was to have it be:
> >> >>
> >> >>     ndctl_dimm_get_event_flags()
> >> >>
> >> >> ...and with that in place get rid of the 'struct monitor_dimm' object.
> >> >> Push everything to be retrieved through api calls against a 'struct ndctl_dimm'
> >> object.
> >> >> In other words, the usage of 'struct ndctl_cmd'
> >> >> should be hidden and all monitor operations should be done in
> >> >> terms of 'struct ndctl_dimm' helper calls.
> >> >>
> >> > Hi Dan,
> >> > Thanks for your comments.
> >> >
> >> > In the v9 of monitor, I use the 'struct ndctl_cmd' object in the following places:
> >> >         ndctl_cmd_smart_get_flags(struct ndctl_cmd *cmd)
> >> >         ndctl_cmd_smart_get_health(struct ndctl_cmd *cmd)
> >> >         ndctl_cmd_smart_get_event_flags(struct ndctl_cmd *cmd) Is
> >> > it that you want to hide all of the 'struct ndctl_cmd' objects and
> >> > add the following
> >> 'struct ndctl_dimm' helper calls?
> >>
> >> I'm primarily reacting to:
> >>
> >> +struct monitor_dimm {
> >> +       struct ndctl_dimm *dimm;
> >> +       int health_eventfd;
> >> +       unsigned int health;
> >> +       unsigned int event_flags;
> >> +       struct list_node list;
> >> +};
> >>
> >> Which is effectively duplicating ndctl_dimm internal data.
> >>
> >> >         ndctl_dimm_get_flags(struct ndctl_dimm *dimm)
> >> >         ndctl_dimm_get_health(struct ndctl_dimm *dimm)
> >> >         ndctl_dimm_get_event_flags(struct ndctl_dimm *dimm)
> >>
> >> I understand why we need ndctl_dimm_get_event_flags() since that
> >> tells you what events have fired. Why do we need the other 2? If the
> >> event has fired then the monitor proceeds to call util_dimm_health_to_json.
> >> Is that not sufficient?
> >>
> > About ndctl_dimm_get_flags(struct ndctl_dimm *dimm), I think the
> ND_SMART_ALARM_VALID of DIMM should be confirmed before monitor start.
> > If the smart alarm invalid, the DIMM no need to monitor.
> > Also, the monitor should know the health of DIMM when it starts. When an event
> fires, then compare it with current health of DIMM.
> > Therefore, we could know if the dimm-health-state event fired.
> 
> Ok, but that sounds like two APIs. _get_supported_flags() and _get_flags().
> 
Ok, I will add both _get_supported_flags() and _get_flags().
Thank you very much.
Qi
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm