On Tue, 2015-12-08 at 13:18 -0800, Dan Williams wrote:

On Tue, Dec 8, 2015 at 1:08 PM, Verma, Vishal L
[off-list ref] wrote:

quoted

On Wed, 2015-12-09 at 08:03 +1100, NeilBrown wrote:

quoted

On Sat, Dec 05 2015, Verma, Vishal L wrote:

quoted

quoted

quoted

+int badblocks_clear(struct badblocks *bb, sector_t s, int
sectors)
+{

[...]

quoted

+#define DO_DEBUG 1

Why have this at all if it's unconditionally defined and
always
set.

Neil - any reason or anything you had in mind for this? Or is it
just an
artifact and can be removed.

Like the comment says:

      /* Allow clearing via sysfs *only* for testing/debugging.
       * Normally only a successful write may clear a badblock
       */

The DO_DEBUG define and ifdefs are documentation identifying bits
of
code that should be removed when it all seems to be working.
Maybe now is a good time to remove that code.

Hm, I think it would be nice to continue to have the ability to
clear
badblocks using sysfs at least for a while more, as we test the
various
error handling paths for NVDIMMS (Dan, thoughts?).

We could either remove it later or (I'm leaning towards) make it a
config option similar to FAIL_MAKE_REQUEST and friends..

"later" as in before v4.5-rc1?  We can always carry this debug feature
locally for testing.  We don't want userspace growing ABI attachments
to this capability now that it's more than just md tooling that will
see this.

Agreed. The following incremental patch removes sysfs support.
All the latest badblocks patches can also be found at:

git://git.kernel.org/pub/scm/linux/kernel/git/vishal/nvdimm.git gendisk-badblocks


8<-----
From 5f0e7ac31d27a132f314106f1db33af22fde03ed Mon Sep 17 00:00:00 2001
From: Vishal Verma <vishal.l.verma@intel.com>
Date: Tue, 8 Dec 2015 16:28:31 -0700
Subject: [PATCH v4 4/3] badblocks: remove support for clearing via sysfs

sysfs support for clearing badblocks was originally meant for testing
only. With the move to generalize the interface, remove this support so
that userspace doesn't start treating this as an ABI.

Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
---
 block/badblocks.c | 15 ---------------
 1 file changed, 15 deletions(-)

diff --git a/block/badblocks.c b/block/badblocks.c
index f0ac279..e5d2a91 100644
--- a/block/badblocks.c
+++ b/block/badblocks.c

@@ -503,16 +503,6 @@ ssize_t badblocks_store(struct badblocks *bb, const

char *page, size_t len,
 	int length;
 	char newline;
 
-	/* Allow clearing via sysfs *only* for testing/debugging.
-	 * Normally only a successful write may clear a badblock
-	 */
-	int clear = 0;
-
-	if (page[0] == '-') {
-		clear = 1;
-		page++;
-	}
-
 	switch (sscanf(page, "%llu %d%c", &sector, &length, &newline))
{
 	case 3:
 		if (newline != '\n')

@@ -525,11 +515,6 @@ ssize_t badblocks_store(struct badblocks *bb, const

char *page, size_t len,
 		return -EINVAL;
 	}
 
-	if (clear) {
-		badblocks_clear(bb, sector, length);
-		return len;
-	}
-
 	if (badblocks_set(bb, sector, length, !unack))
 		return -ENOSPC;
 	else
-- 
2.5.0