[PATCH 0/2] block: null_blk: make fault-injection configurable via configfs

[PATCH 0/2] block: null_blk: make fault-injection configurable via configfs

[Akinobu Mita]

This patch set makes null_blk driver-specific fault-injection dynamically
configurable per device via configfs.

Since the null_blk driver supports configuration such as device creation
via configfs, it is natural to configure fault-injection via configfs as
well.

Currently, only the debugfs interface is provided for dynamically
configuring fault-injection, so this introduces a compatible interface via
configfs.

Akinobu Mita (2):
  fault-inject: allow configuration via configfs
  block: null_blk: make fault-injection dynamically configurable per
    device

 .../fault-injection/fault-injection.rst       |   8 +
 drivers/block/null_blk/Kconfig                |   2 +-
 drivers/block/null_blk/main.c                 |  93 +++++++--
 drivers/block/null_blk/null_blk.h             |   7 +-
 include/linux/fault-inject.h                  |  22 ++
 lib/Kconfig.debug                             |  13 +-
 lib/fault-inject.c                            | 191 ++++++++++++++++++
 7 files changed, 312 insertions(+), 24 deletions(-)

-- 
2.34.1

[PATCH 1/2] fault-inject: allow configuration via configfs

[Akinobu Mita]

This provides a helper function to allow configuration of fault-injection
for configfs-based drivers.

The config items created by this function have the same interface as the
one created under debugfs by fault_create_debugfs_attr().

Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
---
 include/linux/fault-inject.h |  22 ++++
 lib/Kconfig.debug            |  13 ++-
 lib/fault-inject.c           | 191 +++++++++++++++++++++++++++++++++++
 3 files changed, 225 insertions(+), 1 deletion(-)

diff --git a/include/linux/fault-inject.h b/include/linux/fault-inject.h
index 444236dadcf0..481abf530b3c 100644
--- a/include/linux/fault-inject.h
+++ b/include/linux/fault-inject.h
@@ -6,6 +6,7 @@
 
 #include <linux/types.h>
 #include <linux/debugfs.h>
+#include <linux/configfs.h>
 #include <linux/ratelimit.h>
 #include <linux/atomic.h>
 
