* [PATCH v2] btrfs: send: lower memory requirements in common case
@ 2014-02-05 15:17 David Sterba
2014-02-20 12:00 ` Filipe David Manana
0 siblings, 1 reply; 3+ messages in thread
From: David Sterba @ 2014-02-05 15:17 UTC (permalink / raw)
To: linux-btrfs; +Cc: David Sterba
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;
+
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
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v2] btrfs: send: lower memory requirements in common case
2014-02-05 15:17 [PATCH v2] btrfs: send: lower memory requirements in common case David Sterba
@ 2014-02-20 12:00 ` Filipe David Manana
2014-02-25 18:07 ` David Sterba
0 siblings, 1 reply; 3+ messages in thread
From: Filipe David Manana @ 2014-02-20 12:00 UTC (permalink / raw)
To: David Sterba; +Cc: linux-btrfs@vger.kernel.org
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
--
Filipe David Manana,
"Reasonable men adapt themselves to the world.
Unreasonable men adapt the world to themselves.
That's why all progress depends on unreasonable men."
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v2] btrfs: send: lower memory requirements in common case
2014-02-20 12:00 ` Filipe David Manana
@ 2014-02-25 18:07 ` David Sterba
0 siblings, 0 replies; 3+ messages in thread
From: David Sterba @ 2014-02-25 18:07 UTC (permalink / raw)
To: Filipe David Manana; +Cc: linux-btrfs@vger.kernel.org
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.
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2014-02-25 18:07 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-02-05 15:17 [PATCH v2] btrfs: send: lower memory requirements in common case David Sterba
2014-02-20 12:00 ` Filipe David Manana
2014-02-25 18:07 ` David Sterba
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).