More vmalloc cleanups

PATCH In review David Sterba 3 participants 11 messages May 31, 2017 – Jun 1, 2017 Btrfs file system

lore

Share & tools

lore

David Sterba

author maintainer

2017-05-31 17:50 UTC #0 lore raw

cover More vmalloc cleanups

All vmalloc/vzalloc calls should be addressed, in this or the currently pending patches.

David Sterba (5):
btrfs: replace opencoded kvzalloc with the helper
btrfs: send: use kvmalloc in iterate_dir_item
btrfs: scrub: add memalloc_nofs protection around init_ipath
btrfs: use GFP_KERNEL in init_ipath
btrfs: adjust includes after vmalloc removal

 fs/btrfs/backref.c         | 10 +++++-----
 fs/btrfs/check-integrity.c | 11 ++++-------
 fs/btrfs/ctree.c           |  2 +-
 fs/btrfs/ioctl.c           |  4 ++--
 fs/btrfs/raid56.c          | 11 ++++-------
 fs/btrfs/scrub.c           |  9 +++++++++
 fs/btrfs/send.c            | 11 ++++-------
 7 files changed, 29 insertions(+), 29 deletions(-)

Signature · 1 line

2.12.0

David Sterba

author maintainer

2017-05-31 17:50 UTC #1 lore raw

PATCH 1/5 btrfs: replace opencoded kvzalloc with the helper

The logic of kmalloc and vmalloc fallback is open coded in several places, we can now use the existing helper.

Signed-off-by David Sterba <[email protected]>

---
 fs/btrfs/check-integrity.c | 11 ++++-------
 fs/btrfs/raid56.c          | 11 ++++-------
 fs/btrfs/send.c            |  9 +++------
 3 files changed, 11 insertions(+), 20 deletions(-)

fs/btrfs/check-integrity.c +4 -7

index 6cabc8acee2a..5f8006e4de9d 100644
@@ -94,7 +94,7 @@
 #include <linux/mutex.h>
 #include <linux/genhd.h>
 #include <linux/blkdev.h>
-#include <linux/vmalloc.h>
+#include <linux/mm.h>
 #include <linux/string.h>
 #include "ctree.h"
 #include "disk-io.h"

@@ -2920,13 +2920,10 @@ int btrfsic_mount(struct btrfs_fs_info *fs_info,
 		       fs_info->sectorsize, PAGE_SIZE);
 		return -1;
 	}
-	state = kzalloc(sizeof(*state), GFP_KERNEL | __GFP_NOWARN | __GFP_REPEAT);
+	state = kvzalloc(sizeof(*state), GFP_KERNEL);
 	if (!state) {
-		state = vzalloc(sizeof(*state));
-		if (!state) {
-			pr_info("btrfs check-integrity: vzalloc() failed!\n");
-			return -1;
-		}
+		pr_info("btrfs check-integrity: allocation failed!\n");
+		return -1;
 	}
 
 	if (!btrfsic_is_initialized) {

fs/btrfs/raid56.c +4 -7

index d8ea0eb76325..d68af3c61b49 100644
@@ -31,7 +31,7 @@
 #include <linux/hash.h>
 #include <linux/list_sort.h>
 #include <linux/raid/xor.h>
-#include <linux/vmalloc.h>
+#include <linux/mm.h>
 #include <asm/div64.h>
 #include "ctree.h"
 #include "extent_map.h"

@@ -218,12 +218,9 @@ int btrfs_alloc_stripe_hash_table(struct btrfs_fs_info *info)
 	 * of a failing mount.
 	 */
 	table_size = sizeof(*table) + sizeof(*h) * num_entries;
-	table = kzalloc(table_size, GFP_KERNEL | __GFP_NOWARN | __GFP_REPEAT);
-	if (!table) {
-		table = vzalloc(table_size);
-		if (!table)
-			return -ENOMEM;
-	}
+	table = kvzalloc(table_size, GFP_KERNEL);
+	if (!table)
+		return -ENOMEM;
 
 	spin_lock_init(&table->cache_lock);
 	INIT_LIST_HEAD(&table->stripe_cache);

fs/btrfs/send.c +3 -6

index e8185c83f667..924b1d941b53 100644
@@ -6389,13 +6389,10 @@ long btrfs_ioctl_send(struct file *mnt_file, void __user *arg_)
 
 	alloc_size = sizeof(struct clone_root) * (arg->clone_sources_count + 1);
 
-	sctx->clone_roots = kzalloc(alloc_size, GFP_KERNEL | __GFP_NOWARN);
+	sctx->clone_roots = kzalloc(alloc_size, GFP_KERNEL);
 	if (!sctx->clone_roots) {
-		sctx->clone_roots = vzalloc(alloc_size);
-		if (!sctx->clone_roots) {
-			ret = -ENOMEM;
-			goto out;
-		}
+		ret = -ENOMEM;
+		goto out;
 	}
 
 	alloc_size = arg->clone_sources_count * sizeof(*arg->clone_sources);

Signature · 1 line

2.12.0

Anand Jain

2017-06-01 07:58 UTC #2 lore raw

Re: [PATCH 1/5] btrfs: replace opencoded kvzalloc with the helper

fs/btrfs/send.c long btrfs_ioctl_send(struct file *mnt_file, void __user *arg_) · line 6392

Quoted 13 lines

fs/btrfs/send.c +1 -1

index e8185c83f667..924b1d941b53 100644
@@ -6389,13 +6389,10 @@ long btrfs_ioctl_send(struct file *mnt_file, void __user *arg_)
 
 	alloc_size = sizeof(struct clone_root) * (arg->clone_sources_count + 1);
 
-	sctx->clone_roots = kzalloc(alloc_size, GFP_KERNEL | __GFP_NOWARN);
+	sctx->clone_roots = kzalloc(alloc_size, GFP_KERNEL);

Should be kvzalloc ?

Thanks, Anand

Quoted 12 lines

fs/btrfs/send.c +2 -5

 	if (!sctx->clone_roots) {
-		sctx->clone_roots = vzalloc(alloc_size);
-		if (!sctx->clone_roots) {
-			ret = -ENOMEM;
-			goto out;
-		}
+		ret = -ENOMEM;
+		goto out;
 	}
 
 	alloc_size = arg->clone_sources_count * sizeof(*arg->clone_sources);

David Sterba

2017-06-01 14:26 UTC #3 lore raw

Re: [PATCH 1/5] btrfs: replace opencoded kvzalloc with the helper

Quoted 9 lines · Anand Jain

Quoted 9 lines

fs/btrfs/send.c +0 -0

index e8185c83f667..924b1d941b53 100644
@@ -6389,13 +6389,10 @@ long btrfs_ioctl_send(struct file *mnt_file, void __user *arg_)

Quoted 7 lines

fs/btrfs/send.c long btrfs_ioctl_send(struct file *mnt_file, void __user *arg_) · line 6392
Quoted 4 lines
fs/btrfs/send.c +1 -1
 	alloc_size = sizeof(struct clone_root) * (arg->clone_sources_count + 1);
 
-	sctx->clone_roots = kzalloc(alloc_size, GFP_KERNEL | __GFP_NOWARN);
+	sctx->clone_roots = kzalloc(alloc_size, GFP_KERNEL);
Should be kvzalloc ?

Of course, I'll fix it. Thanks.

David Sterba

author maintainer

2017-05-31 17:50 UTC #4 lore raw

PATCH 2/5 btrfs: send: use kvmalloc in iterate_dir_item

In reply to David Sterba

We use a growing buffer for xattrs larger than a page size, at some point vmalloc is unconditionally used for larger buffers. We can still try to avoid it using the kvmalloc helper.

Signed-off-by David Sterba <[email protected]>

---
 fs/btrfs/send.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

fs/btrfs/send.c +1 -1

index 924b1d941b53..7416b17c0eac 100644
@@ -1083,7 +1083,7 @@ static int iterate_dir_item(struct btrfs_root *root, struct btrfs_path *path,
 				buf = tmp;
 			}
 			if (!buf) {
-				buf = vmalloc(buf_len);
+				buf = kvmalloc(buf_len, GFP_KERNEL);
 				if (!buf) {
 					ret = -ENOMEM;
 					goto out;

Signature · 1 line

2.12.0

Anand Jain

Reviewed-by on v1 2/5

2017-06-01 09:17 UTC #5 lore raw

Re: [PATCH 2/5] btrfs: send: use kvmalloc in iterate_dir_item

Quoted 3 lines · David Sterba

We use a growing buffer for xattrs larger than a page size, at some point vmalloc is unconditionally used for larger buffers. We can still try to avoid it using the kvmalloc helper.

Reviewed-by Anand Jain <[email protected]>

Quoted 19 lines

Signed-off-by David Sterba <[email protected]>

---
 fs/btrfs/send.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

fs/btrfs/send.c +1 -1

index 924b1d941b53..7416b17c0eac 100644
@@ -1083,7 +1083,7 @@ static int iterate_dir_item(struct btrfs_root *root, struct btrfs_path *path,
 				buf = tmp;
 			}
 			if (!buf) {
-				buf = vmalloc(buf_len);
+				buf = kvmalloc(buf_len, GFP_KERNEL);
 				if (!buf) {
 					ret = -ENOMEM;
 					goto out;

David Sterba

author maintainer

2017-05-31 17:50 UTC #6 lore raw

PATCH 3/5 btrfs: scrub: add memalloc_nofs protection around init_ipath

In reply to David Sterba

init_ipath is called from a safe ioctl context and from scrub when printing an error. The protection is added for three reasons:

* init_data_container calls vmalloc and this does not work as expected in the GFP_NOFS context, so this silently does GFP_KERNEL and might deadlock in some cases
* keep the context constraint of GFP_NOFS, used by scrub
* we want to use GFP_KERNEL unconditionally inside init_ipath or its callees

Signed-off-by David Sterba <[email protected]>

---
 fs/btrfs/scrub.c | 9 +++++++++
 1 file changed, 9 insertions(+)

fs/btrfs/scrub.c +9 -0

index e99be644b19f..096e503e3ddc 100644
@@ -18,6 +18,7 @@
 
 #include <linux/blkdev.h>
 #include <linux/ratelimit.h>
+#include <linux/sched/mm.h>
 #include "ctree.h"
 #include "volumes.h"
 #include "disk-io.h"

@@ -733,6 +734,7 @@ static int scrub_print_warning_inode(u64 inum, u64 offset, u64 root,
 	u32 nlink;
 	int ret;
 	int i;
+	unsigned nofs_flag;
 	struct extent_buffer *eb;
 	struct btrfs_inode_item *inode_item;
 	struct scrub_warning *swarn = warn_ctx;

@@ -771,7 +773,14 @@ static int scrub_print_warning_inode(u64 inum, u64 offset, u64 root,
 	nlink = btrfs_inode_nlink(eb, inode_item);
 	btrfs_release_path(swarn->path);
 
+	/*
+	 * init_path might indirectly call vmalloc, or use GFP_KERNEL. Scrub
+	 * uses GFP_NOFS in this context, so we keep it consistent but it does
+	 * not seem to be strictly necessary.
+	 */
+	nofs_flag = memalloc_nofs_save();
 	ipath = init_ipath(4096, local_root, swarn->path);
+	memalloc_nofs_restore(nofs_flag);
 	if (IS_ERR(ipath)) {
 		ret = PTR_ERR(ipath);
 		ipath = NULL;

Signature · 1 line

2.12.0

Anand Jain

Reviewed-by on v1 3/5

2017-06-01 09:21 UTC #7 lore raw

Re: [PATCH 3/5] btrfs: scrub: add memalloc_nofs protection around init_ipath

Quoted 9 lines · David Sterba

init_ipath is called from a safe ioctl context and from scrub when printing an error. The protection is added for three reasons:
* init_data_container calls vmalloc and this does not work as expected in the GFP_NOFS context, so this silently does GFP_KERNEL and might deadlock in some cases
* keep the context constraint of GFP_NOFS, used by scrub
* we want to use GFP_KERNEL unconditionally inside init_ipath or its callees

Reviewed-by Anand Jain <[email protected]>

Thanks, Anand

Quoted 41 lines

Signed-off-by David Sterba <[email protected]>

---
 fs/btrfs/scrub.c | 9 +++++++++
 1 file changed, 9 insertions(+)

fs/btrfs/scrub.c +9 -0

index e99be644b19f..096e503e3ddc 100644
@@ -18,6 +18,7 @@
 
 #include <linux/blkdev.h>
 #include <linux/ratelimit.h>
+#include <linux/sched/mm.h>
 #include "ctree.h"
 #include "volumes.h"
 #include "disk-io.h"

@@ -733,6 +734,7 @@ static int scrub_print_warning_inode(u64 inum, u64 offset, u64 root,
 	u32 nlink;
 	int ret;
 	int i;
+	unsigned nofs_flag;
 	struct extent_buffer *eb;
 	struct btrfs_inode_item *inode_item;
 	struct scrub_warning *swarn = warn_ctx;

@@ -771,7 +773,14 @@ static int scrub_print_warning_inode(u64 inum, u64 offset, u64 root,
 	nlink = btrfs_inode_nlink(eb, inode_item);
 	btrfs_release_path(swarn->path);
 
+	/*
+	 * init_path might indirectly call vmalloc, or use GFP_KERNEL. Scrub
+	 * uses GFP_NOFS in this context, so we keep it consistent but it does
+	 * not seem to be strictly necessary.
+	 */
+	nofs_flag = memalloc_nofs_save();
 	ipath = init_ipath(4096, local_root, swarn->path);
+	memalloc_nofs_restore(nofs_flag);
 	if (IS_ERR(ipath)) {
 		ret = PTR_ERR(ipath);
 		ipath = NULL;

David Sterba

author maintainer

2017-05-31 17:50 UTC #8 lore raw

PATCH 4/5 btrfs: use GFP_KERNEL in init_ipath

In reply to David Sterba

Now that init_ipath is called either from a safe context or with memalloc_nofs protection, we can switch to GFP_KERNEL allocations in init_path and init_data_container.

Signed-off-by David Sterba <[email protected]>

---
 fs/btrfs/backref.c | 10 +++++-----
 fs/btrfs/ioctl.c   |  4 ++--
 2 files changed, 7 insertions(+), 7 deletions(-)

fs/btrfs/backref.c +5 -5

index 24865da63d8f..f723c11bb763 100644
@@ -16,7 +16,7 @@
  * Boston, MA 021110-1307, USA.
  */
 
-#include <linux/vmalloc.h>
+#include <linux/mm.h>
 #include <linux/rbtree.h>
 #include "ctree.h"
 #include "disk-io.h"

@@ -2305,7 +2305,7 @@ struct btrfs_data_container *init_data_container(u32 total_bytes)
 	size_t alloc_bytes;
 
 	alloc_bytes = max_t(size_t, total_bytes, sizeof(*data));
-	data = vmalloc(alloc_bytes);
+	data = kvmalloc(alloc_bytes, GFP_KERNEL);
 	if (!data)
 		return ERR_PTR(-ENOMEM);

@@ -2339,9 +2339,9 @@ struct inode_fs_paths *init_ipath(s32 total_bytes, struct btrfs_root *fs_root,
 	if (IS_ERR(fspath))
 		return (void *)fspath;
 
-	ifp = kmalloc(sizeof(*ifp), GFP_NOFS);
+	ifp = kmalloc(sizeof(*ifp), GFP_KERNEL);
 	if (!ifp) {
-		vfree(fspath);
+		kvfree(fspath);
 		return ERR_PTR(-ENOMEM);
 	}

@@ -2356,6 +2356,6 @@ void free_ipath(struct inode_fs_paths *ipath)
 {
 	if (!ipath)
 		return;
-	vfree(ipath->fspath);
+	kvfree(ipath->fspath);
 	kfree(ipath);
 }

fs/btrfs/ioctl.c +2 -2

index c9cdea8061bc..e4116f9248c2 100644
@@ -37,7 +37,7 @@
 #include <linux/bit_spinlock.h>
 #include <linux/security.h>
 #include <linux/xattr.h>
-#include <linux/vmalloc.h>
+#include <linux/mm.h>
 #include <linux/slab.h>
 #include <linux/blkdev.h>
 #include <linux/uuid.h>

@@ -4588,7 +4588,7 @@ static long btrfs_ioctl_logical_to_ino(struct btrfs_fs_info *fs_info,
 
 out:
 	btrfs_free_path(path);
-	vfree(inodes);
+	kvfree(inodes);
 	kfree(loi);
 
 	return ret;

Signature · 1 line

2.12.0

Anand Jain

Reviewed-by on v1 4/5

2017-06-01 09:33 UTC #9 lore raw

Re: [PATCH 4/5] btrfs: use GFP_KERNEL in init_ipath

Quoted 3 lines · David Sterba

Now that init_ipath is called either from a safe context or with memalloc_nofs protection, we can switch to GFP_KERNEL allocations in init_path and init_data_container.

Reviewed-by Anand Jain <[email protected]>

Thanks, Anand

Quoted 71 lines

Signed-off-by David Sterba <[email protected]>

---
 fs/btrfs/backref.c | 10 +++++-----
 fs/btrfs/ioctl.c   |  4 ++--
 2 files changed, 7 insertions(+), 7 deletions(-)

fs/btrfs/backref.c +5 -5

index 24865da63d8f..f723c11bb763 100644
@@ -16,7 +16,7 @@
  * Boston, MA 021110-1307, USA.
  */
 
-#include <linux/vmalloc.h>
+#include <linux/mm.h>
 #include <linux/rbtree.h>
 #include "ctree.h"
 #include "disk-io.h"

@@ -2305,7 +2305,7 @@ struct btrfs_data_container *init_data_container(u32 total_bytes)
 	size_t alloc_bytes;
 
 	alloc_bytes = max_t(size_t, total_bytes, sizeof(*data));
-	data = vmalloc(alloc_bytes);
+	data = kvmalloc(alloc_bytes, GFP_KERNEL);
 	if (!data)
 		return ERR_PTR(-ENOMEM);

@@ -2339,9 +2339,9 @@ struct inode_fs_paths *init_ipath(s32 total_bytes, struct btrfs_root *fs_root,
 	if (IS_ERR(fspath))
 		return (void *)fspath;
 
-	ifp = kmalloc(sizeof(*ifp), GFP_NOFS);
+	ifp = kmalloc(sizeof(*ifp), GFP_KERNEL);
 	if (!ifp) {
-		vfree(fspath);
+		kvfree(fspath);
 		return ERR_PTR(-ENOMEM);
 	}

@@ -2356,6 +2356,6 @@ void free_ipath(struct inode_fs_paths *ipath)
 {
 	if (!ipath)
 		return;
-	vfree(ipath->fspath);
+	kvfree(ipath->fspath);
 	kfree(ipath);
 }

fs/btrfs/ioctl.c +2 -2

index c9cdea8061bc..e4116f9248c2 100644
@@ -37,7 +37,7 @@
 #include <linux/bit_spinlock.h>
 #include <linux/security.h>
 #include <linux/xattr.h>
-#include <linux/vmalloc.h>
+#include <linux/mm.h>
 #include <linux/slab.h>
 #include <linux/blkdev.h>
 #include <linux/uuid.h>

@@ -4588,7 +4588,7 @@ static long btrfs_ioctl_logical_to_ino(struct btrfs_fs_info *fs_info,
 
 out:
 	btrfs_free_path(path);
-	vfree(inodes);
+	kvfree(inodes);
 	kfree(loi);
 
 	return ret;

David Sterba

author maintainer

2017-05-31 17:50 UTC #10 lore raw

PATCH 5/5 btrfs: adjust includes after vmalloc removal

In reply to David Sterba

As we don't use vmalloc/vzalloc/vfree directly in ctree.c, we can now use the proper header that defines kvmalloc.

Signed-off-by David Sterba <[email protected]>

---
 fs/btrfs/ctree.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

fs/btrfs/ctree.c +1 -1

index 6e1b02dd72d3..3f4daa9d6e2c 100644
@@ -19,7 +19,7 @@
 #include <linux/sched.h>
 #include <linux/slab.h>
 #include <linux/rbtree.h>
-#include <linux/vmalloc.h>
+#include <linux/mm.h>
 #include "ctree.h"
 #include "disk-io.h"
 #include "transaction.h"

Signature · 1 line

2.12.0