[PATCH 1/3] erofs-utils: determine the [un]compressed data block is inline or not early

[PATCH 1/3] erofs-utils: determine the [un]compressed data block is inline or not early

[Noboru Asai]

Introducing erofs_get_inodesize function and erofs_get_lowest_offset function,
we can determine the [un]compressed data block is inline or not before
executing z_erofs_merge_segment function. It enable the following,

* skip the redundant write for ztailpacking block.
* simplify erofs_prepare_inode_buffer function. (remove handling ENOSPC error)

Signed-off-by: Noboru Asai <asai@sijam.com>
---
 include/erofs/inode.h |   2 +
 lib/compress.c        |  11 +++--
 lib/inode.c           | 112 ++++++++++++++++++++++++++++++++++++++++--
 3 files changed, 116 insertions(+), 9 deletions(-)

diff --git a/include/erofs/inode.h b/include/erofs/inode.h
index d5a732a..fb93b81 100644
--- a/include/erofs/inode.h
+++ b/include/erofs/inode.h
@@ -40,6 +40,8 @@ int __erofs_fill_inode(struct erofs_inode *inode, struct stat *st,
 struct erofs_inode *erofs_new_inode(void);
 struct erofs_inode *erofs_mkfs_build_tree_from_path(const char *path);
 struct erofs_inode *erofs_mkfs_build_special_from_fd(int fd, const char *name);
+unsigned int erofs_get_inodesize(struct erofs_inode *inode);
+unsigned int erofs_get_lowest_offset(struct erofs_inode *inode);
 
 #ifdef __cplusplus
 }
diff --git a/lib/compress.c b/lib/compress.c
index 74c5707..dfe59da 100644
--- a/lib/compress.c
+++ b/lib/compress.c
@@ -20,6 +20,7 @@
 #include "erofs/block_list.h"
 #include "erofs/compress_hints.h"
 #include "erofs/fragments.h"
+#include "erofs/inode.h"
 #ifdef EROFS_MT_ENABLED
 #include "erofs/workqueue.h"
 #endif
@@ -530,7 +531,8 @@ static int __z_erofs_compress_one(struct z_erofs_compress_sctx *ctx,
 			e->length = len;
 			goto frag_packing;
 		}
-		if (!may_inline && len <= blksz)
+		if ((!may_inline && len <= blksz) ||
+		    (may_inline && erofs_get_lowest_offset(inode) + len > blksz))
 			goto nocompression;
 	}
 
