[PATCH] mm/backing-dev: Check return value of the debugfs_create_dir()

[PATCH] mm/backing-dev: Check return value of the debugfs_create_dir()

[Alexander Kuleshov]

The debugfs_create_dir() function may fail and return error. If the
root directory not created, we can't create anything inside it. This
patch adds check for this case.

Signed-off-by: Alexander Kuleshov <kuleshovmail@gmail.com>
---
 mm/backing-dev.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/mm/backing-dev.c b/mm/backing-dev.c
index dac5bf5..518d26a 100644
--- a/mm/backing-dev.c
+++ b/mm/backing-dev.c
@@ -117,15 +117,21 @@ static const struct file_operations bdi_debug_stats_fops = {
 
 static void bdi_debug_register(struct backing_dev_info *bdi, const char *name)
 {
-	bdi->debug_dir = debugfs_create_dir(name, bdi_debug_root);
-	bdi->debug_stats = debugfs_create_file("stats", 0444, bdi->debug_dir,
-					       bdi, &bdi_debug_stats_fops);
+	if (bdi_debug_root) {
+		bdi->debug_dir = debugfs_create_dir(name, bdi_debug_root);
+		if (bdi->debug_dir)
+			bdi->debug_stats = debugfs_create_file("stats", 0444,
+							bdi->debug_dir, bdi,
+							&bdi_debug_stats_fops);
+	}
 }
 
 static void bdi_debug_unregister(struct backing_dev_info *bdi)
 {
-	debugfs_remove(bdi->debug_stats);
-	debugfs_remove(bdi->debug_dir);
+	if (bdi_debug_root) {
+		debugfs_remove(bdi->debug_stats);
+		debugfs_remove(bdi->debug_dir);
+	}
 }
 #else
 static inline void bdi_debug_init(void)
-- 
2.5.0

Re: [PATCH] mm/backing-dev: Check return value of the debugfs_create_dir()

[Jan Kara]

On Tue 25-08-15 13:54:23, Alexander Kuleshov wrote:
> The debugfs_create_dir() function may fail and return error. If the
> root directory not created, we can't create anything inside it. This
> patch adds check for this case.
> 
> Signed-off-by: Alexander Kuleshov <kuleshovmail@gmail.com>

The patch looks good to me. You can add:

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

								Honza
> ---
>  mm/backing-dev.c | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/mm/backing-dev.c b/mm/backing-dev.c
> index dac5bf5..518d26a 100644
> --- a/mm/backing-dev.c
> +++ b/mm/backing-dev.c
> @@ -117,15 +117,21 @@ static const struct file_operations bdi_debug_stats_fops = {
>  
>  static void bdi_debug_register(struct backing_dev_info *bdi, const char *name)
>  {
> -	bdi->debug_dir = debugfs_create_dir(name, bdi_debug_root);
> -	bdi->debug_stats = debugfs_create_file("stats", 0444, bdi->debug_dir,
> -					       bdi, &bdi_debug_stats_fops);
> +	if (bdi_debug_root) {
> +		bdi->debug_dir = debugfs_create_dir(name, bdi_debug_root);
> +		if (bdi->debug_dir)
> +			bdi->debug_stats = debugfs_create_file("stats", 0444,
> +							bdi->debug_dir, bdi,
> +							&bdi_debug_stats_fops);
> +	}
>  }
>  
>  static void bdi_debug_unregister(struct backing_dev_info *bdi)
>  {
> -	debugfs_remove(bdi->debug_stats);
> -	debugfs_remove(bdi->debug_dir);
> +	if (bdi_debug_root) {
> +		debugfs_remove(bdi->debug_stats);
> +		debugfs_remove(bdi->debug_dir);
> +	}
>  }
>  #else
>  static inline void bdi_debug_init(void)
> -- 
> 2.5.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH] mm/backing-dev: Check return value of the debugfs_create_dir()

[Andrew Morton]

On Tue, 25 Aug 2015 13:54:23 +0600 Alexander Kuleshov <kuleshovmail@gmail.com> wrote:

> The debugfs_create_dir() function may fail and return error. If the
> root directory not created, we can't create anything inside it. This
> patch adds check for this case.
> 
> ...
>
> --- a/mm/backing-dev.c
> +++ b/mm/backing-dev.c
> @@ -117,15 +117,21 @@ static const struct file_operations bdi_debug_stats_fops = {
>  
>  static void bdi_debug_register(struct backing_dev_info *bdi, const char *name)
>  {
> -	bdi->debug_dir = debugfs_create_dir(name, bdi_debug_root);
> -	bdi->debug_stats = debugfs_create_file("stats", 0444, bdi->debug_dir,
> -					       bdi, &bdi_debug_stats_fops);
> +	if (bdi_debug_root) {
> +		bdi->debug_dir = debugfs_create_dir(name, bdi_debug_root);
> +		if (bdi->debug_dir)
> +			bdi->debug_stats = debugfs_create_file("stats", 0444,
> +							bdi->debug_dir, bdi,
> +							&bdi_debug_stats_fops);
> +	}

If debugfs_create_dir() fails, debugfs_create_file() will go ahead and
attempt to create the debugfs file in the debugfs root directory:

: static struct dentry *start_creating(const char *name, struct dentry *parent)
: {
: ...
: 	/* If the parent is not specified, we create it in the root.
: 	 * We need the root dentry to do this, which is in the super
: 	 * block. A pointer to that is in the struct vfsmount that we
: 	 * have around.
: 	 */
: 	if (!parent)
: 		parent = debugfs_mount->mnt_root;

I'm not sure that this is very useful behaviour, and putting the files
in the wrong place is a very obscure way of informing the user that
debugfs_create_dir() failed :(


I don't think it's worth making little changes such as this - handling
debugfs failures needs a deeper rethink.

Re: [PATCH] mm/backing-dev: Check return value of the debugfs_create_dir()

[Jan Kara]

On Tue 25-08-15 14:08:58, Andrew Morton wrote:
> On Tue, 25 Aug 2015 13:54:23 +0600 Alexander Kuleshov <kuleshovmail@gmail.com> wrote:
> 
> > The debugfs_create_dir() function may fail and return error. If the
> > root directory not created, we can't create anything inside it. This
> > patch adds check for this case.
> > 
> > ...
> >
> > --- a/mm/backing-dev.c
> > +++ b/mm/backing-dev.c
> > @@ -117,15 +117,21 @@ static const struct file_operations bdi_debug_stats_fops = {
> >  
> >  static void bdi_debug_register(struct backing_dev_info *bdi, const char *name)
> >  {
> > -	bdi->debug_dir = debugfs_create_dir(name, bdi_debug_root);
> > -	bdi->debug_stats = debugfs_create_file("stats", 0444, bdi->debug_dir,
> > -					       bdi, &bdi_debug_stats_fops);
> > +	if (bdi_debug_root) {
> > +		bdi->debug_dir = debugfs_create_dir(name, bdi_debug_root);
> > +		if (bdi->debug_dir)
> > +			bdi->debug_stats = debugfs_create_file("stats", 0444,
> > +							bdi->debug_dir, bdi,
> > +							&bdi_debug_stats_fops);
> > +	}
> 
> If debugfs_create_dir() fails, debugfs_create_file() will go ahead and
> attempt to create the debugfs file in the debugfs root directory:
> 
> : static struct dentry *start_creating(const char *name, struct dentry *parent)
> : {
> : ...
> : 	/* If the parent is not specified, we create it in the root.
> : 	 * We need the root dentry to do this, which is in the super
> : 	 * block. A pointer to that is in the struct vfsmount that we
> : 	 * have around.
> : 	 */
> : 	if (!parent)
> : 		parent = debugfs_mount->mnt_root;
> 
> I'm not sure that this is very useful behaviour, and putting the files
> in the wrong place is a very obscure way of informing the user that
> debugfs_create_dir() failed :(

But this patch actually makes sure that we don't call debugfs_create_dir()
and debugfs_create_file() with parent == NULL so this patch avoids creation
of entries in debugfs root. So IMHO it really improves the situation. And I
agree with you that falling back to debugfs root is just broken...

> I don't think it's worth making little changes such as this - handling
> debugfs failures needs a deeper rethink.

Well, handling debugfs failures like in this patch is the right way to go,
isn't it? Or what else would you imagine than checking for errors and
bailing out instead of trying to create entries in non-existent dirs?

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

Re: [PATCH] mm/backing-dev: Check return value of the debugfs_create_dir()

[Alexander Kuleshov]

Hello Jan,

2015-08-26 15:23 GMT+06:00 Jan Kara <jack@suse.cz>:
> Well, handling debugfs failures like in this patch is the right way to go,
> isn't it? Or what else would you imagine than checking for errors and
> bailing out instead of trying to create entries in non-existent dirs?

I think Andrew talks about this thread https://lkml.org/lkml/2015/8/14/555