* [PATCH] jffs2: reduce stack usage in jffs2_build_xattr_subsystem()
@ 2023-05-06 4:56 Christian Marangi
2023-05-06 13:11 ` Zhihao Cheng
2023-05-15 10:46 ` Christian Brauner
0 siblings, 2 replies; 3+ messages in thread
From: Christian Marangi @ 2023-05-06 4:56 UTC (permalink / raw)
To: David Woodhouse, Richard Weinberger, Fabian Frederick,
Nick Desaulniers, Christian Marangi, Christian Brauner,
KaiGai Kohei, linux-mtd, linux-kernel
Cc: Tim Gardner, kernel test robot, Ron Economos, Nathan Chancellor, stable
From: Fabian Frederick <fabf@skynet.be>
Use kcalloc() for allocation/flush of 128 pointers table to
reduce stack usage.
Function now returns -ENOMEM or 0 on success.
stackusage
Before:
./fs/jffs2/xattr.c:775 jffs2_build_xattr_subsystem 1208
dynamic,bounded
After:
./fs/jffs2/xattr.c:775 jffs2_build_xattr_subsystem 192
dynamic,bounded
Also update definition when CONFIG_JFFS2_FS_XATTR is not enabled
Tested with an MTD mount point and some user set/getfattr.
Many current target on OpenWRT also suffer from a compilation warning
(that become an error with CONFIG_WERROR) with the following output:
fs/jffs2/xattr.c: In function 'jffs2_build_xattr_subsystem':
fs/jffs2/xattr.c:887:1: error: the frame size of 1088 bytes is larger than 1024 bytes [-Werror=frame-larger-than=]
887 | }
| ^
Using dynamic allocation fix this compilation warning.
Fixes: c9f700f840bd ("[JFFS2][XATTR] using 'delete marker' for xdatum/xref deletion")
Reported-by: Tim Gardner <tim.gardner@canonical.com>
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Ron Economos <re@w6rz.net>
Reported-by: Nathan Chancellor <nathan@kernel.org>
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
Signed-off-by: Fabian Frederick <fabf@skynet.be>
Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
Cc: stable@vger.kernel.org
---
fs/jffs2/build.c | 5 ++++-
fs/jffs2/xattr.c | 13 +++++++++----
fs/jffs2/xattr.h | 4 ++--
3 files changed, 15 insertions(+), 7 deletions(-)
diff --git a/fs/jffs2/build.c b/fs/jffs2/build.c
index 837cd55fd4c5..6ae9d6fefb86 100644
--- a/fs/jffs2/build.c
+++ b/fs/jffs2/build.c
@@ -211,7 +211,10 @@ static int jffs2_build_filesystem(struct jffs2_sb_info *c)
ic->scan_dents = NULL;
cond_resched();
}
- jffs2_build_xattr_subsystem(c);
+ ret = jffs2_build_xattr_subsystem(c);
+ if (ret)
+ goto exit;
+
c->flags &= ~JFFS2_SB_FLAG_BUILDING;
dbg_fsbuild("FS build complete\n");
diff --git a/fs/jffs2/xattr.c b/fs/jffs2/xattr.c
index aa4048a27f31..3b6bdc9a49e1 100644
--- a/fs/jffs2/xattr.c
+++ b/fs/jffs2/xattr.c
@@ -772,10 +772,10 @@ void jffs2_clear_xattr_subsystem(struct jffs2_sb_info *c)
}
#define XREF_TMPHASH_SIZE (128)
-void jffs2_build_xattr_subsystem(struct jffs2_sb_info *c)
+int jffs2_build_xattr_subsystem(struct jffs2_sb_info *c)
{
struct jffs2_xattr_ref *ref, *_ref;
- struct jffs2_xattr_ref *xref_tmphash[XREF_TMPHASH_SIZE];
+ struct jffs2_xattr_ref **xref_tmphash;
struct jffs2_xattr_datum *xd, *_xd;
struct jffs2_inode_cache *ic;
struct jffs2_raw_node_ref *raw;
@@ -784,9 +784,12 @@ void jffs2_build_xattr_subsystem(struct jffs2_sb_info *c)
BUG_ON(!(c->flags & JFFS2_SB_FLAG_BUILDING));
+ xref_tmphash = kcalloc(XREF_TMPHASH_SIZE,
+ sizeof(struct jffs2_xattr_ref *), GFP_KERNEL);
+ if (!xref_tmphash)
+ return -ENOMEM;
+
/* Phase.1 : Merge same xref */
- for (i=0; i < XREF_TMPHASH_SIZE; i++)
- xref_tmphash[i] = NULL;
for (ref=c->xref_temp; ref; ref=_ref) {
struct jffs2_xattr_ref *tmp;
@@ -884,6 +887,8 @@ void jffs2_build_xattr_subsystem(struct jffs2_sb_info *c)
"%u of xref (%u dead, %u orphan) found.\n",
xdatum_count, xdatum_unchecked_count, xdatum_orphan_count,
xref_count, xref_dead_count, xref_orphan_count);
+ kfree(xref_tmphash);
+ return 0;
}
struct jffs2_xattr_datum *jffs2_setup_xattr_datum(struct jffs2_sb_info *c,
diff --git a/fs/jffs2/xattr.h b/fs/jffs2/xattr.h
index 720007b2fd65..1b5030a3349d 100644
--- a/fs/jffs2/xattr.h
+++ b/fs/jffs2/xattr.h
@@ -71,7 +71,7 @@ static inline int is_xattr_ref_dead(struct jffs2_xattr_ref *ref)
#ifdef CONFIG_JFFS2_FS_XATTR
extern void jffs2_init_xattr_subsystem(struct jffs2_sb_info *c);
-extern void jffs2_build_xattr_subsystem(struct jffs2_sb_info *c);
+extern int jffs2_build_xattr_subsystem(struct jffs2_sb_info *c);
extern void jffs2_clear_xattr_subsystem(struct jffs2_sb_info *c);
extern struct jffs2_xattr_datum *jffs2_setup_xattr_datum(struct jffs2_sb_info *c,
@@ -103,7 +103,7 @@ extern ssize_t jffs2_listxattr(struct dentry *, char *, size_t);
#else
#define jffs2_init_xattr_subsystem(c)
-#define jffs2_build_xattr_subsystem(c)
+#define jffs2_build_xattr_subsystem(c) (0)
#define jffs2_clear_xattr_subsystem(c)
#define jffs2_xattr_do_crccheck_inode(c, ic)
--
2.39.2
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH] jffs2: reduce stack usage in jffs2_build_xattr_subsystem()
2023-05-06 4:56 [PATCH] jffs2: reduce stack usage in jffs2_build_xattr_subsystem() Christian Marangi
@ 2023-05-06 13:11 ` Zhihao Cheng
2023-05-15 10:46 ` Christian Brauner
1 sibling, 0 replies; 3+ messages in thread
From: Zhihao Cheng @ 2023-05-06 13:11 UTC (permalink / raw)
To: Christian Marangi, David Woodhouse, Richard Weinberger,
Fabian Frederick, Nick Desaulniers, Christian Brauner,
KaiGai Kohei, linux-mtd, linux-kernel
Cc: Tim Gardner, kernel test robot, Ron Economos, Nathan Chancellor, stable
在 2023/5/6 12:56, Christian Marangi 写道:
> From: Fabian Frederick <fabf@skynet.be>
>
> Use kcalloc() for allocation/flush of 128 pointers table to
> reduce stack usage.
>
> Function now returns -ENOMEM or 0 on success.
>
> stackusage
> Before:
> ./fs/jffs2/xattr.c:775 jffs2_build_xattr_subsystem 1208
> dynamic,bounded
>
> After:
> ./fs/jffs2/xattr.c:775 jffs2_build_xattr_subsystem 192
> dynamic,bounded
>
> Also update definition when CONFIG_JFFS2_FS_XATTR is not enabled
>
> Tested with an MTD mount point and some user set/getfattr.
>
> Many current target on OpenWRT also suffer from a compilation warning
> (that become an error with CONFIG_WERROR) with the following output:
>
> fs/jffs2/xattr.c: In function 'jffs2_build_xattr_subsystem':
> fs/jffs2/xattr.c:887:1: error: the frame size of 1088 bytes is larger than 1024 bytes [-Werror=frame-larger-than=]
> 887 | }
> | ^
>
> Using dynamic allocation fix this compilation warning.
>
> Fixes: c9f700f840bd ("[JFFS2][XATTR] using 'delete marker' for xdatum/xref deletion")
> Reported-by: Tim Gardner <tim.gardner@canonical.com>
> Reported-by: kernel test robot <lkp@intel.com>
> Reported-by: Ron Economos <re@w6rz.net>
> Reported-by: Nathan Chancellor <nathan@kernel.org>
> Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
> Signed-off-by: Fabian Frederick <fabf@skynet.be>
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> Cc: stable@vger.kernel.org
> ---
> fs/jffs2/build.c | 5 ++++-
> fs/jffs2/xattr.c | 13 +++++++++----
> fs/jffs2/xattr.h | 4 ++--
> 3 files changed, 15 insertions(+), 7 deletions(-)
>
> diff --git a/fs/jffs2/build.c b/fs/jffs2/build.c
> index 837cd55fd4c5..6ae9d6fefb86 100644
> --- a/fs/jffs2/build.c
> +++ b/fs/jffs2/build.c
> @@ -211,7 +211,10 @@ static int jffs2_build_filesystem(struct jffs2_sb_info *c)
> ic->scan_dents = NULL;
> cond_resched();
> }
> - jffs2_build_xattr_subsystem(c);
> + ret = jffs2_build_xattr_subsystem(c);
> + if (ret)
> + goto exit;
> +
> c->flags &= ~JFFS2_SB_FLAG_BUILDING;
>
> dbg_fsbuild("FS build complete\n");
> diff --git a/fs/jffs2/xattr.c b/fs/jffs2/xattr.c
> index aa4048a27f31..3b6bdc9a49e1 100644
> --- a/fs/jffs2/xattr.c
> +++ b/fs/jffs2/xattr.c
> @@ -772,10 +772,10 @@ void jffs2_clear_xattr_subsystem(struct jffs2_sb_info *c)
> }
>
> #define XREF_TMPHASH_SIZE (128)
> -void jffs2_build_xattr_subsystem(struct jffs2_sb_info *c)
> +int jffs2_build_xattr_subsystem(struct jffs2_sb_info *c)
> {
> struct jffs2_xattr_ref *ref, *_ref;
> - struct jffs2_xattr_ref *xref_tmphash[XREF_TMPHASH_SIZE];
> + struct jffs2_xattr_ref **xref_tmphash;
> struct jffs2_xattr_datum *xd, *_xd;
> struct jffs2_inode_cache *ic;
> struct jffs2_raw_node_ref *raw;
> @@ -784,9 +784,12 @@ void jffs2_build_xattr_subsystem(struct jffs2_sb_info *c)
>
> BUG_ON(!(c->flags & JFFS2_SB_FLAG_BUILDING));
>
> + xref_tmphash = kcalloc(XREF_TMPHASH_SIZE,
> + sizeof(struct jffs2_xattr_ref *), GFP_KERNEL);
> + if (!xref_tmphash)
> + return -ENOMEM;
> +
I have made some fault injection tests, jffs2 works fine, this patch
imports no memleak problems. It seems okay.
> /* Phase.1 : Merge same xref */
> - for (i=0; i < XREF_TMPHASH_SIZE; i++)
> - xref_tmphash[i] = NULL;
> for (ref=c->xref_temp; ref; ref=_ref) {
> struct jffs2_xattr_ref *tmp;
>
> @@ -884,6 +887,8 @@ void jffs2_build_xattr_subsystem(struct jffs2_sb_info *c)
> "%u of xref (%u dead, %u orphan) found.\n",
> xdatum_count, xdatum_unchecked_count, xdatum_orphan_count,
> xref_count, xref_dead_count, xref_orphan_count);
> + kfree(xref_tmphash);
> + return 0;
> }
>
> struct jffs2_xattr_datum *jffs2_setup_xattr_datum(struct jffs2_sb_info *c,
> diff --git a/fs/jffs2/xattr.h b/fs/jffs2/xattr.h
> index 720007b2fd65..1b5030a3349d 100644
> --- a/fs/jffs2/xattr.h
> +++ b/fs/jffs2/xattr.h
> @@ -71,7 +71,7 @@ static inline int is_xattr_ref_dead(struct jffs2_xattr_ref *ref)
> #ifdef CONFIG_JFFS2_FS_XATTR
>
> extern void jffs2_init_xattr_subsystem(struct jffs2_sb_info *c);
> -extern void jffs2_build_xattr_subsystem(struct jffs2_sb_info *c);
> +extern int jffs2_build_xattr_subsystem(struct jffs2_sb_info *c);
> extern void jffs2_clear_xattr_subsystem(struct jffs2_sb_info *c);
>
> extern struct jffs2_xattr_datum *jffs2_setup_xattr_datum(struct jffs2_sb_info *c,
> @@ -103,7 +103,7 @@ extern ssize_t jffs2_listxattr(struct dentry *, char *, size_t);
> #else
>
> #define jffs2_init_xattr_subsystem(c)
> -#define jffs2_build_xattr_subsystem(c)
> +#define jffs2_build_xattr_subsystem(c) (0)
> #define jffs2_clear_xattr_subsystem(c)
>
> #define jffs2_xattr_do_crccheck_inode(c, ic)
>
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH] jffs2: reduce stack usage in jffs2_build_xattr_subsystem()
2023-05-06 4:56 [PATCH] jffs2: reduce stack usage in jffs2_build_xattr_subsystem() Christian Marangi
2023-05-06 13:11 ` Zhihao Cheng
@ 2023-05-15 10:46 ` Christian Brauner
1 sibling, 0 replies; 3+ messages in thread
From: Christian Brauner @ 2023-05-15 10:46 UTC (permalink / raw)
To: David Woodhouse, Richard Weinberger, Fabian Frederick,
Nick Desaulniers, KaiGai Kohei, linux-mtd, linux-kernel,
Christian Marangi
Cc: Christian Brauner, Tim Gardner, kernel test robot, Ron Economos,
Nathan Chancellor, stable
On Sat, 06 May 2023 06:56:12 +0200, Christian Marangi wrote:
> Use kcalloc() for allocation/flush of 128 pointers table to
> reduce stack usage.
>
> Function now returns -ENOMEM or 0 on success.
>
> stackusage
> Before:
> ./fs/jffs2/xattr.c:775 jffs2_build_xattr_subsystem 1208
> dynamic,bounded
>
> [...]
I assume I was Cced to pick this up. I'm happy to do that. If this is
rather supposed to go through the jffs2 tree then please tell me so we
can drop it.
Applied to the vfs.misc branch of the vfs/vfs.git tree.
Patches in the vfs.misc branch should appear in linux-next soon.
Please report any outstanding bugs that were missed during review in a
new review to the original patch series allowing us to drop it.
tree: https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs.misc
[1/1] jffs2: reduce stack usage in jffs2_build_xattr_subsystem()
https://git.kernel.org/vfs/vfs/c/493e7cebb906
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2023-05-15 10:49 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-05-06 4:56 [PATCH] jffs2: reduce stack usage in jffs2_build_xattr_subsystem() Christian Marangi
2023-05-06 13:11 ` Zhihao Cheng
2023-05-15 10:46 ` Christian Brauner
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).