Re: [PATCH v1 1/2] erofs-utils: Relax the hardchecks on the blocksize

[Gao Xiang]

On 2023/8/31 07:40, Sandeep Dhavale wrote:

On Wed, Aug 30, 2023 at 4:34 PM Gao Xiang <hsiang...@linux.alibaba.com> wrote:

Hi Sandeep,

On 2023/8/31 07:16, Sandeep Dhavale wrote:

As erofs-utils supports different block sizes upto
EROFS_MAX_BLOCK_SIZE, relax the checks so same tools
can be used to create images for platforms where
page size can be greater than 4096.

Signed-off-by: Sandeep Dhavale <dhav...@google.com>
---
   lib/namei.c | 2 --
   mkfs/main.c | 9 +++++----
   2 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/lib/namei.c b/lib/namei.c
index 2bb1d4c..45dbcd3 100644
--- a/lib/namei.c
+++ b/lib/namei.c
@@ -144,8 +144,6 @@ int erofs_read_inode_from_disk(struct erofs_inode *vi)
               vi->u.chunkbits = sbi->blkszbits +
                       (vi->u.chunkformat & EROFS_CHUNK_FORMAT_BLKBITS_MASK);
       } else if (erofs_inode_is_data_compressed(vi->datalayout)) {
-             if (erofs_blksiz(vi->sbi) != EROFS_MAX_BLOCK_SIZE)
-                     return -EOPNOTSUPP;
               return z_erofs_fill_inode(vi);
       }
       return 0;
diff --git a/mkfs/main.c b/mkfs/main.c
index c03a7a8..37bf658 100644
--- a/mkfs/main.c
+++ b/mkfs/main.c
@@ -550,10 +550,11 @@ static int mkfs_parse_options_cfg(int argc, char *argv[])
               cfg.c_dbg_lvl = EROFS_ERR;
               cfg.c_showprogress = false;
       }
-     if (cfg.c_compr_alg[0] && erofs_blksiz(&sbi) != EROFS_MAX_BLOCK_SIZE) {
-             erofs_err("compression is unsupported for now with block size %u",
-                       erofs_blksiz(&sbi));
-             return -EINVAL;
+     if (cfg.c_compr_alg[0] && erofs_blksiz(&sbi) != getpagesize()) {
+             erofs_warn("subpage blocksize with compression is not yet "
+                     "supported. Compressed image will only work with "
+                     "arch pagesize = blocksize = %u bytes",
+                     erofs_blksiz(&sbi));
       }

Thanks for the patches.

I'm fine to relax  EROFS_MAX_BLOCK_SIZE check, yet could we
add a check as erofs_blksiz <= EROFS_MAX_BLOCK_SIZE somewhere?

Otherwise, we could suffer from stack overflow
(if EROFS_MAX_BLOCK_SIZE is outdated or somewhat small...)

Hi Gao,
There is already a check in place for -b option to validate the block
size passed is within range [512..EROFS_MAX_BLOCK_SIZE] in function
mkfs_parse_options_cfg()

Please correct me if I misunderstood your comment.

Oh, sorry I didn't check the current code and thanks for your pointing
out!  Now I have no more questions.

Will apply soon.

Thanks,
Gao Xiang