[PATCH 0/4] add error handle for bdi debugfs register

[PATCH 0/4] add error handle for bdi debugfs register

[weiping zhang]

this series add error handle for bdi debugfs register flow, the first
three patches try to convert void function to int and do some cleanup
if create dir or file fail.

the fourth patch only add a WARN_ON in device_add_disk, no function change.

weiping zhang (4):
  bdi: add check for bdi_debug_root
  bdi: convert bdi_debug_register to int
  bdi: add error handle for bdi_debug_register
  block: add WARN_ON if bdi register fail

 block/genhd.c    |  4 +++-
 mm/backing-dev.c | 41 +++++++++++++++++++++++++++++++++++------
 2 files changed, 38 insertions(+), 7 deletions(-)

-- 
2.14.2

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH 1/4] bdi: add check for bdi_debug_root

[weiping zhang]

this patch add a check for bdi_debug_root and do error handle for it.
we should make sure it was created success, otherwise when add new
block device's bdi folder(eg, 8:0) will be create a debugfs root directory.

Signed-off-by: weiping zhang <zhangweiping@didichuxing.com>
---
 mm/backing-dev.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/mm/backing-dev.c b/mm/backing-dev.c
index 74b52dfd5852..5072be19d9b2 100644
--- a/mm/backing-dev.c
+++ b/mm/backing-dev.c
@@ -36,9 +36,12 @@ struct workqueue_struct *bdi_wq;
 
 static struct dentry *bdi_debug_root;
 
-static void bdi_debug_init(void)
+static int bdi_debug_init(void)
 {
 	bdi_debug_root = debugfs_create_dir("bdi", NULL);
+	if (!bdi_debug_root)
+		return -ENOMEM;
+	return 0;
 }
 
 static int bdi_debug_stats_show(struct seq_file *m, void *v)
@@ -126,8 +129,9 @@ static void bdi_debug_unregister(struct backing_dev_info *bdi)
 	debugfs_remove(bdi->debug_dir);
 }
 #else
-static inline void bdi_debug_init(void)
+static inline int bdi_debug_init(void)
 {
+	return 0;
 }
 static inline void bdi_debug_register(struct backing_dev_info *bdi,
 				      const char *name)
@@ -229,12 +233,19 @@ ATTRIBUTE_GROUPS(bdi_dev);
 
 static __init int bdi_class_init(void)
 {
+	int ret;
+
 	bdi_class = class_create(THIS_MODULE, "bdi");
 	if (IS_ERR(bdi_class))
 		return PTR_ERR(bdi_class);
 
 	bdi_class->dev_groups = bdi_dev_groups;
-	bdi_debug_init();
+	ret = bdi_debug_init();
+	if (ret) {
+		class_destroy(bdi_class);
+		bdi_class = NULL;
+		return ret;
+	}
 
 	return 0;
 }
-- 
2.14.2

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 1/4] bdi: add check for bdi_debug_root

[Jan Kara]

On Fri 27-10-17 01:35:36, weiping zhang wrote:
> this patch add a check for bdi_debug_root and do error handle for it.
> we should make sure it was created success, otherwise when add new
> block device's bdi folder(eg, 8:0) will be create a debugfs root directory.
> 
> Signed-off-by: weiping zhang <zhangweiping@didichuxing.com>
> ---
>  mm/backing-dev.c | 17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)

These functions get called only on system boot - ENOMEM in those cases is
generally considered fatal and oopsing is acceptable result. So I don't
think this patch is needed.

								Honza

> 
> diff --git a/mm/backing-dev.c b/mm/backing-dev.c
> index 74b52dfd5852..5072be19d9b2 100644
> --- a/mm/backing-dev.c
> +++ b/mm/backing-dev.c
> @@ -36,9 +36,12 @@ struct workqueue_struct *bdi_wq;
>  
>  static struct dentry *bdi_debug_root;
>  
> -static void bdi_debug_init(void)
> +static int bdi_debug_init(void)
>  {
>  	bdi_debug_root = debugfs_create_dir("bdi", NULL);
> +	if (!bdi_debug_root)
> +		return -ENOMEM;
> +	return 0;
>  }
>  
>  static int bdi_debug_stats_show(struct seq_file *m, void *v)
> @@ -126,8 +129,9 @@ static void bdi_debug_unregister(struct backing_dev_info *bdi)
>  	debugfs_remove(bdi->debug_dir);
>  }
>  #else
> -static inline void bdi_debug_init(void)
> +static inline int bdi_debug_init(void)
>  {
> +	return 0;
>  }
>  static inline void bdi_debug_register(struct backing_dev_info *bdi,
>  				      const char *name)
> @@ -229,12 +233,19 @@ ATTRIBUTE_GROUPS(bdi_dev);
>  
>  static __init int bdi_class_init(void)
>  {
> +	int ret;
> +
>  	bdi_class = class_create(THIS_MODULE, "bdi");
>  	if (IS_ERR(bdi_class))
>  		return PTR_ERR(bdi_class);
>  
>  	bdi_class->dev_groups = bdi_dev_groups;
> -	bdi_debug_init();
> +	ret = bdi_debug_init();
> +	if (ret) {
> +		class_destroy(bdi_class);
> +		bdi_class = NULL;
> +		return ret;
> +	}
>  
>  	return 0;
>  }
> -- 
> 2.14.2
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 1/4] bdi: add check for bdi_debug_root

