[PATCH v2 0/4] null_blk cleanup and device naming improvements

[PATCH v2 0/4] null_blk cleanup and device naming improvements

[Damien Le Moal]

The first 3 patches of this series are simple code cleanups.

The last patch changes the device naming scheme for devices created
through configfs: the configfs device directory name is used as the disk
name. This name is also checked against potentially exiting nullb
devices created during modprobe when the nr_device module option is not
0.

This series falls short of pre-populating configfs with directories
corresponding to the devices created during modprobe as this is not
easily feasible. However, the added device name check avoids mistakes by
users about reusing configfs names already used by existing devices.

Changes from v1:
* Modified patch 3 to use the pr_fmt() macro instead of manually adding
  null_blk prefix to messages.
* Fixed patch 4 commit message

Damien Le Moal (4):
  block: null_blk: Fix code style issues
  block: null_blk: Cleanup device creation and deletion
  block: null_blk: Cleanup messages
  block: null_blk: Improve device creation with configfs

 drivers/block/null_blk/main.c  | 91 +++++++++++++++++++++++++---------
 drivers/block/null_blk/zoned.c |  7 ++-
 2 files changed, 73 insertions(+), 25 deletions(-)

-- 
2.35.1

[PATCH v2 1/4] block: null_blk: Fix code style issues

[Damien Le Moal]

Fix message grammar and code style issues (brackets and indentation) in
null_init().

Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
---
 drivers/block/null_blk/main.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c
index c441a4972064..1aa4897685f6 100644
--- a/drivers/block/null_blk/main.c
+++ b/drivers/block/null_blk/main.c
@@ -2113,19 +2113,21 @@ static int __init null_init(void)
 	}
 
 	if (g_queue_mode == NULL_Q_RQ) {
-		pr_err("legacy IO path no longer available\n");
+		pr_err("legacy IO path is no longer available\n");
 		return -EINVAL;
 	}
+
 	if (g_queue_mode == NULL_Q_MQ && g_use_per_node_hctx) {
 		if (g_submit_queues != nr_online_nodes) {
 			pr_warn("submit_queues param is set to %u.\n",
-							nr_online_nodes);
+				nr_online_nodes);
 			g_submit_queues = nr_online_nodes;
 		}
-	} else if (g_submit_queues > nr_cpu_ids)
+	} else if (g_submit_queues > nr_cpu_ids) {
 		g_submit_queues = nr_cpu_ids;
-	else if (g_submit_queues <= 0)
+	} else if (g_submit_queues <= 0) {
 		g_submit_queues = 1;
+	}
 
 	if (g_queue_mode == NULL_Q_MQ && shared_tags) {
 		ret = null_init_tag_set(NULL, &tag_set);
-- 
2.35.1

[PATCH v2 2/4] block: null_blk: Cleanup device creation and deletion

[Damien Le Moal]

Introduce the null_create_dev() and null_destroy_dev() helper functions
to respectivel create nullb devices on modprobe and destroy them on
rmmod. The null_destroy_dev() helper avoids duplicated code in the
null_init() and null_exit() functions for deleting devices.

Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
---
 drivers/block/null_blk/main.c | 48 ++++++++++++++++++++++-------------
 1 file changed, 30 insertions(+), 18 deletions(-)

diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c
index 1aa4897685f6..4d6bc94086da 100644
--- a/drivers/block/null_blk/main.c
+++ b/drivers/block/null_blk/main.c
@@ -2088,12 +2088,37 @@ static int null_add_dev(struct nullb_device *dev)
 	return rv;
 }
 
