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 04:59:53 +0000	[thread overview]
Message-ID: <0DEDF3B159719A448A49EF0E7B11E3223DA75A82@g01jpexmbkw24> (raw)
In-Reply-To: <CAPcyv4iJDSutMKeosdrEFikJw=MSGNMgAJP--bO9jwoQ2iaAOQ@mail.gmail.com>

> -----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?
	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)


> This allows for other objects like regions and namespace to grow an event flags api
> in the future:
> 
>    ndctl_namespace_get_event_flags()
>    ndctl_region_get_event_flags()
>    ndctl_bus_get_event_flags()
> 
> ...where those objects don't have any relationship with a 'struct ndctl_cmd', so
> neither should dimm event monitoring, at least not for the library api that
> ndctl-monitor is using.
> 

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm