[PATCH 0/3] Update gfp flags in alloc_workspace

[PATCH 0/3] Update gfp flags in alloc_workspace

[David Sterba]

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(-)

-- 
2.12.0

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

[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.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/compression.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c
index ba511dd454d5..39cd164e5a62 100644
--- a/fs/btrfs/compression.c
+++ b/fs/btrfs/compression.c
@@ -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);
-- 
2.12.0

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

[Anand Jain]

On 05/31/17 23:41, David Sterba wrote:
> 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 <anand.jain@oracle.com>

Thanks, Anand

> Signed-off-by: David Sterba <dsterba@suse.com>
> ---
>  fs/btrfs/compression.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c
> index ba511dd454d5..39cd164e5a62 100644
> --- a/fs/btrfs/compression.c
> +++ b/fs/btrfs/compression.c
> @@ -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);
>

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

[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 <dsterba@suse.com>
---
 fs/btrfs/lzo.c  | 2 +-
 fs/btrfs/zlib.c | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/lzo.c b/fs/btrfs/lzo.c
index a554856a5f8a..c556f3f3fbf0 100644
--- a/fs/btrfs/lzo.c
+++ b/fs/btrfs/lzo.c
@@ -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);
 
diff --git a/fs/btrfs/zlib.c b/fs/btrfs/zlib.c
index d5446e18bb59..c1db7572283b 100644
--- a/fs/btrfs/zlib.c
+++ b/fs/btrfs/zlib.c
@@ -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;
 
-- 
2.12.0

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

[Anand Jain]

On 05/31/17 23:41, David Sterba wrote:
> 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 <dsterba@suse.com>


Reviewed-by: Anand Jain <anand.jain@oracle.com>

Thanks, Anand

> ---
>  fs/btrfs/lzo.c  | 2 +-
>  fs/btrfs/zlib.c | 4 ++--
>  2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/fs/btrfs/lzo.c b/fs/btrfs/lzo.c
> index a554856a5f8a..c556f3f3fbf0 100644
> --- a/fs/btrfs/lzo.c
> +++ b/fs/btrfs/lzo.c
> @@ -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);
>
> diff --git a/fs/btrfs/zlib.c b/fs/btrfs/zlib.c
> index d5446e18bb59..c1db7572283b 100644
> --- a/fs/btrfs/zlib.c
> +++ b/fs/btrfs/zlib.c
> @@ -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;
>
>

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

[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 <dsterba@suse.com>
---
 fs/btrfs/lzo.c  | 14 +++++++-------
 fs/btrfs/zlib.c |  6 +++---
 2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/fs/btrfs/lzo.c b/fs/btrfs/lzo.c
index c556f3f3fbf0..cde13cce01a0 100644
--- a/fs/btrfs/lzo.c
+++ b/fs/btrfs/lzo.c
@@ -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;
 
diff --git a/fs/btrfs/zlib.c b/fs/btrfs/zlib.c
index c1db7572283b..c248f9286366 100644
--- a/fs/btrfs/zlib.c
+++ b/fs/btrfs/zlib.c
@@ -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;
-- 
2.12.0

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

[Anand Jain]

> @@ -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

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

[David Sterba]

On Thu, Jun 01, 2017 at 11:58:31AM +0800, Anand Jain wrote:
> 
> 
> > @@ -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.

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