Re: [PATCH] btrfs: move some zstd work data from stack to workspace

[David Sterba]

On Thu, Nov 16, 2017 at 01:07:19AM +, Nick Terrell wrote:
> On 11/15/17, 9:30 AM, "David Sterba"  wrote:
> > * ZSTD_inBuffer in_buf
> > * ZSTD_outBuffer out_buf
> >
> > are used in all functions to pass the compression parameters and the
> > local variables consume some space. We can move them to the workspace
> > and reduce the stack consumption:
> >
> > zstd.c:zstd_decompress-24 (136 -> 112)
> > zstd.c:zstd_decompress_bio-24 (144 -> 120)
> > zstd.c:zstd_compress_pages-24 (264 -> 240)
> 
> It looks good to me, and I ran my btrfs zstd compression and
> decompression test and everything worked.
> 
> Is there a case where these 24 bytes matter, or is this just an easy
> optimization?

The stacks in kernel are limited, so it's a good practice to keep them
minimal. The size used to be 4kb, 8kb and now is 16kb. Using several
IO/FS layers (DM targets like crypto/raids/integrity, NFS, ecryptfs,
iscsi, ...) can increase the stack consumption and potentially overflow
under some conditions. The compression can be typically called from a
writeout path so all the layers can be active at that point and the
overflow could happen. Also debugging features can increase the stack
consumption even without the layering fun.

The maximal stack consumption can be measured by
CONFIG_DEBUG_STACK_USAGE, the values that I've seen on my dev boxes were
near the 8kb boundary, so we'd be in a much worse situation without the
16k stacks.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] btrfs: move some zstd work data from stack to workspace

[Nick Terrell]

On 11/15/17, 9:30 AM, "David Sterba"  wrote:
> * ZSTD_inBuffer in_buf
> * ZSTD_outBuffer out_buf
>
> are used in all functions to pass the compression parameters and the
> local variables consume some space. We can move them to the workspace
> and reduce the stack consumption:
>
> zstd.c:zstd_decompress-24 (136 -> 112)
> zstd.c:zstd_decompress_bio-24 (144 -> 120)
> zstd.c:zstd_compress_pages-24 (264 -> 240)

It looks good to me, and I ran my btrfs zstd compression and
decompression test and everything worked.

Is there a case where these 24 bytes matter, or is this just an easy
optimization?

Reviewed-by: Nick Terrell 

[PATCH] btrfs: move some zstd work data from stack to workspace

[David Sterba]

* ZSTD_inBuffer in_buf
* ZSTD_outBuffer out_buf

are used in all functions to pass the compression parameters and the
local variables consume some space. We can move them to the workspace
and reduce the stack consumption:

zstd.c:zstd_decompress-24 (136 -> 112)
zstd.c:zstd_decompress_bio-24 (144 -> 120)
zstd.c:zstd_compress_pages-24 (264 -> 240)

Signed-off-by: David Sterba 
---
 fs/btrfs/zstd.c | 132 
 1 file changed, 67 insertions(+), 65 deletions(-)

diff --git a/fs/btrfs/zstd.c b/fs/btrfs/zstd.c
index 17f2dd8fddb8..01a4eab602a3 100644
--- a/fs/btrfs/zstd.c
+++ b/fs/btrfs/zstd.c
@@ -43,6 +43,8 @@ struct workspace {
size_t size;
char *buf;
struct list_head list;
+   ZSTD_inBuffer in_buf;
+   ZSTD_outBuffer out_buf;
 };
 
 static void zstd_free_workspace(struct list_head *ws)
@@ -94,8 +96,6 @@ static int zstd_compress_pages(struct list_head *ws,
int nr_pages = 0;
struct page *in_page = NULL;  /* The current page to read */
struct page *out_page = NULL; /* The current page to write to */
-   ZSTD_inBuffer in_buf = { NULL, 0, 0 };
-   ZSTD_outBuffer out_buf = { NULL, 0, 0 };
unsigned long tot_in = 0;
unsigned long tot_out = 0;
unsigned long len = *total_out;
@@ -118,9 +118,9 @@ static int zstd_compress_pages(struct list_head *ws,
 
/* map in the first page of input data */
in_page = find_get_page(mapping, start >> PAGE_SHIFT);
-   in_buf.src = kmap(in_page);
-   in_buf.pos = 0;
-   in_buf.size = min_t(size_t, len, PAGE_SIZE);
+   workspace->in_buf.src = kmap(in_page);
+   workspace->in_buf.pos = 0;
+   workspace->in_buf.size = min_t(size_t, len, PAGE_SIZE);
 
 
/* Allocate and map in the output buffer */
@@ -130,14 +130,15 @@ static int zstd_compress_pages(struct list_head *ws,
goto out;
}
pages[nr_pages++] = out_page;
-   out_buf.dst = kmap(out_page);
-   out_buf.pos = 0;
-   out_buf.size = min_t(size_t, max_out, PAGE_SIZE);
+   workspace->out_buf.dst = kmap(out_page);
+   workspace->out_buf.pos = 0;
+   workspace->out_buf.size = min_t(size_t, max_out, PAGE_SIZE);
 
while (1) {
size_t ret2;
 
-   ret2 = ZSTD_compressStream(stream, _buf, _buf);
+   ret2 = ZSTD_compressStream(stream, >out_buf,
+   >in_buf);
if (ZSTD_isError(ret2)) {
pr_debug("BTRFS: ZSTD_compressStream returned %d\n",
ZSTD_getErrorCode(ret2));
@@ -146,22 +147,22 @@ static int zstd_compress_pages(struct list_head *ws,
}
 
/* Check to see if we are making it bigger */
-   if (tot_in + in_buf.pos > 8192 &&
-   tot_in + in_buf.pos <
-   tot_out + out_buf.pos) {
+   if (tot_in + workspace->in_buf.pos > 8192 &&
+   tot_in + workspace->in_buf.pos <
+   tot_out + workspace->out_buf.pos) {
ret = -E2BIG;
goto out;
}
 
/* We've reached the end of our output range */
-   if (out_buf.pos >= max_out) {
-   tot_out += out_buf.pos;
+   if (workspace->out_buf.pos >= max_out) {
+   tot_out += workspace->out_buf.pos;
ret = -E2BIG;
goto out;
}
 
/* Check if we need more output space */
-   if (out_buf.pos == out_buf.size) {
+   if (workspace->out_buf.pos == workspace->out_buf.size) {
tot_out += PAGE_SIZE;
max_out -= PAGE_SIZE;
kunmap(out_page);
@@ -176,19 +177,20 @@ static int zstd_compress_pages(struct list_head *ws,
goto out;
}
pages[nr_pages++] = out_page;
-   out_buf.dst = kmap(out_page);
-   out_buf.pos = 0;
-   out_buf.size = min_t(size_t, max_out, PAGE_SIZE);
+   workspace->out_buf.dst = kmap(out_page);
+   workspace->out_buf.pos = 0;
+   workspace->out_buf.size = min_t(size_t, max_out,
+   PAGE_SIZE);
}
 
/* We've reached the end of the input */
-   if (in_buf.pos >= len) {
-   tot_in += in_buf.pos;
+   if (workspace->in_buf.pos >= len) {
+   tot_in += workspace->in_buf.pos;
break;
}
 
/* Check if we