[v2] btrfs: send: lower memory requirements in common case
diff mbox

Message ID 1391613454-30622-1-git-send-email-dsterba@suse.cz
State Accepted
Headers show

Commit Message

David Sterba Feb. 5, 2014, 3:17 p.m. UTC
The fs_path structure uses an inline buffer and falls back to a chain of
allocations, but vmalloc is not necessary because PATH_MAX fits into
PAGE_SIZE.

The size of fs_path has been reduced to 256 bytes from PAGE_SIZE,
usually 4k. Experimental measurements show that most paths on a single
filesystem do not exceed 200 bytes, and these get stored into the inline
buffer directly, which is now 230 bytes. Longer paths are kmalloced when
needed.

Signed-off-by: David Sterba <dsterba@suse.cz>
---
v2:
- intel build test reports that krealloc should not reuse the buffer for return
  value, though it's not a problem in our case, a failed allocation leads to
  immediate return, let's use a temporary variable to keep the check happy

 fs/btrfs/send.c |  106 +++++++++++++++++++-----------------------------------
 1 files changed, 37 insertions(+), 69 deletions(-)

Comments

Filipe Manana Feb. 20, 2014, noon UTC | #1
On Wed, Feb 5, 2014 at 3:17 PM, David Sterba <dsterba@suse.cz> wrote:
> The fs_path structure uses an inline buffer and falls back to a chain of
> allocations, but vmalloc is not necessary because PATH_MAX fits into
> PAGE_SIZE.
>
> The size of fs_path has been reduced to 256 bytes from PAGE_SIZE,
> usually 4k. Experimental measurements show that most paths on a single
> filesystem do not exceed 200 bytes, and these get stored into the inline
> buffer directly, which is now 230 bytes. Longer paths are kmalloced when
> needed.
>
> Signed-off-by: David Sterba <dsterba@suse.cz>
> ---
> v2:
> - intel build test reports that krealloc should not reuse the buffer for return
>   value, though it's not a problem in our case, a failed allocation leads to
>   immediate return, let's use a temporary variable to keep the check happy
>
>  fs/btrfs/send.c |  106 +++++++++++++++++++-----------------------------------
>  1 files changed, 37 insertions(+), 69 deletions(-)
>
> diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c
> index 1f09c74e1c1f..dd0b02adb1e6 100644
> --- a/fs/btrfs/send.c
> +++ b/fs/btrfs/send.c
> @@ -57,7 +57,12 @@ struct fs_path {
>                         unsigned short reversed:1;
>                         char inline_buf[];
>                 };
> -               char pad[PAGE_SIZE];
> +               /*
> +                * Average path length does not exceed 200 bytes, we'll have
> +                * better packing in the slab and higher chance to satisfy
> +                * a allocation later during send.
> +                */
> +               char pad[256];
>         };
>  };
>  #define FS_PATH_INLINE_SIZE \
> @@ -262,12 +267,8 @@ static void fs_path_free(struct fs_path *p)
>  {
>         if (!p)
>                 return;
> -       if (p->buf != p->inline_buf) {
> -               if (is_vmalloc_addr(p->buf))
> -                       vfree(p->buf);
> -               else
> -                       kfree(p->buf);
> -       }
> +       if (p->buf != p->inline_buf)
> +               kfree(p->buf);
>         kfree(p);
>  }
>
> @@ -287,40 +288,31 @@ static int fs_path_ensure_buf(struct fs_path *p, int len)
>         if (p->buf_len >= len)
>                 return 0;
>
> -       path_len = p->end - p->start;
> -       old_buf_len = p->buf_len;
> -       len = PAGE_ALIGN(len);
> -
> +       /*
> +        * First time the inline_buf does not suffice
> +        */
>         if (p->buf == p->inline_buf) {
> -               tmp_buf = kmalloc(len, GFP_NOFS | __GFP_NOWARN);
> -               if (!tmp_buf) {
> -                       tmp_buf = vmalloc(len);
> -                       if (!tmp_buf)
> -                               return -ENOMEM;
> -               }
> -               memcpy(tmp_buf, p->buf, p->buf_len);
> -               p->buf = tmp_buf;
> -               p->buf_len = len;
> +               p->buf = kmalloc(len, GFP_NOFS);
> +               if (!p->buf)
> +                       return -ENOMEM;
> +               /*
> +                * The real size of the buffer is bigger, this will let the
> +                * fast path happen most of the time
> +                */
> +               p->buf_len = ksize(p->buf);
>         } else {
> -               if (is_vmalloc_addr(p->buf)) {
> -                       tmp_buf = vmalloc(len);
> -                       if (!tmp_buf)
> -                               return -ENOMEM;
> -                       memcpy(tmp_buf, p->buf, p->buf_len);
> -                       vfree(p->buf);
> -               } else {
> -                       tmp_buf = krealloc(p->buf, len, GFP_NOFS);
> -                       if (!tmp_buf) {
> -                               tmp_buf = vmalloc(len);
> -                               if (!tmp_buf)
> -                                       return -ENOMEM;
> -                               memcpy(tmp_buf, p->buf, p->buf_len);
> -                               kfree(p->buf);
> -                       }
> -               }
> -               p->buf = tmp_buf;
> -               p->buf_len = len;
> +               char *tmp;
> +
> +               tmp = krealloc(p->buf, len, GFP_NOFS);
> +               if (!tmp)
> +                       return -ENOMEM;
> +               p->buf = tmp;
> +               p->buf_len = ksize(p->buf);
>         }
> +
> +       path_len = p->end - p->start;
> +       old_buf_len = p->buf_len;

Hi Dave,

I think this is not correct here. old_buf_len doesn't get assigned the
old buffer's length but instead the new length. So this assignment
should be before the if-then-else that allocates/reallocates the path
buffer, just like it was done previously before this change.

Or is there anything I missed here?

thanks

> +
>         if (p->reversed) {
>                 tmp_buf = p->buf + old_buf_len - path_len - 1;
>                 p->end = p->buf + p->buf_len - 1;
> @@ -911,9 +903,7 @@ static int iterate_dir_item(struct btrfs_root *root, struct btrfs_path *path,
>         struct btrfs_dir_item *di;
>         struct btrfs_key di_key;
>         char *buf = NULL;
> -       char *buf2 = NULL;
> -       int buf_len;
> -       int buf_virtual = 0;
> +       const int buf_len = PATH_MAX;
>         u32 name_len;
>         u32 data_len;
>         u32 cur;
> @@ -923,7 +913,6 @@ static int iterate_dir_item(struct btrfs_root *root, struct btrfs_path *path,
>         int num;
>         u8 type;
>
> -       buf_len = PAGE_SIZE;
>         buf = kmalloc(buf_len, GFP_NOFS);
>         if (!buf) {
>                 ret = -ENOMEM;
> @@ -945,30 +934,12 @@ static int iterate_dir_item(struct btrfs_root *root, struct btrfs_path *path,
>                 type = btrfs_dir_type(eb, di);
>                 btrfs_dir_item_key_to_cpu(eb, di, &di_key);
>
> +               /*
> +                * Path too long
> +                */
>                 if (name_len + data_len > buf_len) {
> -                       buf_len = PAGE_ALIGN(name_len + data_len);
> -                       if (buf_virtual) {
> -                               buf2 = vmalloc(buf_len);
> -                               if (!buf2) {
> -                                       ret = -ENOMEM;
> -                                       goto out;
> -                               }
> -                               vfree(buf);
> -                       } else {
> -                               buf2 = krealloc(buf, buf_len, GFP_NOFS);
> -                               if (!buf2) {
> -                                       buf2 = vmalloc(buf_len);
> -                                       if (!buf2) {
> -                                               ret = -ENOMEM;
> -                                               goto out;
> -                                       }
> -                                       kfree(buf);
> -                                       buf_virtual = 1;
> -                               }
> -                       }
> -
> -                       buf = buf2;
> -                       buf2 = NULL;
> +                       ret = -ENAMETOOLONG;
> +                       goto out;
>                 }
>
>                 read_extent_buffer(eb, buf, (unsigned long)(di + 1),
> @@ -991,10 +962,7 @@ static int iterate_dir_item(struct btrfs_root *root, struct btrfs_path *path,
>         }
>
>  out:
> -       if (buf_virtual)
> -               vfree(buf);
> -       else
> -               kfree(buf);
> +       kfree(buf);
>         return ret;
>  }
>
> --
> 1.7.9
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Sterba Feb. 25, 2014, 6:07 p.m. UTC | #2
On Thu, Feb 20, 2014 at 12:00:23PM +0000, Filipe David Manana wrote:
> >         } else {
> > -               if (is_vmalloc_addr(p->buf)) {
> > -                       tmp_buf = vmalloc(len);
> > -                       if (!tmp_buf)
> > -                               return -ENOMEM;
> > -                       memcpy(tmp_buf, p->buf, p->buf_len);
> > -                       vfree(p->buf);
> > -               } else {
> > -                       tmp_buf = krealloc(p->buf, len, GFP_NOFS);
> > -                       if (!tmp_buf) {
> > -                               tmp_buf = vmalloc(len);
> > -                               if (!tmp_buf)
> > -                                       return -ENOMEM;
> > -                               memcpy(tmp_buf, p->buf, p->buf_len);
> > -                               kfree(p->buf);
> > -                       }
> > -               }
> > -               p->buf = tmp_buf;
> > -               p->buf_len = len;
> > +               char *tmp;
> > +
> > +               tmp = krealloc(p->buf, len, GFP_NOFS);
> > +               if (!tmp)
> > +                       return -ENOMEM;
> > +               p->buf = tmp;
> > +               p->buf_len = ksize(p->buf);
> >         }
> > +
> > +       path_len = p->end - p->start;
> > +       old_buf_len = p->buf_len;
> 
> I think this is not correct here. old_buf_len doesn't get assigned the
> old buffer's length but instead the new length. So this assignment
> should be before the if-then-else that allocates/reallocates the path
> buffer, just like it was done previously before this change.

You're right, I'll send a fix. Thanks.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch
diff mbox

diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c
index 1f09c74e1c1f..dd0b02adb1e6 100644
--- a/fs/btrfs/send.c
+++ b/fs/btrfs/send.c
@@ -57,7 +57,12 @@  struct fs_path {
 			unsigned short reversed:1;
 			char inline_buf[];
 		};
-		char pad[PAGE_SIZE];
+		/*
+		 * Average path length does not exceed 200 bytes, we'll have
+		 * better packing in the slab and higher chance to satisfy
+		 * a allocation later during send.
+		 */
+		char pad[256];
 	};
 };
 #define FS_PATH_INLINE_SIZE \
@@ -262,12 +267,8 @@  static void fs_path_free(struct fs_path *p)
 {
 	if (!p)
 		return;
-	if (p->buf != p->inline_buf) {
-		if (is_vmalloc_addr(p->buf))
-			vfree(p->buf);
-		else
-			kfree(p->buf);
-	}
+	if (p->buf != p->inline_buf)
+		kfree(p->buf);
 	kfree(p);
 }
 
@@ -287,40 +288,31 @@  static int fs_path_ensure_buf(struct fs_path *p, int len)
 	if (p->buf_len >= len)
 		return 0;
 
-	path_len = p->end - p->start;
-	old_buf_len = p->buf_len;
-	len = PAGE_ALIGN(len);
-
+	/*
+	 * First time the inline_buf does not suffice
+	 */
 	if (p->buf == p->inline_buf) {
-		tmp_buf = kmalloc(len, GFP_NOFS | __GFP_NOWARN);
-		if (!tmp_buf) {
-			tmp_buf = vmalloc(len);
-			if (!tmp_buf)
-				return -ENOMEM;
-		}
-		memcpy(tmp_buf, p->buf, p->buf_len);
-		p->buf = tmp_buf;
-		p->buf_len = len;
+		p->buf = kmalloc(len, GFP_NOFS);
+		if (!p->buf)
+			return -ENOMEM;
+		/*
+		 * The real size of the buffer is bigger, this will let the
+		 * fast path happen most of the time
+		 */
+		p->buf_len = ksize(p->buf);
 	} else {
-		if (is_vmalloc_addr(p->buf)) {
-			tmp_buf = vmalloc(len);
-			if (!tmp_buf)
-				return -ENOMEM;
-			memcpy(tmp_buf, p->buf, p->buf_len);
-			vfree(p->buf);
-		} else {
-			tmp_buf = krealloc(p->buf, len, GFP_NOFS);
-			if (!tmp_buf) {
-				tmp_buf = vmalloc(len);
-				if (!tmp_buf)
-					return -ENOMEM;
-				memcpy(tmp_buf, p->buf, p->buf_len);
-				kfree(p->buf);
-			}
-		}
-		p->buf = tmp_buf;
-		p->buf_len = len;
+		char *tmp;
+
+		tmp = krealloc(p->buf, len, GFP_NOFS);
+		if (!tmp)
+			return -ENOMEM;
+		p->buf = tmp;
+		p->buf_len = ksize(p->buf);
 	}
+
+	path_len = p->end - p->start;
+	old_buf_len = p->buf_len;
+
 	if (p->reversed) {
 		tmp_buf = p->buf + old_buf_len - path_len - 1;
 		p->end = p->buf + p->buf_len - 1;
@@ -911,9 +903,7 @@  static int iterate_dir_item(struct btrfs_root *root, struct btrfs_path *path,
 	struct btrfs_dir_item *di;
 	struct btrfs_key di_key;
 	char *buf = NULL;
-	char *buf2 = NULL;
-	int buf_len;
-	int buf_virtual = 0;
+	const int buf_len = PATH_MAX;
 	u32 name_len;
 	u32 data_len;
 	u32 cur;
@@ -923,7 +913,6 @@  static int iterate_dir_item(struct btrfs_root *root, struct btrfs_path *path,
 	int num;
 	u8 type;
 
-	buf_len = PAGE_SIZE;
 	buf = kmalloc(buf_len, GFP_NOFS);
 	if (!buf) {
 		ret = -ENOMEM;
@@ -945,30 +934,12 @@  static int iterate_dir_item(struct btrfs_root *root, struct btrfs_path *path,
 		type = btrfs_dir_type(eb, di);
 		btrfs_dir_item_key_to_cpu(eb, di, &di_key);
 
+		/*
+		 * Path too long
+		 */
 		if (name_len + data_len > buf_len) {
-			buf_len = PAGE_ALIGN(name_len + data_len);
-			if (buf_virtual) {
-				buf2 = vmalloc(buf_len);
-				if (!buf2) {
-					ret = -ENOMEM;
-					goto out;
-				}
-				vfree(buf);
-			} else {
-				buf2 = krealloc(buf, buf_len, GFP_NOFS);
-				if (!buf2) {
-					buf2 = vmalloc(buf_len);
-					if (!buf2) {
-						ret = -ENOMEM;
-						goto out;
-					}
-					kfree(buf);
-					buf_virtual = 1;
-				}
-			}
-
-			buf = buf2;
-			buf2 = NULL;
+			ret = -ENAMETOOLONG;
+			goto out;
 		}
 
 		read_extent_buffer(eb, buf, (unsigned long)(di + 1),
@@ -991,10 +962,7 @@  static int iterate_dir_item(struct btrfs_root *root, struct btrfs_path *path,
 	}
 
 out:
-	if (buf_virtual)
-		vfree(buf);
-	else
-		kfree(buf);
+	kfree(buf);
 	return ret;
 }