Re: [RFC PATCH v3 0/5] ndctl: monitor: monitor the smart events of

[Qi, Fuli]

On 2018/02/10 13:06, Dan Williams wrote:


On Fri, Feb 9, 2018 at 12:02 AM, QI Fuli  wrote:

This is a patch set of ndctl monitor, a tiny daemon to monitor the smart
events of nvdimm dimms. When a smart event fires, monitor will output
the notification which including dimm health status to syslog or a
special file according to users' configuration. The output notification
follows json format and can be consumed by log collectors like Fluentd.

Currently, I implemeted the following four commands to control monitor daemon.
$ndctl create-monitor
$ndctl list-monitor
$ndctl show-monitor
$ndclt destroy-monitor

I will appreciate if you could give some comments.

Nice to see this new update.

I have a high level comment on the proposed command structure, as far
as I can see the only new command we need is:

ndctl monitor

...that launches a monitor. We can have options to control whether
that monitor is a one-shot check of events sources, or a daemon that
forks into the background, but the 'create', 'list', 'show', and
'destroy' are the responsibility of systemd or System V services. For
example:

 create: systemctl start ndctl-monitor
 show: systemctl status ndctl-monitor
 destroy: systemctl stop ndctl-monitor



Thank you for your comments, I will use systemd in next version.


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

Re: [RFC PATCH v3 0/5] ndctl: monitor: monitor the smart events of

[Qi, Fuli]

On 2018/02/10 3:06, Verma, Vishal L wrote:

On Fri, 2018-02-09 at 17:02 +0900, QI Fuli wrote:

This is a patch set of ndctl monitor, a tiny daemon to monitor the
smart
events of nvdimm dimms. When a smart event fires, monitor will output
the notification which including dimm health status to syslog or a
special file according to users' configuration. The output
notification
follows json format and can be consumed by log collectors like
Fluentd.

Currently, I implemeted the following four commands to control
monitor daemon.
$ndctl create-monitor
$ndctl list-monitor
$ndctl show-monitor
$ndclt destroy-monitor

I will appreciate if you could give some comments.

Change log since v2:
  - Changing the interface of daemon to the ndctl command line
  - Changing the name of daemon form "nvdimmd" to "monitor"
  - Removing the config file, unit_file, nvdimmd dir
  - Removing nvdimmd_test program
  - Adding ndctl/monitor.c

Change log since v1:
  - Adding a config file(/etc/nvdimmd/nvdimmd.conf)
  - Using struct log_ctx instead of syslog()
 - Using log_syslog() to save the notify messages to syslog
 - Using log_file() to save the notify messages to special file
  - Adding LOG_NOTICE level to log_priority
  - Using automake instead of Makefile
  - Adding a new util file(nvdimmd/util.c) including helper functions
needed for nvdimm daemon
  - Adding nvdimmd_test program

QI Fuli (5):
   ndctl: monitor: add LOG_NOTICE level to log_priority
   ndctl: monitor: add ndclt create-monitor command
   ndctl: monitor: add ndclt list-monitor command
   ndctl: monitor: add ndclt show-monitor command
   ndctl: monitor: add ndclt destroy-monitor command

 ^
I haven't had a chance to look at the rest of the series, but spotted a
quick typo - this is in the subject line of each commit as well:
s/ndclt/ndctl/

Thank you for your comment, I will be more careful next time.



  builtin.h |   4 +
  configure.ac  |   3 +
  ndctl/Makefile.am |   3 +-
  ndctl/monitor.c   | 463
++
  ndctl/ndctl.c |   4 +
  util/log.c|   2 +
  util/log.h|   3 +
  7 files changed, 481 insertions(+), 1 deletion(-)
  create mode 100644 ndctl/monitor.c




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

Re: [RFC PATCH v3 2/5] ndctl: monitor: add ndctl create-monitor command

[Verma, Vishal L]

