[PATCH] btrfs: compression: allocate buckets with workspace

[PATCH] btrfs: compression: allocate buckets with workspace

[Rosen Penev]

Convert 3 allocations into one. Simplifies code slightly.

Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 fs/btrfs/compression.c | 18 +++++-------------
 1 file changed, 5 insertions(+), 13 deletions(-)

diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c
index ffb6b52863a7..da6749ff5924 100644
--- a/fs/btrfs/compression.c
+++ b/fs/btrfs/compression.c
@@ -650,11 +650,11 @@ struct heuristic_ws {
 	/* Partial copy of input data */
 	u8 *sample;
 	u32 sample_size;
-	/* Buckets store counters for each byte value */
-	struct bucket_item *bucket;
 	/* Sorting buffer */
 	struct bucket_item *bucket_b;
 	struct list_head list;
+	/* Buckets store counters for each byte value */
+	struct bucket_item bucket[];
 };
 
 static void free_heuristic_ws(struct list_head *ws)
@@ -664,8 +664,6 @@ static void free_heuristic_ws(struct list_head *ws)
 	workspace = list_entry(ws, struct heuristic_ws, list);
 
 	kvfree(workspace->sample);
-	kfree(workspace->bucket);
-	kfree(workspace->bucket_b);
 	kfree(workspace);
 }
 
@@ -673,22 +671,16 @@ static struct list_head *alloc_heuristic_ws(struct btrfs_fs_info *fs_info)
 {
 	struct heuristic_ws *ws;
 
-	ws = kzalloc_obj(*ws);
+	ws = kzalloc(struct_size(ws, bucket, BUCKET_SIZE * 2), GFP_KERNEL);
 	if (!ws)
 		return ERR_PTR(-ENOMEM);
 
+	ws->bucket_b = ws->bucket + BUCKET_SIZE;
+
 	ws->sample = kvmalloc(MAX_SAMPLE_SIZE, GFP_KERNEL);
 	if (!ws->sample)
 		goto fail;
 
-	ws->bucket = kzalloc_objs(*ws->bucket, BUCKET_SIZE);
-	if (!ws->bucket)
-		goto fail;
-
-	ws->bucket_b = kzalloc_objs(*ws->bucket_b, BUCKET_SIZE);
-	if (!ws->bucket_b)
-		goto fail;
-
 	INIT_LIST_HEAD(&ws->list);
 	return &ws->list;
 fail:
-- 
2.54.0

Re: [PATCH] btrfs: compression: allocate buckets with workspace

[Sun YangKai]

On 2026/6/29 08:25, Rosen Penev wrote:
> Convert 3 allocations into one. Simplifies code slightly.
> 
> Signed-off-by: Rosen Penev <rosenp@gmail.com>
> ---
>   fs/btrfs/compression.c | 18 +++++-------------
>   1 file changed, 5 insertions(+), 13 deletions(-)
> 
> diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c
> index ffb6b52863a7..da6749ff5924 100644
> --- a/fs/btrfs/compression.c
> +++ b/fs/btrfs/compression.c
> @@ -650,11 +650,11 @@ struct heuristic_ws {
>   	/* Partial copy of input data */
>   	u8 *sample;
>   	u32 sample_size;
> -	/* Buckets store counters for each byte value */
> -	struct bucket_item *bucket;
>   	/* Sorting buffer */
>   	struct bucket_item *bucket_b;
>   	struct list_head list;
> +	/* Buckets store counters for each byte value */
> +	struct bucket_item bucket[];
>   };
>   
>   static void free_heuristic_ws(struct list_head *ws)
> @@ -664,8 +664,6 @@ static void free_heuristic_ws(struct list_head *ws)
>   	workspace = list_entry(ws, struct heuristic_ws, list);
>   
>   	kvfree(workspace->sample);
> -	kfree(workspace->bucket);
> -	kfree(workspace->bucket_b);
>   	kfree(workspace);
>   }
>   
> @@ -673,22 +671,16 @@ static struct list_head *alloc_heuristic_ws(struct btrfs_fs_info *fs_info)
>   {
>   	struct heuristic_ws *ws;
>   
> -	ws = kzalloc_obj(*ws);
> +	ws = kzalloc(struct_size(ws, bucket, BUCKET_SIZE * 2), GFP_KERNEL);

It seems that size is fixed and known at compile time, and since we want 
to inline the buckets in the struct, I think we can have something like 
this to save one pointer:

struct heuristic_ws {
	...
	struct bucket_item bucket[BUCKET_SIZE];
	struct bucket_item bucket_b[BUCKET_SIZE];
}

However, each bucket array takes exactly 1024B memory. Currently we have 
a 48B allocation for the struct it self, 2 * 1024B allocation for bucket 
array, and 8192B allocation for sample. With these 2 arrays inlined, the 
struct will need 32 + 2048B allocation if my calculation is correct, 
which will goes into kmalloc-4k. Seems not good.

Thanks,
Sun YangKai

>   	if (!ws)
>   		return ERR_PTR(-ENOMEM);
>   
> +	ws->bucket_b = ws->bucket + BUCKET_SIZE;
> +
>   	ws->sample = kvmalloc(MAX_SAMPLE_SIZE, GFP_KERNEL);
>   	if (!ws->sample)
>   		goto fail;
>   
> -	ws->bucket = kzalloc_objs(*ws->bucket, BUCKET_SIZE);
> -	if (!ws->bucket)
> -		goto fail;
> -
> -	ws->bucket_b = kzalloc_objs(*ws->bucket_b, BUCKET_SIZE);
> -	if (!ws->bucket_b)
> -		goto fail;
> -
>   	INIT_LIST_HEAD(&ws->list);
>   	return &ws->list;
>   fail:

Re: [PATCH] btrfs: compression: allocate buckets with workspace

[David Sterba]

On Mon, Jun 29, 2026 at 09:54:11AM +0800, Sun YangKai wrote:
> > -	ws = kzalloc_obj(*ws);
> > +	ws = kzalloc(struct_size(ws, bucket, BUCKET_SIZE * 2), GFP_KERNEL);
> 
> It seems that size is fixed and known at compile time, and since we want 
> to inline the buckets in the struct, I think we can have something like 
> this to save one pointer:
> 
> struct heuristic_ws {
> 	...
> 	struct bucket_item bucket[BUCKET_SIZE];
> 	struct bucket_item bucket_b[BUCKET_SIZE];

I like this idea, given that we know the size at compile time.

> }
> 
> However, each bucket array takes exactly 1024B memory. Currently we have 
> a 48B allocation for the struct it self, 2 * 1024B allocation for bucket 
> array, and 8192B allocation for sample. With these 2 arrays inlined, the 
> struct will need 32 + 2048B allocation if my calculation is correct, 
> which will goes into kmalloc-4k. Seems not good.

SLUB merges same/similar sized structures for the named caches, I'm not
sure if this also works for the generic slabs. We can add the kmem cache
and so we can utilize the merging.

The sharing can be seen in /sys/kernel/slab, "ls -l | grep <SIZE>". The
size is rounded up to hw cacheline, so for the heuristic workspace it'll
end up in size 2112. I see at least one existing named slab on my
system, 'dmaengine-unmap-256'. This may not matter, but a named cache
can fix the slack space in the 4k slab.

Re: [PATCH] btrfs: compression: allocate buckets with workspace

[Sun YangKai]

On 2026/6/30 07:09, David Sterba wrote:
> On Mon, Jun 29, 2026 at 09:54:11AM +0800, Sun YangKai wrote:
>>> -	ws = kzalloc_obj(*ws);
>>> +	ws = kzalloc(struct_size(ws, bucket, BUCKET_SIZE * 2), GFP_KERNEL);
>>
>> It seems that size is fixed and known at compile time, and since we want
>> to inline the buckets in the struct, I think we can have something like
>> this to save one pointer:
>>
>> struct heuristic_ws {
>> 	...
>> 	struct bucket_item bucket[BUCKET_SIZE];
>> 	struct bucket_item bucket_b[BUCKET_SIZE];
> 
> I like this idea, given that we know the size at compile time.
> 
>> }
>>
>> However, each bucket array takes exactly 1024B memory. Currently we have
>> a 48B allocation for the struct it self, 2 * 1024B allocation for bucket
>> array, and 8192B allocation for sample. With these 2 arrays inlined, the
>> struct will need 32 + 2048B allocation if my calculation is correct,
>> which will goes into kmalloc-4k. Seems not good.
> 
> SLUB merges same/similar sized structures for the named caches, I'm not
> sure if this also works for the generic slabs. We can add the kmem cache
> and so we can utilize the merging.

Oh, I've missed that. Then things seems good for me.

> The sharing can be seen in /sys/kernel/slab, "ls -l | grep <SIZE>". The
> size is rounded up to hw cacheline, so for the heuristic workspace it'll
> end up in size 2112. I see at least one existing named slab on my
> system, 'dmaengine-unmap-256'. This may not matter, but a named cache
> can fix the slack space in the 4k slab.

And I have

❯ sudo grep 2112 /proc/slabinfo
sighand_cache        593   1335   2112   15   8

on my system :)

