btrfs: Keep one more workspace around
diff mbox

Message ID 20170629175726.1246810-1-terrelln@fb.com
State New
Headers show

Commit Message

Nick Terrell June 29, 2017, 5:57 p.m. UTC
find_workspace() allocates up to num_online_cpus() + 1 workspaces.
free_workspace() will only keep num_online_cpus() workspaces. When
(de)compressing we will allocate num_online_cpus() + 1 workspaces, then
free one, and repeat. Instead, we can just keep num_online_cpus() + 1
workspaces around, and never have to allocate/free another workspace in the
common case.

I tested on a Ubuntu 14.04 VM with 2 cores and 4 GiB of RAM. I mounted a
BtrFS partition with -o compress-force={lzo,zlib,zstd} and logged whenever
a workspace was allocated of freed. Then I copied vmlinux (527 MB) to the
partition. Before the patch, during the copy it would allocate and free 5-6
workspaces. After, it only allocated the initial 3. This held true for lzo,
zlib, and zstd. The time it took to execute cp vmlinux /mnt/btrfs && sync
dropped from 1.70s to 1.44s with lzo compression, and from 2.04s to 1.80s
for zstd compression.

Signed-off-by: Nick Terrell <terrelln@fb.com>
---
 fs/btrfs/compression.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--
2.9.3
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Adam Borowski June 30, 2017, 12:14 a.m. UTC | #1
On Thu, Jun 29, 2017 at 10:57:26AM -0700, Nick Terrell wrote:
> The time it took to execute cp vmlinux /mnt/btrfs && sync
> dropped from 1.70s to 1.44s with lzo compression, and from 2.04s to 1.80s
> for zstd compression.

> -	if (*free_ws < num_online_cpus()) {
> +	if (*free_ws <= num_online_cpus()) {

A simple, self-contained, one-character fix that gives a nice speed-up.
What about getting this for 4.13 or perhaps even 4.9?

Workspace flickering is not a very serious bug, but if we can restore wasted
write speed _this_ easily...
Omar Sandoval June 30, 2017, 12:49 a.m. UTC | #2
On Thu, Jun 29, 2017 at 10:57:26AM -0700, Nick Terrell wrote:
> find_workspace() allocates up to num_online_cpus() + 1 workspaces.
> free_workspace() will only keep num_online_cpus() workspaces. When
> (de)compressing we will allocate num_online_cpus() + 1 workspaces, then
> free one, and repeat. Instead, we can just keep num_online_cpus() + 1
> workspaces around, and never have to allocate/free another workspace in the
> common case.
> 
> I tested on a Ubuntu 14.04 VM with 2 cores and 4 GiB of RAM. I mounted a
> BtrFS partition with -o compress-force={lzo,zlib,zstd} and logged whenever
> a workspace was allocated of freed. Then I copied vmlinux (527 MB) to the
> partition. Before the patch, during the copy it would allocate and free 5-6
> workspaces. After, it only allocated the initial 3. This held true for lzo,
> zlib, and zstd. The time it took to execute cp vmlinux /mnt/btrfs && sync
> dropped from 1.70s to 1.44s with lzo compression, and from 2.04s to 1.80s
> for zstd compression.

Good catch! It seems to me like it might be easier to just allocate them
all upfront anyways, but that's a battle for another day.

Reviewed-by: Omar Sandoval <osandov@fb.com>

> Signed-off-by: Nick Terrell <terrelln@fb.com>
> ---
>  fs/btrfs/compression.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c
> index 3beb0d0..1a0ef55 100644
> --- a/fs/btrfs/compression.c
> +++ b/fs/btrfs/compression.c
> @@ -874,7 +874,7 @@ static void free_workspace(int type, struct list_head *workspace)
>  	int *free_ws			= &btrfs_comp_ws[idx].free_ws;
> 
>  	spin_lock(ws_lock);
> -	if (*free_ws < num_online_cpus()) {
> +	if (*free_ws <= num_online_cpus()) {
>  		list_add(workspace, idle_ws);
>  		(*free_ws)++;
>  		spin_unlock(ws_lock);
> --
> 2.9.3
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch
diff mbox

diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c
index 3beb0d0..1a0ef55 100644
--- a/fs/btrfs/compression.c
+++ b/fs/btrfs/compression.c
@@ -874,7 +874,7 @@  static void free_workspace(int type, struct list_head *workspace)
 	int *free_ws			= &btrfs_comp_ws[idx].free_ws;

 	spin_lock(ws_lock);
-	if (*free_ws < num_online_cpus()) {
+	if (*free_ws <= num_online_cpus()) {
 		list_add(workspace, idle_ws);
 		(*free_ws)++;
 		spin_unlock(ws_lock);