[PATCH RFC 0/3] xfs: add customizable default values for error configuration

[PATCH RFC 0/3] xfs: add customizable default values for error configuration

[Hou Tao]

Hi all,

XFS has the configurable error handlers for each mounted device, but
the default values of these configuration are static. It will be useful
to make the default values customizable when there are many XFS filesystems
and we need to shutdown the filesystem after getting any error.

The patches are simple. A sysfs tree is created under .../xfs/default_error
and its hierarchies are the same with the tree under .../fs/xfs/<dev>/error.

When the default value of any error configuration is being modified, the
corresponding value of all mount points will be checked again the old
default value. If they are the same, the value of the mount point will
be updated to the new default value as well, else the value of the mount
point will NOT be changed.

Thoughts, reviews, flames appreciated.

Tao

Hou Tao (3):
  xfs: prepare for the customizable default values of error
    configuration
  xfs: add sysfs files for default values of error configuration
  xfs: make the default values of error configuration customizable and
    workable

 fs/xfs/xfs_buf_item.c |   6 +-
 fs/xfs/xfs_mount.c    |   9 +-
 fs/xfs/xfs_mount.h    |  37 +++-
 fs/xfs/xfs_super.c    |  11 +-
 fs/xfs/xfs_sysfs.c    | 569 +++++++++++++++++++++++++++++++++++++++++---------
 fs/xfs/xfs_sysfs.h    |   9 +-
 6 files changed, 518 insertions(+), 123 deletions(-)

-- 
2.5.0

[PATCH RFC 1/3] xfs: prepare for the customizable default values of error configuration

[Hou Tao]

The refactoring includes the following changes:
* move error configuration related fields into a single struct
  xfs_mp_error_obj for convenience.
* move kobj out of xfs_error_cfg, so the sysfs store and show functions
  can be used on both mp-specific error obj and default error obj.
* split the initialization of error cfg and the creation of the sysfs
  files of error cfg. The initial values will come from the default
  error obj instead of a const struct array.
* add struct xfs_error_sysfs_arg to entail the differences between
  mp-specific error obj and default error obj during the initialization
  and the destroy of the sysfs tree

Signed-off-by: Hou Tao <houtao1@huawei.com>
---
 fs/xfs/xfs_buf_item.c |   6 +-
 fs/xfs/xfs_mount.c    |   8 +-
 fs/xfs/xfs_mount.h    |  20 ++--
 fs/xfs/xfs_sysfs.c    | 270 ++++++++++++++++++++++++++++++++++++--------------
 fs/xfs/xfs_sysfs.h    |   7 +-
 5 files changed, 224 insertions(+), 87 deletions(-)

diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
index 573fc72..fec1376 100644
--- a/fs/xfs/xfs_buf_item.c
+++ b/fs/xfs/xfs_buf_item.c
@@ -1088,7 +1088,7 @@ xfs_buf_iodone_callback_error(
 	struct xfs_mount	*mp = lip->li_mountp;
 	static ulong		lasttime;
 	static xfs_buftarg_t	*lasttarg;
-	struct xfs_error_cfg	*cfg;
+	const struct xfs_error_cfg	*cfg;
 
 	/*
 	 * If we've already decided to shutdown the filesystem because of
@@ -1111,7 +1111,7 @@ xfs_buf_iodone_callback_error(
 	trace_xfs_buf_item_iodone_async(bp, _RET_IP_);
 	ASSERT(bp->b_iodone != NULL);
 
-	cfg = xfs_error_get_cfg(mp, XFS_ERR_METADATA, bp->b_error);
+	cfg = xfs_error_get_cfg(&mp->m_eobj, XFS_ERR_METADATA, bp->b_error);
 
 	/*
 	 * If the write was asynchronous then no one will be looking for the
@@ -1146,7 +1146,7 @@ xfs_buf_iodone_callback_error(
 			goto permanent_error;
 
 	/* At unmount we may treat errors differently */
-	if ((mp->m_flags & XFS_MOUNT_UNMOUNTING) && mp->m_fail_unmount)
+	if ((mp->m_flags & XFS_MOUNT_UNMOUNTING) && mp->m_eobj.fail_unmount)
 		goto permanent_error;
 
 	/*
diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
index ea7d4b4..d67b5b6 100644
--- a/fs/xfs/xfs_mount.c
+++ b/fs/xfs/xfs_mount.c
@@ -704,7 +704,7 @@ xfs_mountfs(
 	xfs_set_maxicount(mp);
 
 	/* enable fail_at_unmount as default */
-	mp->m_fail_unmount = 1;
+	mp->m_eobj.fail_unmount = 1;
 
 	error = xfs_sysfs_init(&mp->m_kobj, &xfs_mp_ktype, NULL, mp->m_fsname);
 	if (error)
@@ -715,7 +715,7 @@ xfs_mountfs(
 	if (error)
 		goto out_remove_sysfs;
 
-	error = xfs_error_sysfs_init(mp);
+	error = xfs_error_sysfs_init(&mp->m_eobj, &mp->m_kobj);
 	if (error)
 		goto out_del_stats;
 
@@ -1042,7 +1042,7 @@ xfs_mountfs(
  out_remove_errortag:
 	xfs_errortag_del(mp);
  out_remove_error_sysfs:
-	xfs_error_sysfs_del(mp);
+	xfs_error_sysfs_del(&mp->m_eobj);
  out_del_stats:
 	xfs_sysfs_del(&mp->m_stats.xs_kobj);
  out_remove_sysfs:
@@ -1149,7 +1149,7 @@ xfs_unmountfs(
 	xfs_free_perag(mp);
 
 	xfs_errortag_del(mp);
-	xfs_error_sysfs_del(mp);
+	xfs_error_sysfs_del(&mp->m_eobj);
 	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 e0792d0..3e80aff 100644
--- a/fs/xfs/xfs_mount.h
+++ b/fs/xfs/xfs_mount.h
@@ -64,11 +64,22 @@ enum {
  * signed lets us store the special "-1" value, meaning retry forever.
  */
 struct xfs_error_cfg {
-	struct xfs_kobj	kobj;
 	int		max_retries;
 	long		retry_timeout;	/* in jiffies, -1 = infinite */
 };
 
+struct xfs_mp_error_cfg_kobj {
+	struct xfs_kobj kobj;
+	struct xfs_error_cfg cfg;
+};
+
+struct xfs_mp_error_obj {
+	struct xfs_kobj kobj;
+	struct xfs_kobj meta_kobj;
+	struct xfs_mp_error_cfg_kobj cfg_kobj[XFS_ERR_CLASS_MAX][XFS_ERR_ERRNO_MAX];
+	bool fail_unmount;
+};
+
 typedef struct xfs_mount {
 	struct super_block	*m_super;
 	xfs_tid_t		m_tid;		/* next unused tid for fs */
@@ -171,9 +182,7 @@ 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_kobj		m_error_meta_kobj;
-	struct xfs_error_cfg	m_error_cfg[XFS_ERR_CLASS_MAX][XFS_ERR_ERRNO_MAX];
+	struct xfs_mp_error_obj	m_eobj;
 	struct xstats		m_stats;	/* per-fs stats */
 
 	struct workqueue_struct *m_buf_workqueue;
@@ -196,7 +205,6 @@ typedef struct xfs_mount {
 	 */
 	uint32_t		m_generation;
 
-	bool			m_fail_unmount;
 #ifdef DEBUG
 	/*
 	 * Frequency with which errors are injected.  Replaces xfs_etest; the
@@ -443,7 +451,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,
+const struct xfs_error_cfg *xfs_error_get_cfg(struct xfs_mp_error_obj *eobj,
 		int error_class, int error);
 
 #endif	/* __XFS_MOUNT_H__ */
diff --git a/fs/xfs/xfs_sysfs.c b/fs/xfs/xfs_sysfs.c
index 8b2ccc2..7c15cba 100644
--- a/fs/xfs/xfs_sysfs.c
+++ b/fs/xfs/xfs_sysfs.c
@@ -27,6 +27,17 @@
 #include "xfs_stats.h"
 #include "xfs_mount.h"
 
+typedef struct xfs_kobj * (*xfs_get_error_cfg_kobj_t)(void *priv,
+		int class, int nr);
+struct xfs_error_sysfs_arg {
+	struct xfs_kobj *kobj;
+	struct xfs_kobj *meta_kobj;
+	struct attribute *fail_unmount_attr;
+	struct kobj_type *cfg_ktype;
+	xfs_get_error_cfg_kobj_t get_cfg_kobj;
+	void *priv;
+};
+
 struct xfs_sysfs_attr {
 	struct attribute attr;
 	ssize_t (*show)(struct kobject *kobject, char *buf);
@@ -329,27 +340,26 @@ 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 inline struct xfs_error_cfg *
-to_error_cfg(struct kobject *kobject)
+static inline struct xfs_mp_error_cfg_kobj *
+to_mp_error_cfg_kobj(struct kobject *kobject)
 {
 	struct xfs_kobj *kobj = to_kobj(kobject);
-	return container_of(kobj, struct xfs_error_cfg, kobj);
+	return container_of(kobj, struct xfs_mp_error_cfg_kobj, kobj);
 }
 
-static inline struct xfs_mount *
-err_to_mp(struct kobject *kobject)
+static inline struct xfs_mp_error_obj *
+to_mp_error_obj(struct kobject *kobject)
 {
 	struct xfs_kobj *kobj = to_kobj(kobject);
-	return container_of(kobj, struct xfs_mount, m_error_kobj);
+	return container_of(kobj, struct xfs_mp_error_obj, kobj);
 }
 
 static ssize_t
-max_retries_show(
-	struct kobject	*kobject,
+__max_retries_show(
+	struct xfs_error_cfg *cfg,
 	char		*buf)
 {
 	int		retries;
-	struct xfs_error_cfg *cfg = to_error_cfg(kobject);
 
 	if (cfg->max_retries == XFS_ERR_RETRY_FOREVER)
 		retries = -1;
@@ -360,12 +370,11 @@ max_retries_show(
 }
 
 static ssize_t
-max_retries_store(
-	struct kobject	*kobject,
+__max_retries_store(
+	struct xfs_error_cfg *cfg,
 	const char	*buf,
 	size_t		count)
 {
-	struct xfs_error_cfg *cfg = to_error_cfg(kobject);
 	int		ret;
 	int		val;
 
@@ -382,15 +391,35 @@ max_retries_store(
 		cfg->max_retries = val;
 	return count;
 }
-XFS_SYSFS_ATTR_RW(max_retries);
 
 static ssize_t
-retry_timeout_seconds_show(
+max_retries_show(
+	struct kobject	*kobject,
+	char		*buf)
+{
+	struct xfs_mp_error_cfg_kobj *cfg_kobj = to_mp_error_cfg_kobj(kobject);
+
+	return __max_retries_show(&cfg_kobj->cfg, buf);
+}
+
+static ssize_t
+max_retries_store(
 	struct kobject	*kobject,
+	const char	*buf,
+	size_t		count)
+{
+	struct xfs_mp_error_cfg_kobj *cfg_kobj = to_mp_error_cfg_kobj(kobject);
+
+	return __max_retries_store(&cfg_kobj->cfg, buf, count);
+}
+XFS_SYSFS_ATTR_RW(max_retries);
+
+static ssize_t
+__retry_timeout_seconds_show(
+	struct xfs_error_cfg *cfg,
 	char		*buf)
 {
 	int		timeout;
-	struct xfs_error_cfg *cfg = to_error_cfg(kobject);
 
 	if (cfg->retry_timeout == XFS_ERR_RETRY_FOREVER)
 		timeout = -1;
@@ -401,12 +430,11 @@ retry_timeout_seconds_show(
 }
 
 static ssize_t
-retry_timeout_seconds_store(
-	struct kobject	*kobject,
+__retry_timeout_seconds_store(
+	struct xfs_error_cfg *cfg,
 	const char	*buf,
 	size_t		count)
 {
-	struct xfs_error_cfg *cfg = to_error_cfg(kobject);
 	int		ret;
 	int		val;
 
@@ -426,25 +454,43 @@ retry_timeout_seconds_store(
 	}
 	return count;
 }
-XFS_SYSFS_ATTR_RW(retry_timeout_seconds);
 
 static ssize_t
-fail_at_unmount_show(
+retry_timeout_seconds_show(
 	struct kobject	*kobject,
 	char		*buf)
 {
-	struct xfs_mount	*mp = err_to_mp(kobject);
+	struct xfs_mp_error_cfg_kobj *cfg_kobj = to_mp_error_cfg_kobj(kobject);
 
-	return snprintf(buf, PAGE_SIZE, "%d\n", mp->m_fail_unmount);
+	return __retry_timeout_seconds_show(&cfg_kobj->cfg, buf);
 }
 
 static ssize_t
-fail_at_unmount_store(
+retry_timeout_seconds_store(
 	struct kobject	*kobject,
 	const char	*buf,
 	size_t		count)
 {
-	struct xfs_mount	*mp = err_to_mp(kobject);
+	struct xfs_mp_error_cfg_kobj *cfg_kobj = to_mp_error_cfg_kobj(kobject);
+
+	return __retry_timeout_seconds_store(&cfg_kobj->cfg, buf, count);
+}
+XFS_SYSFS_ATTR_RW(retry_timeout_seconds);
+
+static inline ssize_t
+__fail_at_unmount_show(
+	bool fail_unmount,
+	char		*buf)
+{
+	return snprintf(buf, PAGE_SIZE, "%d\n", fail_unmount);
+}
+
+static ssize_t
+__fail_at_unmount_store(
+	bool *fail_unmount,
+	const char	*buf,
+	size_t		count)
+{
 	int		ret;
 	int		val;
 
@@ -455,9 +501,30 @@ fail_at_unmount_store(
 	if (val < 0 || val > 1)
 		return -EINVAL;
 
-	mp->m_fail_unmount = val;
+	*fail_unmount = val;
 	return count;
 }
+
+static ssize_t
+fail_at_unmount_show(
+	struct kobject	*kobject,
+	char		*buf)
+{
+	struct xfs_mp_error_obj *eobj = to_mp_error_obj(kobject);
+
+	return __fail_at_unmount_show(eobj->fail_unmount, buf);
+}
+
+static ssize_t
+fail_at_unmount_store(
+	struct kobject	*kobject,
+	const char	*buf,
+	size_t		count)
+{
+	struct xfs_mp_error_obj *eobj = to_mp_error_obj(kobject);
+
+	return __fail_at_unmount_store(&eobj->fail_unmount, buf, count);
+}
 XFS_SYSFS_ATTR_RW(fail_at_unmount);
 
 static struct attribute *xfs_error_attrs[] = {
@@ -511,124 +578,183 @@ static const struct xfs_error_init xfs_error_meta_init[XFS_ERR_ERRNO_MAX] = {
 
 static int
 xfs_error_sysfs_init_class(
-	struct xfs_mount	*mp,
+	struct xfs_error_sysfs_arg *arg,
 	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);
+	error = xfs_sysfs_init(arg->meta_kobj, &xfs_error_ktype,
+				arg->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);
+		struct xfs_kobj *kobj = arg->get_cfg_kobj(arg->priv, class, i);
+
+		error = xfs_sysfs_init(kobj, arg->cfg_ktype,
+					arg->meta_kobj, init[i].name);
 		if (error)
 			goto out_error;
-
-		cfg->max_retries = init[i].max_retries;
-		if (init[i].retry_timeout == XFS_ERR_RETRY_FOREVER)
-			cfg->retry_timeout = XFS_ERR_RETRY_FOREVER;
-		else
-			cfg->retry_timeout = msecs_to_jiffies(
-					init[i].retry_timeout * MSEC_PER_SEC);
 	}
+
 	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(arg->get_cfg_kobj(arg->priv, class, i));
 	}
-	xfs_sysfs_del(parent_kobj);
+	xfs_sysfs_del(arg->meta_kobj);
 	return error;
 }
 
-int
-xfs_error_sysfs_init(
-	struct xfs_mount	*mp)
+static int
+__xfs_error_sysfs_init(
+	struct xfs_error_sysfs_arg *arg,
+	const char *name,
+	struct xfs_kobj *parent)
 {
 	int			error;
 
 	/* .../xfs/<dev>/error/ */
-	error = xfs_sysfs_init(&mp->m_error_kobj, &xfs_error_ktype,
-				&mp->m_kobj, "error");
+	error = xfs_sysfs_init(arg->kobj, &xfs_error_ktype, parent, name);
 	if (error)
 		return error;
 
-	error = sysfs_create_file(&mp->m_error_kobj.kobject,
-				  ATTR_LIST(fail_at_unmount));
-
+	error = sysfs_create_file(&arg->kobj->kobject, arg->fail_unmount_attr);
 	if (error)
 		goto out_error;
 
 	/* .../xfs/<dev>/error/metadata/ */
-	error = xfs_error_sysfs_init_class(mp, XFS_ERR_METADATA,
-				"metadata", &mp->m_error_meta_kobj,
-				xfs_error_meta_init);
+	error = xfs_error_sysfs_init_class(arg, XFS_ERR_METADATA,
+				"metadata", xfs_error_meta_init);
 	if (error)
 		goto out_error;
 
 	return 0;
 
 out_error:
-	xfs_sysfs_del(&mp->m_error_kobj);
+	xfs_sysfs_del(arg->kobj);
 	return error;
 }
 
-void
-xfs_error_sysfs_del(
-	struct xfs_mount	*mp)
+static void
+__xfs_error_sysfs_del(
+	struct xfs_error_sysfs_arg *arg)
 {
-	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(arg->get_cfg_kobj(arg->priv, i, j));
 		}
 	}
-	xfs_sysfs_del(&mp->m_error_meta_kobj);
-	xfs_sysfs_del(&mp->m_error_kobj);
+
+	xfs_sysfs_del(arg->meta_kobj);
+	xfs_sysfs_del(arg->kobj);
+}
+
+static struct xfs_kobj *
+xfs_get_mp_error_cfg_kobj(
+	void *priv,
+	int class,
+	int nr)
+{
+	struct xfs_mp_error_obj *eobj = priv;
+
+	return &eobj->cfg_kobj[class][nr].kobj;
+}
+
+static void
+xfs_error_mp_cfg_init(
+	struct xfs_mp_error_obj *eobj,
+	int class,
+	const struct xfs_error_init *init)
+{
+	int i;
+	struct xfs_error_cfg *cfg;
+
+	for (i = 0; i < XFS_ERR_ERRNO_MAX; i++) {
+		cfg = &eobj->cfg_kobj[class][i].cfg;
+
+		cfg->max_retries = init[i].max_retries;
+		if (init[i].retry_timeout == XFS_ERR_RETRY_FOREVER)
+			cfg->retry_timeout = XFS_ERR_RETRY_FOREVER;
+		else
+			cfg->retry_timeout = msecs_to_jiffies(
+					init[i].retry_timeout * MSEC_PER_SEC);
+	}
+}
+
+int
+xfs_error_sysfs_init(
+	struct xfs_mp_error_obj *eobj,
+	struct xfs_kobj *parent)
+{
+	int error;
+	struct xfs_error_sysfs_arg arg;
+
+	xfs_error_mp_cfg_init(eobj, XFS_ERR_METADATA, xfs_error_meta_init);
+
+	arg.kobj = &eobj->kobj;
+	arg.meta_kobj = &eobj->meta_kobj;
+	arg.fail_unmount_attr = ATTR_LIST(fail_at_unmount);
+	arg.cfg_ktype = &xfs_error_cfg_ktype;
+	arg.get_cfg_kobj = xfs_get_mp_error_cfg_kobj;
+	arg.priv = eobj;
+	error = __xfs_error_sysfs_init(&arg, "error", parent);
+	if (error)
+		return error;
+
+	return 0;
+}
+
+void
+xfs_error_sysfs_del(
+	struct xfs_mp_error_obj *eobj)
+{
+	struct xfs_error_sysfs_arg arg;
+
+	arg.kobj = &eobj->kobj;
+	arg.meta_kobj = &eobj->meta_kobj;
+	arg.fail_unmount_attr = NULL;
+	arg.cfg_ktype = NULL;
+	arg.get_cfg_kobj = xfs_get_mp_error_cfg_kobj;
+	arg.priv = eobj;
+
+	__xfs_error_sysfs_del(&arg);
 }
 
-struct xfs_error_cfg *
+const struct xfs_error_cfg *
 xfs_error_get_cfg(
-	struct xfs_mount	*mp,
+	struct xfs_mp_error_obj *eobj,
 	int			error_class,
 	int			error)
 {
-	struct xfs_error_cfg	*cfg;
+	int idx;
 
 	if (error < 0)
 		error = -error;
 
 	switch (error) {
 	case EIO:
-		cfg = &mp->m_error_cfg[error_class][XFS_ERR_EIO];
+		idx = XFS_ERR_EIO;
 		break;
 	case ENOSPC:
-		cfg = &mp->m_error_cfg[error_class][XFS_ERR_ENOSPC];
+		idx = XFS_ERR_ENOSPC;
 		break;
 	case ENODEV:
-		cfg = &mp->m_error_cfg[error_class][XFS_ERR_ENODEV];
+		idx = XFS_ERR_ENODEV;
 		break;
 	default:
-		cfg = &mp->m_error_cfg[error_class][XFS_ERR_DEFAULT];
+		idx = XFS_ERR_DEFAULT;
 		break;
 	}
 
-	return cfg;
+	return &eobj->cfg_kobj[error_class][idx].cfg;
 }
diff --git a/fs/xfs/xfs_sysfs.h b/fs/xfs/xfs_sysfs.h
index d046371..4c8b62c 100644
--- a/fs/xfs/xfs_sysfs.h
+++ b/fs/xfs/xfs_sysfs.h
@@ -24,6 +24,8 @@ extern struct kobj_type xfs_dbg_ktype;	/* debug */
 extern struct kobj_type xfs_log_ktype;	/* xlog */
 extern struct kobj_type xfs_stats_ktype;	/* stats */
 
+struct xfs_mp_error_obj;
+
 static inline struct xfs_kobj *
 to_kobj(struct kobject *kobject)
 {
@@ -58,7 +60,8 @@ 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);
+int	xfs_error_sysfs_init(struct xfs_mp_error_obj *eobj,
+		struct xfs_kobj *parent);
+void	xfs_error_sysfs_del(struct xfs_mp_error_obj *eobj);
 
 #endif	/* __XFS_SYSFS_H__ */
-- 
2.5.0

[PATCH RFC 2/3] xfs: add sysfs files for default values of error configuration

[Hou Tao]

Both the hierarchies and the names are the same with the sysfs tree
of the mount-point specific error configuration. The only difference
is the root of error configuration sysfs tree: for the sysfs tree of
default values it is "xfs/default_error" instead of "xfs/<dev_name>/error".

Signed-off-by: Hou Tao <houtao1@huawei.com>
---
 fs/xfs/xfs_mount.h |  14 +++++
 fs/xfs/xfs_super.c |  11 +++-
 fs/xfs/xfs_sysfs.c | 158 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
 fs/xfs/xfs_sysfs.h |   2 +
 4 files changed, 180 insertions(+), 5 deletions(-)

diff --git a/fs/xfs/xfs_mount.h b/fs/xfs/xfs_mount.h
index 3e80aff..a655821 100644
--- a/fs/xfs/xfs_mount.h
+++ b/fs/xfs/xfs_mount.h
@@ -73,6 +73,13 @@ struct xfs_mp_error_cfg_kobj {
 	struct xfs_error_cfg cfg;
 };
 
+struct xfs_dft_error_cfg_kobj {
+	struct xfs_kobj kobj;
+	struct xfs_error_cfg cfg;
+	int error_class;
+	int error_nr;
+};
+
 struct xfs_mp_error_obj {
 	struct xfs_kobj kobj;
 	struct xfs_kobj meta_kobj;
@@ -80,6 +87,13 @@ struct xfs_mp_error_obj {
 	bool fail_unmount;
 };
 
+struct xfs_dft_error_obj {
+	struct xfs_kobj kobj;
+	struct xfs_kobj meta_kobj;
+	struct xfs_dft_error_cfg_kobj cfg_kobj[XFS_ERR_CLASS_MAX][XFS_ERR_ERRNO_MAX];
+	bool fail_unmount;
+};
+
 typedef struct xfs_mount {
 	struct super_block	*m_super;
 	xfs_tid_t		m_tid;		/* next unused tid for fs */
diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index 38aaacd..3a3812b4 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -2043,11 +2043,15 @@ init_xfs_fs(void)
 	if (error)
 		goto out_free_stats;
 
+	error = xfs_dft_error_sysfs_init(xfs_kset);
+	if (error)
+		goto out_remove_stats_kobj;
+
 #ifdef DEBUG
 	xfs_dbg_kobj.kobject.kset = xfs_kset;
 	error = xfs_sysfs_init(&xfs_dbg_kobj, &xfs_dbg_ktype, NULL, "debug");
 	if (error)
-		goto out_remove_stats_kobj;
+		goto out_remove_dft_error_sysfs;
 #endif
 
 	error = xfs_qm_init();
@@ -2064,8 +2068,10 @@ init_xfs_fs(void)
  out_remove_dbg_kobj:
 #ifdef DEBUG
 	xfs_sysfs_del(&xfs_dbg_kobj);
- out_remove_stats_kobj:
+ out_remove_dft_error_sysfs:
 #endif
+	xfs_dft_error_sysfs_del();
+ out_remove_stats_kobj:
 	xfs_sysfs_del(&xfsstats.xs_kobj);
  out_free_stats:
 	free_percpu(xfsstats.xs_stats);
@@ -2095,6 +2101,7 @@ exit_xfs_fs(void)
 #ifdef DEBUG
 	xfs_sysfs_del(&xfs_dbg_kobj);
 #endif
+	xfs_dft_error_sysfs_del();
 	xfs_sysfs_del(&xfsstats.xs_kobj);
 	free_percpu(xfsstats.xs_stats);
 	kset_unregister(xfs_kset);
diff --git a/fs/xfs/xfs_sysfs.c b/fs/xfs/xfs_sysfs.c
index 7c15cba..9270cb1 100644
--- a/fs/xfs/xfs_sysfs.c
+++ b/fs/xfs/xfs_sysfs.c
@@ -45,6 +45,8 @@ struct xfs_sysfs_attr {
 			 size_t count);
 };
 
+static struct xfs_dft_error_obj xfs_dft_eobj;
+
 static inline struct xfs_sysfs_attr *
 to_attr(struct attribute *attr)
 {
@@ -57,8 +59,12 @@ to_attr(struct attribute *attr)
 	static struct xfs_sysfs_attr xfs_sysfs_attr_##name = __ATTR_RO(name)
 #define XFS_SYSFS_ATTR_WO(name) \
 	static struct xfs_sysfs_attr xfs_sysfs_attr_##name = __ATTR_WO(name)
+#define XFS_SYSFS_DFT_ATTR_RW(name) \
+	static struct xfs_sysfs_attr xfs_sysfs_attr_dft_##name = \
+		__ATTR(name, 0644, dft_##name##_show, dft_##name##_store)
 
-#define ATTR_LIST(name) &xfs_sysfs_attr_##name.attr
+#define ATTR_LIST(name) (&xfs_sysfs_attr_##name.attr)
+#define DFT_ATTR_LIST(name) (&xfs_sysfs_attr_dft_##name.attr)
 
 STATIC ssize_t
 xfs_sysfs_object_show(
@@ -344,16 +350,34 @@ static inline struct xfs_mp_error_cfg_kobj *
 to_mp_error_cfg_kobj(struct kobject *kobject)
 {
 	struct xfs_kobj *kobj = to_kobj(kobject);
+
 	return container_of(kobj, struct xfs_mp_error_cfg_kobj, kobj);
 }
 
+static inline struct xfs_dft_error_cfg_kobj *
+to_dft_error_cfg_kobj(struct kobject *kobject)
+{
+	struct xfs_kobj *kobj = to_kobj(kobject);
+
+	return container_of(kobj, struct xfs_dft_error_cfg_kobj, kobj);
+}
+
 static inline struct xfs_mp_error_obj *
 to_mp_error_obj(struct kobject *kobject)
 {
 	struct xfs_kobj *kobj = to_kobj(kobject);
+
 	return container_of(kobj, struct xfs_mp_error_obj, kobj);
 }
 
+static inline struct xfs_dft_error_obj *
+to_dft_error_obj(struct kobject *kobject)
+{
+	struct xfs_kobj *kobj = to_kobj(kobject);
+
+	return container_of(kobj, struct xfs_dft_error_obj, kobj);
+}
+
 static ssize_t
 __max_retries_show(
 	struct xfs_error_cfg *cfg,
@@ -415,6 +439,30 @@ max_retries_store(
 XFS_SYSFS_ATTR_RW(max_retries);
 
 static ssize_t
+dft_max_retries_show(
+	struct kobject	*kobject,
+	char		*buf)
+{
+	struct xfs_dft_error_cfg_kobj *cfg_kobj =
+	    to_dft_error_cfg_kobj(kobject);
+
+	return __max_retries_show(&cfg_kobj->cfg, buf);
+}
+
+static ssize_t
+dft_max_retries_store(
+	struct kobject	*kobject,
+	const char	*buf,
+	size_t		count)
+{
+	struct xfs_dft_error_cfg_kobj *cfg_kobj =
+	    to_dft_error_cfg_kobj(kobject);
+
+	return __max_retries_store(&cfg_kobj->cfg, buf, count);
+}
+XFS_SYSFS_DFT_ATTR_RW(max_retries);
+
+static ssize_t
 __retry_timeout_seconds_show(
 	struct xfs_error_cfg *cfg,
 	char		*buf)
@@ -477,6 +525,30 @@ retry_timeout_seconds_store(
 }
 XFS_SYSFS_ATTR_RW(retry_timeout_seconds);
 
+static ssize_t
+dft_retry_timeout_seconds_show(
+	struct kobject	*kobject,
+	char		*buf)
+{
+	struct xfs_dft_error_cfg_kobj *cfg_kobj =
+	    to_dft_error_cfg_kobj(kobject);
+
+	return __retry_timeout_seconds_show(&cfg_kobj->cfg, buf);
+}
+
+static ssize_t
+dft_retry_timeout_seconds_store(
+	struct kobject	*kobject,
+	const char	*buf,
+	size_t		count)
+{
+	struct xfs_dft_error_cfg_kobj *cfg_kobj =
+	    to_dft_error_cfg_kobj(kobject);
+
+	return __retry_timeout_seconds_store(&cfg_kobj->cfg, buf, count);
+}
+XFS_SYSFS_DFT_ATTR_RW(retry_timeout_seconds);
+
 static inline ssize_t
 __fail_at_unmount_show(
 	bool fail_unmount,
@@ -527,12 +599,39 @@ fail_at_unmount_store(
 }
 XFS_SYSFS_ATTR_RW(fail_at_unmount);
 
+static ssize_t
+dft_fail_at_unmount_show(
+	struct kobject	*kobject,
+	char		*buf)
+{
+	struct xfs_dft_error_obj *eobj = to_dft_error_obj(kobject);
+
+	return __fail_at_unmount_show(eobj->fail_unmount, buf);
+}
+
+static ssize_t
+dft_fail_at_unmount_store(
+	struct kobject	*kobject,
+	const char	*buf,
+	size_t		count)
+{
+	struct xfs_dft_error_obj *eobj = to_dft_error_obj(kobject);
+
+	return __fail_at_unmount_store(&eobj->fail_unmount, buf, count);
+}
+XFS_SYSFS_DFT_ATTR_RW(fail_at_unmount);
+
 static struct attribute *xfs_error_attrs[] = {
 	ATTR_LIST(max_retries),
 	ATTR_LIST(retry_timeout_seconds),
 	NULL,
 };
 
+static struct attribute *xfs_dft_error_attrs[] = {
+	DFT_ATTR_LIST(max_retries),
+	DFT_ATTR_LIST(retry_timeout_seconds),
+	NULL,
+};
 
 static struct kobj_type xfs_error_cfg_ktype = {
 	.release = xfs_sysfs_release,
@@ -540,6 +639,12 @@ static struct kobj_type xfs_error_cfg_ktype = {
 	.default_attrs = xfs_error_attrs,
 };
 
+static struct kobj_type xfs_dft_error_cfg_ktype = {
+	.release = xfs_sysfs_release,
+	.sysfs_ops = &xfs_sysfs_ops,
+	.default_attrs = xfs_dft_error_attrs,
+};
+
 static struct kobj_type xfs_error_ktype = {
 	.release = xfs_sysfs_release,
 	.sysfs_ops = &xfs_sysfs_ops,
@@ -621,7 +726,7 @@ __xfs_error_sysfs_init(
 {
 	int			error;
 
-	/* .../xfs/<dev>/error/ */
+	/* .../xfs/default_error/ or .../xfs/<dev>/error/ */
 	error = xfs_sysfs_init(arg->kobj, &xfs_error_ktype, parent, name);
 	if (error)
 		return error;
@@ -630,7 +735,7 @@ __xfs_error_sysfs_init(
 	if (error)
 		goto out_error;
 
-	/* .../xfs/<dev>/error/metadata/ */
+	/* .../xfs/error/metadata/ or .../xfs/<dev>/error/metadata/ */
 	error = xfs_error_sysfs_init_class(arg, XFS_ERR_METADATA,
 				"metadata", xfs_error_meta_init);
 	if (error)
@@ -758,3 +863,50 @@ xfs_error_get_cfg(
 
 	return &eobj->cfg_kobj[error_class][idx].cfg;
 }
+
+static struct xfs_kobj *
+xfs_get_dft_error_cfg_kobj(
+	void *priv,
+	int class,
+	int nr)
+{
+	struct xfs_dft_error_obj *eobj = priv;
+
+	return &eobj->cfg_kobj[class][nr].kobj;
+}
+
+int
+xfs_dft_error_sysfs_init(struct kset *kset)
+{
+	struct xfs_dft_error_obj *eobj = &xfs_dft_eobj;
+	struct xfs_error_sysfs_arg arg;
+
+	eobj->kobj.kobject.kset = kset;
+
+	eobj->fail_unmount = 1;
+
+	arg.kobj = &eobj->kobj;
+	arg.meta_kobj = &eobj->meta_kobj;
+	arg.fail_unmount_attr = DFT_ATTR_LIST(fail_at_unmount);
+	arg.cfg_ktype = &xfs_dft_error_cfg_ktype;
+	arg.get_cfg_kobj = xfs_get_dft_error_cfg_kobj;
+	arg.priv = eobj;
+
+	return __xfs_error_sysfs_init(&arg, "default_error", NULL);
+}
+
+void
+xfs_dft_error_sysfs_del(void)
+{
+	struct xfs_dft_error_obj *eobj = &xfs_dft_eobj;
+	struct xfs_error_sysfs_arg arg;
+
+	arg.kobj = &eobj->kobj;
+	arg.meta_kobj = &eobj->meta_kobj;
+	arg.fail_unmount_attr = NULL;
+	arg.cfg_ktype = NULL;
+	arg.get_cfg_kobj = xfs_get_dft_error_cfg_kobj;
+	arg.priv = eobj;
+
+	__xfs_error_sysfs_del(&arg);
+}
diff --git a/fs/xfs/xfs_sysfs.h b/fs/xfs/xfs_sysfs.h
index 4c8b62c..acfaa70 100644
--- a/fs/xfs/xfs_sysfs.h
+++ b/fs/xfs/xfs_sysfs.h
@@ -63,5 +63,7 @@ xfs_sysfs_del(
 int	xfs_error_sysfs_init(struct xfs_mp_error_obj *eobj,
 		struct xfs_kobj *parent);
 void	xfs_error_sysfs_del(struct xfs_mp_error_obj *eobj);
+int	xfs_dft_error_sysfs_init(struct kset *kset);
+void	xfs_dft_error_sysfs_del(void);
 
 #endif	/* __XFS_SYSFS_H__ */
-- 
2.5.0

[PATCH RFC 3/3] xfs: make the default values of error configuration customizable and workable

[Hou Tao]

Merge the error initialization table into the error configurations of the
default error object, and assign the error configurations to mount point
specific error object when initializing the sysfs tree of mount point.

When any default value is being modified and the value of mount point
specific configuration is same with the old default value, the value of
mount point specific configuration will be updated and synced with
the new default value.

Signed-off-by: Hou Tao <houtao1@huawei.com>
---
 fs/xfs/xfs_mount.c |   3 -
 fs/xfs/xfs_mount.h |   3 +
 fs/xfs/xfs_sysfs.c | 185 ++++++++++++++++++++++++++++++++++++++---------------
 3 files changed, 137 insertions(+), 54 deletions(-)

diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
index d67b5b6..3a6d2e0 100644
--- a/fs/xfs/xfs_mount.c
+++ b/fs/xfs/xfs_mount.c
@@ -703,9 +703,6 @@ xfs_mountfs(
 
 	xfs_set_maxicount(mp);
 
-	/* enable fail_at_unmount as default */
-	mp->m_eobj.fail_unmount = 1;
-
 	error = xfs_sysfs_init(&mp->m_kobj, &xfs_mp_ktype, NULL, mp->m_fsname);
 	if (error)
 		goto out;
diff --git a/fs/xfs/xfs_mount.h b/fs/xfs/xfs_mount.h
index a655821..bd22064 100644
--- a/fs/xfs/xfs_mount.h
+++ b/fs/xfs/xfs_mount.h
@@ -81,6 +81,7 @@ struct xfs_dft_error_cfg_kobj {
 };
 
 struct xfs_mp_error_obj {
+	struct list_head node;
 	struct xfs_kobj kobj;
 	struct xfs_kobj meta_kobj;
 	struct xfs_mp_error_cfg_kobj cfg_kobj[XFS_ERR_CLASS_MAX][XFS_ERR_ERRNO_MAX];
@@ -88,6 +89,8 @@ struct xfs_mp_error_obj {
 };
 
 struct xfs_dft_error_obj {
+	struct mutex lock;
+	struct list_head head;
 	struct xfs_kobj kobj;
 	struct xfs_kobj meta_kobj;
 	struct xfs_dft_error_cfg_kobj cfg_kobj[XFS_ERR_CLASS_MAX][XFS_ERR_ERRNO_MAX];
diff --git a/fs/xfs/xfs_sysfs.c b/fs/xfs/xfs_sysfs.c
index 9270cb1..51b6a48 100644
--- a/fs/xfs/xfs_sysfs.c
+++ b/fs/xfs/xfs_sysfs.c
@@ -45,6 +45,10 @@ struct xfs_sysfs_attr {
 			 size_t count);
 };
 
+static const char *xfs_error_meta_name[XFS_ERR_ERRNO_MAX] = {
+	"default", "EIO", "ENOSPC", "ENODEV",
+};
+
 static struct xfs_dft_error_obj xfs_dft_eobj;
 
 static inline struct xfs_sysfs_attr *
@@ -457,8 +461,35 @@ dft_max_retries_store(
 {
 	struct xfs_dft_error_cfg_kobj *cfg_kobj =
 	    to_dft_error_cfg_kobj(kobject);
+	struct xfs_error_cfg *cfg = &cfg_kobj->cfg;
+	int old_val;
+	int ret;
+	struct xfs_dft_error_obj *eobj;
+	struct xfs_mp_error_obj *mp_eobj;
+	int cls;
+	int nr;
+
+	old_val = cfg->max_retries;
+	ret = __max_retries_store(cfg, buf, count);
+	if (ret < 0)
+		return ret;
 
-	return __max_retries_store(&cfg_kobj->cfg, buf, count);
+	eobj = &xfs_dft_eobj;
+	cls = cfg_kobj->error_class;
+	nr = cfg_kobj->error_nr;
+	ASSERT(cls >= 0 && cls < XFS_ERR_CLASS_MAX);
+	ASSERT(nr >= 0 && nr < XFS_ERR_ERRNO_MAX);
+
+	mutex_lock(&eobj->lock);
+	list_for_each_entry(mp_eobj, &eobj->head, node) {
+		struct xfs_error_cfg *mp_cfg = &mp_eobj->cfg_kobj[cls][nr].cfg;
+
+		if (mp_cfg->max_retries == old_val)
+			mp_cfg->max_retries = cfg->max_retries;
+	}
+	mutex_unlock(&eobj->lock);
+
+	return ret;
 }
 XFS_SYSFS_DFT_ATTR_RW(max_retries);
 
@@ -544,8 +575,35 @@ dft_retry_timeout_seconds_store(
 {
 	struct xfs_dft_error_cfg_kobj *cfg_kobj =
 	    to_dft_error_cfg_kobj(kobject);
+	struct xfs_error_cfg *cfg = &cfg_kobj->cfg;
+	int old_val;
+	int ret;
+	struct xfs_dft_error_obj *eobj;
+	struct xfs_mp_error_obj *mp_eobj;
+	int cls;
+	int nr;
+
+	old_val = cfg->retry_timeout;
+	ret = __retry_timeout_seconds_store(cfg, buf, count);
+	if (ret < 0)
+		return ret;
 
-	return __retry_timeout_seconds_store(&cfg_kobj->cfg, buf, count);
+	eobj = &xfs_dft_eobj;
+	cls = cfg_kobj->error_class;
+	nr = cfg_kobj->error_nr;
+	ASSERT(cls >= 0 && cls < XFS_ERR_CLASS_MAX);
+	ASSERT(nr >= 0 && nr < XFS_ERR_ERRNO_MAX);
+
+	mutex_lock(&eobj->lock);
+	list_for_each_entry(mp_eobj, &eobj->head, node) {
+		struct xfs_error_cfg *mp_cfg = &mp_eobj->cfg_kobj[cls][nr].cfg;
+
+		if (mp_cfg->retry_timeout == old_val)
+			mp_cfg->retry_timeout = cfg->retry_timeout;
+	}
+	mutex_unlock(&eobj->lock);
+
+	return ret;
 }
 XFS_SYSFS_DFT_ATTR_RW(retry_timeout_seconds);
 
@@ -616,8 +674,23 @@ dft_fail_at_unmount_store(
 	size_t		count)
 {
 	struct xfs_dft_error_obj *eobj = to_dft_error_obj(kobject);
+	struct xfs_mp_error_obj *mp_eobj;
+	bool old_val;
+	int ret;
 
-	return __fail_at_unmount_store(&eobj->fail_unmount, buf, count);
+	old_val = eobj->fail_unmount;
+	ret = __fail_at_unmount_store(&eobj->fail_unmount, buf, count);
+	if (ret < 0)
+		return ret;
+
+	mutex_lock(&eobj->lock);
+	list_for_each_entry(mp_eobj, &eobj->head, node) {
+		if (mp_eobj->fail_unmount == old_val)
+			mp_eobj->fail_unmount = eobj->fail_unmount;
+	}
+	mutex_unlock(&eobj->lock);
+
+	return ret;
 }
 XFS_SYSFS_DFT_ATTR_RW(fail_at_unmount);
 
@@ -650,43 +723,12 @@ static struct kobj_type xfs_error_ktype = {
 	.sysfs_ops = &xfs_sysfs_ops,
 };
 
-/*
- * 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;
-	int		retry_timeout;	/* in seconds */
-};
-
-static const struct xfs_error_init xfs_error_meta_init[XFS_ERR_ERRNO_MAX] = {
-	{ .name = "default",
-	  .max_retries = XFS_ERR_RETRY_FOREVER,
-	  .retry_timeout = XFS_ERR_RETRY_FOREVER,
-	},
-	{ .name = "EIO",
-	  .max_retries = XFS_ERR_RETRY_FOREVER,
-	  .retry_timeout = XFS_ERR_RETRY_FOREVER,
-	},
-	{ .name = "ENOSPC",
-	  .max_retries = XFS_ERR_RETRY_FOREVER,
-	  .retry_timeout = XFS_ERR_RETRY_FOREVER,
-	},
-	{ .name = "ENODEV",
-	  .max_retries = 0,	/* We can't recover from devices disappearing */
-	  .retry_timeout = 0,
-	},
-};
-
 static int
 xfs_error_sysfs_init_class(
 	struct xfs_error_sysfs_arg *arg,
 	int			class,
 	const char		*parent_name,
-	const struct xfs_error_init init[])
+	const char		**names)
 {
 	int			error;
 	int			i;
@@ -702,7 +744,7 @@ xfs_error_sysfs_init_class(
 		struct xfs_kobj *kobj = arg->get_cfg_kobj(arg->priv, class, i);
 
 		error = xfs_sysfs_init(kobj, arg->cfg_ktype,
-					arg->meta_kobj, init[i].name);
+					arg->meta_kobj, names[i]);
 		if (error)
 			goto out_error;
 	}
@@ -737,7 +779,7 @@ __xfs_error_sysfs_init(
 
 	/* .../xfs/error/metadata/ or .../xfs/<dev>/error/metadata/ */
 	error = xfs_error_sysfs_init_class(arg, XFS_ERR_METADATA,
-				"metadata", xfs_error_meta_init);
+				"metadata", xfs_error_meta_name);
 	if (error)
 		goto out_error;
 
@@ -776,24 +818,36 @@ xfs_get_mp_error_cfg_kobj(
 }
 
 static void
-xfs_error_mp_cfg_init(
+xfs_error_dft_cfg_detach(
 	struct xfs_mp_error_obj *eobj,
-	int class,
-	const struct xfs_error_init *init)
+	struct xfs_dft_error_obj *dft_eobj)
+{
+	mutex_lock(&dft_eobj->lock);
+	list_del_init(&eobj->node);
+	mutex_unlock(&dft_eobj->lock);
+}
+
+static void
+xfs_error_dft_cfg_attach(
+	struct xfs_mp_error_obj *eobj,
+	struct xfs_dft_error_obj *dft_eobj)
 {
 	int i;
-	struct xfs_error_cfg *cfg;
 
+	/*
+	 * When the default error cfg is being modified, to ensure
+	 * the final default error cfg will be propagated to the
+	 * mount point specific error cfg, the lock needs to protect
+	 * the initialization of mount point specific error cfg as well.
+	 */
+	mutex_lock(&dft_eobj->lock);
+	eobj->fail_unmount = dft_eobj->fail_unmount;
 	for (i = 0; i < XFS_ERR_ERRNO_MAX; i++) {
-		cfg = &eobj->cfg_kobj[class][i].cfg;
-
-		cfg->max_retries = init[i].max_retries;
-		if (init[i].retry_timeout == XFS_ERR_RETRY_FOREVER)
-			cfg->retry_timeout = XFS_ERR_RETRY_FOREVER;
-		else
-			cfg->retry_timeout = msecs_to_jiffies(
-					init[i].retry_timeout * MSEC_PER_SEC);
+		eobj->cfg_kobj[XFS_ERR_METADATA][i].cfg =
+			dft_eobj->cfg_kobj[XFS_ERR_METADATA][i].cfg;
 	}
+	list_add_tail(&eobj->node, &dft_eobj->head);
+	mutex_unlock(&dft_eobj->lock);
 }
 
 int
@@ -804,7 +858,7 @@ xfs_error_sysfs_init(
 	int error;
 	struct xfs_error_sysfs_arg arg;
 
-	xfs_error_mp_cfg_init(eobj, XFS_ERR_METADATA, xfs_error_meta_init);
+	xfs_error_dft_cfg_attach(eobj, &xfs_dft_eobj);
 
 	arg.kobj = &eobj->kobj;
 	arg.meta_kobj = &eobj->meta_kobj;
@@ -813,8 +867,10 @@ xfs_error_sysfs_init(
 	arg.get_cfg_kobj = xfs_get_mp_error_cfg_kobj;
 	arg.priv = eobj;
 	error = __xfs_error_sysfs_init(&arg, "error", parent);
-	if (error)
+	if (error) {
+		xfs_error_dft_cfg_detach(eobj, &xfs_dft_eobj);
 		return error;
+	}
 
 	return 0;
 }
@@ -825,6 +881,8 @@ xfs_error_sysfs_del(
 {
 	struct xfs_error_sysfs_arg arg;
 
+	xfs_error_dft_cfg_detach(eobj, &xfs_dft_eobj);
+
 	arg.kobj = &eobj->kobj;
 	arg.meta_kobj = &eobj->meta_kobj;
 	arg.fail_unmount_attr = NULL;
@@ -875,15 +933,40 @@ xfs_get_dft_error_cfg_kobj(
 	return &eobj->cfg_kobj[class][nr].kobj;
 }
 
+static void
+xfs_dft_error_cfg_init(
+	int class,
+	struct xfs_dft_error_cfg_kobj *tbl)
+{
+	int i;
+
+	for (i = 0; i < XFS_ERR_ERRNO_MAX; i++) {
+		tbl[i].cfg.max_retries = XFS_ERR_RETRY_FOREVER;
+		tbl[i].cfg.retry_timeout = XFS_ERR_RETRY_FOREVER;
+		tbl[i].error_class = class;
+		tbl[i].error_nr = i;
+	}
+
+	/* We can't recover from devices disappearing */
+	tbl[XFS_ERR_ENODEV].cfg.max_retries = 0;
+	tbl[XFS_ERR_ENODEV].cfg.retry_timeout = 0;
+}
+
 int
 xfs_dft_error_sysfs_init(struct kset *kset)
 {
 	struct xfs_dft_error_obj *eobj = &xfs_dft_eobj;
 	struct xfs_error_sysfs_arg arg;
 
+	mutex_init(&eobj->lock);
+	INIT_LIST_HEAD(&eobj->head);
+
 	eobj->kobj.kobject.kset = kset;
 
+	/* enable fail_at_unmount as default */
 	eobj->fail_unmount = 1;
+	xfs_dft_error_cfg_init(XFS_ERR_METADATA,
+			eobj->cfg_kobj[XFS_ERR_METADATA]);
 
 	arg.kobj = &eobj->kobj;
 	arg.meta_kobj = &eobj->meta_kobj;
-- 
2.5.0

Re: [PATCH RFC 0/3] xfs: add customizable default values for error configuration

[Dave Chinner]

On Mon, Aug 21, 2017 at 07:54:19PM +0800, Hou Tao wrote:
> Hi all,
> 
> XFS has the configurable error handlers for each mounted device, but
> the default values of these configuration are static. It will be useful
> to make the default values customizable when there are many XFS filesystems
> and we need to shutdown the filesystem after getting any error.

Nice! I can see the usefulness of this functionality - it's a piece
of the puzzle we are missing. I've had a quick look over the code,
and have a few high level questions that I can't answer from looking
at the code.

Was there any reason you decided to put the default policy
management into the kernel rather than try to provide a mechanism to
allow userspace to manage it (e.g. via a udev event at mount time)?
We've still got to manage per filesystem specific settings from
userspace (e.g.  root volume might be different to data volumes), so
I'm interested to know if you have any ideas on how we can handle
that side of the problem, too?

> The patches are simple. A sysfs tree is created under .../xfs/default_error
> and its hierarchies are the same with the tree under .../fs/xfs/<dev>/error.
> 
> When the default value of any error configuration is being modified, the
> corresponding value of all mount points will be checked again the old
> default value. If they are the same, the value of the mount point will
> be updated to the new default value as well, else the value of the mount
> point will NOT be changed.

Assuming we are going to put default policy into the kernel, this
notifier structure seems overly complex when compared to a single
structure that holds the default values and looking it up when a
specific mount error config holds a flag that says "use the
default".

The "check the default flag and grab values" code could all be
hidden inside xfs_error_get_cfg() so propagation happens on demand.
This means all the mounts pick up changes to the default config
without needing to be chained to a notifier.

The "use defaults" flag could be cleared when a new value is set for
that error config, and potentionally we could reset it in the store
method if it matches the current default value.

I might be missing something here (I frequently do, so please tell
me if I have!), but my initial thoughts are that we can do this in a
much simpler manner. What do you think, Tao?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH RFC 0/3] xfs: add customizable default values for error configuration

[Eric Sandeen]

On 8/21/17 6:54 AM, Hou Tao wrote:
> Hi all,
> 
> XFS has the configurable error handlers for each mounted device, but
> the default values of these configuration are static. It will be useful
> to make the default values customizable when there are many XFS filesystems
> and we need to shutdown the filesystem after getting any error.
> 
> The patches are simple. A sysfs tree is created under .../xfs/default_error
> and its hierarchies are the same with the tree under .../fs/xfs/<dev>/error.
> 
> When the default value of any error configuration is being modified, the
> corresponding value of all mount points will be checked again the old
> default value. If they are the same, the value of the mount point will
> be updated to the new default value as well, else the value of the mount
> point will NOT be changed.
> 
> Thoughts, reviews, flames appreciated.

I recognize that it's an RFC, but if this goes in please also update
Documentation/filesystems/xfs.txt which documents the current error
handling tunables.

Thanks!

-Eric

Re: [PATCH RFC 0/3] xfs: add customizable default values for error configuration

[Darrick J. Wong]

On Tue, Aug 22, 2017 at 08:50:03AM +1000, Dave Chinner wrote:
> On Mon, Aug 21, 2017 at 07:54:19PM +0800, Hou Tao wrote:
> > Hi all,
> > 
> > XFS has the configurable error handlers for each mounted device, but
> > the default values of these configuration are static. It will be useful
> > to make the default values customizable when there are many XFS filesystems
> > and we need to shutdown the filesystem after getting any error.
> 
> Nice! I can see the usefulness of this functionality - it's a piece
> of the puzzle we are missing. I've had a quick look over the code,
> and have a few high level questions that I can't answer from looking
> at the code.
> 
> Was there any reason you decided to put the default policy
> management into the kernel rather than try to provide a mechanism to
> allow userspace to manage it (e.g. via a udev event at mount time)?
> We've still got to manage per filesystem specific settings from
> userspace (e.g.  root volume might be different to data volumes), so
> I'm interested to know if you have any ideas on how we can handle
> that side of the problem, too?
> 
> > The patches are simple. A sysfs tree is created under .../xfs/default_error
> > and its hierarchies are the same with the tree under .../fs/xfs/<dev>/error.
> > 
> > When the default value of any error configuration is being modified, the
> > corresponding value of all mount points will be checked again the old
> > default value. If they are the same, the value of the mount point will
> > be updated to the new default value as well, else the value of the mount
> > point will NOT be changed.
> 
> Assuming we are going to put default policy into the kernel, this
> notifier structure seems overly complex when compared to a single
> structure that holds the default values and looking it up when a
> specific mount error config holds a flag that says "use the
> default".
> 
> The "check the default flag and grab values" code could all be
> hidden inside xfs_error_get_cfg() so propagation happens on demand.
> This means all the mounts pick up changes to the default config
> without needing to be chained to a notifier.

Agreed.

> The "use defaults" flag could be cleared when a new value is set for
> that error config, and potentionally we could reset it in the store
> method if it matches the current default value.

I prefer letting userspace explicitly declare that it wants the default
value via:

echo default > /sys/fs/xfs/sda/error/metadata/EFUBAR/something

Say the the default of some attribute is currently 7 and the admin
deploys a script to set sda's value to 6 because they tested it and 6
was the value for them for sda.  Later they upgrade to a kernel where
the default is 6 -- they still want specifically 6, not "whatever the
default is".

(FWIW the sysfs errortag interface lets you set an explicit numeric
value for injection frequency or 'default' to take whatever we baked
into the code.)

--D

> 
> I might be missing something here (I frequently do, so please tell
> me if I have!), but my initial thoughts are that we can do this in a
> much simpler manner. What do you think, Tao?
> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RFC 0/3] xfs: add customizable default values for error configuration

[Hou Tao]

Hi Dave,

On 2017/8/22 6:50, Dave Chinner wrote:
> On Mon, Aug 21, 2017 at 07:54:19PM +0800, Hou Tao wrote:
>> Hi all,
>>
>> XFS has the configurable error handlers for each mounted device, but
>> the default values of these configuration are static. It will be useful
>> to make the default values customizable when there are many XFS filesystems
>> and we need to shutdown the filesystem after getting any error.
> 
> Nice! I can see the usefulness of this functionality - it's a piece
> of the puzzle we are missing. I've had a quick look over the code,
> and have a few high level questions that I can't answer from looking
> at the code.
> Was there any reason you decided to put the default policy
> management into the kernel rather than try to provide a mechanism to
> allow userspace to manage it (e.g. via a udev event at mount time)?
No special reason for the kernel-side default policy, just because I didn't
find an uevent for the mount event. If the uevent is available or added (?),
writing an udev rule to set the default error configuration seems simpler and
works for our scenario.

> We've still got to manage per filesystem specific settings from
> userspace (e.g.  root volume might be different to data volumes), so
> I'm interested to know if you have any ideas on how we can handle
> that side of the problem, too?
Do you mean providing extra tools or files to let userspace to manage
fs specific error settings or something else ? For us, the current sysfs
files of error configuration are enough and there is no need for anything more.

>> The patches are simple. A sysfs tree is created under .../xfs/default_error
>> and its hierarchies are the same with the tree under .../fs/xfs/<dev>/error.
>>
>> When the default value of any error configuration is being modified, the
>> corresponding value of all mount points will be checked again the old
>> default value. If they are the same, the value of the mount point will
>> be updated to the new default value as well, else the value of the mount
>> point will NOT be changed.
> 
> Assuming we are going to put default policy into the kernel, this
> notifier structure seems overly complex when compared to a single
> structure that holds the default values and looking it up when a
> specific mount error config holds a flag that says "use the
> default".
> The "check the default flag and grab values" code could all be
> hidden inside xfs_error_get_cfg() so propagation happens on demand.
> This means all the mounts pick up changes to the default config
> without needing to be chained to a notifier.
> 
> The "use defaults" flag could be cleared when a new value is set for
> that error config, and potentionally we could reset it in the store
> method if it matches the current default value.
> 
> I might be missing something here (I frequently do, so please tell
> me if I have!), but my initial thoughts are that we can do this in a
> much simpler manner. What do you think, Tao?
Yes, I agree that the notifier method is over-weighted, and the proposed method
is much better. However it has no way to reset the "use default flag" of a specific
configuration when the modified default value is modified again to a new value when
the old default value is equal with the current specific value. Maybe the reset is
unnecessary ?

Regards,

Tao

Re: [PATCH RFC 0/3] xfs: add customizable default values for error configuration

[Hou Tao]

Hi Darrick,

On 2017/8/23 0:59, Darrick J. Wong wrote:
> On Tue, Aug 22, 2017 at 08:50:03AM +1000, Dave Chinner wrote:
>> On Mon, Aug 21, 2017 at 07:54:19PM +0800, Hou Tao wrote:
>>> Hi all,
>>>
>>> XFS has the configurable error handlers for each mounted device, but
>>> the default values of these configuration are static. It will be useful
>>> to make the default values customizable when there are many XFS filesystems
>>> and we need to shutdown the filesystem after getting any error.
>>
>> Nice! I can see the usefulness of this functionality - it's a piece
>> of the puzzle we are missing. I've had a quick look over the code,
>> and have a few high level questions that I can't answer from looking
>> at the code.
>>
>> Was there any reason you decided to put the default policy
>> management into the kernel rather than try to provide a mechanism to
>> allow userspace to manage it (e.g. via a udev event at mount time)?
>> We've still got to manage per filesystem specific settings from
>> userspace (e.g.  root volume might be different to data volumes), so
>> I'm interested to know if you have any ideas on how we can handle
>> that side of the problem, too?
>>
>>> The patches are simple. A sysfs tree is created under .../xfs/default_error
>>> and its hierarchies are the same with the tree under .../fs/xfs/<dev>/error.
>>>
>>> When the default value of any error configuration is being modified, the
>>> corresponding value of all mount points will be checked again the old
>>> default value. If they are the same, the value of the mount point will
>>> be updated to the new default value as well, else the value of the mount
>>> point will NOT be changed.
>>
>> Assuming we are going to put default policy into the kernel, this
>> notifier structure seems overly complex when compared to a single
>> structure that holds the default values and looking it up when a
>> specific mount error config holds a flag that says "use the
>> default".
>>
>> The "check the default flag and grab values" code could all be
>> hidden inside xfs_error_get_cfg() so propagation happens on demand.
>> This means all the mounts pick up changes to the default config
>> without needing to be chained to a notifier.
> 
> Agreed.
> 
>> The "use defaults" flag could be cleared when a new value is set for
>> that error config, and potentionally we could reset it in the store
>> method if it matches the current default value.
> 
> I prefer letting userspace explicitly declare that it wants the default
> value via:
> 
> echo default > /sys/fs/xfs/sda/error/metadata/EFUBAR/something
> Say the the default of some attribute is currently 7 and the admin
> deploys a script to set sda's value to 6 because they tested it and 6
> was the value for them for sda.  Later they upgrade to a kernel where
> the default is 6 -- they still want specifically 6, not "whatever the
> default is".
> (FWIW the sysfs errortag interface lets you set an explicit numeric
> value for injection frequency or 'default' to take whatever we baked
> into the code.)
The explicit request of default value is a good idea, so even when the default
value and the specific value is the same and the default value is updated, there
is no need to update the specific value because it is NOT default.


> --D
> 
>>
>> I might be missing something here (I frequently do, so please tell
>> me if I have!), but my initial thoughts are that we can do this in a
>> much simpler manner. What do you think, Tao?
>>
>> Cheers,
>>
>> Dave.
>> -- 
>> Dave Chinner
>> david@fromorbit.com
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> .
> 

Re: [PATCH RFC 0/3] xfs: add customizable default values for error configuration

[Dave Chinner]

On Wed, Aug 23, 2017 at 05:26:36PM +0800, Hou Tao wrote:
> Hi Dave,
> 
> On 2017/8/22 6:50, Dave Chinner wrote:
> > On Mon, Aug 21, 2017 at 07:54:19PM +0800, Hou Tao wrote:
> >> Hi all,
> >>
> >> XFS has the configurable error handlers for each mounted device, but
> >> the default values of these configuration are static. It will be useful
> >> to make the default values customizable when there are many XFS filesystems
> >> and we need to shutdown the filesystem after getting any error.
> > 
> > Nice! I can see the usefulness of this functionality - it's a piece
> > of the puzzle we are missing. I've had a quick look over the code,
> > and have a few high level questions that I can't answer from looking
> > at the code.
> > Was there any reason you decided to put the default policy
> > management into the kernel rather than try to provide a mechanism to
> > allow userspace to manage it (e.g. via a udev event at mount time)?
> No special reason for the kernel-side default policy, just because I didn't
> find an uevent for the mount event. If the uevent is available or added (?),
> writing an udev rule to set the default error configuration seems simpler and
> works for our scenario.

I think we'd need to add one, but given we already have a kobj for
the filesystem being mounted, it seems to me like we should be able
to add a mount notification without too much trouble via
kobject_uevent(KOBJ_ONLINE)

> > We've still got to manage per filesystem specific settings from
> > userspace (e.g.  root volume might be different to data volumes), so
> > I'm interested to know if you have any ideas on how we can handle
> > that side of the problem, too?
> Do you mean providing extra tools or files to let userspace to manage
> fs specific error settings or something else ? For us, the current sysfs
> files of error configuration are enough and there is no need for anything more.

I was thinking we should provide a generic userspace tool to catch
the XFS uevents. On a mount notification it can grab the error
config from a config file (e.g.  /etc/xfs/error.conf) and push the
config specified in that file into the sysfs error config files of
the newly mounted filesystem....

> > Assuming we are going to put default policy into the kernel, this
> > notifier structure seems overly complex when compared to a single
> > structure that holds the default values and looking it up when a
> > specific mount error config holds a flag that says "use the
> > default".
> > The "check the default flag and grab values" code could all be
> > hidden inside xfs_error_get_cfg() so propagation happens on demand.
> > This means all the mounts pick up changes to the default config
> > without needing to be chained to a notifier.
> > 
> > The "use defaults" flag could be cleared when a new value is set for
> > that error config, and potentionally we could reset it in the store
> > method if it matches the current default value.
> > 
> > I might be missing something here (I frequently do, so please tell
> > me if I have!), but my initial thoughts are that we can do this in a
> > much simpler manner. What do you think, Tao?
>
> Yes, I agree that the notifier method is over-weighted, and the proposed method
> is much better. However it has no way to reset the "use default flag" of a specific
> configuration when the modified default value is modified again to a new value when
> the old default value is equal with the current specific value. Maybe the reset is
> unnecessary ?

In that case I think the reset is unnecessary. The fact the default
was changed to match a specific filesystem config doesn't mean that
filesystem should start using the defaults. i.e. if you want to
reset the filesystem to default config, then you need to rewrite
it's config to match the current defaults.

It's a bit clunky, but it's a simple rule that's easy to understand
and it keeps the API and both the kernel and userspace code really
simple.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH RFC 0/3] xfs: add customizable default values for error configuration

[Darrick J. Wong]

On Thu, Aug 24, 2017 at 10:16:37AM +1000, Dave Chinner wrote:
> On Wed, Aug 23, 2017 at 05:26:36PM +0800, Hou Tao wrote:
> > Hi Dave,
> > 
> > On 2017/8/22 6:50, Dave Chinner wrote:
> > > On Mon, Aug 21, 2017 at 07:54:19PM +0800, Hou Tao wrote:
> > >> Hi all,
> > >>
> > >> XFS has the configurable error handlers for each mounted device, but
> > >> the default values of these configuration are static. It will be useful
> > >> to make the default values customizable when there are many XFS filesystems
> > >> and we need to shutdown the filesystem after getting any error.
> > > 
> > > Nice! I can see the usefulness of this functionality - it's a piece
> > > of the puzzle we are missing. I've had a quick look over the code,
> > > and have a few high level questions that I can't answer from looking
> > > at the code.
> > > Was there any reason you decided to put the default policy
> > > management into the kernel rather than try to provide a mechanism to
> > > allow userspace to manage it (e.g. via a udev event at mount time)?
> > No special reason for the kernel-side default policy, just because I didn't
> > find an uevent for the mount event. If the uevent is available or added (?),
> > writing an udev rule to set the default error configuration seems simpler and
> > works for our scenario.
> 
> I think we'd need to add one, but given we already have a kobj for
> the filesystem being mounted, it seems to me like we should be able
> to add a mount notification without too much trouble via
> kobject_uevent(KOBJ_ONLINE)

<ENGAGE BIKESHED MODE>

It might be useful to think more broadly about modifying XFS to send
uevents.  In addition to sending KOBJ_ONLINE to broadcast the mount to
error configuration tools, we could (in the future say) schedule online
fscks with a service manager.  Or we could send KOBJ_CHANGE notices when
we hit IO errors, or someone remounts the fs with different options?

<END BIKESHED MODE>

> > > We've still got to manage per filesystem specific settings from
> > > userspace (e.g.  root volume might be different to data volumes), so
> > > I'm interested to know if you have any ideas on how we can handle
> > > that side of the problem, too?
> > Do you mean providing extra tools or files to let userspace to manage
> > fs specific error settings or something else ? For us, the current sysfs
> > files of error configuration are enough and there is no need for anything more.
> 
> I was thinking we should provide a generic userspace tool to catch
> the XFS uevents. On a mount notification it can grab the error
> config from a config file (e.g.  /etc/xfs/error.conf) and push the
> config specified in that file into the sysfs error config files of
> the newly mounted filesystem....

<nod>

> > > Assuming we are going to put default policy into the kernel, this
> > > notifier structure seems overly complex when compared to a single
> > > structure that holds the default values and looking it up when a
> > > specific mount error config holds a flag that says "use the
> > > default".
> > > The "check the default flag and grab values" code could all be
> > > hidden inside xfs_error_get_cfg() so propagation happens on demand.
> > > This means all the mounts pick up changes to the default config
> > > without needing to be chained to a notifier.
> > > 
> > > The "use defaults" flag could be cleared when a new value is set for
> > > that error config, and potentionally we could reset it in the store
> > > method if it matches the current default value.
> > > 
> > > I might be missing something here (I frequently do, so please tell
> > > me if I have!), but my initial thoughts are that we can do this in a
> > > much simpler manner. What do you think, Tao?
> >
> > Yes, I agree that the notifier method is over-weighted, and the proposed method
> > is much better. However it has no way to reset the "use default flag" of a specific
> > configuration when the modified default value is modified again to a new value when
> > the old default value is equal with the current specific value. Maybe the reset is
> > unnecessary ?
> 
> In that case I think the reset is unnecessary. The fact the default
> was changed to match a specific filesystem config doesn't mean that
> filesystem should start using the defaults. i.e. if you want to
> reset the filesystem to default config, then you need to rewrite
> it's config to match the current defaults.
> 
> It's a bit clunky, but it's a simple rule that's easy to understand
> and it keeps the API and both the kernel and userspace code really
> simple.

Or we just program the knobs to accept 'default', whatever the kernel
thinks that is?

--D

> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RFC 0/3] xfs: add customizable default values for error configuration

[Dave Chinner]

On Wed, Aug 23, 2017 at 05:46:09PM -0700, Darrick J. Wong wrote:
> On Thu, Aug 24, 2017 at 10:16:37AM +1000, Dave Chinner wrote:
> > On Wed, Aug 23, 2017 at 05:26:36PM +0800, Hou Tao wrote:
> > > Hi Dave,
> > > 
> > > On 2017/8/22 6:50, Dave Chinner wrote:
> > > > On Mon, Aug 21, 2017 at 07:54:19PM +0800, Hou Tao wrote:
> > > >> Hi all,
> > > >>
> > > >> XFS has the configurable error handlers for each mounted device, but
> > > >> the default values of these configuration are static. It will be useful
> > > >> to make the default values customizable when there are many XFS filesystems
> > > >> and we need to shutdown the filesystem after getting any error.
> > > > 
> > > > Nice! I can see the usefulness of this functionality - it's a piece
> > > > of the puzzle we are missing. I've had a quick look over the code,
> > > > and have a few high level questions that I can't answer from looking
> > > > at the code.
> > > > Was there any reason you decided to put the default policy
> > > > management into the kernel rather than try to provide a mechanism to
> > > > allow userspace to manage it (e.g. via a udev event at mount time)?
> > > No special reason for the kernel-side default policy, just because I didn't
> > > find an uevent for the mount event. If the uevent is available or added (?),
> > > writing an udev rule to set the default error configuration seems simpler and
> > > works for our scenario.
> > 
> > I think we'd need to add one, but given we already have a kobj for
> > the filesystem being mounted, it seems to me like we should be able
> > to add a mount notification without too much trouble via
> > kobject_uevent(KOBJ_ONLINE)
> 
> <ENGAGE BIKESHED MODE>
> 
> It might be useful to think more broadly about modifying XFS to send
> uevents.  In addition to sending KOBJ_ONLINE to broadcast the mount to
> error configuration tools, we could (in the future say) schedule online
> fscks with a service manager.  Or we could send KOBJ_CHANGE notices when
> we hit IO errors, or someone remounts the fs with different options?
> 
> <END BIKESHED MODE>

Agreed, that's what I'd like to be able to do in the future. Let's
make the small step first of being able to emit an online event,
because that has no other information passed with it that can trap
us into a corner in future. Other events and custom events are
outside the scope of error config injection at mount time....

> > > Yes, I agree that the notifier method is over-weighted, and the proposed method
> > > is much better. However it has no way to reset the "use default flag" of a specific
> > > configuration when the modified default value is modified again to a new value when
> > > the old default value is equal with the current specific value. Maybe the reset is
> > > unnecessary ?
> > 
> > In that case I think the reset is unnecessary. The fact the default
> > was changed to match a specific filesystem config doesn't mean that
> > filesystem should start using the defaults. i.e. if you want to
> > reset the filesystem to default config, then you need to rewrite
> > it's config to match the current defaults.
> > 
> > It's a bit clunky, but it's a simple rule that's easy to understand
> > and it keeps the API and both the kernel and userspace code really
> > simple.
> 
> Or we just program the knobs to accept 'default', whatever the kernel
> thinks that is?

Same concept, really - userspace needs to rewrite the config to tell
the kernel to return to using the defaults - it's just a slightly
different API mechanism to do so. Using the "default" keyword for
this is fine by me....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH RFC 0/3] xfs: add customizable default values for error configuration

[Hou Tao]

Hi Dave and Darrick,

On 2017/8/24 9:00, Dave Chinner wrote:
> On Wed, Aug 23, 2017 at 05:46:09PM -0700, Darrick J. Wong wrote:
>> On Thu, Aug 24, 2017 at 10:16:37AM +1000, Dave Chinner wrote:
>>> On Wed, Aug 23, 2017 at 05:26:36PM +0800, Hou Tao wrote:
>>>> Hi Dave,
>>>>
>>>> On 2017/8/22 6:50, Dave Chinner wrote:
>>>>> On Mon, Aug 21, 2017 at 07:54:19PM +0800, Hou Tao wrote:
>>>>>> Hi all,
>>>>>>
>>>>>> XFS has the configurable error handlers for each mounted device, but
>>>>>> the default values of these configuration are static. It will be useful
>>>>>> to make the default values customizable when there are many XFS filesystems
>>>>>> and we need to shutdown the filesystem after getting any error.
>>>>>
>>>>> Nice! I can see the usefulness of this functionality - it's a piece
>>>>> of the puzzle we are missing. I've had a quick look over the code,
>>>>> and have a few high level questions that I can't answer from looking
>>>>> at the code.
>>>>> Was there any reason you decided to put the default policy
>>>>> management into the kernel rather than try to provide a mechanism to
>>>>> allow userspace to manage it (e.g. via a udev event at mount time)?
>>>> No special reason for the kernel-side default policy, just because I didn't
>>>> find an uevent for the mount event. If the uevent is available or added (?),
>>>> writing an udev rule to set the default error configuration seems simpler and
>>>> works for our scenario.
>>>
>>> I think we'd need to add one, but given we already have a kobj for
>>> the filesystem being mounted, it seems to me like we should be able
>>> to add a mount notification without too much trouble via
>>> kobject_uevent(KOBJ_ONLINE)
>>
>> <ENGAGE BIKESHED MODE>
>>
>> It might be useful to think more broadly about modifying XFS to send
>> uevents.  In addition to sending KOBJ_ONLINE to broadcast the mount to
>> error configuration tools, we could (in the future say) schedule online
>> fscks with a service manager.  Or we could send KOBJ_CHANGE notices when
>> we hit IO errors, or someone remounts the fs with different options?
>>
>> <END BIKESHED MODE>
> 
> Agreed, that's what I'd like to be able to do in the future. Let's
> make the small step first of being able to emit an online event,
> because that has no other information passed with it that can trap
> us into a corner in future. Other events and custom events are
> outside the scope of error config injection at mount time....
So do we plan to add a mount and umount uevent only, or to add both
the uevent and a "default" value for the error configuration ? The former will be
enough for our scenario, thought it introduces a dependency on the udevd program.

Re: [PATCH RFC 0/3] xfs: add customizable default values for error configuration

[Dave Chinner]

On Thu, Aug 24, 2017 at 09:42:27AM +0800, Hou Tao wrote:
> Hi Dave and Darrick,
> 
> On 2017/8/24 9:00, Dave Chinner wrote:
> > On Wed, Aug 23, 2017 at 05:46:09PM -0700, Darrick J. Wong wrote:
> >> On Thu, Aug 24, 2017 at 10:16:37AM +1000, Dave Chinner wrote:
> >>> On Wed, Aug 23, 2017 at 05:26:36PM +0800, Hou Tao wrote:
> >>>> Hi Dave,
> >>>>
> >>>> On 2017/8/22 6:50, Dave Chinner wrote:
> >>>>> On Mon, Aug 21, 2017 at 07:54:19PM +0800, Hou Tao wrote:
> >>>>>> Hi all,
> >>>>>>
> >>>>>> XFS has the configurable error handlers for each mounted device, but
> >>>>>> the default values of these configuration are static. It will be useful
> >>>>>> to make the default values customizable when there are many XFS filesystems
> >>>>>> and we need to shutdown the filesystem after getting any error.
> >>>>>
> >>>>> Nice! I can see the usefulness of this functionality - it's a piece
> >>>>> of the puzzle we are missing. I've had a quick look over the code,
> >>>>> and have a few high level questions that I can't answer from looking
> >>>>> at the code.
> >>>>> Was there any reason you decided to put the default policy
> >>>>> management into the kernel rather than try to provide a mechanism to
> >>>>> allow userspace to manage it (e.g. via a udev event at mount time)?
> >>>> No special reason for the kernel-side default policy, just because I didn't
> >>>> find an uevent for the mount event. If the uevent is available or added (?),
> >>>> writing an udev rule to set the default error configuration seems simpler and
> >>>> works for our scenario.
> >>>
> >>> I think we'd need to add one, but given we already have a kobj for
> >>> the filesystem being mounted, it seems to me like we should be able
> >>> to add a mount notification without too much trouble via
> >>> kobject_uevent(KOBJ_ONLINE)
> >>
> >> <ENGAGE BIKESHED MODE>
> >>
> >> It might be useful to think more broadly about modifying XFS to send
> >> uevents.  In addition to sending KOBJ_ONLINE to broadcast the mount to
> >> error configuration tools, we could (in the future say) schedule online
> >> fscks with a service manager.  Or we could send KOBJ_CHANGE notices when
> >> we hit IO errors, or someone remounts the fs with different options?
> >>
> >> <END BIKESHED MODE>
> > 
> > Agreed, that's what I'd like to be able to do in the future. Let's
> > make the small step first of being able to emit an online event,
> > because that has no other information passed with it that can trap
> > us into a corner in future. Other events and custom events are
> > outside the scope of error config injection at mount time....
> So do we plan to add a mount and umount uevent only, or to add both
> the uevent and a "default" value for the error configuration?

Both, I think. They are separable pieces of work, though. The
uevents can be a single patch, the "default" value handling would be a
part of your series to introduce a global error default config.

> The former will be enough for our scenario, thought it introduces
> a dependency on the udevd program.

Pretty much everything a modern distro does w.r.t. block device
management requires udev to be present and functional, so I don't
see that there's going to be a problem with this.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com