[weiping zhang]

On Mon, Oct 30, 2017 at 02:00:28PM +0100, Jan Kara wrote:
> On Fri 27-10-17 01:35:36, weiping zhang wrote:
> > this patch add a check for bdi_debug_root and do error handle for it.
> > we should make sure it was created success, otherwise when add new
> > block device's bdi folder(eg, 8:0) will be create a debugfs root directory.
> > 
> > Signed-off-by: weiping zhang <zhangweiping@didichuxing.com>
> > ---
> >  mm/backing-dev.c | 17 ++++++++++++++---
> >  1 file changed, 14 insertions(+), 3 deletions(-)
> 
> These functions get called only on system boot - ENOMEM in those cases is
> generally considered fatal and oopsing is acceptable result. So I don't
> think this patch is needed.
> 
OK, I drop this patch.

Thanks a ton.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH 2/4] bdi: convert bdi_debug_register to int

[weiping zhang]

Convert bdi_debug_register to int and then do error handle for it.

Signed-off-by: weiping zhang <zhangweiping@didichuxing.com>
---
 mm/backing-dev.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/mm/backing-dev.c b/mm/backing-dev.c
index 5072be19d9b2..e9d6a1ede12b 100644
--- a/mm/backing-dev.c
+++ b/mm/backing-dev.c
@@ -116,11 +116,23 @@ static const struct file_operations bdi_debug_stats_fops = {
 	.release	= single_release,
 };
 
-static void bdi_debug_register(struct backing_dev_info *bdi, const char *name)
+static int bdi_debug_register(struct backing_dev_info *bdi, const char *name)
 {
+	if (!bdi_debug_root)
+		return -ENOMEM;
+
 	bdi->debug_dir = debugfs_create_dir(name, bdi_debug_root);
+	if (!bdi->debug_dir)
+		return -ENOMEM;
+
 	bdi->debug_stats = debugfs_create_file("stats", 0444, bdi->debug_dir,
 					       bdi, &bdi_debug_stats_fops);
+	if (!bdi->debug_stats) {
+		debugfs_remove(bdi->debug_dir);
+		return -ENOMEM;
+	}
+
+	return 0;
 }
 
 static void bdi_debug_unregister(struct backing_dev_info *bdi)
@@ -133,9 +145,10 @@ static inline int bdi_debug_init(void)
 {
 	return 0;
 }
