[RFC PATCH 0/2] Initial support for badblock checking in xfs

[RFC PATCH 0/2] Initial support for badblock checking in xfs

[Vishal Verma]

These are early/RFC patches to add badblock support in xfs.

Patch 1 should be relatively straightforward - it adds a notifier chain
to badblocks that filesystems can register with.

Patch 2 is the beginnings of xfs support. So far, I have the notifier
registration and building the initial badblock list happening in
xfs_mountfs. The next steps (and I may need some help with this as I'm
no (x)fs developer :)) are to add this badblocks info to the reverse
mapping tree, and then to check for it before accessing the media.

Right now, this just prints the sector numbers/counts/{added, removed}
to the kernel log, for both the initial list, and subsequent notifier
hits.

While I've tested this with a fake pmem device using libnvdimm's
nfit_test framework, it should also work using badblock injection with
any block device:

# mkfs.xfs -f /dev/<device>
# echo 122 1 > /sys/block/<device>/badblocks
# echo 124 1 > /sys/block/<device>/badblocks
# mount -t xfs /dev/<device> /mnt
... in log:
[  +8.803776] XFS (pmem7): Mounting V4 Filesystem
[  +0.009633] XFS (pmem7): Ending clean mount
[  +0.001655] XFS (pmem7): got badblocks: sector 122, count 1
[  +0.002018] XFS (pmem7): got badblocks: sector 124, count 1

# echo 132 5 | > /sys/block/<device>/badblocks
[Jun16 18:56] XFS (pmem7): xfs badblock added sector 132 (count 5)

This is all based on Darrik's rmap work at:
https://github.com/djwong/linux/tree/rmap-reflink-devel

Since this is based on a v4.5-rc kernel, it lacks pmem support for
clearing badblocks on zeroing/writing, so those parts can't easily
be tested yet. The clearing work is in 4.7-rs kernels, and once we
rebase to that, that should also be available.


Vishal Verma (2):
  block, badblocks: add a notifier for badblocks
  xfs: initial/partial support for badblocks

 block/badblocks.c         |  79 +++++++++++++++++++++++++++++++++--
 fs/xfs/xfs_linux.h        |   1 +
 fs/xfs/xfs_mount.c        | 104 ++++++++++++++++++++++++++++++++++++++++++++++
 fs/xfs/xfs_mount.h        |   1 +
 include/linux/badblocks.h |  19 +++++++++
 5 files changed, 201 insertions(+), 3 deletions(-)

-- 
2.5.5

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[RFC PATCH 1/2] block, badblocks: add a notifier for badblocks

[Vishal Verma]

Filesystems with reverse mapping data may wish to do their own badblock
checking if they can be more efficient. This lays the groundwork for
that by providing a notifier in badblocks that such filesystems may
subscribe to. The notification includes information for whether one or
more badblocks were added or removed, the start sector of the operation,
and the number of sectors affected.

Cc: Darrick J. Wong <darrick.wong@oracle.com>
Cc: Dave Chinner <david@fromorbit.com>
Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
---
 block/badblocks.c         | 79 +++++++++++++++++++++++++++++++++++++++++++++--
 include/linux/badblocks.h | 19 ++++++++++++
 2 files changed, 95 insertions(+), 3 deletions(-)

diff --git a/block/badblocks.c b/block/badblocks.c
index 7be53cb..f7cfece 100644
--- a/block/badblocks.c
+++ b/block/badblocks.c
@@ -16,6 +16,7 @@
  */
 
 #include <linux/badblocks.h>
+#include <linux/notifier.h>
 #include <linux/seqlock.h>
 #include <linux/device.h>
 #include <linux/kernel.h>
@@ -25,6 +26,67 @@
 #include <linux/slab.h>
 
 /**
+ * badblocks_notify - call badblocks notifier chain
+ * @bb: struct badblocks that is changing
+ * @op: badblocks operation
+ * @sector: old clk rate
+ *
+ * Triggers a notifier call chain on the badblocks change notification
+ * for 'bb'.  Passes a pointer to the struct badblocks and the operation,
+ * and affected sector. Intended to be called by internal badblocks code
+ * only. Returns whatever atomic_notifier_call_chain returns.
+ */
+static int badblocks_notify(struct badblocks *bb, unsigned long op, sector_t sector,
+		int count)
+{
+	struct bb_notifier_data bbn_data;
+
+	bbn_data.sector = sector;
+	bbn_data.count = count;
+
+	return atomic_notifier_call_chain(&bb->notifier, op, &bbn_data);
+}
+
+/**
+ * bb_notifier_register - add a badblocks change notifier
+ * @bb: struct badblocks * to watch
+ * @nb: struct notifier_block * with callback info
+ *
+ * Request notification when badblocks are added or cleared.
+ * This uses a notifier of the 'atomic' type.
+ *
+ * Returns -EINVAL if called with null arguments otherwise, passes along
+ * the return value of atomic_notifier_chain_register().
+ */
+int bb_notifier_register(struct badblocks *bb, struct notifier_block *nb)
+{
+	if (!bb || !nb)
+		return -EINVAL;
+
+	return atomic_notifier_chain_register(&bb->notifier, nb);
+}
+EXPORT_SYMBOL_GPL(bb_notifier_register);
+
+/**
+ * bb_notifier_unregister - remove a badblocks change notifier
+ * @bb: struct badblocks *
+ * @nb: struct notifier_block * with callback info
+ *
+ * Request no further notification for changes to the badblocks list.
+ *
+ * Returns -EINVAL if called with null arguments; otherwise, passes
+ * along the return value of atomic_notifier_chain_unregister().
+ */
+int bb_notifier_unregister(struct badblocks *bb, struct notifier_block *nb)
+{
+	if (!bb || !nb)
+		return -EINVAL;
+
+	return atomic_notifier_chain_unregister(&bb->notifier, nb);
+}
+EXPORT_SYMBOL_GPL(bb_notifier_unregister);
+
+/**
  * badblocks_check() - check a given range for bad sectors
  * @bb:		the badblocks structure that holds all badblock information
  * @s:		sector (start) at which to check for badblocks
@@ -152,9 +214,10 @@ int badblocks_set(struct badblocks *bb, sector_t s, int sectors,
 			int acknowledged)
 {
 	u64 *p;
-	int lo, hi;
-	int rv = 0;
+	int rv = 0, ret;
 	unsigned long flags;
+	sector_t saved_s = s;
+	int lo, hi, saved_count = sectors;
 
 	if (bb->shift < 0)
 		/* badblocks are disabled */