On Sun, 2018-02-11 at 12:48 -0800, Dan Williams wrote:
> On Fri, Feb 9, 2018 at 12:02 AM, QI Fuli 
> wrote:
> > This patch is used to add $ndctl create-monitor command, by which
> > users can
> > create a new monitor. Users can select the DIMMS to be monitored by
> > using
> > [--dimm] [--bus] [--region] [--namespace] options. The
> > notifications can
> > be outputed to a special file or syslog by using [--output] option,
> > the
> > special file will be placed under /var/log/ndctl. A name is also
> > required for
> > a monitor,so users can destroy the monitor by the name. When a
> > monitor is
> > created successfully, a file with same name will be created under
> > /var/ndctl/monitor.
> > Example:
> > #ndctl create-monitor --monitor m_nmem1 --dimm nmem1 --output
> > m_nmem.log
> 
> Hi Qi,
> 
> This is getting closer to where I want to see this go, but still some
> architecture details to incorporate. I mentioned on the cover letter
> that systemd can handle starting, stopping, and show the status of
> the
> monitor. The other detail to incorporate is that monitor events can
> come DIMMs, but also namespaces, regions, and the bus.
> 
> The event list I have collected to date is:
> 
> dimm-spares-remaining
> dimm-media-temperature
> dimm-controller-temperature
> dimm-health-state
> dimm-unclean-shutdown
> dimm-detected
> namespace-media-error
> namespace-detected
> region-media-error
> region-detected
> bus-media-error
> bus-address-range-scrub-complete
> bus-detected
> 
> ...and I think all of those should be separate options, probably
> something like the following, but I'd Vishal to comment if this
> scheme
> can be handled with the bash tab-completion implementation:
> 
>ndctl monitor --dimm-events=spares-remaining,media-temperature
> --namespace-events=all --regions-events --bus=ACPI.NFIT

Yes I think we should be able to extend bash completion for a comma
separated list of arguments.

> 
> ...where an empty ---events option is equivalent to
> ---events=all. Also, similar to "ndctl list" specifying
> specific buses, namespaces, etc causes the monitor to filter the
> objects based on those properties.
> 
> Since "ndctl list" already has this filtering implemented I'd like to
> see it refactored and shared between the 2 implementations rather
> than
> duplicated as is done in this patch. In other words rework cmd_list()
> into a generic nvdimm object walking routine with callback functions
> to 'list' or 'monitor' a given object that matches the filter.
> 
> Let me know if the above makes sense. I'm thinking the 'ndctl list'
> refactoring might be something I need to handle.
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

[PATCH] ndctl: add option to list firmware information for a DIMM

[Dave Jiang]

Adding firmware output of firmware information when ndctl list -D -F is used.
Components displayed are current firmware version, updated firmware version,
and if a coldboot is required (firmware updated).

Signed-off-by: Dave Jiang 
---
 Documentation/ndctl/ndctl-list.txt |   13 +
 1 file changed, 13 insertions(+)

diff --git a/Documentation/ndctl/ndctl-list.txt 
b/Documentation/ndctl/ndctl-list.txt
index fc07a71..85b87d4 100644
--- a/Documentation/ndctl/ndctl-list.txt
+++ b/Documentation/ndctl/ndctl-list.txt
@@ -110,6 +110,19 @@ include::xable-region-options.txt[]
   }
 }
 
+-F::
+--firmware::
+   Include dimm firmware info in the listing. For example:
+[verse]
+{
+  "dev":"nmem0",
+  "firmware":{
+  "current_version":0,
+  "next_version":1,
+  "coldboot_required":true
+  }
+}
+
 -X::
 --device-dax::
Include device-dax ("daxregion") details when a namespace is in
diff --git a/ndctl/Makefile.am b/ndctl/Makefile.am
index 2054c1a..e0db97b 100644
--- a/ndctl/Makefile.am
+++ b/ndctl/Makefile.am
@@ -13,6 +13,7 @@ ndctl_SOURCES = ndctl.c \
test.c \
../util/json.c \
util/json-smart.c \
+   util/json-firmware.c \
inject-error.c \
update.c \
inject-smart.c
diff --git a/ndctl/list.c b/ndctl/list.c
index 37a224a..ebe0a67 100644
--- a/ndctl/list.c
+++ b/ndctl/list.c
@@ -35,6 +35,7 @@ static struct {
bool dax;
bool media_errors;
bool human;
+   bool firmware;
 } list;
 
 static unsigned long listopts_to_flags(void)
@@ -277,6 +278,7 @@ int cmd_list(int argc, const char **argv, void *ctx)
"filter by region-type"),
OPT_BOOLEAN('B', "buses", , "include bus info"),
OPT_BOOLEAN('D', "dimms", , "include dimm info"),
+   OPT_BOOLEAN('F', "firmware", , "include firmware 
info"),
OPT_BOOLEAN('H', "health", , "include dimm health"),
OPT_BOOLEAN('R', "regions", ,
"include region info"),
@@ -420,6 +422,25 @@ int cmd_list(int argc, const char **argv, void *ctx)
}
}
 