@@ -550,7 +552,7 @@ static int __z_erofs_compress_one(struct z_erofs_compress_sctx *ctx,
 
 	/* check if there is enough gain to keep the compressed data */
 	if (ret * h->compress_threshold / 100 >= e->length) {
-		if (may_inline && len < blksz) {
+		if (may_inline && (erofs_get_lowest_offset(inode) + len < blksz)) {
 			ret = z_erofs_fill_inline_data(inode,
 					ctx->queue + ctx->head, len, true);
 		} else {
@@ -584,7 +586,8 @@ frag_packing:
 		e->raw = false;
 		ictx->fragemitted = true;
 	/* tailpcluster should be less than 1 block */
-	} else if (may_inline && len == e->length && compressedsize < blksz) {
+	} else if (may_inline && len == e->length &&
+		   (erofs_get_lowest_offset(inode) + compressedsize < blksz)) {
 		if (ctx->clusterofs + len <= blksz) {
 			inode->eof_tailraw = malloc(len);
 			if (!inode->eof_tailraw)
@@ -621,7 +624,7 @@ frag_packing:
 					&e->length, dst, &compressedsize);
 
 		e->compressedblks = BLK_ROUND_UP(sbi, compressedsize);
-		DBG_BUGON(e->compressedblks * blksz >= e->length);
+		DBG_BUGON(!may_inline && e->compressedblks * blksz >= e->length);
 
 		padding = 0;
 		tailused = compressedsize & (blksz - 1);
diff --git a/lib/inode.c b/lib/inode.c
index 7508c74..45d2fbc 100644
--- a/lib/inode.c
+++ b/lib/inode.c
@@ -440,6 +440,11 @@ static int write_uncompressed_file_from_fd(struct erofs_inode *inode, int fd)
 
 	inode->datalayout = EROFS_INODE_FLAT_INLINE;
 	nblocks = inode->i_size / erofs_blksiz(sbi);
+	inode->idata_size = inode->i_size % erofs_blksiz(sbi);
+	if (erofs_get_inodesize(inode) + inode->idata_size > erofs_blksiz(sbi)) {
+		nblocks += 1;
+		inode->idata_size = 0;
+	}
 
 	ret = __allocate_inode_bh_data(inode, nblocks, DATA);
 	if (ret)
@@ -452,7 +457,7 @@ static int write_uncompressed_file_from_fd(struct erofs_inode *inode, int fd)
 		if (ret != erofs_blksiz(sbi)) {
 			if (ret < 0)
 				return -errno;
-			return -EAGAIN;
+			memset(buf + ret, 0, erofs_blksiz(sbi) - ret);
 		}
 
 		ret = blk_write(sbi, buf, inode->u.i_blkaddr + i, 1);
@@ -461,7 +466,6 @@ static int write_uncompressed_file_from_fd(struct erofs_inode *inode, int fd)
 	}
 
 	/* read the tail-end data */
-	inode->idata_size = inode->i_size % erofs_blksiz(sbi);
 	if (inode->idata_size) {
 		inode->idata = malloc(inode->idata_size);
 		if (!inode->idata)
@@ -667,6 +671,104 @@ static int erofs_prepare_tail_block(struct erofs_inode *inode)
 	return 0;
 }
 
+static unsigned int erofs_get_compacted_extent_isize(struct erofs_inode *inode)
+{
+	struct erofs_sb_info *sbi = inode->sbi;
+	const unsigned int mpos = roundup(inode->inode_isize +
+					  inode->xattr_isize, 8) +
+				  sizeof(struct z_erofs_map_header);
+	const unsigned int totalidx = BLK_ROUND_UP(sbi, inode->i_size);
+	/* # of 8-byte units so that it can be aligned with 32 bytes */
+	unsigned int compacted_4b_initial, compacted_4b_end;
+	unsigned int compacted_2b;
+	unsigned int extent_isize = 0;
+
+	if (inode->z_advise & Z_EROFS_ADVISE_COMPACTED_2B) {
+		compacted_4b_initial = (32 - mpos % 32) / 4;
+		if (compacted_4b_initial == 32 / 4)
+			compacted_4b_initial = 0;
+
+		if (compacted_4b_initial > totalidx) {
+			compacted_4b_initial = compacted_2b = 0;
+			compacted_4b_end = totalidx;
+		} else {
+			compacted_2b = rounddown(totalidx -
+						 compacted_4b_initial, 16);
+			compacted_4b_end = totalidx - compacted_4b_initial -
+					   compacted_2b;
+		}
+	} else {
+		compacted_2b = compacted_4b_initial = 0;
+		compacted_4b_end = totalidx;
+	}
+
+	extent_isize += sizeof(struct z_erofs_map_header);
+
+	/* generate compacted_4b_initial */
+	while (compacted_4b_initial) {
+		extent_isize += 4 * 2;
+		compacted_4b_initial -= 2;
+	}
+	DBG_BUGON(compacted_4b_initial);
+
+	/* generate compacted_2b */
+	while (compacted_2b) {
+		extent_isize += 2 * 16;
+		compacted_2b -= 16;
+	}
+	DBG_BUGON(compacted_2b);
+
+	/* generate compacted_4b_end */
+	while (compacted_4b_end > 1) {
+		extent_isize += 4 * 2;
+		compacted_4b_end -= 2;
+	}
+
+	/* generate final compacted_4b_end if needed */
+	if (compacted_4b_end) {
+		extent_isize += 4 * 2;
+	}
+	return extent_isize;
+}
+
+/* datalayout may change in z_erofs_compress_dedupe function,
+ * so use carefully.
+ */
+unsigned int erofs_get_inodesize(struct erofs_inode *inode)
+{
+	struct erofs_sb_info *sbi = inode->sbi;
+	unsigned int inodesize;
+
+	inodesize = inode->inode_isize + inode->xattr_isize;
+
+	if (inode->extent_isize)
+		return roundup(inodesize, 8) + inode->extent_isize;
+
+	switch (inode->datalayout) {
+	case EROFS_INODE_COMPRESSED_FULL:
+		inodesize = roundup(inodesize, 8);
+		inodesize += BLK_ROUND_UP(sbi, inode->i_size) *
+			sizeof(struct z_erofs_lcluster_index) +
+			Z_EROFS_FULL_INDEX_ALIGN(0); /* Z_EROFS_LEGACY_MAP_HEADER_SIZE */
+		break;
+	case EROFS_INODE_COMPRESSED_COMPACT:
+		inodesize = roundup(inodesize, 8);
+		inodesize += erofs_get_compacted_extent_isize(inode);
+		break;
+	default:
+	}
+
+	return inodesize;
+}
+
+unsigned int erofs_get_lowest_offset(struct erofs_inode *inode)
+{
+	struct erofs_sb_info *sbi = inode->sbi;
+	unsigned int blksz = erofs_blksiz(sbi);
+
+	return erofs_get_inodesize(inode) & (blksz - 1);
+}
+
 static int erofs_prepare_inode_buffer(struct erofs_inode *inode)
 {
 	unsigned int inodesize;
@@ -674,9 +776,7 @@ static int erofs_prepare_inode_buffer(struct erofs_inode *inode)
 
 	DBG_BUGON(inode->bh || inode->bh_inline);
 
-	inodesize = inode->inode_isize + inode->xattr_isize;
-	if (inode->extent_isize)
-		inodesize = roundup(inodesize, 8) + inode->extent_isize;
+	inodesize = erofs_get_inodesize(inode);
 
 	/* TODO: tailpacking inline of chunk-based format isn't finalized */
 	if (inode->datalayout == EROFS_INODE_CHUNK_BASED)
@@ -699,6 +799,8 @@ static int erofs_prepare_inode_buffer(struct erofs_inode *inode)
 	if (bh == ERR_PTR(-ENOSPC)) {
 		int ret;
 
+		BUG_ON(1);
+
 		if (is_inode_layout_compression(inode))
 			z_erofs_drop_inline_pcluster(inode);
 		else
-- 
2.44.0

[PATCH 2/3] erofs-utils: skip the redundant write for ztailpacking block

[Noboru Asai]

z_erofs_merge_segment() doesn't consider the ztailpacking block in the
extent list and unnecessarily writes it back to the disk. This patch
fixes this issue by changing compressdblks to 0.

And the value of blkaddr corresponding to the ztailpacking block
in the extent list is handled in z_erofs_write_extent function.

* legacy:   0 (fragmentoff >> 32)
* compact: -1 (EROFS_NULL_ADDR)

Signed-off-by: Noboru Asai <asai@sijam.com>
---
 lib/compress.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/lib/compress.c b/lib/compress.c
index dfe59da..d745e5b 100644
--- a/lib/compress.c
+++ b/lib/compress.c
@@ -555,24 +555,19 @@ static int __z_erofs_compress_one(struct z_erofs_compress_sctx *ctx,
 		if (may_inline && (erofs_get_lowest_offset(inode) + len < blksz)) {
 			ret = z_erofs_fill_inline_data(inode,
 					ctx->queue + ctx->head, len, true);
+			e->compressedblks = 0;
 		} else {
 			may_inline = false;
 			may_packing = false;
 nocompression:
 			/* TODO: reset clusterofs to 0 if permitted */
 			ret = write_uncompressed_extent(ctx, len, dst);
+			e->compressedblks = 1;
 		}
 
 		if (ret < 0)
 			return ret;
 		e->length = ret;
-
-		/*
-		 * XXX: For now, we have to leave `ctx->compressedblk = 1'
-		 * since there is no way to generate compressed indexes after
-		 * the time that ztailpacking is decided.
-		 */
-		e->compressedblks = 1;
 		e->raw = true;
 	} else if (may_packing && len == e->length &&
 		   compressedsize < ctx->pclustersize &&
@@ -601,7 +596,7 @@ frag_packing:
 				compressedsize, false);
 		if (ret < 0)
 			return ret;
-		e->compressedblks = 1;
+		e->compressedblks = 0;
 		e->raw = false;
 	} else {
 		unsigned int tailused, padding;
@@ -1151,6 +1146,9 @@ int z_erofs_merge_segment(struct z_erofs_compress_ictx *ictx,
 		if (ei->e.blkaddr != EROFS_NULL_ADDR)	/* deduped extents */
 			continue;
 
+		if (!ei->e.compressedblks)
+			continue;
+
 		ei->e.blkaddr = sctx->blkaddr;
 		sctx->blkaddr += ei->e.compressedblks;
 
@@ -1358,10 +1356,6 @@ int erofs_write_compressed_file(struct erofs_inode *inode, int fd, u64 fpos)
 		compressed_blocks = sctx.blkaddr - blkaddr;
 	}
 
-	/* fall back to no compression mode */
-	DBG_BUGON(compressed_blocks < !!inode->idata_size);
-	compressed_blocks -= !!inode->idata_size;
-
 	/* generate an extent for the deduplicated fragment */
 	if (inode->fragment_size && !ctx.fragemitted) {
 		struct z_erofs_extent_item *ei;
-- 
2.44.0

[PATCH 3/3] erofs-utils: allow blkaddr == EROFS_NULL_ADDR for zpacking in write_compacted_indexes()

[Noboru Asai]

With ztailpacking, the value of blkaddr corresponding to the ztailpacking block
in the extent list is EROFS_NULL_ADDR(-1), allow this value in write_compacted_indexes()
function.

Signed-off-by: Noboru Asai <asai@sijam.com>
---
 lib/compress.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lib/compress.c b/lib/compress.c
index d745e5b..5b70490 100644
--- a/lib/compress.c
+++ b/lib/compress.c
@@ -790,7 +790,8 @@ static void *write_compacted_indexes(u8 *out,
 			*dummy_head = true;
 			update_blkaddr = false;
 
-			if (cv[i].u.blkaddr != blkaddr) {
+			if (cv[i].u.blkaddr != EROFS_NULL_ADDR &&
+			    cv[i].u.blkaddr != blkaddr) {
 				if (i + 1 != vcnt)
 					DBG_BUGON(!final);
 				DBG_BUGON(cv[i].u.blkaddr);
-- 
2.44.0

Re: [PATCH 1/3] erofs-utils: determine the [un]compressed data block is inline or not early

[Gao Xiang]

Hi Noboru,

On Thu, Apr 18, 2024 at 02:52:29PM +0900, Noboru Asai wrote:
> Introducing erofs_get_inodesize function and erofs_get_lowest_offset function,
> we can determine the [un]compressed data block is inline or not before
> executing z_erofs_merge_segment function. It enable the following,
> 
> * skip the redundant write for ztailpacking block.
> * simplify erofs_prepare_inode_buffer function. (remove handling ENOSPC error)
> 
> Signed-off-by: Noboru Asai <asai@sijam.com>

I appreciate and thanks for your effort and time.

Yet I tend to avoid assuming if the inline is ok or not before
prepare_inode_buffer() since it will be free for space allocator
to decide inline or not at that time.

So personally I still would like to write a final compressed
index for inline fallback.

I will fix this issue myself later (it should be just a small
patch to fix this). 

Thanks for your effort on this issue again!

Thanks,
Gao Xiang

Re: [PATCH 1/3] erofs-utils: determine the [un]compressed data block is inline or not early

[Noboru Asai]

Hi Gao,

Thank you for your kind explanation. I respect your policy.

Please let me know if you change your mind,
since I will maintainance this patch personally.

2024年4月18日(木) 16:37 Gao Xiang <xiang@kernel.org>:
>
> Hi Noboru,
>
> On Thu, Apr 18, 2024 at 02:52:29PM +0900, Noboru Asai wrote:
> > Introducing erofs_get_inodesize function and erofs_get_lowest_offset function,
> > we can determine the [un]compressed data block is inline or not before
> > executing z_erofs_merge_segment function. It enable the following,
> >
> > * skip the redundant write for ztailpacking block.
> > * simplify erofs_prepare_inode_buffer function. (remove handling ENOSPC error)
> >
> > Signed-off-by: Noboru Asai <asai@sijam.com>
>
> I appreciate and thanks for your effort and time.
>
> Yet I tend to avoid assuming if the inline is ok or not before
> prepare_inode_buffer() since it will be free for space allocator
> to decide inline or not at that time.
>
> So personally I still would like to write a final compressed
> index for inline fallback.
>
> I will fix this issue myself later (it should be just a small
> patch to fix this).
>
> Thanks for your effort on this issue again!
>
> Thanks,
> Gao Xiang
>