@@ -294,6 +357,11 @@ int badblocks_set(struct badblocks *bb, sector_t s, int sectors,
 	bb->changed = 1;
 	if (!acknowledged)
 		bb->unacked_exist = 1;
+
+	ret = badblocks_notify(bb, BB_ADD, saved_s, saved_count);
+	if (ret)
+		WARN(1, "Failure in badblocks notifier chain: %d\n", ret);
+
 	write_sequnlock_irqrestore(&bb->lock, flags);
 
 	return rv;
@@ -318,8 +386,9 @@ int badblocks_clear(struct badblocks *bb, sector_t s, int sectors)
 {
 	u64 *p;
 	int lo, hi;
+	sector_t saved_s = s;
 	sector_t target = s + sectors;
-	int rv = 0;
+	int rv = 0, ret, saved_count = sectors;
 
 	if (bb->shift > 0) {
 		/* When clearing we round the start up and the end down.
@@ -400,6 +469,9 @@ int badblocks_clear(struct badblocks *bb, sector_t s, int sectors)
 	}
 
 	bb->changed = 1;
+	ret = badblocks_notify(bb, BB_CLEAR, saved_s, saved_count);
+	if (ret)
+		WARN(1, "Failure in badblocks notifier chain: %d\n", ret);
 out:
 	write_sequnlock_irq(&bb->lock);
 	return rv;
@@ -541,6 +613,7 @@ static int __badblocks_init(struct device *dev, struct badblocks *bb,
 		return -ENOMEM;
 	}
 	seqlock_init(&bb->lock);
+	ATOMIC_INIT_NOTIFIER_HEAD(&bb->notifier);
 
 	return 0;
 }
diff --git a/include/linux/badblocks.h b/include/linux/badblocks.h
index c3bdf8c..67539f0 100644
--- a/include/linux/badblocks.h
+++ b/include/linux/badblocks.h
@@ -1,6 +1,7 @@
 #ifndef _LINUX_BADBLOCKS_H
 #define _LINUX_BADBLOCKS_H
 
+#include <linux/notifier.h>
 #include <linux/seqlock.h>
 #include <linux/device.h>
 #include <linux/kernel.h>
@@ -38,8 +39,26 @@ struct badblocks {
 	seqlock_t lock;
 	sector_t sector;
 	sector_t size;		/* in sectors */
+	struct atomic_notifier_head notifier; /* notifier for filesystems */
 };
 
+#define BB_ADD		0
+#define BB_CLEAR	1
+
+/**
+ * struct bb_notifier_data - data to pass to the notifier callback
+ * @bb: struct badblocks * being changed
+ * @sector: the 512B sector being added/removed
+ * @count: number of sectors after @sector being added/removed
+ */
+struct bb_notifier_data {
+	struct badblocks *bb;
+	sector_t sector;
+	int count;
+};
+
+int bb_notifier_register(struct badblocks *bb, struct notifier_block *nb);
+int bb_notifier_unregister(struct badblocks *bb, struct notifier_block *nb);
 int badblocks_check(struct badblocks *bb, sector_t s, int sectors,
 		   sector_t *first_bad, int *bad_sectors);
 int badblocks_set(struct badblocks *bb, sector_t s, int sectors,
-- 
2.5.5

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[RFC PATCH 2/2] xfs: initial/partial support for badblocks

[Vishal Verma]

RFC/WIP commit.

This adds the foollowing:
1. In xfs_mountfs(), get an initial badblocks list from gendisk's
badblocks infrastructure.
2. Register with the badblocks notifier to get updates for this disk's
badblocks

TODO:
1. Add badblocks info to the reverse mapping tree (and remove if a
badblock was cleared).
2. Before doing file IO, refer the rmap/badblocks to error out early if
the IO will attempt wo read a bad sector
3. Figure out interactions with mmap/DAX.

Cc: Darrick J. Wong <darrick.wong@oracle.com>
Cc: Dave Chinner <david@fromorbit.com>
Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
---
 fs/xfs/xfs_linux.h |   1 +
 fs/xfs/xfs_mount.c | 104 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 fs/xfs/xfs_mount.h |   1 +
 3 files changed, 106 insertions(+)

diff --git a/fs/xfs/xfs_linux.h b/fs/xfs/xfs_linux.h
index 7e749be..f66d181 100644
--- a/fs/xfs/xfs_linux.h
+++ b/fs/xfs/xfs_linux.h
@@ -78,6 +78,7 @@ typedef __u32			xfs_nlink_t;
 #include <linux/freezer.h>
 #include <linux/list_sort.h>
 #include <linux/ratelimit.h>
+#include <linux/badblocks.h>
 
 #include <asm/page.h>
 #include <asm/div64.h>
diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
index 5e68b2c..1a47737 100644
--- a/fs/xfs/xfs_mount.c
+++ b/fs/xfs/xfs_mount.c
@@ -618,6 +618,96 @@ xfs_default_resblks(xfs_mount_t *mp)
 	return resblks;
 }
 
+STATIC int
+xfs_notifier_call(
+	struct notifier_block	*nb,
+	unsigned long		action,
+	void			*data)
+{
+	struct bb_notifier_data *bb_data = data;
+	struct xfs_mount *mp;
+
+	mp = container_of(nb, struct xfs_mount, m_badblocks_nb);
+	/* TODO xfs_add_bb_to_rmap(mp, bb_data->sector, bb_data->count); */
+	xfs_warn(mp, "xfs badblock %s sector %lu (count %d)\n",
+		(action == BB_ADD)?"added":"cleared",
+		bb_data->sector, bb_data->count);
+	return 0;
+}
+
+STATIC int
+xfs_init_badblocks(struct xfs_mount *mp)
+{
+	struct badblocks *bb = mp->m_super->s_bdev->bd_disk->bb;
+	int done = 0, error = 0;
+	ssize_t len, off = 0;
+	char *p;
+
+	/*
+	 * TODO: get a list of known badblocks so far and process it.
+	 * Can we just parse the sysfs format that badblocks_show()
+	 * returns? That should be the fastest way to get this.
+	 * Something like: (Is this too hacky? Should we just do
+	 * badblocks_check() in a (rather large) loop?)
+	 */
+	p = kzalloc(PAGE_SIZE, GFP_KERNEL);
+	len = badblocks_show(bb, p, 0);
+	while (len) {
+		int count, n;
+		sector_t s;
+
+		/*
+		 * The sysfs badblocks format is multiple lines of:
+		 * "<sector> <count>"
+		 */
+		n = sscanf(p + off, "%lu %d\n%n", &s, &count, &done);
+		if (n < 2 || done < 3) {
+			error = -1;
+			break;
+		}
+		off += done;
+		len -= done;
+		xfs_warn(mp, "got badblocks: sector %ld, count %d", s, count);
+		/* TODO xfs_add_bb_to_rmap(mp, s, count); */
+	}
+	kfree(p);
+	if (error)
+		return error;
+
+	mp->m_badblocks_nb.notifier_call = xfs_notifier_call;
+	error = bb_notifier_register(bb, &mp->m_badblocks_nb);
+	if (error)
+		return error;
+
+	/*
+	 * TODO: There is probably a TOCTOU race hiding here - what if the
+	 * badblocks list gets updated before we register for notifications..
+	 * Can likely be solved by registering for notifications _first_ (the
+	 * above xfs_add_bb_to_rmap function has to be ready to accept new
+	 * blocks), then querying for the initial list (there could be overlap
+	 * here, shich the above function could handle), and then setting the
+	 * mount flag below.
+	 */
+
+	/*
+	 * TODO: set some flag (mount flag?) in xfs so that xfs knows
+	 * it will be doing error checking, and can possibly, later,
+	 * tell the block layer (possibly using a REQ_ flag in its IO
+	 * requests) not to do further badblock checking for those IOs.
+	 */
+
+	/* mp->m_flags |= XFS_MOUNT_FS_BADBLOCKS; */
+	return 0;
+}
+
+STATIC void
+xfs_badblocks_unmount(struct xfs_mount *mp)
+{
+	struct badblocks *bb = mp->m_super->s_bdev->bd_disk->bb;
+
+	bb_notifier_unregister(bb, &mp->m_badblocks_nb);
+}
+
 /*
  * This function does the following on an initial mount of a file system:
  *	- reads the superblock from disk and init the mount struct
@@ -955,6 +1045,19 @@ xfs_mountfs(
 	}
 
 	/*
+	 * Register with the badblocks notifier chain
+	 */
+	error = xfs_init_badblocks(mp);
+	if (error) {
+		xfs_warn(mp, "Unable to register to badblocks notifications\n");
+		/*
+		 * TODO is this a hard error or can we simply
+		 * warn and continue?
+		 */
+		goto out_rtunmount;
+	}
+
+	/*
 	 * Now we are mounted, reserve a small amount of unused space for
 	 * privileged transactions. This is needed so that transaction
 	 * space required for critical operations can dip into this pool
@@ -1085,6 +1188,7 @@ xfs_unmountfs(
 	xfs_log_unmount(mp);
 	xfs_da_unmount(mp);
 	xfs_uuid_unmount(mp);
+	xfs_badblocks_unmount(mp);
 
 #if defined(DEBUG)
 	xfs_errortag_clearall(mp, 0);
diff --git a/fs/xfs/xfs_mount.h b/fs/xfs/xfs_mount.h
index 0ca9244..f0d1111 100644
--- a/fs/xfs/xfs_mount.h
+++ b/fs/xfs/xfs_mount.h
@@ -139,6 +139,7 @@ typedef struct xfs_mount {
 						/* low free space thresholds */
 	struct xfs_kobj		m_kobj;
 	struct xstats		m_stats;	/* per-fs stats */
+	struct notifier_block	m_badblocks_nb;	/* disk badblocks notifier */
 
 	struct workqueue_struct *m_buf_workqueue;
 	struct workqueue_struct	*m_data_workqueue;
-- 
2.5.5

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [RFC PATCH 2/2] xfs: initial/partial support for badblocks

[Darrick J. Wong]

On Thu, Jun 16, 2016 at 07:03:39PM -0600, Vishal Verma wrote:
> RFC/WIP commit.
> 
> This adds the foollowing:
> 1. In xfs_mountfs(), get an initial badblocks list from gendisk's
> badblocks infrastructure.
> 2. Register with the badblocks notifier to get updates for this disk's
> badblocks
> 
> TODO:
> 1. Add badblocks info to the reverse mapping tree (and remove if a
> badblock was cleared).

Well... there are a number of things we /could/ do... theoretically one
could use the rmap info to find all the affected inodes and simply convert
that part of their extent trees to 'unwritten'.  Reads will just get zeroes,
and writes will remove the unwritten flag (for that file, anyway).  Will a
successful write trigger the BB_CLEAR notifier?

I guess you could punch out the bad blocks too... not clear if you'd want
all the owner files staying mapped to data they'll never get back.

We could also create another rmap "special owner" (XFS_RMAP_OWN_BAD?) for bad
blocks, so we'd know which parts are just plain bad.  It might be useful to
know that kind of thing.

I think earlier Dave was talking about adding a new 'badblocks' bit to both
the file extent records and the rmap records to signify that something's
wrong with the extent.  <shrug> I'll let him write about that.

> 2. Before doing file IO, refer the rmap/badblocks to error out early if
> the IO will attempt wo read a bad sector
> 3. Figure out interactions with mmap/DAX.

Woot.

> Cc: Darrick J. Wong <darrick.wong@oracle.com>
> Cc: Dave Chinner <david@fromorbit.com>
> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> ---
>  fs/xfs/xfs_linux.h |   1 +
>  fs/xfs/xfs_mount.c | 104 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  fs/xfs/xfs_mount.h |   1 +
>  3 files changed, 106 insertions(+)
> 
> diff --git a/fs/xfs/xfs_linux.h b/fs/xfs/xfs_linux.h
> index 7e749be..f66d181 100644
> --- a/fs/xfs/xfs_linux.h
> +++ b/fs/xfs/xfs_linux.h
> @@ -78,6 +78,7 @@ typedef __u32			xfs_nlink_t;
>  #include <linux/freezer.h>
>  #include <linux/list_sort.h>
>  #include <linux/ratelimit.h>
> +#include <linux/badblocks.h>
>  
>  #include <asm/page.h>
>  #include <asm/div64.h>
> diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
> index 5e68b2c..1a47737 100644
> --- a/fs/xfs/xfs_mount.c
> +++ b/fs/xfs/xfs_mount.c
> @@ -618,6 +618,96 @@ xfs_default_resblks(xfs_mount_t *mp)
>  	return resblks;
>  }
>  
> +STATIC int
> +xfs_notifier_call(
> +	struct notifier_block	*nb,
> +	unsigned long		action,
> +	void			*data)
> +{
> +	struct bb_notifier_data *bb_data = data;
> +	struct xfs_mount *mp;
> +
> +	mp = container_of(nb, struct xfs_mount, m_badblocks_nb);
> +	/* TODO xfs_add_bb_to_rmap(mp, bb_data->sector, bb_data->count); */
> +	xfs_warn(mp, "xfs badblock %s sector %lu (count %d)\n",
> +		(action == BB_ADD)?"added":"cleared",
> +		bb_data->sector, bb_data->count);
> +	return 0;

Probably a xfs_rmapbt_query_range here?

> +}
> +
> +STATIC int
> +xfs_init_badblocks(struct xfs_mount *mp)
> +{
> +	struct badblocks *bb = mp->m_super->s_bdev->bd_disk->bb;
> +	int done = 0, error = 0;
> +	ssize_t len, off = 0;
> +	char *p;
> +
> +	/*
> +	 * TODO: get a list of known badblocks so far and process it.
> +	 * Can we just parse the sysfs format that badblocks_show()
> +	 * returns? That should be the fastest way to get this.
> +	 * Something like: (Is this too hacky? Should we just do
> +	 * badblocks_check() in a (rather large) loop?)
> +	 */
> +	p = kzalloc(PAGE_SIZE, GFP_KERNEL);
> +	len = badblocks_show(bb, p, 0);
> +	while (len) {
> +		int count, n;
> +		sector_t s;
> +
> +		/*
> +		 * The sysfs badblocks format is multiple lines of:
> +		 * "<sector> <count>"
> +		 */
> +		n = sscanf(p + off, "%lu %d\n%n", &s, &count, &done);

Ick, there's got to be a better way to iterate the badblocks list than this.

It would be very nice to have a single function that deals with a change in
badness status, which would be called by both the notifier and the badblocks
iterator.

> +		if (n < 2 || done < 3) {
> +			error = -1;
> +			break;
> +		}
> +		off += done;
> +		len -= done;
> +		xfs_warn(mp, "got badblocks: sector %ld, count %d", s, count);
> +		/* TODO xfs_add_bb_to_rmap(mp, s, count); */
> +	}
> +	kfree(p);
> +	if (error)
> +		return error;
> +
> +	mp->m_badblocks_nb.notifier_call = xfs_notifier_call;
> +	error = bb_notifier_register(bb, &mp->m_badblocks_nb);
> +	if (error)
> +		return error;
> +
> +	/*
> +	 * TODO: There is probably a TOCTOU race hiding here - what if the
> +	 * badblocks list gets updated before we register for notifications..
> +	 * Can likely be solved by registering for notifications _first_ (the
> +	 * above xfs_add_bb_to_rmap function has to be ready to accept new
> +	 * blocks), then querying for the initial list (there could be overlap
> +	 * here, shich the above function could handle), and then setting the
> +	 * mount flag below.
> +	 */

(Yeah, pretty much. :))

> +
> +	/*
> +	 * TODO: set some flag (mount flag?) in xfs so that xfs knows
> +	 * it will be doing error checking, and can possibly, later,
> +	 * tell the block layer (possibly using a REQ_ flag in its IO
> +	 * requests) not to do further badblock checking for those IOs.
> +	 */
> +
> +	/* mp->m_flags |= XFS_MOUNT_FS_BADBLOCKS; */
> +	return 0;
> +}
> +
> +STATIC void
> +xfs_badblocks_unmount(struct xfs_mount *mp)
> +{
> +	struct badblocks *bb = mp->m_super->s_bdev->bd_disk->bb;
> +
> +	bb_notifier_unregister(bb, &mp->m_badblocks_nb);
> +}
> +
>  /*
>   * This function does the following on an initial mount of a file system:
>   *	- reads the superblock from disk and init the mount struct
> @@ -955,6 +1045,19 @@ xfs_mountfs(
>  	}
>  
>  	/*
> +	 * Register with the badblocks notifier chain
> +	 */
> +	error = xfs_init_badblocks(mp);
> +	if (error) {
> +		xfs_warn(mp, "Unable to register to badblocks notifications\n");
> +		/*
> +		 * TODO is this a hard error or can we simply
> +		 * warn and continue?
> +		 */
> +		goto out_rtunmount;
> +	}
> +
> +	/*
>  	 * Now we are mounted, reserve a small amount of unused space for
>  	 * privileged transactions. This is needed so that transaction
>  	 * space required for critical operations can dip into this pool
> @@ -1085,6 +1188,7 @@ xfs_unmountfs(
>  	xfs_log_unmount(mp);
>  	xfs_da_unmount(mp);
>  	xfs_uuid_unmount(mp);
> +	xfs_badblocks_unmount(mp);
>  
>  #if defined(DEBUG)
>  	xfs_errortag_clearall(mp, 0);
> diff --git a/fs/xfs/xfs_mount.h b/fs/xfs/xfs_mount.h
> index 0ca9244..f0d1111 100644
> --- a/fs/xfs/xfs_mount.h
> +++ b/fs/xfs/xfs_mount.h
> @@ -139,6 +139,7 @@ typedef struct xfs_mount {
>  						/* low free space thresholds */
>  	struct xfs_kobj		m_kobj;
>  	struct xstats		m_stats;	/* per-fs stats */
> +	struct notifier_block	m_badblocks_nb;	/* disk badblocks notifier */
>  
>  	struct workqueue_struct *m_buf_workqueue;
>  	struct workqueue_struct	*m_data_workqueue;
> -- 
> 2.5.5
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [RFC PATCH 2/2] xfs: initial/partial support for badblocks