Thanks,
Sun YangKai

Re: [PATCH] btrfs: compression: allocate buckets with workspace

[Qu Wenruo]

在 2026/6/29 09:55, Rosen Penev 写道:
> Convert 3 allocations into one. Simplifies code slightly.
> 
> Signed-off-by: Rosen Penev <rosenp@gmail.com>
> ---
>   fs/btrfs/compression.c | 18 +++++-------------
>   1 file changed, 5 insertions(+), 13 deletions(-)
> 
> diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c
> index ffb6b52863a7..da6749ff5924 100644
> --- a/fs/btrfs/compression.c
> +++ b/fs/btrfs/compression.c
> @@ -650,11 +650,11 @@ struct heuristic_ws {
>   	/* Partial copy of input data */
>   	u8 *sample;
>   	u32 sample_size;
> -	/* Buckets store counters for each byte value */
> -	struct bucket_item *bucket;
>   	/* Sorting buffer */
>   	struct bucket_item *bucket_b;
>   	struct list_head list;
> +	/* Buckets store counters for each byte value */
> +	struct bucket_item bucket[];
>   };
>   
>   static void free_heuristic_ws(struct list_head *ws)
> @@ -664,8 +664,6 @@ static void free_heuristic_ws(struct list_head *ws)
>   	workspace = list_entry(ws, struct heuristic_ws, list);
>   
>   	kvfree(workspace->sample);
> -	kfree(workspace->bucket);
> -	kfree(workspace->bucket_b);
>   	kfree(workspace);
>   }
>   
> @@ -673,22 +671,16 @@ static struct list_head *alloc_heuristic_ws(struct btrfs_fs_info *fs_info)
>   {
>   	struct heuristic_ws *ws;
>   
> -	ws = kzalloc_obj(*ws);
> +	ws = kzalloc(struct_size(ws, bucket, BUCKET_SIZE * 2), GFP_KERNEL);
>   	if (!ws)
>   		return ERR_PTR(-ENOMEM);
>   
> +	ws->bucket_b = ws->bucket + BUCKET_SIZE;
> +

This barely improves any readability.

We have two different bucket pointers, one is a variable sized member, 
one (bucket_b) is a pointer.

Not a fan of this at all.
>   	ws->sample = kvmalloc(MAX_SAMPLE_SIZE, GFP_KERNEL);
>   	if (!ws->sample)
>   		goto fail;
>   
> -	ws->bucket = kzalloc_objs(*ws->bucket, BUCKET_SIZE);
> -	if (!ws->bucket)
> -		goto fail;
> -
> -	ws->bucket_b = kzalloc_objs(*ws->bucket_b, BUCKET_SIZE);
> -	if (!ws->bucket_b)
> -		goto fail;
> -
>   	INIT_LIST_HEAD(&ws->list);
>   	return &ws->list;
>   fail: