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