[Vishal Verma]

On 06/16, Darrick J. Wong wrote:
> On Thu, Jun 16, 2016 at 07:03:39PM -0600, Vishal Verma wrote:
> > RFC/WIP commit.
> > 
> > This adds the foollowing:
> > 1. In xfs_mountfs(), get an initial badblocks list from gendisk's
> > badblocks infrastructure.
> > 2. Register with the badblocks notifier to get updates for this disk's
> > badblocks
> > 
> > TODO:
> > 1. Add badblocks info to the reverse mapping tree (and remove if a
> > badblock was cleared).
> 
> Well... there are a number of things we /could/ do... theoretically one
> could use the rmap info to find all the affected inodes and simply convert
> that part of their extent trees to 'unwritten'.  Reads will just get zeroes,

Hm, not sure we can do that - if a block became bad at some point,
subsequent reads _have to_ error out (-EIO) so that the user doesn't see
silent data corruption.

> and writes will remove the unwritten flag (for that file, anyway).  Will a
> successful write trigger the BB_CLEAR notifier?

Currently, a successful write that goes through the driver (i.e. not
DAX) will cause the badblock to get cleared, and trigger BB_CLEAR. DAX
is trickier, and currently DAX writes won't clear errors.

> 
> I guess you could punch out the bad blocks too... not clear if you'd want
> all the owner files staying mapped to data they'll never get back.

The block mappings don't have to be maintained, but some sort of a flag
needs to be kept that will allow us to return errors on reads like I
said above.