-static inline void bdi_debug_register(struct backing_dev_info *bdi,
+static inline int bdi_debug_register(struct backing_dev_info *bdi,
 				      const char *name)
 {
+	return 0;
 }
 static inline void bdi_debug_unregister(struct backing_dev_info *bdi)
 {
-- 
2.14.2

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 2/4] bdi: convert bdi_debug_register to int

[Jan Kara]

On Fri 27-10-17 01:35:57, weiping zhang wrote:
> Convert bdi_debug_register to int and then do error handle for it.
> 
> Signed-off-by: weiping zhang <zhangweiping@didichuxing.com>

This patch looks good to me. You can add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  mm/backing-dev.c | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/backing-dev.c b/mm/backing-dev.c
> index 5072be19d9b2..e9d6a1ede12b 100644
> --- a/mm/backing-dev.c
> +++ b/mm/backing-dev.c
> @@ -116,11 +116,23 @@ static const struct file_operations bdi_debug_stats_fops = {
>  	.release	= single_release,
>  };
>  
> -static void bdi_debug_register(struct backing_dev_info *bdi, const char *name)
> +static int bdi_debug_register(struct backing_dev_info *bdi, const char *name)
>  {
> +	if (!bdi_debug_root)
> +		return -ENOMEM;
> +
>  	bdi->debug_dir = debugfs_create_dir(name, bdi_debug_root);
> +	if (!bdi->debug_dir)
> +		return -ENOMEM;
> +
>  	bdi->debug_stats = debugfs_create_file("stats", 0444, bdi->debug_dir,
>  					       bdi, &bdi_debug_stats_fops);
> +	if (!bdi->debug_stats) {
> +		debugfs_remove(bdi->debug_dir);
> +		return -ENOMEM;
> +	}
> +
> +	return 0;
>  }
>  
>  static void bdi_debug_unregister(struct backing_dev_info *bdi)
> @@ -133,9 +145,10 @@ static inline int bdi_debug_init(void)
>  {
>  	return 0;
>  }
> -static inline void bdi_debug_register(struct backing_dev_info *bdi,
> +static inline int bdi_debug_register(struct backing_dev_info *bdi,
>  				      const char *name)
>  {
> +	return 0;
>  }
>  static inline void bdi_debug_unregister(struct backing_dev_info *bdi)
>  {
> -- 
> 2.14.2
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH 3/4] bdi: add error handle for bdi_debug_register

[weiping zhang]

In order to make error handle more cleaner we call bdi_debug_register
before set state to WB_registered, that we can avoid call bdi_unregister
in release_bdi().

Signed-off-by: weiping zhang <zhangweiping@didichuxing.com>
---
 mm/backing-dev.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/mm/backing-dev.c b/mm/backing-dev.c
index e9d6a1ede12b..54396d53f471 100644
--- a/mm/backing-dev.c
+++ b/mm/backing-dev.c
@@ -893,10 +893,13 @@ int bdi_register_va(struct backing_dev_info *bdi, const char *fmt, va_list args)
 	if (IS_ERR(dev))
 		return PTR_ERR(dev);
 
+	if (bdi_debug_register(bdi, dev_name(dev))) {
+		device_destroy(bdi_class, dev->devt);
+		return -ENOMEM;
+	}
 	cgwb_bdi_register(bdi);
 	bdi->dev = dev;
 
-	bdi_debug_register(bdi, dev_name(dev));
 	set_bit(WB_registered, &bdi->wb.state);
 
 	spin_lock_bh(&bdi_lock);
@@ -916,6 +919,8 @@ int bdi_register(struct backing_dev_info *bdi, const char *fmt, ...)
 	va_start(args, fmt);
 	ret = bdi_register_va(bdi, fmt, args);
 	va_end(args);
+	if (ret)
+		bdi_put(bdi);
 	return ret;
 }
 EXPORT_SYMBOL(bdi_register);
-- 
2.14.2

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 3/4] bdi: add error handle for bdi_debug_register

[Jan Kara]

On Fri 27-10-17 01:36:14, weiping zhang wrote:
> In order to make error handle more cleaner we call bdi_debug_register
> before set state to WB_registered, that we can avoid call bdi_unregister
> in release_bdi().
> 
> Signed-off-by: weiping zhang <zhangweiping@didichuxing.com>
> ---
>  mm/backing-dev.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/backing-dev.c b/mm/backing-dev.c
> index e9d6a1ede12b..54396d53f471 100644
> --- a/mm/backing-dev.c
> +++ b/mm/backing-dev.c
> @@ -893,10 +893,13 @@ int bdi_register_va(struct backing_dev_info *bdi, const char *fmt, va_list args)
>  	if (IS_ERR(dev))
>  		return PTR_ERR(dev);
>  
> +	if (bdi_debug_register(bdi, dev_name(dev))) {
> +		device_destroy(bdi_class, dev->devt);
> +		return -ENOMEM;
> +	}
>  	cgwb_bdi_register(bdi);
>  	bdi->dev = dev;
>  
> -	bdi_debug_register(bdi, dev_name(dev));
>  	set_bit(WB_registered, &bdi->wb.state);
>  
>  	spin_lock_bh(&bdi_lock);
> @@ -916,6 +919,8 @@ int bdi_register(struct backing_dev_info *bdi, const char *fmt, ...)
>  	va_start(args, fmt);
>  	ret = bdi_register_va(bdi, fmt, args);
>  	va_end(args);
> +	if (ret)
> +		bdi_put(bdi);

Why do you drop bdi reference here in case of error? We didn't do it
previously if bdi_register_va() failed for other reasons...

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 3/4] bdi: add error handle for bdi_debug_register

[weiping zhang]

