| Message ID | 20220419110038.3728406-5-damien.lemoal@opensource.wdc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | null_blk cleanup and device naming improvements | expand |
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.
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 ?
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.
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.
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.
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;
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(-)