[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 <dhavale at 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,