* [PATCH] block: add_disk: fix error handling
@ 2010-12-05 15:15 Richard Weinberger
2010-12-05 21:17 ` Neil Brown
0 siblings, 1 reply; 3+ messages in thread
From: Richard Weinberger @ 2010-12-05 15:15 UTC (permalink / raw)
To: axboe
Cc: paulmck, shemminger, isimatu.yasuaki, neilb, linux-kernel,
Richard Weinberger
This patch brings better error handling to the
add_disk function.
Now it reports success or failure.
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);
--
1.6.6.1
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH] block: add_disk: fix error handling
2010-12-05 15:15 [PATCH] block: add_disk: fix error handling Richard Weinberger
@ 2010-12-05 21:17 ` Neil Brown
2010-12-05 21:51 ` Richard Weinberger
0 siblings, 1 reply; 3+ messages in thread
From: Neil Brown @ 2010-12-05 21:17 UTC (permalink / raw)
To: Richard Weinberger
Cc: axboe, paulmck, shemminger, isimatu.yasuaki, linux-kernel
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.
Could you summaries the real-life situations in which add_disk can fail?
And what the consequences of failure are?
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);
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH] block: add_disk: fix error handling
2010-12-05 21:17 ` Neil Brown
@ 2010-12-05 21:51 ` Richard Weinberger
0 siblings, 0 replies; 3+ messages in thread
From: Richard Weinberger @ 2010-12-05 21:51 UTC (permalink / raw)
To: Neil Brown; +Cc: axboe, paulmck, shemminger, isimatu.yasuaki, linux-kernel
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);
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2010-12-05 21:52 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-12-05 15:15 [PATCH] block: add_disk: fix error handling Richard Weinberger
2010-12-05 21:17 ` Neil Brown
2010-12-05 21:51 ` Richard Weinberger
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.