> 
> We could also create another rmap "special owner" (XFS_RMAP_OWN_BAD?) for bad
> blocks, so we'd know which parts are just plain bad.  It might be useful to
> know that kind of thing.
> 
> I think earlier Dave was talking about adding a new 'badblocks' bit to both
> the file extent records and the rmap records to signify that something's
> wrong with the extent.  <shrug> I'll let him write about that.
> 
> > 2. Before doing file IO, refer the rmap/badblocks to error out early if
> > the IO will attempt wo read a bad sector
> > 3. Figure out interactions with mmap/DAX.
> 
> Woot.
> 
> > Cc: Darrick J. Wong <darrick.wong@oracle.com>
> > Cc: Dave Chinner <david@fromorbit.com>
> > Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> > ---
> >  fs/xfs/xfs_linux.h |   1 +
> >  fs/xfs/xfs_mount.c | 104 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  fs/xfs/xfs_mount.h |   1 +
> >  3 files changed, 106 insertions(+)
> > 
> > diff --git a/fs/xfs/xfs_linux.h b/fs/xfs/xfs_linux.h
> > index 7e749be..f66d181 100644
> > --- a/fs/xfs/xfs_linux.h
> > +++ b/fs/xfs/xfs_linux.h
> > @@ -78,6 +78,7 @@ typedef __u32			xfs_nlink_t;
> >  #include <linux/freezer.h>
> >  #include <linux/list_sort.h>
> >  #include <linux/ratelimit.h>
> > +#include <linux/badblocks.h>
> >  
> >  #include <asm/page.h>
> >  #include <asm/div64.h>
> > diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
> > index 5e68b2c..1a47737 100644
> > --- a/fs/xfs/xfs_mount.c
> > +++ b/fs/xfs/xfs_mount.c
> > @@ -618,6 +618,96 @@ xfs_default_resblks(xfs_mount_t *mp)
> >  	return resblks;
> >  }
> >  
> > +STATIC int
> > +xfs_notifier_call(
> > +	struct notifier_block	*nb,
> > +	unsigned long		action,
> > +	void			*data)
> > +{
> > +	struct bb_notifier_data *bb_data = data;
> > +	struct xfs_mount *mp;
> > +
> > +	mp = container_of(nb, struct xfs_mount, m_badblocks_nb);
> > +	/* TODO xfs_add_bb_to_rmap(mp, bb_data->sector, bb_data->count); */
> > +	xfs_warn(mp, "xfs badblock %s sector %lu (count %d)\n",
> > +		(action == BB_ADD)?"added":"cleared",
> > +		bb_data->sector, bb_data->count);
> > +	return 0;
> 
> Probably a xfs_rmapbt_query_range here?
> 
> > +}
> > +
> > +STATIC int
> > +xfs_init_badblocks(struct xfs_mount *mp)
> > +{
> > +	struct badblocks *bb = mp->m_super->s_bdev->bd_disk->bb;
> > +	int done = 0, error = 0;
> > +	ssize_t len, off = 0;
> > +	char *p;
> > +
> > +	/*
> > +	 * TODO: get a list of known badblocks so far and process it.
> > +	 * Can we just parse the sysfs format that badblocks_show()
> > +	 * returns? That should be the fastest way to get this.
> > +	 * Something like: (Is this too hacky? Should we just do
> > +	 * badblocks_check() in a (rather large) loop?)
> > +	 */
> > +	p = kzalloc(PAGE_SIZE, GFP_KERNEL);
> > +	len = badblocks_show(bb, p, 0);
> > +	while (len) {
> > +		int count, n;
> > +		sector_t s;
> > +
> > +		/*
> > +		 * The sysfs badblocks format is multiple lines of:
> > +		 * "<sector> <count>"
> > +		 */
> > +		n = sscanf(p + off, "%lu %d\n%n", &s, &count, &done);
> 
> Ick, there's got to be a better way to iterate the badblocks list than this.
> 
> It would be very nice to have a single function that deals with a change in
> badness status, which would be called by both the notifier and the badblocks
> iterator.
> 
There is a function in block/badblocks.c - badblocks_check() which can
tell you for a given range, what is the first bad sector in that range,
and the number of bad sectors after that first sector. Using that would
certainly be cleaner, as it uses the well-defined API, but it would be
much slower, as you have to iterate through the entire address space.
The above function - badblocks_show - just looks at the actual stored
representation of the badblocks, and lists them out, which is quick
(reading a 4K page from memory), and so in the above, I just parse it..

> > +		if (n < 2 || done < 3) {
> > +			error = -1;
> > +			break;
> > +		}
> > +		off += done;
> > +		len -= done;
> > +		xfs_warn(mp, "got badblocks: sector %ld, count %d", s, count);
> > +		/* TODO xfs_add_bb_to_rmap(mp, s, count); */
> > +	}
> > +	kfree(p);
> > +	if (error)
> > +		return error;
> > +
> > +	mp->m_badblocks_nb.notifier_call = xfs_notifier_call;
> > +	error = bb_notifier_register(bb, &mp->m_badblocks_nb);
> > +	if (error)
> > +		return error;
> > +
> > +	/*
> > +	 * TODO: There is probably a TOCTOU race hiding here - what if the
> > +	 * badblocks list gets updated before we register for notifications..
> > +	 * Can likely be solved by registering for notifications _first_ (the
> > +	 * above xfs_add_bb_to_rmap function has to be ready to accept new
> > +	 * blocks), then querying for the initial list (there could be overlap
> > +	 * here, shich the above function could handle), and then setting the
> > +	 * mount flag below.
> > +	 */
> 
> (Yeah, pretty much. :))
> 
> > +
> > +	/*
> > +	 * TODO: set some flag (mount flag?) in xfs so that xfs knows
> > +	 * it will be doing error checking, and can possibly, later,
> > +	 * tell the block layer (possibly using a REQ_ flag in its IO
> > +	 * requests) not to do further badblock checking for those IOs.
> > +	 */
> > +
> > +	/* mp->m_flags |= XFS_MOUNT_FS_BADBLOCKS; */
> > +	return 0;
> > +}
> > +
> > +STATIC void
> > +xfs_badblocks_unmount(struct xfs_mount *mp)
> > +{
> > +	struct badblocks *bb = mp->m_super->s_bdev->bd_disk->bb;
> > +
> > +	bb_notifier_unregister(bb, &mp->m_badblocks_nb);
> > +}
> > +
> >  /*
> >   * This function does the following on an initial mount of a file system:
> >   *	- reads the superblock from disk and init the mount struct
> > @@ -955,6 +1045,19 @@ xfs_mountfs(
> >  	}
> >  
> >  	/*
> > +	 * Register with the badblocks notifier chain
> > +	 */
> > +	error = xfs_init_badblocks(mp);
> > +	if (error) {
> > +		xfs_warn(mp, "Unable to register to badblocks notifications\n");
> > +		/*
> > +		 * TODO is this a hard error or can we simply
> > +		 * warn and continue?
> > +		 */
> > +		goto out_rtunmount;
> > +	}
> > +
> > +	/*
> >  	 * Now we are mounted, reserve a small amount of unused space for
> >  	 * privileged transactions. This is needed so that transaction
> >  	 * space required for critical operations can dip into this pool
> > @@ -1085,6 +1188,7 @@ xfs_unmountfs(
> >  	xfs_log_unmount(mp);
> >  	xfs_da_unmount(mp);
> >  	xfs_uuid_unmount(mp);
> > +	xfs_badblocks_unmount(mp);
> >  
> >  #if defined(DEBUG)
> >  	xfs_errortag_clearall(mp, 0);
> > diff --git a/fs/xfs/xfs_mount.h b/fs/xfs/xfs_mount.h
> > index 0ca9244..f0d1111 100644
> > --- a/fs/xfs/xfs_mount.h
> > +++ b/fs/xfs/xfs_mount.h
> > @@ -139,6 +139,7 @@ typedef struct xfs_mount {
> >  						/* low free space thresholds */
> >  	struct xfs_kobj		m_kobj;
> >  	struct xstats		m_stats;	/* per-fs stats */
> > +	struct notifier_block	m_badblocks_nb;	/* disk badblocks notifier */
> >  
> >  	struct workqueue_struct *m_buf_workqueue;
> >  	struct workqueue_struct	*m_data_workqueue;
> > -- 
> > 2.5.5
> > 
> > _______________________________________________
> > xfs mailing list
> > xfs@oss.sgi.com
> > http://oss.sgi.com/mailman/listinfo/xfs

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [RFC PATCH 2/2] xfs: initial/partial support for badblocks

[Darrick J. Wong]

On Fri, Jun 17, 2016 at 01:26:47PM -0600, Vishal Verma wrote:
> On 06/16, Darrick J. Wong wrote:
> > On Thu, Jun 16, 2016 at 07:03:39PM -0600, Vishal Verma wrote:
> > > RFC/WIP commit.
> > > 
> > > This adds the foollowing:
> > > 1. In xfs_mountfs(), get an initial badblocks list from gendisk's
> > > badblocks infrastructure.
> > > 2. Register with the badblocks notifier to get updates for this disk's
> > > badblocks
> > > 
> > > TODO:
> > > 1. Add badblocks info to the reverse mapping tree (and remove if a
> > > badblock was cleared).
> > 
> > Well... there are a number of things we /could/ do... theoretically one
> > could use the rmap info to find all the affected inodes and simply convert
> > that part of their extent trees to 'unwritten'.  Reads will just get zeroes,
> 
> Hm, not sure we can do that - if a block became bad at some point,
> subsequent reads _have to_ error out (-EIO) so that the user doesn't see
> silent data corruption.

Ooh, right, forgot about that whole MCE-on-bad-read thing...

> > and writes will remove the unwritten flag (for that file, anyway).  Will a
> > successful write trigger the BB_CLEAR notifier?
> 
> Currently, a successful write that goes through the driver (i.e. not
> DAX) will cause the badblock to get cleared, and trigger BB_CLEAR. DAX
> is trickier, and currently DAX writes won't clear errors.

Oh...?

> > 
> > I guess you could punch out the bad blocks too... not clear if you'd want
> > all the owner files staying mapped to data they'll never get back.
> 
> The block mappings don't have to be maintained, but some sort of a flag
> needs to be kept that will allow us to return errors on reads like I
> said above.

