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
