[PATCH 0/2] configfs: reduce memory consumption by symlinks

[PATCH 0/2] configfs: reduce memory consumption by symlinks

[Dmitry Bogdanov]

A SCSI target configuration of 2 ports, 650 backstores, 1200 ACLs consumes
170 GB RAM.
As it turned out that is because configfs allocates PAGE_SIZE for symlink
path. In PowerPC architecture a page is of 64 KB size and millions of
symlinks become hundreds of used GB.

This patch series reduses the usage of memory by symlinks in configfs to a
minimal possible amount - from of 64KB down to ~20 Bytes.


Dmitry Bogdanov (2):
  configfs: reduce memory consumption by symlinks
  configfs: make a minimal path of symlink

 fs/configfs/symlink.c | 75 ++++++++++++++++++++++++++-----------------
 1 file changed, 46 insertions(+), 29 deletions(-)

-- 
2.25.1

[PATCH 1/2] configfs: reduce memory consumption by symlinks

[Dmitry Bogdanov]

Instead of preallocating PAGE_SIZE for a symlink path, allocate the exact
size of that path.

Fixes: e9c03af21cc7 (configfs: calculate the symlink target only once)
Signed-off-by: Dmitry Bogdanov <d.bogdanov@yadro.com>

---
I treat this as bugfux due to reducing of enourmous memory consumption.
---
 fs/configfs/symlink.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/fs/configfs/symlink.c b/fs/configfs/symlink.c