+static int null_create_dev(void)
+{
+	struct nullb_device *dev;
+	int ret;
+
+	dev = null_alloc_dev();
+	if (!dev)
+		return -ENOMEM;
+
+	ret = null_add_dev(dev);
+	if (ret) {
+		null_free_dev(dev);
+		return ret;
+	}
+
+	return 0;
+}
+
+static void null_destroy_dev(struct nullb *nullb)
+{
+	struct nullb_device *dev = nullb->dev;
+
+	null_del_dev(nullb);
+	null_free_dev(dev);
+}
+
 static int __init null_init(void)
 {
 	int ret = 0;
 	unsigned int i;
 	struct nullb *nullb;
-	struct nullb_device *dev;
 
 	if (g_bs > PAGE_SIZE) {
 		pr_warn("invalid block size\n");
@@ -2151,16 +2176,9 @@ static int __init null_init(void)
 	}
 
 	for (i = 0; i < nr_devices; i++) {
-		dev = null_alloc_dev();
-		if (!dev) {
-			ret = -ENOMEM;
-			goto err_dev;
-		}
-		ret = null_add_dev(dev);
-		if (ret) {
-			null_free_dev(dev);
+		ret = null_create_dev();
+		if (ret)
 			goto err_dev;
-		}
 	}
 
 	pr_info("module loaded\n");
@@ -2169,9 +2187,7 @@ static int __init null_init(void)
 err_dev:
 	while (!list_empty(&nullb_list)) {
 		nullb = list_entry(nullb_list.next, struct nullb, list);
-		dev = nullb->dev;
-		null_del_dev(nullb);
-		null_free_dev(dev);
+		null_destroy_dev(nullb);
 	}
 	unregister_blkdev(null_major, "nullb");
 err_conf:
@@ -2192,12 +2208,8 @@ static void __exit null_exit(void)
 
 	mutex_lock(&lock);
 	while (!list_empty(&nullb_list)) {
-		struct nullb_device *dev;
-
 		nullb = list_entry(nullb_list.next, struct nullb, list);
-		dev = nullb->dev;
-		null_del_dev(nullb);
-		null_free_dev(dev);
+		null_destroy_dev(nullb);
 	}
 	mutex_unlock(&lock);
 
-- 
2.35.1

[PATCH v2 3/4] block: null_blk: Cleanup messages

[Damien Le Moal]

Use the pr_fmt() macro to prefix all null_blk pr_xxx() messages with
"null_blk:" to clarify which module is printing the messages. Also add
a pr_info() message in null_add_dev() to print the name of a newly
created disk.

Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
---
 drivers/block/null_blk/main.c  | 5 +++++
 drivers/block/null_blk/zoned.c | 7 +++++--
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c
index 4d6bc94086da..7bc36d5114a9 100644
--- a/drivers/block/null_blk/main.c
+++ b/drivers/block/null_blk/main.c
@@ -11,6 +11,9 @@
 #include <linux/init.h>
 #include "null_blk.h"
 
+#undef pr_fmt
+#define pr_fmt(fmt)	"null_blk: " fmt
+
 #define FREE_BATCH		16
 
 #define TICKS_PER_SEC		50ULL
@@ -2071,6 +2074,8 @@ static int null_add_dev(struct nullb_device *dev)
 	list_add_tail(&nullb->list, &nullb_list);
 	mutex_unlock(&lock);
 
+	pr_info("disk %s created\n", nullb->disk_name);
+
 	return 0;
 out_cleanup_zone:
 	null_free_zoned_dev(dev);
diff --git a/drivers/block/null_blk/zoned.c b/drivers/block/null_blk/zoned.c
index dae54dd1aeac..ed158ea4fdd1 100644
--- a/drivers/block/null_blk/zoned.c
+++ b/drivers/block/null_blk/zoned.c
@@ -6,6 +6,9 @@
 #define CREATE_TRACE_POINTS
 #include "trace.h"
 
+#undef pr_fmt
+#define pr_fmt(fmt)	"null_blk: " fmt
+
 static inline sector_t mb_to_sects(unsigned long mb)
 {
 	return ((sector_t)mb * SZ_1M) >> SECTOR_SHIFT;
@@ -75,8 +78,8 @@ int null_init_zoned_dev(struct nullb_device *dev, struct request_queue *q)
 		dev->zone_capacity = dev->zone_size;
 
 	if (dev->zone_capacity > dev->zone_size) {
-		pr_err("null_blk: zone capacity (%lu MB) larger than zone size (%lu MB)\n",
-					dev->zone_capacity, dev->zone_size);
+		pr_err("zone capacity (%lu MB) larger than zone size (%lu MB)\n",
+		       dev->zone_capacity, dev->zone_size);
 		return -EINVAL;
 	}
 
-- 
2.35.1

[PATCH v2 4/4] block: null_blk: Improve device creation with configfs

[Damien Le Moal]

Currently, the directory name used to create a nullb device through
sysfs is not used as the device name, potentially causing headaches for
users if devices are already created through the modprobe operation
withe the nr_device module parameter not set to 0. E.g. a user can do
"mkdir /sys/kernel/config/nullb/nullb0" to create a nullb device even
though /dev/nullb0 was already created by modprobe. In this case, the
configfs nullb device will be named nullb1, causing confusion for the
user.

Simplify this by using the configfs directory name as the nullb device
name, always, unless another nullb device is already using the same
name. E.g. if modprobe created nullb0, then:

$ mkdir /sys/kernel/config/nullb/nullb0
mkdir: cannot create directory '/sys/kernel/config/nullb/nullb0': File
exists

will be reported to the user.

To implement this, the function null_find_dev_by_name() is added to
check for the existence of a nullb device with the name used for a new
configfs device directory. nullb_group_make_item() uses this new
function to check if the directory name can be used as the disk name.
Finally, null_add_dev() is modified to use the device config item name
as the disk name for a new nullb device created using configfs.
The naming of devices created though modprobe remains unchanged.

Of note is that it is possible for a user to create through configfs a
nullb device with the same name as an existing device. E.g.

$ mkdir /sys/kernel/config/nullb/null

will successfully create the nullb device named "null" but this block
device will however not appear under /dev/ since /dev/null already
exists.

Suggested-by: Joseph Bacik <josef@toxicpanda.com>
Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
---
 drivers/block/null_blk/main.c | 28 +++++++++++++++++++++++++++-
 1 file changed, 27 insertions(+), 1 deletion(-)

diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c
index 7bc36d5114a9..9ee72f484fc6 100644
--- a/drivers/block/null_blk/main.c
+++ b/drivers/block/null_blk/main.c
@@ -235,6 +235,7 @@ static struct nullb_device *null_alloc_dev(void);
 static void null_free_dev(struct nullb_device *dev);
 static void null_del_dev(struct nullb *nullb);
 static int null_add_dev(struct nullb_device *dev);
+static struct nullb *null_find_dev_by_name(const char *name);
 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)
@@ -563,6 +564,9 @@ config_item *nullb_group_make_item(struct config_group *group, const char *name)
 {
 	struct nullb_device *dev;
 
+	if (null_find_dev_by_name(name))
+		return ERR_PTR(-EEXIST);
+
 	dev = null_alloc_dev();
 	if (!dev)
 		return ERR_PTR(-ENOMEM);
@@ -2064,7 +2068,13 @@ static int null_add_dev(struct nullb_device *dev)
 
 	null_config_discard(nullb);
 
-	sprintf(nullb->disk_name, "nullb%d", nullb->index);
+	if (config_item_name(&dev->item)) {
+		/* Use configfs dir name as the device name */
+		snprintf(nullb->disk_name, sizeof(nullb->disk_name),
+			 "%s", config_item_name(&dev->item));
+	} else {
+		sprintf(nullb->disk_name, "nullb%d", nullb->index);
+	}
 
 	rv = null_gendisk_register(nullb);
 	if (rv)
@@ -2093,6 +2103,22 @@ static int null_add_dev(struct nullb_device *dev)
 	return rv;
 }
 
+static struct nullb *null_find_dev_by_name(const char *name)
+{
+	struct nullb *nullb = NULL, *nb;
+
+	mutex_lock(&lock);
+	list_for_each_entry(nb, &nullb_list, list) {
+		if (strcmp(nb->disk_name, name) == 0) {
+			nullb = nb;
+			break;
+		}
+	}
+	mutex_unlock(&lock);
+
+	return nullb;
+}
+
 static int null_create_dev(void)
 {
 	struct nullb_device *dev;
-- 
2.35.1

Re: [PATCH v2 1/4] block: null_blk: Fix code style issues

[Chaitanya Kulkarni]

On 4/19/22 17:57, Damien Le Moal wrote:
> Fix message grammar and code style issues (brackets and indentation) in
> null_init().
> 
> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
> ---

Looks good.

Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>

-ck

Re: [PATCH v2 2/4] block: null_blk: Cleanup device creation and deletion

[Chaitanya Kulkarni]

On 4/19/22 17:57, Damien Le Moal wrote:
> Introduce the null_create_dev() and null_destroy_dev() helper functions
> to respectivel create nullb devices on modprobe and destroy them on
> rmmod. The null_destroy_dev() helper avoids duplicated code in the
> null_init() and null_exit() functions for deleting devices.
> 
> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
> ---

nice cleanup...

Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>

-ck

Re: [PATCH v2 3/4] block: null_blk: Cleanup messages

[Chaitanya Kulkarni]

On 4/19/22 17:57, Damien Le Moal wrote:
> Use the pr_fmt() macro to prefix all null_blk pr_xxx() messages with
> "null_blk:" to clarify which module is printing the messages. Also add
> a pr_info() message in null_add_dev() to print the name of a newly
> created disk.
> 
> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
> ---

why not [1] to keep #define pr_fmt at one place in header file
instead of duplicating it in zoned.c main.c  ?

irrespective of that, looks good.

Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>

-ck


[1]

diff --git a/drivers/block/null_blk/null_blk.h 
b/drivers/block/null_blk/null_blk.h
index 78eb56b0ca55..450849fb3038 100644
--- a/drivers/block/null_blk/null_blk.h
+++ b/drivers/block/null_blk/null_blk.h
@@ -167,4 +167,8 @@ static inline size_t null_zone_valid_read_len(struct 
nullb *nullb,
  }
  #define null_report_zones      NULL
  #endif /* CONFIG_BLK_DEV_ZONED */
+
+#undef pr_fmt
+#define pr_fmt(fmt)    "null_blk: " fmt
+
  #endif /* __NULL_BLK_H */

Re: [PATCH v2 3/4] block: null_blk: Cleanup messages

[Damien Le Moal]

On 4/20/22 12:57, Chaitanya Kulkarni wrote:
> On 4/19/22 17:57, Damien Le Moal wrote:
>> Use the pr_fmt() macro to prefix all null_blk pr_xxx() messages with
>> "null_blk:" to clarify which module is printing the messages. Also add
>> a pr_info() message in null_add_dev() to print the name of a newly
>> created disk.
>>
>> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
>> ---
> 
> why not [1] to keep #define pr_fmt at one place in header file
> instead of duplicating it in zoned.c main.c  ?
> 
> irrespective of that, looks good.
> 
> Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>
> 
> -ck
> 
> 
> [1]
> 
> diff --git a/drivers/block/null_blk/null_blk.h 
> b/drivers/block/null_blk/null_blk.h
> index 78eb56b0ca55..450849fb3038 100644
> --- a/drivers/block/null_blk/null_blk.h
> +++ b/drivers/block/null_blk/null_blk.h
> @@ -167,4 +167,8 @@ static inline size_t null_zone_valid_read_len(struct 
> nullb *nullb,
>   }
>   #define null_report_zones      NULL
>   #endif /* CONFIG_BLK_DEV_ZONED */
> +
> +#undef pr_fmt
> +#define pr_fmt(fmt)    "null_blk: " fmt
> +
>   #endif /* __NULL_BLK_H */

Right, could do that. Jens ? Do you want an update for this ?

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2 1/4] block: null_blk: Fix code style issues

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

Re: [PATCH v2 2/4] block: null_blk: Cleanup device creation and deletion

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

Re: [PATCH v2 4/4] block: null_blk: Improve device creation with configfs

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

Re: [PATCH v2 0/4] null_blk cleanup and device naming improvements

[Jens Axboe]

On Wed, 20 Apr 2022 09:57:14 +0900, Damien Le Moal wrote:
> The first 3 patches of this series are simple code cleanups.
> 
> The last patch changes the device naming scheme for devices created
> through configfs: the configfs device directory name is used as the disk
> name. This name is also checked against potentially exiting nullb
> devices created during modprobe when the nr_device module option is not
> 0.
> 
> [...]

Applied, thanks!

[1/4] block: null_blk: Fix code style issues
      commit: 8ea7b68edc5fd7b257759fa8588f7f8b57fedd51
[2/4] block: null_blk: Cleanup device creation and deletion
      commit: 2fdd541e4c7288f09077842654adc925b81ec620
[3/4] block: null_blk: Cleanup messages
      commit: 12e995654f2ef62dad1205629a3cd3c2d7cd405b
[4/4] block: null_blk: Improve device creation with configfs
      commit: 9986ac16c508d613b371a902a0c53b22bac195ee

Best regards,
-- 
Jens Axboe