Re: [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 <a...@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