[PATCH v2 0/2] fs: jffs2: remove slab cache substitute with malloc

[PATCH v2 0/2] fs: jffs2: remove slab cache substitute with malloc

[Holger Assmann]

This series aimes to handle a bug in the Barebox jffs2 driver where the
system crashes once a second unmounting is performed when more than one
jffs2 file system were mounted at the same time at one point.

- The first patch "introduce reference counting at probe" solves the
  initial issue.

- The second patch "remove unnecessary slab cache structure" performs a
  cleanup on the respective code with the opportunity for simplification
  being taken.

Signed-off-by: Holger Assmann <h.assmann@pengutronix.de>

---

This series is a v2 of a former single commit now split up into two for
clarity reasons.

changes v1 -> v2:

   - split up v1 patch into two distinct patches
   - reworded commit messages

   by Ahmad Fatoum <a.fatoum@pengutronix.de>:
   - bugfix: perform jffs2_fill_super() with every probe() again
   - bugfix: removed unfunctional if-branch in probe()
   - use sizeof(*ret) instead of hardcoding the size in malloc calls

   by Sascha Hauer <s.hauer@pengutronix.de>:
   - additional infornation for commit messages added

---

 fs/jffs2/fs.c       |  32 +++++------
 fs/jffs2/malloc.c   | 131 +++++++-------------------------------------
 fs/jffs2/nodelist.h |   2 -
 3 files changed, 35 insertions(+), 130 deletions(-)

-- 
2.30.2


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH v2 1/2] fs: jffs2: introduce reference counting at probe

[Holger Assmann]

The Barebox jffs2 driver initialises global slab caches and compressors
within the probing stage [1]. In Barebox, jffs2_create_slab_caches() has
several calls to kmem_cache_create() which does nothing more than
allocating the context data structure for the kmem_cache.

Probing a second jffs2 however will overwrite the original pointers
returned by kmem_cache_create(), leading to a double free when more than
one jffs2 file system gets unmounted and jffs2_destroy_slab_caches() is
called. The same issue exists regarding jffs2_compressors_init().

We can fix this bug by introducing reference counting for both the slab
caches and the compressors so that the global data structures are kept
as long as at least one file system is present.

[1] jffs2_compressors_init(), jffs2_create_slab_caches() in probe()

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
Signed-off-by: Holger Assmann <h.assmann@pengutronix.de>
---
 fs/jffs2/fs.c | 34 ++++++++++++++++++++++------------
 1 file changed, 22 insertions(+), 12 deletions(-)

diff --git a/fs/jffs2/fs.c b/fs/jffs2/fs.c
index c1d04c397d..7538252336 100644
--- a/fs/jffs2/fs.c
+++ b/fs/jffs2/fs.c
@@ -386,6 +386,8 @@ void jffs2_flash_cleanup(struct jffs2_sb_info *c) {
 	}
 }
 
+static int jffs2_probe_cnt;
+
 static int jffs2_probe(struct device_d *dev)
 {
 	struct fs_device_d *fsdev;
@@ -408,17 +410,19 @@ static int jffs2_probe(struct device_d *dev)
 
 	sb->s_fs_info = ctx;
 
-        ret = jffs2_compressors_init();
-        if (ret) {
-		pr_err("error: Failed to initialise compressors\n");
-		goto err_out;
-        }
+	if (!jffs2_probe_cnt) {
+		ret = jffs2_compressors_init();
+		if (ret) {
+			pr_err("error: Failed to initialise compressors\n");
+			goto err_out;
+		}
 
-        ret = jffs2_create_slab_caches();
-        if (ret) {
-		pr_err("error: Failed to initialise slab caches\n");
-		goto err_compressors;
-        }
+		ret = jffs2_create_slab_caches();
+		if (ret) {
+			pr_err("error: Failed to initialise slab caches\n");
+			goto err_compressors;
+		}
+	}
 
         if (jffs2_fill_super(fsdev, 0)) {
 		dev_err(dev, "no valid jffs2 found\n");
@@ -426,6 +430,8 @@ static int jffs2_probe(struct device_d *dev)
 		goto err_slab;
 	}
 
+	jffs2_probe_cnt++;
+
 	return 0;
 
 err_slab:
@@ -445,8 +451,12 @@ static void jffs2_remove(struct device_d *dev)
 	fsdev = dev_to_fs_device(dev);
 	sb = &fsdev->sb;
 
-	jffs2_destroy_slab_caches();
-	jffs2_compressors_exit();
+	jffs2_probe_cnt--;
+
+	if (!jffs2_probe_cnt) {
+		jffs2_destroy_slab_caches();
+		jffs2_compressors_exit();
+	}
 
 	jffs2_put_super(sb);
 }
