[v4] Btrfs: send, lower mem requirements for processing xattrs

Commit Message

Filipe Manana Aug. 11, 2014, 2:09 a.m. UTC

Maximum xattr size can be up to nearly the leaf size. For an fs with a
leaf size larger than the page size, using kmalloc requires allocating
multiple pages that are contiguous, which might not be possible if
there's heavy memory fragmentation. Therefore fallback to vmalloc if
we fail to allocate with kmalloc. Also start with a smaller buffer size,
since xattr values typically are smaller than a page.

Reported-by: Chris Murphy <[e-mail]>
Signed-off-by: Filipe Manana <[e-mail]>
---

V2: Use is_vmalloc_addr() instead of keeping a boolean variable around.
V3: Use krealloc instead of kfree + kmalloc.
V4: Fixed a checkpatch warning about missing blank line after var declaration.

 fs/btrfs/send.c | 42 ++++++++++++++++++++++++++++++++++--------
 1 file changed, 34 insertions(+), 8 deletions(-)

Comments

David Sterba Aug. 19, 2014, 2:46 p.m. UTC | #1

On Mon, Aug 11, 2014 at 03:09:35AM +0100, Filipe Manana wrote:
> +		if (name_len + data_len > buf_len) {
> +			buf_len = name_len + data_len;
> +			if (is_vmalloc_addr(buf)) {
> +				vfree(buf);
> +				buf = NULL;
> +			} else {
> +				char *tmp = krealloc(buf, buf_len, GFP_NOFS);

This could fail with a warning (high order allocation) so I suggest to
add __GFP_NOWARN, the vmalloc fallback will catch fragmented memory case
and fail if theres no memory.

> +
> +				if (!tmp)
> +					kfree(buf);
> +				buf = tmp;
> +			}
> +			if (!buf) {
> +				buf = vmalloc(buf_len);
> +				if (!buf) {
> +					ret = -ENOMEM;
> +					goto out;
> +				}
> +			}
> +		}
> +
>  		read_extent_buffer(eb, buf, (unsigned long)(di + 1),
>  				name_len + data_len);
>  
> @@ -1071,7 +1094,10 @@ static int iterate_dir_item(struct btrfs_root *root, struct btrfs_path *path,
>  	}
>  
>  out:
> -	kfree(buf);
> +	if (is_vmalloc_addr(buf))
> +		vfree(buf);
> +	else
> +		kfree(buf);

There's even kvfree to do this.

>  	return ret;
>  }
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c
index 3c63b29..b29fc5c 100644
--- a/fs/btrfs/send.c
+++ b/fs/btrfs/send.c
@@ -1006,11 +1006,13 @@  static int iterate_dir_item(struct btrfs_root *root, struct btrfs_path *path,
 	int num;
 	u8 type;
 
-	if (found_key->type == BTRFS_XATTR_ITEM_KEY)
-		buf_len = BTRFS_MAX_XATTR_SIZE(root);
-	else
-		buf_len = PATH_MAX;
-
+	/*
+	 * Start with a small buffer (1 page). If later we end up needing more
+	 * space, which can happen for xattrs on a fs with a leaf size greater
+	 * then the page size, attempt to increase the buffer. Typically xattr
+	 * values are small.
+	 */
+	buf_len = PATH_MAX;
 	buf = kmalloc(buf_len, GFP_NOFS);
 	if (!buf) {
 		ret = -ENOMEM;
@@ -1037,7 +1039,7 @@  static int iterate_dir_item(struct btrfs_root *root, struct btrfs_path *path,
 				ret = -ENAMETOOLONG;
 				goto out;
 			}
-			if (name_len + data_len > buf_len) {
+			if (name_len + data_len > BTRFS_MAX_XATTR_SIZE(root)) {
 				ret = -E2BIG;
 				goto out;
 			}
@@ -1045,12 +1047,33 @@  static int iterate_dir_item(struct btrfs_root *root, struct btrfs_path *path,
 			/*
 			 * Path too long
 			 */
-			if (name_len + data_len > buf_len) {
+			if (name_len + data_len > PATH_MAX) {
 				ret = -ENAMETOOLONG;
 				goto out;
 			}
 		}
 
+		if (name_len + data_len > buf_len) {
+			buf_len = name_len + data_len;
+			if (is_vmalloc_addr(buf)) {
+				vfree(buf);
+				buf = NULL;
+			} else {
+				char *tmp = krealloc(buf, buf_len, GFP_NOFS);
+
+				if (!tmp)
+					kfree(buf);
+				buf = tmp;
+			}
+			if (!buf) {
+				buf = vmalloc(buf_len);
+				if (!buf) {
+					ret = -ENOMEM;
+					goto out;
+				}
+			}
+		}
+
 		read_extent_buffer(eb, buf, (unsigned long)(di + 1),
 				name_len + data_len);
 
@@ -1071,7 +1094,10 @@  static int iterate_dir_item(struct btrfs_root *root, struct btrfs_path *path,
 	}
 
 out:
-	kfree(buf);
+	if (is_vmalloc_addr(buf))
+		vfree(buf);
+	else
+		kfree(buf);
 	return ret;
 }