index 0623c3edcfb9..224c9e4899d4 100644
--- a/fs/configfs/symlink.c
+++ b/fs/configfs/symlink.c
@@ -54,7 +54,7 @@ static void fill_item_path(struct config_item * item, char * buffer, int length)
 }
 
 static int configfs_get_target_path(struct config_item *item,
-		struct config_item *target, char *path)
+		struct config_item *target, char **path)
 {
 	int depth, size;
 	char *s;
@@ -66,11 +66,16 @@ static int configfs_get_target_path(struct config_item *item,
 
 	pr_debug("%s: depth = %d, size = %d\n", __func__, depth, size);
 
-	for (s = path; depth--; s += 3)
+	*path = kzalloc(size, GFP_KERNEL);
+	if (!*path)
+		return -ENOMEM;
+
+
+	for (s = *path; depth--; s += 3)
 		strcpy(s,"../");
 
-	fill_item_path(target, path, size);
-	pr_debug("%s: path = '%s'\n", __func__, path);
+	fill_item_path(target, *path, size);
+	pr_debug("%s: path = '%s'\n", __func__, *path);
 	return 0;
 }
 
@@ -79,27 +84,22 @@ static int create_link(struct config_item *parent_item,
 		       struct dentry *dentry)
 {
 	struct configfs_dirent *target_sd = item->ci_dentry->d_fsdata;
-	char *body;
+	char *body = NULL;
 	int ret;
 
 	if (!configfs_dirent_is_ready(target_sd))
 		return -ENOENT;
 
-	body = kzalloc(PAGE_SIZE, GFP_KERNEL);
-	if (!body)
-		return -ENOMEM;
-
 	configfs_get(target_sd);
 	spin_lock(&configfs_dirent_lock);
 	if (target_sd->s_type & CONFIGFS_USET_DROPPING) {
 		spin_unlock(&configfs_dirent_lock);
 		configfs_put(target_sd);
-		kfree(body);
 		return -ENOENT;
 	}
 	target_sd->s_links++;
 	spin_unlock(&configfs_dirent_lock);
-	ret = configfs_get_target_path(parent_item, item, body);
+	ret = configfs_get_target_path(parent_item, item, &body);
 	if (!ret)
 		ret = configfs_create_link(target_sd, parent_item->ci_dentry,
 					   dentry, body);
-- 
2.25.1

Re: [PATCH 1/2] configfs: reduce memory consumption by symlinks

[Joel Becker]

On Mon, Apr 01, 2024 at 11:26:54AM +0300, Dmitry Bogdanov wrote:
> Instead of preallocating PAGE_SIZE for a symlink path, allocate the exact
> size of that path.
> 
> Fixes: e9c03af21cc7 (configfs: calculate the symlink target only once)
> Signed-off-by: Dmitry Bogdanov <d.bogdanov@yadro.com>

Reviewed-by: Joel Becker <jlbec@evilplan.org>

> 
> ---
> I treat this as bugfux due to reducing of enourmous memory consumption.
> ---
>  fs/configfs/symlink.c | 22 +++++++++++-----------
>  1 file changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/fs/configfs/symlink.c b/fs/configfs/symlink.c
> index 0623c3edcfb9..224c9e4899d4 100644
> --- a/fs/configfs/symlink.c
> +++ b/fs/configfs/symlink.c
> @@ -54,7 +54,7 @@ static void fill_item_path(struct config_item * item, char * buffer, int length)
>  }
>  
>  static int configfs_get_target_path(struct config_item *item,
> -		struct config_item *target, char *path)
> +		struct config_item *target, char **path)
>  {
>  	int depth, size;
>  	char *s;
> @@ -66,11 +66,16 @@ static int configfs_get_target_path(struct config_item *item,
>  
>  	pr_debug("%s: depth = %d, size = %d\n", __func__, depth, size);
>  
> -	for (s = path; depth--; s += 3)
> +	*path = kzalloc(size, GFP_KERNEL);
> +	if (!*path)
> +		return -ENOMEM;
> +
> +
> +	for (s = *path; depth--; s += 3)
>  		strcpy(s,"../");
>  
> -	fill_item_path(target, path, size);
> -	pr_debug("%s: path = '%s'\n", __func__, path);
> +	fill_item_path(target, *path, size);
> +	pr_debug("%s: path = '%s'\n", __func__, *path);
>  	return 0;
>  }
>  
> @@ -79,27 +84,22 @@ static int create_link(struct config_item *parent_item,
>  		       struct dentry *dentry)
>  {
>  	struct configfs_dirent *target_sd = item->ci_dentry->d_fsdata;
> -	char *body;
> +	char *body = NULL;
>  	int ret;
>  
>  	if (!configfs_dirent_is_ready(target_sd))
>  		return -ENOENT;
>  
> -	body = kzalloc(PAGE_SIZE, GFP_KERNEL);
> -	if (!body)
> -		return -ENOMEM;
> -
>  	configfs_get(target_sd);
>  	spin_lock(&configfs_dirent_lock);
>  	if (target_sd->s_type & CONFIGFS_USET_DROPPING) {
>  		spin_unlock(&configfs_dirent_lock);
>  		configfs_put(target_sd);
> -		kfree(body);
>  		return -ENOENT;
>  	}
>  	target_sd->s_links++;
>  	spin_unlock(&configfs_dirent_lock);
> -	ret = configfs_get_target_path(parent_item, item, body);
> +	ret = configfs_get_target_path(parent_item, item, &body);
>  	if (!ret)
>  		ret = configfs_create_link(target_sd, parent_item->ci_dentry,
>  					   dentry, body);
> -- 
> 2.25.1
> 

-- 

"Can any of you seriously say the Bill of Rights could get through
 Congress today?  It wouldn't even get out of committee."
	- F. Lee Bailey

			http://www.jlbec.org/
			jlbec@evilplan.org

[PATCH 2/2] configfs: make a minimal path of symlink

[Dmitry Bogdanov]

Symlinks in configfs are used to be created from near places. Currently the
path is artificially inflated by multiple ../ to the configfs root an then
a full path of the target.

For scsi target subsystem the difference between such a path and a minimal
possible path is ~100 characters.

This patch makes a minimal relative path of symlink - from the closest
common parent.

Signed-off-by: Dmitry Bogdanov <d.bogdanov@yadro.com>
---
 fs/configfs/symlink.c | 59 ++++++++++++++++++++++++++++---------------
 1 file changed, 38 insertions(+), 21 deletions(-)

diff --git a/fs/configfs/symlink.c b/fs/configfs/symlink.c
index 224c9e4899d4..a61f5a4763e1 100644
--- a/fs/configfs/symlink.c
+++ b/fs/configfs/symlink.c
@@ -19,62 +19,79 @@
 /* Protects attachments of new symlinks */
 DEFINE_MUTEX(configfs_symlink_mutex);
 
-static int item_depth(struct config_item * item)
-{
-	struct config_item * p = item;
-	int depth = 0;
-	do { depth++; } while ((p = p->ci_parent) && !configfs_is_root(p));
-	return depth;
-}
-
-static int item_path_length(struct config_item * item)
+static int item_path_length(struct config_item *item, int depth)
 {
 	struct config_item * p = item;
 	int length = 1;
+
+	if (!depth)
+		return length;
+
 	do {
 		length += strlen(config_item_name(p)) + 1;
 		p = p->ci_parent;
-	} while (p && !configfs_is_root(p));
+		depth--;
+	} while (depth && p && !configfs_is_root(p));
 	return length;
 }
 
-static void fill_item_path(struct config_item * item, char * buffer, int length)
+
+static void fill_item_path(struct config_item *item, int depth, char *buffer, int length)
 {
 	struct config_item * p;
 
 	--length;
-	for (p = item; p && !configfs_is_root(p); p = p->ci_parent) {
+	for (p = item; depth && p && !configfs_is_root(p); p = p->ci_parent, depth--) {
 		int cur = strlen(config_item_name(p));
 
 		/* back up enough to print this bus id with '/' */
 		length -= cur;
 		memcpy(buffer + length, config_item_name(p), cur);
-		*(buffer + --length) = '/';
+		if (depth > 1)
+			*(buffer + --length) = '/';
 	}
 }
 
 static int configfs_get_target_path(struct config_item *item,
 		struct config_item *target, char **path)
 {
-	int depth, size;
+	struct config_item *pdest, *ptarget;
+	int target_depth = 0, item_depth = 0;
+	int size;
 	char *s;
 
-	depth = item_depth(item);
-	size = item_path_length(target) + depth * 3 - 1;
+	/* find closest common parent to make a minimal path */
+	for (ptarget = target;
+	     ptarget && !configfs_is_root(ptarget);
+	     ptarget = ptarget->ci_parent) {
+		item_depth = 0;
+		for (pdest = item;
+		     pdest && !configfs_is_root(pdest);
+		     pdest = pdest->ci_parent) {
+			if (pdest == ptarget)
+				goto out;
+
+			item_depth++;
+		}
+
+		target_depth++;
+	}
+out:
+	size = 3 * item_depth + item_path_length(target, target_depth) - 1;
 	if (size > PATH_MAX)
 		return -ENAMETOOLONG;
 
-	pr_debug("%s: depth = %d, size = %d\n", __func__, depth, size);
+	pr_debug("%s: item_depth = %d, target_depth = %d, size = %d\n",
+		 __func__, item_depth, target_depth, size);
 
 	*path = kzalloc(size, GFP_KERNEL);
 	if (!*path)
 		return -ENOMEM;
 
+	for (s = *path; item_depth--; s += 3)
+		strcpy(s, "../");
 
-	for (s = *path; depth--; s += 3)
-		strcpy(s,"../");
-
-	fill_item_path(target, *path, size);
+	fill_item_path(target, target_depth, *path, size);
 	pr_debug("%s: path = '%s'\n", __func__, *path);
 	return 0;
 }
-- 
2.25.1

Re: [PATCH 2/2] configfs: make a minimal path of symlink

[Joel Becker]

On Mon, Apr 01, 2024 at 11:26:55AM +0300, Dmitry Bogdanov wrote:
> Symlinks in configfs are used to be created from near places. Currently the

Perhaps "... are usually created from nearby places. ..."

> path is artificially inflated by multiple ../ to the configfs root an then
> a full path of the target.
> 
> For scsi target subsystem the difference between such a path and a minimal
> possible path is ~100 characters.
> 
> This patch makes a minimal relative path of symlink - from the closest
> common parent.
> 
> Signed-off-by: Dmitry Bogdanov <d.bogdanov@yadro.com>
> ---
>  fs/configfs/symlink.c | 59 ++++++++++++++++++++++++++++---------------
>  1 file changed, 38 insertions(+), 21 deletions(-)
> 
> diff --git a/fs/configfs/symlink.c b/fs/configfs/symlink.c
> index 224c9e4899d4..a61f5a4763e1 100644

<snip>

>  static int configfs_get_target_path(struct config_item *item,
>  		struct config_item *target, char **path)
>  {
> -	int depth, size;
> +	struct config_item *pdest, *ptarget;
> +	int target_depth = 0, item_depth = 0;
> +	int size;
>  	char *s;
>  
> -	depth = item_depth(item);
> -	size = item_path_length(target) + depth * 3 - 1;
> +	/* find closest common parent to make a minimal path */
> +	for (ptarget = target;
> +	     ptarget && !configfs_is_root(ptarget);
> +	     ptarget = ptarget->ci_parent) {
> +		item_depth = 0;
> +		for (pdest = item;
> +		     pdest && !configfs_is_root(pdest);
> +		     pdest = pdest->ci_parent) {
> +			if (pdest == ptarget)
> +				goto out;
> +
> +			item_depth++;
> +		}
> +
> +		target_depth++;
> +	}

This O(n^2) loop tickles my spidey senses.  Reading it over, I think it
is correct, though I'm wary of how it might handle certain inputs.  I
also tried to think of ways it could short circuit the inner loop based
on the max target depth, but it can't know that without precomputing
the max target depth.

There are enough corner cases that I would think the depth computation
is a good candidate for KUnit tests.

Thanks,
Joel

-- 

"Hell is oneself, hell is alone, the other figures in it, merely projections."
        - T. S. Eliot

			http://www.jlbec.org/
			jlbec@evilplan.org