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

[PATCH 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.

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  | 113 +++++++++++++++++++++++----------
 drivers/block/null_blk/zoned.c |  14 ++--
 2 files changed, 85 insertions(+), 42 deletions(-)

-- 
2.35.1

[PATCH 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 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 3/4] block: null_blk: Cleanup messages

[Damien Le Moal]

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  | 29 ++++++++++++++++-------------
 drivers/block/null_blk/zoned.c | 14 +++++++-------
 2 files changed, 23 insertions(+), 20 deletions(-)

diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c
index 4d6bc94086da..96b6eb4ca60a 100644
--- a/drivers/block/null_blk/main.c
+++ b/drivers/block/null_blk/main.c
@@ -1521,7 +1521,7 @@ static int null_map_queues(struct blk_mq_tag_set *set)
 			submit_queues = dev->prev_submit_queues;
 			poll_queues = dev->prev_poll_queues;
 		} else {
-			pr_warn("tag set has unexpected nr_hw_queues: %d\n",
+			pr_warn("null_blk: tag set has unexpected nr_hw_queues: %d\n",
 				set->nr_hw_queues);
 			return -EINVAL;
 		}
@@ -1582,7 +1582,7 @@ static enum blk_eh_timer_return null_timeout_rq(struct request *rq, bool res)
 	struct blk_mq_hw_ctx *hctx = rq->mq_hctx;
 	struct nullb_cmd *cmd = blk_mq_rq_to_pdu(rq);
 
-	pr_info("rq %p timed out\n", rq);
+	pr_info("null_blk: rq %p timed out\n", rq);
 
 	if (hctx->type == HCTX_TYPE_POLL) {
 		struct nullb_queue *nq = hctx->driver_data;
@@ -1754,13 +1754,13 @@ static void null_config_discard(struct nullb *nullb)
 
 	if (!nullb->dev->memory_backed) {
 		nullb->dev->discard = false;
-		pr_info("discard option is ignored without memory backing\n");
+		pr_info("null_blk: discard option is ignored without memory backing\n");
 		return;
 	}
 
 	if (nullb->dev->zoned) {
 		nullb->dev->discard = false;
-		pr_info("discard option is ignored in zoned mode\n");
+		pr_info("null_blk: discard option is ignored in zoned mode\n");
 		return;
 	}
 
@@ -1932,7 +1932,7 @@ static int null_validate_conf(struct nullb_device *dev)
 
 	if (dev->zoned &&
 	    (!dev->zone_size || !is_power_of_2(dev->zone_size))) {
-		pr_err("zone_size must be power-of-two\n");
+		pr_err("null_blk: zone_size must be power-of-two\n");
 		return -EINVAL;
 	}
 
@@ -2071,6 +2071,8 @@ static int null_add_dev(struct nullb_device *dev)
 	list_add_tail(&nullb->list, &nullb_list);
 	mutex_unlock(&lock);
 
+	pr_info("null_blk: disk %s created\n", nullb->disk_name);
+
 	return 0;
 out_cleanup_zone:
 	null_free_zoned_dev(dev);
@@ -2121,30 +2123,31 @@ static int __init null_init(void)
 	struct nullb *nullb;
 
 	if (g_bs > PAGE_SIZE) {
-		pr_warn("invalid block size\n");
-		pr_warn("defaults block size to %lu\n", PAGE_SIZE);
+		pr_warn("null_blk: invalid block size\n");
+		pr_warn("null_blk: defaults block size to %lu\n", PAGE_SIZE);
 		g_bs = PAGE_SIZE;
 	}
 
 	if (g_max_sectors > BLK_DEF_MAX_SECTORS) {
-		pr_warn("invalid max sectors\n");
-		pr_warn("defaults max sectors to %u\n", BLK_DEF_MAX_SECTORS);
+		pr_warn("null_blk: invalid max sectors\n");
+		pr_warn("null_blk: defaults max sectors to %u\n",
+			BLK_DEF_MAX_SECTORS);
 		g_max_sectors = BLK_DEF_MAX_SECTORS;
 	}
 
 	if (g_home_node != NUMA_NO_NODE && g_home_node >= nr_online_nodes) {
-		pr_err("invalid home_node value\n");
+		pr_err("null_blk: invalid home_node value\n");
 		g_home_node = NUMA_NO_NODE;
 	}
 
 	if (g_queue_mode == NULL_Q_RQ) {
-		pr_err("legacy IO path is no longer available\n");
+		pr_err("null_blk: 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",
+			pr_warn("null_blk: submit_queues param is set to %u.\n",
 				nr_online_nodes);
 			g_submit_queues = nr_online_nodes;
 		}
@@ -2181,7 +2184,7 @@ static int __init null_init(void)
 			goto err_dev;
 	}
 
-	pr_info("module loaded\n");
+	pr_info("null_blk: module loaded\n");
 	return 0;
 
 err_dev:
diff --git a/drivers/block/null_blk/zoned.c b/drivers/block/null_blk/zoned.c
index dae54dd1aeac..f7114a16a342 100644
--- a/drivers/block/null_blk/zoned.c
+++ b/drivers/block/null_blk/zoned.c
@@ -63,11 +63,11 @@ int null_init_zoned_dev(struct nullb_device *dev, struct request_queue *q)
 	unsigned int i;
 
 	if (!is_power_of_2(dev->zone_size)) {
-		pr_err("zone_size must be power-of-two\n");
+		pr_err("null_blk: zone_size must be power-of-two\n");
 		return -EINVAL;
 	}
 	if (dev->zone_size > dev->size) {
-		pr_err("Zone size larger than device capacity\n");
+		pr_err("null_blk: Zone size larger than device capacity\n");
 		return -EINVAL;
 	}
 
@@ -76,7 +76,7 @@ int null_init_zoned_dev(struct nullb_device *dev, struct request_queue *q)
 
 	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);
+		       dev->zone_capacity, dev->zone_size);
 		return -EINVAL;
 	}
 
@@ -95,24 +95,24 @@ int null_init_zoned_dev(struct nullb_device *dev, struct request_queue *q)
 
 	if (dev->zone_nr_conv >= dev->nr_zones) {
 		dev->zone_nr_conv = dev->nr_zones - 1;
-		pr_info("changed the number of conventional zones to %u",
+		pr_info("null_blk: changed the number of conventional zones to %u",
 			dev->zone_nr_conv);
 	}
 
 	/* Max active zones has to be < nbr of seq zones in order to be enforceable */
 	if (dev->zone_max_active >= dev->nr_zones - dev->zone_nr_conv) {
 		dev->zone_max_active = 0;
-		pr_info("zone_max_active limit disabled, limit >= zone count\n");
+		pr_info("null_blk: zone_max_active limit disabled, limit >= zone count\n");
 	}
 
 	/* Max open zones has to be <= max active zones */
 	if (dev->zone_max_active && dev->zone_max_open > dev->zone_max_active) {
 		dev->zone_max_open = dev->zone_max_active;
-		pr_info("changed the maximum number of open zones to %u\n",
+		pr_info("null_blk: changed the maximum number of open zones to %u\n",
 			dev->nr_zones);
 	} else if (dev->zone_max_open >= dev->nr_zones - dev->zone_nr_conv) {
 		dev->zone_max_open = 0;
-		pr_info("zone_max_open limit disabled, limit >= zone count\n");
+		pr_info("null_blk: zone_max_open limit disabled, limit >= zone count\n");
 	}
 	dev->need_zone_res_mgmt = dev->zone_max_active || dev->zone_max_open;
 	dev->imp_close_zone_no = dev->zone_nr_conv;
-- 
2.35.1

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

[Johannes Thumshirn]

Shouldn't that be done using

#define pr_fmt(fmt) "null_blk: " fmt

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

[Damien Le Moal]

On 4/19/22 23:00, Johannes Thumshirn wrote:
> Shouldn't that be done using
> 
> #define pr_fmt(fmt) "null_blk: " fmt

Yep, thought of that after sending :)

-- 
Damien Le Moal
Western Digital Research

[PATCH 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 while
/dev/nullb0 wasalready created from 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 th 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 new nullb device, for devices 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 "null" but this 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 96b6eb4ca60a..49d89ae013de 100644
--- a/drivers/block/null_blk/main.c
+++ b/drivers/block/null_blk/main.c
@@ -232,6 +232,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)
@@ -560,6 +561,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);
@@ -2061,7 +2065,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)
@@ -2090,6 +2100,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 4/4] block: null_blk: Improve device creation with configfs