On Mon, Oct 30, 2017 at 02:10:16PM +0100, Jan Kara wrote:
> On Fri 27-10-17 01:36:14, weiping zhang wrote:
> > In order to make error handle more cleaner we call bdi_debug_register
> > before set state to WB_registered, that we can avoid call bdi_unregister
> > in release_bdi().
> > 
> > Signed-off-by: weiping zhang <zhangweiping@didichuxing.com>
> > ---
> >  mm/backing-dev.c | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> > 
> > diff --git a/mm/backing-dev.c b/mm/backing-dev.c
> > index e9d6a1ede12b..54396d53f471 100644
> > --- a/mm/backing-dev.c
> > +++ b/mm/backing-dev.c
> > @@ -893,10 +893,13 @@ int bdi_register_va(struct backing_dev_info *bdi, const char *fmt, va_list args)
> >  	if (IS_ERR(dev))
> >  		return PTR_ERR(dev);
> >  
> > +	if (bdi_debug_register(bdi, dev_name(dev))) {
> > +		device_destroy(bdi_class, dev->devt);
> > +		return -ENOMEM;
> > +	}
> >  	cgwb_bdi_register(bdi);
> >  	bdi->dev = dev;
> >  
> > -	bdi_debug_register(bdi, dev_name(dev));
> >  	set_bit(WB_registered, &bdi->wb.state);
> >  
> >  	spin_lock_bh(&bdi_lock);
> > @@ -916,6 +919,8 @@ int bdi_register(struct backing_dev_info *bdi, const char *fmt, ...)
> >  	va_start(args, fmt);
> >  	ret = bdi_register_va(bdi, fmt, args);
> >  	va_end(args);
> > +	if (ret)
> > +		bdi_put(bdi);
> 
> Why do you drop bdi reference here in case of error? We didn't do it
> previously if bdi_register_va() failed for other reasons...
> 
At first I want add cleanup, because
device_add_disk->bdi_register_owner->bdi_register doen't do clanup. But
I notice that mtd_bdi_init also call bdi_register and do cleanup, so
this bdi_put() is wrong. I'll remove it at V2. Thanks a lot.

--
weiping

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH 4/4] block: add WARN_ON if bdi register fail

[weiping zhang]

device_add_disk need do more safety error handle, so this patch just
add WARN_ON.

Signed-off-by: weiping zhang <zhangweiping@didichuxing.com>
---
 block/genhd.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/block/genhd.c b/block/genhd.c
index dd305c65ffb0..cb55eea821eb 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -660,7 +660,9 @@ void device_add_disk(struct device *parent, struct gendisk *disk)
 
 	/* Register BDI before referencing it from bdev */
 	bdi = disk->queue->backing_dev_info;
-	bdi_register_owner(bdi, disk_to_dev(disk));
+	retval = bdi_register_owner(bdi, disk_to_dev(disk));
+	if (retval)
+		WARN_ON(1);
 
 	blk_register_region(disk_devt(disk), disk->minors, NULL,
 			    exact_match, exact_lock, disk);
-- 
2.14.2

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 4/4] block: add WARN_ON if bdi register fail

[Jan Kara]

On Fri 27-10-17 01:36:42, weiping zhang wrote:
> device_add_disk need do more safety error handle, so this patch just
> add WARN_ON.
> 
> Signed-off-by: weiping zhang <zhangweiping@didichuxing.com>
> ---
>  block/genhd.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/block/genhd.c b/block/genhd.c
> index dd305c65ffb0..cb55eea821eb 100644
> --- a/block/genhd.c
> +++ b/block/genhd.c
> @@ -660,7 +660,9 @@ void device_add_disk(struct device *parent, struct gendisk *disk)
>  
>  	/* Register BDI before referencing it from bdev */
>  	bdi = disk->queue->backing_dev_info;
> -	bdi_register_owner(bdi, disk_to_dev(disk));
> +	retval = bdi_register_owner(bdi, disk_to_dev(disk));
> +	if (retval)
> +		WARN_ON(1);

Just a nit: You can do

	WARN_ON(retval);

Otherwise you can add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 4/4] block: add WARN_ON if bdi register fail

[weiping zhang]

On Mon, Oct 30, 2017 at 02:14:30PM +0100, Jan Kara wrote:
> On Fri 27-10-17 01:36:42, weiping zhang wrote:
> > device_add_disk need do more safety error handle, so this patch just
> > add WARN_ON.
> > 
> > Signed-off-by: weiping zhang <zhangweiping@didichuxing.com>
> > ---
> >  block/genhd.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/block/genhd.c b/block/genhd.c
> > index dd305c65ffb0..cb55eea821eb 100644
> > --- a/block/genhd.c
> > +++ b/block/genhd.c
> > @@ -660,7 +660,9 @@ void device_add_disk(struct device *parent, struct gendisk *disk)
> >  
> >  	/* Register BDI before referencing it from bdev */
> >  	bdi = disk->queue->backing_dev_info;
> > -	bdi_register_owner(bdi, disk_to_dev(disk));
> > +	retval = bdi_register_owner(bdi, disk_to_dev(disk));
> > +	if (retval)
> > +		WARN_ON(1);
> 
> Just a nit: You can do
> 
> 	WARN_ON(retval);
> 
> Otherwise you can add:
> 
> Reviewed-by: Jan Kara <jack@suse.cz>
> 
more claner, I'll apply at V2, Thanks

--
weiping

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>