Re: [PATCH] block/genhd.c: Add error handling

[Jeff Moyer <jmoyer@redhat.com>]

Vishnu Pratap Singh <vishnu.ps@samsung.com> writes:

> This patch adds error handling for blk_register_region(),
> register_disk(), add_disk(), disk_alloc_events() & disk_add_events().
> add_disk() & register_disk() functions  error handling is very much needed
> as this is used mainly by many modules like mmc, zram, mtd, scsi etc.
> But there is no error handling and it may cause stability issues also.

Hi, Vishnu,

The patch looks fine, but I have to ask what prompted you to write it?
If there's a specific failure you saw, it would be interesting to know
what that was.

If we're going to go down this path, then I think we should add error
handling to the callers of add_disk and blk_register_region.  Is that
something you'd be interested in working on?

Cheers,
Jeff

> Verfied on X86 based ubuntu machine.
>
> Signed-off-by: Vishnu Pratap Singh <vishnu.ps@samsung.com>
> ---
>  block/genhd.c         |   92 ++++++++++++++++++++++++++++++++++---------------
>  include/linux/genhd.h |    4 +--
>  2 files changed, 67 insertions(+), 29 deletions(-)
>
> diff --git a/block/genhd.c b/block/genhd.c
> index e552e1b..8589e01 100644
> --- a/block/genhd.c
> +++ b/block/genhd.c
> @@ -39,8 +39,8 @@ static struct device_type disk_type;
>  
>  static void disk_check_events(struct disk_events *ev,
>  			      unsigned int *clearing_ptr);
> -static void disk_alloc_events(struct gendisk *disk);
> -static void disk_add_events(struct gendisk *disk);
> +static int disk_alloc_events(struct gendisk *disk);
> +static int disk_add_events(struct gendisk *disk);
>  static void disk_del_events(struct gendisk *disk);
>  static void disk_release_events(struct gendisk *disk);
>  
> @@ -473,11 +473,11 @@ static char *bdevt_str(dev_t devt, char *buf)
>   * range must be nonzero
>   * The hash chain is sorted on range, so that subranges can override.
>   */
> -void blk_register_region(dev_t devt, unsigned long range, struct module *module,
> +int blk_register_region(dev_t devt, unsigned long range, struct module *module,
>  			 struct kobject *(*probe)(dev_t, int *, void *),
>  			 int (*lock)(dev_t, void *), void *data)
>  {
> -	kobj_map(bdev_map, devt, range, module, probe, lock, data);
> +	return kobj_map(bdev_map, devt, range, module, probe, lock, data);
>  }
>  
>  EXPORT_SYMBOL(blk_register_region);
> @@ -505,7 +505,7 @@ static int exact_lock(dev_t devt, void *data)
>  	return 0;
>  }
>  
> -static void register_disk(struct gendisk *disk)
> +static int register_disk(struct gendisk *disk)
>  {
>  	struct device *ddev = disk_to_dev(disk);
>  	struct block_device *bdev;
> @@ -520,14 +520,15 @@ static void register_disk(struct gendisk *disk)
>  	/* delay uevents, until we scanned partition table */
>  	dev_set_uevent_suppress(ddev, 1);
>  
> -	if (device_add(ddev))
> -		return;
> +	err = device_add(ddev);
> +	if (err)
> +		return err;
>  	if (!sysfs_deprecated) {
>  		err = sysfs_create_link(block_depr, &ddev->kobj,
>  					kobject_name(&ddev->kobj));
>  		if (err) {
>  			device_del(ddev);
> -			return;
> +			return err;
>  		}
>  	}
>  
> @@ -558,6 +559,7 @@ static void register_disk(struct gendisk *disk)
>  	if (err < 0)
>  		goto exit;
>  	blkdev_put(bdev, FMODE_READ);
> +	return 0;
>  
>  exit:
>  	/* announce disk after possible partitions are created */
> @@ -569,6 +571,7 @@ exit:
>  	while ((part = disk_part_iter_next(&piter)))
>  		kobject_uevent(&part_to_dev(part)->kobj, KOBJ_ADD);
>  	disk_part_iter_exit(&piter);
> +	return err;
>  }
>  
>  /**
> @@ -577,10 +580,8 @@ exit:
>   *
>   * This function registers the partitioning information in @disk
>   * with the kernel.
> - *
> - * FIXME: error handling
>   */
> -void add_disk(struct gendisk *disk)
> +int add_disk(struct gendisk *disk)
>  {
>  	struct backing_dev_info *bdi;
>  	dev_t devt;
> @@ -598,7 +599,7 @@ void add_disk(struct gendisk *disk)
>  	retval = blk_alloc_devt(&disk->part0, &devt);
>  	if (retval) {
>  		WARN_ON(1);
> -		return;
> +		return retval;
>  	}
>  	disk_to_dev(disk)->devt = devt;
>  
> @@ -608,16 +609,27 @@ void add_disk(struct gendisk *disk)
>  	disk->major = MAJOR(devt);
>  	disk->first_minor = MINOR(devt);
>  
> -	disk_alloc_events(disk);
> -
> +	retval = disk_alloc_events(disk);
> +	if (retval)
> +		goto err_blk_devt;
>  	/* Register BDI before referencing it from bdev */
>  	bdi = &disk->queue->backing_dev_info;
> -	bdi_register_dev(bdi, disk_devt(disk));
> +	retval = bdi_register_dev(bdi, disk_devt(disk));
> +	if (retval)
> +		goto err_alloc_event;
>  
> -	blk_register_region(disk_devt(disk), disk->minors, NULL,
> +	retval = blk_register_region(disk_devt(disk), disk->minors, NULL,
>  			    exact_match, exact_lock, disk);
> -	register_disk(disk);
> -	blk_register_queue(disk);
> +	if (retval)
> +		goto err_alloc_event;
> +
> +	retval = register_disk(disk);
> +	if (retval)
> +		goto err_reg_region;
> +
> +	retval = blk_register_queue(disk);
> +	if (retval)
> +		goto err_reg_disk;
>  
>  	/*
>  	 * Take an extra ref on queue which will be put on disk_release()
> @@ -627,9 +639,29 @@ void add_disk(struct gendisk *disk)
>  
>  	retval = sysfs_create_link(&disk_to_dev(disk)->kobj, &bdi->dev->kobj,
>  				   "bdi");
> -	WARN_ON(retval);
> +	if (retval)
> +		goto err_blk_reg_queue;
> +
> +	retval = disk_add_events(disk);
> +	if (retval)
> +		goto err_sysfs;
> +
> +	return 0;
> +
> +err_blk_devt:
> +	blk_free_devt(devt);
> +err_alloc_event:
> +	disk_del_events(disk);
> +err_reg_region:
> +	blk_unregister_region(disk_devt(disk), disk->minors);
> +err_reg_disk:
> +	del_gendisk(disk);
> +err_blk_reg_queue:
> +	blk_unregister_queue(disk);
> +err_sysfs:
> +	sysfs_remove_link(&disk_to_dev(disk)->kobj, "bdi");
> +	return retval;
>  
> -	disk_add_events(disk);
>  }
>  EXPORT_SYMBOL(add_disk);
>  
> @@ -1779,17 +1811,17 @@ module_param_cb(events_dfl_poll_msecs, &disk_events_dfl_poll_msecs_param_ops,
>  /*
>   * disk_{alloc|add|del|release}_events - initialize and destroy disk_events.
>   */
> -static void disk_alloc_events(struct gendisk *disk)
> +static int disk_alloc_events(struct gendisk *disk)
>  {
>  	struct disk_events *ev;
>  
>  	if (!disk->fops->check_events)
> -		return;
> +		return 0;
>  
>  	ev = kzalloc(sizeof(*ev), GFP_KERNEL);
>  	if (!ev) {
>  		pr_warn("%s: failed to initialize events\n", disk->disk_name);
> -		return;
> +		return -ENOMEM;
>  	}
>  
>  	INIT_LIST_HEAD(&ev->node);
> @@ -1801,17 +1833,22 @@ static void disk_alloc_events(struct gendisk *disk)
>  	INIT_DELAYED_WORK(&ev->dwork, disk_events_workfn);
>  
>  	disk->ev = ev;
> +	return 0;
>  }
>  
> -static void disk_add_events(struct gendisk *disk)
> +static int disk_add_events(struct gendisk *disk)
>  {
> +	int ret;
> +
>  	if (!disk->ev)
> -		return;
> +		return 0;
>  
> -	/* FIXME: error handling */
> -	if (sysfs_create_files(&disk_to_dev(disk)->kobj, disk_events_attrs) < 0)
> +	ret = sysfs_create_files(&disk_to_dev(disk)->kobj, disk_events_attrs);
> +	if (ret) {
>  		pr_warn("%s: failed to create sysfs files for events\n",
>  			disk->disk_name);
> +		return ret;
> +	}
>  
>  	mutex_lock(&disk_events_mutex);
>  	list_add_tail(&disk->ev->node, &disk_events);
> @@ -1822,6 +1859,7 @@ static void disk_add_events(struct gendisk *disk)
>  	 * unblock kicks it into action.
>  	 */
>  	__disk_unblock_events(disk, true);
> +	return ret;
>  }
>  
>  static void disk_del_events(struct gendisk *disk)
> diff --git a/include/linux/genhd.h b/include/linux/genhd.h
> index ec274e0..9bcd710 100644
> --- a/include/linux/genhd.h
> +++ b/include/linux/genhd.h
> @@ -416,7 +416,7 @@ static inline void free_part_info(struct hd_struct *part)
>  extern void part_round_stats(int cpu, struct hd_struct *part);
>  
>  /* block/genhd.c */
> -extern void add_disk(struct gendisk *disk);
> +extern int add_disk(struct gendisk *disk);
>  extern void del_gendisk(struct gendisk *gp);
>  extern struct gendisk *get_gendisk(dev_t dev, int *partno);
>  extern struct block_device *bdget_disk(struct gendisk *disk, int partno);
> @@ -619,7 +619,7 @@ extern struct gendisk *alloc_disk_node(int minors, int node_id);
>  extern struct gendisk *alloc_disk(int minors);
>  extern struct kobject *get_disk(struct gendisk *disk);
>  extern void put_disk(struct gendisk *disk);
> -extern void blk_register_region(dev_t devt, unsigned long range,
> +extern int blk_register_region(dev_t devt, unsigned long range,
>  			struct module *module,
>  			struct kobject *(*probe)(dev_t, int *, void *),
>  			int (*lock)(dev_t, void *),