[Jens Axboe]

On 4/19/22 5:00 AM, Damien Le Moal wrote:
> 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 while
> /dev/nullb0 wasalready created from modprobe. In this case, the configfs
                ^^^

space

> 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 th 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 new nullb device, for devices 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.

This is nice, and solves both the confusing part of having
pre-configured devices, but also using the actual directory name as the
device name even if they are not ordered.

Only odd bit is you can create a device name where a special file of
that name already exists, but I don't think that's solvable in a clean
way and we just need to ignore that. That's arguably a user error, don't
pick names that already exist.

-- 
Jens Axboe

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

[Damien Le Moal]

On 4/19/22 20:55, Jens Axboe wrote:
> On 4/19/22 5:00 AM, Damien Le Moal wrote:
>> 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 while
>> /dev/nullb0 wasalready created from modprobe. In this case, the configfs
>                 ^^^
> 
> space

Re-sending to fix this. Also realized that using "#define pr_fmt" would
simplify patch 3. Updating that.

> 
>> 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 th 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 new nullb device, for devices 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.
> 
> This is nice, and solves both the confusing part of having
> pre-configured devices, but also using the actual directory name as the
> device name even if they are not ordered.
> 
> Only odd bit is you can create a device name where a special file of
> that name already exists, but I don't think that's solvable in a clean
> way and we just need to ignore that. That's arguably a user error, don't
> pick names that already exist.

Yes. add_disk() will fail if the device name already exist within the
"block" device class, but will not complain about anything if the existing
device belongs to another class.

I could add a "block" class wide check for device name existence in
null_find_dev_by_name() instead of only checking the nullb list ?


-- 
Damien Le Moal
Western Digital Research

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

[Damien Le Moal]

On 4/20/22 05:58, Damien Le Moal wrote:
> On 4/19/22 20:55, Jens Axboe wrote:
>> On 4/19/22 5:00 AM, Damien Le Moal wrote:
>>> 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 while
>>> /dev/nullb0 wasalready created from modprobe. In this case, the configfs
>>                 ^^^
>>
>> space
> 
> Re-sending to fix this. Also realized that using "#define pr_fmt" would
> simplify patch 3. Updating that.
> 
>>
>>> 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 th 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 new nullb device, for devices 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.
>>
>> This is nice, and solves both the confusing part of having
>> pre-configured devices, but also using the actual directory name as the
>> device name even if they are not ordered.
>>
>> Only odd bit is you can create a device name where a special file of
>> that name already exists, but I don't think that's solvable in a clean
>> way and we just need to ignore that. That's arguably a user error, don't
>> pick names that already exist.
> 
> Yes. add_disk() will fail if the device name already exist within the
> "block" device class, but will not complain about anything if the existing
> device belongs to another class.
> 
> I could add a "block" class wide check for device name existence in
> null_find_dev_by_name() instead of only checking the nullb list ?

Looked into this, but I do not see any easy ready-to-use way to do it
since "struct class block_class" is not exported. And I would not wnat to
export this block/dev internal symbol.

We could play with vfs_stat() to test for device existence, but that is a
little ugly...

What about simply enforcing a name pattern like "nullb*" for the configfs
directory/device names ? That is simple and the current
nullb_find_dev_by_name() will keep working as is.


-- 
Damien Le Moal
Western Digital Research

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

[Jens Axboe]

On 4/19/22 5:13 PM, Damien Le Moal wrote:
> On 4/20/22 05:58, Damien Le Moal wrote:
>> On 4/19/22 20:55, Jens Axboe wrote:
>>> On 4/19/22 5:00 AM, Damien Le Moal wrote:
>>>> 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 while
>>>> /dev/nullb0 wasalready created from modprobe. In this case, the configfs
>>>                 ^^^
>>>
>>> space
>>
>> Re-sending to fix this. Also realized that using "#define pr_fmt" would
>> simplify patch 3. Updating that.
>>
>>>
>>>> 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 th 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 new nullb device, for devices 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.
>>>
>>> This is nice, and solves both the confusing part of having
>>> pre-configured devices, but also using the actual directory name as the
>>> device name even if they are not ordered.
>>>
>>> Only odd bit is you can create a device name where a special file of
>>> that name already exists, but I don't think that's solvable in a clean
>>> way and we just need to ignore that. That's arguably a user error, don't
>>> pick names that already exist.
>>
>> Yes. add_disk() will fail if the device name already exist within the
>> "block" device class, but will not complain about anything if the existing
>> device belongs to another class.
>>
>> I could add a "block" class wide check for device name existence in
>> null_find_dev_by_name() instead of only checking the nullb list ?
> 
> Looked into this, but I do not see any easy ready-to-use way to do it
> since "struct class block_class" is not exported. And I would not wnat
> to export this block/dev internal symbol.
> 
> We could play with vfs_stat() to test for device existence, but that
> is a little ugly...

Eek no, let's not go that far!

> What about simply enforcing a name pattern like "nullb*" for the
> configfs directory/device names ? That is simple and the current
> nullb_find_dev_by_name() will keep working as is.

That might break existing use cases. I say just leave it alone. If you
pick a name that already exists in /dev, then too bad for you.

What I cared most about fixing here was situation of having an empty
directory and being able to mkdir nullb0 when that was already assigned.
And that's fixed with your patches.

-- 
Jens Axboe

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

[Damien Le Moal]

On 4/20/22 09:17, Jens Axboe wrote:
> On 4/19/22 5:13 PM, Damien Le Moal wrote:
>> On 4/20/22 05:58, Damien Le Moal wrote:
>>> On 4/19/22 20:55, Jens Axboe wrote:
>>>> On 4/19/22 5:00 AM, Damien Le Moal wrote:
>>>>> 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 while
>>>>> /dev/nullb0 wasalready created from modprobe. In this case, the configfs
>>>>                 ^^^
>>>>
>>>> space
>>>
>>> Re-sending to fix this. Also realized that using "#define pr_fmt" would
>>> simplify patch 3. Updating that.
>>>
>>>>
>>>>> 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 th 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 new nullb device, for devices 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.
>>>>
>>>> This is nice, and solves both the confusing part of having
>>>> pre-configured devices, but also using the actual directory name as the
>>>> device name even if they are not ordered.
>>>>
>>>> Only odd bit is you can create a device name where a special file of
>>>> that name already exists, but I don't think that's solvable in a clean
>>>> way and we just need to ignore that. That's arguably a user error, don't
>>>> pick names that already exist.
>>>
>>> Yes. add_disk() will fail if the device name already exist within the
>>> "block" device class, but will not complain about anything if the existing
>>> device belongs to another class.
>>>
>>> I could add a "block" class wide check for device name existence in
>>> null_find_dev_by_name() instead of only checking the nullb list ?
>>
>> Looked into this, but I do not see any easy ready-to-use way to do it
>> since "struct class block_class" is not exported. And I would not wnat
>> to export this block/dev internal symbol.
>>
>> We could play with vfs_stat() to test for device existence, but that
>> is a little ugly...
> 
> Eek no, let's not go that far!
> 
>> What about simply enforcing a name pattern like "nullb*" for the
>> configfs directory/device names ? That is simple and the current
>> nullb_find_dev_by_name() will keep working as is.
> 
> That might break existing use cases. I say just leave it alone. If you
> pick a name that already exists in /dev, then too bad for you.
> 
> What I cared most about fixing here was situation of having an empty
> directory and being able to mkdir nullb0 when that was already assigned.
> And that's fixed with your patches.
> 

OK. Works for me.
Sending v2 with the cleanups in path 3 and patch 4 commit message.
Thanks.

-- 
Damien Le Moal
Western Digital Research