| Message ID | 20241028090726.2958921-1-yangerkun@huaweicloud.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | brd: defer automatic disk creation until module initialization succeeds | expand |
On Mon, Oct 28, 2024 at 05:07:26PM +0800, Yang Erkun wrote: > Fix this problem by following what loop_init() does. Besides, > reintroduce brd_devices_mutex to help serialize modifications to > brd_list. This looks generally good. Minor comments below: > + snprintf(buf, DISK_NAME_LEN, "ram%d", i); > + mutex_lock(&brd_devices_mutex); > + list_for_each_entry(brd, &brd_devices, brd_list) { > + if (brd->brd_number == i) { > + mutex_unlock(&brd_devices_mutex); > + err = -EEXIST; > + goto out; This now prints an error message for an already existing device, which should not happen for the module_init case, but will happen all the time for the probe callback, and is really annoying. Please drop that part of the change. > + } > + } > brd = kzalloc(sizeof(*brd), GFP_KERNEL); > + if (!brd) { > + mutex_unlock(&brd_devices_mutex); > + err = -ENOMEM; > + goto out; > + } > brd->brd_number = i; > list_add_tail(&brd->brd_list, &brd_devices); > + mutex_unlock(&brd_devices_mutex); And maybe just split the whole locked section into a brd_find_or_alloc_device helper to make verifying the locking easier? > + mutex_lock(&brd_devices_mutex); > list_del(&brd->brd_list); > + mutex_unlock(&brd_devices_mutex); > kfree(brd); > + mutex_lock(&brd_devices_mutex); > list_del(&brd->brd_list); > + mutex_unlock(&brd_devices_mutex); > kfree(brd); Maybe a brd_free_device helper for this pattern would be nice as well. > + brd_check_and_reset_par(); > + brd_debugfs_dir = debugfs_create_dir("ramdisk_pages", NULL); > > if (__register_blkdev(RAMDISK_MAJOR, "ramdisk", brd_probe)) { > - err = -EIO; > - goto out_free; > + pr_info("brd: module NOT loaded !!!\n"); > + debugfs_remove_recursive(brd_debugfs_dir); > + return -EIO; I'd keep an error handling goto for this to keep the main path straight.
在 2024/10/28 17:44, Christoph Hellwig 写道: > On Mon, Oct 28, 2024 at 05:07:26PM +0800, Yang Erkun wrote: >> Fix this problem by following what loop_init() does. Besides, >> reintroduce brd_devices_mutex to help serialize modifications to >> brd_list. > > This looks generally good. Minor comments below: Hi, Thanks for your review! > >> + snprintf(buf, DISK_NAME_LEN, "ram%d", i); >> + mutex_lock(&brd_devices_mutex); >> + list_for_each_entry(brd, &brd_devices, brd_list) { >> + if (brd->brd_number == i) { >> + mutex_unlock(&brd_devices_mutex); >> + err = -EEXIST; >> + goto out; > > This now prints an error message for an already existing > device, which should not happen for the module_init case, > but will happen all the time for the probe callback, and > is really annoying. Please drop that part of the change. OK, maybe print nothing is better like loop_add in loop_init has did to leave code clean? mknod can help to create /dev/ram%d... Hi, Kuai, what do you think? > >> + } >> + } >> brd = kzalloc(sizeof(*brd), GFP_KERNEL); >> + if (!brd) { >> + mutex_unlock(&brd_devices_mutex); >> + err = -ENOMEM; >> + goto out; >> + } >> brd->brd_number = i; >> list_add_tail(&brd->brd_list, &brd_devices); >> + mutex_unlock(&brd_devices_mutex); > > And maybe just split the whole locked section into a > brd_find_or_alloc_device helper to make verifying the locking easier? Ok. > >> + mutex_lock(&brd_devices_mutex); >> list_del(&brd->brd_list); >> + mutex_unlock(&brd_devices_mutex); >> kfree(brd); > >> + mutex_lock(&brd_devices_mutex); >> list_del(&brd->brd_list); >> + mutex_unlock(&brd_devices_mutex); >> kfree(brd); > > Maybe a brd_free_device helper for this pattern would be nice as well. Ok. > >> + brd_check_and_reset_par(); >> + brd_debugfs_dir = debugfs_create_dir("ramdisk_pages", NULL); >> >> if (__register_blkdev(RAMDISK_MAJOR, "ramdisk", brd_probe)) { >> - err = -EIO; >> - goto out_free; >> + pr_info("brd: module NOT loaded !!!\n"); >> + debugfs_remove_recursive(brd_debugfs_dir); >> + return -EIO; > > I'd keep an error handling goto for this to keep the main path > straight. Ok.
Hi, 在 2024/10/28 17:44, Christoph Hellwig 写道: > On Mon, Oct 28, 2024 at 05:07:26PM +0800, Yang Erkun wrote: >> Fix this problem by following what loop_init() does. Besides, >> reintroduce brd_devices_mutex to help serialize modifications to >> brd_list. > > This looks generally good. Minor comments below: > >> + snprintf(buf, DISK_NAME_LEN, "ram%d", i); >> + mutex_lock(&brd_devices_mutex); >> + list_for_each_entry(brd, &brd_devices, brd_list) { >> + if (brd->brd_number == i) { >> + mutex_unlock(&brd_devices_mutex); >> + err = -EEXIST; >> + goto out; > > This now prints an error message for an already existing > device, which should not happen for the module_init case, > but will happen all the time for the probe callback, and > is really annoying. Please drop that part of the change. I don't quite understand this, if the gendisk already exists, the probe callback won't be called from the open path, because ilookup() from blkdev_get_no_open() will found the bdev inode. Hence there will only be a small race windown for concurrent create on open callers to return -EEXIST here. Thanks, Kuai
On Mon, Oct 28, 2024 at 07:29:54PM +0800, Yu Kuai wrote: > I don't quite understand this, if the gendisk already exists, > the probe callback won't be called from the open path, because > ilookup() from blkdev_get_no_open() will found the bdev inode. > Hence there will only be a small race windown for concurrent > create on open callers to return -EEXIST here. True. I'd still avoid the noice printk for that corner case, though.
Hi, 在 2024/10/29 0:13, Christoph Hellwig 写道: > On Mon, Oct 28, 2024 at 07:29:54PM +0800, Yu Kuai wrote: >> I don't quite understand this, if the gendisk already exists, >> the probe callback won't be called from the open path, because >> ilookup() from blkdev_get_no_open() will found the bdev inode. >> Hence there will only be a small race windown for concurrent >> create on open callers to return -EEXIST here. > > True. I'd still avoid the noice printk for that corner case, though. > ok, then I'm fine with remove this printk or move it to brd_init(), I don't have strong preference. Thanks, Kuai > . >
diff --git a/drivers/block/brd.c b/drivers/block/brd.c index 2fd1ed101748..a8615256fc57 100644 --- a/drivers/block/brd.c +++ b/drivers/block/brd.c @@ -316,6 +316,7 @@ __setup("ramdisk_size=", ramdisk_size); * (should share code eventually). */ static LIST_HEAD(brd_devices); +static DEFINE_MUTEX(brd_devices_mutex); static struct dentry *brd_debugfs_dir; static int brd_alloc(int i) @@ -340,18 +341,27 @@ static int brd_alloc(int i) BLK_FEAT_NOWAIT, }; - list_for_each_entry(brd, &brd_devices, brd_list) - if (brd->brd_number == i) - return -EEXIST; + snprintf(buf, DISK_NAME_LEN, "ram%d", i); + mutex_lock(&brd_devices_mutex); + list_for_each_entry(brd, &brd_devices, brd_list) { + if (brd->brd_number == i) { + mutex_unlock(&brd_devices_mutex); + err = -EEXIST; + goto out; + } + } brd = kzalloc(sizeof(*brd), GFP_KERNEL); - if (!brd) - return -ENOMEM; + if (!brd) { + mutex_unlock(&brd_devices_mutex); + err = -ENOMEM; + goto out; + } brd->brd_number = i; list_add_tail(&brd->brd_list, &brd_devices); + mutex_unlock(&brd_devices_mutex); xa_init(&brd->brd_pages); - snprintf(buf, DISK_NAME_LEN, "ram%d", i); if (!IS_ERR_OR_NULL(brd_debugfs_dir)) debugfs_create_u64(buf, 0444, brd_debugfs_dir, &brd->brd_nr_pages); @@ -378,8 +388,13 @@ static int brd_alloc(int i) out_cleanup_disk: put_disk(disk); out_free_dev: + mutex_lock(&brd_devices_mutex); list_del(&brd->brd_list); + mutex_unlock(&brd_devices_mutex); kfree(brd); +out: + pr_info("brd: %s for %s failed with %d.\n", + __func__, buf, err); return err; } @@ -398,7 +413,9 @@ static void brd_cleanup(void) del_gendisk(brd->brd_disk); put_disk(brd->brd_disk); brd_free_pages(brd); + mutex_lock(&brd_devices_mutex); list_del(&brd->brd_list); + mutex_unlock(&brd_devices_mutex); kfree(brd); } } @@ -424,17 +441,7 @@ static inline void brd_check_and_reset_par(void) static int __init brd_init(void) { - int err, i; - - brd_check_and_reset_par(); - - brd_debugfs_dir = debugfs_create_dir("ramdisk_pages", NULL); - - for (i = 0; i < rd_nr; i++) { - err = brd_alloc(i); - if (err) - goto out_free; - } + int i; /* * brd module now has a feature to instantiate underlying device @@ -450,20 +457,20 @@ static int __init brd_init(void) * If (X / max_part) was not already created it will be created * dynamically. */ + brd_check_and_reset_par(); + brd_debugfs_dir = debugfs_create_dir("ramdisk_pages", NULL); if (__register_blkdev(RAMDISK_MAJOR, "ramdisk", brd_probe)) { - err = -EIO; - goto out_free; + pr_info("brd: module NOT loaded !!!\n"); + debugfs_remove_recursive(brd_debugfs_dir); + return -EIO; } + for (i = 0; i < rd_nr; i++) + brd_alloc(i); + pr_info("brd: module loaded\n"); return 0; - -out_free: - brd_cleanup(); - - pr_info("brd: module NOT loaded !!!\n"); - return err; } static void __exit brd_exit(void)