[PATCH 0/7] Configurable error behavior [V3]

[PATCH 0/7] Configurable error behavior [V3]

[Carlos Maiolino]

This is the new revision of this patchset, according to last comments.

This patchset is aimed to implement a configurable error behavior in XFS, and
most of the design has been done by Dave, so, that's why I kept his signed-off
in the patches.

This new revision has the detailed changelog written on each patch, but the
major changes are:

	- Detailed changelog by-patch and description fixed to become
	  (hopefuly) more clear
	- kept fail_at_unmount as a sysfs attribute


Regarding fail_at_unmount, I left it almost exactly as Dave's design, giving his
comments on the last revision, although, I still think there is no need to keep
it as a per-error granularity, so, I was wondering if a single, global option in
/sys/fs/xfs/<dev>/error/fail_at_unmount wouldn't suffice, but, this will require
a new place to store the value inside kernel, instead of keeping it inside
struct xfs_error_cfg, or maybe use the same structure but use it outside of the
m_error_cfg array?

First 6 patches are ready, the fail_at_unmount one, need to be re-worked if we
want it in a less granular way, but until now I don't think we reached any
decision about how it should be implemented.

 fs/xfs/xfs_buf.h      |  22 ++++
 fs/xfs/xfs_buf_item.c | 126 ++++++++++++++--------
 fs/xfs/xfs_mount.c    |  19 +++-
 fs/xfs/xfs_mount.h    |  32 ++++++
 fs/xfs/xfs_sysfs.c    | 283 +++++++++++++++++++++++++++++++++++++++++++++++++-
 fs/xfs/xfs_sysfs.h    |   3 +
 6 files changed, 437 insertions(+), 48 deletions(-)

-- 
2.4.11

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

[PATCH 1/7] xfs: configurable error behavior via sysfs

[Carlos Maiolino]

We need to be able to change the way XFS behaviours in error
conditions depending on the type of underlying storage. This is
necessary for handling non-traditional block devices with extended
error cases, such as thin provisioned devices that can return ENOSPC
as an IO error.

Introduce the basic sysfs infrastructure needed to define and
configure error behaviours. This is done to be generic enough to
extend to configuring behaviour in other error conditions, such as
ENOMEM, which also has different desired behaviours according to
machine configuration.

Changelog:

V3:
	- Rebase patch on top of linux-xfs tree
	- Remove XFS_ERR_FAIL enums, once we are not using them
	  to control the fail speed, but base it on max_retries value
	- Get rid of xfs_error_cfg.fail_speed field. We will use only
	  .max_retries to control the failure speed for each error

Signed-off-by: Dave Chinner <dchinner@redhat.com>
Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
---
 fs/xfs/xfs_mount.c | 10 +++++++++-
 fs/xfs/xfs_mount.h | 20 ++++++++++++++++++++
 fs/xfs/xfs_sysfs.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
 fs/xfs/xfs_sysfs.h |  3 +++
 4 files changed, 84 insertions(+), 2 deletions(-)

diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
index 654799f..eda3906 100644
--- a/fs/xfs/xfs_mount.c
+++ b/fs/xfs/xfs_mount.c
@@ -689,10 +689,15 @@ xfs_mountfs(
 	if (error)
 		goto out_remove_sysfs;
 
-	error = xfs_uuid_mount(mp);
+	error = xfs_error_sysfs_init(mp);
 	if (error)
 		goto out_del_stats;
 
+
+	error = xfs_uuid_mount(mp);
+	if (error)
+		goto out_remove_error_sysfs;
+
 	/*
 	 * Set the minimum read and write sizes
 	 */
@@ -967,6 +972,8 @@ xfs_mountfs(
 	xfs_da_unmount(mp);
  out_remove_uuid:
 	xfs_uuid_unmount(mp);
+ out_remove_error_sysfs:
+	xfs_error_sysfs_del(mp);
  out_del_stats:
 	xfs_sysfs_del(&mp->m_stats.xs_kobj);
  out_remove_sysfs:
@@ -1055,6 +1062,7 @@ xfs_unmountfs(
 #endif
 	xfs_free_perag(mp);
 
+	xfs_error_sysfs_del(mp);
 	xfs_sysfs_del(&mp->m_stats.xs_kobj);
 	xfs_sysfs_del(&mp->m_kobj);
 }
diff --git a/fs/xfs/xfs_mount.h b/fs/xfs/xfs_mount.h
index bac6b34..d639795 100644
--- a/fs/xfs/xfs_mount.h
+++ b/fs/xfs/xfs_mount.h
@@ -37,6 +37,24 @@ enum {
 	XFS_LOWSP_MAX,
 };
 
+/*
+ * Error Configuration
+ *
+ * Error classes define the subsystem the configuration belongs to.
+ * Error numbers define the errors that are configurable.
+ */
+enum {
+	XFS_ERR_CLASS_MAX,
+};
+enum {
+	XFS_ERR_ERRNO_MAX,
+};
+
+struct xfs_error_cfg {
+	struct xfs_kobj	kobj;
+	int		max_retries;
+};
+
 typedef struct xfs_mount {
 	struct super_block	*m_super;
 	xfs_tid_t		m_tid;		/* next unused tid for fs */
@@ -127,6 +145,8 @@ typedef struct xfs_mount {
 	int64_t			m_low_space[XFS_LOWSP_MAX];
 						/* low free space thresholds */
 	struct xfs_kobj		m_kobj;
+	struct xfs_kobj		m_error_kobj;
+	struct xfs_error_cfg	m_error_cfg[XFS_ERR_CLASS_MAX][XFS_ERR_ERRNO_MAX];
 	struct xstats		m_stats;	/* per-fs stats */
 
 	struct workqueue_struct *m_buf_workqueue;
diff --git a/fs/xfs/xfs_sysfs.c b/fs/xfs/xfs_sysfs.c
index 6ced4f1..b971113 100644
--- a/fs/xfs/xfs_sysfs.c
+++ b/fs/xfs/xfs_sysfs.c
@@ -17,10 +17,11 @@
  */
 
 #include "xfs.h"
-#include "xfs_sysfs.h"
+#include "xfs_shared.h"
 #include "xfs_format.h"
 #include "xfs_log_format.h"
 #include "xfs_trans_resv.h"
+#include "xfs_sysfs.h"
 #include "xfs_log.h"
 #include "xfs_log_priv.h"
 #include "xfs_stats.h"
@@ -362,3 +363,53 @@ struct kobj_type xfs_log_ktype = {
 	.sysfs_ops = &xfs_sysfs_ops,
 	.default_attrs = xfs_log_attrs,
 };
+
+/*
+ * Metadata IO error configuration
+ *
+ * The sysfs structure here is:
+ *	...xfs/<dev>/error/<class>/<errno>/<error_attrs>
+ *
+ * where <class> allows use to discriminate between data IO and metadata IO,
+ * and any other future type of IO (e.g. special inode or directory error
+ * handling) we care to support.
+ */
+static struct attribute *xfs_error_attrs[] = {
+	NULL,
+};
+
+static inline struct xfs_error_cfg *
+to_error_cfg(struct kobject *kobject)
+{
+	struct xfs_kobj *kobj = to_kobj(kobject);
+	return container_of(kobj, struct xfs_error_cfg, kobj);
+}
+
+struct kobj_type xfs_error_cfg_ktype = {
+	.release = xfs_sysfs_release,
+	.sysfs_ops = &xfs_sysfs_ops,
+	.default_attrs = xfs_error_attrs,
+};
+
+struct kobj_type xfs_error_ktype = {
+	.release = xfs_sysfs_release,
+};
+
+int
+xfs_error_sysfs_init(
+	struct xfs_mount	*mp)
+{
+	int			error;
+
+	/* .../xfs/<dev>/error/ */
+	error = xfs_sysfs_init(&mp->m_error_kobj, &xfs_error_ktype,
+				&mp->m_kobj, "error");
+	return error;
+}
+
+void
+xfs_error_sysfs_del(
+	struct xfs_mount	*mp)
+{
+	xfs_sysfs_del(&mp->m_error_kobj);
+}
diff --git a/fs/xfs/xfs_sysfs.h b/fs/xfs/xfs_sysfs.h
index be692e5..d046371 100644
--- a/fs/xfs/xfs_sysfs.h
+++ b/fs/xfs/xfs_sysfs.h
@@ -58,4 +58,7 @@ xfs_sysfs_del(
 	wait_for_completion(&kobj->complete);
 }
 
+int	xfs_error_sysfs_init(struct xfs_mount *mp);
+void	xfs_error_sysfs_del(struct xfs_mount *mp);
+
 #endif	/* __XFS_SYSFS_H__ */
-- 
2.4.11

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

[PATCH 2/7] xfs: introduce metadata IO error class

[Carlos Maiolino]

Now we have the basic infrastructure, add the first error class so
we can build up the infrastructure in a meaningful way. Add the
metadata async write IO error class and sysfs entry, and introduce a
default configuration that matches the existing "retry forever" behavior
for async write metadata buffers.

Changelog:

V3:
	- Use 'cfg->max_retries = -1' as the default configuration for the
	  "retry forever" behavior, instead of
	  cfg->fail_speed = XFS_ERR_FAIL_NEVER

Signed-off-by: Dave Chinner <dchinner@redhat.com>
Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
---
 fs/xfs/xfs_mount.h |  3 +++
 fs/xfs/xfs_sysfs.c | 34 ++++++++++++++++++++++++++++++++++
 2 files changed, 37 insertions(+)

diff --git a/fs/xfs/xfs_mount.h b/fs/xfs/xfs_mount.h
index d639795..352a5c8 100644
--- a/fs/xfs/xfs_mount.h
+++ b/fs/xfs/xfs_mount.h
@@ -44,9 +44,11 @@ enum {
  * Error numbers define the errors that are configurable.
  */
 enum {
+	XFS_ERR_METADATA,
 	XFS_ERR_CLASS_MAX,
 };
 enum {
+	XFS_ERR_DEFAULT,
 	XFS_ERR_ERRNO_MAX,
 };
 
@@ -146,6 +148,7 @@ typedef struct xfs_mount {
 						/* low free space thresholds */
 	struct xfs_kobj		m_kobj;
 	struct xfs_kobj		m_error_kobj;
+	struct xfs_kobj		m_error_meta_kobj;
 	struct xfs_error_cfg	m_error_cfg[XFS_ERR_CLASS_MAX][XFS_ERR_ERRNO_MAX];
 	struct xstats		m_stats;	/* per-fs stats */
 
diff --git a/fs/xfs/xfs_sysfs.c b/fs/xfs/xfs_sysfs.c
index b971113..7180cff 100644
--- a/fs/xfs/xfs_sysfs.c
+++ b/fs/xfs/xfs_sysfs.c
@@ -399,11 +399,34 @@ int
 xfs_error_sysfs_init(
 	struct xfs_mount	*mp)
 {
+	struct xfs_error_cfg	*cfg;
 	int			error;
 
 	/* .../xfs/<dev>/error/ */
 	error = xfs_sysfs_init(&mp->m_error_kobj, &xfs_error_ktype,
 				&mp->m_kobj, "error");
+	if (error)
+		return error;
+
+	/* .../xfs/<dev>/error/metadata/ */
+	error = xfs_sysfs_init(&mp->m_error_meta_kobj, &xfs_error_ktype,
+				&mp->m_error_kobj, "metadata");
+	if (error)
+		goto out_error;
+
+	cfg = &mp->m_error_cfg[XFS_ERR_METADATA][XFS_ERR_DEFAULT];
+	error = xfs_sysfs_init(&cfg->kobj, &xfs_error_cfg_ktype,
+				&mp->m_error_meta_kobj, "default");
+	if (error)
+		goto out_error_meta;
+	cfg->max_retries = -1;
+
+	return 0;
+
+out_error_meta:
+	xfs_sysfs_del(&mp->m_error_meta_kobj);
+out_error:
+	xfs_sysfs_del(&mp->m_error_kobj);
 	return error;
 }
 
@@ -411,5 +434,16 @@ void
 xfs_error_sysfs_del(
 	struct xfs_mount	*mp)
 {
+	struct xfs_error_cfg	*cfg;
+	int			i, j;
+
+	for (i = 0; i < XFS_ERR_CLASS_MAX; i++) {
+		for (j = 0; j < XFS_ERR_ERRNO_MAX; j++) {
+			cfg = &mp->m_error_cfg[i][j];
+
+			xfs_sysfs_del(&cfg->kobj);
+		}
+	}
+	xfs_sysfs_del(&mp->m_error_meta_kobj);
 	xfs_sysfs_del(&mp->m_error_kobj);
 }
-- 
2.4.11

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

[PATCH 3/7] xfs: add configurable error support to metadata buffers

[Carlos Maiolino]

With the error configuration handle for async metadata write errors
in place, we can now add initial support to the IO error processing
in xfs_buf_iodone_error().

Add an infrastructure function to look up the configuration handle,
and rearrange the error handling to prepare the way for different
error handling conigurations to be used.

Changelog:

V3:
	- in xfs_buf_iodone_callbacks, use cfg->max_retries to
	  determine if the filesystem should fail or not, a zero value
	  means it should never retry.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
---
 fs/xfs/xfs_buf.h      |   1 +
 fs/xfs/xfs_buf_item.c | 112 ++++++++++++++++++++++++++++++--------------------
 fs/xfs/xfs_mount.h    |   3 ++
 fs/xfs/xfs_sysfs.c    |  17 ++++++++
 4 files changed, 88 insertions(+), 45 deletions(-)

diff --git a/fs/xfs/xfs_buf.h b/fs/xfs/xfs_buf.h
index 4eb89bd..adef116 100644
--- a/fs/xfs/xfs_buf.h
+++ b/fs/xfs/xfs_buf.h
@@ -183,6 +183,7 @@ typedef struct xfs_buf {
 	unsigned int		b_page_count;	/* size of page array */
 	unsigned int		b_offset;	/* page offset in first page */
 	int			b_error;	/* error code on I/O */
+	int			b_last_error;	/* previous async I/O error */
 	const struct xfs_buf_ops	*b_ops;
 
 #ifdef XFS_BUF_LOCK_TRACKING
diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
index 99e91a0..3a30a88 100644
--- a/fs/xfs/xfs_buf_item.c
+++ b/fs/xfs/xfs_buf_item.c
@@ -1042,35 +1042,22 @@ xfs_buf_do_callbacks(
 	}
 }
 
-/*
- * This is the iodone() function for buffers which have had callbacks
- * attached to them by xfs_buf_attach_iodone().  It should remove each
- * log item from the buffer's list and call the callback of each in turn.
- * When done, the buffer's fsprivate field is set to NULL and the buffer
- * is unlocked with a call to iodone().
- */
-void
-xfs_buf_iodone_callbacks(
+static bool
+xfs_buf_iodone_callback_error(
 	struct xfs_buf		*bp)
 {
 	struct xfs_log_item	*lip = bp->b_fspriv;
 	struct xfs_mount	*mp = lip->li_mountp;
 	static ulong		lasttime;
 	static xfs_buftarg_t	*lasttarg;
-
-	if (likely(!bp->b_error))
-		goto do_callbacks;
+	struct xfs_error_cfg	*cfg;
 
 	/*
 	 * If we've already decided to shutdown the filesystem because of
 	 * I/O errors, there's no point in giving this a retry.
 	 */
-	if (XFS_FORCED_SHUTDOWN(mp)) {
-		xfs_buf_stale(bp);
-		bp->b_flags |= XBF_DONE;
-		trace_xfs_buf_item_iodone(bp, _RET_IP_);
-		goto do_callbacks;
-	}
+	if (XFS_FORCED_SHUTDOWN(mp))
+		goto out_stale;
 
 	if (bp->b_target != lasttarg ||
 	    time_after(jiffies, (lasttime + 5*HZ))) {
@@ -1079,45 +1066,80 @@ xfs_buf_iodone_callbacks(
 	}
 	lasttarg = bp->b_target;
 
+	/* synchronous writes will have callers process the error */
+	if (!(bp->b_flags & XBF_ASYNC))
+		goto out_stale;
+
+	trace_xfs_buf_item_iodone_async(bp, _RET_IP_);
+	ASSERT(bp->b_iodone != NULL);
+
 	/*
 	 * If the write was asynchronous then no one will be looking for the
-	 * error.  Clear the error state and write the buffer out again.
-	 *
-	 * XXX: This helps against transient write errors, but we need to find
-	 * a way to shut the filesystem down if the writes keep failing.
-	 *
-	 * In practice we'll shut the filesystem down soon as non-transient
-	 * errors tend to affect the whole device and a failing log write
-	 * will make us give up.  But we really ought to do better here.
+	 * error.  If this is the first failure of this type, clear the error
+	 * state and write the buffer out again. This means we always retry an
+	 * async write failure at least once, but we also need to set the buffer
+	 * up to behave correctly now for repeated failures.
 	 */
-	if (bp->b_flags & XBF_ASYNC) {
-		ASSERT(bp->b_iodone != NULL);
-
-		trace_xfs_buf_item_iodone_async(bp, _RET_IP_);
+	cfg = xfs_error_get_cfg(mp, XFS_ERR_METADATA, bp->b_error);
+	if (!(bp->b_flags & (XBF_STALE|XBF_WRITE_FAIL)) ||
+	     bp->b_last_error != bp->b_error) {
+		bp->b_flags |= (XBF_WRITE | XBF_ASYNC |
+			        XBF_DONE | XBF_WRITE_FAIL);
+		bp->b_last_error = bp->b_error;
+		xfs_buf_ioerror(bp, 0);
+		xfs_buf_submit(bp);
+		return true;
+	}
 
-		xfs_buf_ioerror(bp, 0); /* errno of 0 unsets the flag */
+	/*
+	 * Repeated failure on an async write. Take action according to the
+	 * error configuration we have been set up to use.
+	 */
+	if (!cfg->max_retries)
+		goto permanent_error;
 
-		if (!(bp->b_flags & (XBF_STALE|XBF_WRITE_FAIL))) {
-			bp->b_flags |= XBF_WRITE | XBF_ASYNC |
-				       XBF_DONE | XBF_WRITE_FAIL;
-			xfs_buf_submit(bp);
-		} else {
-			xfs_buf_relse(bp);
-		}
-
-		return;
-	}
+	/* still a transient error, higher layers will retry */
+	xfs_buf_ioerror(bp, 0);
+	xfs_buf_relse(bp);
+	return true;
 
 	/*
-	 * If the write of the buffer was synchronous, we want to make
-	 * sure to return the error to the caller of xfs_bwrite().
+	 * Permanent error - we need to trigger a shutdown if we haven't already
+	 * to indicate that inconsistency will result from this action.
 	 */
+permanent_error:
+	xfs_force_shutdown(mp, SHUTDOWN_META_IO_ERROR);
+out_stale:
 	xfs_buf_stale(bp);
 	bp->b_flags |= XBF_DONE;
-
 	trace_xfs_buf_error_relse(bp, _RET_IP_);
+	return false;
+}
+
+/*
+ * This is the iodone() function for buffers which have had callbacks attached
+ * to them by xfs_buf_attach_iodone(). We need to iterate the items on the
+ * callback list, mark the buffer as having no more callbacks and then push the
+ * buffer through IO completion processing.
+ */
+void
+xfs_buf_iodone_callbacks(
+	struct xfs_buf		*bp)
+{
+	/*
+	 * If there is an error, process it. Some errors require us
+	 * to run callbacks after failure processing is done so we
+	 * detect that and take appropriate action.
+	 */
+	if (bp->b_error && xfs_buf_iodone_callback_error(bp))
+		return;
+
+	/*
+	 * Successful IO or permanent error. Either way, we can clear the
+	 * retry state here in preparation for the next error that may occur.
+	 */
+	bp->b_last_error = 0;
 
-do_callbacks:
 	xfs_buf_do_callbacks(bp);
 	bp->b_fspriv = NULL;
 	bp->b_iodone = NULL;
diff --git a/fs/xfs/xfs_mount.h b/fs/xfs/xfs_mount.h
index 352a5c8..0c5a976 100644
--- a/fs/xfs/xfs_mount.h
+++ b/fs/xfs/xfs_mount.h
@@ -387,4 +387,7 @@ extern void	xfs_set_low_space_thresholds(struct xfs_mount *);
 int	xfs_zero_extent(struct xfs_inode *ip, xfs_fsblock_t start_fsb,
 			xfs_off_t count_fsb);
 
+struct xfs_error_cfg * xfs_error_get_cfg(struct xfs_mount *mp,
+		int error_class, int error);
+
 #endif	/* __XFS_MOUNT_H__ */
diff --git a/fs/xfs/xfs_sysfs.c b/fs/xfs/xfs_sysfs.c
index 7180cff..0a9adcd 100644
--- a/fs/xfs/xfs_sysfs.c
+++ b/fs/xfs/xfs_sysfs.c
@@ -447,3 +447,20 @@ xfs_error_sysfs_del(
 	xfs_sysfs_del(&mp->m_error_meta_kobj);
 	xfs_sysfs_del(&mp->m_error_kobj);
 }
+
+struct xfs_error_cfg *
+xfs_error_get_cfg(
+	struct xfs_mount	*mp,
+	int			error_class,
+	int			error)
+{
+	struct xfs_error_cfg	*cfg;
+
+	switch (error) {
+	default:
+		cfg = &mp->m_error_cfg[error_class][XFS_ERR_DEFAULT];
+		break;
+	}
+
+	return cfg;
+}
-- 
2.4.11

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

[PATCH 4/7] xfs: introduce table-based init for error behaviors

[Carlos Maiolino]

Before we start expanding the number of error classes and errors we
can configure behaviour for, we need a simple and clear way to
define the default behaviour that we initialized each mount with.
Introduce a table based method for keeping the initial configuration
in, and apply that to the existing initialization code.

Changelog:

V3:
	- Replace all .fail_speed fields by .max_retries, once the code will no
	  longer use .fail_speed to decide when it should fail
	- The "Default" attribute is also in lower-case (default). I don't
	  believe it's a good idea to have a mixed-case attribute names in sysfs.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
---
 fs/xfs/xfs_sysfs.c | 72 +++++++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 60 insertions(+), 12 deletions(-)

diff --git a/fs/xfs/xfs_sysfs.c b/fs/xfs/xfs_sysfs.c
index 0a9adcd..2a5b1cf 100644
--- a/fs/xfs/xfs_sysfs.c
+++ b/fs/xfs/xfs_sysfs.c
@@ -395,11 +395,67 @@ struct kobj_type xfs_error_ktype = {
 	.release = xfs_sysfs_release,
 };
 
+/*
+ * Error initialization tables. These need to be ordered in the same
+ * order as the enums used to index the array. All class init tables need to
+ * define a "default" behaviour as the first entry, all other entries can be
+ * empty.
+ */
+struct xfs_error_init {
+	char		*name;
+	int		max_retries;
+};
+
+static const struct xfs_error_init xfs_error_meta_init[XFS_ERR_ERRNO_MAX] = {
+	{ .name = "default",
+	  .max_retries = -1,
+	},
+};
+
+static int
+xfs_error_sysfs_init_class(
+	struct xfs_mount	*mp,
+	int			class,
+	const char		*parent_name,
+	struct xfs_kobj		*parent_kobj,
+	const struct xfs_error_init init[])
+{
+	struct xfs_error_cfg	*cfg;
+	int			error;
+	int			i;
+
+	ASSERT(class < XFS_ERR_CLASS_MAX);
+
+	error = xfs_sysfs_init(parent_kobj, &xfs_error_ktype,
+				&mp->m_error_kobj, parent_name);
+	if (error)
+		return error;
+
+	for (i = 0; i < XFS_ERR_ERRNO_MAX; i++) {
+		cfg = &mp->m_error_cfg[class][i];
+		error = xfs_sysfs_init(&cfg->kobj, &xfs_error_cfg_ktype,
+					parent_kobj, init[i].name);
+		if (error)
+			goto out_error;
+
+		cfg->max_retries = init[i].max_retries;
+	}
+	return 0;
+
+out_error:
+	/* unwind the entries that succeeded */
+	for (i--; i >= 0; i--) {
+		cfg = &mp->m_error_cfg[class][i];
+		xfs_sysfs_del(&cfg->kobj);
+	}
+	xfs_sysfs_del(parent_kobj);
+	return error;
+}
+
 int
 xfs_error_sysfs_init(
 	struct xfs_mount	*mp)
 {
-	struct xfs_error_cfg	*cfg;
 	int			error;
 
 	/* .../xfs/<dev>/error/ */
@@ -409,22 +465,14 @@ xfs_error_sysfs_init(
 		return error;
 
 	/* .../xfs/<dev>/error/metadata/ */
-	error = xfs_sysfs_init(&mp->m_error_meta_kobj, &xfs_error_ktype,
-				&mp->m_error_kobj, "metadata");
+	error = xfs_error_sysfs_init_class(mp, XFS_ERR_METADATA,
+				"metadata", &mp->m_error_meta_kobj,
+				xfs_error_meta_init);
 	if (error)
 		goto out_error;
 
-	cfg = &mp->m_error_cfg[XFS_ERR_METADATA][XFS_ERR_DEFAULT];
-	error = xfs_sysfs_init(&cfg->kobj, &xfs_error_cfg_ktype,
-				&mp->m_error_meta_kobj, "default");
-	if (error)
-		goto out_error_meta;
-	cfg->max_retries = -1;
-
 	return 0;
 
-out_error_meta:
-	xfs_sysfs_del(&mp->m_error_meta_kobj);
 out_error:
 	xfs_sysfs_del(&mp->m_error_kobj);
 	return error;
-- 
2.4.11

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

[PATCH 5/7] xfs: add configuration of error failure speed

[Carlos Maiolino]

On reception of an error, we can fail immediately, perform some
bound amount of retries or retry indefinitely. The current behaviour
we have is to retry forever.

However, we'd like the ability to choose how long the filesystem should try
after an error, it can either fail immediately, retry a few times, or retry
forever. This is implemented by using max_retries sysfs attribute, to hold the
amount of times we allow the filesystem to retry after an error. Being -1 a
special case where the filesystem will retry indefinitely.

Add both a maximum retry count and a retry timeout so that we can bound by
time and/or physical IO attempts.

Finally, plumb these into xfs_buf_iodone error processing so that
the error behaviour follows the selected configuration.

Changelog:

V3:
	- In xfs_buf_iodone_callback_error, use max_retries to decide how long
	  the filesystem should retry on errors instead of XFS_ERR_FAIL enums
	  and fail_speed

	- Remove all code implementing fail_speed attribute from the original
	  patch
		-- failure_speed_show/store attributes function implementation
		-- max_retries_store() now accepts values from -1 up to INT_MAX

	- retry_timeout_seconds_show() print fixed:
		-- jiffies_to_msecs() should be divided by MSEC_PER_SEC
		-- trailing whitespace removed


Signed-off-by: Dave Chinner <dchinner@redhat.com>
Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
---
 fs/xfs/xfs_buf.h      | 23 ++++++++++++++-
 fs/xfs/xfs_buf_item.c | 12 ++++++--
 fs/xfs/xfs_mount.h    |  3 +-
 fs/xfs/xfs_sysfs.c    | 81 ++++++++++++++++++++++++++++++++++++++++++++++++---
 4 files changed, 111 insertions(+), 8 deletions(-)

diff --git a/fs/xfs/xfs_buf.h b/fs/xfs/xfs_buf.h
index adef116..bd978f0 100644
--- a/fs/xfs/xfs_buf.h
+++ b/fs/xfs/xfs_buf.h
@@ -183,7 +183,28 @@ typedef struct xfs_buf {
 	unsigned int		b_page_count;	/* size of page array */
 	unsigned int		b_offset;	/* page offset in first page */
 	int			b_error;	/* error code on I/O */
-	int			b_last_error;	/* previous async I/O error */
+
+	/*
+	 * async write failure retry count. Initialised to zero on the first
+	 * failure, then when it exceeds the maximum configured without a
+	 * success the write is considered to be failed permanently and the
+	 * iodone handler will take appropriate action.
+	 *
+	 * For retry timeouts, we record the jiffie of the first failure. This
+	 * means that we can change the retry timeout and it on the next error
+	 * it will be checked against the newly configured timeout. This
+	 * prevents buffers getting stuck in retry loops with a really long
+	 * timeout.
+	 *
+	 * last_error is used to ensure that we are getting repeated errors, not
+	 * different errors. e.g. a block device might change ENOSPC to EIO when
+	 * a failure timeout occurs, so we want to re-initialise the error
+	 * retry behaviour appropriately when that happens.
+	 */
+	int			b_retries;
+	unsigned long		b_first_retry_time; /* in jiffies */
+	int			b_last_error;
+
 	const struct xfs_buf_ops	*b_ops;
 
 #ifdef XFS_BUF_LOCK_TRACKING
diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
index 3a30a88..6fe6852 100644
--- a/fs/xfs/xfs_buf_item.c
+++ b/fs/xfs/xfs_buf_item.c
@@ -1086,6 +1086,9 @@ xfs_buf_iodone_callback_error(
 		bp->b_flags |= (XBF_WRITE | XBF_ASYNC |
 			        XBF_DONE | XBF_WRITE_FAIL);
 		bp->b_last_error = bp->b_error;
+		bp->b_retries = 0;
+		bp->b_first_retry_time = jiffies;
+
 		xfs_buf_ioerror(bp, 0);
 		xfs_buf_submit(bp);
 		return true;
@@ -1095,8 +1098,12 @@ xfs_buf_iodone_callback_error(
 	 * Repeated failure on an async write. Take action according to the
 	 * error configuration we have been set up to use.
 	 */
-	if (!cfg->max_retries)
-		goto permanent_error;
+	if ((cfg->max_retries >= 0) &&
+	    (++bp->b_retries > cfg->max_retries))
+			goto permanent_error;
+	if (cfg->retry_timeout &&
+	    time_after(jiffies, cfg->retry_timeout + bp->b_first_retry_time))
+			goto permanent_error;
 
 	/* still a transient error, higher layers will retry */
 	xfs_buf_ioerror(bp, 0);
@@ -1139,6 +1146,7 @@ xfs_buf_iodone_callbacks(
 	 * retry state here in preparation for the next error that may occur.
 	 */
 	bp->b_last_error = 0;
+	bp->b_retries = 0;
 
 	xfs_buf_do_callbacks(bp);
 	bp->b_fspriv = NULL;
diff --git a/fs/xfs/xfs_mount.h b/fs/xfs/xfs_mount.h
index 0c5a976..0382140 100644
--- a/fs/xfs/xfs_mount.h
+++ b/fs/xfs/xfs_mount.h
@@ -54,7 +54,8 @@ enum {
 
 struct xfs_error_cfg {
 	struct xfs_kobj	kobj;
-	int		max_retries;
+	int		max_retries;	/* -1 = retry forever */
+	unsigned long	retry_timeout;	/* in jiffies, 0 = no timeout */
 };
 
 typedef struct xfs_mount {
diff --git a/fs/xfs/xfs_sysfs.c b/fs/xfs/xfs_sysfs.c
index 2a5b1cf..c881360 100644
--- a/fs/xfs/xfs_sysfs.c
+++ b/fs/xfs/xfs_sysfs.c
@@ -374,10 +374,6 @@ struct kobj_type xfs_log_ktype = {
  * and any other future type of IO (e.g. special inode or directory error
  * handling) we care to support.
  */
-static struct attribute *xfs_error_attrs[] = {
-	NULL,
-};
-
 static inline struct xfs_error_cfg *
 to_error_cfg(struct kobject *kobject)
 {
@@ -385,6 +381,79 @@ to_error_cfg(struct kobject *kobject)
 	return container_of(kobj, struct xfs_error_cfg, kobj);
 }
 
+static ssize_t
+max_retries_show(
+	struct kobject	*kobject,
+	char		*buf)
+{
+	struct xfs_error_cfg *cfg = to_error_cfg(kobject);
+
+	return snprintf(buf, PAGE_SIZE, "%d\n", cfg->max_retries);
+}
+
+static ssize_t
+max_retries_store(
+	struct kobject	*kobject,
+	const char	*buf,
+	size_t		count)
+{
+	struct xfs_error_cfg *cfg = to_error_cfg(kobject);
+	int		ret;
+	int		val;
+
+	ret = kstrtoint(buf, 0, &val);
+	if (ret)
+		return ret;
+
+	if (val < -1 || val > INT_MAX)
+		return -EINVAL;
+
+	cfg->max_retries = val;
+	return count;
+}
+XFS_SYSFS_ATTR_RW(max_retries);
+
+static ssize_t
+retry_timeout_seconds_show(
+	struct kobject	*kobject,
+	char		*buf)
+{
+	struct xfs_error_cfg *cfg = to_error_cfg(kobject);
+
+	return snprintf(buf, PAGE_SIZE, "%ld\n",
+			jiffies_to_msecs(cfg->retry_timeout) / MSEC_PER_SEC);
+}
+
+static ssize_t
+retry_timeout_seconds_store(
+	struct kobject	*kobject,
+	const char	*buf,
+	size_t		count)
+{
+	struct xfs_error_cfg *cfg = to_error_cfg(kobject);
+	int		ret;
+	int		val;
+
+	ret = kstrtoint(buf, 0, &val);
+	if (ret)
+		return ret;
+
+	/* 1 day timeout maximum */
+	if (val < 0 || val > 86400)
+		return -EINVAL;
+
+	cfg->retry_timeout = msecs_to_jiffies(val * MSEC_PER_SEC);
+	return count;
+}
+XFS_SYSFS_ATTR_RW(retry_timeout_seconds);
+
+static struct attribute *xfs_error_attrs[] = {
+	ATTR_LIST(max_retries),
+	ATTR_LIST(retry_timeout_seconds),
+	NULL,
+};
+
+
 struct kobj_type xfs_error_cfg_ktype = {
 	.release = xfs_sysfs_release,
 	.sysfs_ops = &xfs_sysfs_ops,
@@ -404,11 +473,13 @@ struct kobj_type xfs_error_ktype = {
 struct xfs_error_init {
 	char		*name;
 	int		max_retries;
+	int		retry_timeout;	/* in seconds */
 };
 
 static const struct xfs_error_init xfs_error_meta_init[XFS_ERR_ERRNO_MAX] = {
 	{ .name = "default",
 	  .max_retries = -1,
+	  .retry_timeout = 0,
 	},
 };
 
@@ -439,6 +510,8 @@ xfs_error_sysfs_init_class(
 			goto out_error;
 
 		cfg->max_retries = init[i].max_retries;
+		cfg->retry_timeout = msecs_to_jiffies(
+					init[i].retry_timeout * MSEC_PER_SEC);
 	}
 	return 0;
 
-- 
2.4.11

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

[PATCH 6/7] xfs: add configuration handlers for specific errors

[Carlos Maiolino]

now most of the infrastructure is in place, we can start adding
support for configuring specific errors such as ENODEV, ENOSPC, EIO,
etc. Add these error configurations and configure them all to have
appropriate behaviours. That is, all will be configured to retry forever by
default, except for ENODEV, which is an unrecoverable error, so it will be
configured to not retry on error

Changelog:

V3:
	- Do not implement .fail_speed and .fail_at_unmount
	- .fail_at_unmount will be implemented in a different patch

Signed-off-by: Dave Chinner <dchinner@redhat.com>
Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
---
 fs/xfs/xfs_mount.h |  3 +++
 fs/xfs/xfs_sysfs.c | 20 ++++++++++++++++++++
 2 files changed, 23 insertions(+)

diff --git a/fs/xfs/xfs_mount.h b/fs/xfs/xfs_mount.h
index 0382140..e3b3267 100644
--- a/fs/xfs/xfs_mount.h
+++ b/fs/xfs/xfs_mount.h
@@ -49,6 +49,9 @@ enum {
 };
 enum {
 	XFS_ERR_DEFAULT,
+	XFS_ERR_EIO,
+	XFS_ERR_ENOSPC,
+	XFS_ERR_ENODEV,
 	XFS_ERR_ERRNO_MAX,
 };
 
diff --git a/fs/xfs/xfs_sysfs.c b/fs/xfs/xfs_sysfs.c
index c881360..1ed9033 100644
--- a/fs/xfs/xfs_sysfs.c
+++ b/fs/xfs/xfs_sysfs.c
@@ -481,6 +481,17 @@ static const struct xfs_error_init xfs_error_meta_init[XFS_ERR_ERRNO_MAX] = {
 	  .max_retries = -1,
 	  .retry_timeout = 0,
 	},
+	{ .name = "EIO",
+	  .max_retries = -1,
+	  .retry_timeout = 0,
+	},
+	{ .name = "ENOSPC",
+	  .max_retries = -1,
+	  .retry_timeout = 0,
+	},
+	{ .name = "ENODEV",
+	  .max_retries = -1,
+	},
 };
 
 static int
@@ -578,6 +589,15 @@ xfs_error_get_cfg(
 	struct xfs_error_cfg	*cfg;
 
 	switch (error) {
+	case EIO:
+		cfg = &mp->m_error_cfg[error_class][XFS_ERR_EIO];
+		break;
+	case ENOSPC:
+		cfg = &mp->m_error_cfg[error_class][XFS_ERR_ENOSPC];
+		break;
+	case ENODEV:
+		cfg = &mp->m_error_cfg[error_class][XFS_ERR_ENODEV];
+		break;
 	default:
 		cfg = &mp->m_error_cfg[error_class][XFS_ERR_DEFAULT];
 		break;
-- 
2.4.11

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

[PATCH 7/7] xfs: add "fail at unmount" error handling configuration

[Carlos Maiolino]

If we take "retry forever" literally on metadata IO errors, we can
hang at unmount, once it retries those writes forever. This is the
default behavior, unfortunately.

Add an error configuration option for this behavior and default it to "fail" so
that an unmount will trigger actuall errors, a shutdown and allow the unmount to
succeed. It will be noisy, though, as it will log the errors and shutdown that
occurs.

To fix this, we need to mark the filesystem as being in the process of
unmounting. Do this with a mount flag that is added at the appropriate time
(i.e. before the blocking AIL sync). We also need to add this flag if mount
fails after the initial phase of log recovery has been run.

Changelog:

V3:
	- No major changes have been done to this patch, only the ones needed to
	  accomodate it on top of the remaining patches

Signed-off-by: Dave Chinner <dchinner@redhat.com>
Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
---
 fs/xfs/xfs_buf_item.c |  4 ++++
 fs/xfs/xfs_mount.c    |  9 +++++++++
 fs/xfs/xfs_mount.h    |  2 ++
 fs/xfs/xfs_sysfs.c    | 38 ++++++++++++++++++++++++++++++++++++++
 4 files changed, 53 insertions(+)

diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
index 6fe6852..c37f118 100644
--- a/fs/xfs/xfs_buf_item.c
+++ b/fs/xfs/xfs_buf_item.c
@@ -1105,6 +1105,10 @@ xfs_buf_iodone_callback_error(
 	    time_after(jiffies, cfg->retry_timeout + bp->b_first_retry_time))
 			goto permanent_error;
 
+	/* At unmount we may treat errors differently */
+	if ((mp->m_flags & XFS_MOUNT_UNMOUNTING) && cfg->fail_at_unmount)
+		goto permanent_error;
+
 	/* still a transient error, higher layers will retry */
 	xfs_buf_ioerror(bp, 0);
 	xfs_buf_relse(bp);
diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
index eda3906..d89fba6 100644
--- a/fs/xfs/xfs_mount.c
+++ b/fs/xfs/xfs_mount.c
@@ -961,6 +961,7 @@ xfs_mountfs(
 	cancel_delayed_work_sync(&mp->m_reclaim_work);
 	xfs_reclaim_inodes(mp, SYNC_WAIT);
  out_log_dealloc:
+	mp->m_flags |= XFS_MOUNT_UNMOUNTING;
 	xfs_log_mount_cancel(mp);
  out_fail_wait:
 	if (mp->m_logdev_targp && mp->m_logdev_targp != mp->m_ddev_targp)
@@ -1012,6 +1013,14 @@ xfs_unmountfs(
 	xfs_log_force(mp, XFS_LOG_SYNC);
 
 	/*
+	 * We now need to tell the world we are unmounting. This will allow
+	 * us to detect that the filesystem is going away and we should error
+	 * out anything that we have been retrying in the background. This will
+	 * prevent neverending retries iin AIL pushing from hanging the unmount.
+	 */
+	mp->m_flags |= XFS_MOUNT_UNMOUNTING;
+
+	/*
 	 * Flush all pending changes from the AIL.
 	 */
 	xfs_ail_push_all_sync(mp->m_ail);
diff --git a/fs/xfs/xfs_mount.h b/fs/xfs/xfs_mount.h
index e3b3267..86f4344 100644
--- a/fs/xfs/xfs_mount.h
+++ b/fs/xfs/xfs_mount.h
@@ -59,6 +59,7 @@ struct xfs_error_cfg {
 	struct xfs_kobj	kobj;
 	int		max_retries;	/* -1 = retry forever */
 	unsigned long	retry_timeout;	/* in jiffies, 0 = no timeout */
+	bool		fail_at_unmount;
 };
 
 typedef struct xfs_mount {
@@ -193,6 +194,7 @@ typedef struct xfs_mount {
 #define XFS_MOUNT_WSYNC		(1ULL << 0)	/* for nfs - all metadata ops
 						   must be synchronous except
 						   for space allocations */
+#define XFS_MOUNT_UNMOUNTING	(1ULL << 1)	/* filesystem is unmounting */
 #define XFS_MOUNT_WAS_CLEAN	(1ULL << 3)
 #define XFS_MOUNT_FS_SHUTDOWN	(1ULL << 4)	/* atomic stop of all filesystem
 						   operations, typically for
diff --git a/fs/xfs/xfs_sysfs.c b/fs/xfs/xfs_sysfs.c
index 1ed9033..ab27445 100644
--- a/fs/xfs/xfs_sysfs.c
+++ b/fs/xfs/xfs_sysfs.c
@@ -447,9 +447,42 @@ retry_timeout_seconds_store(
 }
 XFS_SYSFS_ATTR_RW(retry_timeout_seconds);
 
+static ssize_t
+fail_at_unmount_show(
+	struct kobject	*kobject,
+	char		*buf)
+{
+		struct xfs_error_cfg *cfg = to_error_cfg(kobject);
+
+		return snprintf(buf, PAGE_SIZE, "%d\n", cfg->fail_at_unmount);
+}
+
+static ssize_t
+fail_at_unmount_store(
+	struct kobject	*kobject,
+	const char	*buf,
+	size_t		count)
+{
+	struct xfs_error_cfg *cfg = to_error_cfg(kobject);
+	int		ret;
+	int		val;
+
+	ret = kstrtoint(buf, 0, &val);
+	if (ret)
+		return ret;
+
+	if (val < 0 || val > 1)
+		return -EINVAL;
+
+	cfg->fail_at_unmount = val;
+	return count;
+}
+XFS_SYSFS_ATTR_RW(fail_at_unmount);
+
 static struct attribute *xfs_error_attrs[] = {
 	ATTR_LIST(max_retries),
 	ATTR_LIST(retry_timeout_seconds),
+	ATTR_LIST(fail_at_unmount),
 	NULL,
 };
 
@@ -474,20 +507,24 @@ struct xfs_error_init {
 	char		*name;
 	int		max_retries;
 	int		retry_timeout;	/* in seconds */
+	bool		fail_at_unmount;
 };
 
 static const struct xfs_error_init xfs_error_meta_init[XFS_ERR_ERRNO_MAX] = {
 	{ .name = "default",
 	  .max_retries = -1,
 	  .retry_timeout = 0,
+	  .fail_at_unmount = true,
 	},
 	{ .name = "EIO",
 	  .max_retries = -1,
 	  .retry_timeout = 0,
+	  .fail_at_unmount = true,
 	},
 	{ .name = "ENOSPC",
 	  .max_retries = -1,
 	  .retry_timeout = 0,
+	  .fail_at_unmount = true,
 	},
 	{ .name = "ENODEV",
 	  .max_retries = -1,
@@ -523,6 +560,7 @@ xfs_error_sysfs_init_class(
 		cfg->max_retries = init[i].max_retries;
 		cfg->retry_timeout = msecs_to_jiffies(
 					init[i].retry_timeout * MSEC_PER_SEC);
+		cfg->fail_at_unmount = init[i].fail_at_unmount;
 	}
 	return 0;
 
-- 
2.4.11

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

Re: [PATCH 1/7] xfs: configurable error behavior via sysfs

[Brian Foster]

On Wed, May 04, 2016 at 05:43:14PM +0200, Carlos Maiolino wrote:
> We need to be able to change the way XFS behaviours in error
> conditions depending on the type of underlying storage. This is
> necessary for handling non-traditional block devices with extended
> error cases, such as thin provisioned devices that can return ENOSPC
> as an IO error.
> 
> Introduce the basic sysfs infrastructure needed to define and
> configure error behaviours. This is done to be generic enough to
> extend to configuring behaviour in other error conditions, such as
> ENOMEM, which also has different desired behaviours according to
> machine configuration.
> 
> Changelog:
> 
> V3:
> 	- Rebase patch on top of linux-xfs tree
> 	- Remove XFS_ERR_FAIL enums, once we are not using them
> 	  to control the fail speed, but base it on max_retries value
> 	- Get rid of xfs_error_cfg.fail_speed field. We will use only
> 	  .max_retries to control the failure speed for each error
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> ---
>  fs/xfs/xfs_mount.c | 10 +++++++++-
>  fs/xfs/xfs_mount.h | 20 ++++++++++++++++++++
>  fs/xfs/xfs_sysfs.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  fs/xfs/xfs_sysfs.h |  3 +++
>  4 files changed, 84 insertions(+), 2 deletions(-)
> 
...
> diff --git a/fs/xfs/xfs_sysfs.c b/fs/xfs/xfs_sysfs.c
> index 6ced4f1..b971113 100644
> --- a/fs/xfs/xfs_sysfs.c
> +++ b/fs/xfs/xfs_sysfs.c
> @@ -17,10 +17,11 @@
>   */
>  
>  #include "xfs.h"
> -#include "xfs_sysfs.h"
> +#include "xfs_shared.h"
>  #include "xfs_format.h"
>  #include "xfs_log_format.h"
>  #include "xfs_trans_resv.h"
> +#include "xfs_sysfs.h"
>  #include "xfs_log.h"
>  #include "xfs_log_priv.h"
>  #include "xfs_stats.h"
> @@ -362,3 +363,53 @@ struct kobj_type xfs_log_ktype = {
>  	.sysfs_ops = &xfs_sysfs_ops,
>  	.default_attrs = xfs_log_attrs,
>  };
> +
> +/*
> + * Metadata IO error configuration
> + *
> + * The sysfs structure here is:
> + *	...xfs/<dev>/error/<class>/<errno>/<error_attrs>
> + *
> + * where <class> allows use to discriminate between data IO and metadata IO,

Spelling:		   us

Otherwise looks fine:

Reviewed-by: Brian Foster <bfoster@redhat.com>

> + * and any other future type of IO (e.g. special inode or directory error
> + * handling) we care to support.
> + */
> +static struct attribute *xfs_error_attrs[] = {
> +	NULL,
> +};
> +
> +static inline struct xfs_error_cfg *
> +to_error_cfg(struct kobject *kobject)
> +{
> +	struct xfs_kobj *kobj = to_kobj(kobject);
> +	return container_of(kobj, struct xfs_error_cfg, kobj);
> +}
> +
> +struct kobj_type xfs_error_cfg_ktype = {
> +	.release = xfs_sysfs_release,
> +	.sysfs_ops = &xfs_sysfs_ops,
> +	.default_attrs = xfs_error_attrs,
> +};
> +
> +struct kobj_type xfs_error_ktype = {
> +	.release = xfs_sysfs_release,
> +};
> +
> +int
> +xfs_error_sysfs_init(
> +	struct xfs_mount	*mp)
> +{
> +	int			error;
> +
> +	/* .../xfs/<dev>/error/ */
> +	error = xfs_sysfs_init(&mp->m_error_kobj, &xfs_error_ktype,
> +				&mp->m_kobj, "error");
> +	return error;
> +}
> +
> +void
> +xfs_error_sysfs_del(
> +	struct xfs_mount	*mp)
> +{
> +	xfs_sysfs_del(&mp->m_error_kobj);
> +}
> diff --git a/fs/xfs/xfs_sysfs.h b/fs/xfs/xfs_sysfs.h
> index be692e5..d046371 100644
> --- a/fs/xfs/xfs_sysfs.h
> +++ b/fs/xfs/xfs_sysfs.h
> @@ -58,4 +58,7 @@ xfs_sysfs_del(
>  	wait_for_completion(&kobj->complete);
>  }
>  
> +int	xfs_error_sysfs_init(struct xfs_mount *mp);
> +void	xfs_error_sysfs_del(struct xfs_mount *mp);
> +
>  #endif	/* __XFS_SYSFS_H__ */
> -- 
> 2.4.11
> 
> _______________________________________________
> 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: [PATCH 2/7] xfs: introduce metadata IO error class

[Brian Foster]

On Wed, May 04, 2016 at 05:43:15PM +0200, Carlos Maiolino wrote:
> Now we have the basic infrastructure, add the first error class so
> we can build up the infrastructure in a meaningful way. Add the
> metadata async write IO error class and sysfs entry, and introduce a
> default configuration that matches the existing "retry forever" behavior
> for async write metadata buffers.
> 
> Changelog:
> 
> V3:
> 	- Use 'cfg->max_retries = -1' as the default configuration for the
> 	  "retry forever" behavior, instead of
> 	  cfg->fail_speed = XFS_ERR_FAIL_NEVER
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> ---

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  fs/xfs/xfs_mount.h |  3 +++
>  fs/xfs/xfs_sysfs.c | 34 ++++++++++++++++++++++++++++++++++
>  2 files changed, 37 insertions(+)
> 
> diff --git a/fs/xfs/xfs_mount.h b/fs/xfs/xfs_mount.h
> index d639795..352a5c8 100644
> --- a/fs/xfs/xfs_mount.h
> +++ b/fs/xfs/xfs_mount.h
> @@ -44,9 +44,11 @@ enum {
>   * Error numbers define the errors that are configurable.
>   */
>  enum {
> +	XFS_ERR_METADATA,
>  	XFS_ERR_CLASS_MAX,
>  };
>  enum {
> +	XFS_ERR_DEFAULT,
>  	XFS_ERR_ERRNO_MAX,
>  };
>  
> @@ -146,6 +148,7 @@ typedef struct xfs_mount {
>  						/* low free space thresholds */
>  	struct xfs_kobj		m_kobj;
>  	struct xfs_kobj		m_error_kobj;
> +	struct xfs_kobj		m_error_meta_kobj;
>  	struct xfs_error_cfg	m_error_cfg[XFS_ERR_CLASS_MAX][XFS_ERR_ERRNO_MAX];
>  	struct xstats		m_stats;	/* per-fs stats */
>  
> diff --git a/fs/xfs/xfs_sysfs.c b/fs/xfs/xfs_sysfs.c
> index b971113..7180cff 100644
> --- a/fs/xfs/xfs_sysfs.c
> +++ b/fs/xfs/xfs_sysfs.c
> @@ -399,11 +399,34 @@ int
>  xfs_error_sysfs_init(
>  	struct xfs_mount	*mp)
>  {
> +	struct xfs_error_cfg	*cfg;
>  	int			error;
>  
>  	/* .../xfs/<dev>/error/ */
>  	error = xfs_sysfs_init(&mp->m_error_kobj, &xfs_error_ktype,
>  				&mp->m_kobj, "error");
> +	if (error)
> +		return error;
> +
> +	/* .../xfs/<dev>/error/metadata/ */
> +	error = xfs_sysfs_init(&mp->m_error_meta_kobj, &xfs_error_ktype,
> +				&mp->m_error_kobj, "metadata");
> +	if (error)
> +		goto out_error;
> +
> +	cfg = &mp->m_error_cfg[XFS_ERR_METADATA][XFS_ERR_DEFAULT];
> +	error = xfs_sysfs_init(&cfg->kobj, &xfs_error_cfg_ktype,
> +				&mp->m_error_meta_kobj, "default");
> +	if (error)
> +		goto out_error_meta;
> +	cfg->max_retries = -1;
> +
> +	return 0;
> +
> +out_error_meta:
> +	xfs_sysfs_del(&mp->m_error_meta_kobj);
> +out_error:
> +	xfs_sysfs_del(&mp->m_error_kobj);
>  	return error;
>  }
>  
> @@ -411,5 +434,16 @@ void
>  xfs_error_sysfs_del(
>  	struct xfs_mount	*mp)
>  {
> +	struct xfs_error_cfg	*cfg;
> +	int			i, j;
> +
> +	for (i = 0; i < XFS_ERR_CLASS_MAX; i++) {
> +		for (j = 0; j < XFS_ERR_ERRNO_MAX; j++) {
> +			cfg = &mp->m_error_cfg[i][j];
> +
> +			xfs_sysfs_del(&cfg->kobj);
> +		}
> +	}
> +	xfs_sysfs_del(&mp->m_error_meta_kobj);
>  	xfs_sysfs_del(&mp->m_error_kobj);
>  }
> -- 
> 2.4.11
> 
> _______________________________________________
> 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: [PATCH 3/7] xfs: add configurable error support to metadata buffers

[Brian Foster]

On Wed, May 04, 2016 at 05:43:16PM +0200, Carlos Maiolino wrote:
> With the error configuration handle for async metadata write errors
> in place, we can now add initial support to the IO error processing
> in xfs_buf_iodone_error().
> 
> Add an infrastructure function to look up the configuration handle,
> and rearrange the error handling to prepare the way for different
> error handling conigurations to be used.
> 
> Changelog:
> 
> V3:
> 	- in xfs_buf_iodone_callbacks, use cfg->max_retries to
> 	  determine if the filesystem should fail or not, a zero value
> 	  means it should never retry.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> ---
>  fs/xfs/xfs_buf.h      |   1 +
>  fs/xfs/xfs_buf_item.c | 112 ++++++++++++++++++++++++++++++--------------------
>  fs/xfs/xfs_mount.h    |   3 ++
>  fs/xfs/xfs_sysfs.c    |  17 ++++++++
>  4 files changed, 88 insertions(+), 45 deletions(-)
> 
> diff --git a/fs/xfs/xfs_buf.h b/fs/xfs/xfs_buf.h
> index 4eb89bd..adef116 100644
> --- a/fs/xfs/xfs_buf.h
> +++ b/fs/xfs/xfs_buf.h
> @@ -183,6 +183,7 @@ typedef struct xfs_buf {
>  	unsigned int		b_page_count;	/* size of page array */
>  	unsigned int		b_offset;	/* page offset in first page */
>  	int			b_error;	/* error code on I/O */
> +	int			b_last_error;	/* previous async I/O error */
>  	const struct xfs_buf_ops	*b_ops;
>  
>  #ifdef XFS_BUF_LOCK_TRACKING
> diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
> index 99e91a0..3a30a88 100644
> --- a/fs/xfs/xfs_buf_item.c
> +++ b/fs/xfs/xfs_buf_item.c
> @@ -1042,35 +1042,22 @@ xfs_buf_do_callbacks(
>  	}
>  }
>  
> -/*
> - * This is the iodone() function for buffers which have had callbacks
> - * attached to them by xfs_buf_attach_iodone().  It should remove each
> - * log item from the buffer's list and call the callback of each in turn.
> - * When done, the buffer's fsprivate field is set to NULL and the buffer
> - * is unlocked with a call to iodone().
> - */
> -void
> -xfs_buf_iodone_callbacks(
> +static bool
> +xfs_buf_iodone_callback_error(
>  	struct xfs_buf		*bp)
>  {
>  	struct xfs_log_item	*lip = bp->b_fspriv;
>  	struct xfs_mount	*mp = lip->li_mountp;
>  	static ulong		lasttime;
>  	static xfs_buftarg_t	*lasttarg;
> -
> -	if (likely(!bp->b_error))
> -		goto do_callbacks;
> +	struct xfs_error_cfg	*cfg;
>  
>  	/*
>  	 * If we've already decided to shutdown the filesystem because of
>  	 * I/O errors, there's no point in giving this a retry.
>  	 */
> -	if (XFS_FORCED_SHUTDOWN(mp)) {
> -		xfs_buf_stale(bp);
> -		bp->b_flags |= XBF_DONE;
> -		trace_xfs_buf_item_iodone(bp, _RET_IP_);
> -		goto do_callbacks;
> -	}
> +	if (XFS_FORCED_SHUTDOWN(mp))
> +		goto out_stale;

Looks like we've lost a trace_xfs_buf_item_iodone() call here. Maybe we
don't really need it, but this is the only caller so we should at least
remove it.

>  
>  	if (bp->b_target != lasttarg ||
>  	    time_after(jiffies, (lasttime + 5*HZ))) {
> @@ -1079,45 +1066,80 @@ xfs_buf_iodone_callbacks(
>  	}
>  	lasttarg = bp->b_target;
>  
> +	/* synchronous writes will have callers process the error */
> +	if (!(bp->b_flags & XBF_ASYNC))
> +		goto out_stale;
> +
> +	trace_xfs_buf_item_iodone_async(bp, _RET_IP_);
> +	ASSERT(bp->b_iodone != NULL);
> +
>  	/*
>  	 * If the write was asynchronous then no one will be looking for the
> -	 * error.  Clear the error state and write the buffer out again.
> -	 *
> -	 * XXX: This helps against transient write errors, but we need to find
> -	 * a way to shut the filesystem down if the writes keep failing.
> -	 *
> -	 * In practice we'll shut the filesystem down soon as non-transient
> -	 * errors tend to affect the whole device and a failing log write
> -	 * will make us give up.  But we really ought to do better here.
> +	 * error.  If this is the first failure of this type, clear the error
> +	 * state and write the buffer out again. This means we always retry an
> +	 * async write failure at least once, but we also need to set the buffer
> +	 * up to behave correctly now for repeated failures.
>  	 */
> -	if (bp->b_flags & XBF_ASYNC) {
> -		ASSERT(bp->b_iodone != NULL);
> -
> -		trace_xfs_buf_item_iodone_async(bp, _RET_IP_);
> +	cfg = xfs_error_get_cfg(mp, XFS_ERR_METADATA, bp->b_error);

Can we push this down? We don't use cfg until after the subsequent hunk.

> +	if (!(bp->b_flags & (XBF_STALE|XBF_WRITE_FAIL)) ||
> +	     bp->b_last_error != bp->b_error) {
> +		bp->b_flags |= (XBF_WRITE | XBF_ASYNC |
> +			        XBF_DONE | XBF_WRITE_FAIL);
> +		bp->b_last_error = bp->b_error;
> +		xfs_buf_ioerror(bp, 0);
> +		xfs_buf_submit(bp);
> +		return true;
> +	}
>  
> -		xfs_buf_ioerror(bp, 0); /* errno of 0 unsets the flag */
> +	/*
> +	 * Repeated failure on an async write. Take action according to the
> +	 * error configuration we have been set up to use.
> +	 */
> +	if (!cfg->max_retries)
> +		goto permanent_error;
>  
> -		if (!(bp->b_flags & (XBF_STALE|XBF_WRITE_FAIL))) {
> -			bp->b_flags |= XBF_WRITE | XBF_ASYNC |
> -				       XBF_DONE | XBF_WRITE_FAIL;
> -			xfs_buf_submit(bp);
> -		} else {
> -			xfs_buf_relse(bp);
> -		}
> -
> -		return;
> -	}
> +	/* still a transient error, higher layers will retry */
> +	xfs_buf_ioerror(bp, 0);
> +	xfs_buf_relse(bp);
> +	return true;
>  
>  	/*
> -	 * If the write of the buffer was synchronous, we want to make
> -	 * sure to return the error to the caller of xfs_bwrite().
> +	 * Permanent error - we need to trigger a shutdown if we haven't already
> +	 * to indicate that inconsistency will result from this action.
>  	 */
> +permanent_error:
> +	xfs_force_shutdown(mp, SHUTDOWN_META_IO_ERROR);
> +out_stale:
>  	xfs_buf_stale(bp);
>  	bp->b_flags |= XBF_DONE;
> -
>  	trace_xfs_buf_error_relse(bp, _RET_IP_);
> +	return false;
> +}
> +
> +/*
> + * This is the iodone() function for buffers which have had callbacks attached
> + * to them by xfs_buf_attach_iodone(). We need to iterate the items on the
> + * callback list, mark the buffer as having no more callbacks and then push the
> + * buffer through IO completion processing.
> + */
> +void
> +xfs_buf_iodone_callbacks(
> +	struct xfs_buf		*bp)
> +{
> +	/*
> +	 * If there is an error, process it. Some errors require us
> +	 * to run callbacks after failure processing is done so we
> +	 * detect that and take appropriate action.
> +	 */
> +	if (bp->b_error && xfs_buf_iodone_callback_error(bp))
> +		return;

Just a nit, but I'm not a fan of this kind of logic just because it
tends to obfuscate the fact that a function is being called. Could we
make it look something like the following?

	/* ... */
	if (bp->b_error) {
		if (xfs_buf_iodone_callback_error(bp))
			return;
	}

Also FWIW, the return value logic of "return if true" seems a little
weird to me because I read it as "don't do something if true." The
inverse ("run callbacks if true") is more clear, IMO, but that's not a
big deal.

Brian

> +
> +	/*
> +	 * Successful IO or permanent error. Either way, we can clear the
> +	 * retry state here in preparation for the next error that may occur.
> +	 */
> +	bp->b_last_error = 0;
>  
> -do_callbacks:
>  	xfs_buf_do_callbacks(bp);
>  	bp->b_fspriv = NULL;
>  	bp->b_iodone = NULL;
> diff --git a/fs/xfs/xfs_mount.h b/fs/xfs/xfs_mount.h
> index 352a5c8..0c5a976 100644
> --- a/fs/xfs/xfs_mount.h
> +++ b/fs/xfs/xfs_mount.h
> @@ -387,4 +387,7 @@ extern void	xfs_set_low_space_thresholds(struct xfs_mount *);
>  int	xfs_zero_extent(struct xfs_inode *ip, xfs_fsblock_t start_fsb,
>  			xfs_off_t count_fsb);
>  
> +struct xfs_error_cfg * xfs_error_get_cfg(struct xfs_mount *mp,
> +		int error_class, int error);
> +
>  #endif	/* __XFS_MOUNT_H__ */
> diff --git a/fs/xfs/xfs_sysfs.c b/fs/xfs/xfs_sysfs.c
> index 7180cff..0a9adcd 100644
> --- a/fs/xfs/xfs_sysfs.c
> +++ b/fs/xfs/xfs_sysfs.c
> @@ -447,3 +447,20 @@ xfs_error_sysfs_del(
>  	xfs_sysfs_del(&mp->m_error_meta_kobj);
>  	xfs_sysfs_del(&mp->m_error_kobj);
>  }
> +
> +struct xfs_error_cfg *
> +xfs_error_get_cfg(
> +	struct xfs_mount	*mp,
> +	int			error_class,
> +	int			error)
> +{
> +	struct xfs_error_cfg	*cfg;
> +
> +	switch (error) {
> +	default:
> +		cfg = &mp->m_error_cfg[error_class][XFS_ERR_DEFAULT];
> +		break;
> +	}
> +
> +	return cfg;
> +}
> -- 
> 2.4.11
> 
> _______________________________________________
> 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: [PATCH 4/7] xfs: introduce table-based init for error behaviors

[Brian Foster]

On Wed, May 04, 2016 at 05:43:17PM +0200, Carlos Maiolino wrote:
> Before we start expanding the number of error classes and errors we
> can configure behaviour for, we need a simple and clear way to
> define the default behaviour that we initialized each mount with.
> Introduce a table based method for keeping the initial configuration
> in, and apply that to the existing initialization code.
> 
> Changelog:
> 
> V3:
> 	- Replace all .fail_speed fields by .max_retries, once the code will no
> 	  longer use .fail_speed to decide when it should fail
> 	- The "Default" attribute is also in lower-case (default). I don't
> 	  believe it's a good idea to have a mixed-case attribute names in sysfs.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> ---

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  fs/xfs/xfs_sysfs.c | 72 +++++++++++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 60 insertions(+), 12 deletions(-)
> 
> diff --git a/fs/xfs/xfs_sysfs.c b/fs/xfs/xfs_sysfs.c
> index 0a9adcd..2a5b1cf 100644
> --- a/fs/xfs/xfs_sysfs.c
> +++ b/fs/xfs/xfs_sysfs.c
> @@ -395,11 +395,67 @@ struct kobj_type xfs_error_ktype = {
>  	.release = xfs_sysfs_release,
>  };
>  
> +/*
> + * Error initialization tables. These need to be ordered in the same
> + * order as the enums used to index the array. All class init tables need to
> + * define a "default" behaviour as the first entry, all other entries can be
> + * empty.
> + */
> +struct xfs_error_init {
> +	char		*name;
> +	int		max_retries;
> +};
> +
> +static const struct xfs_error_init xfs_error_meta_init[XFS_ERR_ERRNO_MAX] = {
> +	{ .name = "default",
> +	  .max_retries = -1,
> +	},
> +};
> +
> +static int
> +xfs_error_sysfs_init_class(
> +	struct xfs_mount	*mp,
> +	int			class,
> +	const char		*parent_name,
> +	struct xfs_kobj		*parent_kobj,
> +	const struct xfs_error_init init[])
> +{
> +	struct xfs_error_cfg	*cfg;
> +	int			error;
> +	int			i;
> +
> +	ASSERT(class < XFS_ERR_CLASS_MAX);
> +
> +	error = xfs_sysfs_init(parent_kobj, &xfs_error_ktype,
> +				&mp->m_error_kobj, parent_name);
> +	if (error)
> +		return error;
> +
> +	for (i = 0; i < XFS_ERR_ERRNO_MAX; i++) {
> +		cfg = &mp->m_error_cfg[class][i];
> +		error = xfs_sysfs_init(&cfg->kobj, &xfs_error_cfg_ktype,
> +					parent_kobj, init[i].name);
> +		if (error)
> +			goto out_error;
> +
> +		cfg->max_retries = init[i].max_retries;
> +	}
> +	return 0;
> +
> +out_error:
> +	/* unwind the entries that succeeded */
> +	for (i--; i >= 0; i--) {
> +		cfg = &mp->m_error_cfg[class][i];
> +		xfs_sysfs_del(&cfg->kobj);
> +	}
> +	xfs_sysfs_del(parent_kobj);
> +	return error;
> +}
> +
>  int
>  xfs_error_sysfs_init(
>  	struct xfs_mount	*mp)
>  {
> -	struct xfs_error_cfg	*cfg;
>  	int			error;
>  
>  	/* .../xfs/<dev>/error/ */
> @@ -409,22 +465,14 @@ xfs_error_sysfs_init(
>  		return error;
>  
>  	/* .../xfs/<dev>/error/metadata/ */
> -	error = xfs_sysfs_init(&mp->m_error_meta_kobj, &xfs_error_ktype,
> -				&mp->m_error_kobj, "metadata");
> +	error = xfs_error_sysfs_init_class(mp, XFS_ERR_METADATA,
> +				"metadata", &mp->m_error_meta_kobj,
> +				xfs_error_meta_init);
>  	if (error)
>  		goto out_error;
>  
> -	cfg = &mp->m_error_cfg[XFS_ERR_METADATA][XFS_ERR_DEFAULT];
> -	error = xfs_sysfs_init(&cfg->kobj, &xfs_error_cfg_ktype,
> -				&mp->m_error_meta_kobj, "default");
> -	if (error)
> -		goto out_error_meta;
> -	cfg->max_retries = -1;
> -
>  	return 0;
>  
> -out_error_meta:
> -	xfs_sysfs_del(&mp->m_error_meta_kobj);
>  out_error:
>  	xfs_sysfs_del(&mp->m_error_kobj);
>  	return error;
> -- 
> 2.4.11
> 
> _______________________________________________
> 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: [PATCH 5/7] xfs: add configuration of error failure speed

[Brian Foster]

On Wed, May 04, 2016 at 05:43:18PM +0200, Carlos Maiolino wrote:
> On reception of an error, we can fail immediately, perform some
> bound amount of retries or retry indefinitely. The current behaviour
> we have is to retry forever.
> 
> However, we'd like the ability to choose how long the filesystem should try
> after an error, it can either fail immediately, retry a few times, or retry
> forever. This is implemented by using max_retries sysfs attribute, to hold the
> amount of times we allow the filesystem to retry after an error. Being -1 a
> special case where the filesystem will retry indefinitely.
> 
> Add both a maximum retry count and a retry timeout so that we can bound by
> time and/or physical IO attempts.
> 
> Finally, plumb these into xfs_buf_iodone error processing so that
> the error behaviour follows the selected configuration.
> 
> Changelog:
> 
> V3:
> 	- In xfs_buf_iodone_callback_error, use max_retries to decide how long
> 	  the filesystem should retry on errors instead of XFS_ERR_FAIL enums
> 	  and fail_speed
> 
> 	- Remove all code implementing fail_speed attribute from the original
> 	  patch
> 		-- failure_speed_show/store attributes function implementation
> 		-- max_retries_store() now accepts values from -1 up to INT_MAX
> 
> 	- retry_timeout_seconds_show() print fixed:
> 		-- jiffies_to_msecs() should be divided by MSEC_PER_SEC
> 		-- trailing whitespace removed
> 
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> ---
>  fs/xfs/xfs_buf.h      | 23 ++++++++++++++-
>  fs/xfs/xfs_buf_item.c | 12 ++++++--
>  fs/xfs/xfs_mount.h    |  3 +-
>  fs/xfs/xfs_sysfs.c    | 81 ++++++++++++++++++++++++++++++++++++++++++++++++---
>  4 files changed, 111 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/xfs/xfs_buf.h b/fs/xfs/xfs_buf.h
> index adef116..bd978f0 100644
> --- a/fs/xfs/xfs_buf.h
> +++ b/fs/xfs/xfs_buf.h
> @@ -183,7 +183,28 @@ typedef struct xfs_buf {
>  	unsigned int		b_page_count;	/* size of page array */
>  	unsigned int		b_offset;	/* page offset in first page */
>  	int			b_error;	/* error code on I/O */
> -	int			b_last_error;	/* previous async I/O error */
> +
> +	/*
> +	 * async write failure retry count. Initialised to zero on the first
> +	 * failure, then when it exceeds the maximum configured without a
> +	 * success the write is considered to be failed permanently and the
> +	 * iodone handler will take appropriate action.
> +	 *
> +	 * For retry timeouts, we record the jiffie of the first failure. This
> +	 * means that we can change the retry timeout and it on the next error
> +	 * it will be checked against the newly configured timeout. This
> +	 * prevents buffers getting stuck in retry loops with a really long
> +	 * timeout.

I'd reword the last bit to something like the following:

"This means we can change the retry timeout for buffers already under
I/O and thus avoid getting stuck in retry loops with a really long
timeout."

> +	 *
> +	 * last_error is used to ensure that we are getting repeated errors, not
> +	 * different errors. e.g. a block device might change ENOSPC to EIO when
> +	 * a failure timeout occurs, so we want to re-initialise the error
> +	 * retry behaviour appropriately when that happens.
> +	 */
> +	int			b_retries;
> +	unsigned long		b_first_retry_time; /* in jiffies */
> +	int			b_last_error;
> +
>  	const struct xfs_buf_ops	*b_ops;
>  
>  #ifdef XFS_BUF_LOCK_TRACKING
> diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
> index 3a30a88..6fe6852 100644
> --- a/fs/xfs/xfs_buf_item.c
> +++ b/fs/xfs/xfs_buf_item.c
> @@ -1086,6 +1086,9 @@ xfs_buf_iodone_callback_error(
>  		bp->b_flags |= (XBF_WRITE | XBF_ASYNC |
>  			        XBF_DONE | XBF_WRITE_FAIL);
>  		bp->b_last_error = bp->b_error;
> +		bp->b_retries = 0;
> +		bp->b_first_retry_time = jiffies;
> +
>  		xfs_buf_ioerror(bp, 0);
>  		xfs_buf_submit(bp);
>  		return true;
> @@ -1095,8 +1098,12 @@ xfs_buf_iodone_callback_error(
>  	 * Repeated failure on an async write. Take action according to the
>  	 * error configuration we have been set up to use.
>  	 */
> -	if (!cfg->max_retries)
> -		goto permanent_error;
> +	if ((cfg->max_retries >= 0) &&
> +	    (++bp->b_retries > cfg->max_retries))
> +			goto permanent_error;
> +	if (cfg->retry_timeout &&
> +	    time_after(jiffies, cfg->retry_timeout + bp->b_first_retry_time))
> +			goto permanent_error;
>  
>  	/* still a transient error, higher layers will retry */
>  	xfs_buf_ioerror(bp, 0);
> @@ -1139,6 +1146,7 @@ xfs_buf_iodone_callbacks(
>  	 * retry state here in preparation for the next error that may occur.
>  	 */
>  	bp->b_last_error = 0;
> +	bp->b_retries = 0;
>  
>  	xfs_buf_do_callbacks(bp);
>  	bp->b_fspriv = NULL;
> diff --git a/fs/xfs/xfs_mount.h b/fs/xfs/xfs_mount.h
> index 0c5a976..0382140 100644
> --- a/fs/xfs/xfs_mount.h
> +++ b/fs/xfs/xfs_mount.h
> @@ -54,7 +54,8 @@ enum {
>  
>  struct xfs_error_cfg {
>  	struct xfs_kobj	kobj;
> -	int		max_retries;
> +	int		max_retries;	/* -1 = retry forever */
> +	unsigned long	retry_timeout;	/* in jiffies, 0 = no timeout */
>  };
>  
>  typedef struct xfs_mount {
> diff --git a/fs/xfs/xfs_sysfs.c b/fs/xfs/xfs_sysfs.c
> index 2a5b1cf..c881360 100644
> --- a/fs/xfs/xfs_sysfs.c
> +++ b/fs/xfs/xfs_sysfs.c
> @@ -374,10 +374,6 @@ struct kobj_type xfs_log_ktype = {
>   * and any other future type of IO (e.g. special inode or directory error
>   * handling) we care to support.
>   */
> -static struct attribute *xfs_error_attrs[] = {
> -	NULL,
> -};
> -
>  static inline struct xfs_error_cfg *
>  to_error_cfg(struct kobject *kobject)
>  {
> @@ -385,6 +381,79 @@ to_error_cfg(struct kobject *kobject)
>  	return container_of(kobj, struct xfs_error_cfg, kobj);
>  }
>  
> +static ssize_t
> +max_retries_show(
> +	struct kobject	*kobject,
> +	char		*buf)
> +{
> +	struct xfs_error_cfg *cfg = to_error_cfg(kobject);
> +
> +	return snprintf(buf, PAGE_SIZE, "%d\n", cfg->max_retries);
> +}
> +
> +static ssize_t
> +max_retries_store(
> +	struct kobject	*kobject,
> +	const char	*buf,
> +	size_t		count)
> +{
> +	struct xfs_error_cfg *cfg = to_error_cfg(kobject);
> +	int		ret;
> +	int		val;
> +
> +	ret = kstrtoint(buf, 0, &val);
> +	if (ret)
> +		return ret;
> +
> +	if (val < -1 || val > INT_MAX)
> +		return -EINVAL;
> +

Can an int be greater than INT_MAX? :)

The rest looks fine so you can add:

Reviewed-by: Brian Foster <bfoster@redhat.com>

> +	cfg->max_retries = val;
> +	return count;
> +}
> +XFS_SYSFS_ATTR_RW(max_retries);
> +
> +static ssize_t
> +retry_timeout_seconds_show(
> +	struct kobject	*kobject,
> +	char		*buf)
> +{
> +	struct xfs_error_cfg *cfg = to_error_cfg(kobject);
> +
> +	return snprintf(buf, PAGE_SIZE, "%ld\n",
> +			jiffies_to_msecs(cfg->retry_timeout) / MSEC_PER_SEC);
> +}
> +
> +static ssize_t
> +retry_timeout_seconds_store(
> +	struct kobject	*kobject,
> +	const char	*buf,
> +	size_t		count)
> +{
> +	struct xfs_error_cfg *cfg = to_error_cfg(kobject);
> +	int		ret;
> +	int		val;
> +
> +	ret = kstrtoint(buf, 0, &val);
> +	if (ret)
> +		return ret;
> +
> +	/* 1 day timeout maximum */
> +	if (val < 0 || val > 86400)
> +		return -EINVAL;
> +
> +	cfg->retry_timeout = msecs_to_jiffies(val * MSEC_PER_SEC);
> +	return count;
> +}
> +XFS_SYSFS_ATTR_RW(retry_timeout_seconds);
> +
> +static struct attribute *xfs_error_attrs[] = {
> +	ATTR_LIST(max_retries),
> +	ATTR_LIST(retry_timeout_seconds),
> +	NULL,
> +};
> +
> +
>  struct kobj_type xfs_error_cfg_ktype = {
>  	.release = xfs_sysfs_release,
>  	.sysfs_ops = &xfs_sysfs_ops,
> @@ -404,11 +473,13 @@ struct kobj_type xfs_error_ktype = {
>  struct xfs_error_init {
>  	char		*name;
>  	int		max_retries;
> +	int		retry_timeout;	/* in seconds */
>  };
>  
>  static const struct xfs_error_init xfs_error_meta_init[XFS_ERR_ERRNO_MAX] = {
>  	{ .name = "default",
>  	  .max_retries = -1,
> +	  .retry_timeout = 0,
>  	},
>  };
>  
> @@ -439,6 +510,8 @@ xfs_error_sysfs_init_class(
>  			goto out_error;
>  
>  		cfg->max_retries = init[i].max_retries;
> +		cfg->retry_timeout = msecs_to_jiffies(
> +					init[i].retry_timeout * MSEC_PER_SEC);
>  	}
>  	return 0;
>  
> -- 
> 2.4.11
> 
> _______________________________________________
> 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: [PATCH 6/7] xfs: add configuration handlers for specific errors

[Brian Foster]

On Wed, May 04, 2016 at 05:43:19PM +0200, Carlos Maiolino wrote:
> now most of the infrastructure is in place, we can start adding
> support for configuring specific errors such as ENODEV, ENOSPC, EIO,
> etc. Add these error configurations and configure them all to have
> appropriate behaviours. That is, all will be configured to retry forever by
> default, except for ENODEV, which is an unrecoverable error, so it will be
> configured to not retry on error
> 
> Changelog:
> 
> V3:
> 	- Do not implement .fail_speed and .fail_at_unmount
> 	- .fail_at_unmount will be implemented in a different patch
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> ---
>  fs/xfs/xfs_mount.h |  3 +++
>  fs/xfs/xfs_sysfs.c | 20 ++++++++++++++++++++
>  2 files changed, 23 insertions(+)
> 
> diff --git a/fs/xfs/xfs_mount.h b/fs/xfs/xfs_mount.h
> index 0382140..e3b3267 100644
> --- a/fs/xfs/xfs_mount.h
> +++ b/fs/xfs/xfs_mount.h
> @@ -49,6 +49,9 @@ enum {
>  };
>  enum {
>  	XFS_ERR_DEFAULT,
> +	XFS_ERR_EIO,
> +	XFS_ERR_ENOSPC,
> +	XFS_ERR_ENODEV,
>  	XFS_ERR_ERRNO_MAX,
>  };
>  
> diff --git a/fs/xfs/xfs_sysfs.c b/fs/xfs/xfs_sysfs.c
> index c881360..1ed9033 100644
> --- a/fs/xfs/xfs_sysfs.c
> +++ b/fs/xfs/xfs_sysfs.c
> @@ -481,6 +481,17 @@ static const struct xfs_error_init xfs_error_meta_init[XFS_ERR_ERRNO_MAX] = {
>  	  .max_retries = -1,
>  	  .retry_timeout = 0,
>  	},
> +	{ .name = "EIO",
> +	  .max_retries = -1,
> +	  .retry_timeout = 0,
> +	},
> +	{ .name = "ENOSPC",
> +	  .max_retries = -1,
> +	  .retry_timeout = 0,
> +	},
> +	{ .name = "ENODEV",
> +	  .max_retries = -1,

Shouldn't this be 0?

Brian

> +	},
>  };
>  
>  static int
> @@ -578,6 +589,15 @@ xfs_error_get_cfg(
>  	struct xfs_error_cfg	*cfg;
>  
>  	switch (error) {
> +	case EIO:
> +		cfg = &mp->m_error_cfg[error_class][XFS_ERR_EIO];
> +		break;
> +	case ENOSPC:
> +		cfg = &mp->m_error_cfg[error_class][XFS_ERR_ENOSPC];
> +		break;
> +	case ENODEV:
> +		cfg = &mp->m_error_cfg[error_class][XFS_ERR_ENODEV];
> +		break;
>  	default:
>  		cfg = &mp->m_error_cfg[error_class][XFS_ERR_DEFAULT];
>  		break;
> -- 
> 2.4.11
> 
> _______________________________________________
> 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: [PATCH 0/7] Configurable error behavior [V3]

[Brian Foster]

On Wed, May 04, 2016 at 05:43:13PM +0200, Carlos Maiolino wrote:
> This is the new revision of this patchset, according to last comments.
> 
> This patchset is aimed to implement a configurable error behavior in XFS, and
> most of the design has been done by Dave, so, that's why I kept his signed-off
> in the patches.
> 
> This new revision has the detailed changelog written on each patch, but the
> major changes are:
> 
> 	- Detailed changelog by-patch and description fixed to become
> 	  (hopefuly) more clear
> 	- kept fail_at_unmount as a sysfs attribute
> 
> 
> Regarding fail_at_unmount, I left it almost exactly as Dave's design, giving his
> comments on the last revision, although, I still think there is no need to keep
> it as a per-error granularity, so, I was wondering if a single, global option in
> /sys/fs/xfs/<dev>/error/fail_at_unmount wouldn't suffice, but, this will require
> a new place to store the value inside kernel, instead of keeping it inside
> struct xfs_error_cfg, or maybe use the same structure but use it outside of the
> m_error_cfg array?
> 

I agree with regard to the granularity of fail_at_unmount. This was
brought up previously:

http://oss.sgi.com/archives/xfs/2016-02/msg00558.html

... and I haven't heard a use case for per-error granularity.

I suggest just to pull it out of the error classification stuff entirely
and place it under xfs_mount. E.g., at the same level as "fail_writes"
(but not a DEBUG mode only option).

I'm also wondering whether we need more mechanism for the
fail_at_unmount behavior. For example, instead of defining
XFS_MOUNT_UNMOUNTING, could we just call a function that resets
max_retries (of each class) to 0 in the unmount path? Then maybe call
the mount tunable retry_on_unmount or something like that. Thoughts?

Brian

> First 6 patches are ready, the fail_at_unmount one, need to be re-worked if we
> want it in a less granular way, but until now I don't think we reached any
> decision about how it should be implemented.
> 
>  fs/xfs/xfs_buf.h      |  22 ++++
>  fs/xfs/xfs_buf_item.c | 126 ++++++++++++++--------
>  fs/xfs/xfs_mount.c    |  19 +++-
>  fs/xfs/xfs_mount.h    |  32 ++++++
>  fs/xfs/xfs_sysfs.c    | 283 +++++++++++++++++++++++++++++++++++++++++++++++++-
>  fs/xfs/xfs_sysfs.h    |   3 +
>  6 files changed, 437 insertions(+), 48 deletions(-)
> 
> -- 
> 2.4.11
> 
> _______________________________________________
> 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: [PATCH 0/7] Configurable error behavior [V3]

[Carlos Maiolino]

On Thu, May 05, 2016 at 10:11:07AM -0400, Brian Foster wrote:
> On Wed, May 04, 2016 at 05:43:13PM +0200, Carlos Maiolino wrote:
> > This is the new revision of this patchset, according to last comments.
> > 
> > This patchset is aimed to implement a configurable error behavior in XFS, and
> > most of the design has been done by Dave, so, that's why I kept his signed-off
> > in the patches.
> > 
> > This new revision has the detailed changelog written on each patch, but the
> > major changes are:
> > 
> > 	- Detailed changelog by-patch and description fixed to become
> > 	  (hopefuly) more clear
> > 	- kept fail_at_unmount as a sysfs attribute
> > 
> > 
> > Regarding fail_at_unmount, I left it almost exactly as Dave's design, giving his
> > comments on the last revision, although, I still think there is no need to keep
> > it as a per-error granularity, so, I was wondering if a single, global option in
> > /sys/fs/xfs/<dev>/error/fail_at_unmount wouldn't suffice, but, this will require
> > a new place to store the value inside kernel, instead of keeping it inside
> > struct xfs_error_cfg, or maybe use the same structure but use it outside of the
> > m_error_cfg array?
> > 
> 
> I agree with regard to the granularity of fail_at_unmount. This was
> brought up previously:
> 
> http://oss.sgi.com/archives/xfs/2016-02/msg00558.html
> 
> ... and I haven't heard a use case for per-error granularity.

Hi, yes, my comment was based on our previous discussion, my apologies to not
have made it clear.

> 
> I suggest just to pull it out of the error classification stuff entirely
> and place it under xfs_mount. E.g., at the same level as "fail_writes"
> (but not a DEBUG mode only option).
> 
> I'm also wondering whether we need more mechanism for the
> fail_at_unmount behavior. For example, instead of defining
> XFS_MOUNT_UNMOUNTING, could we just call a function that resets
> max_retries (of each class) to 0 in the unmount path? Then maybe call
> the mount tunable retry_on_unmount or something like that. Thoughts?
> 
I don't oppose to that, although, having a flag like XFS_MOUNT_UNMOUNTING, might
be useful in the future, but still, wouldn't be better this single flag, instead
of walk through all classes/errors resetting the max_retries? It sounds as
granular as having fail_at_unmount inside each error, despite the fact it's not
exposed to user-space, we will need to interact over each max_retries to
actually shutdown the filesystem during unmount, which, is also error-prone
IMHO.
It also depends on how granular we will implement fail_at_unmount. If it's a
single global option, resetting all max_retries works, otherwise it might not
work, for example, if we decide to have fail_at_unmount for each class, we might
need to reset max_retries only in specific errors, which will increase the
complexity of the code.

Well, hope my comments make sense, just giving my $0.02 :)

cheers

> Brian
> 
> > First 6 patches are ready, the fail_at_unmount one, need to be re-worked if we
> > want it in a less granular way, but until now I don't think we reached any
> > decision about how it should be implemented.
> > 
> >  fs/xfs/xfs_buf.h      |  22 ++++
> >  fs/xfs/xfs_buf_item.c | 126 ++++++++++++++--------
> >  fs/xfs/xfs_mount.c    |  19 +++-
> >  fs/xfs/xfs_mount.h    |  32 ++++++
> >  fs/xfs/xfs_sysfs.c    | 283 +++++++++++++++++++++++++++++++++++++++++++++++++-
> >  fs/xfs/xfs_sysfs.h    |   3 +
> >  6 files changed, 437 insertions(+), 48 deletions(-)
> > 
> > -- 
> > 2.4.11
> > 
> > _______________________________________________
> > 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

-- 
Carlos

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

Re: [PATCH 0/7] Configurable error behavior [V3]

[Brian Foster]

On Thu, May 05, 2016 at 04:37:18PM +0200, Carlos Maiolino wrote:
> On Thu, May 05, 2016 at 10:11:07AM -0400, Brian Foster wrote:
> > On Wed, May 04, 2016 at 05:43:13PM +0200, Carlos Maiolino wrote:
> > > This is the new revision of this patchset, according to last comments.
> > > 
> > > This patchset is aimed to implement a configurable error behavior in XFS, and
> > > most of the design has been done by Dave, so, that's why I kept his signed-off
> > > in the patches.
> > > 
> > > This new revision has the detailed changelog written on each patch, but the
> > > major changes are:
> > > 
> > > 	- Detailed changelog by-patch and description fixed to become
> > > 	  (hopefuly) more clear
> > > 	- kept fail_at_unmount as a sysfs attribute
> > > 
> > > 
> > > Regarding fail_at_unmount, I left it almost exactly as Dave's design, giving his
> > > comments on the last revision, although, I still think there is no need to keep
> > > it as a per-error granularity, so, I was wondering if a single, global option in
> > > /sys/fs/xfs/<dev>/error/fail_at_unmount wouldn't suffice, but, this will require
> > > a new place to store the value inside kernel, instead of keeping it inside
> > > struct xfs_error_cfg, or maybe use the same structure but use it outside of the
> > > m_error_cfg array?
> > > 
> > 
> > I agree with regard to the granularity of fail_at_unmount. This was
> > brought up previously:
> > 
> > http://oss.sgi.com/archives/xfs/2016-02/msg00558.html
> > 
> > ... and I haven't heard a use case for per-error granularity.
> 
> Hi, yes, my comment was based on our previous discussion, my apologies to not
> have made it clear.
> 

Ok..

> > 
> > I suggest just to pull it out of the error classification stuff entirely
> > and place it under xfs_mount. E.g., at the same level as "fail_writes"
> > (but not a DEBUG mode only option).
> > 
> > I'm also wondering whether we need more mechanism for the
> > fail_at_unmount behavior. For example, instead of defining
> > XFS_MOUNT_UNMOUNTING, could we just call a function that resets
> > max_retries (of each class) to 0 in the unmount path? Then maybe call
> > the mount tunable retry_on_unmount or something like that. Thoughts?
> > 
> I don't oppose to that, although, having a flag like XFS_MOUNT_UNMOUNTING, might
> be useful in the future, but still, wouldn't be better this single flag, instead
> of walk through all classes/errors resetting the max_retries? It sounds as
> granular as having fail_at_unmount inside each error, despite the fact it's not
> exposed to user-space, we will need to interact over each max_retries to
> actually shutdown the filesystem during unmount, which, is also error-prone
> IMHO.

I view the granularity problem as a usability problem, not necessarily a
code problem. E.g., why would somebody know or care to configure certain
errors to fail on unmount but not others. If we have a knob, I think the
knob is more clear as a general behavior knob rather than an error
classification knob. Of course, that assumes there isn't some unknown
good reason for per-error behavior (and/or a userspace mgmt tool that
could provide a more usable interface on top of per-error knobs).

> It also depends on how granular we will implement fail_at_unmount. If it's a
> single global option, resetting all max_retries works, otherwise it might not
> work, for example, if we decide to have fail_at_unmount for each class, we might
> need to reset max_retries only in specific errors, which will increase the
> complexity of the code.
> 

I'm assuming a per-mount option is sufficient. :) Otherwise, I'm just
thinking out loud for ways to try and condense and/or reuse the code a
bit here. I don't see a reason to add new mechanisms or config tunables
in cases where we can accomplish the same thing by making existing
knobs/mechanisms sufficiently generic.

Sure, the code might be slightly more complex (or maybe some of the
existing code can be refactored to support a reinit) and it might
introduce the issue of unmount racing against sysfs knob updates. The
tradeoff is that it reuses an existing mechanism, for what that's worth.
Just an idea, though. ;)

Brian

> Well, hope my comments make sense, just giving my $0.02 :)
> 
> cheers
> 
> > Brian
> > 
> > > First 6 patches are ready, the fail_at_unmount one, need to be re-worked if we
> > > want it in a less granular way, but until now I don't think we reached any
> > > decision about how it should be implemented.
> > > 
> > >  fs/xfs/xfs_buf.h      |  22 ++++
> > >  fs/xfs/xfs_buf_item.c | 126 ++++++++++++++--------
> > >  fs/xfs/xfs_mount.c    |  19 +++-
> > >  fs/xfs/xfs_mount.h    |  32 ++++++
> > >  fs/xfs/xfs_sysfs.c    | 283 +++++++++++++++++++++++++++++++++++++++++++++++++-
> > >  fs/xfs/xfs_sysfs.h    |   3 +
> > >  6 files changed, 437 insertions(+), 48 deletions(-)
> > > 
> > > -- 
> > > 2.4.11
> > > 
> > > _______________________________________________
> > > 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
> 
> -- 
> Carlos
> 
> _______________________________________________
> 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: [PATCH 0/7] Configurable error behavior [V3]

[Dave Chinner]

On Thu, May 05, 2016 at 11:18:28AM -0400, Brian Foster wrote:
> On Thu, May 05, 2016 at 04:37:18PM +0200, Carlos Maiolino wrote:
> > On Thu, May 05, 2016 at 10:11:07AM -0400, Brian Foster wrote:
> > > I suggest just to pull it out of the error classification stuff entirely
> > > and place it under xfs_mount. E.g., at the same level as "fail_writes"
> > > (but not a DEBUG mode only option).
> > > 
> > > I'm also wondering whether we need more mechanism for the
> > > fail_at_unmount behavior. For example, instead of defining
> > > XFS_MOUNT_UNMOUNTING, could we just call a function that resets
> > > max_retries (of each class) to 0 in the unmount path? Then maybe call
> > > the mount tunable retry_on_unmount or something like that. Thoughts?
> > > 
> > I don't oppose to that, although, having a flag like XFS_MOUNT_UNMOUNTING, might
> > be useful in the future, but still, wouldn't be better this single flag, instead
> > of walk through all classes/errors resetting the max_retries? It sounds as
> > granular as having fail_at_unmount inside each error, despite the fact it's not
> > exposed to user-space, we will need to interact over each max_retries to
> > actually shutdown the filesystem during unmount, which, is also error-prone
> > IMHO.
> 
> I view the granularity problem as a usability problem, not necessarily a
> code problem. E.g., why would somebody know or care to configure certain
> errors to fail on unmount but not others. If we have a knob, I think the
> knob is more clear as a general behavior knob rather than an error
> classification knob. Of course, that assumes there isn't some unknown
> good reason for per-error behavior (and/or a userspace mgmt tool that
> could provide a more usable interface on top of per-error knobs).

FWIW, this is exactly what we need to work out with this patch -
what makes sense in the UAPI ;)

I have to agree with Brian here - it doesn't make a whole lot of
sense to have per-error "fail-at-umount" configuration without an
obvious use-case. Hence a single option at the top level of the
error heirarchy makes sense i.e.
/sys/fs/xfs/<dev>/error/fail_at_unmount.

If, in future, someone needs more fine-grained control we can always
add it back in to the per-error config as an override to the global
setting...

> > It also depends on how granular we will implement fail_at_unmount. If it's a
> > single global option, resetting all max_retries works, otherwise it might not
> > work, for example, if we decide to have fail_at_unmount for each class, we might
> > need to reset max_retries only in specific errors, which will increase the
> > complexity of the code.
> > 
> 
> I'm assuming a per-mount option is sufficient. :) Otherwise, I'm just
> thinking out loud for ways to try and condense and/or reuse the code a
> bit here. I don't see a reason to add new mechanisms or config tunables
> in cases where we can accomplish the same thing by making existing
> knobs/mechanisms sufficiently generic.

Code re-use is good, but I don't think it is good when it conflates
or obfuscates specific behaviours we are checking for.  As such, I'd
prefer that we have a generic flag we can check for an unmount in
progress. There's likely to be code outside the IO error checking
that we might need to check for unmount, so we really should have a
separate flag for it and make all checks explicit.

Think of it this way: we explicitly check for a shutdown situation
to trigger imeediate failure processing in the IO path. Modifying
max retries at unmount to trigger immediate failure is akin to doing
the same thing when a shutdown is triggered rather than checking for
the shutdown flag.

IOWs, the flag is as much documentation as it is implementation - it
makes it explicit that this code path was intended to allow failure
at unmount, just like it is explicit that we intend to fail if a
shutdown is detected...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

Re: [PATCH 6/7] xfs: add configuration handlers for specific errors

[Dave Chinner]

On Thu, May 05, 2016 at 10:11:02AM -0400, Brian Foster wrote:
> On Wed, May 04, 2016 at 05:43:19PM +0200, Carlos Maiolino wrote:
> > now most of the infrastructure is in place, we can start adding
> > support for configuring specific errors such as ENODEV, ENOSPC, EIO,
> > etc. Add these error configurations and configure them all to have
> > appropriate behaviours. That is, all will be configured to retry forever by
> > default, except for ENODEV, which is an unrecoverable error, so it will be
> > configured to not retry on error
> > 
> > Changelog:
> > 
> > V3:
> > 	- Do not implement .fail_speed and .fail_at_unmount
> > 	- .fail_at_unmount will be implemented in a different patch
> > 
> > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> > Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> > ---
> >  fs/xfs/xfs_mount.h |  3 +++
> >  fs/xfs/xfs_sysfs.c | 20 ++++++++++++++++++++
> >  2 files changed, 23 insertions(+)
> > 
> > diff --git a/fs/xfs/xfs_mount.h b/fs/xfs/xfs_mount.h
> > index 0382140..e3b3267 100644
> > --- a/fs/xfs/xfs_mount.h
> > +++ b/fs/xfs/xfs_mount.h
> > @@ -49,6 +49,9 @@ enum {
> >  };
> >  enum {
> >  	XFS_ERR_DEFAULT,
> > +	XFS_ERR_EIO,
> > +	XFS_ERR_ENOSPC,
> > +	XFS_ERR_ENODEV,
> >  	XFS_ERR_ERRNO_MAX,
> >  };
> >  
> > diff --git a/fs/xfs/xfs_sysfs.c b/fs/xfs/xfs_sysfs.c
> > index c881360..1ed9033 100644
> > --- a/fs/xfs/xfs_sysfs.c
> > +++ b/fs/xfs/xfs_sysfs.c
> > @@ -481,6 +481,17 @@ static const struct xfs_error_init xfs_error_meta_init[XFS_ERR_ERRNO_MAX] = {
> >  	  .max_retries = -1,
> >  	  .retry_timeout = 0,
> >  	},
> > +	{ .name = "EIO",
> > +	  .max_retries = -1,
> > +	  .retry_timeout = 0,
> > +	},
> > +	{ .name = "ENOSPC",
> > +	  .max_retries = -1,
> > +	  .retry_timeout = 0,
> > +	},
> > +	{ .name = "ENODEV",
> > +	  .max_retries = -1,
> 
> Shouldn't this be 0?

If they are supposed to be "retry forever", they shoul dbe set to
the define I asked to be added in the last review. i.e.
XFS_ERR_RETRY_FOREVER. But if a device goes away, then we may as
well fail immediately as it will never come back as the same
device....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

Re: [PATCH 5/7] xfs: add configuration of error failure speed

[Dave Chinner]

On Wed, May 04, 2016 at 05:43:18PM +0200, Carlos Maiolino wrote:
> On reception of an error, we can fail immediately, perform some
> bound amount of retries or retry indefinitely. The current behaviour
> we have is to retry forever.
> 
> However, we'd like the ability to choose how long the filesystem should try
> after an error, it can either fail immediately, retry a few times, or retry
> forever. This is implemented by using max_retries sysfs attribute, to hold the
> amount of times we allow the filesystem to retry after an error. Being -1 a
> special case where the filesystem will retry indefinitely.
> 
> Add both a maximum retry count and a retry timeout so that we can bound by
> time and/or physical IO attempts.
> 
> Finally, plumb these into xfs_buf_iodone error processing so that
> the error behaviour follows the selected configuration.
> 
> Changelog:
> 
> V3:
> 	- In xfs_buf_iodone_callback_error, use max_retries to decide how long
> 	  the filesystem should retry on errors instead of XFS_ERR_FAIL enums
> 	  and fail_speed
> 
> 	- Remove all code implementing fail_speed attribute from the original
> 	  patch
> 		-- failure_speed_show/store attributes function implementation
> 		-- max_retries_store() now accepts values from -1 up to INT_MAX
> 
> 	- retry_timeout_seconds_show() print fixed:
> 		-- jiffies_to_msecs() should be divided by MSEC_PER_SEC
> 		-- trailing whitespace removed

Where's XFS_ERR_RETRY_FOREVER?

> @@ -1095,8 +1098,12 @@ xfs_buf_iodone_callback_error(
>  	 * Repeated failure on an async write. Take action according to the
>  	 * error configuration we have been set up to use.
>  	 */
> -	if (!cfg->max_retries)
> -		goto permanent_error;
> +	if ((cfg->max_retries >= 0) &&
> +	    (++bp->b_retries > cfg->max_retries))
> +			goto permanent_error;

I suggested:

        if (cfg->max_retries != XFS_ERR_RETRY_FOREVER &&
            ++bp->b_retries > cfg->max_retries)
                goto permanent_error;

so that we document that there is a "retry forever" case being
handled here. I really don't like magic "-1", ">= 0" or other
implicit comparisions that don't document that it is valid to retry
forever in these cases.

> +	if (cfg->retry_timeout &&
> +	    time_after(jiffies, cfg->retry_timeout + bp->b_first_retry_time))
> +			goto permanent_error;
>  
>  	/* still a transient error, higher layers will retry */
>  	xfs_buf_ioerror(bp, 0);
> @@ -1139,6 +1146,7 @@ xfs_buf_iodone_callbacks(
>  	 * retry state here in preparation for the next error that may occur.
>  	 */
>  	bp->b_last_error = 0;
> +	bp->b_retries = 0;
>  
>  	xfs_buf_do_callbacks(bp);
>  	bp->b_fspriv = NULL;
> diff --git a/fs/xfs/xfs_mount.h b/fs/xfs/xfs_mount.h
> index 0c5a976..0382140 100644
> --- a/fs/xfs/xfs_mount.h
> +++ b/fs/xfs/xfs_mount.h
> @@ -54,7 +54,8 @@ enum {
>  
>  struct xfs_error_cfg {
>  	struct xfs_kobj	kobj;
> -	int		max_retries;
> +	int		max_retries;	/* -1 = retry forever */

as per my last review, remove the comment, add XFS_ERR_RETRY_FOREVER
to document that "-1 = retry forever" and use that in the code so
it's explicit that the code is intended to handle this case.

Cheers,

Dave.

-- 
Dave Chinner
david@fromorbit.com

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

Re: [PATCH 5/7] xfs: add configuration of error failure speed

[Carlos Maiolino]

On Fri, May 06, 2016 at 10:04:33AM +1000, Dave Chinner wrote:
> On Wed, May 04, 2016 at 05:43:18PM +0200, Carlos Maiolino wrote:
> > On reception of an error, we can fail immediately, perform some
> > bound amount of retries or retry indefinitely. The current behaviour
> > we have is to retry forever.
> > 
> > However, we'd like the ability to choose how long the filesystem should try
> > after an error, it can either fail immediately, retry a few times, or retry
> > forever. This is implemented by using max_retries sysfs attribute, to hold the
> > amount of times we allow the filesystem to retry after an error. Being -1 a
> > special case where the filesystem will retry indefinitely.
> > 
> > Add both a maximum retry count and a retry timeout so that we can bound by
> > time and/or physical IO attempts.
> > 
> > Finally, plumb these into xfs_buf_iodone error processing so that
> > the error behaviour follows the selected configuration.
> > 
> > Changelog:
> > 
> > V3:
> > 	- In xfs_buf_iodone_callback_error, use max_retries to decide how long
> > 	  the filesystem should retry on errors instead of XFS_ERR_FAIL enums
> > 	  and fail_speed
> > 
> > 	- Remove all code implementing fail_speed attribute from the original
> > 	  patch
> > 		-- failure_speed_show/store attributes function implementation
> > 		-- max_retries_store() now accepts values from -1 up to INT_MAX
> > 
> > 	- retry_timeout_seconds_show() print fixed:
> > 		-- jiffies_to_msecs() should be divided by MSEC_PER_SEC
> > 		-- trailing whitespace removed
> 
> Where's XFS_ERR_RETRY_FOREVER?

Hi, I didn't understand you literally meant to have XFS_ERR_RETRY_FOREVER macro
implemented, sorry about that, I'll implement it, add fail_at_unmount to
<dev>/error and re-send the patchset

> 
> > @@ -1095,8 +1098,12 @@ xfs_buf_iodone_callback_error(
> >  	 * Repeated failure on an async write. Take action according to the
> >  	 * error configuration we have been set up to use.
> >  	 */
> > -	if (!cfg->max_retries)
> > -		goto permanent_error;
> > +	if ((cfg->max_retries >= 0) &&
> > +	    (++bp->b_retries > cfg->max_retries))
> > +			goto permanent_error;
> 
> I suggested:
> 
>         if (cfg->max_retries != XFS_ERR_RETRY_FOREVER &&
>             ++bp->b_retries > cfg->max_retries)
>                 goto permanent_error;
> 
> so that we document that there is a "retry forever" case being
> handled here. I really don't like magic "-1", ">= 0" or other
> implicit comparisions that don't document that it is valid to retry
> forever in these cases.
> 
> > +	if (cfg->retry_timeout &&
> > +	    time_after(jiffies, cfg->retry_timeout + bp->b_first_retry_time))
> > +			goto permanent_error;
> >  
> >  	/* still a transient error, higher layers will retry */
> >  	xfs_buf_ioerror(bp, 0);
> > @@ -1139,6 +1146,7 @@ xfs_buf_iodone_callbacks(
> >  	 * retry state here in preparation for the next error that may occur.
> >  	 */
> >  	bp->b_last_error = 0;
> > +	bp->b_retries = 0;
> >  
> >  	xfs_buf_do_callbacks(bp);
> >  	bp->b_fspriv = NULL;
> > diff --git a/fs/xfs/xfs_mount.h b/fs/xfs/xfs_mount.h
> > index 0c5a976..0382140 100644
> > --- a/fs/xfs/xfs_mount.h
> > +++ b/fs/xfs/xfs_mount.h
> > @@ -54,7 +54,8 @@ enum {
> >  
> >  struct xfs_error_cfg {
> >  	struct xfs_kobj	kobj;
> > -	int		max_retries;
> > +	int		max_retries;	/* -1 = retry forever */
> 
> as per my last review, remove the comment, add XFS_ERR_RETRY_FOREVER
> to document that "-1 = retry forever" and use that in the code so
> it's explicit that the code is intended to handle this case.
> 
> Cheers,
> 
> Dave.
> 
> -- 
> Dave Chinner
> david@fromorbit.com
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs

-- 
Carlos

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

[PATCH 5/7] xfs: add configuration of error failure speed

[Carlos Maiolino]

On reception of an error, we can fail immediately, perform some
bound amount of retries or retry indefinitely. The current behaviour
we have is to retry forever.

However, we'd like the ability to choose how long the filesystem should try
after an error, it can either fail immediately, retry a few times, or retry
forever. This is implemented by using max_retries sysfs attribute, to hold the
amount of times we allow the filesystem to retry after an error. Being -1 a
special case where the filesystem will retry indefinitely.

Add both a maximum retry count and a retry timeout so that we can bound by
time and/or physical IO attempts.

Finally, plumb these into xfs_buf_iodone error processing so that
the error behaviour follows the selected configuration.

Changelog:

V3:
	- In xfs_buf_iodone_callback_error, use max_retries to decide how long
	  the filesystem should retry on errors instead of XFS_ERR_FAIL enums
	  and fail_speed

	- Remove all code implementing fail_speed attribute from the original
	  patch
		-- failure_speed_show/store attributes function implementation
		-- max_retries_store() now accepts values from -1 up to INT_MAX

	- retry_timeout_seconds_show() print fixed:
		-- jiffies_to_msecs() should be divided by MSEC_PER_SEC
		-- trailing whitespace removed

V4:
	- Add XFS_ERR_RETRY_FOREVER flag, to avoid a -1 magic number
	- Reword xfs_buf comment according Brian's suggestion
	- Remove pointless check for val > INT_MAX


Signed-off-by: Dave Chinner <dchinner@redhat.com>
Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
---
 fs/xfs/xfs_buf.h      | 21 ++++++++++++-
 fs/xfs/xfs_buf_item.c | 13 +++++++--
 fs/xfs/xfs_mount.h    |  3 ++
 fs/xfs/xfs_sysfs.c    | 81 ++++++++++++++++++++++++++++++++++++++++++++++++---
 4 files changed, 111 insertions(+), 7 deletions(-)

diff --git a/fs/xfs/xfs_buf.h b/fs/xfs/xfs_buf.h
index adef116..8bfb974 100644
--- a/fs/xfs/xfs_buf.h
+++ b/fs/xfs/xfs_buf.h
@@ -183,7 +183,26 @@ typedef struct xfs_buf {
 	unsigned int		b_page_count;	/* size of page array */
 	unsigned int		b_offset;	/* page offset in first page */
 	int			b_error;	/* error code on I/O */
-	int			b_last_error;	/* previous async I/O error */
+
+	/*
+	 * async write failure retry count. Initialised to zero on the first
+	 * failure, then when it exceeds the maximum configured without a
+	 * success the write is considered to be failed permanently and the
+	 * iodone handler will take appropriate action.
+	 *
+	 * For retry timeouts, we record the jiffie of the first failure. This
+	 * means that we can change the retry timeout for buffers already under
+	 * I/O and thus avoid getting stuck in a retry loop with a long timeout.
+	 *
+	 * last_error is used to ensure that we are getting repeated errors, not
+	 * different errors. e.g. a block device might change ENOSPC to EIO when
+	 * a failure timeout occurs, so we want to re-initialise the error
+	 * retry behaviour appropriately when that happens.
+	 */
+	int			b_retries;
+	unsigned long		b_first_retry_time; /* in jiffies */
+	int			b_last_error;
+
 	const struct xfs_buf_ops	*b_ops;
 
 #ifdef XFS_BUF_LOCK_TRACKING
diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
index b8d0cd4..0d95c59 100644
--- a/fs/xfs/xfs_buf_item.c
+++ b/fs/xfs/xfs_buf_item.c
@@ -1085,6 +1085,9 @@ xfs_buf_iodone_callback_error(
 		bp->b_flags |= (XBF_WRITE | XBF_ASYNC |
 			        XBF_DONE | XBF_WRITE_FAIL);
 		bp->b_last_error = bp->b_error;
+		bp->b_retries = 0;
+		bp->b_first_retry_time = jiffies;
+
 		xfs_buf_ioerror(bp, 0);
 		xfs_buf_submit(bp);
 		return true;
@@ -1095,8 +1098,13 @@ xfs_buf_iodone_callback_error(
 	 * error configuration we have been set up to use.
 	 */
 	cfg = xfs_error_get_cfg(mp, XFS_ERR_METADATA, bp->b_error);
-	if (!cfg->max_retries)
-		goto permanent_error;
+
+	if (cfg->max_retries != XFS_ERR_RETRY_FOREVER &&
+	    ++bp->b_retries > cfg->max_retries)
+			goto permanent_error;
+	if (cfg->retry_timeout &&
+	    time_after(jiffies, cfg->retry_timeout + bp->b_first_retry_time))
+			goto permanent_error;
 
 	/* still a transient error, higher layers will retry */
 	xfs_buf_ioerror(bp, 0);
@@ -1139,6 +1147,7 @@ xfs_buf_iodone_callbacks(
 	 * retry state here in preparation for the next error that may occur.
 	 */
 	bp->b_last_error = 0;
+	bp->b_retries = 0;
 
 	xfs_buf_do_callbacks(bp);
 	bp->b_fspriv = NULL;
diff --git a/fs/xfs/xfs_mount.h b/fs/xfs/xfs_mount.h
index 0c5a976..2fafa94 100644
--- a/fs/xfs/xfs_mount.h
+++ b/fs/xfs/xfs_mount.h
@@ -52,9 +52,12 @@ enum {
 	XFS_ERR_ERRNO_MAX,
 };
 
+#define XFS_ERR_RETRY_FOREVER	-1
+
 struct xfs_error_cfg {
 	struct xfs_kobj	kobj;
 	int		max_retries;
+	unsigned long	retry_timeout;	/* in jiffies, 0 = no timeout */
 };
 
 typedef struct xfs_mount {
diff --git a/fs/xfs/xfs_sysfs.c b/fs/xfs/xfs_sysfs.c
index 71046d9..918d144 100644
--- a/fs/xfs/xfs_sysfs.c
+++ b/fs/xfs/xfs_sysfs.c
@@ -374,10 +374,6 @@ struct kobj_type xfs_log_ktype = {
  * and any other future type of IO (e.g. special inode or directory error
  * handling) we care to support.
  */
-static struct attribute *xfs_error_attrs[] = {
-	NULL,
-};
-
 static inline struct xfs_error_cfg *
 to_error_cfg(struct kobject *kobject)
 {
@@ -385,6 +381,79 @@ to_error_cfg(struct kobject *kobject)
 	return container_of(kobj, struct xfs_error_cfg, kobj);
 }
 
+static ssize_t
+max_retries_show(
+	struct kobject	*kobject,
+	char		*buf)
+{
+	struct xfs_error_cfg *cfg = to_error_cfg(kobject);
+
+	return snprintf(buf, PAGE_SIZE, "%d\n", cfg->max_retries);
+}
+
+static ssize_t
+max_retries_store(
+	struct kobject	*kobject,
+	const char	*buf,
+	size_t		count)
+{
+	struct xfs_error_cfg *cfg = to_error_cfg(kobject);
+	int		ret;
+	int		val;
+
+	ret = kstrtoint(buf, 0, &val);
+	if (ret)
+		return ret;
+
+	if (val < -1)
+		return -EINVAL;
+
+	cfg->max_retries = val;
+	return count;
+}
+XFS_SYSFS_ATTR_RW(max_retries);
+
+static ssize_t
+retry_timeout_seconds_show(
+	struct kobject	*kobject,
+	char		*buf)
+{
+	struct xfs_error_cfg *cfg = to_error_cfg(kobject);
+
+	return snprintf(buf, PAGE_SIZE, "%ld\n",
+			jiffies_to_msecs(cfg->retry_timeout) / MSEC_PER_SEC);
+}
+
+static ssize_t
+retry_timeout_seconds_store(
+	struct kobject	*kobject,
+	const char	*buf,
+	size_t		count)
+{
+	struct xfs_error_cfg *cfg = to_error_cfg(kobject);
+	int		ret;
+	int		val;
+
+	ret = kstrtoint(buf, 0, &val);
+	if (ret)
+		return ret;
+
+	/* 1 day timeout maximum */
+	if (val < 0 || val > 86400)
+		return -EINVAL;
+
+	cfg->retry_timeout = msecs_to_jiffies(val * MSEC_PER_SEC);
+	return count;
+}
+XFS_SYSFS_ATTR_RW(retry_timeout_seconds);
+
+static struct attribute *xfs_error_attrs[] = {
+	ATTR_LIST(max_retries),
+	ATTR_LIST(retry_timeout_seconds),
+	NULL,
+};
+
+
 struct kobj_type xfs_error_cfg_ktype = {
 	.release = xfs_sysfs_release,
 	.sysfs_ops = &xfs_sysfs_ops,
@@ -404,11 +473,13 @@ struct kobj_type xfs_error_ktype = {
 struct xfs_error_init {
 	char		*name;
 	int		max_retries;
+	int		retry_timeout;	/* in seconds */
 };
 
 static const struct xfs_error_init xfs_error_meta_init[XFS_ERR_ERRNO_MAX] = {
 	{ .name = "default",
 	  .max_retries = -1,
+	  .retry_timeout = 0,
 	},
 };
 
@@ -439,6 +510,8 @@ xfs_error_sysfs_init_class(
 			goto out_error;
 
 		cfg->max_retries = init[i].max_retries;
+		cfg->retry_timeout = msecs_to_jiffies(
+					init[i].retry_timeout * MSEC_PER_SEC);
 	}
 	return 0;
 
-- 
2.4.11

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

Re: [PATCH 5/7] xfs: add configuration of error failure speed

[Carlos Maiolino]

On Wed, May 04, 2016 at 09:24:19AM +1000, Dave Chinner wrote:
> On Tue, May 03, 2016 at 07:55:38PM +0200, Carlos Maiolino wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> > 
> > On reception of an error, we can fail immediately, perform some
> > bound amount of retries or retry indefinitely. The current behaviour
> > we have is to retry forever.
> > 
> > However, we'd like the ability to choose what behaviour we have, and
> > that requires the ability to configure the behaviour through the new
> > sysfs interfaces.
> > 
> > max_retries is used to set the number of retries to do until permanently fail,
> > the only special case is when it is set to -1, which, will cause the system to
> > retry indefinitely.
> 
> This doesn't read particularly well, and its more a description of
> the mechanism (which shoul dbe in the code) rather than the reason
> for implementing it.. The original commit message talked about
> different failure characteristics, which still need to be mentioned
> here. e.g:
> 
> 	We'd like to be able to configure errors to either fail
> 	immediately (fail fast), retry for some time before
> 	failing (fail slow) or retry forever (fail never). This is
> 	implemented by using specific values for the number of
> 	retries we allow.

Fair enough, I will re-work the patch descriptions in the next version.


> > 
> > Add both a maximum retry count and a retry timeout so that we can
> > bound by time and/or physical IO attempts.
> > 
> > Finally, plumb these into xfs_buf_iodone error processing so that
> > the error behaviour follows the selected configuration.
> > 
> > Signed-off-by: Dave Chinner <dchinner@redhat.com> Signed-off-by:
> > Carlos Maiolino <cmaiolino@redhat.com>
> 
> You also need to document what changes you've made to the original
> patches. I can't tell what you've changed in each patch just by
> looking at them - this is the first one where I noticed something
> while reading it. i.e. the commit message should have:
> 
> [ cmaiolino: removed fail-fast/slow/never and instead use retry count
> to determine behaviour ]
>

Ok
 
> > ---
> >  fs/xfs/xfs_buf.h      | 23 ++++++++++++++-
> >  fs/xfs/xfs_buf_item.c | 13 +++++++--
> >  fs/xfs/xfs_mount.h    |  3 +-
> >  fs/xfs/xfs_sysfs.c    | 81 ++++++++++++++++++++++++++++++++++++++++++++++++---
> >  4 files changed, 112 insertions(+), 8 deletions(-)
> > 
> > diff --git a/fs/xfs/xfs_buf.h b/fs/xfs/xfs_buf.h
> > index adef116..bd978f0 100644
> > --- a/fs/xfs/xfs_buf.h
> > +++ b/fs/xfs/xfs_buf.h
> > @@ -183,7 +183,28 @@ typedef struct xfs_buf {
> >  	unsigned int		b_page_count;	/* size of page array */
> >  	unsigned int		b_offset;	/* page offset in first page */
> >  	int			b_error;	/* error code on I/O */
> > -	int			b_last_error;	/* previous async I/O error */
> > +
> > +	/*
> > +	 * async write failure retry count. Initialised to zero on the first
> > +	 * failure, then when it exceeds the maximum configured without a
> > +	 * success the write is considered to be failed permanently and the
> > +	 * iodone handler will take appropriate action.
> > +	 *
> > +	 * For retry timeouts, we record the jiffie of the first failure. This
> > +	 * means that we can change the retry timeout and it on the next error
> > +	 * it will be checked against the newly configured timeout. This
> > +	 * prevents buffers getting stuck in retry loops with a really long
> > +	 * timeout.
> > +	 *
> > +	 * last_error is used to ensure that we are getting repeated errors, not
> > +	 * different errors. e.g. a block device might change ENOSPC to EIO when
> > +	 * a failure timeout occurs, so we want to re-initialise the error
> > +	 * retry behaviour appropriately when that happens.
> > +	 */
> > +	int			b_retries;
> > +	unsigned long		b_first_retry_time; /* in jiffies */
> > +	int			b_last_error;
> > +
> >  	const struct xfs_buf_ops	*b_ops;
> >  
> >  #ifdef XFS_BUF_LOCK_TRACKING
> > diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
> > index 3a30a88..68ed2a0 100644
> > --- a/fs/xfs/xfs_buf_item.c
> > +++ b/fs/xfs/xfs_buf_item.c
> > @@ -1086,6 +1086,9 @@ xfs_buf_iodone_callback_error(
> >  		bp->b_flags |= (XBF_WRITE | XBF_ASYNC |
> >  			        XBF_DONE | XBF_WRITE_FAIL);
> >  		bp->b_last_error = bp->b_error;
> > +		bp->b_retries = 0;
> > +		bp->b_first_retry_time = jiffies;
> > +
> >  		xfs_buf_ioerror(bp, 0);
> >  		xfs_buf_submit(bp);
> >  		return true;
> > @@ -1095,8 +1098,13 @@ xfs_buf_iodone_callback_error(
> >  	 * Repeated failure on an async write. Take action according to the
> >  	 * error configuration we have been set up to use.
> >  	 */
> > -	if (!cfg->max_retries)
> > -		goto permanent_error;
> > +	if (cfg->max_retries >= 0)
> > +		if (++bp->b_retries > cfg->max_retries)
> > +			goto permanent_error;
> > +	if (cfg->retry_timeout)
> > +		if (time_after(jiffies,
> > +			       cfg->retry_timeout + bp->b_first_retry_time))
> > +			goto permanent_error;
> 
> This is hard to read and the pattern is prone to future maintenance
> problems. i.e. nested single line if statements are prone to people
> making mistakes with indentation when adding more conditions to
> them.
> 
> So:
> 
> 	if (cfg->max_retries != XFS_ERR_RETRY_FOREVER &&
> 	    ++bp->b_retries > cfg->max_retries)
> 		goto permanent_error;
> 	if (cfg->retry_timeout &&
> 	    time_after(jiffies, cfg->retry_timeout + bp->b_first_retry_time))
> 		goto permanent_error;
>

Fair enough, will change this in the next revision.

Thanks for the review
 
> >  	/* still a transient error, higher layers will retry */
> >  	xfs_buf_ioerror(bp, 0);
> > @@ -1139,6 +1147,7 @@ xfs_buf_iodone_callbacks(
> >  	 * retry state here in preparation for the next error that may occur.
> >  	 */
> >  	bp->b_last_error = 0;
> > +	bp->b_retries = 0;
> >  
> >  	xfs_buf_do_callbacks(bp);
> >  	bp->b_fspriv = NULL;
> > diff --git a/fs/xfs/xfs_mount.h b/fs/xfs/xfs_mount.h
> > index 0c5a976..0382140 100644
> > --- a/fs/xfs/xfs_mount.h
> > +++ b/fs/xfs/xfs_mount.h
> > @@ -54,7 +54,8 @@ enum {
> >  
> >  struct xfs_error_cfg {
> >  	struct xfs_kobj	kobj;
> > -	int		max_retries;
> > +	int		max_retries;
> 
> #define XFS_ERR_RETRY_FOREVER	(-1)
> 
> > +	unsigned long	retry_timeout;	/* in jiffies, 0 = no timeout */
> >  };
> ....
> > +static ssize_t
> > +max_retries_store(
> > +	struct kobject	*kobject,
> > +	const char	*buf,
> > +	size_t		count)
> > +{
> > +	struct xfs_error_cfg *cfg = to_error_cfg(kobject);
> > +	int		ret;
> > +	int		val;
> > +
> > +	ret = kstrtoint(buf, 0, &val);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (val < -1 || val > INT_MAX)
> > +		return -EINVAL;
> 
> Should special case XFS_ERR_RETRY_FOREVER here. i.e. range is
> 0-INT_MAX || XFS_ERR_RETRY_FOREVER.
> 
> 	if ((val < -1 || val > INT_MAX) &&
> 	    val != XFS_ERR_RETRY_FOREVER)
> 		return -EINVAL;
> 
> -- 
> Dave Chinner
> david@fromorbit.com

-- 
Carlos

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

Re: [PATCH 5/7] xfs: add configuration of error failure speed

[Dave Chinner]

On Tue, May 03, 2016 at 07:55:38PM +0200, Carlos Maiolino wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> On reception of an error, we can fail immediately, perform some
> bound amount of retries or retry indefinitely. The current behaviour
> we have is to retry forever.
> 
> However, we'd like the ability to choose what behaviour we have, and
> that requires the ability to configure the behaviour through the new
> sysfs interfaces.
> 
> max_retries is used to set the number of retries to do until permanently fail,
> the only special case is when it is set to -1, which, will cause the system to
> retry indefinitely.

This doesn't read particularly well, and its more a description of
the mechanism (which shoul dbe in the code) rather than the reason
for implementing it.. The original commit message talked about
different failure characteristics, which still need to be mentioned
here. e.g:

	We'd like to be able to configure errors to either fail
	immediately (fail fast), retry for some time before
	failing (fail slow) or retry forever (fail never). This is
	implemented by using specific values for the number of
	retries we allow.
> 
> Add both a maximum retry count and a retry timeout so that we can
> bound by time and/or physical IO attempts.
> 
> Finally, plumb these into xfs_buf_iodone error processing so that
> the error behaviour follows the selected configuration.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com> Signed-off-by:
> Carlos Maiolino <cmaiolino@redhat.com>

You also need to document what changes you've made to the original
patches. I can't tell what you've changed in each patch just by
looking at them - this is the first one where I noticed something
while reading it. i.e. the commit message should have:

[ cmaiolino: removed fail-fast/slow/never and instead use retry count
to determine behaviour ]

> ---
>  fs/xfs/xfs_buf.h      | 23 ++++++++++++++-
>  fs/xfs/xfs_buf_item.c | 13 +++++++--
>  fs/xfs/xfs_mount.h    |  3 +-
>  fs/xfs/xfs_sysfs.c    | 81 ++++++++++++++++++++++++++++++++++++++++++++++++---
>  4 files changed, 112 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/xfs/xfs_buf.h b/fs/xfs/xfs_buf.h
> index adef116..bd978f0 100644
> --- a/fs/xfs/xfs_buf.h
> +++ b/fs/xfs/xfs_buf.h
> @@ -183,7 +183,28 @@ typedef struct xfs_buf {
>  	unsigned int		b_page_count;	/* size of page array */
>  	unsigned int		b_offset;	/* page offset in first page */
>  	int			b_error;	/* error code on I/O */
> -	int			b_last_error;	/* previous async I/O error */
> +
> +	/*
> +	 * async write failure retry count. Initialised to zero on the first
> +	 * failure, then when it exceeds the maximum configured without a
> +	 * success the write is considered to be failed permanently and the
> +	 * iodone handler will take appropriate action.
> +	 *
> +	 * For retry timeouts, we record the jiffie of the first failure. This
> +	 * means that we can change the retry timeout and it on the next error
> +	 * it will be checked against the newly configured timeout. This
> +	 * prevents buffers getting stuck in retry loops with a really long
> +	 * timeout.
> +	 *
> +	 * last_error is used to ensure that we are getting repeated errors, not
> +	 * different errors. e.g. a block device might change ENOSPC to EIO when
> +	 * a failure timeout occurs, so we want to re-initialise the error
> +	 * retry behaviour appropriately when that happens.
> +	 */
> +	int			b_retries;
> +	unsigned long		b_first_retry_time; /* in jiffies */
> +	int			b_last_error;
> +
>  	const struct xfs_buf_ops	*b_ops;
>  
>  #ifdef XFS_BUF_LOCK_TRACKING
> diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
> index 3a30a88..68ed2a0 100644
> --- a/fs/xfs/xfs_buf_item.c
> +++ b/fs/xfs/xfs_buf_item.c
> @@ -1086,6 +1086,9 @@ xfs_buf_iodone_callback_error(
>  		bp->b_flags |= (XBF_WRITE | XBF_ASYNC |
>  			        XBF_DONE | XBF_WRITE_FAIL);
>  		bp->b_last_error = bp->b_error;
> +		bp->b_retries = 0;
> +		bp->b_first_retry_time = jiffies;
> +
>  		xfs_buf_ioerror(bp, 0);
>  		xfs_buf_submit(bp);
>  		return true;
> @@ -1095,8 +1098,13 @@ xfs_buf_iodone_callback_error(
>  	 * Repeated failure on an async write. Take action according to the
>  	 * error configuration we have been set up to use.
>  	 */
> -	if (!cfg->max_retries)
> -		goto permanent_error;
> +	if (cfg->max_retries >= 0)
> +		if (++bp->b_retries > cfg->max_retries)
> +			goto permanent_error;
> +	if (cfg->retry_timeout)
> +		if (time_after(jiffies,
> +			       cfg->retry_timeout + bp->b_first_retry_time))
> +			goto permanent_error;

This is hard to read and the pattern is prone to future maintenance
problems. i.e. nested single line if statements are prone to people
making mistakes with indentation when adding more conditions to
them.

So:

	if (cfg->max_retries != XFS_ERR_RETRY_FOREVER &&
	    ++bp->b_retries > cfg->max_retries)
		goto permanent_error;
	if (cfg->retry_timeout &&
	    time_after(jiffies, cfg->retry_timeout + bp->b_first_retry_time))
		goto permanent_error;

>  	/* still a transient error, higher layers will retry */
>  	xfs_buf_ioerror(bp, 0);
> @@ -1139,6 +1147,7 @@ xfs_buf_iodone_callbacks(
>  	 * retry state here in preparation for the next error that may occur.
>  	 */
>  	bp->b_last_error = 0;
> +	bp->b_retries = 0;
>  
>  	xfs_buf_do_callbacks(bp);
>  	bp->b_fspriv = NULL;
> diff --git a/fs/xfs/xfs_mount.h b/fs/xfs/xfs_mount.h
> index 0c5a976..0382140 100644
> --- a/fs/xfs/xfs_mount.h
> +++ b/fs/xfs/xfs_mount.h
> @@ -54,7 +54,8 @@ enum {
>  
>  struct xfs_error_cfg {
>  	struct xfs_kobj	kobj;
> -	int		max_retries;
> +	int		max_retries;

#define XFS_ERR_RETRY_FOREVER	(-1)

> +	unsigned long	retry_timeout;	/* in jiffies, 0 = no timeout */
>  };
....
> +static ssize_t
> +max_retries_store(
> +	struct kobject	*kobject,
> +	const char	*buf,
> +	size_t		count)
> +{
> +	struct xfs_error_cfg *cfg = to_error_cfg(kobject);
> +	int		ret;
> +	int		val;
> +
> +	ret = kstrtoint(buf, 0, &val);
> +	if (ret)
> +		return ret;
> +
> +	if (val < -1 || val > INT_MAX)
> +		return -EINVAL;

Should special case XFS_ERR_RETRY_FOREVER here. i.e. range is
0-INT_MAX || XFS_ERR_RETRY_FOREVER.

	if ((val < -1 || val > INT_MAX) &&
	    val != XFS_ERR_RETRY_FOREVER)
		return -EINVAL;

-- 
Dave Chinner
david@fromorbit.com

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

[PATCH 5/7] xfs: add configuration of error failure speed

[Carlos Maiolino]

From: Dave Chinner <dchinner@redhat.com>

On reception of an error, we can fail immediately, perform some
bound amount of retries or retry indefinitely. The current behaviour
we have is to retry forever.

However, we'd like the ability to choose what behaviour we have, and
that requires the ability to configure the behaviour through the new
sysfs interfaces.

max_retries is used to set the number of retries to do until permanently fail,
the only special case is when it is set to -1, which, will cause the system to
retry indefinitely.

Add both a maximum retry count and a retry timeout so that we can bound by
time and/or physical IO attempts.

Finally, plumb these into xfs_buf_iodone error processing so that
the error behaviour follows the selected configuration.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
---
 fs/xfs/xfs_buf.h      | 23 ++++++++++++++-
 fs/xfs/xfs_buf_item.c | 13 +++++++--
 fs/xfs/xfs_mount.h    |  3 +-
 fs/xfs/xfs_sysfs.c    | 81 ++++++++++++++++++++++++++++++++++++++++++++++++---
 4 files changed, 112 insertions(+), 8 deletions(-)

diff --git a/fs/xfs/xfs_buf.h b/fs/xfs/xfs_buf.h
index adef116..bd978f0 100644
--- a/fs/xfs/xfs_buf.h
+++ b/fs/xfs/xfs_buf.h
@@ -183,7 +183,28 @@ typedef struct xfs_buf {
 	unsigned int		b_page_count;	/* size of page array */
 	unsigned int		b_offset;	/* page offset in first page */
 	int			b_error;	/* error code on I/O */
-	int			b_last_error;	/* previous async I/O error */
+
+	/*
+	 * async write failure retry count. Initialised to zero on the first
+	 * failure, then when it exceeds the maximum configured without a
+	 * success the write is considered to be failed permanently and the
+	 * iodone handler will take appropriate action.
+	 *
+	 * For retry timeouts, we record the jiffie of the first failure. This
+	 * means that we can change the retry timeout and it on the next error
+	 * it will be checked against the newly configured timeout. This
+	 * prevents buffers getting stuck in retry loops with a really long
+	 * timeout.
+	 *
+	 * last_error is used to ensure that we are getting repeated errors, not
+	 * different errors. e.g. a block device might change ENOSPC to EIO when
+	 * a failure timeout occurs, so we want to re-initialise the error
+	 * retry behaviour appropriately when that happens.
+	 */
+	int			b_retries;
+	unsigned long		b_first_retry_time; /* in jiffies */
+	int			b_last_error;
+
 	const struct xfs_buf_ops	*b_ops;
 
 #ifdef XFS_BUF_LOCK_TRACKING
diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
index 3a30a88..68ed2a0 100644
--- a/fs/xfs/xfs_buf_item.c
+++ b/fs/xfs/xfs_buf_item.c
@@ -1086,6 +1086,9 @@ xfs_buf_iodone_callback_error(
 		bp->b_flags |= (XBF_WRITE | XBF_ASYNC |
 			        XBF_DONE | XBF_WRITE_FAIL);
 		bp->b_last_error = bp->b_error;
+		bp->b_retries = 0;
+		bp->b_first_retry_time = jiffies;
+
 		xfs_buf_ioerror(bp, 0);
 		xfs_buf_submit(bp);
 		return true;
@@ -1095,8 +1098,13 @@ xfs_buf_iodone_callback_error(
 	 * Repeated failure on an async write. Take action according to the
 	 * error configuration we have been set up to use.
 	 */
-	if (!cfg->max_retries)
-		goto permanent_error;
+	if (cfg->max_retries >= 0)
+		if (++bp->b_retries > cfg->max_retries)
+			goto permanent_error;
+	if (cfg->retry_timeout)
+		if (time_after(jiffies,
+			       cfg->retry_timeout + bp->b_first_retry_time))
+			goto permanent_error;
 
 	/* still a transient error, higher layers will retry */
 	xfs_buf_ioerror(bp, 0);
@@ -1139,6 +1147,7 @@ xfs_buf_iodone_callbacks(
 	 * retry state here in preparation for the next error that may occur.
 	 */
 	bp->b_last_error = 0;
+	bp->b_retries = 0;
 
 	xfs_buf_do_callbacks(bp);
 	bp->b_fspriv = NULL;
diff --git a/fs/xfs/xfs_mount.h b/fs/xfs/xfs_mount.h
index 0c5a976..0382140 100644
--- a/fs/xfs/xfs_mount.h
+++ b/fs/xfs/xfs_mount.h
@@ -54,7 +54,8 @@ enum {
 
 struct xfs_error_cfg {
 	struct xfs_kobj	kobj;
-	int		max_retries;
+	int		max_retries;	/* -1 = retry forever */
+	unsigned long	retry_timeout;	/* in jiffies, 0 = no timeout */
 };
 
 typedef struct xfs_mount {
diff --git a/fs/xfs/xfs_sysfs.c b/fs/xfs/xfs_sysfs.c
index e49a6de..fa291c4 100644
--- a/fs/xfs/xfs_sysfs.c
+++ b/fs/xfs/xfs_sysfs.c
@@ -374,10 +374,6 @@ struct kobj_type xfs_log_ktype = {
  * and any other future type of IO (e.g. special inode or directory error
  * handling) we care to support.
  */
-static struct attribute *xfs_error_attrs[] = {
-	NULL,
-};
-
 static inline struct xfs_error_cfg *
 to_error_cfg(struct kobject *kobject)
 {
@@ -385,6 +381,79 @@ to_error_cfg(struct kobject *kobject)
 	return container_of(kobj, struct xfs_error_cfg, kobj);
 }
 
+static ssize_t
+max_retries_show(
+	struct kobject	*kobject,
+	char		*buf)
+{
+	struct xfs_error_cfg *cfg = to_error_cfg(kobject);
+
+	return snprintf(buf, PAGE_SIZE, "%d\n", cfg->max_retries);
+}
+
+static ssize_t
+max_retries_store(
+	struct kobject	*kobject,
+	const char	*buf,
+	size_t		count)
+{
+	struct xfs_error_cfg *cfg = to_error_cfg(kobject);
+	int		ret;
+	int		val;
+
+	ret = kstrtoint(buf, 0, &val);
+	if (ret)
+		return ret;
+
+	if (val < -1 || val > INT_MAX)
+		return -EINVAL;
+
+	cfg->max_retries = val;
+	return count;
+}
+XFS_SYSFS_ATTR_RW(max_retries);
+
+static ssize_t
+retry_timeout_seconds_show(
+	struct kobject	*kobject,
+	char		*buf)
+{
+	struct xfs_error_cfg *cfg = to_error_cfg(kobject);
+
+	return snprintf(buf, PAGE_SIZE, "%ld\n",
+			jiffies_to_msecs(cfg->retry_timeout) / MSEC_PER_SEC);
+}
+
+static ssize_t
+retry_timeout_seconds_store(
+	struct kobject	*kobject,
+	const char	*buf,
+	size_t		count)
+{
+	struct xfs_error_cfg *cfg = to_error_cfg(kobject);
+	int		ret;
+	int		val;
+
+	ret = kstrtoint(buf, 0, &val);
+	if (ret)
+		return ret;
+
+	/* 1 day timeout maximum */
+	if (val < 0 || val > 86400)
+		return -EINVAL;
+
+	cfg->retry_timeout = msecs_to_jiffies(val * MSEC_PER_SEC);
+	return count;
+}
+XFS_SYSFS_ATTR_RW(retry_timeout_seconds);
+
+static struct attribute *xfs_error_attrs[] = {
+	ATTR_LIST(max_retries),
+	ATTR_LIST(retry_timeout_seconds),
+	NULL,
+};
+
+
 struct kobj_type xfs_error_cfg_ktype = {
 	.release = xfs_sysfs_release,
 	.sysfs_ops = &xfs_sysfs_ops,
@@ -404,11 +473,13 @@ struct kobj_type xfs_error_ktype = {
 struct xfs_error_init {
 	char		*name;
 	int		max_retries;
+	int		retry_timeout;	/* in seconds */
 };
 
 static const struct xfs_error_init xfs_error_meta_init[XFS_ERR_ERRNO_MAX] = {
 	{ .name = "Default",
 	  .max_retries = -1,
+	  .retry_timeout = 0,
 	},
 };
 
@@ -439,6 +510,8 @@ xfs_error_sysfs_init_class(
 			goto out_error;
 
 		cfg->max_retries = init[i].max_retries;
+		cfg->retry_timeout = msecs_to_jiffies(
+					init[i].retry_timeout * MSEC_PER_SEC);
 	}
 	return 0;
 
-- 
2.4.11

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