What happens if pmem signals a bad block and we try to read anyway?  I know
about the read-error-MCE thing, but what's the "more expensive" alternative?
CPU exception?

(Really what I'm digging at is, if the whole thing becomes a soft exception
that we can handle in the OS by, say, 2020, do we care about reintroducing
the whole ext2 badblocks thing?)

(Beats me...)

--D

> 
> > 
> > We could also create another rmap "special owner" (XFS_RMAP_OWN_BAD?) for bad
> > blocks, so we'd know which parts are just plain bad.  It might be useful to
> > know that kind of thing.
> > 
> > I think earlier Dave was talking about adding a new 'badblocks' bit to both
> > the file extent records and the rmap records to signify that something's
> > wrong with the extent.  <shrug> I'll let him write about that.
> > 
> > > 2. Before doing file IO, refer the rmap/badblocks to error out early if
> > > the IO will attempt wo read a bad sector
> > > 3. Figure out interactions with mmap/DAX.
> > 
> > Woot.
> > 
> > > Cc: Darrick J. Wong <darrick.wong@oracle.com>
> > > Cc: Dave Chinner <david@fromorbit.com>
> > > Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> > > ---
> > >  fs/xfs/xfs_linux.h |   1 +
> > >  fs/xfs/xfs_mount.c | 104 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> > >  fs/xfs/xfs_mount.h |   1 +
> > >  3 files changed, 106 insertions(+)
> > > 
> > > diff --git a/fs/xfs/xfs_linux.h b/fs/xfs/xfs_linux.h
> > > index 7e749be..f66d181 100644
> > > --- a/fs/xfs/xfs_linux.h
> > > +++ b/fs/xfs/xfs_linux.h
> > > @@ -78,6 +78,7 @@ typedef __u32			xfs_nlink_t;
> > >  #include <linux/freezer.h>
> > >  #include <linux/list_sort.h>
> > >  #include <linux/ratelimit.h>
> > > +#include <linux/badblocks.h>
> > >  
> > >  #include <asm/page.h>
> > >  #include <asm/div64.h>
> > > diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
> > > index 5e68b2c..1a47737 100644
> > > --- a/fs/xfs/xfs_mount.c
> > > +++ b/fs/xfs/xfs_mount.c
> > > @@ -618,6 +618,96 @@ xfs_default_resblks(xfs_mount_t *mp)
> > >  	return resblks;
> > >  }
> > >  
> > > +STATIC int
> > > +xfs_notifier_call(
> > > +	struct notifier_block	*nb,
> > > +	unsigned long		action,
> > > +	void			*data)
> > > +{
> > > +	struct bb_notifier_data *bb_data = data;
> > > +	struct xfs_mount *mp;
> > > +
> > > +	mp = container_of(nb, struct xfs_mount, m_badblocks_nb);
> > > +	/* TODO xfs_add_bb_to_rmap(mp, bb_data->sector, bb_data->count); */
> > > +	xfs_warn(mp, "xfs badblock %s sector %lu (count %d)\n",
> > > +		(action == BB_ADD)?"added":"cleared",
> > > +		bb_data->sector, bb_data->count);
> > > +	return 0;
> > 
> > Probably a xfs_rmapbt_query_range here?
> > 
> > > +}
> > > +
> > > +STATIC int
> > > +xfs_init_badblocks(struct xfs_mount *mp)
> > > +{
> > > +	struct badblocks *bb = mp->m_super->s_bdev->bd_disk->bb;
> > > +	int done = 0, error = 0;
> > > +	ssize_t len, off = 0;
> > > +	char *p;
> > > +
> > > +	/*
> > > +	 * TODO: get a list of known badblocks so far and process it.
> > > +	 * Can we just parse the sysfs format that badblocks_show()
> > > +	 * returns? That should be the fastest way to get this.
> > > +	 * Something like: (Is this too hacky? Should we just do
> > > +	 * badblocks_check() in a (rather large) loop?)
> > > +	 */
> > > +	p = kzalloc(PAGE_SIZE, GFP_KERNEL);
> > > +	len = badblocks_show(bb, p, 0);
> > > +	while (len) {
> > > +		int count, n;
> > > +		sector_t s;
> > > +
> > > +		/*
> > > +		 * The sysfs badblocks format is multiple lines of:
> > > +		 * "<sector> <count>"
> > > +		 */
> > > +		n = sscanf(p + off, "%lu %d\n%n", &s, &count, &done);
> > 
> > Ick, there's got to be a better way to iterate the badblocks list than this.
> > 
> > It would be very nice to have a single function that deals with a change in
> > badness status, which would be called by both the notifier and the badblocks
> > iterator.
> > 
> There is a function in block/badblocks.c - badblocks_check() which can
> tell you for a given range, what is the first bad sector in that range,
> and the number of bad sectors after that first sector. Using that would
> certainly be cleaner, as it uses the well-defined API, but it would be
> much slower, as you have to iterate through the entire address space.
> The above function - badblocks_show - just looks at the actual stored
> representation of the badblocks, and lists them out, which is quick
> (reading a 4K page from memory), and so in the above, I just parse it..
> 
> > > +		if (n < 2 || done < 3) {
> > > +			error = -1;
> > > +			break;
> > > +		}
> > > +		off += done;
> > > +		len -= done;
> > > +		xfs_warn(mp, "got badblocks: sector %ld, count %d", s, count);
> > > +		/* TODO xfs_add_bb_to_rmap(mp, s, count); */
> > > +	}
> > > +	kfree(p);
> > > +	if (error)
> > > +		return error;
> > > +
> > > +	mp->m_badblocks_nb.notifier_call = xfs_notifier_call;
> > > +	error = bb_notifier_register(bb, &mp->m_badblocks_nb);
> > > +	if (error)
> > > +		return error;
> > > +
> > > +	/*
> > > +	 * TODO: There is probably a TOCTOU race hiding here - what if the
> > > +	 * badblocks list gets updated before we register for notifications..
> > > +	 * Can likely be solved by registering for notifications _first_ (the
> > > +	 * above xfs_add_bb_to_rmap function has to be ready to accept new
> > > +	 * blocks), then querying for the initial list (there could be overlap
> > > +	 * here, shich the above function could handle), and then setting the
> > > +	 * mount flag below.
> > > +	 */
> > 
> > (Yeah, pretty much. :))
> > 
> > > +
> > > +	/*
> > > +	 * TODO: set some flag (mount flag?) in xfs so that xfs knows
> > > +	 * it will be doing error checking, and can possibly, later,
> > > +	 * tell the block layer (possibly using a REQ_ flag in its IO
> > > +	 * requests) not to do further badblock checking for those IOs.
> > > +	 */
> > > +
> > > +	/* mp->m_flags |= XFS_MOUNT_FS_BADBLOCKS; */
> > > +	return 0;
> > > +}
> > > +
> > > +STATIC void
> > > +xfs_badblocks_unmount(struct xfs_mount *mp)
> > > +{
> > > +	struct badblocks *bb = mp->m_super->s_bdev->bd_disk->bb;
> > > +
> > > +	bb_notifier_unregister(bb, &mp->m_badblocks_nb);
> > > +}
> > > +
> > >  /*
> > >   * This function does the following on an initial mount of a file system:
> > >   *	- reads the superblock from disk and init the mount struct
> > > @@ -955,6 +1045,19 @@ xfs_mountfs(
> > >  	}
> > >  
> > >  	/*
> > > +	 * Register with the badblocks notifier chain
> > > +	 */
> > > +	error = xfs_init_badblocks(mp);
> > > +	if (error) {
> > > +		xfs_warn(mp, "Unable to register to badblocks notifications\n");
> > > +		/*
> > > +		 * TODO is this a hard error or can we simply
> > > +		 * warn and continue?
> > > +		 */
> > > +		goto out_rtunmount;
> > > +	}
> > > +
> > > +	/*
> > >  	 * Now we are mounted, reserve a small amount of unused space for
> > >  	 * privileged transactions. This is needed so that transaction
> > >  	 * space required for critical operations can dip into this pool
> > > @@ -1085,6 +1188,7 @@ xfs_unmountfs(
> > >  	xfs_log_unmount(mp);
> > >  	xfs_da_unmount(mp);
> > >  	xfs_uuid_unmount(mp);
> > > +	xfs_badblocks_unmount(mp);
> > >  
> > >  #if defined(DEBUG)
> > >  	xfs_errortag_clearall(mp, 0);
> > > diff --git a/fs/xfs/xfs_mount.h b/fs/xfs/xfs_mount.h
> > > index 0ca9244..f0d1111 100644
> > > --- a/fs/xfs/xfs_mount.h
> > > +++ b/fs/xfs/xfs_mount.h
> > > @@ -139,6 +139,7 @@ typedef struct xfs_mount {
> > >  						/* low free space thresholds */
> > >  	struct xfs_kobj		m_kobj;
> > >  	struct xstats		m_stats;	/* per-fs stats */
> > > +	struct notifier_block	m_badblocks_nb;	/* disk badblocks notifier */
> > >  
> > >  	struct workqueue_struct *m_buf_workqueue;
> > >  	struct workqueue_struct	*m_data_workqueue;
> > > -- 
> > > 2.5.5
> > > 
> > > _______________________________________________
> > > xfs mailing list
> > > xfs@oss.sgi.com
> > > http://oss.sgi.com/mailman/listinfo/xfs

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [RFC PATCH 2/2] xfs: initial/partial support for badblocks

