* [PATCH] erofs-utils: mkfs: skip the redundant write for ztailpacking block
@ 2024-04-17 14:42 Yifan Zhao
2024-04-18 1:09 ` Noboru Asai
0 siblings, 1 reply; 5+ messages in thread
From: Yifan Zhao @ 2024-04-17 14:42 UTC (permalink / raw)
To: linux-erofs; +Cc: Yifan Zhao
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 introducing a new `inlined` field in the struct
`z_erofs_inmem_extent`.
Signed-off-by: Yifan Zhao <zhaoyifan@sjtu.edu.cn>
---
include/erofs/dedupe.h | 2 +-
lib/compress.c | 8 ++++++++
lib/dedupe.c | 1 +
3 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/include/erofs/dedupe.h b/include/erofs/dedupe.h
index 153bd4c..4cbfb2c 100644
--- a/include/erofs/dedupe.h
+++ b/include/erofs/dedupe.h
@@ -16,7 +16,7 @@ struct z_erofs_inmem_extent {
erofs_blk_t blkaddr;
unsigned int compressedblks;
unsigned int length;
- bool raw, partial;
+ bool raw, partial, inlined;
};
struct z_erofs_dedupe_ctx {
diff --git a/lib/compress.c b/lib/compress.c
index 74c5707..41628e7 100644
--- a/lib/compress.c
+++ b/lib/compress.c
@@ -572,6 +572,7 @@ nocompression:
*/
e->compressedblks = 1;
e->raw = true;
+ e->inlined = false;
} else if (may_packing && len == e->length &&
compressedsize < ctx->pclustersize &&
(!inode->fragment_size || ictx->fix_dedupedfrag)) {
@@ -582,6 +583,7 @@ frag_packing:
return ret;
e->compressedblks = 0; /* indicate a fragment */
e->raw = false;
+ e->inlined = false;
ictx->fragemitted = true;
/* tailpcluster should be less than 1 block */
} else if (may_inline && len == e->length && compressedsize < blksz) {
@@ -600,6 +602,7 @@ frag_packing:
return ret;
e->compressedblks = 1;
e->raw = false;
+ e->inlined = true;
} else {
unsigned int tailused, padding;
@@ -652,6 +655,7 @@ frag_packing:
return ret;
}
e->raw = false;
+ e->inlined = false;
may_inline = false;
may_packing = false;
}
@@ -1151,6 +1155,9 @@ int z_erofs_merge_segment(struct z_erofs_compress_ictx *ictx,
ei->e.blkaddr = sctx->blkaddr;
sctx->blkaddr += ei->e.compressedblks;
+ if (ei->e.inlined)
+ continue;
+
ret2 = blk_write(sbi, sctx->membuf + blkoff * erofs_blksiz(sbi),
ei->e.blkaddr, ei->e.compressedblks);
blkoff += ei->e.compressedblks;
@@ -1374,6 +1381,7 @@ int erofs_write_compressed_file(struct erofs_inode *inode, int fd, u64 fpos)
.compressedblks = 0,
.raw = false,
.partial = false,
+ .inlined = false,
.blkaddr = sctx.blkaddr,
};
init_list_head(&ei->list);
diff --git a/lib/dedupe.c b/lib/dedupe.c
index 19a1c8d..aaaccb5 100644
--- a/lib/dedupe.c
+++ b/lib/dedupe.c
@@ -138,6 +138,7 @@ int z_erofs_dedupe_match(struct z_erofs_dedupe_ctx *ctx)
ctx->e.partial = e->partial ||
(window_size + extra < e->original_length);
ctx->e.raw = e->raw;
+ ctx->e.inlined = false;
ctx->e.blkaddr = e->compressed_blkaddr;
ctx->e.compressedblks = e->compressed_blks;
return 0;
--
2.44.0
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] erofs-utils: mkfs: skip the redundant write for ztailpacking block
2024-04-17 14:42 [PATCH] erofs-utils: mkfs: skip the redundant write for ztailpacking block Yifan Zhao
@ 2024-04-18 1:09 ` Noboru Asai
2024-04-18 4:57 ` Gao Xiang
2024-04-18 5:03 ` Yifan Zhao
0 siblings, 2 replies; 5+ messages in thread
From: Noboru Asai @ 2024-04-18 1:09 UTC (permalink / raw)
To: Yifan Zhao, Gao Xiang; +Cc: linux-erofs
In this patch, the value of blkaddr in z_erofs_lcluster_index
corresponding to the ztailpacking block in the extent list
is invalid value. It looks that the linux kernel doesn't refer to this
value, but what value is correct?
0 or -1 (EROFS_NULL_ADDR) or don't care?
2024年4月17日(水) 23:43 Yifan Zhao <zhaoyifan@sjtu.edu.cn>:
>
> 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 introducing a new `inlined` field in the struct
> `z_erofs_inmem_extent`.
>
> Signed-off-by: Yifan Zhao <zhaoyifan@sjtu.edu.cn>
> ---
> include/erofs/dedupe.h | 2 +-
> lib/compress.c | 8 ++++++++
> lib/dedupe.c | 1 +
> 3 files changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/include/erofs/dedupe.h b/include/erofs/dedupe.h
> index 153bd4c..4cbfb2c 100644
> --- a/include/erofs/dedupe.h
> +++ b/include/erofs/dedupe.h
> @@ -16,7 +16,7 @@ struct z_erofs_inmem_extent {
> erofs_blk_t blkaddr;
> unsigned int compressedblks;
> unsigned int length;
> - bool raw, partial;
> + bool raw, partial, inlined;
> };
>
> struct z_erofs_dedupe_ctx {
> diff --git a/lib/compress.c b/lib/compress.c
> index 74c5707..41628e7 100644
> --- a/lib/compress.c
> +++ b/lib/compress.c
> @@ -572,6 +572,7 @@ nocompression:
> */
> e->compressedblks = 1;
> e->raw = true;
> + e->inlined = false;
> } else if (may_packing && len == e->length &&
> compressedsize < ctx->pclustersize &&
> (!inode->fragment_size || ictx->fix_dedupedfrag)) {
> @@ -582,6 +583,7 @@ frag_packing:
> return ret;
> e->compressedblks = 0; /* indicate a fragment */
> e->raw = false;
> + e->inlined = false;
> ictx->fragemitted = true;
> /* tailpcluster should be less than 1 block */
> } else if (may_inline && len == e->length && compressedsize < blksz) {
> @@ -600,6 +602,7 @@ frag_packing:
> return ret;
> e->compressedblks = 1;
> e->raw = false;
> + e->inlined = true;
> } else {
> unsigned int tailused, padding;
>
> @@ -652,6 +655,7 @@ frag_packing:
> return ret;
> }
> e->raw = false;
> + e->inlined = false;
> may_inline = false;
> may_packing = false;
> }
> @@ -1151,6 +1155,9 @@ int z_erofs_merge_segment(struct z_erofs_compress_ictx *ictx,
> ei->e.blkaddr = sctx->blkaddr;
> sctx->blkaddr += ei->e.compressedblks;
>
> + if (ei->e.inlined)
> + continue;
> +
> ret2 = blk_write(sbi, sctx->membuf + blkoff * erofs_blksiz(sbi),
> ei->e.blkaddr, ei->e.compressedblks);
> blkoff += ei->e.compressedblks;
> @@ -1374,6 +1381,7 @@ int erofs_write_compressed_file(struct erofs_inode *inode, int fd, u64 fpos)
> .compressedblks = 0,
> .raw = false,
> .partial = false,
> + .inlined = false,
> .blkaddr = sctx.blkaddr,
> };
> init_list_head(&ei->list);
> diff --git a/lib/dedupe.c b/lib/dedupe.c
> index 19a1c8d..aaaccb5 100644
> --- a/lib/dedupe.c
> +++ b/lib/dedupe.c
> @@ -138,6 +138,7 @@ int z_erofs_dedupe_match(struct z_erofs_dedupe_ctx *ctx)
> ctx->e.partial = e->partial ||
> (window_size + extra < e->original_length);
> ctx->e.raw = e->raw;
> + ctx->e.inlined = false;
> ctx->e.blkaddr = e->compressed_blkaddr;
> ctx->e.compressedblks = e->compressed_blks;
> return 0;
> --
> 2.44.0
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] erofs-utils: mkfs: skip the redundant write for ztailpacking block
2024-04-18 1:09 ` Noboru Asai
@ 2024-04-18 4:57 ` Gao Xiang
2024-04-18 5:52 ` Noboru Asai
2024-04-18 5:03 ` Yifan Zhao
1 sibling, 1 reply; 5+ messages in thread
From: Gao Xiang @ 2024-04-18 4:57 UTC (permalink / raw)
To: Noboru Asai; +Cc: Gao Xiang, Yifan Zhao, linux-erofs
Hi Noboru,
On Thu, Apr 18, 2024 at 10:09:22AM +0900, Noboru Asai wrote:
> In this patch, the value of blkaddr in z_erofs_lcluster_index
> corresponding to the ztailpacking block in the extent list
> is invalid value. It looks that the linux kernel doesn't refer to this
> value, but what value is correct?
> 0 or -1 (EROFS_NULL_ADDR) or don't care?
Thanks for pointing out!
On the kernel side, it doesn't care this value if it's really
_inlined_.
But on the mkfs side, since we have inline fallback so I don't
think an invalid blkaddr is correct. The next blkaddr should
be filled for inline fallback instead.
Let me think more about it and update the patch.
Thanks,
Gao Xiang
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] erofs-utils: mkfs: skip the redundant write for ztailpacking block
2024-04-18 1:09 ` Noboru Asai
2024-04-18 4:57 ` Gao Xiang
@ 2024-04-18 5:03 ` Yifan Zhao
1 sibling, 0 replies; 5+ messages in thread
From: Yifan Zhao @ 2024-04-18 5:03 UTC (permalink / raw)
To: Noboru Asai; +Cc: Gao Xiang, linux-erofs
On 4/18/24 9:09 AM, Noboru Asai wrote:
> In this patch, the value of blkaddr in z_erofs_lcluster_index
> corresponding to the ztailpacking block in the extent list
> is invalid value. It looks that the linux kernel doesn't refer to this
> value, but what value is correct?
> 0 or -1 (EROFS_NULL_ADDR) or don't care?
For the read side, AFAIK the kernel (and also the user lib of
erofs-utils) will turn to the meta for inlined data
for read offset beyond the last data block if ztailpacking is enabled,
see `erofs_map_blocks_flatmode`.
So I believe the blkaddr recorded in z_erofs_lcluster_index is
irrelevant and we don't need to care about it.
For the mkfs side I think things could be polished according to Gao's
review opinion.
Thanks,
Yifan Zhao
>
> 2024年4月17日(水) 23:43 Yifan Zhao <zhaoyifan@sjtu.edu.cn>:
>> 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 introducing a new `inlined` field in the struct
>> `z_erofs_inmem_extent`.
>>
>> Signed-off-by: Yifan Zhao <zhaoyifan@sjtu.edu.cn>
>> ---
>> include/erofs/dedupe.h | 2 +-
>> lib/compress.c | 8 ++++++++
>> lib/dedupe.c | 1 +
>> 3 files changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/erofs/dedupe.h b/include/erofs/dedupe.h
>> index 153bd4c..4cbfb2c 100644
>> --- a/include/erofs/dedupe.h
>> +++ b/include/erofs/dedupe.h
>> @@ -16,7 +16,7 @@ struct z_erofs_inmem_extent {
>> erofs_blk_t blkaddr;
>> unsigned int compressedblks;
>> unsigned int length;
>> - bool raw, partial;
>> + bool raw, partial, inlined;
>> };
>>
>> struct z_erofs_dedupe_ctx {
>> diff --git a/lib/compress.c b/lib/compress.c
>> index 74c5707..41628e7 100644
>> --- a/lib/compress.c
>> +++ b/lib/compress.c
>> @@ -572,6 +572,7 @@ nocompression:
>> */
>> e->compressedblks = 1;
>> e->raw = true;
>> + e->inlined = false;
>> } else if (may_packing && len == e->length &&
>> compressedsize < ctx->pclustersize &&
>> (!inode->fragment_size || ictx->fix_dedupedfrag)) {
>> @@ -582,6 +583,7 @@ frag_packing:
>> return ret;
>> e->compressedblks = 0; /* indicate a fragment */
>> e->raw = false;
>> + e->inlined = false;
>> ictx->fragemitted = true;
>> /* tailpcluster should be less than 1 block */
>> } else if (may_inline && len == e->length && compressedsize < blksz) {
>> @@ -600,6 +602,7 @@ frag_packing:
>> return ret;
>> e->compressedblks = 1;
>> e->raw = false;
>> + e->inlined = true;
>> } else {
>> unsigned int tailused, padding;
>>
>> @@ -652,6 +655,7 @@ frag_packing:
>> return ret;
>> }
>> e->raw = false;
>> + e->inlined = false;
>> may_inline = false;
>> may_packing = false;
>> }
>> @@ -1151,6 +1155,9 @@ int z_erofs_merge_segment(struct z_erofs_compress_ictx *ictx,
>> ei->e.blkaddr = sctx->blkaddr;
>> sctx->blkaddr += ei->e.compressedblks;
>>
>> + if (ei->e.inlined)
>> + continue;
>> +
>> ret2 = blk_write(sbi, sctx->membuf + blkoff * erofs_blksiz(sbi),
>> ei->e.blkaddr, ei->e.compressedblks);
>> blkoff += ei->e.compressedblks;
>> @@ -1374,6 +1381,7 @@ int erofs_write_compressed_file(struct erofs_inode *inode, int fd, u64 fpos)
>> .compressedblks = 0,
>> .raw = false,
>> .partial = false,
>> + .inlined = false,
>> .blkaddr = sctx.blkaddr,
>> };
>> init_list_head(&ei->list);
>> diff --git a/lib/dedupe.c b/lib/dedupe.c
>> index 19a1c8d..aaaccb5 100644
>> --- a/lib/dedupe.c
>> +++ b/lib/dedupe.c
>> @@ -138,6 +138,7 @@ int z_erofs_dedupe_match(struct z_erofs_dedupe_ctx *ctx)
>> ctx->e.partial = e->partial ||
>> (window_size + extra < e->original_length);
>> ctx->e.raw = e->raw;
>> + ctx->e.inlined = false;
>> ctx->e.blkaddr = e->compressed_blkaddr;
>> ctx->e.compressedblks = e->compressed_blks;
>> return 0;
>> --
>> 2.44.0
>>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] erofs-utils: mkfs: skip the redundant write for ztailpacking block
2024-04-18 4:57 ` Gao Xiang
@ 2024-04-18 5:52 ` Noboru Asai
0 siblings, 0 replies; 5+ messages in thread
From: Noboru Asai @ 2024-04-18 5:52 UTC (permalink / raw)
To: Noboru Asai, Yifan Zhao, Gao Xiang, linux-erofs
Hi Gao,
I made a patch set and send it. I'm grad that this patch set help your thinking.
2024年4月18日(木) 13:58 Gao Xiang <xiang@kernel.org>:
>
> Hi Noboru,
>
> On Thu, Apr 18, 2024 at 10:09:22AM +0900, Noboru Asai wrote:
> > In this patch, the value of blkaddr in z_erofs_lcluster_index
> > corresponding to the ztailpacking block in the extent list
> > is invalid value. It looks that the linux kernel doesn't refer to this
> > value, but what value is correct?
> > 0 or -1 (EROFS_NULL_ADDR) or don't care?
>
> Thanks for pointing out!
>
> On the kernel side, it doesn't care this value if it's really
> _inlined_.
>
> But on the mkfs side, since we have inline fallback so I don't
> think an invalid blkaddr is correct. The next blkaddr should
> be filled for inline fallback instead.
>
> Let me think more about it and update the patch.
>
> Thanks,
> Gao Xiang
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2024-04-18 5:52 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-04-17 14:42 [PATCH] erofs-utils: mkfs: skip the redundant write for ztailpacking block Yifan Zhao
2024-04-18 1:09 ` Noboru Asai
2024-04-18 4:57 ` Gao Xiang
2024-04-18 5:52 ` Noboru Asai
2024-04-18 5:03 ` Yifan Zhao
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.