Re: [PATCH v1 1/1] erofs-utils: lib: Fix calculation of minextblks when working with sparse files

[Gao Xiang]

Hi Sandeep,

On 2024/4/3 14:23, Sandeep Dhavale wrote:

When we work with sparse files (files with holes), we need to consider
when the contiguous data block starts after each hole to correctly calculate
minextblks so we can merge consecutive chunks later.
Now that we need to recalculate minextblks multiple places, put the logic
in helper function for avoiding repetition and easier reading.

Fixes: 7b46f7a0160a (erofs-utils: lib: merge consecutive chunks if possible)
Signed-off-by: Sandeep Dhavale <dhav...@google.com>

Thanks for catching this!

---
  lib/blobchunk.c | 28 ++++++++++++++++++++++------
  1 file changed, 22 insertions(+), 6 deletions(-)

diff --git a/lib/blobchunk.c b/lib/blobchunk.c
index ee12194..41bee19 100644
--- a/lib/blobchunk.c
+++ b/lib/blobchunk.c
@@ -223,6 +223,16 @@ out:
        return 0;
  }

+static erofs_blk_t erofs_minblks_from_interval(struct erofs_sb_info *sbi,

+                       erofs_off_t start, erofs_off_t end, erofs_blk_t cur_min)
+{
+       erofs_blk_t lb;
+       lb = lowbit((end - start) >> sbi->blkszbits);
+       if (lb && lb < cur_min)
+               return lb;
+       return cur_min;
+}

Just a minor thought, maybe

static erofs_blk_t erofs_update_minextblks(struct erofs_sb_info *sbi,
                        erofs_off_t start, erofs_off_t end, erofs_blk_t 
*minextblks)
{
        erofs_blk_t lb;

        lb = lowbit((end - start) >> sbi->blkszbits);
        if (lb && lb < *minextblks)
                *minextblks = lb;
}

could be better (or just my own perference) since no input + output on a same
variable, otherwise it looks good to me.

If you agree on this minor nit, I could revise it directly or you could send
another version.  Both are fine with me :)

Thanks,
Gao Xiang

+
  int erofs_blob_write_chunked_file(struct erofs_inode *inode, int fd,
                                  erofs_off_t startoff)
  {
@@ -231,8 +241,8 @@ int erofs_blob_write_chunked_file(struct erofs_inode 
*inode, int fd,
        unsigned int count, unit;
        struct erofs_blobchunk *chunk, *lastch;
        struct erofs_inode_chunk_index *idx;
-       erofs_off_t pos, len, chunksize;
-       erofs_blk_t lb, minextblks;
+       erofs_off_t pos, len, chunksize, interval_start;
+       erofs_blk_t minextblks;
        u8 *chunkdata;
        int ret;

@@ -267,9 +277,10 @@ int erofs_blob_write_chunked_file(struct erofs_inode *inode, int fd,

                goto err;
        }
        idx = inode->chunkindexes;
-
        lastch = NULL;
        minextblks = BLK_ROUND_UP(sbi, inode->i_size);
+       interval_start = 0;
+
        for (pos = 0; pos < inode->i_size; pos += len) {
  #ifdef SEEK_DATA
                off_t offset = lseek(fd, pos + startoff, SEEK_DATA);
@@ -294,12 +305,15 @@ int erofs_blob_write_chunked_file(struct erofs_inode 
*inode, int fd,

if (offset > pos) {

                        len = 0;
+                       minextblks = erofs_minblks_from_interval(sbi,
+                                          interval_start, pos, minextblks);
                        do {
                                *(void **)idx++ = &erofs_holechunk;
                                pos += chunksize;
                        } while (pos < offset);
                        DBG_BUGON(pos != offset);
                        lastch = NULL;
+                       interval_start = pos;
                        continue;
                }
  #endif
@@ -320,13 +334,15 @@ int erofs_blob_write_chunked_file(struct erofs_inode 
*inode, int fd,
                if (lastch && (lastch->device_id != chunk->device_id ||
                    erofs_pos(sbi, lastch->blkaddr) + lastch->chunksize !=
                    erofs_pos(sbi, chunk->blkaddr))) {
-                       lb = lowbit(pos >> sbi->blkszbits);
-                       if (lb && lb < minextblks)
-                               minextblks = lb;
+                       minextblks = erofs_minblks_from_interval(sbi,
+                                           interval_start, pos, minextblks);
+                       interval_start = pos;
                }
                *(void **)idx++ = chunk;
                lastch = chunk;
        }
+       minextblks = erofs_minblks_from_interval(sbi, interval_start, pos,
+                                       minextblks);
        inode->datalayout = EROFS_INODE_CHUNK_BASED;
        free(chunkdata);
        return erofs_blob_mergechunks(inode, chunkbits,