[PATCH v2] libnvdimm, pmem: Add sysfs notifications to badblocks

[PATCH v2] libnvdimm, pmem: Add sysfs notifications to badblocks

[Toshi Kani]

Sysfs "badblocks" information may be updated during run-time that:
 - MCE, SCI, and sysfs "scrub" may add new bad blocks
 - Writes and ioctl() may clear bad blocks

Add support to send sysfs notifications to sysfs "badblocks" file
under region and pmem directories when their badblocks information
is re-evaluated (but is not necessarily changed) during run-time.

Signed-off-by: Toshi Kani <toshi.kani@hpe.com>
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: Vishal Verma <vishal.l.verma@intel.com>
Cc: Linda Knippers <linda.knippers@hpe.com>
---
v2: Send notifications for the clearing case
---
 drivers/nvdimm/bus.c    |    3 +++
 drivers/nvdimm/nd.h     |    1 +
 drivers/nvdimm/pmem.c   |   14 ++++++++++++++
 drivers/nvdimm/pmem.h   |    1 +
 drivers/nvdimm/region.c |   12 ++++++++++--
 5 files changed, 29 insertions(+), 2 deletions(-)

diff --git a/drivers/nvdimm/bus.c b/drivers/nvdimm/bus.c
index e9361bf..63ce50d 100644
--- a/drivers/nvdimm/bus.c
+++ b/drivers/nvdimm/bus.c
@@ -198,6 +198,9 @@ static int nvdimm_clear_badblocks_region(struct device *dev, void *data)
 	sector = (ctx->phys - nd_region->ndr_start) / 512;
 	badblocks_clear(&nd_region->bb, sector, ctx->cleared / 512);
 
+	if (nd_region->bb_state)
+		sysfs_notify_dirent(nd_region->bb_state);
+
 	return 0;
 }
 
diff --git a/drivers/nvdimm/nd.h b/drivers/nvdimm/nd.h
index 03852d7..4bb57ff 100644
--- a/drivers/nvdimm/nd.h
+++ b/drivers/nvdimm/nd.h
@@ -155,6 +155,7 @@ struct nd_region {
 	u64 ndr_start;
 	int id, num_lanes, ro, numa_node;
 	void *provider_data;
+	struct kernfs_node *bb_state;
 	struct badblocks bb;
 	struct nd_interleave_set *nd_set;
 	struct nd_percpu_lane __percpu *lane;
diff --git a/drivers/nvdimm/pmem.c b/drivers/nvdimm/pmem.c
index c544d46..6c14c72 100644
--- a/drivers/nvdimm/pmem.c
+++ b/drivers/nvdimm/pmem.c
@@ -68,6 +68,8 @@ static int pmem_clear_poison(struct pmem_device *pmem, phys_addr_t offset,
 				(unsigned long long) sector, cleared,
 				cleared > 1 ? "s" : "");
 		badblocks_clear(&pmem->bb, sector, cleared);
+		if (pmem->bb_state)
+			sysfs_notify_dirent(pmem->bb_state);
 	}
 
 	invalidate_pmem(pmem->virt_addr + offset, len);
@@ -377,6 +379,13 @@ static int pmem_attach_disk(struct device *dev,
 
 	revalidate_disk(disk);
 
+	pmem->bb_state = sysfs_get_dirent(disk_to_dev(disk)->kobj.sd,
+					  "badblocks");
+	if (pmem->bb_state)
+		sysfs_put(pmem->bb_state);
+	else
+		dev_warn(dev, "sysfs_get_dirent 'badblocks' failed\n");
+
 	return 0;
 }
 
@@ -428,6 +437,7 @@ static void nd_pmem_notify(struct device *dev, enum nvdimm_event event)
 	struct nd_namespace_io *nsio;
 	struct resource res;
 	struct badblocks *bb;
+	struct kernfs_node *bb_state;
 
 	if (event != NVDIMM_REVALIDATE_POISON)
 		return;
@@ -439,11 +449,13 @@ static void nd_pmem_notify(struct device *dev, enum nvdimm_event event)
 		nd_region = to_nd_region(ndns->dev.parent);
 		nsio = to_nd_namespace_io(&ndns->dev);
 		bb = &nsio->bb;
