Nullblk configfs oddities

Nullblk configfs oddities

[Josef Bacik]

Hello,

I'm trying to add a test to fsperf and it requires the use of nullblk.  I'm
trying to use the configfs thing, and it's doing some odd things.  My basic
reproducer is

modprobe null_blk
mkdir /sys/kernel/config/nullb/nullb0
echo some shit into the config
echo 1 > /sys/kernel/config/nullb/nullb0/power

Now null_blk apparently defaults to nr_devices == 1, so it creates nullb0 on
modprobe.  But this doesn't show up in the configfs directory.  There's no way
to find this out until when I try to mkfs my nullb0 and it doesn't work.  The
above steps gets my device created at /dev/nullb1, but there's no actual way to
figure out that's what happened.  If I do something like
/sys/kernel/config/nullb/nullbfsperf I still just get nullb<number>, I don't get
my fancy name.

I don't really care what the solution looks like, I either need to be able to
create a random name and have a symlink automatically created so I can get to
the right device node (which I don't think can be done without udev, so I'm not
fan of this) or I need a way to figure out from configfs which device my thing
actually got created as.

It would also be nice if /sys/kernel/config/nullb/nullb0 was populated at
modinfo time so I know it's there already, then I could do something silly like
get the next number and use that as my device.  Thanks,

Josef

Re: Nullblk configfs oddities

[Chaitanya Kulkarni]

On 4/18/22 14:38, Josef Bacik wrote:
> Hello,
> 
> I'm trying to add a test to fsperf and it requires the use of nullblk.  I'm
> trying to use the configfs thing, and it's doing some odd things.  My basic
> reproducer is
> 
> modprobe null_blk
> mkdir /sys/kernel/config/nullb/nullb0
> echo some shit into the config
> echo 1 > /sys/kernel/config/nullb/nullb0/power
> 
> Now null_blk apparently defaults to nr_devices == 1, so it creates nullb0 on
> modprobe.  But this doesn't show up in the configfs directory.  There's no way
> to find this out until when I try to mkfs my nullb0 and it doesn't work.  The
> above steps gets my device created at /dev/nullb1, but there's no actual way to
> figure out that's what happened.  If I do something like
> /sys/kernel/config/nullb/nullbfsperf I still just get nullb<number>, I don't get
> my fancy name.
> 

when you load module with default module parameter it will create a 
default device with no memory backed mode, that will not be visible in 
the configfs.

So you need to load the module with nr_devices=0 that will prevent the 
null_blk to create the default device which is not memory backed and not 
present in the configfs:-

linux-block (for-next) # modprobe null_blk
linux-block (for-next) # lsblk
NAME    MAJ:MIN RM  SIZE RO TYPE MOUNTPOINT
sda       8:0    0   50G  0 disk
├─sda1    8:1    0    1G  0 part /boot
└─sda2    8:2    0   49G  0 part /home
sdb       8:16   0  100G  0 disk /mnt/data
sr0      11:0    1 1024M  0 rom
nullb0  250:0    0  250G  0 disk <-------------------
zram0   251:0    0    8G  0 disk [SWAP]
vda     252:0    0  512M  0 disk
nvme0n1 259:0    0    1G  0 disk
linux-block (for-next) # tree config
config
└── nullb
     └── features

1 directory, 1 file
linux-block (for-next) # modprobe  -r null_blk
linux-block (for-next) # modprobe null_blk nr_devices=0
linux-block (for-next) # lsblk
NAME    MAJ:MIN RM  SIZE RO TYPE MOUNTPOINT
sda       8:0    0   50G  0 disk
├─sda1    8:1    0    1G  0 part /boot
└─sda2    8:2    0   49G  0 part /home
sdb       8:16   0  100G  0 disk /mnt/data
sr0      11:0    1 1024M  0 rom
zram0   251:0    0    8G  0 disk [SWAP]
vda     252:0    0  512M  0 disk
nvme0n1 259:0    0    1G  0 disk
linux-block (for-next) # tree config
config
└── nullb
     └── features

1 directory, 1 file
linux-block (for-next) #

-ck

Re: Nullblk configfs oddities

[Josef Bacik]

On Mon, Apr 18, 2022 at 5:54 PM Chaitanya Kulkarni
<chaitanyak@nvidia.com> wrote:
>
> On 4/18/22 14:38, Josef Bacik wrote:
> > Hello,
> >
> > I'm trying to add a test to fsperf and it requires the use of nullblk.  I'm
> > trying to use the configfs thing, and it's doing some odd things.  My basic
> > reproducer is
> >
> > modprobe null_blk
> > mkdir /sys/kernel/config/nullb/nullb0
> > echo some shit into the config
> > echo 1 > /sys/kernel/config/nullb/nullb0/power
> >
> > Now null_blk apparently defaults to nr_devices == 1, so it creates nullb0 on
> > modprobe.  But this doesn't show up in the configfs directory.  There's no way
> > to find this out until when I try to mkfs my nullb0 and it doesn't work.  The
> > above steps gets my device created at /dev/nullb1, but there's no actual way to
> > figure out that's what happened.  If I do something like
> > /sys/kernel/config/nullb/nullbfsperf I still just get nullb<number>, I don't get
> > my fancy name.
> >
>
> when you load module with default module parameter it will create a
> default device with no memory backed mode, that will not be visible in
> the configfs.
>
> So you need to load the module with nr_devices=0 that will prevent the
> null_blk to create the default device which is not memory backed and not
> present in the configfs:-
>

Yup I know what it's doing, I'm raising this as it's weird and took me
a bit to work out what was happening, and it annoyed me.  It's not
anything I can't work around, but the UX kinda sucks.  Thanks,

Josef

Re: Nullblk configfs oddities

[Jens Axboe]

On 4/18/22 3:54 PM, Chaitanya Kulkarni wrote:
> On 4/18/22 14:38, Josef Bacik wrote:
>> Hello,
>>
>> I'm trying to add a test to fsperf and it requires the use of nullblk.  I'm
>> trying to use the configfs thing, and it's doing some odd things.  My basic
>> reproducer is
>>
>> modprobe null_blk
>> mkdir /sys/kernel/config/nullb/nullb0
>> echo some shit into the config
>> echo 1 > /sys/kernel/config/nullb/nullb0/power
>>
>> Now null_blk apparently defaults to nr_devices == 1, so it creates nullb0 on
>> modprobe.  But this doesn't show up in the configfs directory.  There's no way
>> to find this out until when I try to mkfs my nullb0 and it doesn't work.  The
>> above steps gets my device created at /dev/nullb1, but there's no actual way to
>> figure out that's what happened.  If I do something like
>> /sys/kernel/config/nullb/nullbfsperf I still just get nullb<number>, I don't get
>> my fancy name.
>>
> 
> when you load module with default module parameter it will create a 
> default device with no memory backed mode, that will not be visible in 
> the configfs.

Right, the problem is really that pre-configured devices (via nr_devices
being bigger than 0, which is the default) don't show up in configfs.
That, to me, is the real issue here, because it means you need to know
which ones are already setup before doing mkdir for a new one.

On top of that, it's also odd that they don't show up there to begin
with.

-- 
Jens Axboe

Re: Nullblk configfs oddities

[Chaitanya Kulkarni]

>> when you load module with default module parameter it will create a
>> default device with no memory backed mode, that will not be visible in
>> the configfs.
>>
>> So you need to load the module with nr_devices=0 that will prevent the
>> null_blk to create the default device which is not memory backed and not
>> present in the configfs:-
>>
> 
> Yup I know what it's doing, I'm raising this as it's weird and took me
> a bit to work out what was happening, and it annoyed me.  It's not
> anything I can't work around, but the UX kinda sucks.  Thanks,
> 
> Josef

hmmm, how about a following patch, this will create memory backed
default devices at the time of loading the module ?

# git diff
diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c
index c441a4972064..d45c2f3f5692 100644
--- a/drivers/block/null_blk/main.c
+++ b/drivers/block/null_blk/main.c
@@ -148,7 +148,7 @@ static const struct kernel_param_ops 
null_queue_mode_param_ops = {
  device_param_cb(queue_mode, &null_queue_mode_param_ops, &g_queue_mode, 
0444);
  MODULE_PARM_DESC(queue_mode, "Block interface to use 
(0=bio,1=rq,2=multiqueue)");

-static int g_gb = 250;
+static int g_gb = 1;
  module_param_named(gb, g_gb, int, 0444);
  MODULE_PARM_DESC(gb, "Size in GB");

@@ -168,6 +168,10 @@ static bool g_blocking;
  module_param_named(blocking, g_blocking, bool, 0444);
  MODULE_PARM_DESC(blocking, "Register as a blocking blk-mq driver device");

+static bool g_memory_backed;
+module_param_named(memory_backed, g_memory_backed, bool, 0444);
+MODULE_PARM_DESC(memory_backed, "memory backed device, default:false");
+
  static bool shared_tags;
  module_param(shared_tags, bool, 0444);
  MODULE_PARM_DESC(shared_tags, "Share tag set between devices for blk-mq");
@@ -657,6 +661,8 @@ static struct nullb_device *null_alloc_dev(void)
         dev->zone_max_open = g_zone_max_open;
         dev->zone_max_active = g_zone_max_active;
         dev->virt_boundary = g_virt_boundary;
+       dev->memory_backed = g_memory_backed;
+
         return dev;
  }


This passed the fio verification test when loaded with newly added 
module parameter memory_backed=1:-

+ modprobe -r null_blk
+ modprobe null_blk
+ tree config/
config/
└── nullb
     └── features

1 directory, 1 file
+ lsblk
+ grep null
nullb0  250:0    0    1G  0 disk
+ fio fio/verify.fio --filename=/dev/nullb0 --output=/tmp/fio.log
verify: bad header offset 668917760, wanted 0 at file /dev/nullb0 offset 
0, length 4096 (requested block: offset=0, length=4096)
verify: bad header offset 746139648, wanted 4096 at file /dev/nullb0 
offset 4096, length 4096 (requested block: offset=4096, length=4096)
verify: bad header offset 705691648, wanted 8192 at file /dev/nullb0 
offset 8192, length 4096 (requested block: offset=8192, length=4096)
verify: bad header offset 363917312, wanted 12288 at file /dev/nullb0 
offset 12288, length 4096 (requested block: offset=12288, length=4096)
+ grep err= /tmp/fio.log
write-and-verify: (groupid=0, jobs=1): err=84 (file:io_u.c:2141, 
func=io_u_queued_complete, error=Invalid or incomplete multibyte or wide 
character): pid=21015: Mon Apr 18 15:14:36 2022
+ modprobe -r null_blk
+ modprobe -r null_blk
+ modprobe null_blk memory_backed=1
+ tree config/
config/
└── nullb
     └── features

1 directory, 1 file
+ lsblk
+ grep null
nullb0  250:0    0    1G  0 disk
+ fio fio/verify.fio --filename=/dev/nullb0 --output=/tmp/fio.log
+ grep err= /tmp/fio.log
write-and-verify: (groupid=0, jobs=1): err= 0: pid=21025: Mon Apr 18 
15:14:38 2022
+ modprobe -r null_blk


-ck

Re: Nullblk configfs oddities

[Chaitanya Kulkarni]

On 4/18/22 15:14, Jens Axboe wrote:
> On 4/18/22 3:54 PM, Chaitanya Kulkarni wrote:
>> On 4/18/22 14:38, Josef Bacik wrote:
>>> Hello,
>>>
>>> I'm trying to add a test to fsperf and it requires the use of nullblk.  I'm
>>> trying to use the configfs thing, and it's doing some odd things.  My basic
>>> reproducer is
>>>
>>> modprobe null_blk
>>> mkdir /sys/kernel/config/nullb/nullb0
>>> echo some shit into the config
>>> echo 1 > /sys/kernel/config/nullb/nullb0/power
>>>
>>> Now null_blk apparently defaults to nr_devices == 1, so it creates nullb0 on
>>> modprobe.  But this doesn't show up in the configfs directory.  There's no way
>>> to find this out until when I try to mkfs my nullb0 and it doesn't work.  The
>>> above steps gets my device created at /dev/nullb1, but there's no actual way to
>>> figure out that's what happened.  If I do something like
>>> /sys/kernel/config/nullb/nullbfsperf I still just get nullb<number>, I don't get
>>> my fancy name.
>>>
>>
>> when you load module with default module parameter it will create a
>> default device with no memory backed mode, that will not be visible in
>> the configfs.
> 
> Right, the problem is really that pre-configured devices (via nr_devices
> being bigger than 0, which is the default) don't show up in configfs.
> That, to me, is the real issue here, because it means you need to know
> which ones are already setup before doing mkdir for a new one.
> 
> On top of that, it's also odd that they don't show up there to begin
> with.
> 

it is indeed confusing, maybe we need to find a way to populate the
configfs when loading the module? but I'm not sure if that is
the right approach since configs ideally should be populated by
user.

OTOH we can make the memory_backed module param [1] so user can
tentatively not use configfs and only rely on default configuration ?


-ck

[1] https://marc.info/?l=linux-block&m=165032010206884&w=2

Re: Nullblk configfs oddities

[Jens Axboe]

On 4/18/22 4:21 PM, Chaitanya Kulkarni wrote:
> On 4/18/22 15:14, Jens Axboe wrote:
>> On 4/18/22 3:54 PM, Chaitanya Kulkarni wrote:
>>> On 4/18/22 14:38, Josef Bacik wrote:
>>>> Hello,
>>>>
>>>> I'm trying to add a test to fsperf and it requires the use of nullblk.  I'm
>>>> trying to use the configfs thing, and it's doing some odd things.  My basic
>>>> reproducer is
>>>>
>>>> modprobe null_blk
>>>> mkdir /sys/kernel/config/nullb/nullb0
>>>> echo some shit into the config
>>>> echo 1 > /sys/kernel/config/nullb/nullb0/power
>>>>
>>>> Now null_blk apparently defaults to nr_devices == 1, so it creates nullb0 on
>>>> modprobe.  But this doesn't show up in the configfs directory.  There's no way
>>>> to find this out until when I try to mkfs my nullb0 and it doesn't work.  The
>>>> above steps gets my device created at /dev/nullb1, but there's no actual way to
>>>> figure out that's what happened.  If I do something like
>>>> /sys/kernel/config/nullb/nullbfsperf I still just get nullb<number>, I don't get
>>>> my fancy name.
>>>>
>>>
>>> when you load module with default module parameter it will create a
>>> default device with no memory backed mode, that will not be visible in
>>> the configfs.
>>
>> Right, the problem is really that pre-configured devices (via nr_devices
>> being bigger than 0, which is the default) don't show up in configfs.
>> That, to me, is the real issue here, because it means you need to know
>> which ones are already setup before doing mkdir for a new one.
>>
>> On top of that, it's also odd that they don't show up there to begin
>> with.
>>
> 
> it is indeed confusing, maybe we need to find a way to populate the
> configfs when loading the module? but I'm not sure if that is
> the right approach since configs ideally should be populated by
> user.
> 
> OTOH we can make the memory_backed module param [1] so user can
> tentatively not use configfs and only rely on default configuration ?

Arguably configfs should just be disabled if loading with nr_devices
larger than 0, as it's a mess of an API as it stands. But probably too
late for that. The fact that we also have an option that's specific to
configfs just makes it even worse.

I don't know much about configfs, but pre-populating with the configured
devices and options would be ideal and completely solve this. I think
that would be the best solution given the current situation. Not that
it's THAT important, null_blk is a developer tool and as such can have
some sharper and rouger edges. Still would be nice to make it saner,
though.

-- 
Jens Axboe

Re: Nullblk configfs oddities

[Josef Bacik]

On Mon, Apr 18, 2022 at 6:24 PM Jens Axboe <axboe@kernel.dk> wrote:
>
> On 4/18/22 4:21 PM, Chaitanya Kulkarni wrote:
> > On 4/18/22 15:14, Jens Axboe wrote:
> >> On 4/18/22 3:54 PM, Chaitanya Kulkarni wrote:
> >>> On 4/18/22 14:38, Josef Bacik wrote:
> >>>> Hello,
> >>>>
> >>>> I'm trying to add a test to fsperf and it requires the use of nullblk.  I'm
> >>>> trying to use the configfs thing, and it's doing some odd things.  My basic
> >>>> reproducer is
> >>>>
> >>>> modprobe null_blk
> >>>> mkdir /sys/kernel/config/nullb/nullb0
> >>>> echo some shit into the config
> >>>> echo 1 > /sys/kernel/config/nullb/nullb0/power
> >>>>
> >>>> Now null_blk apparently defaults to nr_devices == 1, so it creates nullb0 on
> >>>> modprobe.  But this doesn't show up in the configfs directory.  There's no way
> >>>> to find this out until when I try to mkfs my nullb0 and it doesn't work.  The
> >>>> above steps gets my device created at /dev/nullb1, but there's no actual way to
> >>>> figure out that's what happened.  If I do something like
> >>>> /sys/kernel/config/nullb/nullbfsperf I still just get nullb<number>, I don't get
> >>>> my fancy name.
> >>>>
> >>>
> >>> when you load module with default module parameter it will create a
> >>> default device with no memory backed mode, that will not be visible in
> >>> the configfs.
> >>
> >> Right, the problem is really that pre-configured devices (via nr_devices
> >> being bigger than 0, which is the default) don't show up in configfs.
> >> That, to me, is the real issue here, because it means you need to know
> >> which ones are already setup before doing mkdir for a new one.
> >>
> >> On top of that, it's also odd that they don't show up there to begin
> >> with.
> >>
> >
> > it is indeed confusing, maybe we need to find a way to populate the
> > configfs when loading the module? but I'm not sure if that is
> > the right approach since configs ideally should be populated by
> > user.
> >
> > OTOH we can make the memory_backed module param [1] so user can
> > tentatively not use configfs and only rely on default configuration ?
>
> Arguably configfs should just be disabled if loading with nr_devices
> larger than 0, as it's a mess of an API as it stands. But probably too
> late for that. The fact that we also have an option that's specific to
> configfs just makes it even worse.
>
> I don't know much about configfs, but pre-populating with the configured
> devices and options would be ideal and completely solve this. I think
> that would be the best solution given the current situation. Not that
> it's THAT important, null_blk is a developer tool and as such can have
> some sharper and rouger edges. Still would be nice to make it saner,
> though.
>

I'd settle for a sysfs link to the device node that got created, so I
can maybe just do

mdkir nullb/lolmine
dev=$(find nullb/lolmine --maxdepth 1 -type l)
devname=$(basename $dev)

so I know what my device actually got created as.  Thanks,

Josef

Re: Nullblk configfs oddities

[Damien Le Moal]

On 4/19/22 07:24, Jens Axboe wrote:
> On 4/18/22 4:21 PM, Chaitanya Kulkarni wrote:
>> On 4/18/22 15:14, Jens Axboe wrote:
>>> On 4/18/22 3:54 PM, Chaitanya Kulkarni wrote:
>>>> On 4/18/22 14:38, Josef Bacik wrote:
>>>>> Hello,
>>>>>
>>>>> I'm trying to add a test to fsperf and it requires the use of nullblk.  I'm
>>>>> trying to use the configfs thing, and it's doing some odd things.  My basic
>>>>> reproducer is
>>>>>
>>>>> modprobe null_blk
>>>>> mkdir /sys/kernel/config/nullb/nullb0
>>>>> echo some shit into the config
>>>>> echo 1 > /sys/kernel/config/nullb/nullb0/power
>>>>>
>>>>> Now null_blk apparently defaults to nr_devices == 1, so it creates nullb0 on
>>>>> modprobe.  But this doesn't show up in the configfs directory.  There's no way
>>>>> to find this out until when I try to mkfs my nullb0 and it doesn't work.  The
>>>>> above steps gets my device created at /dev/nullb1, but there's no actual way to
>>>>> figure out that's what happened.  If I do something like
>>>>> /sys/kernel/config/nullb/nullbfsperf I still just get nullb<number>, I don't get
>>>>> my fancy name.
>>>>>
>>>>
>>>> when you load module with default module parameter it will create a
>>>> default device with no memory backed mode, that will not be visible in
>>>> the configfs.
>>>
>>> Right, the problem is really that pre-configured devices (via nr_devices
>>> being bigger than 0, which is the default) don't show up in configfs.
>>> That, to me, is the real issue here, because it means you need to know
>>> which ones are already setup before doing mkdir for a new one.
>>>
>>> On top of that, it's also odd that they don't show up there to begin
>>> with.
>>>
>>
>> it is indeed confusing, maybe we need to find a way to populate the
>> configfs when loading the module? but I'm not sure if that is
>> the right approach since configs ideally should be populated by
>> user.
>>
>> OTOH we can make the memory_backed module param [1] so user can
>> tentatively not use configfs and only rely on default configuration ?
> 
> Arguably configfs should just be disabled if loading with nr_devices
> larger than 0, as it's a mess of an API as it stands. But probably too
> late for that. The fact that we also have an option that's specific to
> configfs just makes it even worse.
> 
> I don't know much about configfs, but pre-populating with the configured
> devices and options would be ideal and completely solve this. I think
> that would be the best solution given the current situation. Not that
> it's THAT important, null_blk is a developer tool and as such can have
> some sharper and rouger edges. Still would be nice to make it saner,
> though.
> 

I came up with this. It does prepopulate configfs nullb directory with the
devices created on modprobe. But... doing an rmmod now always gives a
"rmmod: ERROR: Module null_blk is in use" because configfs takes a ref on
nullblk module for each entry. So the user now must do an rmdir of all
prepopulated devices before doing rmmod. Not ideal. And messing up with
the module references is probably not a good idea...


diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c
index 1aa4897685f6..98933555b59f 100644
--- a/drivers/block/null_blk/main.c
+++ b/drivers/block/null_blk/main.c
@@ -9,6 +9,7 @@
 #include <linux/sched.h>
 #include <linux/fs.h>
 #include <linux/init.h>
+#include <linux/namei.h>
 #include "null_blk.h"

 #define FREE_BATCH		16
@@ -667,6 +668,7 @@ static void null_free_dev(struct nullb_device *dev)

 	null_free_zoned_dev(dev);
 	badblocks_exit(&dev->badblocks);
+	kfree(dev->config_path);
 	kfree(dev);
 }

@@ -2088,12 +2090,121 @@ static int null_add_dev(struct nullb_device *dev)
 	return rv;
 }

+#ifdef CONFIG_CONFIGFS_FS
+
+static int nullb_create_dev(int idx)
+{
+	char disk_name[DISK_NAME_LEN];
+	struct nullb_device *dev;
+	struct config_item *item;
+	struct dentry *dentry;
+	struct path parent;
+	const char *path;
+	int ret;
+
+	/* Use configfs to allocate the device */
+	sprintf(disk_name, "nullb%d", idx);
+	path = kasprintf(GFP_KERNEL, "/sys/kernel/config/nullb/%s", disk_name);
+	if (!path)
+		return -ENOMEM;
+
+	dentry = kern_path_create(AT_FDCWD, path, &parent, LOOKUP_DIRECTORY);
+	if (IS_ERR(dentry)) {
+		ret = PTR_ERR(dentry);
+		goto free;
+	}
+
+	ret = vfs_mkdir(mnt_user_ns(parent.mnt), d_inode(parent.dentry),
+			dentry, 0755);
+	if (ret)
+		goto done;
+
+	/* Start the device */
+	item = config_group_find_item(&nullb_subsys.su_group, disk_name);
+	if (!item) {
+		pr_err("Device %s not powered up\n", disk_name);
+		goto done;
+	}
+
+	dev = to_nullb_device(item);
+	set_bit(NULLB_DEV_FL_UP, &dev->flags);
+	ret = null_add_dev(dev);
+	if (ret) {
+		clear_bit(NULLB_DEV_FL_UP, &dev->flags);
+		goto put;
+	}
+
+	set_bit(NULLB_DEV_FL_CONFIGURED, &dev->flags);
+	dev->power = true;
+	dev->config_path = path;
+	path = NULL;
+
+put:
+	config_item_put(item);
+done:
+	done_path_create(&parent, dentry);
+free:
+	kfree(path);
+
+	return ret;
+}
+
+static void nullb_destroy_dev(struct nullb *nullb)
+{
+	struct nullb_device *dev = nullb->dev;
+	struct dentry *dentry;
+	struct path parent;
+
+	dentry = kern_path_create(AT_FDCWD, dev->config_path, &parent,
LOOKUP_DIRECTORY);
+	if (IS_ERR(dentry)) {
+		pr_err("Lookup %s failed %ld\n",
+		       dev->config_path, PTR_ERR(dentry));
+		return;
+	}
+
+	if (d_really_is_positive(dentry)) {
+		vfs_rmdir(mnt_user_ns(parent.mnt), d_inode(parent.dentry),
+			  dentry);
+		dput(dentry);
+	}
+
+	done_path_create(&parent, dentry);
+}
+
+#else
+
+static int nullb_create_dev(int idx)
+{
+	struct nullb_device *dev;
+
+	dev = null_alloc_dev();
+	if (!dev)
+		return -ENOMEM;
+
+	ret = null_add_dev(dev);
+	if (ret) {
+		null_free_dev(dev);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int nullb_destroy_dev(struct nullb *nullb)
+{
+	struct nullb_device *dev = nullb->dev;
+
+	null_del_dev(nullb);
+	null_free_dev(dev);
+}
+
+#endif
+
 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 +2262,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 = nullb_create_dev(i);
+		if (ret)
 			goto err_dev;
-		}
 	}

 	pr_info("module loaded\n");
@@ -2169,9 +2273,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);
+		nullb_destroy_dev(nullb);
 	}
 	unregister_blkdev(null_major, "nullb");
 err_conf:
diff --git a/drivers/block/null_blk/null_blk.h
b/drivers/block/null_blk/null_blk.h
index 78eb56b0ca55..ecdc22e74d35 100644
--- a/drivers/block/null_blk/null_blk.h
+++ b/drivers/block/null_blk/null_blk.h
@@ -61,6 +61,7 @@ struct nullb_zone {
 struct nullb_device {
 	struct nullb *nullb;
 	struct config_item item;
+	const char *config_path;
 	struct radix_tree_root data; /* data stored in the disk */
 	struct radix_tree_root cache; /* disk cache data */
 	unsigned long flags; /* device flags */



-- 
Damien Le Moal
Western Digital Research

Re: Nullblk configfs oddities

[Bart Van Assche]

On 4/18/22 21:23, Damien Le Moal wrote:
> +	ret = vfs_mkdir(mnt_user_ns(parent.mnt), d_inode(parent.dentry),
> +			dentry, 0755);

Christoph, if I remember correctly functionality has been added recently
to configfs for creating directories from inside the kernel without 
calling vfs_mkdir()?

Thanks,

Bart.

Re: Nullblk configfs oddities

[Josef Bacik]

On Tue, Apr 19, 2022 at 01:23:22PM +0900, Damien Le Moal wrote:
> On 4/19/22 07:24, Jens Axboe wrote:
> > On 4/18/22 4:21 PM, Chaitanya Kulkarni wrote:
> >> On 4/18/22 15:14, Jens Axboe wrote:
> >>> On 4/18/22 3:54 PM, Chaitanya Kulkarni wrote:
> >>>> On 4/18/22 14:38, Josef Bacik wrote:
> >>>>> Hello,
> >>>>>
> >>>>> I'm trying to add a test to fsperf and it requires the use of nullblk.  I'm
> >>>>> trying to use the configfs thing, and it's doing some odd things.  My basic
> >>>>> reproducer is
> >>>>>
> >>>>> modprobe null_blk
> >>>>> mkdir /sys/kernel/config/nullb/nullb0
> >>>>> echo some shit into the config
> >>>>> echo 1 > /sys/kernel/config/nullb/nullb0/power
> >>>>>
> >>>>> Now null_blk apparently defaults to nr_devices == 1, so it creates nullb0 on
> >>>>> modprobe.  But this doesn't show up in the configfs directory.  There's no way
> >>>>> to find this out until when I try to mkfs my nullb0 and it doesn't work.  The
> >>>>> above steps gets my device created at /dev/nullb1, but there's no actual way to
> >>>>> figure out that's what happened.  If I do something like
> >>>>> /sys/kernel/config/nullb/nullbfsperf I still just get nullb<number>, I don't get
> >>>>> my fancy name.
> >>>>>
> >>>>
> >>>> when you load module with default module parameter it will create a
> >>>> default device with no memory backed mode, that will not be visible in
> >>>> the configfs.
> >>>
> >>> Right, the problem is really that pre-configured devices (via nr_devices
> >>> being bigger than 0, which is the default) don't show up in configfs.
> >>> That, to me, is the real issue here, because it means you need to know
> >>> which ones are already setup before doing mkdir for a new one.
> >>>
> >>> On top of that, it's also odd that they don't show up there to begin
> >>> with.
> >>>
> >>
> >> it is indeed confusing, maybe we need to find a way to populate the
> >> configfs when loading the module? but I'm not sure if that is
> >> the right approach since configs ideally should be populated by
> >> user.
> >>
> >> OTOH we can make the memory_backed module param [1] so user can
> >> tentatively not use configfs and only rely on default configuration ?
> > 
> > Arguably configfs should just be disabled if loading with nr_devices
> > larger than 0, as it's a mess of an API as it stands. But probably too
> > late for that. The fact that we also have an option that's specific to
> > configfs just makes it even worse.
> > 
> > I don't know much about configfs, but pre-populating with the configured
> > devices and options would be ideal and completely solve this. I think
> > that would be the best solution given the current situation. Not that
> > it's THAT important, null_blk is a developer tool and as such can have
> > some sharper and rouger edges. Still would be nice to make it saner,
> > though.
> > 
> 
> I came up with this. It does prepopulate configfs nullb directory with the
> devices created on modprobe. But... doing an rmmod now always gives a
> "rmmod: ERROR: Module null_blk is in use" because configfs takes a ref on
> nullblk module for each entry. So the user now must do an rmdir of all
> prepopulated devices before doing rmmod. Not ideal. And messing up with
> the module references is probably not a good idea...
> 

I meant to reply to this previously, I tried this and it worked fine for me.
I'll leave it up to you guys if the new behavior is acceptable to you, but for
me this worked.  Thanks,

Josef