[PATCH] erofs-utils: simplify file handling

[Gao Xiang]

Hi Noboru,

On 2024/4/30 14:37, Noboru Asai wrote:
> Opening files again when data compression doesn't save space,
> simplify file handling.
> 
> * remove dup and lseek.
> * call pthread_cond_signal once per file.
> 
> I think the probability of the above case occurring is a few percent.
> 
> Signed-off-by: Noboru Asai <asai at sijam.com>
> ---
>   lib/compress.c | 11 ++++++-----
>   lib/inode.c    | 24 ++++++++++++------------
>   2 files changed, 18 insertions(+), 17 deletions(-)
> 
> diff --git a/lib/compress.c b/lib/compress.c
> index 7fef698..4c7351f 100644
> --- a/lib/compress.c
> +++ b/lib/compress.c
> @@ -1261,8 +1261,10 @@ void z_erofs_mt_workfn(struct erofs_work *work, void *tlsp)
>   out:
>   	cwork->errcode = ret;
>   	pthread_mutex_lock(&ictx->mutex);
> -	++ictx->nfini;
> -	pthread_cond_signal(&ictx->cond);
> +	if (++ictx->nfini == ictx->seg_num) {
> +		close(ictx->fd);

Thanks for the patch.

I think it's better to close fd in the main writer thread
(erofs_mt_write_compressed_file) rather than some random
compression worker.

> +		pthread_cond_signal(&ictx->cond);

Could you send this fix seperately, also I'm not sure if
it could be some benefits to merge segments in advance
(maybe in bulk) without waiting all tasks finished.

But I may not have enough time to work on this for now..
If you have some interest, I think it's worth doing..

> +	}
>   	pthread_mutex_unlock(&ictx->mutex);
>   }
>   
> @@ -1406,7 +1408,6 @@ int erofs_mt_write_compressed_file(struct z_erofs_compress_ictx *ictx)
>   			blkaddr - compressed_blocks, compressed_blocks);
>   
>   out:
> -	close(ictx->fd);
>   	free(ictx);
>   	return ret;
>   }
> @@ -1456,7 +1457,6 @@ void *erofs_begin_compressed_file(struct erofs_inode *inode, int fd, u64 fpos)
>   		ictx = malloc(sizeof(*ictx));
>   		if (!ictx)
>   			return ERR_PTR(-ENOMEM);
> -		ictx->fd = dup(fd);
>   	} else {
>   #ifdef EROFS_MT_ENABLED
>   		pthread_mutex_lock(&g_ictx.mutex);
> @@ -1466,8 +1466,8 @@ void *erofs_begin_compressed_file(struct erofs_inode *inode, int fd, u64 fpos)
>   		pthread_mutex_unlock(&g_ictx.mutex);
>   #endif
>   		ictx = &g_ictx;
> -		ictx->fd = fd;
>   	}
> +	ictx->fd = fd;
>   
>   	ictx->ccfg = &erofs_ccfg[inode->z_algorithmtype[0]];
>   	inode->z_algorithmtype[0] = ictx->ccfg->algorithmtype;
> @@ -1551,6 +1551,7 @@ int erofs_write_compressed_file(struct z_erofs_compress_ictx *ictx)
>   	init_list_head(&sctx.extents);
>   
>   	ret = z_erofs_compress_segment(&sctx, -1, blkaddr);
> +	close(ictx->fd);
>   	if (ret)
>   		goto err_free_idata;
>   
> diff --git a/lib/inode.c b/lib/inode.c
> index 44d684f..a30975b 100644
> --- a/lib/inode.c
> +++ b/lib/inode.c
> @@ -1112,27 +1112,27 @@ static void erofs_fixup_meta_blkaddr(struct erofs_inode *rootdir)
>   struct erofs_mkfs_job_ndir_ctx {
>   	struct erofs_inode *inode;
>   	void *ictx;
> -	int fd;
>   };
>   
>   static int erofs_mkfs_job_write_file(struct erofs_mkfs_job_ndir_ctx *ctx)
>   {
>   	struct erofs_inode *inode = ctx->inode;
> +	int fd;
>   	int ret;
>   
>   	if (ctx->ictx) {
>   		ret = erofs_write_compressed_file(ctx->ictx);
>   		if (ret != -ENOSPC)
> -			goto out;
> -		if (lseek(ctx->fd, 0, SEEK_SET) < 0) {
> -			ret = -errno;
> -			goto out;
> -		}
> +			return ret;
>   	}
> +
>   	/* fallback to all data uncompressed */
> -	ret = erofs_write_unencoded_file(inode, ctx->fd, 0);
> -out:
> -	close(ctx->fd);
> +	fd = open(inode->i_srcpath, O_RDONLY | O_BINARY);

At a quick glance, here we need to open i_srcpath again, I tend to
avoid it so I use dup instead.

Thanks,
Gao Xiang