-- 
2.30.2


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH v2 2/2] fs: jffs2: remove unnecessary slab cache structure

[Holger Assmann]

jffs2_create_slab_caches() and its subsequent kmem_cache calls are not
needed in Barebox since they can directly be replaced by malloc calls.

This patch performs that replacement as well as the related clean up.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
Signed-off-by: Holger Assmann <h.assmann@pengutronix.de>
---
 fs/jffs2/fs.c       |  10 ----
 fs/jffs2/malloc.c   | 131 +++++++-------------------------------------
 fs/jffs2/nodelist.h |   2 -
 3 files changed, 19 insertions(+), 124 deletions(-)

diff --git a/fs/jffs2/fs.c b/fs/jffs2/fs.c
index 7538252336..b72721cfc4 100644
--- a/fs/jffs2/fs.c
+++ b/fs/jffs2/fs.c
@@ -416,26 +416,17 @@ static int jffs2_probe(struct device_d *dev)
 			pr_err("error: Failed to initialise compressors\n");
 			goto err_out;
 		}
-
-		ret = jffs2_create_slab_caches();
-		if (ret) {
-			pr_err("error: Failed to initialise slab caches\n");
-			goto err_compressors;
-		}
 	}
 
         if (jffs2_fill_super(fsdev, 0)) {
 		dev_err(dev, "no valid jffs2 found\n");
 		ret = -EINVAL;
-		goto err_slab;
 	}
 
 	jffs2_probe_cnt++;
 
 	return 0;
 
-err_slab:
-        jffs2_destroy_slab_caches();
 err_compressors:
 	jffs2_compressors_exit();
 err_out:
@@ -454,7 +445,6 @@ static void jffs2_remove(struct device_d *dev)
 	jffs2_probe_cnt--;
 
 	if (!jffs2_probe_cnt) {
-		jffs2_destroy_slab_caches();
 		jffs2_compressors_exit();
 	}
 
diff --git a/fs/jffs2/malloc.c b/fs/jffs2/malloc.c
index b7afc68cea..7e3e0797b3 100644
--- a/fs/jffs2/malloc.c
+++ b/fs/jffs2/malloc.c
@@ -17,103 +17,10 @@
 #include <linux/jffs2.h>
 #include "nodelist.h"
 
-/* These are initialised to NULL in the kernel startup code.
-   If you're porting to other operating systems, beware */
-static struct kmem_cache *full_dnode_slab;
-static struct kmem_cache *raw_dirent_slab;
-static struct kmem_cache *raw_inode_slab;
-static struct kmem_cache *tmp_dnode_info_slab;
-static struct kmem_cache *raw_node_ref_slab;
-static struct kmem_cache *node_frag_slab;
-static struct kmem_cache *inode_cache_slab;
-#ifdef CONFIG_JFFS2_FS_XATTR
-static struct kmem_cache *xattr_datum_cache;
-static struct kmem_cache *xattr_ref_cache;
-#endif
-
-int __init jffs2_create_slab_caches(void)
-{
-	full_dnode_slab = kmem_cache_create("jffs2_full_dnode",
-					    sizeof(struct jffs2_full_dnode),
-					    0, 0, NULL);
-	if (!full_dnode_slab)
-		goto err;
-
-	raw_dirent_slab = kmem_cache_create("jffs2_raw_dirent",
-					    sizeof(struct jffs2_raw_dirent),
-					    0, SLAB_HWCACHE_ALIGN, NULL);
-	if (!raw_dirent_slab)
-		goto err;
-
-	raw_inode_slab = kmem_cache_create("jffs2_raw_inode",
-					   sizeof(struct jffs2_raw_inode),
-					   0, SLAB_HWCACHE_ALIGN, NULL);
-	if (!raw_inode_slab)
-		goto err;
-
-	tmp_dnode_info_slab = kmem_cache_create("jffs2_tmp_dnode",
-						sizeof(struct jffs2_tmp_dnode_info),
-						0, 0, NULL);
-	if (!tmp_dnode_info_slab)
-		goto err;
-
-	raw_node_ref_slab = kmem_cache_create("jffs2_refblock",
-					      sizeof(struct jffs2_raw_node_ref) * (REFS_PER_BLOCK + 1),
-					      0, 0, NULL);
-	if (!raw_node_ref_slab)
-		goto err;
-
-	node_frag_slab = kmem_cache_create("jffs2_node_frag",
-					   sizeof(struct jffs2_node_frag),
-					   0, 0, NULL);
-	if (!node_frag_slab)
-		goto err;
-
-	inode_cache_slab = kmem_cache_create("jffs2_inode_cache",
-					     sizeof(struct jffs2_inode_cache),
-					     0, 0, NULL);
-	if (!inode_cache_slab)
-		goto err;
-
-#ifdef CONFIG_JFFS2_FS_XATTR
-	xattr_datum_cache = kmem_cache_create("jffs2_xattr_datum",
-					     sizeof(struct jffs2_xattr_datum),
-					     0, 0, NULL);
-	if (!xattr_datum_cache)
-		goto err;
-
-	xattr_ref_cache = kmem_cache_create("jffs2_xattr_ref",
-					   sizeof(struct jffs2_xattr_ref),
-					   0, 0, NULL);
-	if (!xattr_ref_cache)
-		goto err;
-#endif
-
-	return 0;
- err:
-	jffs2_destroy_slab_caches();
-	return -ENOMEM;
-}
-
-void jffs2_destroy_slab_caches(void)
-{
-	kmem_cache_destroy(full_dnode_slab);
-	kmem_cache_destroy(raw_dirent_slab);
-	kmem_cache_destroy(raw_inode_slab);
-	kmem_cache_destroy(tmp_dnode_info_slab);
-	kmem_cache_destroy(raw_node_ref_slab);
-	kmem_cache_destroy(node_frag_slab);
-	kmem_cache_destroy(inode_cache_slab);
-#ifdef CONFIG_JFFS2_FS_XATTR
-	kmem_cache_destroy(xattr_datum_cache);
-	kmem_cache_destroy(xattr_ref_cache);
-#endif
-}
-
 struct jffs2_full_dirent *jffs2_alloc_full_dirent(int namesize)
 {
 	struct jffs2_full_dirent *ret;
-	ret = kmalloc(sizeof(struct jffs2_full_dirent) + namesize, GFP_KERNEL);
+	ret = kmalloc(sizeof(*ret) + namesize, GFP_KERNEL);
 	dbg_memalloc("%p\n", ret);
 	return ret;
 }
