Re: [PATCH 1/2 v2] fsck.erofs: add multi-threaded decompression

[Gao Xiang <xiang@kernel.org>]

On Mon, Jun 29, 2026 at 03:25:29PM +0530, Nithurshen wrote:
> Currently, fsck.erofs extracts files synchronously. When decompressing
> heavily packed images, the main thread spends the majority of its time
> blocked on a combination of synchronous vfs_write() syscalls and
> decompression routines, bottlenecking overall extraction speed.
> 
> This patch introduces a scalable, multi-threaded decompression framework
> using the existing erofs_workqueue infrastructure to decouple compute
> from the main thread's I/O.
> 
> To prevent massive scheduling overhead (futex contention) where worker
> threads spend more CPU time waking up than actually decompressing small
> clusters, this implementation introduces a batching context. Because
> different compression algorithms exhibit vastly different scheduling
> thresholds, the batch size is algorithm-aware:
> - Fast algorithms like LZ4 utilize a larger batch limit (up to 32
>   pclusters) to effectively hide synchronization overhead.
> - Compute-heavy algorithms like LZMA or ZSTD trigger at a lower
>   threshold (8 pclusters) to prevent memory bloat and thread starvation.
> 
> Key details of this implementation:
> - The worker pool is dynamically sized based on available system CPUs.
> - Decompression tasks take strict ownership of the raw and output
>   buffers (safely tracking memory via a `free_out` flag) to prevent
>   data races and memory leaks.
> - Output buffers are explicitly zero-initialized via calloc() to
>   prevent trailing garbage bytes from leaking into extracted files.
> - Tail-end packed fragments are processed synchronously by the main
>   thread, as their minimal overhead does not benefit from asynchronous
>   offloading.
> 
> Signed-off-by: Nithurshen <nithurshen.dev@gmail.com>
> ---
>  fsck/main.c              | 122 +++++++------------
>  include/erofs/cond.h     |  31 +++++
>  include/erofs/internal.h |  15 ++-
>  include/erofs/lock.h     |   3 +
>  lib/data.c               | 254 +++++++++++++++++++++++++++++++--------
>  lib/global.c             |  15 +++
>  6 files changed, 307 insertions(+), 133 deletions(-)
>  create mode 100644 include/erofs/cond.h
> 
> diff --git a/fsck/main.c b/fsck/main.c
> index 16cc627..c427a70 100644
> --- a/fsck/main.c
> +++ b/fsck/main.c
> @@ -8,11 +8,13 @@
>  #include <time.h>
>  #include <utime.h>
>  #include <unistd.h>
> +#include "erofs/lock.h"
>  #include <sys/stat.h>
>  #include "erofs/print.h"
>  #include "erofs/decompress.h"
>  #include "erofs/dir.h"
>  #include "erofs/xattr.h"
> +#include "erofs/workqueue.h"
>  #include "../lib/compressor.h"
>  #include "../lib/liberofs_compress.h"
>  
> @@ -509,40 +511,31 @@ static int erofs_verify_inode_data(struct erofs_inode *inode, int outfd)
>  		.buf = __EROFS_BUF_INITIALIZER,
>  	};
>  	bool needdecode = fsckcfg.check_decomp && !erofs_is_packed_inode(inode);
> -	int ret = 0;
> -	bool compressed;
> +	int ret = 0, wait_err;
> +	bool compressed = erofs_inode_is_data_compressed(inode->datalayout);
>  	erofs_off_t pos = 0;
>  	u64 pchunk_len = 0;
> -	unsigned int raw_size = 0, buffer_size = 0;
> -	char *raw = NULL, *buffer = NULL;
> +	struct z_erofs_mt_read_ctx *ctx;
> +
> +	ctx = z_erofs_mt_read_ctx_alloc(outfd, true);
> +	if (!ctx)
> +		return -ENOMEM;
>  
>  	erofs_dbg("verify data chunk of nid(%llu): type(%d)",
> -		  inode->nid | 0ULL, inode->datalayout);
> +		inode->nid | 0ULL, inode->datalayout);

I'm still not sure why this line needs to be changed.