@@ -65,6 +66,27 @@ static inline struct dentry *fault_create_debugfs_attr(const char *name,
 
 #endif /* CONFIG_FAULT_INJECTION_DEBUG_FS */
 
+#ifdef CONFIG_FAULT_INJECTION_CONFIGFS
+
+struct fault_config {
+	struct fault_attr attr;
+	struct config_group group;
+};
+
+void fault_config_init(struct fault_config *config, const char *name);
+
+#else /* CONFIG_FAULT_INJECTION_CONFIGFS */
+
+struct fault_config {
+};
+
+static inline void fault_config_init(struct fault_config *config,
+			const char *name)
+{
+}
+
+#endif /* CONFIG_FAULT_INJECTION_CONFIGFS */
+
 #endif /* CONFIG_FAULT_INJECTION */
 
 struct kmem_cache;
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index f0d5b82e478d..6f64b49a2a8e 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -1977,9 +1977,20 @@ config FAIL_SUNRPC
 	  Provide fault-injection capability for SunRPC and
 	  its consumers.
 
+config FAULT_INJECTION_CONFIGFS
+	bool "Configfs interface for fault-injection capabilities"
+	depends on FAULT_INJECTION && CONFIGFS_FS
+	help
+	  This option allows configfs-based drivers to dynamically configure
+	  fault-injection via configfs.  Each parameter for driver-specific
+	  fault-injection can be made visible as a configfs attribute in a
+	  configfs group.
+
+
 config FAULT_INJECTION_STACKTRACE_FILTER
 	bool "stacktrace filter for fault-injection capabilities"
-	depends on FAULT_INJECTION_DEBUG_FS && STACKTRACE_SUPPORT
+	depends on FAULT_INJECTION
+	depends on (FAULT_INJECTION_DEBUG_FS || FAULT_INJECTION_CONFIGFS) && STACKTRACE_SUPPORT
 	select STACKTRACE
 	depends on FRAME_POINTER || MIPS || PPC || S390 || MICROBLAZE || ARM || ARC || X86
 	help
diff --git a/lib/fault-inject.c b/lib/fault-inject.c
index 6cff320c4eb4..d608f9b48c10 100644
--- a/lib/fault-inject.c
+++ b/lib/fault-inject.c
@@ -244,3 +244,194 @@ struct dentry *fault_create_debugfs_attr(const char *name,
 EXPORT_SYMBOL_GPL(fault_create_debugfs_attr);
 
 #endif /* CONFIG_FAULT_INJECTION_DEBUG_FS */
+
+#ifdef CONFIG_FAULT_INJECTION_CONFIGFS
+
+/* These configfs attribute utilities are copied from drivers/block/null_blk/main.c */
+
+static ssize_t fault_uint_attr_show(unsigned int val, char *page)
+{
+	return snprintf(page, PAGE_SIZE, "%u\n", val);
+}
+
+static ssize_t fault_ulong_attr_show(unsigned long val, char *page)
+{
+	return snprintf(page, PAGE_SIZE, "%lu\n", val);
+}
+
+static ssize_t fault_bool_attr_show(bool val, char *page)
+{
+	return snprintf(page, PAGE_SIZE, "%u\n", val);
+}
+
+static ssize_t fault_atomic_t_attr_show(atomic_t val, char *page)
+{
+	return snprintf(page, PAGE_SIZE, "%d\n", atomic_read(&val));
+}
+
+static ssize_t fault_uint_attr_store(unsigned int *val, const char *page, size_t count)
+{
+	unsigned int tmp;
+	int result;
+
+	result = kstrtouint(page, 0, &tmp);
+	if (result < 0)
+		return result;
+
+	*val = tmp;
+	return count;
+}
+
+static ssize_t fault_ulong_attr_store(unsigned long *val, const char *page, size_t count)
+{
+	int result;
+	unsigned long tmp;
+
+	result = kstrtoul(page, 0, &tmp);
+	if (result < 0)
+		return result;
+
+	*val = tmp;
+	return count;
+}
+
+static ssize_t fault_bool_attr_store(bool *val, const char *page, size_t count)
+{
+	bool tmp;
+	int result;
+
+	result = kstrtobool(page, &tmp);
+	if (result < 0)
+		return result;
+
+	*val = tmp;
+	return count;
+}
+
+static ssize_t fault_atomic_t_attr_store(atomic_t *val, const char *page, size_t count)
+{
+	int tmp;
+	int result;
+
+	result = kstrtoint(page, 0, &tmp);
+	if (result < 0)
+		return result;
+
+	atomic_set(val, tmp);
+	return count;
+}
+
+#define CONFIGFS_ATTR_NAMED(_pfx, _name, _attr_name)	\
+static struct configfs_attribute _pfx##attr_##_name = {	\
+	.ca_name	= _attr_name,			\
+	.ca_mode	= 0644,				\
+	.ca_owner	= THIS_MODULE,			\
+	.show		= _pfx##_name##_show,		\
+	.store		= _pfx##_name##_store,		\
+}
+
+static struct fault_config *to_fault_config(struct config_item *item)
+{
+	return container_of(to_config_group(item), struct fault_config, group);
+}
+
+#define FAULT_CONFIGFS_ATTR_NAMED(NAME, ATTR_NAME, MEMBER, TYPE)				\
+static ssize_t fault_##NAME##_show(struct config_item *item, char *page)			\
+{												\
+	return fault_##TYPE##_attr_show(to_fault_config(item)->attr.MEMBER, page);		\
+}												\
+static ssize_t fault_##NAME##_store(struct config_item *item, const char *page, size_t count)	\
+{												\
+	struct fault_config *config = to_fault_config(item);					\
+	return fault_##TYPE##_attr_store(&config->attr.MEMBER, page, count);			\
+}												\
+CONFIGFS_ATTR_NAMED(fault_, NAME, ATTR_NAME)
+
+#define FAULT_CONFIGFS_ATTR(NAME, TYPE)	\
+	FAULT_CONFIGFS_ATTR_NAMED(NAME, __stringify(NAME), NAME, TYPE)
+
+FAULT_CONFIGFS_ATTR(probability, ulong);
+FAULT_CONFIGFS_ATTR(interval, ulong);
+FAULT_CONFIGFS_ATTR(times, atomic_t);
+FAULT_CONFIGFS_ATTR(space, atomic_t);
+FAULT_CONFIGFS_ATTR(verbose, ulong);
+FAULT_CONFIGFS_ATTR_NAMED(ratelimit_interval, "verbose_ratelimit_interval_ms",
+		ratelimit_state.interval, uint);
+FAULT_CONFIGFS_ATTR_NAMED(ratelimit_burst, "verbose_ratelimit_burst",
+		ratelimit_state.burst, uint);
+FAULT_CONFIGFS_ATTR_NAMED(task_filter, "task-filter", task_filter, bool);
+
+#ifdef CONFIG_FAULT_INJECTION_STACKTRACE_FILTER
+
+static ssize_t fault_stacktrace_depth_show(struct config_item *item, char *page)
+{
+	return fault_ulong_attr_show(to_fault_config(item)->attr.stacktrace_depth, page);
+}
+
+static ssize_t fault_stacktrace_depth_store(struct config_item *item, const char *page,
+		size_t count)
+{
+	int result;
+	unsigned long tmp;
+
+	result = kstrtoul(page, 0, &tmp);
+	if (result < 0)
+		return result;
+
+	to_fault_config(item)->attr.stacktrace_depth =
+		min_t(unsigned long, tmp, MAX_STACK_TRACE_DEPTH);
+
+	return count;
+}
+
+CONFIGFS_ATTR_NAMED(fault_, stacktrace_depth, "stacktrace-depth");
+
+static ssize_t fault_xul_attr_show(unsigned long val, char *page)
+{
+	return snprintf(page, PAGE_SIZE,
+			sizeof(val) == sizeof(u32) ? "0x%08lx\n" : "0x%016lx\n", val);
+}
+
+static ssize_t fault_xul_attr_store(unsigned long *val, const char *page, size_t count)
+{
+	return fault_ulong_attr_store(val, page, count);
+}
+
+FAULT_CONFIGFS_ATTR_NAMED(require_start, "require-start", require_start, xul);
+FAULT_CONFIGFS_ATTR_NAMED(require_end, "require-end", require_end, xul);
+FAULT_CONFIGFS_ATTR_NAMED(reject_start, "reject-start", reject_start, xul);
+FAULT_CONFIGFS_ATTR_NAMED(reject_end, "reject-end", reject_end, xul);
+
+#endif /* CONFIG_FAULT_INJECTION_STACKTRACE_FILTER */
+
+static struct configfs_attribute *fault_config_attrs[] = {
+	&fault_attr_probability,
+	&fault_attr_interval,
+	&fault_attr_times,
+	&fault_attr_space,
+	&fault_attr_verbose,
+	&fault_attr_ratelimit_interval,
+	&fault_attr_ratelimit_burst,
+	&fault_attr_task_filter,
+#ifdef CONFIG_FAULT_INJECTION_STACKTRACE_FILTER
+	&fault_attr_stacktrace_depth,
+	&fault_attr_require_start,
+	&fault_attr_require_end,
+	&fault_attr_reject_start,
+	&fault_attr_reject_end,
+#endif /* CONFIG_FAULT_INJECTION_STACKTRACE_FILTER */
+	NULL,
+};
+
+static const struct config_item_type fault_config_type = {
+	.ct_attrs	= fault_config_attrs,
+	.ct_owner	= THIS_MODULE,
+};
+
+void fault_config_init(struct fault_config *config, const char *name)
+{
+	config_group_init_type_name(&config->group, name, &fault_config_type);
+}
+EXPORT_SYMBOL_GPL(fault_config_init);
+
+#endif /* CONFIG_FAULT_INJECTION_CONFIGFS */
-- 
2.34.1

Re: [PATCH 1/2] fault-inject: allow configuration via configfs

[Geert Uytterhoeven]

Hi Mita-san,

On Mon, Mar 27, 2023 at 4:48 PM Akinobu Mita <akinobu.mita@gmail.com> wrote:
> This provides a helper function to allow configuration of fault-injection
> for configfs-based drivers.
>
> The config items created by this function have the same interface as the
> one created under debugfs by fault_create_debugfs_attr().
>
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>

Thanks for your patch, which is now commit 4668c7a2940d134b
("fault-inject: allow configuration via configfs") in linux-next
(to be tagged soon as next-20140414).

> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -1977,9 +1977,20 @@ config FAIL_SUNRPC
>           Provide fault-injection capability for SunRPC and
>           its consumers.
>
> +config FAULT_INJECTION_CONFIGFS
> +       bool "Configfs interface for fault-injection capabilities"
> +       depends on FAULT_INJECTION && CONFIGFS_FS

noreply@ellerman.id.au reported build failures for e.g. m68k-allmodconfig
http://kisskb.ellerman.id.au/kisskb/buildresult/14911188/:

fault-inject.c:(.text+0x1ee): undefined reference to
`config_group_init_type_name'

This fails because FAULT_INJECTION=y but CONFIGFS_FS=m.

> +       help
> +         This option allows configfs-based drivers to dynamically configure
> +         fault-injection via configfs.  Each parameter for driver-specific
> +         fault-injection can be made visible as a configfs attribute in a
> +         configfs group.
> +
> +

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 1/2] fault-inject: allow configuration via configfs

[Akinobu Mita]

2023年4月15日(土) 23:53 Geert Uytterhoeven <geert@linux-m68k.org>:
>
> Hi Mita-san,
>
> On Mon, Mar 27, 2023 at 4:48 PM Akinobu Mita <akinobu.mita@gmail.com> wrote:
> > This provides a helper function to allow configuration of fault-injection
> > for configfs-based drivers.
> >
> > The config items created by this function have the same interface as the
> > one created under debugfs by fault_create_debugfs_attr().
> >
> > Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
>
> Thanks for your patch, which is now commit 4668c7a2940d134b
> ("fault-inject: allow configuration via configfs") in linux-next
> (to be tagged soon as next-20140414).
>
> > --- a/lib/Kconfig.debug
> > +++ b/lib/Kconfig.debug
> > @@ -1977,9 +1977,20 @@ config FAIL_SUNRPC
> >           Provide fault-injection capability for SunRPC and
> >           its consumers.
> >
> > +config FAULT_INJECTION_CONFIGFS
> > +       bool "Configfs interface for fault-injection capabilities"
> > +       depends on FAULT_INJECTION && CONFIGFS_FS
>
> noreply@ellerman.id.au reported build failures for e.g. m68k-allmodconfig
> http://kisskb.ellerman.id.au/kisskb/buildresult/14911188/:
>
> fault-inject.c:(.text+0x1ee): undefined reference to
> `config_group_init_type_name'
>
> This fails because FAULT_INJECTION=y but CONFIGFS_FS=m.

Oh, I just sent that build fix patch.
https://lore.kernel.org/all/20230415125705.180426-1-akinobu.mita@gmail.com/

Could you please check if this is the correct way to fix it?

[PATCH 2/2] block: null_blk: make fault-injection dynamically configurable per device

[Akinobu Mita]

The null_blk driver has multiple driver-specific fault injection
mechanisms.  Each fault injection configuration can only be specified by a
module parameter and cannot be reconfigured without reloading the driver.
Also, each configuration is common to all devices and is initialized every
time a new device is added.

This change adds the following subdirectories for each null_blk device.

/sys/kernel/config/nullb/<disk>/timeout_inject
/sys/kernel/config/nullb/<disk>/requeue_inject
/sys/kernel/config/nullb/<disk>/init_hctx_fault_inject

Each fault injection attribute can be dynamically set per device by a
corresponding file in these directories.

Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
---
 .../fault-injection/fault-injection.rst       |  8 ++
 drivers/block/null_blk/Kconfig                |  2 +-
 drivers/block/null_blk/main.c                 | 93 ++++++++++++++-----
 drivers/block/null_blk/null_blk.h             |  7 +-
 4 files changed, 87 insertions(+), 23 deletions(-)

diff --git a/Documentation/fault-injection/fault-injection.rst b/Documentation/fault-injection/fault-injection.rst
index 08e420e10973..b64809514b0f 100644
--- a/Documentation/fault-injection/fault-injection.rst
+++ b/Documentation/fault-injection/fault-injection.rst
@@ -52,6 +52,14 @@ Available fault injection capabilities
   status code is NVME_SC_INVALID_OPCODE with no retry. The status code and
   retry flag can be set via the debugfs.
 
+- Null test block driver fault injection
+
+  inject IO timeouts by setting config items under
+  /sys/kernel/config/nullb/<disk>/timeout_inject,
+  inject requeue requests by setting config items under
+  /sys/kernel/config/nullb/<disk>/requeue_inject, and
+  inject init_hctx() errors by setting config items under
+  /sys/kernel/config/nullb/<disk>/init_hctx_fault_inject.
 
 Configure fault-injection capabilities behavior
 -----------------------------------------------
diff --git a/drivers/block/null_blk/Kconfig b/drivers/block/null_blk/Kconfig
index 6bf1f8ca20a2..ff23bb9346d0 100644
--- a/drivers/block/null_blk/Kconfig
+++ b/drivers/block/null_blk/Kconfig
@@ -9,4 +9,4 @@ config BLK_DEV_NULL_BLK
 
 config BLK_DEV_NULL_BLK_FAULT_INJECTION
 	bool "Support fault injection for Null test block driver"
-	depends on BLK_DEV_NULL_BLK && FAULT_INJECTION
+	depends on BLK_DEV_NULL_BLK && FAULT_INJECTION_CONFIGFS
diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c
index 9e6b032c8ecc..fd3a9cf2ea92 100644
--- a/drivers/block/null_blk/main.c
+++ b/drivers/block/null_blk/main.c
@@ -250,7 +250,7 @@ static void null_free_device_storage(struct nullb_device *dev, bool is_cache);
 
 static inline struct nullb_device *to_nullb_device(struct config_item *item)
 {
-	return item ? container_of(item, struct nullb_device, item) : NULL;
+	return item ? container_of(to_config_group(item), struct nullb_device, group) : NULL;
 }
 
 static inline ssize_t nullb_device_uint_attr_show(unsigned int val, char *page)
@@ -593,8 +593,29 @@ static const struct config_item_type nullb_device_type = {
 	.ct_owner	= THIS_MODULE,
 };
 
+#ifdef CONFIG_BLK_DEV_NULL_BLK_FAULT_INJECTION
+
+static void nullb_add_fault_config(struct nullb_device *dev)
+{
+	fault_config_init(&dev->timeout_config, "timeout_inject");
+	fault_config_init(&dev->requeue_config, "requeue_inject");
+	fault_config_init(&dev->init_hctx_fault_config, "init_hctx_fault_inject");
+
+	configfs_add_default_group(&dev->timeout_config.group, &dev->group);
+	configfs_add_default_group(&dev->requeue_config.group, &dev->group);
+	configfs_add_default_group(&dev->init_hctx_fault_config.group, &dev->group);
+}
+
+#else
+
+static void nullb_add_fault_config(struct nullb_device *dev)
+{
+}
+
+#endif
+
 static struct
-config_item *nullb_group_make_item(struct config_group *group, const char *name)
+config_group *nullb_group_make_group(struct config_group *group, const char *name)
 {
 	struct nullb_device *dev;
 
@@ -605,9 +626,10 @@ config_item *nullb_group_make_item(struct config_group *group, const char *name)
 	if (!dev)
 		return ERR_PTR(-ENOMEM);
 
-	config_item_init_type_name(&dev->item, name, &nullb_device_type);
+	config_group_init_type_name(&dev->group, name, &nullb_device_type);
+	nullb_add_fault_config(dev);
 
-	return &dev->item;
+	return &dev->group;
 }
 
 static void
@@ -645,7 +667,7 @@ static struct configfs_attribute *nullb_group_attrs[] = {
 };
 
 static struct configfs_group_operations nullb_group_ops = {
-	.make_item	= nullb_group_make_item,
+	.make_group	= nullb_group_make_group,
 	.drop_item	= nullb_group_drop_item,
 };
 
@@ -676,6 +698,13 @@ static struct nullb_device *null_alloc_dev(void)
 	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
 	if (!dev)
 		return NULL;
+
+#ifdef CONFIG_BLK_DEV_NULL_BLK_FAULT_INJECTION
+	dev->timeout_config.attr = null_timeout_attr;
+	dev->requeue_config.attr = null_requeue_attr;
+	dev->init_hctx_fault_config.attr = null_init_hctx_attr;
+#endif
+
 	INIT_RADIX_TREE(&dev->data, GFP_ATOMIC);
 	INIT_RADIX_TREE(&dev->cache, GFP_ATOMIC);
 	if (badblocks_init(&dev->badblocks, 0)) {
@@ -1529,24 +1558,48 @@ static void null_submit_bio(struct bio *bio)
 	null_handle_cmd(alloc_cmd(nq, bio), sector, nr_sectors, bio_op(bio));
 }
 
+#ifdef CONFIG_BLK_DEV_NULL_BLK_FAULT_INJECTION
+
+static bool should_timeout_request(struct request *rq)
+{
+	struct nullb_cmd *cmd = blk_mq_rq_to_pdu(rq);
+	struct nullb_device *dev = cmd->nq->dev;
+
+	return should_fail(&dev->timeout_config.attr, 1);
+}
+
+static bool should_requeue_request(struct request *rq)
+{
+	struct nullb_cmd *cmd = blk_mq_rq_to_pdu(rq);
+	struct nullb_device *dev = cmd->nq->dev;
+
+	return should_fail(&dev->requeue_config.attr, 1);
+}
+
+static bool should_init_hctx_fail(struct nullb_device *dev)
+{
+	return should_fail(&dev->init_hctx_fault_config.attr, 1);
+}
+
+#else
+
 static bool should_timeout_request(struct request *rq)
 {
-#ifdef CONFIG_BLK_DEV_NULL_BLK_FAULT_INJECTION
-	if (g_timeout_str[0])
-		return should_fail(&null_timeout_attr, 1);
-#endif
 	return false;
 }
 
 static bool should_requeue_request(struct request *rq)
 {
-#ifdef CONFIG_BLK_DEV_NULL_BLK_FAULT_INJECTION
-	if (g_requeue_str[0])
-		return should_fail(&null_requeue_attr, 1);
-#endif
 	return false;
 }
 
+static bool should_init_hctx_fail(struct nullb_device *dev)
+{
+	return false;
+}
+
+#endif
+
 static void null_map_queues(struct blk_mq_tag_set *set)
 {
 	struct nullb *nullb = set->driver_data;
@@ -1743,10 +1796,8 @@ static int null_init_hctx(struct blk_mq_hw_ctx *hctx, void *driver_data,
 	struct nullb *nullb = hctx->queue->queuedata;
 	struct nullb_queue *nq;
 
-#ifdef CONFIG_BLK_DEV_NULL_BLK_FAULT_INJECTION
-	if (g_init_hctx_str[0] && should_fail(&null_init_hctx_attr, 1))
+	if (should_init_hctx_fail(nullb->dev))
 		return -EFAULT;
-#endif
 
 	nq = &nullb->queues[hctx_idx];
 	hctx->driver_data = nq;
@@ -2066,9 +2117,6 @@ static int null_add_dev(struct nullb_device *dev)
 		if (rv)
 			goto out_cleanup_queues;
 
-		if (!null_setup_fault())
-			goto out_cleanup_tags;
-
 		nullb->tag_set->timeout = 5 * HZ;
 		nullb->disk = blk_mq_alloc_disk(nullb->tag_set, nullb);
 		if (IS_ERR(nullb->disk)) {
@@ -2130,10 +2178,10 @@ static int null_add_dev(struct nullb_device *dev)
 
 	null_config_discard(nullb);
 
-	if (config_item_name(&dev->item)) {
+	if (config_item_name(&dev->group.cg_item)) {
 		/* Use configfs dir name as the device name */
 		snprintf(nullb->disk_name, sizeof(nullb->disk_name),
-			 "%s", config_item_name(&dev->item));
+			 "%s", config_item_name(&dev->group.cg_item));
 	} else {
 		sprintf(nullb->disk_name, "nullb%d", nullb->index);
 	}
@@ -2233,6 +2281,9 @@ static int __init null_init(void)
 		g_home_node = NUMA_NO_NODE;
 	}
 
+	if (!null_setup_fault())
+		return -EINVAL;
+
 	if (g_queue_mode == NULL_Q_RQ) {
 		pr_err("legacy IO path is no longer available\n");
 		return -EINVAL;
diff --git a/drivers/block/null_blk/null_blk.h b/drivers/block/null_blk/null_blk.h
index eb5972c50be8..929f659dd255 100644
--- a/drivers/block/null_blk/null_blk.h
+++ b/drivers/block/null_blk/null_blk.h
@@ -69,7 +69,12 @@ enum {
 
 struct nullb_device {
 	struct nullb *nullb;
-	struct config_item item;
+	struct config_group group;
+#ifdef CONFIG_BLK_DEV_NULL_BLK_FAULT_INJECTION
+	struct fault_config timeout_config;
+	struct fault_config requeue_config;
+	struct fault_config init_hctx_fault_config;
+#endif
 	struct radix_tree_root data; /* data stored in the disk */
 	struct radix_tree_root cache; /* disk cache data */
 	unsigned long flags; /* device flags */
-- 
2.34.1

Re: [PATCH 0/2] block: null_blk: make fault-injection configurable via configfs

[Christoph Hellwig]

On Mon, Mar 27, 2023 at 11:37:31PM +0900, Akinobu Mita wrote:
> This patch set makes null_blk driver-specific fault-injection dynamically
> configurable per device via configfs.
> 
> Since the null_blk driver supports configuration such as device creation
> via configfs, it is natural to configure fault-injection via configfs as
> well.
> 
> Currently, only the debugfs interface is provided for dynamically
> configuring fault-injection, so this introduces a compatible interface via
> configfs.

Oh, nice.  Can you also update blktests to take advantage of this and
not require built-in null_blk for fault injection tests?

Re: [PATCH 0/2] block: null_blk: make fault-injection configurable via configfs

[Chaitanya Kulkarni]

On 3/27/23 15:13, Christoph Hellwig wrote:
> On Mon, Mar 27, 2023 at 11:37:31PM +0900, Akinobu Mita wrote:
>> This patch set makes null_blk driver-specific fault-injection dynamically
>> configurable per device via configfs.
>>
>> Since the null_blk driver supports configuration such as device creation
>> via configfs, it is natural to configure fault-injection via configfs as
>> well.
>>
>> Currently, only the debugfs interface is provided for dynamically
>> configuring fault-injection, so this introduces a compatible interface via
>> configfs.
> Oh, nice.  Can you also update blktests to take advantage of this and
> not require built-in null_blk for fault injection tests?

+1 on blktests, please CC me when you do.

-ck

[PATCH blktests] don't require modular null_blk for fault-injection

[Akinobu Mita]

I'm trying to allow configuration of null_blk fault-injection via configfs.

This blktests change changes null_blk fault-injection settings to be
configured via configfs instead of module parameters.
This allows null_blk fault-injection tests to run even if the null_blk is
built-in the kernel and not built as a module.

However, to keep the scripts simple, we will skip testing if the null_blk
does not yet support configuring fault-injection via configfs.

Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
---
 tests/block/014 | 18 ++++++++++++------
 tests/block/015 | 18 ++++++++++++------
 tests/block/030 | 26 +++++++++++++++-----------
 3 files changed, 39 insertions(+), 23 deletions(-)

diff --git a/tests/block/014 b/tests/block/014
index facd4bc..65913e5 100755
--- a/tests/block/014
+++ b/tests/block/014
@@ -16,20 +16,26 @@ requires() {
 test() {
 	echo "Running ${TEST_NAME}"
 
-	# The format is "<interval>,<probability>,<space>,<times>". Here, we
-	# fail 50% of I/Os.
-	if ! _init_null_blk timeout='1,50,0,-1'; then
+	# Here, we fail 50% of I/Os.
+	if ! _configure_null_blk faultb0 timeout_inject/probability=50 \
+	     timeout_inject/times=-1 timeout_inject/verbose=0 power=1; then
+		if [[ -d /sys/kernel/config/nullb/faultb0/timeout_inject ]]; then
+			echo "Configuring null_blk failed"
+		else
+			SKIP_REASONS+=("requires fault injection via configfs")
+		fi
+		rmdir /sys/kernel/config/nullb/faultb0
 		return 1
 	fi
 
-	for sched in $(_io_schedulers nullb0); do
+	for sched in $(_io_schedulers faultb0); do
 		echo "Testing $sched" >> "$FULL"
-		echo "$sched" > /sys/block/nullb0/queue/scheduler
+		echo "$sched" > /sys/block/faultb0/queue/scheduler
 		# Do a bunch of I/Os which will timeout and then complete. The
 		# only thing we're really testing here is that this doesn't
 		# crash or hang.
 		for ((i = 0; i < 100; i++)); do
-			dd if=/dev/nullb0 of=/dev/null bs=4K count=4 \
+			dd if=/dev/faultb0 of=/dev/null bs=4K count=4 \
 				iflag=direct status=none > /dev/null 2>&1 &
 		done
 		wait
diff --git a/tests/block/015 b/tests/block/015
index 389c67f..5257d33 100755
--- a/tests/block/015
+++ b/tests/block/015
@@ -18,16 +18,22 @@ requires() {
 test() {
 	echo "Running ${TEST_NAME}"
 
-	# The format is "<interval>,<probability>,<space>,<times>". Here, we
-	# requeue 10% of the time.
-	if ! _init_null_blk requeue='1,10,0,-1'; then
+	# Here, we requeue 10% of the time.
+	if ! _configure_null_blk faultb0 requeue_inject/probability=10 \
+	     requeue_inject/times=-1 requeue_inject/verbose=0 power=1; then
+		if [[ -d /sys/kernel/config/nullb/faultb0/requeue_inject ]]; then
+			echo "Configuring null_blk failed"
+		else
+			SKIP_REASONS+=("requires fault injection via configfs")
+		fi
+		rmdir /sys/kernel/config/nullb/faultb0
 		return 1
 	fi
 
-	for sched in $(_io_schedulers nullb0); do
+	for sched in $(_io_schedulers faultb0); do
 		echo "Testing $sched" >> "$FULL"
-		echo "$sched" > /sys/block/nullb0/queue/scheduler
-		dd if=/dev/nullb0 of=/dev/null bs=4K count=$((512 * 1024)) \
+		echo "$sched" > /sys/block/faultb0/queue/scheduler
+		dd if=/dev/faultb0 of=/dev/null bs=4K count=$((512 * 1024)) \
 			iflag=direct status=none
 	done
 
diff --git a/tests/block/030 b/tests/block/030
index 7a0712b..f3cc337 100755
--- a/tests/block/030
+++ b/tests/block/030
@@ -17,19 +17,23 @@ requires() {
 }
 
 test() {
-	local i sq=/sys/kernel/config/nullb/nullb0/submit_queues
+	local i sq=/sys/kernel/config/nullb/faultb0/submit_queues
 
 	: "${TIMEOUT:=30}"
-	# Legend: init_hctx=<interval>,<probability>,<space>,<times>
 	# Set <space> to $(nproc) + 1 to make loading of null_blk succeed.
-	if ! _init_null_blk nr_devices=0 \
-	     "init_hctx=$(nproc),100,$(($(nproc) + 1)),-1"; then
-		echo "Loading null_blk failed"
-		return 1
-	fi
-	if ! _configure_null_blk nullb0 completion_nsec=0 blocksize=512 size=16\
-	     submit_queues="$(nproc)" memory_backed=1 power=1; then
-		echo "Configuring null_blk failed"
+	if ! _configure_null_blk faultb0 completion_nsec=0 blocksize=512 size=16\
+	     submit_queues="$(nproc)" memory_backed=1 \
+	     init_hctx_fault_inject/interval="$(nproc)" \
+	     init_hctx_fault_inject/probability=100 \
+	     init_hctx_fault_inject/space="$(($(nproc) + 1))" \
+	     init_hctx_fault_inject/times=-1 \
+	     init_hctx_fault_inject/verbose=0 power=1; then
+		if [[ -d /sys/kernel/config/nullb/faultb0/init_hctx_fault_inject ]]; then
+			echo "Configuring null_blk failed"
+		else
+			SKIP_REASONS+=("requires fault injection via configfs")
+		fi
+		rmdir /sys/kernel/config/nullb/faultb0
 		return 1
 	fi
 	# Since older null_blk versions do not allow "submit_queues" to be
@@ -47,7 +51,7 @@ test() {
 	else
 		SKIP_REASONS+=("Skipping test because $sq cannot be modified")
 	fi
-	rmdir /sys/kernel/config/nullb/nullb0
+	rmdir /sys/kernel/config/nullb/faultb0
 	_exit_null_blk
 	echo Passed
 }
-- 
2.34.1

Re: [PATCH blktests] don't require modular null_blk for fault-injection

[Shin'ichiro Kawasaki]

Akinobu, thanks for the patch, and sorry for this slow response.

On Mar 30, 2023 / 01:21, Akinobu Mita wrote:
> I'm trying to allow configuration of null_blk fault-injection via configfs.
> 
> This blktests change changes null_blk fault-injection settings to be
> configured via configfs instead of module parameters.
> This allows null_blk fault-injection tests to run even if the null_blk is
> built-in the kernel and not built as a module.

It's good that the three test case can run with built-in null_blk.

> 
> However, to keep the scripts simple, we will skip testing if the null_blk
> does not yet support configuring fault-injection via configfs.

Hmm, it means that blktests coverage will be lost on older kernels without the
configfs support. Before this change, the test cases were not skipped on older
kernel with loadable null_blk. After this patch, the test cases will be skipped.

Can we cover both the new and old ways? When the null_blk set up with configfs
fails, we can fallback to the old way with module parameters.

For example, null_blk set up of block/014 can be like this:

	# Here, we fail 50% of I/Os.
	if ! _configure_null_blk faultb0 timeout_inject/probability=50 \
	     timeout_inject/times=-1 timeout_inject/verbose=0 power=1 \
	     > /dev/null 2>&1; then
		rmdir /sys/kernel/config/nullb/faultb0
		if [[ -d /sys/kernel/config/nullb/faultb0/timeout_inject ]]; then
			echo "Configuring null_blk failed"
			return 1
		elif _module_file_exists null_blk; then
			# Fall back to set up with module parameter. The format
			# is "<interval>,<probability>,<space>,<times>"
			if ! _init_null_blk timeout='1,50,0,-1'; then
				echo "Configuring null_blk failed"
				return 1;
			fi
		else
			SKIP_REASONS+=("requires fault injection via configfs or modular null_blk")
			return 1
		fi
	fi

Re: [PATCH blktests] don't require modular null_blk for fault-injection

[Akinobu Mita]

2023年4月12日(水) 12:05 Shin'ichiro Kawasaki <shinichiro@fastmail.com>:
>
> Akinobu, thanks for the patch, and sorry for this slow response.
>
> On Mar 30, 2023 / 01:21, Akinobu Mita wrote:
> > I'm trying to allow configuration of null_blk fault-injection via configfs.
> >
> > This blktests change changes null_blk fault-injection settings to be
> > configured via configfs instead of module parameters.
> > This allows null_blk fault-injection tests to run even if the null_blk is
> > built-in the kernel and not built as a module.
>
> It's good that the three test case can run with built-in null_blk.
>
> >
> > However, to keep the scripts simple, we will skip testing if the null_blk
> > does not yet support configuring fault-injection via configfs.
>
> Hmm, it means that blktests coverage will be lost on older kernels without the
> configfs support. Before this change, the test cases were not skipped on older
> kernel with loadable null_blk. After this patch, the test cases will be skipped.
>
> Can we cover both the new and old ways? When the null_blk set up with configfs
> fails, we can fallback to the old way with module parameters.
>
> For example, null_blk set up of block/014 can be like this:

OK, I'll update the tests and give it a try.

Re: [PATCH 0/2] block: null_blk: make fault-injection configurable via configfs

[Jens Axboe]

On Mon, 27 Mar 2023 23:37:31 +0900, Akinobu Mita wrote:
> This patch set makes null_blk driver-specific fault-injection dynamically
> configurable per device via configfs.
> 
> Since the null_blk driver supports configuration such as device creation
> via configfs, it is natural to configure fault-injection via configfs as
> well.
> 
> [...]

Applied, thanks!

[1/2] fault-inject: allow configuration via configfs
      commit: 4668c7a2940d134bea50058e138591b97485c5da
[2/2] block: null_blk: make fault-injection dynamically configurable per device
      commit: bb4c19e030f45c5416f1eb4daa94fbaf7165e9ea

Best regards,
-- 
Jens Axboe