@@ -127,7 +34,7 @@ void jffs2_free_full_dirent(struct jffs2_full_dirent *x)
 struct jffs2_full_dnode *jffs2_alloc_full_dnode(void)
 {
 	struct jffs2_full_dnode *ret;
-	ret = kmem_cache_alloc(full_dnode_slab, GFP_KERNEL);
+	ret = malloc(sizeof(*ret));
 	dbg_memalloc("%p\n", ret);
 	return ret;
 }
@@ -135,13 +42,13 @@ struct jffs2_full_dnode *jffs2_alloc_full_dnode(void)
 void jffs2_free_full_dnode(struct jffs2_full_dnode *x)
 {
 	dbg_memalloc("%p\n", x);
-	kmem_cache_free(full_dnode_slab, x);
+	free(x);
 }
 
 struct jffs2_raw_dirent *jffs2_alloc_raw_dirent(void)
 {
 	struct jffs2_raw_dirent *ret;
-	ret = kmem_cache_alloc(raw_dirent_slab, GFP_KERNEL);
+	ret = malloc(sizeof(*ret));
 	dbg_memalloc("%p\n", ret);
 	return ret;
 }
@@ -149,13 +56,13 @@ struct jffs2_raw_dirent *jffs2_alloc_raw_dirent(void)
 void jffs2_free_raw_dirent(struct jffs2_raw_dirent *x)
 {
 	dbg_memalloc("%p\n", x);
-	kmem_cache_free(raw_dirent_slab, x);
+	free(x);
 }
 
 struct jffs2_raw_inode *jffs2_alloc_raw_inode(void)
 {
 	struct jffs2_raw_inode *ret;
-	ret = kmem_cache_alloc(raw_inode_slab, GFP_KERNEL);
+	ret = malloc(sizeof(*ret));
 	dbg_memalloc("%p\n", ret);
 	return ret;
 }