>  
> -	compressed = erofs_inode_is_data_compressed(inode->datalayout);
>  	while (pos < inode->i_size) {
> -		unsigned int alloc_rawsize;
> -
>  		map.m_la = pos;
>  		ret = erofs_map_blocks(inode, &map, EROFS_GET_BLOCKS_FIEMAP);
>  		if (ret)
>  			goto out;
>  
> -		if (!compressed && map.m_llen != map.m_plen) {
> -			erofs_err("broken chunk length m_la %" PRIu64 " m_llen %" PRIu64 " m_plen %" PRIu64,
> -				  map.m_la, map.m_llen, map.m_plen);
> -			ret = -EFSCORRUPTED;
> -			goto out;

Where is the check now?

> -		}
> -
> -		/* the last lcluster can be divided into 3 parts */
>  		if (map.m_la + map.m_llen > inode->i_size)
>  			map.m_llen = inode->i_size - map.m_la;
>  
>  		pchunk_len += map.m_plen;
>  		pos += map.m_llen;
>  
> -		/* should skip decomp? */
>  		if (map.m_la >= inode->i_size || !needdecode)
>  			continue;
>  
> @@ -555,85 +548,53 @@ static int erofs_verify_inode_data(struct erofs_inode *inode, int outfd)
>  			continue;
>  		}
>  
> -		if (map.m_plen > Z_EROFS_PCLUSTER_MAX_SIZE) {
> -			if (compressed && !(map.m_flags & __EROFS_MAP_FRAGMENT)) {
> -				erofs_err("invalid pcluster size %" PRIu64 " @ offset %" PRIu64 " of nid %" PRIu64,
> -					  map.m_plen, map.m_la,
> -					  inode->nid | 0ULL);
> -				ret = -EFSCORRUPTED;
> -				goto out;
> -			}

Where is the check now?

> -			alloc_rawsize = Z_EROFS_PCLUSTER_MAX_SIZE;
> -		} else {
> -			alloc_rawsize = map.m_plen;
> -		}
> -
> -		if (alloc_rawsize > raw_size) {
> -			char *newraw = realloc(raw, alloc_rawsize);
> -
> -			if (!newraw) {
> +		if (compressed) {
> +			size_t buffer_size = map.m_llen > erofs_blksiz(inode->sbi) ? 
> +				map.m_llen : erofs_blksiz(inode->sbi);

Can you explain why?

> +			char *raw, *buffer;
> +			raw = malloc(map.m_plen);
> +			buffer = calloc(1, buffer_size);
> +			
> +			if (!raw || !buffer) {
> +				free(raw);
> +				free(buffer);
>  				ret = -ENOMEM;
>  				goto out;
>  			}
> -			raw = newraw;
> -			raw_size = alloc_rawsize;
> -		}
>  
> -		if (compressed) {
> -			if (map.m_llen > buffer_size) {
> -				char *newbuffer;
> -
> -				buffer_size = map.m_llen;
> -				newbuffer = realloc(buffer, buffer_size);
> -				if (!newbuffer) {
> -					ret = -ENOMEM;
> -					goto out;
> -				}
> -				buffer = newbuffer;
> +			ret = z_erofs_read_one_data(inode, &map, raw, buffer, 0, map.m_llen, 
> +				false, map.m_la, ctx);
> +			if (ret) {
> +				goto out;
>  			}
> -			ret = z_erofs_read_one_data(inode, &map, raw, buffer,
> -						    0, map.m_llen, false);
> +		} else {
> +			char *raw = calloc(1, map.m_llen);
> +			ret = erofs_read_one_data(inode, &map, raw, 0, map.m_llen);
> +			if (ret >= 0 && outfd >= 0)
> +				pwrite(outfd, raw, map.m_llen, map.m_la);
> +			free(raw);
>  			if (ret)
>  				goto out;
> -
> -			if (outfd >= 0 && write(outfd, buffer, map.m_llen) < 0)
> -				goto fail_eio;
> -		} else {
> -			u64 p = 0;
> -
> -			do {
> -				u64 count = min_t(u64, alloc_rawsize,
> -						  map.m_llen);
> -
> -				ret = erofs_read_one_data(inode, &map, raw, p, count);
> -				if (ret)
> -					goto out;
> -
> -				if (outfd >= 0 && write(outfd, raw, count) < 0)
> -					goto fail_eio;
> -				map.m_llen -= count;
> -				p += count;
> -			} while (map.m_llen);
>  		}
>  	}
> +	z_erofs_mt_read_enqueue(ctx);
> +
> +out:
> +	wait_err = z_erofs_mt_read_ctx_wait(ctx);
> +	if (wait_err < 0 && ret >= 0)
> +		ret = wait_err;
>  
>  	if (fsckcfg.print_comp_ratio) {
>  		if (!erofs_is_packed_inode(inode))
>  			fsckcfg.logical_blocks += BLK_ROUND_UP(inode->sbi, inode->i_size);
>  		fsckcfg.physical_blocks += BLK_ROUND_UP(inode->sbi, pchunk_len);
>  	}
> -out:
> -	if (raw)
> -		free(raw);
> -	if (buffer)
> -		free(buffer);
> -	return ret < 0 ? ret : 0;
>  
> -fail_eio:
> -	erofs_err("I/O error occurred when verifying data chunk @ nid %llu",
> -		  inode->nid | 0ULL);
> -	ret = -EIO;
> -	goto out;
> +	if (outfd >= 0 && ret >= 0)
> +		ftruncate(outfd, inode->i_size);

Why ftruncate here?


In short, it seems that you remove a lot sanity checks, if you
touch the existing logic, please explain why it's safe to remove
them.

Also please show valid benchmark results (at least with enough
sizes, and the decompression should take many seconds), so I can
imagine it's a good improvement.

Thanks,
Gao Xiang