linux-erofs.lists.ozlabs.org archive mirror
 help / color / mirror / Atom feed
* [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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).