@@ -163,13 +70,13 @@ struct jffs2_raw_inode *jffs2_alloc_raw_inode(void)
 void jffs2_free_raw_inode(struct jffs2_raw_inode *x)
 {
 	dbg_memalloc("%p\n", x);
-	kmem_cache_free(raw_inode_slab, x);
+	free(x);
 }
 
 struct jffs2_tmp_dnode_info *jffs2_alloc_tmp_dnode_info(void)
 {
 	struct jffs2_tmp_dnode_info *ret;
-	ret = kmem_cache_alloc(tmp_dnode_info_slab, GFP_KERNEL);
+	ret = malloc(sizeof(*ret));
 	dbg_memalloc("%p\n",
 		ret);
 	return ret;
@@ -178,14 +85,14 @@ struct jffs2_tmp_dnode_info *jffs2_alloc_tmp_dnode_info(void)
 void jffs2_free_tmp_dnode_info(struct jffs2_tmp_dnode_info *x)
 {
 	dbg_memalloc("%p\n", x);
-	kmem_cache_free(tmp_dnode_info_slab, x);
+	free(x);
 }
 
 static struct jffs2_raw_node_ref *jffs2_alloc_refblock(void)
 {
 	struct jffs2_raw_node_ref *ret;
 
-	ret = kmem_cache_alloc(raw_node_ref_slab, GFP_KERNEL);
+	ret = malloc(sizeof(*ret) * (REFS_PER_BLOCK + 1));
 	if (ret) {
 		int i = 0;
 		for (i=0; i < REFS_PER_BLOCK; i++) {
@@ -242,13 +149,13 @@ int jffs2_prealloc_raw_node_refs(struct jffs2_sb_info *c,
 void jffs2_free_refblock(struct jffs2_raw_node_ref *x)
 {
 	dbg_memalloc("%p\n", x);
-	kmem_cache_free(raw_node_ref_slab, x);
+	free(x);
 }
 
 struct jffs2_node_frag *jffs2_alloc_node_frag(void)
 {
 	struct jffs2_node_frag *ret;
-	ret = kmem_cache_alloc(node_frag_slab, GFP_KERNEL);
+	ret = malloc(sizeof(*ret));
 	dbg_memalloc("%p\n", ret);
 	return ret;
 }
@@ -256,13 +163,13 @@ struct jffs2_node_frag *jffs2_alloc_node_frag(void)
 void jffs2_free_node_frag(struct jffs2_node_frag *x)
 {
 	dbg_memalloc("%p\n", x);
-	kmem_cache_free(node_frag_slab, x);
+	free(x);
 }
 
 struct jffs2_inode_cache *jffs2_alloc_inode_cache(void)
 {
 	struct jffs2_inode_cache *ret;
-	ret = kmem_cache_alloc(inode_cache_slab, GFP_KERNEL);
+	ret = malloc(sizeof(*ret));
 	dbg_memalloc("%p\n", ret);
 	return ret;
 }
@@ -270,14 +177,14 @@ struct jffs2_inode_cache *jffs2_alloc_inode_cache(void)
 void jffs2_free_inode_cache(struct jffs2_inode_cache *x)
 {
 	dbg_memalloc("%p\n", x);
-	kmem_cache_free(inode_cache_slab, x);
+	free(x);
 }
 
 #ifdef CONFIG_JFFS2_FS_XATTR
 struct jffs2_xattr_datum *jffs2_alloc_xattr_datum(void)
 {
 	struct jffs2_xattr_datum *xd;
-	xd = kmem_cache_zalloc(xattr_datum_cache, GFP_KERNEL);
+	xd = malloc(sizeof(*ret));
 	dbg_memalloc("%p\n", xd);
 	if (!xd)
 		return NULL;
@@ -291,13 +198,13 @@ struct jffs2_xattr_datum *jffs2_alloc_xattr_datum(void)
 void jffs2_free_xattr_datum(struct jffs2_xattr_datum *xd)
 {
 	dbg_memalloc("%p\n", xd);
-	kmem_cache_free(xattr_datum_cache, xd);
+	free(xd);
 }
 
 struct jffs2_xattr_ref *jffs2_alloc_xattr_ref(void)
 {
 	struct jffs2_xattr_ref *ref;
-	ref = kmem_cache_zalloc(xattr_ref_cache, GFP_KERNEL);
+	ref = malloc(sizeof(*ret));
 	dbg_memalloc("%p\n", ref);
 	if (!ref)
 		return NULL;
@@ -310,6 +217,6 @@ struct jffs2_xattr_ref *jffs2_alloc_xattr_ref(void)
 void jffs2_free_xattr_ref(struct jffs2_xattr_ref *ref)
 {
 	dbg_memalloc("%p\n", ref);
-	kmem_cache_free(xattr_ref_cache, ref);
+	free(ref);
 }
 #endif
diff --git a/fs/jffs2/nodelist.h b/fs/jffs2/nodelist.h
index 20deb639f6..7ea18cd2fc 100644
--- a/fs/jffs2/nodelist.h
+++ b/fs/jffs2/nodelist.h
@@ -441,8 +441,6 @@ int jffs2_do_crccheck_inode(struct jffs2_sb_info *c, struct jffs2_inode_cache *i
 void jffs2_do_clear_inode(struct jffs2_sb_info *c, struct jffs2_inode_info *f);
 
 /* malloc.c */
-int jffs2_create_slab_caches(void);
-void jffs2_destroy_slab_caches(void);
 
 struct jffs2_full_dirent *jffs2_alloc_full_dirent(int namesize);
 void jffs2_free_full_dirent(struct jffs2_full_dirent *);
-- 
2.30.2


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v2 2/2] fs: jffs2: remove unnecessary slab cache structure

[Ahmad Fatoum]

Hi,

On 29.11.21 13:45, Holger Assmann wrote:
> jffs2_create_slab_caches() and its subsequent kmem_cache calls are not
> needed in Barebox since they can directly be replaced by malloc calls.
> 
> This patch performs that replacement as well as the related clean up.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> Signed-off-by: Holger Assmann <h.assmann@pengutronix.de>
> ---
>  fs/jffs2/fs.c       |  10 ----
>  fs/jffs2/malloc.c   | 131 +++++++-------------------------------------
>  fs/jffs2/nodelist.h |   2 -
>  3 files changed, 19 insertions(+), 124 deletions(-)
> 
> diff --git a/fs/jffs2/fs.c b/fs/jffs2/fs.c
> index 7538252336..b72721cfc4 100644
> --- a/fs/jffs2/fs.c
> +++ b/fs/jffs2/fs.c
> @@ -416,26 +416,17 @@ static int jffs2_probe(struct device_d *dev)
>  			pr_err("error: Failed to initialise compressors\n");
>  			goto err_out;
>  		}
> -
> -		ret = jffs2_create_slab_caches();
> -		if (ret) {
> -			pr_err("error: Failed to initialise slab caches\n");
> -			goto err_compressors;
> -		}
>  	}
>  
>          if (jffs2_fill_super(fsdev, 0)) {
>  		dev_err(dev, "no valid jffs2 found\n");
>  		ret = -EINVAL;
> -		goto err_slab;

This should be goto err_compressors. ret = -EINVAL is't
used in the success path, so the probe will succeed despite the error
and likely triggering a crash on mount. In that case,
jffs2_compressors_exit is also never called.

@Sascha, can you fixup?

>  	}
>  
>  	jffs2_probe_cnt++;
>  
>  	return 0;
>  
> -err_slab:
> -        jffs2_destroy_slab_caches();
>  err_compressors:
>  	jffs2_compressors_exit();
>  err_out:
> @@ -454,7 +445,6 @@ static void jffs2_remove(struct device_d *dev)
>  	jffs2_probe_cnt--;
>  
>  	if (!jffs2_probe_cnt) {
> -		jffs2_destroy_slab_caches();
>  		jffs2_compressors_exit();
>  	}
>  
> diff --git a/fs/jffs2/malloc.c b/fs/jffs2/malloc.c
> index b7afc68cea..7e3e0797b3 100644
> --- a/fs/jffs2/malloc.c
> +++ b/fs/jffs2/malloc.c
> @@ -17,103 +17,10 @@
>  #include <linux/jffs2.h>
>  #include "nodelist.h"
>  
> -/* These are initialised to NULL in the kernel startup code.
> -   If you're porting to other operating systems, beware */
> -static struct kmem_cache *full_dnode_slab;
> -static struct kmem_cache *raw_dirent_slab;
> -static struct kmem_cache *raw_inode_slab;
> -static struct kmem_cache *tmp_dnode_info_slab;
> -static struct kmem_cache *raw_node_ref_slab;
> -static struct kmem_cache *node_frag_slab;
> -static struct kmem_cache *inode_cache_slab;
> -#ifdef CONFIG_JFFS2_FS_XATTR
> -static struct kmem_cache *xattr_datum_cache;
> -static struct kmem_cache *xattr_ref_cache;
> -#endif
> -
> -int __init jffs2_create_slab_caches(void)
> -{
> -	full_dnode_slab = kmem_cache_create("jffs2_full_dnode",
> -					    sizeof(struct jffs2_full_dnode),
> -					    0, 0, NULL);
> -	if (!full_dnode_slab)
> -		goto err;
> -
> -	raw_dirent_slab = kmem_cache_create("jffs2_raw_dirent",
> -					    sizeof(struct jffs2_raw_dirent),
> -					    0, SLAB_HWCACHE_ALIGN, NULL);
> -	if (!raw_dirent_slab)
> -		goto err;
> -
> -	raw_inode_slab = kmem_cache_create("jffs2_raw_inode",
> -					   sizeof(struct jffs2_raw_inode),
> -					   0, SLAB_HWCACHE_ALIGN, NULL);
> -	if (!raw_inode_slab)
> -		goto err;
> -
> -	tmp_dnode_info_slab = kmem_cache_create("jffs2_tmp_dnode",
> -						sizeof(struct jffs2_tmp_dnode_info),
> -						0, 0, NULL);
> -	if (!tmp_dnode_info_slab)
> -		goto err;
> -
> -	raw_node_ref_slab = kmem_cache_create("jffs2_refblock",
> -					      sizeof(struct jffs2_raw_node_ref) * (REFS_PER_BLOCK + 1),
> -					      0, 0, NULL);
> -	if (!raw_node_ref_slab)
> -		goto err;
> -
> -	node_frag_slab = kmem_cache_create("jffs2_node_frag",
> -					   sizeof(struct jffs2_node_frag),
> -					   0, 0, NULL);
> -	if (!node_frag_slab)
> -		goto err;
> -
> -	inode_cache_slab = kmem_cache_create("jffs2_inode_cache",
> -					     sizeof(struct jffs2_inode_cache),
> -					     0, 0, NULL);
> -	if (!inode_cache_slab)
> -		goto err;
> -
> -#ifdef CONFIG_JFFS2_FS_XATTR
> -	xattr_datum_cache = kmem_cache_create("jffs2_xattr_datum",
> -					     sizeof(struct jffs2_xattr_datum),
> -					     0, 0, NULL);
> -	if (!xattr_datum_cache)
> -		goto err;
> -
> -	xattr_ref_cache = kmem_cache_create("jffs2_xattr_ref",
> -					   sizeof(struct jffs2_xattr_ref),
> -					   0, 0, NULL);
> -	if (!xattr_ref_cache)
> -		goto err;
> -#endif
> -
> -	return 0;
> - err:
> -	jffs2_destroy_slab_caches();
> -	return -ENOMEM;
> -}
> -
> -void jffs2_destroy_slab_caches(void)
> -{
> -	kmem_cache_destroy(full_dnode_slab);
> -	kmem_cache_destroy(raw_dirent_slab);
> -	kmem_cache_destroy(raw_inode_slab);
> -	kmem_cache_destroy(tmp_dnode_info_slab);
> -	kmem_cache_destroy(raw_node_ref_slab);
> -	kmem_cache_destroy(node_frag_slab);
> -	kmem_cache_destroy(inode_cache_slab);
> -#ifdef CONFIG_JFFS2_FS_XATTR
> -	kmem_cache_destroy(xattr_datum_cache);
> -	kmem_cache_destroy(xattr_ref_cache);
> -#endif
> -}
> -
>  struct jffs2_full_dirent *jffs2_alloc_full_dirent(int namesize)
>  {
>  	struct jffs2_full_dirent *ret;
> -	ret = kmalloc(sizeof(struct jffs2_full_dirent) + namesize, GFP_KERNEL);
> +	ret = kmalloc(sizeof(*ret) + namesize, GFP_KERNEL);
>  	dbg_memalloc("%p\n", ret);
>  	return ret;
>  }
> @@ -127,7 +34,7 @@ void jffs2_free_full_dirent(struct jffs2_full_dirent *x)
>  struct jffs2_full_dnode *jffs2_alloc_full_dnode(void)
>  {
>  	struct jffs2_full_dnode *ret;
> -	ret = kmem_cache_alloc(full_dnode_slab, GFP_KERNEL);
> +	ret = malloc(sizeof(*ret));
>  	dbg_memalloc("%p\n", ret);
>  	return ret;
>  }
> @@ -135,13 +42,13 @@ struct jffs2_full_dnode *jffs2_alloc_full_dnode(void)
>  void jffs2_free_full_dnode(struct jffs2_full_dnode *x)
>  {
>  	dbg_memalloc("%p\n", x);
> -	kmem_cache_free(full_dnode_slab, x);
> +	free(x);
>  }
>  
>  struct jffs2_raw_dirent *jffs2_alloc_raw_dirent(void)
>  {
>  	struct jffs2_raw_dirent *ret;
> -	ret = kmem_cache_alloc(raw_dirent_slab, GFP_KERNEL);
> +	ret = malloc(sizeof(*ret));
>  	dbg_memalloc("%p\n", ret);
>  	return ret;
>  }
> @@ -149,13 +56,13 @@ struct jffs2_raw_dirent *jffs2_alloc_raw_dirent(void)
>  void jffs2_free_raw_dirent(struct jffs2_raw_dirent *x)
>  {
>  	dbg_memalloc("%p\n", x);
> -	kmem_cache_free(raw_dirent_slab, x);
> +	free(x);
>  }
>  
>  struct jffs2_raw_inode *jffs2_alloc_raw_inode(void)
>  {
>  	struct jffs2_raw_inode *ret;
> -	ret = kmem_cache_alloc(raw_inode_slab, GFP_KERNEL);
> +	ret = malloc(sizeof(*ret));
>  	dbg_memalloc("%p\n", ret);
>  	return ret;
>  }
> @@ -163,13 +70,13 @@ struct jffs2_raw_inode *jffs2_alloc_raw_inode(void)
>  void jffs2_free_raw_inode(struct jffs2_raw_inode *x)
>  {
>  	dbg_memalloc("%p\n", x);
> -	kmem_cache_free(raw_inode_slab, x);
> +	free(x);
>  }
>  
>  struct jffs2_tmp_dnode_info *jffs2_alloc_tmp_dnode_info(void)
>  {
>  	struct jffs2_tmp_dnode_info *ret;
> -	ret = kmem_cache_alloc(tmp_dnode_info_slab, GFP_KERNEL);
> +	ret = malloc(sizeof(*ret));
>  	dbg_memalloc("%p\n",
>  		ret);
>  	return ret;
> @@ -178,14 +85,14 @@ struct jffs2_tmp_dnode_info *jffs2_alloc_tmp_dnode_info(void)
>  void jffs2_free_tmp_dnode_info(struct jffs2_tmp_dnode_info *x)
>  {
>  	dbg_memalloc("%p\n", x);
> -	kmem_cache_free(tmp_dnode_info_slab, x);
> +	free(x);
>  }
>  
>  static struct jffs2_raw_node_ref *jffs2_alloc_refblock(void)
>  {
>  	struct jffs2_raw_node_ref *ret;
>  
> -	ret = kmem_cache_alloc(raw_node_ref_slab, GFP_KERNEL);
> +	ret = malloc(sizeof(*ret) * (REFS_PER_BLOCK + 1));
>  	if (ret) {
>  		int i = 0;
>  		for (i=0; i < REFS_PER_BLOCK; i++) {
> @@ -242,13 +149,13 @@ int jffs2_prealloc_raw_node_refs(struct jffs2_sb_info *c,
>  void jffs2_free_refblock(struct jffs2_raw_node_ref *x)
>  {
>  	dbg_memalloc("%p\n", x);
> -	kmem_cache_free(raw_node_ref_slab, x);
> +	free(x);
>  }
>  
>  struct jffs2_node_frag *jffs2_alloc_node_frag(void)
>  {
>  	struct jffs2_node_frag *ret;
> -	ret = kmem_cache_alloc(node_frag_slab, GFP_KERNEL);
> +	ret = malloc(sizeof(*ret));
>  	dbg_memalloc("%p\n", ret);
>  	return ret;
>  }
> @@ -256,13 +163,13 @@ struct jffs2_node_frag *jffs2_alloc_node_frag(void)
>  void jffs2_free_node_frag(struct jffs2_node_frag *x)
>  {
>  	dbg_memalloc("%p\n", x);
> -	kmem_cache_free(node_frag_slab, x);
> +	free(x);
>  }
>  
>  struct jffs2_inode_cache *jffs2_alloc_inode_cache(void)
>  {
>  	struct jffs2_inode_cache *ret;
> -	ret = kmem_cache_alloc(inode_cache_slab, GFP_KERNEL);
> +	ret = malloc(sizeof(*ret));
>  	dbg_memalloc("%p\n", ret);
>  	return ret;
>  }
> @@ -270,14 +177,14 @@ struct jffs2_inode_cache *jffs2_alloc_inode_cache(void)
>  void jffs2_free_inode_cache(struct jffs2_inode_cache *x)
>  {
>  	dbg_memalloc("%p\n", x);
> -	kmem_cache_free(inode_cache_slab, x);
> +	free(x);
>  }
>  
>  #ifdef CONFIG_JFFS2_FS_XATTR
>  struct jffs2_xattr_datum *jffs2_alloc_xattr_datum(void)
>  {
>  	struct jffs2_xattr_datum *xd;
> -	xd = kmem_cache_zalloc(xattr_datum_cache, GFP_KERNEL);
> +	xd = malloc(sizeof(*ret));
>  	dbg_memalloc("%p\n", xd);
>  	if (!xd)
>  		return NULL;
> @@ -291,13 +198,13 @@ struct jffs2_xattr_datum *jffs2_alloc_xattr_datum(void)
>  void jffs2_free_xattr_datum(struct jffs2_xattr_datum *xd)
>  {
>  	dbg_memalloc("%p\n", xd);
> -	kmem_cache_free(xattr_datum_cache, xd);
> +	free(xd);
>  }
>  
>  struct jffs2_xattr_ref *jffs2_alloc_xattr_ref(void)
>  {
>  	struct jffs2_xattr_ref *ref;
> -	ref = kmem_cache_zalloc(xattr_ref_cache, GFP_KERNEL);
> +	ref = malloc(sizeof(*ret));
>  	dbg_memalloc("%p\n", ref);
>  	if (!ref)
>  		return NULL;
> @@ -310,6 +217,6 @@ struct jffs2_xattr_ref *jffs2_alloc_xattr_ref(void)
>  void jffs2_free_xattr_ref(struct jffs2_xattr_ref *ref)
>  {
>  	dbg_memalloc("%p\n", ref);
> -	kmem_cache_free(xattr_ref_cache, ref);
> +	free(ref);
>  }
>  #endif
> diff --git a/fs/jffs2/nodelist.h b/fs/jffs2/nodelist.h
> index 20deb639f6..7ea18cd2fc 100644
> --- a/fs/jffs2/nodelist.h
> +++ b/fs/jffs2/nodelist.h
> @@ -441,8 +441,6 @@ int jffs2_do_crccheck_inode(struct jffs2_sb_info *c, struct jffs2_inode_cache *i
>  void jffs2_do_clear_inode(struct jffs2_sb_info *c, struct jffs2_inode_info *f);
>  
>  /* malloc.c */
> -int jffs2_create_slab_caches(void);
> -void jffs2_destroy_slab_caches(void);
>  
>  struct jffs2_full_dirent *jffs2_alloc_full_dirent(int namesize);
>  void jffs2_free_full_dirent(struct jffs2_full_dirent *);
> 


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v2 2/2] fs: jffs2: remove unnecessary slab cache structure

[Sascha Hauer]

On Tue, Nov 30, 2021 at 11:29:11AM +0100, Ahmad Fatoum wrote:
> Hi,
> 
> On 29.11.21 13:45, Holger Assmann wrote:
> > jffs2_create_slab_caches() and its subsequent kmem_cache calls are not
> > needed in Barebox since they can directly be replaced by malloc calls.
> > 
> > This patch performs that replacement as well as the related clean up.
> > 
> > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > Signed-off-by: Holger Assmann <h.assmann@pengutronix.de>
> > ---
> >  fs/jffs2/fs.c       |  10 ----
> >  fs/jffs2/malloc.c   | 131 +++++++-------------------------------------
> >  fs/jffs2/nodelist.h |   2 -
> >  3 files changed, 19 insertions(+), 124 deletions(-)
> > 
> > diff --git a/fs/jffs2/fs.c b/fs/jffs2/fs.c
> > index 7538252336..b72721cfc4 100644
> > --- a/fs/jffs2/fs.c
> > +++ b/fs/jffs2/fs.c
> > @@ -416,26 +416,17 @@ static int jffs2_probe(struct device_d *dev)
> >  			pr_err("error: Failed to initialise compressors\n");
> >  			goto err_out;
> >  		}
> > -
> > -		ret = jffs2_create_slab_caches();
> > -		if (ret) {
> > -			pr_err("error: Failed to initialise slab caches\n");
> > -			goto err_compressors;
> > -		}
> >  	}
> >  
> >          if (jffs2_fill_super(fsdev, 0)) {
> >  		dev_err(dev, "no valid jffs2 found\n");
> >  		ret = -EINVAL;
> > -		goto err_slab;
> 
> This should be goto err_compressors. ret = -EINVAL is't
> used in the success path, so the probe will succeed despite the error
> and likely triggering a crash on mount. In that case,
> jffs2_compressors_exit is also never called.
> 
> @Sascha, can you fixup?

Did that.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v2 0/2] fs: jffs2: remove slab cache substitute with malloc

[Sascha Hauer]

On Mon, Nov 29, 2021 at 01:45:43PM +0100, Holger Assmann wrote:
> This series aimes to handle a bug in the Barebox jffs2 driver where the
> system crashes once a second unmounting is performed when more than one
> jffs2 file system were mounted at the same time at one point.
> 
> - The first patch "introduce reference counting at probe" solves the
>   initial issue.
> 
> - The second patch "remove unnecessary slab cache structure" performs a
>   cleanup on the respective code with the opportunity for simplification
>   being taken.
> 
> Signed-off-by: Holger Assmann <h.assmann@pengutronix.de>
> 
> ---

Applied, thanks

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox