Update gfp flags in alloc_workspace

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

Share & tools

David Sterba

author maintainer

2017-05-31 15:42 UTC #0 lore raw

cover Update gfp flags in alloc_workspace

We can cheaply get rid of a few GFP_NOFS, as there are only two contexts, initializaton (GFP_KERNEL is ok), and potentially a "NOFS", but with memalloc_nofs it's made safe.

David Sterba (3):
btrfs: add memalloc_nofs protections around alloc_workspace callback
btrfs: switch kmallocs to GFP_KERNEL in lzo/zlib alloc_workspace
btrfs: switch to kvmalloc and GFP_KERNEL in lzo/zlib alloc_workspace

 fs/btrfs/compression.c | 10 ++++++++++
 fs/btrfs/lzo.c         | 16 ++++++++--------
 fs/btrfs/zlib.c        | 10 +++++-----
 3 files changed, 23 insertions(+), 13 deletions(-)

Signature · 1 line

2.12.0

David Sterba

author maintainer

2017-05-31 15:42 UTC #1 lore raw

PATCH 1/3 btrfs: add memalloc_nofs protections around alloc_workspace callback

The workspaces are preallocated at the beginning where we can safely use GFP_KERNEL, but in some cases the find_workspace might reach the allocation again, now in a more restricted context when the bios or pages are being compressed.

To avoid potential lockup when alloc_workspace -> vmalloc would silently use the GFP_KERNEL, add the memalloc_nofs helpers around the critical call site.

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

---
 fs/btrfs/compression.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

fs/btrfs/compression.c +10 -0

index ba511dd454d5..39cd164e5a62 100644
@@ -32,6 +32,7 @@
 #include <linux/writeback.h>
 #include <linux/bit_spinlock.h>
 #include <linux/slab.h>
+#include <linux/sched/mm.h>
 #include "ctree.h"
 #include "disk-io.h"
 #include "transaction.h"

@@ -757,6 +758,7 @@ static struct list_head *find_workspace(int type)
 	struct list_head *workspace;
 	int cpus = num_online_cpus();
 	int idx = type - 1;
+	unsigned nofs_flag;
 
 	struct list_head *idle_ws	= &btrfs_comp_ws[idx].idle_ws;
 	spinlock_t *ws_lock		= &btrfs_comp_ws[idx].ws_lock;

@@ -786,7 +788,15 @@ static struct list_head *find_workspace(int type)
 	atomic_inc(total_ws);
 	spin_unlock(ws_lock);
 
+	/*
+	 * Allocation helpers call vmalloc that can't use GFP_NOFS, so we have
+	 * to turn it off here because we might get called from the restricted
+	 * context of btrfs_compress_bio/btrfs_compress_pages
+	 */
+	nofs_flag = memalloc_nofs_save();
 	workspace = btrfs_compress_op[idx]->alloc_workspace();
+	memalloc_nofs_restore(nofs_flag);
+
 	if (IS_ERR(workspace)) {
 		atomic_dec(total_ws);
 		wake_up(ws_wait);

Signature · 1 line

2.12.0

Anand Jain

Reviewed-by on v1 1/3

2017-06-01 03:30 UTC #2 lore raw

Re: [PATCH 1/3] btrfs: add memalloc_nofs protections around alloc_workspace callback

Quoted 8 lines · David Sterba

The workspaces are preallocated at the beginning where we can safely use GFP_KERNEL, but in some cases the find_workspace might reach the allocation again, now in a more restricted context when the bios or pages are being compressed.
To avoid potential lockup when alloc_workspace -> vmalloc would silently use the GFP_KERNEL, add the memalloc_nofs helpers around the critical call site.

Reviewed-by Anand Jain <[email protected]>

Thanks, Anand

Quoted 42 lines

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

---
 fs/btrfs/compression.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

fs/btrfs/compression.c +10 -0

index ba511dd454d5..39cd164e5a62 100644
@@ -32,6 +32,7 @@
 #include <linux/writeback.h>
 #include <linux/bit_spinlock.h>
 #include <linux/slab.h>
+#include <linux/sched/mm.h>
 #include "ctree.h"
 #include "disk-io.h"
 #include "transaction.h"

@@ -757,6 +758,7 @@ static struct list_head *find_workspace(int type)
 	struct list_head *workspace;
 	int cpus = num_online_cpus();
 	int idx = type - 1;
+	unsigned nofs_flag;
 
 	struct list_head *idle_ws	= &btrfs_comp_ws[idx].idle_ws;
 	spinlock_t *ws_lock		= &btrfs_comp_ws[idx].ws_lock;

@@ -786,7 +788,15 @@ static struct list_head *find_workspace(int type)
 	atomic_inc(total_ws);
 	spin_unlock(ws_lock);
 
+	/*
+	 * Allocation helpers call vmalloc that can't use GFP_NOFS, so we have
+	 * to turn it off here because we might get called from the restricted
+	 * context of btrfs_compress_bio/btrfs_compress_pages
+	 */
+	nofs_flag = memalloc_nofs_save();
 	workspace = btrfs_compress_op[idx]->alloc_workspace();
+	memalloc_nofs_restore(nofs_flag);
+
 	if (IS_ERR(workspace)) {
 		atomic_dec(total_ws);
 		wake_up(ws_wait);

David Sterba

author maintainer

2017-05-31 15:42 UTC #3 lore raw

PATCH 2/3 btrfs: switch kmallocs to GFP_KERNEL in lzo/zlib alloc_workspace

In reply to David Sterba

As alloc_workspace is now protected by memalloc_nofs where needed, we can switch the kmalloc to use GFP_KERNEL.

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

---
 fs/btrfs/lzo.c  | 2 +-
 fs/btrfs/zlib.c | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

fs/btrfs/lzo.c +1 -1

index a554856a5f8a..c556f3f3fbf0 100644
@@ -51,7 +51,7 @@ static struct list_head *lzo_alloc_workspace(void)
 {
 	struct workspace *workspace;
 
-	workspace = kzalloc(sizeof(*workspace), GFP_NOFS);
+	workspace = kzalloc(sizeof(*workspace), GFP_KERNEL);
 	if (!workspace)
 		return ERR_PTR(-ENOMEM);

fs/btrfs/zlib.c +2 -2

index d5446e18bb59..c1db7572283b 100644
@@ -53,14 +53,14 @@ static struct list_head *zlib_alloc_workspace(void)
 	struct workspace *workspace;
 	int workspacesize;
 
-	workspace = kzalloc(sizeof(*workspace), GFP_NOFS);
+	workspace = kzalloc(sizeof(*workspace), GFP_KERNEL);
 	if (!workspace)
 		return ERR_PTR(-ENOMEM);
 
 	workspacesize = max(zlib_deflate_workspacesize(MAX_WBITS, MAX_MEM_LEVEL),
 			zlib_inflate_workspacesize());
 	workspace->strm.workspace = vmalloc(workspacesize);
-	workspace->buf = kmalloc(PAGE_SIZE, GFP_NOFS);
+	workspace->buf = kmalloc(PAGE_SIZE, GFP_KERNEL);
 	if (!workspace->strm.workspace || !workspace->buf)
 		goto fail;

Signature · 1 line

2.12.0

Anand Jain

Reviewed-by on v1 2/3

2017-06-01 03:30 UTC #4 lore raw

Re: [PATCH 2/3] btrfs: switch kmallocs to GFP_KERNEL in lzo/zlib alloc_workspace

Quoted 4 lines · David Sterba

As alloc_workspace is now protected by memalloc_nofs where needed, we can switch the kmalloc to use GFP_KERNEL.
Signed-off-by David Sterba <[email protected]>

Reviewed-by Anand Jain <[email protected]>

Thanks, Anand

Quoted 40 lines

---
 fs/btrfs/lzo.c  | 2 +-
 fs/btrfs/zlib.c | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

fs/btrfs/lzo.c +1 -1

index a554856a5f8a..c556f3f3fbf0 100644
@@ -51,7 +51,7 @@ static struct list_head *lzo_alloc_workspace(void)
 {
 	struct workspace *workspace;
 
-	workspace = kzalloc(sizeof(*workspace), GFP_NOFS);
+	workspace = kzalloc(sizeof(*workspace), GFP_KERNEL);
 	if (!workspace)
 		return ERR_PTR(-ENOMEM);

fs/btrfs/zlib.c +2 -2

index d5446e18bb59..c1db7572283b 100644
@@ -53,14 +53,14 @@ static struct list_head *zlib_alloc_workspace(void)
 	struct workspace *workspace;
 	int workspacesize;
 
-	workspace = kzalloc(sizeof(*workspace), GFP_NOFS);
+	workspace = kzalloc(sizeof(*workspace), GFP_KERNEL);
 	if (!workspace)
 		return ERR_PTR(-ENOMEM);
 
 	workspacesize = max(zlib_deflate_workspacesize(MAX_WBITS, MAX_MEM_LEVEL),
 			zlib_inflate_workspacesize());
 	workspace->strm.workspace = vmalloc(workspacesize);
-	workspace->buf = kmalloc(PAGE_SIZE, GFP_NOFS);
+	workspace->buf = kmalloc(PAGE_SIZE, GFP_KERNEL);
 	if (!workspace->strm.workspace || !workspace->buf)
 		goto fail;

David Sterba

author maintainer

2017-05-31 15:42 UTC #5 lore raw

PATCH 3/3 btrfs: switch to kvmalloc and GFP_KERNEL in lzo/zlib alloc_workspace

In reply to David Sterba

The compression workspace buffers are larger than a page so we use vmalloc, unconditionally. This is not always necessary as there might be contiguous memory available.

Let's use the kvmalloc helpers that will try kmalloc first and fallback to vmalloc. For that they require GFP_KERNEL flags. As we now have the alloc_workspace calls protected by memalloc_nofs in the critical contexts, we can safely use GFP_KERNEL.

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

---
 fs/btrfs/lzo.c  | 14 +++++++-------
 fs/btrfs/zlib.c |  6 +++---
 2 files changed, 10 insertions(+), 10 deletions(-)

fs/btrfs/lzo.c +7 -7

index c556f3f3fbf0..cde13cce01a0 100644
@@ -18,7 +18,7 @@
 
 #include <linux/kernel.h>
 #include <linux/slab.h>
-#include <linux/vmalloc.h>
+#include <linux/mm.h>
 #include <linux/init.h>
 #include <linux/err.h>
 #include <linux/sched.h>

@@ -41,9 +41,9 @@ static void lzo_free_workspace(struct list_head *ws)
 {
 	struct workspace *workspace = list_entry(ws, struct workspace, list);
 
-	vfree(workspace->buf);
-	vfree(workspace->cbuf);
-	vfree(workspace->mem);
+	kvfree(workspace->buf);
+	kvfree(workspace->cbuf);
+	kvfree(workspace->mem);
 	kfree(workspace);
 }

@@ -55,9 +55,9 @@ static struct list_head *lzo_alloc_workspace(void)
 	if (!workspace)
 		return ERR_PTR(-ENOMEM);
 
-	workspace->mem = vmalloc(LZO1X_MEM_COMPRESS);
-	workspace->buf = vmalloc(lzo1x_worst_compress(PAGE_SIZE));
-	workspace->cbuf = vmalloc(lzo1x_worst_compress(PAGE_SIZE));
+	workspace->mem = kvmalloc(LZO1X_MEM_COMPRESS, GFP_KERNEL);
+	workspace->buf = kvmalloc(lzo1x_worst_compress(PAGE_SIZE), GFP_KERNEL);
+	workspace->cbuf = kvmalloc(lzo1x_worst_compress(PAGE_SIZE), GFP_KERNEL);
 	if (!workspace->mem || !workspace->buf || !workspace->cbuf)
 		goto fail;

fs/btrfs/zlib.c +3 -3

index c1db7572283b..c248f9286366 100644
@@ -24,7 +24,7 @@
 #include <linux/slab.h>
 #include <linux/zlib.h>
 #include <linux/zutil.h>
-#include <linux/vmalloc.h>
+#include <linux/mm.h>
 #include <linux/init.h>
 #include <linux/err.h>
 #include <linux/sched.h>

@@ -43,7 +43,7 @@ static void zlib_free_workspace(struct list_head *ws)
 {
 	struct workspace *workspace = list_entry(ws, struct workspace, list);
 
-	vfree(workspace->strm.workspace);
+	kvfree(workspace->strm.workspace);
 	kfree(workspace->buf);
 	kfree(workspace);
 }

@@ -59,7 +59,7 @@ static struct list_head *zlib_alloc_workspace(void)
 
 	workspacesize = max(zlib_deflate_workspacesize(MAX_WBITS, MAX_MEM_LEVEL),
 			zlib_inflate_workspacesize());
-	workspace->strm.workspace = vmalloc(workspacesize);
+	workspace->strm.workspace = kvmalloc(workspacesize, GFP_KERNEL);
 	workspace->buf = kmalloc(PAGE_SIZE, GFP_KERNEL);
 	if (!workspace->strm.workspace || !workspace->buf)
 		goto fail;

Signature · 1 line

2.12.0

Anand Jain

2017-06-01 03:52 UTC #6 lore raw

Re: [PATCH 3/3] btrfs: switch to kvmalloc and GFP_KERNEL in lzo/zlib alloc_workspace

fs/btrfs/zlib.c static struct list_head *zlib_alloc_workspace(void) · line 63

Quoted 9 lines

fs/btrfs/zlib.c +1 -1

@@ -59,7 +59,7 @@ static struct list_head *zlib_alloc_workspace(void)
 
 	workspacesize = max(zlib_deflate_workspacesize(MAX_WBITS, MAX_MEM_LEVEL),
 			zlib_inflate_workspacesize());
-	workspace->strm.workspace = vmalloc(workspacesize);
+	workspace->strm.workspace = kvmalloc(workspacesize, GFP_KERNEL);
 	workspace->buf = kmalloc(PAGE_SIZE, GFP_KERNEL);

Workspace->buf can be a kvmalloc as well as in lzo? or I don't know if there was any purpose for not doing it for zlib.

Thanks, Anand

David Sterba

2017-06-01 14:25 UTC #7 lore raw

Re: [PATCH 3/3] btrfs: switch to kvmalloc and GFP_KERNEL in lzo/zlib alloc_workspace

Quoted 4 lines · Anand Jain

Quoted 4 lines

(quoted diff) +0 -0

@@ -59,7 +59,7 @@ static struct list_head *zlib_alloc_workspace(void)

Quoted 8 lines

fs/btrfs/zlib.c static struct list_head *zlib_alloc_workspace(void) · line 63
Quoted 5 lines
fs/btrfs/zlib.c +1 -1
 	workspacesize = max(zlib_deflate_workspacesize(MAX_WBITS, MAX_MEM_LEVEL),
 			zlib_inflate_workspacesize());
-	workspace->strm.workspace = vmalloc(workspacesize);
+	workspace->strm.workspace = kvmalloc(workspacesize, GFP_KERNEL);
 	workspace->buf = kmalloc(PAGE_SIZE, GFP_KERNEL);
Workspace->buf can be a kvmalloc as well as in lzo? or I don't know if there was any purpose for not doing it for zlib.

Zlib is fine with PAGE_SIZE. Lzo needs potentially more than a page, and kvmalloc does not fallback to vmalloc in that case:

http://elixir.free-electrons.com/linux/v4.12-rc3/source/mm/util.c#L378