+   if (list.firmware) {
+   struct json_object *jfirmware;
+
+   jfirmware = util_dimm_firmware_to_json(dimm);
+   if (jfirmware)
+   json_object_object_add(jdimm,
+   "firmware",
+   jfirmware);
+   else if (ndctl_dimm_is_cmd_supported(dimm,
+   ND_CMD_CALL)) {
+   /*
+* Failed to retrieve firmware
+* version.
+*/
+   fail("\n");
+   continue;
+   }
+   }
+
/*
 * Without a bus we are collecting dimms anonymously
 * across the platform.
diff --git a/ndctl/util/json-firmware.c b/ndctl/util/json-firmware.c
new file mode 100644
index 000..0010493
--- /dev/null
+++ b/ndctl/util/json-firmware.c
@@ -0,0 +1,88 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+struct json_object *util_dimm_firmware_to_json(struct ndctl_dimm *dimm)
+{
+   struct json_object *jfirmware = json_object_new_object();
+   struct json_object *jobj;
+   struct ndctl_cmd *cmd;
+   int rc;
+   uint64_t run, updated;
+   bool reboot = false;
+
+   if (!jfirmware)
+   return NULL;
+
+   cmd = ndctl_dimm_cmd_new_fw_get_info(dimm);
+   if (!cmd)
+   goto err;
+
+   rc = ndctl_cmd_submit(cmd);
+   if (rc || ndctl_cmd_fw_xlat_firmware_status(cmd) != FW_SUCCESS) {
+   jobj = json_object_new_string("unknown");
+   if (jobj)
+   json_object_object_add(jfirmware, "version",
+   jobj);
+   goto out;
+   }
+
+   run = ndctl_cmd_fw_info_get_run_version(cmd);
+   if (run == ULLONG_MAX) {
+   jobj = json_object_new_string("unknown");
+   if (jobj)
+   json_object_object_add(jfirmware, "current_version",
+   jobj);
+   goto out;
+   }
+
+   jobj = json_object_new_int64(run);
+   if (jobj)
+   json_object_object_add(jfirmware, "current_version", jobj);
+
+   updated = ndctl_cmd_fw_info_get_updated_version(cmd);
+ 

Re: [PATCH v2] libnvdimm: re-enable deep flush for pmem devices

[Ross Zwisler]

On Mon, Feb 12, 2018 at 03:05:10PM -0800, Dan Williams wrote:
> On Mon, Feb 12, 2018 at 2:53 PM, Jeff Moyer  wrote:
> > Dave Jiang  writes:
> >
> >> Re-enable deep flush so that users always have a way to be sure that a 
> >> write
> >> does make it all the way out to the NVDIMM. The PMEM driver writes always
> >> make it "all the way to the NVDIMM", and it relies on the ADR mechanism to
> >> flush the write buffers on power failure. Deep flush is there to explicitly
> >> flush those write buffers to protect against (rare) ADR failure.
> >> This change prevents a regression in deep flush behavior so that 
> >> applications
> >> can continue to depend on fsync() as a mechanism to trigger deep flush in 
> >> the
> >> filesystem-dax case.
> >
> > That's still very confusing text.  Specifically, the part where you say
> > that pmem driver writes always make it to the DIMM.  I think the
> > changelog could start with "Deep flush is there to explicitly flush
> > write buffers"  Anyway, the fix looks right to me.
> 
> I ended up changing the commit message to this, let me know if it reads 
> better:
> 
> 
> libnvdimm: re-enable deep flush for pmem devices via fsync()
> 
> Re-enable deep flush so that users always have a way to be sure that a
> write makes it all the way out to media. The PMEM driver writes always
> arrive at the NVDIMM, and it relies on the ADR (Asynchronous DRAM
> Refresh) mechanism to flush the write buffers on power failure. Deep
> flush is there to explicitly flush those write buffers to protect
> against (rare) ADR failure.  This change prevents a regression in deep
> flush behavior so that applications can continue to depend on fsync() as
> a mechanism to trigger deep flush in the filesystem-DAX case.
> 
> Fixes: 06e8ccdab15f4 ("acpi: nfit: Add support for detect platform
> CPU cache...")
> Signed-off-by: Dave Jiang 
> Signed-off-by: Dan Williams 

Plus Jeff's reviewed-by.
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

[PATCH] loop: Fix lost writes caused by missing flag

[Ross Zwisler]

The following commit:

commit aa4d86163e4e ("block: loop: switch to VFS ITER_BVEC")

replaced __do_lo_send_write(), which used ITER_KVEC iterators, with
lo_write_bvec() which uses ITER_BVEC iterators.  In this change, though,
the WRITE flag was lost:

-   iov_iter_kvec(, ITER_KVEC | WRITE, , 1, len);
+   iov_iter_bvec(, ITER_BVEC, bvec, 1, bvec->bv_len);

This flag is necessary for the DAX case because we make decisions based on
whether or not the iterator is a READ or a WRITE in dax_iomap_actor() and
in dax_iomap_rw().

We end up going through this path in configurations where we combine a PMEM
device with 4k sectors, a loopback device and DAX.  The consequence of this
missed flag is that what we intend as a write actually turns into a read in
the DAX code, so no data is ever written.

The very simplest test case is to create a loopback device and try and
write a small string to it, then hexdump a few bytes of the device to see
if the write took.  Without this patch you read back all zeros, with this
you read back the string you wrote.

For XFS this causes us to fail or panic during the following xfstests:

xfs/074 xfs/078 xfs/216 xfs/217 xfs/250

For ext4 we have a similar issue where writes never happen, but we don't
currently have any xfstests that use loopback and show this issue.

Fix this by restoring the WRITE flag argument to iov_iter_bvec().  This
causes the xfstests to all pass.

Signed-off-by: Ross Zwisler 
Fixes: commit aa4d86163e4e ("block: loop: switch to VFS ITER_BVEC")
Cc: Christoph Hellwig 
Cc: Al Viro 
Cc: sta...@vger.kernel.org
---
 drivers/block/loop.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index d5fe720cf149..89d2ee00cced 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -266,7 +266,7 @@ static int lo_write_bvec(struct file *file, struct bio_vec 
*bvec, loff_t *ppos)
struct iov_iter i;
ssize_t bw;
 
-   iov_iter_bvec(, ITER_BVEC, bvec, 1, bvec->bv_len);
+   iov_iter_bvec(, ITER_BVEC | WRITE, bvec, 1, bvec->bv_len);
 
file_start_write(file);
bw = vfs_iter_write(file, , ppos, 0);
-- 
2.14.3

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

Re: [PATCH v2] libnvdimm: re-enable deep flush for pmem devices

[Dan Williams]

On Mon, Feb 12, 2018 at 2:53 PM, Jeff Moyer  wrote:
> Dave Jiang  writes:
>
>> Re-enable deep flush so that users always have a way to be sure that a write
>> does make it all the way out to the NVDIMM. The PMEM driver writes always
>> make it "all the way to the NVDIMM", and it relies on the ADR mechanism to
>> flush the write buffers on power failure. Deep flush is there to explicitly
>> flush those write buffers to protect against (rare) ADR failure.
>> This change prevents a regression in deep flush behavior so that applications
>> can continue to depend on fsync() as a mechanism to trigger deep flush in the
>> filesystem-dax case.
>
> That's still very confusing text.  Specifically, the part where you say
> that pmem driver writes always make it to the DIMM.  I think the
> changelog could start with "Deep flush is there to explicitly flush
> write buffers"  Anyway, the fix looks right to me.

I ended up changing the commit message to this, let me know if it reads better:


libnvdimm: re-enable deep flush for pmem devices via fsync()

Re-enable deep flush so that users always have a way to be sure that a
write makes it all the way out to media. The PMEM driver writes always
arrive at the NVDIMM, and it relies on the ADR (Asynchronous DRAM
Refresh) mechanism to flush the write buffers on power failure. Deep
flush is there to explicitly flush those write buffers to protect
against (rare) ADR failure.  This change prevents a regression in deep
flush behavior so that applications can continue to depend on fsync() as
a mechanism to trigger deep flush in the filesystem-DAX case.

Fixes: 06e8ccdab15f4 ("acpi: nfit: Add support for detect platform
CPU cache...")
Signed-off-by: Dave Jiang 
Signed-off-by: Dan Williams 
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH v2] libnvdimm: re-enable deep flush for pmem devices

[Jeff Moyer]

Dave Jiang  writes:

> Re-enable deep flush so that users always have a way to be sure that a write
> does make it all the way out to the NVDIMM. The PMEM driver writes always
> make it "all the way to the NVDIMM", and it relies on the ADR mechanism to
> flush the write buffers on power failure. Deep flush is there to explicitly
> flush those write buffers to protect against (rare) ADR failure.
> This change prevents a regression in deep flush behavior so that applications
> can continue to depend on fsync() as a mechanism to trigger deep flush in the
> filesystem-dax case.

That's still very confusing text.  Specifically, the part where you say
that pmem driver writes always make it to the DIMM.  I think the
changelog could start with "Deep flush is there to explicitly flush
write buffers"  Anyway, the fix looks right to me.

Reviewed-by: Jeff Moyer 
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH v2] libnvdimm: re-enable deep flush for pmem devices

[Dan Williams]

On Mon, Feb 12, 2018 at 1:46 PM, Dave Jiang  wrote:
> Re-enable deep flush so that users always have a way to be sure that a write
> does make it all the way out to the NVDIMM. The PMEM driver writes always
> make it "all the way to the NVDIMM", and it relies on the ADR mechanism to
> flush the write buffers on power failure. Deep flush is there to explicitly
> flush those write buffers to protect against (rare) ADR failure.
> This change prevents a regression in deep flush behavior so that applications
> can continue to depend on fsync() as a mechanism to trigger deep flush in the
> filesystem-dax case.
>
> Fixes: 06e8ccdab15f4 ("acpi: nfit: Add support for detect platform CPU cache 
> flush on power loss")
>
> Signed-off-by: Dave Jiang 

Thanks Dave, applied.
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

[PATCH v2] libnvdimm: re-enable deep flush for pmem devices

[Dave Jiang]

Re-enable deep flush so that users always have a way to be sure that a write
does make it all the way out to the NVDIMM. The PMEM driver writes always
make it "all the way to the NVDIMM", and it relies on the ADR mechanism to
flush the write buffers on power failure. Deep flush is there to explicitly
flush those write buffers to protect against (rare) ADR failure.
This change prevents a regression in deep flush behavior so that applications
can continue to depend on fsync() as a mechanism to trigger deep flush in the
filesystem-dax case.

Fixes: 06e8ccdab15f4 ("acpi: nfit: Add support for detect platform CPU cache 
flush on power loss")

Signed-off-by: Dave Jiang 
---

v2: Updated commit patch header from Dan's comments.

 drivers/nvdimm/pmem.c |3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/nvdimm/pmem.c b/drivers/nvdimm/pmem.c
index 10041ac4032c..06f8dcc52ca6 100644
--- a/drivers/nvdimm/pmem.c
+++ b/drivers/nvdimm/pmem.c
@@ -335,8 +335,7 @@ static int pmem_attach_disk(struct device *dev,
dev_warn(dev, "unable to guarantee persistence of writes\n");
fua = 0;
}
-   wbc = nvdimm_has_cache(nd_region) &&
-   !test_bit(ND_REGION_PERSIST_CACHE, _region->flags);
+   wbc = nvdimm_has_cache(nd_region);
 
if (!devm_request_mem_region(dev, res->start, resource_size(res),
dev_name(>dev))) {

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

Re: [PATCH] libnvdimm: re-enable deep flush for pmem devices

[Dan Williams]

On Mon, Feb 12, 2018 at 1:28 PM, Dave Jiang  wrote:
> Re-enable deep flush so that users always have a way to be sure that a write
> does make it all the way out to the NVDIMM.

I think this changelog needs to be a bit more clear/precise. The PMEM
driver writes always make it "all the way to the NVDIMM", and it
relies on the ADR mechanism them through write buffers on power
failure. Deep flush is there to explicitly flush those write buffers
to protect against (rare) ADR failure. This change prevents a
regression in deep flush behavior so that applications can continue to
depend on fsync() as a mechanism to trigger deep flush in the
filesystem-dax case.

Can you incorporate that feedback and also copy lkml on the resend?
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

[PATCH] libnvdimm: re-enable deep flush for pmem devices

[Dave Jiang]

Re-enable deep flush so that users always have a way to be sure that a write
does make it all the way out to the NVDIMM.

Signed-off-by: Dave Jiang 
---
 drivers/nvdimm/pmem.c |3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/nvdimm/pmem.c b/drivers/nvdimm/pmem.c
index 10041ac4032c..06f8dcc52ca6 100644
--- a/drivers/nvdimm/pmem.c
+++ b/drivers/nvdimm/pmem.c
@@ -335,8 +335,7 @@ static int pmem_attach_disk(struct device *dev,
dev_warn(dev, "unable to guarantee persistence of writes\n");
fua = 0;
}
-   wbc = nvdimm_has_cache(nd_region) &&
-   !test_bit(ND_REGION_PERSIST_CACHE, _region->flags);
+   wbc = nvdimm_has_cache(nd_region);
 
if (!devm_request_mem_region(dev, res->start, resource_size(res),
dev_name(>dev))) {

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