+		bb_state = NULL;
 	} else {
 		struct pmem_device *pmem = dev_get_drvdata(dev);
 
 		nd_region = to_region(pmem);
 		bb = &pmem->bb;
+		bb_state = pmem->bb_state;
 
 		if (is_nd_pfn(dev)) {
 			struct nd_pfn *nd_pfn = to_nd_pfn(dev);
@@ -463,6 +475,8 @@ static void nd_pmem_notify(struct device *dev, enum nvdimm_event event)
 	res.start = nsio->res.start + offset;
 	res.end = nsio->res.end - end_trunc;
 	nvdimm_badblocks_populate(nd_region, bb, &res);
+	if (bb_state)
+		sysfs_notify_dirent(bb_state);
 }
 
 MODULE_ALIAS("pmem");
diff --git a/drivers/nvdimm/pmem.h b/drivers/nvdimm/pmem.h
index 7f4dbd7..c5917f0 100644
--- a/drivers/nvdimm/pmem.h
+++ b/drivers/nvdimm/pmem.h
@@ -17,6 +17,7 @@ struct pmem_device {
 	size_t			size;
 	/* trim size when namespace capacity has been section aligned */
 	u32			pfn_pad;
+	struct kernfs_node	*bb_state;
 	struct badblocks	bb;
 	struct dax_device	*dax_dev;
 	struct gendisk		*disk;
diff --git a/drivers/nvdimm/region.c b/drivers/nvdimm/region.c
index 869a886..ca94029 100644
--- a/drivers/nvdimm/region.c
+++ b/drivers/nvdimm/region.c
@@ -58,10 +58,16 @@ static int nd_region_probe(struct device *dev)
 
 		if (devm_init_badblocks(dev, &nd_region->bb))
 			return -ENODEV;
+		nd_region->bb_state = sysfs_get_dirent(nd_region->dev.kobj.sd,
+						       "badblocks");
+		if (nd_region->bb_state)
+			sysfs_put(nd_region->bb_state);
+		else
+			dev_warn(&nd_region->dev,
+				"sysfs_get_dirent 'badblocks' failed\n");
 		ndr_res.start = nd_region->ndr_start;
 		ndr_res.end = nd_region->ndr_start + nd_region->ndr_size - 1;
-		nvdimm_badblocks_populate(nd_region,
-				&nd_region->bb, &ndr_res);
+		nvdimm_badblocks_populate(nd_region, &nd_region->bb, &ndr_res);
 	}
 
 	nd_region->btt_seed = nd_btt_create(nd_region);
@@ -126,6 +132,8 @@ static void nd_region_notify(struct device *dev, enum nvdimm_event event)
 				nd_region->ndr_size - 1;
 			nvdimm_badblocks_populate(nd_region,
 					&nd_region->bb, &res);
+			if (nd_region->bb_state)
+				sysfs_notify_dirent(nd_region->bb_state);
 		}
 	}
 	device_for_each_child(dev, &event, child_notify);
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

[PATCH v2] libnvdimm, pmem: Add sysfs notifications to badblocks

[Toshi Kani]

Sysfs "badblocks" information may be updated during run-time that:
 - MCE, SCI, and sysfs "scrub" may add new bad blocks
 - Writes and ioctl() may clear bad blocks

Add support to send sysfs notifications to sysfs "badblocks" file
under region and pmem directories when their badblocks information
is re-evaluated (but is not necessarily changed) during run-time.

Signed-off-by: Toshi Kani <toshi.kani@hpe.com>
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: Vishal Verma <vishal.l.verma@intel.com>
Cc: Linda Knippers <linda.knippers@hpe.com>
---
v2: Send notifications for the clearing case
---
 drivers/nvdimm/bus.c    |    3 +++
 drivers/nvdimm/nd.h     |    1 +
 drivers/nvdimm/pmem.c   |   14 ++++++++++++++
 drivers/nvdimm/pmem.h   |    1 +
 drivers/nvdimm/region.c |   12 ++++++++++--
 5 files changed, 29 insertions(+), 2 deletions(-)

diff --git a/drivers/nvdimm/bus.c b/drivers/nvdimm/bus.c
index e9361bf..63ce50d 100644
--- a/drivers/nvdimm/bus.c
+++ b/drivers/nvdimm/bus.c
@@ -198,6 +198,9 @@ static int nvdimm_clear_badblocks_region(struct device *dev, void *data)
 	sector = (ctx->phys - nd_region->ndr_start) / 512;
 	badblocks_clear(&nd_region->bb, sector, ctx->cleared / 512);
 
+	if (nd_region->bb_state)
+		sysfs_notify_dirent(nd_region->bb_state);
+
 	return 0;
 }
 
diff --git a/drivers/nvdimm/nd.h b/drivers/nvdimm/nd.h
index 03852d7..4bb57ff 100644
--- a/drivers/nvdimm/nd.h
+++ b/drivers/nvdimm/nd.h
@@ -155,6 +155,7 @@ struct nd_region {
 	u64 ndr_start;
 	int id, num_lanes, ro, numa_node;
 	void *provider_data;
+	struct kernfs_node *bb_state;
 	struct badblocks bb;
 	struct nd_interleave_set *nd_set;
 	struct nd_percpu_lane __percpu *lane;
diff --git a/drivers/nvdimm/pmem.c b/drivers/nvdimm/pmem.c
index c544d46..6c14c72 100644
--- a/drivers/nvdimm/pmem.c
+++ b/drivers/nvdimm/pmem.c
@@ -68,6 +68,8 @@ static int pmem_clear_poison(struct pmem_device *pmem, phys_addr_t offset,
 				(unsigned long long) sector, cleared,
 				cleared > 1 ? "s" : "");
 		badblocks_clear(&pmem->bb, sector, cleared);
+		if (pmem->bb_state)
+			sysfs_notify_dirent(pmem->bb_state);
 	}
 
 	invalidate_pmem(pmem->virt_addr + offset, len);
@@ -377,6 +379,13 @@ static int pmem_attach_disk(struct device *dev,
 
 	revalidate_disk(disk);
 
+	pmem->bb_state = sysfs_get_dirent(disk_to_dev(disk)->kobj.sd,
+					  "badblocks");
+	if (pmem->bb_state)
+		sysfs_put(pmem->bb_state);
+	else
+		dev_warn(dev, "sysfs_get_dirent 'badblocks' failed\n");
+
 	return 0;
 }
 
@@ -428,6 +437,7 @@ static void nd_pmem_notify(struct device *dev, enum nvdimm_event event)
 	struct nd_namespace_io *nsio;
 	struct resource res;
 	struct badblocks *bb;
+	struct kernfs_node *bb_state;
 
 	if (event != NVDIMM_REVALIDATE_POISON)
 		return;
@@ -439,11 +449,13 @@ static void nd_pmem_notify(struct device *dev, enum nvdimm_event event)
 		nd_region = to_nd_region(ndns->dev.parent);
 		nsio = to_nd_namespace_io(&ndns->dev);
 		bb = &nsio->bb;
+		bb_state = NULL;
 	} else {
 		struct pmem_device *pmem = dev_get_drvdata(dev);
 
 		nd_region = to_region(pmem);
 		bb = &pmem->bb;
+		bb_state = pmem->bb_state;
 
 		if (is_nd_pfn(dev)) {
 			struct nd_pfn *nd_pfn = to_nd_pfn(dev);
@@ -463,6 +475,8 @@ static void nd_pmem_notify(struct device *dev, enum nvdimm_event event)
 	res.start = nsio->res.start + offset;
 	res.end = nsio->res.end - end_trunc;
 	nvdimm_badblocks_populate(nd_region, bb, &res);
+	if (bb_state)
+		sysfs_notify_dirent(bb_state);
 }
 
 MODULE_ALIAS("pmem");
diff --git a/drivers/nvdimm/pmem.h b/drivers/nvdimm/pmem.h
index 7f4dbd7..c5917f0 100644
--- a/drivers/nvdimm/pmem.h
+++ b/drivers/nvdimm/pmem.h
@@ -17,6 +17,7 @@ struct pmem_device {
 	size_t			size;
 	/* trim size when namespace capacity has been section aligned */
 	u32			pfn_pad;
+	struct kernfs_node	*bb_state;
 	struct badblocks	bb;
 	struct dax_device	*dax_dev;
 	struct gendisk		*disk;
diff --git a/drivers/nvdimm/region.c b/drivers/nvdimm/region.c
index 869a886..ca94029 100644
--- a/drivers/nvdimm/region.c
+++ b/drivers/nvdimm/region.c
@@ -58,10 +58,16 @@ static int nd_region_probe(struct device *dev)
 
 		if (devm_init_badblocks(dev, &nd_region->bb))
 			return -ENODEV;
+		nd_region->bb_state = sysfs_get_dirent(nd_region->dev.kobj.sd,
+						       "badblocks");
+		if (nd_region->bb_state)
+			sysfs_put(nd_region->bb_state);
+		else
+			dev_warn(&nd_region->dev,
+				"sysfs_get_dirent 'badblocks' failed\n");
 		ndr_res.start = nd_region->ndr_start;
 		ndr_res.end = nd_region->ndr_start + nd_region->ndr_size - 1;
-		nvdimm_badblocks_populate(nd_region,
-				&nd_region->bb, &ndr_res);
+		nvdimm_badblocks_populate(nd_region, &nd_region->bb, &ndr_res);
 	}
 
 	nd_region->btt_seed = nd_btt_create(nd_region);
@@ -126,6 +132,8 @@ static void nd_region_notify(struct device *dev, enum nvdimm_event event)
 				nd_region->ndr_size - 1;
 			nvdimm_badblocks_populate(nd_region,
 					&nd_region->bb, &res);
+			if (nd_region->bb_state)
+				sysfs_notify_dirent(nd_region->bb_state);
 		}
 	}
 	device_for_each_child(dev, &event, child_notify);

Re: [PATCH v2] libnvdimm, pmem: Add sysfs notifications to badblocks

[Dan Williams]

On Mon, Jun 12, 2017 at 3:25 PM, Toshi Kani <toshi.kani@hpe.com> wrote:
> Sysfs "badblocks" information may be updated during run-time that:
>  - MCE, SCI, and sysfs "scrub" may add new bad blocks
>  - Writes and ioctl() may clear bad blocks
>
> Add support to send sysfs notifications to sysfs "badblocks" file
> under region and pmem directories when their badblocks information
> is re-evaluated (but is not necessarily changed) during run-time.
>
> Signed-off-by: Toshi Kani <toshi.kani@hpe.com>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Cc: Vishal Verma <vishal.l.verma@intel.com>
> Cc: Linda Knippers <linda.knippers@hpe.com>
> ---
> v2: Send notifications for the clearing case
> ---

This looks good to me, I've applied it, but I also want to extend the
ndctl unit tests to cover this mechanism.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH v2] libnvdimm, pmem: Add sysfs notifications to badblocks

[Dan Williams]