[Vishal Verma]

On 06/17, Darrick J. Wong wrote:
> On Fri, Jun 17, 2016 at 01:26:47PM -0600, Vishal Verma wrote:
> > On 06/16, Darrick J. Wong wrote:
> > > On Thu, Jun 16, 2016 at 07:03:39PM -0600, Vishal Verma wrote:
> > > > RFC/WIP commit.
> > > > 
> > > > This adds the foollowing:
> > > > 1. In xfs_mountfs(), get an initial badblocks list from gendisk's
> > > > badblocks infrastructure.
> > > > 2. Register with the badblocks notifier to get updates for this disk's
> > > > badblocks
> > > > 
> > > > TODO:
> > > > 1. Add badblocks info to the reverse mapping tree (and remove if a
> > > > badblock was cleared).
> > > 
> > > Well... there are a number of things we /could/ do... theoretically one
> > > could use the rmap info to find all the affected inodes and simply convert
> > > that part of their extent trees to 'unwritten'.  Reads will just get zeroes,
> > 
> > Hm, not sure we can do that - if a block became bad at some point,
> > subsequent reads _have to_ error out (-EIO) so that the user doesn't see
> > silent data corruption.
> 
> Ooh, right, forgot about that whole MCE-on-bad-read thing...
> 
> > > and writes will remove the unwritten flag (for that file, anyway).  Will a
> > > successful write trigger the BB_CLEAR notifier?
> > 
> > Currently, a successful write that goes through the driver (i.e. not
> > DAX) will cause the badblock to get cleared, and trigger BB_CLEAR. DAX
> > is trickier, and currently DAX writes won't clear errors.
> 
> Oh...?

I should expand.. post 4.7, DAX and badblocks can co-exist, and there
are a couple of scenarios:

1. When we take a DAX fault for the first time, we now check if the range
we're faulting for has bad blocks (see the is_bad_pmem check in
pmem_direct_access), and if it does, we signal a VM_FAULT_SIGBUS from the
fault handler.

2. If a latent error develops in a region that has been mmapped and faulted
in, then any attempts to load/store from/to it will cause a machine
check.

There isn't much that can be done for the second scenario - and the
dax-error-handling stuff in 4.7 should handle everything for the first
scenario (i.e. not cause a machine check induced crash). What we
probably still have to do is have a well defined recovery flow for an
application that gets a SIGBUS when accessing an mmaped file due to a
known bad block. Currently (4.7), what works is deleting, truncating,
or hole-punching the file, causing the sector (extent) to become
free/unwritten, and then a subsequent zeroout of it before the next time
it is used will go through the driver and clear the error.

The one thing missing is that When a filesystem is mounted with DAX, all
IO goes through dax_do_io, which is currently unable to clear any
errors. The ideal user experience, IMO, should be:
1. Get a SIGBUS accessing a mmaped file
2. Be told of an (offset+length) of the file that is 'gone'
3. Be able to open(), seek(), and write() to that offset to restore data
4. Be able to mmap etc. again and go on as usual

> 
> > > 
> > > I guess you could punch out the bad blocks too... not clear if you'd want
> > > all the owner files staying mapped to data they'll never get back.
> > 
> > The block mappings don't have to be maintained, but some sort of a flag
> > needs to be kept that will allow us to return errors on reads like I
> > said above.
> 
> What happens if pmem signals a bad block and we try to read anyway?  I know
> about the read-error-MCE thing, but what's the "more expensive" alternative?
> CPU exception?

MCE is the CPU exception, and is what we want to avoid at all costs, as
it can cause the machine to crash. Currently, the driver also checks
every IO (every bvec in fact) for badblocks before reading/writing.
Presumably, once we are able to push this checking into the filesystem,
we can signal the driver to stop doing this extra check (Dan points out
that it is good to have the redundant checking as a safety net till the
filesystem implementation is fully baked/tested). But yes in either of
these cases, we we know it is a bad block and choose to read (memcpy)
from it anyway, we get the machine check exception.

> 
> (Really what I'm digging at is, if the whole thing becomes a soft exception
> that we can handle in the OS by, say, 2020, do we care about reintroducing
> the whole ext2 badblocks thing?)

Not sure if this is something the OS can just learn to handle as the
machine check recovery stuff is a hardware/platform feature that only
some CPUs have. ('machine check recovery' allows the mce code in linux
to unmap the offending page(s) from the address space of any processes
that have it mapped, and sends them a SIGBUS). On CPUs without this
recovery feature, the CPU goes into a fault state and needs to be
restarted.

> 
> (Beats me...)
> 
> --D
> 
> > 
> > > 
> > > We could also create another rmap "special owner" (XFS_RMAP_OWN_BAD?) for bad
> > > blocks, so we'd know which parts are just plain bad.  It might be useful to
> > > know that kind of thing.
> > > 
> > > I think earlier Dave was talking about adding a new 'badblocks' bit to both
> > > the file extent records and the rmap records to signify that something's
> > > wrong with the extent.  <shrug> I'll let him write about that.
> > > 
> > > > 2. Before doing file IO, refer the rmap/badblocks to error out early if
> > > > the IO will attempt wo read a bad sector
> > > > 3. Figure out interactions with mmap/DAX.
> > > 
> > > Woot.
> > > 
> > > > Cc: Darrick J. Wong <darrick.wong@oracle.com>
> > > > Cc: Dave Chinner <david@fromorbit.com>
> > > > Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> > > > ---
> > > >  fs/xfs/xfs_linux.h |   1 +
> > > >  fs/xfs/xfs_mount.c | 104 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> > > >  fs/xfs/xfs_mount.h |   1 +
> > > >  3 files changed, 106 insertions(+)
> > > > 
> > > > diff --git a/fs/xfs/xfs_linux.h b/fs/xfs/xfs_linux.h
> > > > index 7e749be..f66d181 100644
> > > > --- a/fs/xfs/xfs_linux.h
> > > > +++ b/fs/xfs/xfs_linux.h
> > > > @@ -78,6 +78,7 @@ typedef __u32			xfs_nlink_t;
> > > >  #include <linux/freezer.h>
> > > >  #include <linux/list_sort.h>
> > > >  #include <linux/ratelimit.h>
> > > > +#include <linux/badblocks.h>
> > > >  
> > > >  #include <asm/page.h>
> > > >  #include <asm/div64.h>
> > > > diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
> > > > index 5e68b2c..1a47737 100644
> > > > --- a/fs/xfs/xfs_mount.c
> > > > +++ b/fs/xfs/xfs_mount.c
> > > > @@ -618,6 +618,96 @@ xfs_default_resblks(xfs_mount_t *mp)
> > > >  	return resblks;
> > > >  }
> > > >  
> > > > +STATIC int
> > > > +xfs_notifier_call(
> > > > +	struct notifier_block	*nb,
> > > > +	unsigned long		action,
> > > > +	void			*data)
> > > > +{
> > > > +	struct bb_notifier_data *bb_data = data;
> > > > +	struct xfs_mount *mp;
> > > > +
> > > > +	mp = container_of(nb, struct xfs_mount, m_badblocks_nb);
> > > > +	/* TODO xfs_add_bb_to_rmap(mp, bb_data->sector, bb_data->count); */
> > > > +	xfs_warn(mp, "xfs badblock %s sector %lu (count %d)\n",
> > > > +		(action == BB_ADD)?"added":"cleared",
> > > > +		bb_data->sector, bb_data->count);
> > > > +	return 0;
> > > 
> > > Probably a xfs_rmapbt_query_range here?
> > > 
> > > > +}
> > > > +
> > > > +STATIC int
> > > > +xfs_init_badblocks(struct xfs_mount *mp)
> > > > +{
> > > > +	struct badblocks *bb = mp->m_super->s_bdev->bd_disk->bb;
> > > > +	int done = 0, error = 0;
> > > > +	ssize_t len, off = 0;
> > > > +	char *p;
> > > > +
> > > > +	/*
> > > > +	 * TODO: get a list of known badblocks so far and process it.
> > > > +	 * Can we just parse the sysfs format that badblocks_show()
> > > > +	 * returns? That should be the fastest way to get this.
> > > > +	 * Something like: (Is this too hacky? Should we just do
> > > > +	 * badblocks_check() in a (rather large) loop?)
> > > > +	 */
> > > > +	p = kzalloc(PAGE_SIZE, GFP_KERNEL);
> > > > +	len = badblocks_show(bb, p, 0);
> > > > +	while (len) {
> > > > +		int count, n;
> > > > +		sector_t s;
> > > > +
> > > > +		/*
> > > > +		 * The sysfs badblocks format is multiple lines of:
> > > > +		 * "<sector> <count>"
> > > > +		 */
> > > > +		n = sscanf(p + off, "%lu %d\n%n", &s, &count, &done);
> > > 
> > > Ick, there's got to be a better way to iterate the badblocks list than this.
> > > 
> > > It would be very nice to have a single function that deals with a change in
> > > badness status, which would be called by both the notifier and the badblocks
> > > iterator.
> > > 
> > There is a function in block/badblocks.c - badblocks_check() which can
> > tell you for a given range, what is the first bad sector in that range,
> > and the number of bad sectors after that first sector. Using that would
> > certainly be cleaner, as it uses the well-defined API, but it would be
> > much slower, as you have to iterate through the entire address space.
> > The above function - badblocks_show - just looks at the actual stored
> > representation of the badblocks, and lists them out, which is quick
> > (reading a 4K page from memory), and so in the above, I just parse it..
> > 
> > > > +		if (n < 2 || done < 3) {
> > > > +			error = -1;
> > > > +			break;
> > > > +		}
> > > > +		off += done;
> > > > +		len -= done;
> > > > +		xfs_warn(mp, "got badblocks: sector %ld, count %d", s, count);
> > > > +		/* TODO xfs_add_bb_to_rmap(mp, s, count); */
> > > > +	}
> > > > +	kfree(p);
> > > > +	if (error)
> > > > +		return error;
> > > > +
> > > > +	mp->m_badblocks_nb.notifier_call = xfs_notifier_call;
> > > > +	error = bb_notifier_register(bb, &mp->m_badblocks_nb);
> > > > +	if (error)
> > > > +		return error;
> > > > +
> > > > +	/*
> > > > +	 * TODO: There is probably a TOCTOU race hiding here - what if the
> > > > +	 * badblocks list gets updated before we register for notifications..
> > > > +	 * Can likely be solved by registering for notifications _first_ (the
> > > > +	 * above xfs_add_bb_to_rmap function has to be ready to accept new
> > > > +	 * blocks), then querying for the initial list (there could be overlap
> > > > +	 * here, shich the above function could handle), and then setting the
> > > > +	 * mount flag below.
> > > > +	 */
> > > 
> > > (Yeah, pretty much. :))
> > > 
> > > > +
> > > > +	/*
> > > > +	 * TODO: set some flag (mount flag?) in xfs so that xfs knows
> > > > +	 * it will be doing error checking, and can possibly, later,
> > > > +	 * tell the block layer (possibly using a REQ_ flag in its IO
> > > > +	 * requests) not to do further badblock checking for those IOs.
> > > > +	 */
> > > > +
> > > > +	/* mp->m_flags |= XFS_MOUNT_FS_BADBLOCKS; */
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +STATIC void
> > > > +xfs_badblocks_unmount(struct xfs_mount *mp)
> > > > +{
> > > > +	struct badblocks *bb = mp->m_super->s_bdev->bd_disk->bb;
> > > > +
> > > > +	bb_notifier_unregister(bb, &mp->m_badblocks_nb);
> > > > +}
> > > > +
> > > >  /*
> > > >   * This function does the following on an initial mount of a file system:
> > > >   *	- reads the superblock from disk and init the mount struct
> > > > @@ -955,6 +1045,19 @@ xfs_mountfs(
> > > >  	}
> > > >  
> > > >  	/*
> > > > +	 * Register with the badblocks notifier chain
> > > > +	 */
> > > > +	error = xfs_init_badblocks(mp);
> > > > +	if (error) {
> > > > +		xfs_warn(mp, "Unable to register to badblocks notifications\n");
> > > > +		/*
> > > > +		 * TODO is this a hard error or can we simply
> > > > +		 * warn and continue?
> > > > +		 */
> > > > +		goto out_rtunmount;
> > > > +	}
> > > > +
> > > > +	/*
> > > >  	 * Now we are mounted, reserve a small amount of unused space for
> > > >  	 * privileged transactions. This is needed so that transaction
> > > >  	 * space required for critical operations can dip into this pool
> > > > @@ -1085,6 +1188,7 @@ xfs_unmountfs(
> > > >  	xfs_log_unmount(mp);
> > > >  	xfs_da_unmount(mp);
> > > >  	xfs_uuid_unmount(mp);
> > > > +	xfs_badblocks_unmount(mp);
> > > >  
> > > >  #if defined(DEBUG)
> > > >  	xfs_errortag_clearall(mp, 0);
> > > > diff --git a/fs/xfs/xfs_mount.h b/fs/xfs/xfs_mount.h
> > > > index 0ca9244..f0d1111 100644
> > > > --- a/fs/xfs/xfs_mount.h
> > > > +++ b/fs/xfs/xfs_mount.h
> > > > @@ -139,6 +139,7 @@ typedef struct xfs_mount {
> > > >  						/* low free space thresholds */
> > > >  	struct xfs_kobj		m_kobj;
> > > >  	struct xstats		m_stats;	/* per-fs stats */
> > > > +	struct notifier_block	m_badblocks_nb;	/* disk badblocks notifier */
> > > >  
> > > >  	struct workqueue_struct *m_buf_workqueue;
> > > >  	struct workqueue_struct	*m_data_workqueue;
> > > > -- 
> > > > 2.5.5
> > > > 
> > > > _______________________________________________
> > > > xfs mailing list
> > > > xfs@oss.sgi.com
> > > > http://oss.sgi.com/mailman/listinfo/xfs

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [RFC PATCH 2/2] xfs: initial/partial support for badblocks

[Dan Williams]

On Fri, Jun 17, 2016 at 1:32 PM, Vishal Verma <vishal.l.verma@intel.com> wrote:
>> What happens if pmem signals a bad block and we try to read anyway?  I know
>> about the read-error-MCE thing, but what's the "more expensive" alternative?
>> CPU exception?
>
> MCE is the CPU exception, and is what we want to avoid at all costs, as
> it can cause the machine to crash.

The MCE being fatal vs recoverable is the difference between platforms
that must avoid badblocks vs those that can consume them and keep
going.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [RFC PATCH 0/2] Initial support for badblock checking in xfs

[Darrick J. Wong]

On Thu, Jun 16, 2016 at 07:03:37PM -0600, Vishal Verma wrote:
> These are early/RFC patches to add badblock support in xfs.
> 
> Patch 1 should be relatively straightforward - it adds a notifier chain
> to badblocks that filesystems can register with.
> 
> Patch 2 is the beginnings of xfs support. So far, I have the notifier
> registration and building the initial badblock list happening in
> xfs_mountfs. The next steps (and I may need some help with this as I'm
> no (x)fs developer :)) are to add this badblocks info to the reverse
> mapping tree, and then to check for it before accessing the media.
> 
> Right now, this just prints the sector numbers/counts/{added, removed}
> to the kernel log, for both the initial list, and subsequent notifier
> hits.
> 
> While I've tested this with a fake pmem device using libnvdimm's
> nfit_test framework, it should also work using badblock injection with
> any block device:
> 
> # mkfs.xfs -f /dev/<device>
> # echo 122 1 > /sys/block/<device>/badblocks
> # echo 124 1 > /sys/block/<device>/badblocks
> # mount -t xfs /dev/<device> /mnt
> ... in log:
> [  +8.803776] XFS (pmem7): Mounting V4 Filesystem
> [  +0.009633] XFS (pmem7): Ending clean mount
> [  +0.001655] XFS (pmem7): got badblocks: sector 122, count 1
> [  +0.002018] XFS (pmem7): got badblocks: sector 124, count 1
> 
> # echo 132 5 | > /sys/block/<device>/badblocks
> [Jun16 18:56] XFS (pmem7): xfs badblock added sector 132 (count 5)
> 
> This is all based on Darrik's rmap work at:
> https://github.com/djwong/linux/tree/rmap-reflink-devel
> 
> Since this is based on a v4.5-rc kernel, it lacks pmem support for
> clearing badblocks on zeroing/writing, so those parts can't easily
> be tested yet. The clearing work is in 4.7-rs kernels, and once we
> rebase to that, that should also be available.

I'm assuming you got at least a few of the 300 patches I just sent.
IOW: The whole kit and kaboodle have been rebased to for-next, which
means the kernel patches are based on 4.7-rc3.

Will have a look at the other patches.

:)

--D

> 
> 
> Vishal Verma (2):
>   block, badblocks: add a notifier for badblocks
>   xfs: initial/partial support for badblocks
> 
>  block/badblocks.c         |  79 +++++++++++++++++++++++++++++++++--
>  fs/xfs/xfs_linux.h        |   1 +
>  fs/xfs/xfs_mount.c        | 104 ++++++++++++++++++++++++++++++++++++++++++++++
>  fs/xfs/xfs_mount.h        |   1 +
>  include/linux/badblocks.h |  19 +++++++++
>  5 files changed, 201 insertions(+), 3 deletions(-)
> 
> -- 
> 2.5.5
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [RFC PATCH 0/2] Initial support for badblock checking in xfs

[Vishal Verma]

On 06/16, Vishal Verma wrote:
> These are early/RFC patches to add badblock support in xfs.
> 
> Patch 1 should be relatively straightforward - it adds a notifier chain
> to badblocks that filesystems can register with.
> 
> Patch 2 is the beginnings of xfs support. So far, I have the notifier
> registration and building the initial badblock list happening in
> xfs_mountfs. The next steps (and I may need some help with this as I'm
> no (x)fs developer :)) are to add this badblocks info to the reverse
> mapping tree, and then to check for it before accessing the media.
> 
> Right now, this just prints the sector numbers/counts/{added, removed}
> to the kernel log, for both the initial list, and subsequent notifier
> hits.
> 
> While I've tested this with a fake pmem device using libnvdimm's
> nfit_test framework, it should also work using badblock injection with
> any block device:
> 
> # mkfs.xfs -f /dev/<device>
> # echo 122 1 > /sys/block/<device>/badblocks
> # echo 124 1 > /sys/block/<device>/badblocks
> # mount -t xfs /dev/<device> /mnt
> ... in log:
> [  +8.803776] XFS (pmem7): Mounting V4 Filesystem
> [  +0.009633] XFS (pmem7): Ending clean mount
> [  +0.001655] XFS (pmem7): got badblocks: sector 122, count 1
> [  +0.002018] XFS (pmem7): got badblocks: sector 124, count 1
> 
> # echo 132 5 | > /sys/block/<device>/badblocks
> [Jun16 18:56] XFS (pmem7): xfs badblock added sector 132 (count 5)
> 
> This is all based on Darrik's rmap work at:
> https://github.com/djwong/linux/tree/rmap-reflink-devel
> 
> Since this is based on a v4.5-rc kernel, it lacks pmem support for
> clearing badblocks on zeroing/writing, so those parts can't easily
> be tested yet. The clearing work is in 4.7-rs kernels, and once we
> rebase to that, that should also be available.

Just fyi, These patches should also cleanly apply to Darrick's for-next
rebase:
	https://github.com/djwong/linux/tree/djwong-devel

With this, I can now test badblock clearing also:

$ cat /sys/block/pmem7/badblocks 
122 1
124 1
126 1
128 1

$ sudo dd if=/dev/zero of=/dev/pmem7 bs=512 count=8 seek=120
oflag=direct
8+0 records in
8+0 records out
4096 bytes (4.1 kB) copied, 0.0206735 s, 198 kB/s

[  +9.510106] nd_pmem namespace7.0: pmem_clear_poison: 78 clear 1 sector
[  +0.001827] XFS (pmem7): xfs badblock cleared sector 120 (count 1)

[  +0.002772] nd_pmem namespace7.0: pmem_clear_poison: 7a clear 1 sector
[  +0.002527] XFS (pmem7): xfs badblock cleared sector 122 (count 1)

[  +0.003004] nd_pmem namespace7.0: pmem_clear_poison: 7c clear 1 sector
[  +0.002215] XFS (pmem7): xfs badblock cleared sector 124 (count 1)

[  +0.003063] nd_pmem namespace7.0: pmem_clear_poison: 7e clear 1 sector
[  +0.002307] XFS (pmem7): xfs badblock cleared sector 126 (count 1)


> 
> 
> Vishal Verma (2):
>   block, badblocks: add a notifier for badblocks
>   xfs: initial/partial support for badblocks
> 
>  block/badblocks.c         |  79 +++++++++++++++++++++++++++++++++--
>  fs/xfs/xfs_linux.h        |   1 +
>  fs/xfs/xfs_mount.c        | 104 ++++++++++++++++++++++++++++++++++++++++++++++
>  fs/xfs/xfs_mount.h        |   1 +
>  include/linux/badblocks.h |  19 +++++++++
>  5 files changed, 201 insertions(+), 3 deletions(-)
> 
> -- 
> 2.5.5
> 

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [RFC PATCH 0/2] Initial support for badblock checking in xfs

[Darrick J. Wong]

On Mon, Jun 20, 2016 at 12:48:27PM -0600, Vishal Verma wrote:
> On 06/16, Vishal Verma wrote:
> > These are early/RFC patches to add badblock support in xfs.
> > 
> > Patch 1 should be relatively straightforward - it adds a notifier chain
> > to badblocks that filesystems can register with.
> > 
> > Patch 2 is the beginnings of xfs support. So far, I have the notifier
> > registration and building the initial badblock list happening in
> > xfs_mountfs. The next steps (and I may need some help with this as I'm
> > no (x)fs developer :)) are to add this badblocks info to the reverse
> > mapping tree, and then to check for it before accessing the media.
> > 
> > Right now, this just prints the sector numbers/counts/{added, removed}
> > to the kernel log, for both the initial list, and subsequent notifier
> > hits.
> > 
> > While I've tested this with a fake pmem device using libnvdimm's
> > nfit_test framework, it should also work using badblock injection with
> > any block device:
> > 
> > # mkfs.xfs -f /dev/<device>
> > # echo 122 1 > /sys/block/<device>/badblocks
> > # echo 124 1 > /sys/block/<device>/badblocks
> > # mount -t xfs /dev/<device> /mnt
> > ... in log:
> > [  +8.803776] XFS (pmem7): Mounting V4 Filesystem
> > [  +0.009633] XFS (pmem7): Ending clean mount
> > [  +0.001655] XFS (pmem7): got badblocks: sector 122, count 1
> > [  +0.002018] XFS (pmem7): got badblocks: sector 124, count 1
> > 
> > # echo 132 5 | > /sys/block/<device>/badblocks
> > [Jun16 18:56] XFS (pmem7): xfs badblock added sector 132 (count 5)
> > 
> > This is all based on Darrik's rmap work at:
> > https://github.com/djwong/linux/tree/rmap-reflink-devel
> > 
> > Since this is based on a v4.5-rc kernel, it lacks pmem support for
> > clearing badblocks on zeroing/writing, so those parts can't easily
> > be tested yet. The clearing work is in 4.7-rs kernels, and once we
> > rebase to that, that should also be available.
> 
> Just fyi, These patches should also cleanly apply to Darrick's for-next
> rebase:
> 	https://github.com/djwong/linux/tree/djwong-devel
> 
> With this, I can now test badblock clearing also:
> 
> $ cat /sys/block/pmem7/badblocks 
> 122 1
> 124 1
> 126 1
> 128 1
> 
> $ sudo dd if=/dev/zero of=/dev/pmem7 bs=512 count=8 seek=120
> oflag=direct
> 8+0 records in
> 8+0 records out
> 4096 bytes (4.1 kB) copied, 0.0206735 s, 198 kB/s
> 
> [  +9.510106] nd_pmem namespace7.0: pmem_clear_poison: 78 clear 1 sector
> [  +0.001827] XFS (pmem7): xfs badblock cleared sector 120 (count 1)
> 
> [  +0.002772] nd_pmem namespace7.0: pmem_clear_poison: 7a clear 1 sector
> [  +0.002527] XFS (pmem7): xfs badblock cleared sector 122 (count 1)
> 
> [  +0.003004] nd_pmem namespace7.0: pmem_clear_poison: 7c clear 1 sector
> [  +0.002215] XFS (pmem7): xfs badblock cleared sector 124 (count 1)
> 
> [  +0.003063] nd_pmem namespace7.0: pmem_clear_poison: 7e clear 1 sector
> [  +0.002307] XFS (pmem7): xfs badblock cleared sector 126 (count 1)

I guess the next step looks something like this?

static int
xfs_whine_about_corruption(
	struct getfsmapx	*fmv,
	void			*priv)
{
	struct xfs_mount	*mp = priv;

	if (fmv->fmv_owner < 0) {
		xfs_err(mp, "fs metadata corrupt, bye bye!");
		/* XXX: maybe shut down now? */
		return 0;
	}

	xfs_err(mp, "inode %llu is corrupt at offset %llu length %u",
			fmv->fmv_owner, fmv->fmv_offset, fmv->fmv_length);
	return 0;
}

int
xfs_whine_about_badblocks(
	struct xfs_mount	*mp,
	sector_t		low,
	sector_t		high)
{
	struct getfsmapx	fmv[2];
	struct getfsmapx	*l, *h;
	int			error;

	memset(l, 0, sizeof(*l));
	memset(h, 0xFF, sizeof(*h));
	l = fmv;
	h = fmv + 1;
	l->fmv_block = low;
	h->fmv_block = high;
	l->fmv_count = 2;

	return xfs_getfsmap(mp, fmv, xfs_whine_about_corruption, mp);
}

static int
xfs_notifier_call(...)
{
	...all the stuff that's already there...
	xfs_whine_about_badblocks(mp, bb_data->sector,
			bb_data->sector + bb_data->count - 1);
}

(Yeah, you need rmap for this to do anything useful.)

--D

> 
> 
> > 
> > 
> > Vishal Verma (2):
> >   block, badblocks: add a notifier for badblocks
> >   xfs: initial/partial support for badblocks
> > 
> >  block/badblocks.c         |  79 +++++++++++++++++++++++++++++++++--
> >  fs/xfs/xfs_linux.h        |   1 +
> >  fs/xfs/xfs_mount.c        | 104 ++++++++++++++++++++++++++++++++++++++++++++++
> >  fs/xfs/xfs_mount.h        |   1 +
> >  include/linux/badblocks.h |  19 +++++++++
> >  5 files changed, 201 insertions(+), 3 deletions(-)
> > 
> > -- 
> > 2.5.5
> > 

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs