Re: [PATCH] block: add_disk: fix error handling

[Richard Weinberger <richard@nod.at>]

Am Sonntag 05 Dezember 2010, 22:17:52 schrieb Neil Brown:
> On Sun,  5 Dec 2010 16:15:27 +0100 Richard Weinberger <richard@nod.at> wrote:
> > This patch brings better error handling to the
> > add_disk function.
> > Now it reports success or failure.
> 
> So does that mean that all callers of add_disk should now check the return
> status and possible error-out ???  Sounds a bit painful.

Currently add_disk() is used 77 times in the kernel.
If this patch goes into mainline I'll happily provide a patch for every call.
When this is done we can also remove most the WARN_ON()s within add_disk().

> Could you summaries the real-life situations in which add_disk can fail?
> And what the consequences of failure are?

blk_alloc_devt() can fail i case of no memory.
bdi_register_dev() too.

In the current implementation of add_disk() the return values of bdi_register_dev()
and blk_register_queue() are ignored which isn't very nice.

Thanks,
//richard

> Thanks,
> NeilBrown
> 
> > Signed-off-by: Richard Weinberger <richard@nod.at>
> > ---
> > 
> >  block/genhd.c         |   34 +++++++++++++++++++++++++++-------
> >  include/linux/genhd.h |    2 +-
> >  2 files changed, 28 insertions(+), 8 deletions(-)
> > 
> > diff --git a/block/genhd.c b/block/genhd.c
> > index 5fa2b44..075e630 100644
> > --- a/block/genhd.c
> > +++ b/block/genhd.c
> > @@ -508,10 +508,8 @@ static int exact_lock(dev_t devt, void *data)
> > 
> >   *
> >   * 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;
> > 
> > @@ -529,7 +527,7 @@ void add_disk(struct gendisk *disk)
> > 
> >  	retval = blk_alloc_devt(&disk->part0, &devt);
> >  	if (retval) {
> >  	
> >  		WARN_ON(1);
> > 
> > -		return;
> > +		goto out;
> > 
> >  	}
> >  	disk_to_dev(disk)->devt = devt;
> > 
> > @@ -541,16 +539,38 @@ void add_disk(struct gendisk *disk)
> > 
> >  	/* 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) {
> > +		WARN_ON(1);
> > +		goto out_free_devt;
> > +	}
> > 
> >  	blk_register_region(disk_devt(disk), disk->minors, NULL,
> >  	
> >  			    exact_match, exact_lock, disk);
> >  	
> >  	register_disk(disk);
> > 
> > -	blk_register_queue(disk);
> > +	retval = blk_register_queue(disk);
> > +	if (retval) {
> > +		WARN_ON(1);
> > +		goto out_unregister_bdi;
> > +	}
> > 
> >  	retval = sysfs_create_link(&disk_to_dev(disk)->kobj, &bdi->dev->kobj,
> >  	
> >  				   "bdi");
> > 
> > -	WARN_ON(retval);
> > +	if (retval) {
> > +		WARN_ON(1);
> > +		goto out_unregister_queue;
> > +	}
> > +
> > +	return 0;
> > +
> > +out_unregister_queue:
> > +	blk_unregister_queue(disk);
> > +out_unregister_bdi:
> > +	bdi_unregister(bdi);
> > +out_free_devt:
> > +	blk_free_devt(devt);
> > +out:
> > +	return -1;
> > 
> >  }
> >  
> >  EXPORT_SYMBOL(add_disk);
> > 
> > diff --git a/include/linux/genhd.h b/include/linux/genhd.h
> > index 7a7b9c1..4e7ab8e 100644
> > --- a/include/linux/genhd.h
> > +++ b/include/linux/genhd.h
> > @@ -393,7 +393,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 void unlink_gendisk(struct gendisk *gp);
> >  extern struct gendisk *get_gendisk(dev_t dev, int *partno);