On Mon, Jun 12, 2017 at 3:25 PM, Toshi Kani <toshi.kani@hpe.com> wrote:
> Sysfs "badblocks" information may be updated during run-time that:
>  - MCE, SCI, and sysfs "scrub" may add new bad blocks
>  - Writes and ioctl() may clear bad blocks
>
> Add support to send sysfs notifications to sysfs "badblocks" file
> under region and pmem directories when their badblocks information
> is re-evaluated (but is not necessarily changed) during run-time.
>
> Signed-off-by: Toshi Kani <toshi.kani@hpe.com>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Cc: Vishal Verma <vishal.l.verma@intel.com>
> Cc: Linda Knippers <linda.knippers@hpe.com>
> ---
> v2: Send notifications for the clearing case
> ---

This looks good to me, I've applied it, but I also want to extend the
ndctl unit tests to cover this mechanism.

RE: [PATCH v2] libnvdimm, pmem: Add sysfs notifications to badblocks

[Kani, Toshimitsu]

[-- Attachment #1: Type: text/plain, Size: 1233 bytes --]

> On Mon, Jun 12, 2017 at 3:25 PM, Toshi Kani <toshi.kani@hpe.com> wrote:
> > Sysfs "badblocks" information may be updated during run-time that:
> >  - MCE, SCI, and sysfs "scrub" may add new bad blocks
> >  - Writes and ioctl() may clear bad blocks
> >
> > Add support to send sysfs notifications to sysfs "badblocks" file
> > under region and pmem directories when their badblocks information
> > is re-evaluated (but is not necessarily changed) during run-time.
> >
> > Signed-off-by: Toshi Kani <toshi.kani@hpe.com>
> > Cc: Dan Williams <dan.j.williams@intel.com>
> > Cc: Vishal Verma <vishal.l.verma@intel.com>
> > Cc: Linda Knippers <linda.knippers@hpe.com>
> > ---
> > v2: Send notifications for the clearing case
> > ---
> 
> This looks good to me, I've applied it, but I also want to extend the
> ndctl unit tests to cover this mechanism.

Right.  For the time being, would you mind to use the attached test
program for your sanity tests?  It simply monitors sysfs notifications
and prints badblocks info...  Sorry for inconvenience.

Since I am not familiar with the ndctl unit tests and I am traveling for
the rest of the month, I may have to look into it after I am back.

Thanks!
-Toshi



[-- Attachment #2: test_sysfs_notify.c --]
[-- Type: text/plain, Size: 1160 bytes --]

/*
 * Copyright (C) 2017  Hewlett Packard Enterprise Development LP
 *
 * This program is free software; you can redistribute it and/or
 * modify it under the terms of the GNU General Public License
 * as published by the Free Software Foundation; either version 2
 * of the License, or (at your option) any later version.
 */
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <unistd.h>
#include <fcntl.h>
#include <sys/types.h> 
#include <sys/stat.h> 

int main(int argc, char **argv)
{
	int fd, ret;
	fd_set fds;
	char buf[256];

	if (argc != 2) {
		printf("USAGE: test_sysfs_notify badblocks-path\n");
		exit(1);
	}

	if ((fd = open(argv[1], O_RDONLY)) < 0) {
		printf("Unable to open %s\n", argv[1]);
		exit(1);
	}
	printf("Monitoring %s - ctl-c to stop\n", argv[1]);

	while (1) {
		memset(buf, 0, sizeof(buf));
		ret = lseek(fd, 0, SEEK_SET);
		ret = read(fd, buf, sizeof(buf));
		printf("%s\n", buf);

		FD_ZERO(&fds);
		FD_SET(fd, &fds);

		ret = select(fd + 1, NULL, NULL, &fds, NULL);
		if (ret <= 0) {
			printf("error (%d)\n", ret);
			exit(1);
		} else if (FD_ISSET(fd, &fds)) {
			printf("NOTIFIED!!\n");
		}
	}

	close(fd);
}

[-- Attachment #3: Type: text/plain, Size: 151 bytes --]

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

RE: [PATCH v2] libnvdimm, pmem: Add sysfs notifications to badblocks

[Kani, Toshimitsu]

[-- Attachment #1: Type: text/plain, Size: 1233 bytes --]

> On Mon, Jun 12, 2017 at 3:25 PM, Toshi Kani <toshi.kani@hpe.com> wrote:
> > Sysfs "badblocks" information may be updated during run-time that:
> >  - MCE, SCI, and sysfs "scrub" may add new bad blocks
> >  - Writes and ioctl() may clear bad blocks
> >
> > Add support to send sysfs notifications to sysfs "badblocks" file
> > under region and pmem directories when their badblocks information
> > is re-evaluated (but is not necessarily changed) during run-time.
> >
> > Signed-off-by: Toshi Kani <toshi.kani@hpe.com>
> > Cc: Dan Williams <dan.j.williams@intel.com>
> > Cc: Vishal Verma <vishal.l.verma@intel.com>
> > Cc: Linda Knippers <linda.knippers@hpe.com>
> > ---
> > v2: Send notifications for the clearing case
> > ---
> 
> This looks good to me, I've applied it, but I also want to extend the
> ndctl unit tests to cover this mechanism.

Right.  For the time being, would you mind to use the attached test
program for your sanity tests?  It simply monitors sysfs notifications
and prints badblocks info...  Sorry for inconvenience.

Since I am not familiar with the ndctl unit tests and I am traveling for
the rest of the month, I may have to look into it after I am back.

Thanks!
-Toshi



[-- Attachment #2: test_sysfs_notify.c --]
[-- Type: text/plain, Size: 1160 bytes --]

/*
 * Copyright (C) 2017  Hewlett Packard Enterprise Development LP
 *
 * This program is free software; you can redistribute it and/or
 * modify it under the terms of the GNU General Public License
 * as published by the Free Software Foundation; either version 2
 * of the License, or (at your option) any later version.
 */
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <unistd.h>
#include <fcntl.h>
#include <sys/types.h> 
#include <sys/stat.h> 

int main(int argc, char **argv)
{
	int fd, ret;
	fd_set fds;
	char buf[256];

	if (argc != 2) {
		printf("USAGE: test_sysfs_notify badblocks-path\n");
		exit(1);
	}

	if ((fd = open(argv[1], O_RDONLY)) < 0) {
		printf("Unable to open %s\n", argv[1]);
		exit(1);
	}
	printf("Monitoring %s - ctl-c to stop\n", argv[1]);

	while (1) {
		memset(buf, 0, sizeof(buf));
		ret = lseek(fd, 0, SEEK_SET);
		ret = read(fd, buf, sizeof(buf));
		printf("%s\n", buf);

		FD_ZERO(&fds);
		FD_SET(fd, &fds);

		ret = select(fd + 1, NULL, NULL, &fds, NULL);
		if (ret <= 0) {
			printf("error (%d)\n", ret);
			exit(1);
		} else if (FD_ISSET(fd, &fds)) {
			printf("NOTIFIED!!\n");
		}
	}

	close(fd);
}

Re: [PATCH v2] libnvdimm, pmem: Add sysfs notifications to badblocks

[Dan Williams]

On Fri, Jun 16, 2017 at 5:35 PM, Kani, Toshimitsu <toshi.kani@hpe.com> wrote:
>> On Mon, Jun 12, 2017 at 3:25 PM, Toshi Kani <toshi.kani@hpe.com> wrote:
>> > Sysfs "badblocks" information may be updated during run-time that:
>> >  - MCE, SCI, and sysfs "scrub" may add new bad blocks
>> >  - Writes and ioctl() may clear bad blocks
>> >
>> > Add support to send sysfs notifications to sysfs "badblocks" file
>> > under region and pmem directories when their badblocks information
>> > is re-evaluated (but is not necessarily changed) during run-time.
>> >
>> > Signed-off-by: Toshi Kani <toshi.kani@hpe.com>
>> > Cc: Dan Williams <dan.j.williams@intel.com>
>> > Cc: Vishal Verma <vishal.l.verma@intel.com>
>> > Cc: Linda Knippers <linda.knippers@hpe.com>
>> > ---
>> > v2: Send notifications for the clearing case
>> > ---
>>
>> This looks good to me, I've applied it, but I also want to extend the
>> ndctl unit tests to cover this mechanism.
>
> Right.  For the time being, would you mind to use the attached test
> program for your sanity tests?  It simply monitors sysfs notifications
> and prints badblocks info...  Sorry for inconvenience.
>
> Since I am not familiar with the ndctl unit tests and I am traveling for
> the rest of the month, I may have to look into it after I am back.
>

No worries, I'll take a look at integrating this. Have a nice trip!
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH v2] libnvdimm, pmem: Add sysfs notifications to badblocks

[Dan Williams]

On Fri, Jun 16, 2017 at 5:35 PM, Kani, Toshimitsu <toshi.kani@hpe.com> wrote:
>> On Mon, Jun 12, 2017 at 3:25 PM, Toshi Kani <toshi.kani@hpe.com> wrote:
>> > Sysfs "badblocks" information may be updated during run-time that:
>> >  - MCE, SCI, and sysfs "scrub" may add new bad blocks
>> >  - Writes and ioctl() may clear bad blocks
>> >
>> > Add support to send sysfs notifications to sysfs "badblocks" file
>> > under region and pmem directories when their badblocks information
>> > is re-evaluated (but is not necessarily changed) during run-time.
>> >
>> > Signed-off-by: Toshi Kani <toshi.kani@hpe.com>
>> > Cc: Dan Williams <dan.j.williams@intel.com>
>> > Cc: Vishal Verma <vishal.l.verma@intel.com>
>> > Cc: Linda Knippers <linda.knippers@hpe.com>
>> > ---
>> > v2: Send notifications for the clearing case
>> > ---
>>
>> This looks good to me, I've applied it, but I also want to extend the
>> ndctl unit tests to cover this mechanism.
>
> Right.  For the time being, would you mind to use the attached test
> program for your sanity tests?  It simply monitors sysfs notifications
> and prints badblocks info...  Sorry for inconvenience.
>
> Since I am not familiar with the ndctl unit tests and I am traveling for
> the rest of the month, I may have to look into it after I am back.
>

No worries, I'll take a look at integrating this. Have a nice trip!

RE: [PATCH v2] libnvdimm, pmem: Add sysfs notifications to badblocks

[Kani, Toshimitsu]

> On Fri, Jun 16, 2017 at 5:35 PM, Kani, Toshimitsu <toshi.kani@hpe.com> wrote:
> >> On Mon, Jun 12, 2017 at 3:25 PM, Toshi Kani <toshi.kani@hpe.com> wrote:
> >> > Sysfs "badblocks" information may be updated during run-time that:
> >> >  - MCE, SCI, and sysfs "scrub" may add new bad blocks
> >> >  - Writes and ioctl() may clear bad blocks
> >> >
> >> > Add support to send sysfs notifications to sysfs "badblocks" file
> >> > under region and pmem directories when their badblocks information
> >> > is re-evaluated (but is not necessarily changed) during run-time.
> >> >
> >> > Signed-off-by: Toshi Kani <toshi.kani@hpe.com>
> >> > Cc: Dan Williams <dan.j.williams@intel.com>
> >> > Cc: Vishal Verma <vishal.l.verma@intel.com>
> >> > Cc: Linda Knippers <linda.knippers@hpe.com>
> >> > ---
> >> > v2: Send notifications for the clearing case
> >> > ---
> >>
> >> This looks good to me, I've applied it, but I also want to extend the
> >> ndctl unit tests to cover this mechanism.
> >
> > Right.  For the time being, would you mind to use the attached test
> > program for your sanity tests?  It simply monitors sysfs notifications
> > and prints badblocks info...  Sorry for inconvenience.
> >
> > Since I am not familiar with the ndctl unit tests and I am traveling for
> > the rest of the month, I may have to look into it after I am back.
> >
> 
> No worries, I'll take a look at integrating this. Have a nice trip!

Thanks Dan!!  I really appreciate it! 
-Toshi
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

RE: [PATCH v2] libnvdimm, pmem: Add sysfs notifications to badblocks

[Kani, Toshimitsu]

> On Fri, Jun 16, 2017 at 5:35 PM, Kani, Toshimitsu <toshi.kani@hpe.com> wrote:
> >> On Mon, Jun 12, 2017 at 3:25 PM, Toshi Kani <toshi.kani@hpe.com> wrote:
> >> > Sysfs "badblocks" information may be updated during run-time that:
> >> >  - MCE, SCI, and sysfs "scrub" may add new bad blocks
> >> >  - Writes and ioctl() may clear bad blocks
> >> >
> >> > Add support to send sysfs notifications to sysfs "badblocks" file
> >> > under region and pmem directories when their badblocks information
> >> > is re-evaluated (but is not necessarily changed) during run-time.
> >> >
> >> > Signed-off-by: Toshi Kani <toshi.kani@hpe.com>
> >> > Cc: Dan Williams <dan.j.williams@intel.com>
> >> > Cc: Vishal Verma <vishal.l.verma@intel.com>
> >> > Cc: Linda Knippers <linda.knippers@hpe.com>
> >> > ---
> >> > v2: Send notifications for the clearing case
> >> > ---
> >>
> >> This looks good to me, I've applied it, but I also want to extend the
> >> ndctl unit tests to cover this mechanism.
> >
> > Right.  For the time being, would you mind to use the attached test
> > program for your sanity tests?  It simply monitors sysfs notifications
> > and prints badblocks info...  Sorry for inconvenience.
> >
> > Since I am not familiar with the ndctl unit tests and I am traveling for
> > the rest of the month, I may have to look into it after I am back.
> >
> 
> No worries, I'll take a look at integrating this. Have a nice trip!

Thanks Dan!!  I really appreciate it! 
-Toshi

Re: [PATCH v2] libnvdimm, pmem: Add sysfs notifications to badblocks

[Dan Williams]

On Mon, Jun 12, 2017 at 3:25 PM, Toshi Kani <toshi.kani@hpe.com> wrote:
> Sysfs "badblocks" information may be updated during run-time that:
>  - MCE, SCI, and sysfs "scrub" may add new bad blocks
>  - Writes and ioctl() may clear bad blocks
>
> Add support to send sysfs notifications to sysfs "badblocks" file
> under region and pmem directories when their badblocks information
> is re-evaluated (but is not necessarily changed) during run-time.
>
> Signed-off-by: Toshi Kani <toshi.kani@hpe.com>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Cc: Vishal Verma <vishal.l.verma@intel.com>
> Cc: Linda Knippers <linda.knippers@hpe.com>
> ---
> v2: Send notifications for the clearing case
> ---
>  drivers/nvdimm/bus.c    |    3 +++
>  drivers/nvdimm/nd.h     |    1 +
>  drivers/nvdimm/pmem.c   |   14 ++++++++++++++
>  drivers/nvdimm/pmem.h   |    1 +
>  drivers/nvdimm/region.c |   12 ++++++++++--
>  5 files changed, 29 insertions(+), 2 deletions(-)
>
[..]
> diff --git a/drivers/nvdimm/pmem.c b/drivers/nvdimm/pmem.c
> index c544d46..6c14c72 100644
> --- a/drivers/nvdimm/pmem.c
> +++ b/drivers/nvdimm/pmem.c
[..]
> @@ -377,6 +379,13 @@ static int pmem_attach_disk(struct device *dev,
>
>         revalidate_disk(disk);
>
> +       pmem->bb_state = sysfs_get_dirent(disk_to_dev(disk)->kobj.sd,
> +                                         "badblocks");
> +       if (pmem->bb_state)
> +               sysfs_put(pmem->bb_state);

Sorry I missed this on the first review, but this looks broken. We
need to hold the reference for as long as we might trigger
notifications, so the sysfs_put() should wait until
pmem_release_disk().

[..]
> diff --git a/drivers/nvdimm/region.c b/drivers/nvdimm/region.c
> index 869a886..ca94029 100644
> --- a/drivers/nvdimm/region.c
> +++ b/drivers/nvdimm/region.c
> @@ -58,10 +58,16 @@ static int nd_region_probe(struct device *dev)
>
>                 if (devm_init_badblocks(dev, &nd_region->bb))
>                         return -ENODEV;
> +               nd_region->bb_state = sysfs_get_dirent(nd_region->dev.kobj.sd,
> +                                                      "badblocks");
> +               if (nd_region->bb_state)
> +                       sysfs_put(nd_region->bb_state);

...same here. This should wait until we tear down the region.

I'll take a look at an incremental fix patch.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH v2] libnvdimm, pmem: Add sysfs notifications to badblocks

[Dan Williams]

On Mon, Jun 12, 2017 at 3:25 PM, Toshi Kani <toshi.kani@hpe.com> wrote:
> Sysfs "badblocks" information may be updated during run-time that:
>  - MCE, SCI, and sysfs "scrub" may add new bad blocks
>  - Writes and ioctl() may clear bad blocks
>
> Add support to send sysfs notifications to sysfs "badblocks" file
> under region and pmem directories when their badblocks information
> is re-evaluated (but is not necessarily changed) during run-time.
>
> Signed-off-by: Toshi Kani <toshi.kani@hpe.com>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Cc: Vishal Verma <vishal.l.verma@intel.com>
> Cc: Linda Knippers <linda.knippers@hpe.com>
> ---
> v2: Send notifications for the clearing case
> ---
>  drivers/nvdimm/bus.c    |    3 +++
>  drivers/nvdimm/nd.h     |    1 +
>  drivers/nvdimm/pmem.c   |   14 ++++++++++++++
>  drivers/nvdimm/pmem.h   |    1 +
>  drivers/nvdimm/region.c |   12 ++++++++++--
>  5 files changed, 29 insertions(+), 2 deletions(-)
>
[..]
> diff --git a/drivers/nvdimm/pmem.c b/drivers/nvdimm/pmem.c
> index c544d46..6c14c72 100644
> --- a/drivers/nvdimm/pmem.c
> +++ b/drivers/nvdimm/pmem.c
[..]
> @@ -377,6 +379,13 @@ static int pmem_attach_disk(struct device *dev,
>
>         revalidate_disk(disk);
>
> +       pmem->bb_state = sysfs_get_dirent(disk_to_dev(disk)->kobj.sd,
> +                                         "badblocks");
> +       if (pmem->bb_state)
> +               sysfs_put(pmem->bb_state);

Sorry I missed this on the first review, but this looks broken. We
need to hold the reference for as long as we might trigger
notifications, so the sysfs_put() should wait until
pmem_release_disk().

[..]
> diff --git a/drivers/nvdimm/region.c b/drivers/nvdimm/region.c
> index 869a886..ca94029 100644
> --- a/drivers/nvdimm/region.c
> +++ b/drivers/nvdimm/region.c
> @@ -58,10 +58,16 @@ static int nd_region_probe(struct device *dev)
>
>                 if (devm_init_badblocks(dev, &nd_region->bb))
>                         return -ENODEV;
> +               nd_region->bb_state = sysfs_get_dirent(nd_region->dev.kobj.sd,
> +                                                      "badblocks");
> +               if (nd_region->bb_state)
> +                       sysfs_put(nd_region->bb_state);

...same here. This should wait until we tear down the region.

I'll take a look at an incremental fix patch.

RE: [PATCH v2] libnvdimm, pmem: Add sysfs notifications to badblocks

[Kani, Toshimitsu]

> > Sysfs "badblocks" information may be updated during run-time that:
> >  - MCE, SCI, and sysfs "scrub" may add new bad blocks
> >  - Writes and ioctl() may clear bad blocks
> >
> > Add support to send sysfs notifications to sysfs "badblocks" file
> > under region and pmem directories when their badblocks information
> > is re-evaluated (but is not necessarily changed) during run-time.
> >
> > Signed-off-by: Toshi Kani <toshi.kani@hpe.com>
> > Cc: Dan Williams <dan.j.williams@intel.com>
> > Cc: Vishal Verma <vishal.l.verma@intel.com>
> > Cc: Linda Knippers <linda.knippers@hpe.com>
> > ---
> > v2: Send notifications for the clearing case
> > ---
> >  drivers/nvdimm/bus.c    |    3 +++
> >  drivers/nvdimm/nd.h     |    1 +
> >  drivers/nvdimm/pmem.c   |   14 ++++++++++++++
> >  drivers/nvdimm/pmem.h   |    1 +
> >  drivers/nvdimm/region.c |   12 ++++++++++--
> >  5 files changed, 29 insertions(+), 2 deletions(-)
> >
> [..]
> > diff --git a/drivers/nvdimm/pmem.c b/drivers/nvdimm/pmem.c
> > index c544d46..6c14c72 100644
> > --- a/drivers/nvdimm/pmem.c
> > +++ b/drivers/nvdimm/pmem.c
> [..]
> > @@ -377,6 +379,13 @@ static int pmem_attach_disk(struct device *dev,
> >
> >         revalidate_disk(disk);
> >
> > +       pmem->bb_state = sysfs_get_dirent(disk_to_dev(disk)->kobj.sd,
> > +                                         "badblocks");
> > +       if (pmem->bb_state)
> > +               sysfs_put(pmem->bb_state);
> 
> Sorry I missed this on the first review, but this looks broken. We
> need to hold the reference for as long as we might trigger
> notifications, so the sysfs_put() should wait until
> pmem_release_disk().

I see.
 
> [..]
> > diff --git a/drivers/nvdimm/region.c b/drivers/nvdimm/region.c
> > index 869a886..ca94029 100644
> > --- a/drivers/nvdimm/region.c
> > +++ b/drivers/nvdimm/region.c
> > @@ -58,10 +58,16 @@ static int nd_region_probe(struct device *dev)
> >
> >                 if (devm_init_badblocks(dev, &nd_region->bb))
> >                         return -ENODEV;
> > +               nd_region->bb_state = sysfs_get_dirent(nd_region->dev.kobj.sd,
> > +                                                      "badblocks");
> > +               if (nd_region->bb_state)
> > +                       sysfs_put(nd_region->bb_state);
> 
> ...same here. This should wait until we tear down the region.
> 
> I'll take a look at an incremental fix patch.

Thanks Dan!
-Toshi

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

RE: [PATCH v2] libnvdimm, pmem: Add sysfs notifications to badblocks

[Kani, Toshimitsu]

> > Sysfs "badblocks" information may be updated during run-time that:
> >  - MCE, SCI, and sysfs "scrub" may add new bad blocks
> >  - Writes and ioctl() may clear bad blocks
> >
> > Add support to send sysfs notifications to sysfs "badblocks" file
> > under region and pmem directories when their badblocks information
> > is re-evaluated (but is not necessarily changed) during run-time.
> >
> > Signed-off-by: Toshi Kani <toshi.kani@hpe.com>
> > Cc: Dan Williams <dan.j.williams@intel.com>
> > Cc: Vishal Verma <vishal.l.verma@intel.com>
> > Cc: Linda Knippers <linda.knippers@hpe.com>
> > ---
> > v2: Send notifications for the clearing case
> > ---
> >  drivers/nvdimm/bus.c    |    3 +++
> >  drivers/nvdimm/nd.h     |    1 +
> >  drivers/nvdimm/pmem.c   |   14 ++++++++++++++
> >  drivers/nvdimm/pmem.h   |    1 +
> >  drivers/nvdimm/region.c |   12 ++++++++++--
> >  5 files changed, 29 insertions(+), 2 deletions(-)
> >
> [..]
> > diff --git a/drivers/nvdimm/pmem.c b/drivers/nvdimm/pmem.c
> > index c544d46..6c14c72 100644
> > --- a/drivers/nvdimm/pmem.c
> > +++ b/drivers/nvdimm/pmem.c
> [..]
> > @@ -377,6 +379,13 @@ static int pmem_attach_disk(struct device *dev,
> >
> >         revalidate_disk(disk);
> >
> > +       pmem->bb_state = sysfs_get_dirent(disk_to_dev(disk)->kobj.sd,
> > +                                         "badblocks");
> > +       if (pmem->bb_state)
> > +               sysfs_put(pmem->bb_state);
> 
> Sorry I missed this on the first review, but this looks broken. We
> need to hold the reference for as long as we might trigger
> notifications, so the sysfs_put() should wait until
> pmem_release_disk().

I see.
 
> [..]
> > diff --git a/drivers/nvdimm/region.c b/drivers/nvdimm/region.c
> > index 869a886..ca94029 100644
> > --- a/drivers/nvdimm/region.c
> > +++ b/drivers/nvdimm/region.c
> > @@ -58,10 +58,16 @@ static int nd_region_probe(struct device *dev)
> >
> >                 if (devm_init_badblocks(dev, &nd_region->bb))
> >                         return -ENODEV;
> > +               nd_region->bb_state = sysfs_get_dirent(nd_region->dev.kobj.sd,
> > +                                                      "badblocks");
> > +               if (nd_region->bb_state)
> > +                       sysfs_put(nd_region->bb_state);
> 
> ...same here. This should wait until we tear down the region.
> 
> I'll take a look at an incremental fix patch.

Thanks Dan!
-Toshi