Re: [PATCH] brd: defer automatic disk creation until module initialization succeeds

[Christoph Hellwig <hch@lst.de>]

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.