* [RFC PATCH 1/2] erofs-utils: dump: cleanup update_file_size_statatics() @ 2022-12-19 9:50 Yue Hu 2022-12-19 9:50 ` [RFC PATCH 2/2] erofs-utils: dump: support fragments Yue Hu 0 siblings, 1 reply; 7+ messages in thread From: Yue Hu @ 2022-12-19 9:50 UTC (permalink / raw) To: linux-erofs; +Cc: Yue Hu, zhangwen From: Yue Hu <huyue2@coolpad.com> The statistic update of occupied_size is the same as original size. Also, correct naming to update_file_size_statistics(). Signed-off-by: Yue Hu <huyue2@coolpad.com> --- dump/main.c | 38 +++++++++++++------------------------- 1 file changed, 13 insertions(+), 25 deletions(-) diff --git a/dump/main.c b/dump/main.c index 93dce8b..bc4f047 100644 --- a/dump/main.c +++ b/dump/main.c @@ -225,36 +225,23 @@ static void inc_file_extension_count(const char *dname, unsigned int len) ++stats.file_type_stat[type]; } -static void update_file_size_statatics(erofs_off_t occupied_size, - erofs_off_t original_size) +static void update_file_size_statistics(erofs_off_t size, bool original) { - int occupied_size_mark, original_size_mark; + unsigned int *file_size = original ? stats.file_original_size : + stats.file_comp_size; + int size_mark = 0; - original_size_mark = 0; - occupied_size_mark = 0; - occupied_size >>= 10; - original_size >>= 10; + size >>= 10; - while (occupied_size || original_size) { - if (occupied_size) { - occupied_size >>= 1; - occupied_size_mark++; - } - if (original_size) { - original_size >>= 1; - original_size_mark++; - } + while (size) { + size >>= 1; + size_mark++; } - if (original_size_mark >= FILE_MAX_SIZE_BITS) - stats.file_original_size[FILE_MAX_SIZE_BITS]++; - else - stats.file_original_size[original_size_mark]++; - - if (occupied_size_mark >= FILE_MAX_SIZE_BITS) - stats.file_comp_size[FILE_MAX_SIZE_BITS]++; + if (size_mark >= FILE_MAX_SIZE_BITS) + file_size[FILE_MAX_SIZE_BITS]++; else - stats.file_comp_size[occupied_size_mark]++; + file_size[size_mark]++; } static int erofsdump_ls_dirent_iter(struct erofs_dir_context *ctx) @@ -301,7 +288,8 @@ static int erofsdump_readdir(struct erofs_dir_context *ctx) stats.files_total_origin_size += vi.i_size; inc_file_extension_count(ctx->dname, ctx->de_namelen); stats.files_total_size += occupied_size; - update_file_size_statatics(occupied_size, vi.i_size); + update_file_size_statistics(vi.i_size, true); + update_file_size_statistics(occupied_size, false); } /* XXXX: the dir depth should be restricted in order to avoid loops */ -- 2.17.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [RFC PATCH 2/2] erofs-utils: dump: support fragments 2022-12-19 9:50 [RFC PATCH 1/2] erofs-utils: dump: cleanup update_file_size_statatics() Yue Hu @ 2022-12-19 9:50 ` Yue Hu 2023-01-04 3:13 ` Xiang Gao 0 siblings, 1 reply; 7+ messages in thread From: Yue Hu @ 2022-12-19 9:50 UTC (permalink / raw) To: linux-erofs; +Cc: Yue Hu, zhangwen From: Yue Hu <huyue2@coolpad.com> Add compressed fragments support for dump feature. Signed-off-by: Yue Hu <huyue2@coolpad.com> --- dump/main.c | 78 ++++++++++++++++++++++++++++++++-------- include/erofs/internal.h | 1 + lib/zmap.c | 2 +- 3 files changed, 65 insertions(+), 16 deletions(-) diff --git a/dump/main.c b/dump/main.c index bc4f047..d387841 100644 --- a/dump/main.c +++ b/dump/main.c @@ -14,6 +14,8 @@ #include "erofs/inode.h" #include "erofs/io.h" #include "erofs/dir.h" +#include "erofs/compress.h" +#include "erofs/fragments.h" #include "../lib/liberofs_private.h" #ifdef HAVE_LIBUUID @@ -96,6 +98,7 @@ static struct erofsdump_feature feature_lists[] = { { false, EROFS_FEATURE_INCOMPAT_CHUNKED_FILE, "chunked_file" }, { false, EROFS_FEATURE_INCOMPAT_DEVICE_TABLE, "device_table" }, { false, EROFS_FEATURE_INCOMPAT_ZTAILPACKING, "ztailpacking" }, + { false, EROFS_FEATURE_INCOMPAT_FRAGMENTS, "fragments" }, }; static int erofsdump_readdir(struct erofs_dir_context *ctx); @@ -285,10 +288,12 @@ static int erofsdump_readdir(struct erofs_dir_context *ctx) } if (S_ISREG(vi.i_mode)) { - stats.files_total_origin_size += vi.i_size; - inc_file_extension_count(ctx->dname, ctx->de_namelen); + if (!erofs_is_packed_inode(&vi)) { + stats.files_total_origin_size += vi.i_size; + inc_file_extension_count(ctx->dname, ctx->de_namelen); + update_file_size_statistics(vi.i_size, true); + } stats.files_total_size += occupied_size; - update_file_size_statistics(vi.i_size, true); update_file_size_statistics(occupied_size, false); } @@ -320,6 +325,10 @@ static void erofsdump_show_fileinfo(bool show_extent) "%4d: %8" PRIu64 "..%8" PRIu64 " | %7" PRIu64 " : %10" PRIu64 "..%10" PRIu64 " | %7" PRIu64 "\n", "%4d: %8" PRIu64 "..%8" PRIu64 " | %7" PRIu64 " : %10" PRIu64 "..%10" PRIu64 " | %7" PRIu64 " # device %u\n" }; + const char *frag_ext_fmt[] = { + "%4d: %8" PRIu64 "..%8" PRIu64 " | %7" PRIu64 "\n", + "%4d: %8" PRIu64 "..%8" PRIu64 " | %7" PRIu64 " # device %u\n" + }; int err, i; erofs_off_t size; u16 access_mode; @@ -348,16 +357,31 @@ static void erofsdump_show_fileinfo(bool show_extent) } } + if (erofs_inode_is_data_compressed(inode.datalayout)) { + err = z_erofs_fill_inode_lazy(&inode); + if (err) { + erofs_err("read inode map header failed @ nid %llu", + inode.nid | 0ULL); + return; + } + } + err = erofs_get_occupied_size(&inode, &size); if (err) { erofs_err("get file size failed @ nid %llu", inode.nid | 0ULL); return; } - err = erofs_get_pathname(inode.nid, path, sizeof(path)); - if (err < 0) { - erofs_err("file path not found @ nid %llu", inode.nid | 0ULL); - return; + if (erofs_is_packed_inode(&inode)) { + strncpy(path, EROFS_PACKED_INODE, sizeof(path) - 1); + path[sizeof(path) - 1] = '\0'; + } else { + err = erofs_get_pathname(inode.nid, path, sizeof(path)); + if (err < 0) { + erofs_err("file path not found @ nid %llu", + inode.nid | 0ULL); + return; + } } strftime(timebuf, sizeof(timebuf), @@ -372,9 +396,13 @@ static void erofsdump_show_fileinfo(bool show_extent) file_category_types[erofs_mode_to_ftype(inode.i_mode)]); fprintf(stdout, "NID: %" PRIu64 " ", inode.nid); fprintf(stdout, "Links: %u ", inode.i_nlink); - fprintf(stdout, "Layout: %d Compression ratio: %.2f%%\n", - inode.datalayout, - (double)(100 * size) / (double)(inode.i_size)); + if (inode.z_advise & Z_EROFS_ADVISE_FRAGMENT_PCLUSTER) + fprintf(stdout, "Layout: %d Fragment: %s\n", + inode.datalayout, size ? "partial" : "full"); + else + fprintf(stdout, "Layout: %d Compression ratio: %.2f%%\n", + inode.datalayout, + (double)(100 * size) / (double)(inode.i_size)); fprintf(stdout, "Inode size: %d ", inode.inode_isize); fprintf(stdout, "Extent size: %u ", inode.extent_isize); fprintf(stdout, "Xattr size: %u\n", inode.xattr_isize); @@ -404,7 +432,8 @@ static void erofsdump_show_fileinfo(bool show_extent) if (!dumpcfg.show_extent) return; - fprintf(stdout, "\n Ext: logical offset | length : physical offset | length\n"); + fprintf(stdout, "\n Ext: logical offset | length%s\n", + size ? " : physical offset | length" : ""); while (map.m_la < inode.i_size) { struct erofs_map_dev mdev; @@ -425,10 +454,17 @@ static void erofsdump_show_fileinfo(bool show_extent) return; } - fprintf(stdout, ext_fmt[!!mdev.m_deviceid], extent_count++, - map.m_la, map.m_la + map.m_llen, map.m_llen, - mdev.m_pa, mdev.m_pa + map.m_plen, map.m_plen, - mdev.m_deviceid); + if (map.m_flags & EROFS_MAP_FRAGMENT) + fprintf(stdout, frag_ext_fmt[!!mdev.m_deviceid], + extent_count++, + map.m_la, map.m_la + map.m_llen, map.m_llen, + mdev.m_deviceid); + else + fprintf(stdout, ext_fmt[!!mdev.m_deviceid], + extent_count++, + map.m_la, map.m_la + map.m_llen, map.m_llen, + mdev.m_pa, mdev.m_pa + map.m_plen, map.m_plen, + mdev.m_deviceid); map.m_la += map.m_llen; } fprintf(stdout, "%s: %d extents found\n", path, extent_count); @@ -537,6 +573,15 @@ static void erofsdump_print_statistic(void) erofs_err("read dir failed"); return; } + if (erofs_sb_has_fragments()) { + err = erofsdump_readdir(&(struct erofs_dir_context) { + .de_nid = sbi.packed_nid + }); + if (err) { + erofs_err("read packed inode failed"); + return; + } + } erofsdump_file_statistic(); erofsdump_filesize_distribution("Original", stats.file_original_size, @@ -563,6 +608,9 @@ static void erofsdump_show_superblock(void) sbi.xattr_blkaddr); fprintf(stdout, "Filesystem root nid: %llu\n", sbi.root_nid | 0ULL); + if (erofs_sb_has_fragments()) + fprintf(stdout, "Filesystem packed nid: %llu\n", + sbi.packed_nid | 0ULL); fprintf(stdout, "Filesystem inode count: %llu\n", sbi.inos | 0ULL); fprintf(stdout, "Filesystem created: %s", diff --git a/include/erofs/internal.h b/include/erofs/internal.h index 206913c..947894d 100644 --- a/include/erofs/internal.h +++ b/include/erofs/internal.h @@ -383,6 +383,7 @@ int erofs_listxattr(struct erofs_inode *vi, char *buffer, size_t buffer_size); /* zmap.c */ int z_erofs_fill_inode(struct erofs_inode *vi); +int z_erofs_fill_inode_lazy(struct erofs_inode *vi); int z_erofs_map_blocks_iter(struct erofs_inode *vi, struct erofs_map_blocks *map, int flags); diff --git a/lib/zmap.c b/lib/zmap.c index 89e9da1..41e0713 100644 --- a/lib/zmap.c +++ b/lib/zmap.c @@ -29,7 +29,7 @@ int z_erofs_fill_inode(struct erofs_inode *vi) return 0; } -static int z_erofs_fill_inode_lazy(struct erofs_inode *vi) +int z_erofs_fill_inode_lazy(struct erofs_inode *vi) { int ret; erofs_off_t pos; -- 2.17.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [RFC PATCH 2/2] erofs-utils: dump: support fragments 2022-12-19 9:50 ` [RFC PATCH 2/2] erofs-utils: dump: support fragments Yue Hu @ 2023-01-04 3:13 ` Xiang Gao 2023-01-04 6:16 ` Yue Hu 0 siblings, 1 reply; 7+ messages in thread From: Xiang Gao @ 2023-01-04 3:13 UTC (permalink / raw) To: Yue Hu, linux-erofs; +Cc: Yue Hu, zhangwen On 2022/12/19 17:50, Yue Hu wrote: > From: Yue Hu <huyue2@coolpad.com> > > Add compressed fragments support for dump feature. > > Signed-off-by: Yue Hu <huyue2@coolpad.com> > --- > dump/main.c | 78 ++++++++++++++++++++++++++++++++-------- > include/erofs/internal.h | 1 + > lib/zmap.c | 2 +- > 3 files changed, 65 insertions(+), 16 deletions(-) > > diff --git a/dump/main.c b/dump/main.c > index bc4f047..d387841 100644 > --- a/dump/main.c > +++ b/dump/main.c > @@ -14,6 +14,8 @@ > #include "erofs/inode.h" > #include "erofs/io.h" > #include "erofs/dir.h" > +#include "erofs/compress.h" > +#include "erofs/fragments.h" > #include "../lib/liberofs_private.h" > > #ifdef HAVE_LIBUUID > @@ -96,6 +98,7 @@ static struct erofsdump_feature feature_lists[] = { > { false, EROFS_FEATURE_INCOMPAT_CHUNKED_FILE, "chunked_file" }, > { false, EROFS_FEATURE_INCOMPAT_DEVICE_TABLE, "device_table" }, > { false, EROFS_FEATURE_INCOMPAT_ZTAILPACKING, "ztailpacking" }, > + { false, EROFS_FEATURE_INCOMPAT_FRAGMENTS, "fragments" }, > }; > > static int erofsdump_readdir(struct erofs_dir_context *ctx); > @@ -285,10 +288,12 @@ static int erofsdump_readdir(struct erofs_dir_context *ctx) > } > > if (S_ISREG(vi.i_mode)) { > - stats.files_total_origin_size += vi.i_size; > - inc_file_extension_count(ctx->dname, ctx->de_namelen); > + if (!erofs_is_packed_inode(&vi)) { > + stats.files_total_origin_size += vi.i_size; > + inc_file_extension_count(ctx->dname, ctx->de_namelen); > + update_file_size_statistics(vi.i_size, true); > + } > stats.files_total_size += occupied_size; > - update_file_size_statistics(vi.i_size, true); > update_file_size_statistics(occupied_size, false); > } > > @@ -320,6 +325,10 @@ static void erofsdump_show_fileinfo(bool show_extent) > "%4d: %8" PRIu64 "..%8" PRIu64 " | %7" PRIu64 " : %10" PRIu64 "..%10" PRIu64 " | %7" PRIu64 "\n", > "%4d: %8" PRIu64 "..%8" PRIu64 " | %7" PRIu64 " : %10" PRIu64 "..%10" PRIu64 " | %7" PRIu64 " # device %u\n" > }; > + const char *frag_ext_fmt[] = { > + "%4d: %8" PRIu64 "..%8" PRIu64 " | %7" PRIu64 "\n", > + "%4d: %8" PRIu64 "..%8" PRIu64 " | %7" PRIu64 " # device %u\n" > + }; Why do we need another fmt rather than just fill fragment extent with physical addr 0? > int err, i; > erofs_off_t size; > u16 access_mode; > @@ -348,16 +357,31 @@ static void erofsdump_show_fileinfo(bool show_extent) > } > } > > + if (erofs_inode_is_data_compressed(inode.datalayout)) { > + err = z_erofs_fill_inode_lazy(&inode); > + if (err) { > + erofs_err("read inode map header failed @ nid %llu", > + inode.nid | 0ULL); > + return; > + } > + } Why do we need to call z_erofs_fill_inode_lazy here? > + > err = erofs_get_occupied_size(&inode, &size); > if (err) { > erofs_err("get file size failed @ nid %llu", inode.nid | 0ULL); > return; > } > > - err = erofs_get_pathname(inode.nid, path, sizeof(path)); > - if (err < 0) { > - erofs_err("file path not found @ nid %llu", inode.nid | 0ULL); > - return; Can we just ignore pathname if it doesn't exist? > + if (erofs_is_packed_inode(&inode) { > + strncpy(path, EROFS_PACKED_INODE, sizeof(path) - 1); > + path[sizeof(path) - 1] = '\0'; > + } else { > + err = erofs_get_pathname(inode.nid, path, sizeof(path)); > + if (err < 0) { > + erofs_err("file path not found @ nid %llu", > + inode.nid | 0ULL); > + return; > + } > } > > strftime(timebuf, sizeof(timebuf), > @@ -372,9 +396,13 @@ static void erofsdump_show_fileinfo(bool show_extent) > file_category_types[erofs_mode_to_ftype(inode.i_mode)]); > fprintf(stdout, "NID: %" PRIu64 " ", inode.nid); > fprintf(stdout, "Links: %u ", inode.i_nlink); > - fprintf(stdout, "Layout: %d Compression ratio: %.2f%%\n", > - inode.datalayout, > - (double)(100 * size) / (double)(inode.i_size)); > + if (inode.z_advise & Z_EROFS_ADVISE_FRAGMENT_PCLUSTER) > + fprintf(stdout, "Layout: %d Fragment: %s\n", > + inode.datalayout, size ? "partial" : "full"); > + else > + fprintf(stdout, "Layout: %d Compression ratio: %.2f%%\n", > + inode.datalayout, > + (double)(100 * size) / (double)(inode.i_size)); > fprintf(stdout, "Inode size: %d ", inode.inode_isize); > fprintf(stdout, "Extent size: %u ", inode.extent_isize); > fprintf(stdout, "Xattr size: %u\n", inode.xattr_isize); > @@ -404,7 +432,8 @@ static void erofsdump_show_fileinfo(bool show_extent) > if (!dumpcfg.show_extent) > return; > > - fprintf(stdout, "\n Ext: logical offset | length : physical offset | length\n"); > + fprintf(stdout, "\n Ext: logical offset | length%s\n", > + size ? " : physical offset | length" : ""); > while (map.m_la < inode.i_size) { > struct erofs_map_dev mdev; > > @@ -425,10 +454,17 @@ static void erofsdump_show_fileinfo(bool show_extent) > return; > } > > - fprintf(stdout, ext_fmt[!!mdev.m_deviceid], extent_count++, > - map.m_la, map.m_la + map.m_llen, map.m_llen, > - mdev.m_pa, mdev.m_pa + map.m_plen, map.m_plen, > - mdev.m_deviceid); > + if (map.m_flags & EROFS_MAP_FRAGMENT) > + fprintf(stdout, frag_ext_fmt[!!mdev.m_deviceid], > + extent_count++, > + map.m_la, map.m_la + map.m_llen, map.m_llen, > + mdev.m_deviceid); except for the last fragment extent, the other extents all have physical addr and length... > + else > + fprintf(stdout, ext_fmt[!!mdev.m_deviceid], > + extent_count++, > + map.m_la, map.m_la + map.m_llen, map.m_llen, > + mdev.m_pa, mdev.m_pa + map.m_plen, map.m_plen, > + mdev.m_deviceid); > map.m_la += map.m_llen; > } > fprintf(stdout, "%s: %d extents found\n", path, extent_count); > @@ -537,6 +573,15 @@ static void erofsdump_print_statistic(void) > erofs_err("read dir failed"); > return; > } > + if (erofs_sb_has_fragments()) { > + err = erofsdump_readdir(&(struct erofs_dir_context) { > + .de_nid = sbi.packed_nid > + }); why do we need this? Thanks, Gao Xiang > + if (err) { > + erofs_err("read packed inode failed"); > + return; > + } > + } > erofsdump_file_statistic(); > erofsdump_filesize_distribution("Original", > stats.file_original_size, > @@ -563,6 +608,9 @@ static void erofsdump_show_superblock(void) > sbi.xattr_blkaddr); > fprintf(stdout, "Filesystem root nid: %llu\n", > sbi.root_nid | 0ULL); > + if (erofs_sb_has_fragments()) > + fprintf(stdout, "Filesystem packed nid: %llu\n", > + sbi.packed_nid | 0ULL); > fprintf(stdout, "Filesystem inode count: %llu\n", > sbi.inos | 0ULL); > fprintf(stdout, "Filesystem created: %s", > diff --git a/include/erofs/internal.h b/include/erofs/internal.h > index 206913c..947894d 100644 > --- a/include/erofs/internal.h > +++ b/include/erofs/internal.h > @@ -383,6 +383,7 @@ int erofs_listxattr(struct erofs_inode *vi, char *buffer, size_t buffer_size); > > /* zmap.c */ > int z_erofs_fill_inode(struct erofs_inode *vi); > +int z_erofs_fill_inode_lazy(struct erofs_inode *vi); > int z_erofs_map_blocks_iter(struct erofs_inode *vi, > struct erofs_map_blocks *map, int flags); > > diff --git a/lib/zmap.c b/lib/zmap.c > index 89e9da1..41e0713 100644 > --- a/lib/zmap.c > +++ b/lib/zmap.c > @@ -29,7 +29,7 @@ int z_erofs_fill_inode(struct erofs_inode *vi) > return 0; > } > > -static int z_erofs_fill_inode_lazy(struct erofs_inode *vi) > +int z_erofs_fill_inode_lazy(struct erofs_inode *vi) > { > int ret; > erofs_off_t pos; ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC PATCH 2/2] erofs-utils: dump: support fragments 2023-01-04 3:13 ` Xiang Gao @ 2023-01-04 6:16 ` Yue Hu 2023-01-04 6:44 ` Xiang Gao 0 siblings, 1 reply; 7+ messages in thread From: Yue Hu @ 2023-01-04 6:16 UTC (permalink / raw) To: Xiang Gao; +Cc: Yue Hu, linux-erofs, zhangwen Hi Xiang, Thanks for the reviewing. I'm planning to send v2. On Wed, 4 Jan 2023 11:13:55 +0800 Xiang Gao <hsiangkao@linux.alibaba.com> wrote: > On 2022/12/19 17:50, Yue Hu wrote: > > From: Yue Hu <huyue2@coolpad.com> > > > > Add compressed fragments support for dump feature. > > > > Signed-off-by: Yue Hu <huyue2@coolpad.com> > > --- > > dump/main.c | 78 ++++++++++++++++++++++++++++++++-------- > > include/erofs/internal.h | 1 + > > lib/zmap.c | 2 +- > > 3 files changed, 65 insertions(+), 16 deletions(-) > > > > diff --git a/dump/main.c b/dump/main.c > > index bc4f047..d387841 100644 > > --- a/dump/main.c > > +++ b/dump/main.c > > @@ -14,6 +14,8 @@ > > #include "erofs/inode.h" > > #include "erofs/io.h" > > #include "erofs/dir.h" > > +#include "erofs/compress.h" > > +#include "erofs/fragments.h" > > #include "../lib/liberofs_private.h" > > > > #ifdef HAVE_LIBUUID > > @@ -96,6 +98,7 @@ static struct erofsdump_feature feature_lists[] = { > > { false, EROFS_FEATURE_INCOMPAT_CHUNKED_FILE, "chunked_file" }, > > { false, EROFS_FEATURE_INCOMPAT_DEVICE_TABLE, "device_table" }, > > { false, EROFS_FEATURE_INCOMPAT_ZTAILPACKING, "ztailpacking" }, > > + { false, EROFS_FEATURE_INCOMPAT_FRAGMENTS, "fragments" }, > > }; > > > > static int erofsdump_readdir(struct erofs_dir_context *ctx); > > @@ -285,10 +288,12 @@ static int erofsdump_readdir(struct erofs_dir_context *ctx) > > } > > > > if (S_ISREG(vi.i_mode)) { > > - stats.files_total_origin_size += vi.i_size; > > - inc_file_extension_count(ctx->dname, ctx->de_namelen); > > + if (!erofs_is_packed_inode(&vi)) { > > + stats.files_total_origin_size += vi.i_size; > > + inc_file_extension_count(ctx->dname, ctx->de_namelen); > > + update_file_size_statistics(vi.i_size, true); > > + } > > stats.files_total_size += occupied_size; > > - update_file_size_statistics(vi.i_size, true); > > update_file_size_statistics(occupied_size, false); > > } > > > > @@ -320,6 +325,10 @@ static void erofsdump_show_fileinfo(bool show_extent) > > "%4d: %8" PRIu64 "..%8" PRIu64 " | %7" PRIu64 " : %10" PRIu64 "..%10" PRIu64 " | %7" PRIu64 "\n", > > "%4d: %8" PRIu64 "..%8" PRIu64 " | %7" PRIu64 " : %10" PRIu64 "..%10" PRIu64 " | %7" PRIu64 " # device %u\n" > > }; > > + const char *frag_ext_fmt[] = { > > + "%4d: %8" PRIu64 "..%8" PRIu64 " | %7" PRIu64 "\n", > > + "%4d: %8" PRIu64 "..%8" PRIu64 " | %7" PRIu64 " # device %u\n" > > + }; > > Why do we need another fmt rather than just fill fragment extent with I also plan to remove this fmt in v2. > physical addr 0? Okay. Just leave latter physical length as empty? > > > int err, i; > > erofs_off_t size; > > u16 access_mode; > > @@ -348,16 +357,31 @@ static void erofsdump_show_fileinfo(bool show_extent) > > } > > } > > > > + if (erofs_inode_is_data_compressed(inode.datalayout)) { > > + err = z_erofs_fill_inode_lazy(&inode); > > + if (err) { > > + erofs_err("read inode map header failed @ nid %llu", > > + inode.nid | 0ULL); > > + return; > > + } > > + } > > Why do we need to call z_erofs_fill_inode_lazy here? Used to check if the file has fragment. If yes, i think 'Compression ratio' can not be showed exactly. So, I'd like to show fragment information related instead as below: NID: 715 Links: 1 Layout: 1 Fragment: entire file NID: 715 Links: 1 Layout: 3 Fragment: tail of file > > > + > > err = erofs_get_occupied_size(&inode, &size); > > if (err) { > > erofs_err("get file size failed @ nid %llu", inode.nid | 0ULL); > > return; > > } > > > > - err = erofs_get_pathname(inode.nid, path, sizeof(path)); > > - if (err < 0) { > > - erofs_err("file path not found @ nid %llu", inode.nid | 0ULL); > > - return; > > Can we just ignore pathname if it doesn't exist? Okay. > > > + if (erofs_is_packed_inode(&inode) { > + strncpy(path, EROFS_PACKED_INODE, sizeof(path) - 1); > > + path[sizeof(path) - 1] = '\0'; > > + } else { > > + err = erofs_get_pathname(inode.nid, path, sizeof(path)); > > + if (err < 0) { > > + erofs_err("file path not found @ nid %llu", > > + inode.nid | 0ULL); > > + return; > > + } > > } > > > > strftime(timebuf, sizeof(timebuf), > > @@ -372,9 +396,13 @@ static void erofsdump_show_fileinfo(bool show_extent) > > file_category_types[erofs_mode_to_ftype(inode.i_mode)]); > > fprintf(stdout, "NID: %" PRIu64 " ", inode.nid); > > fprintf(stdout, "Links: %u ", inode.i_nlink); > > - fprintf(stdout, "Layout: %d Compression ratio: %.2f%%\n", > > - inode.datalayout, > > - (double)(100 * size) / (double)(inode.i_size)); > > + if (inode.z_advise & Z_EROFS_ADVISE_FRAGMENT_PCLUSTER) > > + fprintf(stdout, "Layout: %d Fragment: %s\n", > > + inode.datalayout, size ? "partial" : "full"); > > + else > > + fprintf(stdout, "Layout: %d Compression ratio: %.2f%%\n", > > + inode.datalayout, > > + (double)(100 * size) / (double)(inode.i_size)); > > fprintf(stdout, "Inode size: %d ", inode.inode_isize); > > fprintf(stdout, "Extent size: %u ", inode.extent_isize); > > fprintf(stdout, "Xattr size: %u\n", inode.xattr_isize); > > @@ -404,7 +432,8 @@ static void erofsdump_show_fileinfo(bool show_extent) > > if (!dumpcfg.show_extent) > > return; > > > > - fprintf(stdout, "\n Ext: logical offset | length : physical offset | length\n"); > > + fprintf(stdout, "\n Ext: logical offset | length%s\n", > > + size ? " : physical offset | length" : ""); > > while (map.m_la < inode.i_size) { > > struct erofs_map_dev mdev; > > > > @@ -425,10 +454,17 @@ static void erofsdump_show_fileinfo(bool show_extent) > > return; > > } > > > > - fprintf(stdout, ext_fmt[!!mdev.m_deviceid], extent_count++, > > - map.m_la, map.m_la + map.m_llen, map.m_llen, > > - mdev.m_pa, mdev.m_pa + map.m_plen, map.m_plen, > > - mdev.m_deviceid); > > + if (map.m_flags & EROFS_MAP_FRAGMENT) > > + fprintf(stdout, frag_ext_fmt[!!mdev.m_deviceid], > > + extent_count++, > > + map.m_la, map.m_la + map.m_llen, map.m_llen, > > + mdev.m_deviceid); > > except for the last fragment extent, the other extents all have physical > addr and length... It's true. What bothers me is how to better show fragment extent based on current format (especially no physical address and offset for fragment extent). Now, i think the fragment extent should be showed like below (just use a '0' to represent it)? a) the whole file is a fragment: Ext: logical offset | length : physical offset | length 1: 0.. 1046 | 1046 : 0 b) the tail of file is a fragment: Ext: logical offset | length : physical offset | length 0: 0.. 19672 | 19672 : 1314816.. 1323008 | 8192 ... 13: 165989.. 182474 | 16485 : 1421312.. 1429504 | 8192 14: 182474.. 196435 | 13961 : 0 > > > + else > > + fprintf(stdout, ext_fmt[!!mdev.m_deviceid], > > + extent_count++, > > + map.m_la, map.m_la + map.m_llen, map.m_llen, > > + mdev.m_pa, mdev.m_pa + map.m_plen, map.m_plen, > > + mdev.m_deviceid); > > map.m_la += map.m_llen; > > } > > fprintf(stdout, "%s: %d extents found\n", path, extent_count); > > @@ -537,6 +573,15 @@ static void erofsdump_print_statistic(void) > > erofs_err("read dir failed"); > > return; > > } > > + if (erofs_sb_has_fragments()) { > > + err = erofsdump_readdir(&(struct erofs_dir_context) { > > + .de_nid = sbi.packed_nid > > + }); > > why do we need this? Since some status need to be updated such as stats.files_total_size. > > Thanks, > Gao Xiang > > > + if (err) { > > + erofs_err("read packed inode failed"); > > + return; > > + } > > + } > > erofsdump_file_statistic(); > > erofsdump_filesize_distribution("Original", > > stats.file_original_size, > > @@ -563,6 +608,9 @@ static void erofsdump_show_superblock(void) > > sbi.xattr_blkaddr); > > fprintf(stdout, "Filesystem root nid: %llu\n", > > sbi.root_nid | 0ULL); > > + if (erofs_sb_has_fragments()) > > + fprintf(stdout, "Filesystem packed nid: %llu\n", > > + sbi.packed_nid | 0ULL); > > fprintf(stdout, "Filesystem inode count: %llu\n", > > sbi.inos | 0ULL); > > fprintf(stdout, "Filesystem created: %s", > > diff --git a/include/erofs/internal.h b/include/erofs/internal.h > > index 206913c..947894d 100644 > > --- a/include/erofs/internal.h > > +++ b/include/erofs/internal.h > > @@ -383,6 +383,7 @@ int erofs_listxattr(struct erofs_inode *vi, char *buffer, size_t buffer_size); > > > > /* zmap.c */ > > int z_erofs_fill_inode(struct erofs_inode *vi); > > +int z_erofs_fill_inode_lazy(struct erofs_inode *vi); > > int z_erofs_map_blocks_iter(struct erofs_inode *vi, > > struct erofs_map_blocks *map, int flags); > > > > diff --git a/lib/zmap.c b/lib/zmap.c > > index 89e9da1..41e0713 100644 > > --- a/lib/zmap.c > > +++ b/lib/zmap.c > > @@ -29,7 +29,7 @@ int z_erofs_fill_inode(struct erofs_inode *vi) > > return 0; > > } > > > > -static int z_erofs_fill_inode_lazy(struct erofs_inode *vi) > > +int z_erofs_fill_inode_lazy(struct erofs_inode *vi) > > { > > int ret; > > erofs_off_t pos; ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC PATCH 2/2] erofs-utils: dump: support fragments 2023-01-04 6:16 ` Yue Hu @ 2023-01-04 6:44 ` Xiang Gao 2023-01-04 7:12 ` Yue Hu 0 siblings, 1 reply; 7+ messages in thread From: Xiang Gao @ 2023-01-04 6:44 UTC (permalink / raw) To: Yue Hu; +Cc: Yue Hu, linux-erofs, zhangwen On 2023/1/4 14:16, Yue Hu wrote: > Hi Xiang, > > Thanks for the reviewing. I'm planning to send v2. > > On Wed, 4 Jan 2023 11:13:55 +0800 > Xiang Gao <hsiangkao@linux.alibaba.com> wrote: > >> On 2022/12/19 17:50, Yue Hu wrote: >>> From: Yue Hu <huyue2@coolpad.com> >>> >>> Add compressed fragments support for dump feature. >>> >>> Signed-off-by: Yue Hu <huyue2@coolpad.com> >>> --- >>> dump/main.c | 78 ++++++++++++++++++++++++++++++++-------- >>> include/erofs/internal.h | 1 + >>> lib/zmap.c | 2 +- >>> 3 files changed, 65 insertions(+), 16 deletions(-) >>> >>> diff --git a/dump/main.c b/dump/main.c >>> index bc4f047..d387841 100644 >>> --- a/dump/main.c >>> +++ b/dump/main.c >>> @@ -14,6 +14,8 @@ >>> #include "erofs/inode.h" >>> #include "erofs/io.h" >>> #include "erofs/dir.h" >>> +#include "erofs/compress.h" >>> +#include "erofs/fragments.h" >>> #include "../lib/liberofs_private.h" >>> >>> #ifdef HAVE_LIBUUID >>> @@ -96,6 +98,7 @@ static struct erofsdump_feature feature_lists[] = { >>> { false, EROFS_FEATURE_INCOMPAT_CHUNKED_FILE, "chunked_file" }, >>> { false, EROFS_FEATURE_INCOMPAT_DEVICE_TABLE, "device_table" }, >>> { false, EROFS_FEATURE_INCOMPAT_ZTAILPACKING, "ztailpacking" }, >>> + { false, EROFS_FEATURE_INCOMPAT_FRAGMENTS, "fragments" }, >>> }; >>> >>> static int erofsdump_readdir(struct erofs_dir_context *ctx); >>> @@ -285,10 +288,12 @@ static int erofsdump_readdir(struct erofs_dir_context *ctx) >>> } >>> >>> if (S_ISREG(vi.i_mode)) { >>> - stats.files_total_origin_size += vi.i_size; >>> - inc_file_extension_count(ctx->dname, ctx->de_namelen); >>> + if (!erofs_is_packed_inode(&vi)) { >>> + stats.files_total_origin_size += vi.i_size; >>> + inc_file_extension_count(ctx->dname, ctx->de_namelen); >>> + update_file_size_statistics(vi.i_size, true); >>> + } >>> stats.files_total_size += occupied_size; >>> - update_file_size_statistics(vi.i_size, true); >>> update_file_size_statistics(occupied_size, false); >>> } >>> >>> @@ -320,6 +325,10 @@ static void erofsdump_show_fileinfo(bool show_extent) >>> "%4d: %8" PRIu64 "..%8" PRIu64 " | %7" PRIu64 " : %10" PRIu64 "..%10" PRIu64 " | %7" PRIu64 "\n", >>> "%4d: %8" PRIu64 "..%8" PRIu64 " | %7" PRIu64 " : %10" PRIu64 "..%10" PRIu64 " | %7" PRIu64 " # device %u\n" >>> }; >>> + const char *frag_ext_fmt[] = { >>> + "%4d: %8" PRIu64 "..%8" PRIu64 " | %7" PRIu64 "\n", >>> + "%4d: %8" PRIu64 "..%8" PRIu64 " | %7" PRIu64 " # device %u\n" >>> + }; >> >> Why do we need another fmt rather than just fill fragment extent with > > I also plan to remove this fmt in v2. > >> physical addr 0? > > Okay. Just leave latter physical length as empty? > >> >>> int err, i; >>> erofs_off_t size; >>> u16 access_mode; >>> @@ -348,16 +357,31 @@ static void erofsdump_show_fileinfo(bool show_extent) >>> } >>> } >>> >>> + if (erofs_inode_is_data_compressed(inode.datalayout)) { >>> + err = z_erofs_fill_inode_lazy(&inode); >>> + if (err) { >>> + erofs_err("read inode map header failed @ nid %llu", >>> + inode.nid | 0ULL); >>> + return; >>> + } >>> + } >> >> Why do we need to call z_erofs_fill_inode_lazy here? > > Used to check if the file has fragment. If yes, i think 'Compression ratio' can not be showed > exactly. > > So, I'd like to show fragment information related instead as below: > > NID: 715 Links: 1 Layout: 1 Fragment: entire file > > NID: 715 Links: 1 Layout: 3 Fragment: tail of file I think no need to distinguish these two stuffs considering extra complexity. > >> >>> + >>> err = erofs_get_occupied_size(&inode, &size); >>> if (err) { >>> erofs_err("get file size failed @ nid %llu", inode.nid | 0ULL); >>> return; >>> } >>> >>> - err = erofs_get_pathname(inode.nid, path, sizeof(path)); >>> - if (err < 0) { >>> - erofs_err("file path not found @ nid %llu", inode.nid | 0ULL); >>> - return; >> >> Can we just ignore pathname if it doesn't exist? > > Okay. > >> >>> + if (erofs_is_packed_inode(&inode) { > + strncpy(path, EROFS_PACKED_INODE, sizeof(path) - 1); >>> + path[sizeof(path) - 1] = '\0'; >>> + } else { >>> + err = erofs_get_pathname(inode.nid, path, sizeof(path)); >>> + if (err < 0) { >>> + erofs_err("file path not found @ nid %llu", >>> + inode.nid | 0ULL); >>> + return; >>> + } >>> } >>> >>> strftime(timebuf, sizeof(timebuf), >>> @@ -372,9 +396,13 @@ static void erofsdump_show_fileinfo(bool show_extent) >>> file_category_types[erofs_mode_to_ftype(inode.i_mode)]); >>> fprintf(stdout, "NID: %" PRIu64 " ", inode.nid); >>> fprintf(stdout, "Links: %u ", inode.i_nlink); >>> - fprintf(stdout, "Layout: %d Compression ratio: %.2f%%\n", >>> - inode.datalayout, >>> - (double)(100 * size) / (double)(inode.i_size)); >>> + if (inode.z_advise & Z_EROFS_ADVISE_FRAGMENT_PCLUSTER) >>> + fprintf(stdout, "Layout: %d Fragment: %s\n", >>> + inode.datalayout, size ? "partial" : "full"); >>> + else >>> + fprintf(stdout, "Layout: %d Compression ratio: %.2f%%\n", >>> + inode.datalayout, >>> + (double)(100 * size) / (double)(inode.i_size)); >>> fprintf(stdout, "Inode size: %d ", inode.inode_isize); >>> fprintf(stdout, "Extent size: %u ", inode.extent_isize); >>> fprintf(stdout, "Xattr size: %u\n", inode.xattr_isize); >>> @@ -404,7 +432,8 @@ static void erofsdump_show_fileinfo(bool show_extent) >>> if (!dumpcfg.show_extent) >>> return; >>> >>> - fprintf(stdout, "\n Ext: logical offset | length : physical offset | length\n"); >>> + fprintf(stdout, "\n Ext: logical offset | length%s\n", >>> + size ? " : physical offset | length" : ""); >>> while (map.m_la < inode.i_size) { >>> struct erofs_map_dev mdev; >>> >>> @@ -425,10 +454,17 @@ static void erofsdump_show_fileinfo(bool show_extent) >>> return; >>> } >>> >>> - fprintf(stdout, ext_fmt[!!mdev.m_deviceid], extent_count++, >>> - map.m_la, map.m_la + map.m_llen, map.m_llen, >>> - mdev.m_pa, mdev.m_pa + map.m_plen, map.m_plen, >>> - mdev.m_deviceid); >>> + if (map.m_flags & EROFS_MAP_FRAGMENT) >>> + fprintf(stdout, frag_ext_fmt[!!mdev.m_deviceid], >>> + extent_count++, >>> + map.m_la, map.m_la + map.m_llen, map.m_llen, >>> + mdev.m_deviceid); >> >> except for the last fragment extent, the other extents all have physical >> addr and length... > > It's true. > > What bothers me is how to better show fragment extent based on current format (especially > no physical address and offset for fragment extent). > > Now, i think the fragment extent should be showed like below (just use a '0' to represent it)? > > a) the whole file is a fragment: > > Ext: logical offset | length : physical offset | length > 1: 0.. 1046 | 1046 : 0 Can we use Ext: logical offset | length : physical offset | length 1: 0.. 1046 | 1046 : 0..0 | 0 > > b) the tail of file is a fragment: > > Ext: logical offset | length : physical offset | length > 0: 0.. 19672 | 19672 : 1314816.. 1323008 | 8192 > ... > 13: 165989.. 182474 | 16485 : 1421312.. 1429504 | 8192 > 14: 182474.. 196435 | 13961 : 0 Here 14: 182474.. 196435 | 13961 : 0.. 0 | 0 as well? > >> >>> + else >>> + fprintf(stdout, ext_fmt[!!mdev.m_deviceid], >>> + extent_count++, >>> + map.m_la, map.m_la + map.m_llen, map.m_llen, >>> + mdev.m_pa, mdev.m_pa + map.m_plen, map.m_plen, >>> + mdev.m_deviceid); >>> map.m_la += map.m_llen; >>> } >>> fprintf(stdout, "%s: %d extents found\n", path, extent_count); >>> @@ -537,6 +573,15 @@ static void erofsdump_print_statistic(void) >>> erofs_err("read dir failed"); >>> return; >>> } >>> + if (erofs_sb_has_fragments()) { >>> + err = erofsdump_readdir(&(struct erofs_dir_context) { >>> + .de_nid = sbi.packed_nid >>> + }); >> >> why do we need this? > > Since some status need to be updated such as stats.files_total_size. why should packed file be recorded in files_total_size? Thanks, Gao Xiang > >> >> Thanks, >> Gao Xiang ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC PATCH 2/2] erofs-utils: dump: support fragments 2023-01-04 6:44 ` Xiang Gao @ 2023-01-04 7:12 ` Yue Hu 2023-01-04 7:19 ` Xiang Gao 0 siblings, 1 reply; 7+ messages in thread From: Yue Hu @ 2023-01-04 7:12 UTC (permalink / raw) To: Xiang Gao; +Cc: Yue Hu, linux-erofs, zhangwen On Wed, 4 Jan 2023 14:44:35 +0800 Xiang Gao <hsiangkao@linux.alibaba.com> wrote: > On 2023/1/4 14:16, Yue Hu wrote: > > Hi Xiang, > > > > Thanks for the reviewing. I'm planning to send v2. > > > > On Wed, 4 Jan 2023 11:13:55 +0800 > > Xiang Gao <hsiangkao@linux.alibaba.com> wrote: > > > >> On 2022/12/19 17:50, Yue Hu wrote: > >>> From: Yue Hu <huyue2@coolpad.com> > >>> > >>> Add compressed fragments support for dump feature. > >>> > >>> Signed-off-by: Yue Hu <huyue2@coolpad.com> > >>> --- > >>> dump/main.c | 78 ++++++++++++++++++++++++++++++++-------- > >>> include/erofs/internal.h | 1 + > >>> lib/zmap.c | 2 +- > >>> 3 files changed, 65 insertions(+), 16 deletions(-) > >>> > >>> diff --git a/dump/main.c b/dump/main.c > >>> index bc4f047..d387841 100644 > >>> --- a/dump/main.c > >>> +++ b/dump/main.c > >>> @@ -14,6 +14,8 @@ > >>> #include "erofs/inode.h" > >>> #include "erofs/io.h" > >>> #include "erofs/dir.h" > >>> +#include "erofs/compress.h" > >>> +#include "erofs/fragments.h" > >>> #include "../lib/liberofs_private.h" > >>> > >>> #ifdef HAVE_LIBUUID > >>> @@ -96,6 +98,7 @@ static struct erofsdump_feature feature_lists[] = { > >>> { false, EROFS_FEATURE_INCOMPAT_CHUNKED_FILE, "chunked_file" }, > >>> { false, EROFS_FEATURE_INCOMPAT_DEVICE_TABLE, "device_table" }, > >>> { false, EROFS_FEATURE_INCOMPAT_ZTAILPACKING, "ztailpacking" }, > >>> + { false, EROFS_FEATURE_INCOMPAT_FRAGMENTS, "fragments" }, > >>> }; > >>> > >>> static int erofsdump_readdir(struct erofs_dir_context *ctx); > >>> @@ -285,10 +288,12 @@ static int erofsdump_readdir(struct erofs_dir_context *ctx) > >>> } > >>> > >>> if (S_ISREG(vi.i_mode)) { > >>> - stats.files_total_origin_size += vi.i_size; > >>> - inc_file_extension_count(ctx->dname, ctx->de_namelen); > >>> + if (!erofs_is_packed_inode(&vi)) { > >>> + stats.files_total_origin_size += vi.i_size; > >>> + inc_file_extension_count(ctx->dname, ctx->de_namelen); > >>> + update_file_size_statistics(vi.i_size, true); > >>> + } > >>> stats.files_total_size += occupied_size; > >>> - update_file_size_statistics(vi.i_size, true); > >>> update_file_size_statistics(occupied_size, false); > >>> } > >>> > >>> @@ -320,6 +325,10 @@ static void erofsdump_show_fileinfo(bool show_extent) > >>> "%4d: %8" PRIu64 "..%8" PRIu64 " | %7" PRIu64 " : %10" PRIu64 "..%10" PRIu64 " | %7" PRIu64 "\n", > >>> "%4d: %8" PRIu64 "..%8" PRIu64 " | %7" PRIu64 " : %10" PRIu64 "..%10" PRIu64 " | %7" PRIu64 " # device %u\n" > >>> }; > >>> + const char *frag_ext_fmt[] = { > >>> + "%4d: %8" PRIu64 "..%8" PRIu64 " | %7" PRIu64 "\n", > >>> + "%4d: %8" PRIu64 "..%8" PRIu64 " | %7" PRIu64 " # device %u\n" > >>> + }; > >> > >> Why do we need another fmt rather than just fill fragment extent with > > > > I also plan to remove this fmt in v2. > > > >> physical addr 0? > > > > Okay. Just leave latter physical length as empty? > > > >> > >>> int err, i; > >>> erofs_off_t size; > >>> u16 access_mode; > >>> @@ -348,16 +357,31 @@ static void erofsdump_show_fileinfo(bool show_extent) > >>> } > >>> } > >>> > >>> + if (erofs_inode_is_data_compressed(inode.datalayout)) { > >>> + err = z_erofs_fill_inode_lazy(&inode); > >>> + if (err) { > >>> + erofs_err("read inode map header failed @ nid %llu", > >>> + inode.nid | 0ULL); > >>> + return; > >>> + } > >>> + } > >> > >> Why do we need to call z_erofs_fill_inode_lazy here? > > > > Used to check if the file has fragment. If yes, i think 'Compression ratio' can not be showed > > exactly. > > > > So, I'd like to show fragment information related instead as below: > > > > NID: 715 Links: 1 Layout: 1 Fragment: entire file > > > > NID: 715 Links: 1 Layout: 3 Fragment: tail of file > > I think no need to distinguish these two stuffs considering extra > complexity. So, no need to check fragment or not? just keep the original look? > > > > >> > >>> + > >>> err = erofs_get_occupied_size(&inode, &size); > >>> if (err) { > >>> erofs_err("get file size failed @ nid %llu", inode.nid | 0ULL); > >>> return; > >>> } > >>> > >>> - err = erofs_get_pathname(inode.nid, path, sizeof(path)); > >>> - if (err < 0) { > >>> - erofs_err("file path not found @ nid %llu", inode.nid | 0ULL); > >>> - return; > >> > >> Can we just ignore pathname if it doesn't exist? > > > > Okay. > > > >> > >>> + if (erofs_is_packed_inode(&inode) { > + strncpy(path, EROFS_PACKED_INODE, sizeof(path) - 1); > >>> + path[sizeof(path) - 1] = '\0'; > >>> + } else { > >>> + err = erofs_get_pathname(inode.nid, path, sizeof(path)); > >>> + if (err < 0) { > >>> + erofs_err("file path not found @ nid %llu", > >>> + inode.nid | 0ULL); > >>> + return; > >>> + } > >>> } > >>> > >>> strftime(timebuf, sizeof(timebuf), > >>> @@ -372,9 +396,13 @@ static void erofsdump_show_fileinfo(bool show_extent) > >>> file_category_types[erofs_mode_to_ftype(inode.i_mode)]); > >>> fprintf(stdout, "NID: %" PRIu64 " ", inode.nid); > >>> fprintf(stdout, "Links: %u ", inode.i_nlink); > >>> - fprintf(stdout, "Layout: %d Compression ratio: %.2f%%\n", > >>> - inode.datalayout, > >>> - (double)(100 * size) / (double)(inode.i_size)); > >>> + if (inode.z_advise & Z_EROFS_ADVISE_FRAGMENT_PCLUSTER) > >>> + fprintf(stdout, "Layout: %d Fragment: %s\n", > >>> + inode.datalayout, size ? "partial" : "full"); > >>> + else > >>> + fprintf(stdout, "Layout: %d Compression ratio: %.2f%%\n", > >>> + inode.datalayout, > >>> + (double)(100 * size) / (double)(inode.i_size)); > >>> fprintf(stdout, "Inode size: %d ", inode.inode_isize); > >>> fprintf(stdout, "Extent size: %u ", inode.extent_isize); > >>> fprintf(stdout, "Xattr size: %u\n", inode.xattr_isize); > >>> @@ -404,7 +432,8 @@ static void erofsdump_show_fileinfo(bool show_extent) > >>> if (!dumpcfg.show_extent) > >>> return; > >>> > >>> - fprintf(stdout, "\n Ext: logical offset | length : physical offset | length\n"); > >>> + fprintf(stdout, "\n Ext: logical offset | length%s\n", > >>> + size ? " : physical offset | length" : ""); > >>> while (map.m_la < inode.i_size) { > >>> struct erofs_map_dev mdev; > >>> > >>> @@ -425,10 +454,17 @@ static void erofsdump_show_fileinfo(bool show_extent) > >>> return; > >>> } > >>> > >>> - fprintf(stdout, ext_fmt[!!mdev.m_deviceid], extent_count++, > >>> - map.m_la, map.m_la + map.m_llen, map.m_llen, > >>> - mdev.m_pa, mdev.m_pa + map.m_plen, map.m_plen, > >>> - mdev.m_deviceid); > >>> + if (map.m_flags & EROFS_MAP_FRAGMENT) > >>> + fprintf(stdout, frag_ext_fmt[!!mdev.m_deviceid], > >>> + extent_count++, > >>> + map.m_la, map.m_la + map.m_llen, map.m_llen, > >>> + mdev.m_deviceid); > >> > >> except for the last fragment extent, the other extents all have physical > >> addr and length... > > > > It's true. > > > > What bothers me is how to better show fragment extent based on current format (especially > > no physical address and offset for fragment extent). > > > > Now, i think the fragment extent should be showed like below (just use a '0' to represent it)? > > > > a) the whole file is a fragment: > > > > Ext: logical offset | length : physical offset | length > > 1: 0.. 1046 | 1046 : 0 > > > Can we use > Ext: logical offset | length : physical offset | length > 1: 0.. 1046 | 1046 : 0..0 | 0 > > > > > b) the tail of file is a fragment: > > > > Ext: logical offset | length : physical offset | length > > 0: 0.. 19672 | 19672 : 1314816.. 1323008 | 8192 > > ... > > 13: 165989.. 182474 | 16485 : 1421312.. 1429504 | 8192 > > 14: 182474.. 196435 | 13961 : 0 > > Here > 14: 182474.. 196435 | 13961 : 0.. 0 | 0 > > as well? No problem now. > > > > >> > >>> + else > >>> + fprintf(stdout, ext_fmt[!!mdev.m_deviceid], > >>> + extent_count++, > >>> + map.m_la, map.m_la + map.m_llen, map.m_llen, > >>> + mdev.m_pa, mdev.m_pa + map.m_plen, map.m_plen, > >>> + mdev.m_deviceid); > >>> map.m_la += map.m_llen; > >>> } > >>> fprintf(stdout, "%s: %d extents found\n", path, extent_count); > >>> @@ -537,6 +573,15 @@ static void erofsdump_print_statistic(void) > >>> erofs_err("read dir failed"); > >>> return; > >>> } > >>> + if (erofs_sb_has_fragments()) { > >>> + err = erofsdump_readdir(&(struct erofs_dir_context) { > >>> + .de_nid = sbi.packed_nid > >>> + }); > >> > >> why do we need this? > > > > Since some status need to be updated such as stats.files_total_size. > > why should packed file be recorded in files_total_size? Mainly related to compression ratio. files_total_size records on-disk (un)compressed size. 296 stats.files_total_size += occupied_size; 544 stats.compress_rate = (double)(100 * stats.files_total_size) / 545 (double)(stats.files_total_origin_size); The file system compression ratio will be incorrect (since all compressed fragments are missing). > > Thanks, > Gao Xiang > > > > >> > >> Thanks, > >> Gao Xiang ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC PATCH 2/2] erofs-utils: dump: support fragments 2023-01-04 7:12 ` Yue Hu @ 2023-01-04 7:19 ` Xiang Gao 0 siblings, 0 replies; 7+ messages in thread From: Xiang Gao @ 2023-01-04 7:19 UTC (permalink / raw) To: Yue Hu; +Cc: Yue Hu, linux-erofs, Qi Wang, zhangwen (+ add Qi Wang) On 2023/1/4 15:12, Yue Hu wrote: > On Wed, 4 Jan 2023 14:44:35 +0800 > Xiang Gao <hsiangkao@linux.alibaba.com> wrote: > >> On 2023/1/4 14:16, Yue Hu wrote: >>> Hi Xiang, >>> >>> Thanks for the reviewing. I'm planning to send v2. >>> >>> On Wed, 4 Jan 2023 11:13:55 +0800 >>> Xiang Gao <hsiangkao@linux.alibaba.com> wrote: >>> >>>> On 2022/12/19 17:50, Yue Hu wrote: >>>>> From: Yue Hu <huyue2@coolpad.com> >>>>> >>>>> Add compressed fragments support for dump feature. >>>>> >>>>> Signed-off-by: Yue Hu <huyue2@coolpad.com> >>>>> --- >>>>> dump/main.c | 78 ++++++++++++++++++++++++++++++++-------- >>>>> include/erofs/internal.h | 1 + >>>>> lib/zmap.c | 2 +- >>>>> 3 files changed, 65 insertions(+), 16 deletions(-) >>>>> >>>>> diff --git a/dump/main.c b/dump/main.c >>>>> index bc4f047..d387841 100644 >>>>> --- a/dump/main.c >>>>> +++ b/dump/main.c >>>>> @@ -14,6 +14,8 @@ >>>>> #include "erofs/inode.h" >>>>> #include "erofs/io.h" >>>>> #include "erofs/dir.h" >>>>> +#include "erofs/compress.h" >>>>> +#include "erofs/fragments.h" >>>>> #include "../lib/liberofs_private.h" >>>>> >>>>> #ifdef HAVE_LIBUUID >>>>> @@ -96,6 +98,7 @@ static struct erofsdump_feature feature_lists[] = { >>>>> { false, EROFS_FEATURE_INCOMPAT_CHUNKED_FILE, "chunked_file" }, >>>>> { false, EROFS_FEATURE_INCOMPAT_DEVICE_TABLE, "device_table" }, >>>>> { false, EROFS_FEATURE_INCOMPAT_ZTAILPACKING, "ztailpacking" }, >>>>> + { false, EROFS_FEATURE_INCOMPAT_FRAGMENTS, "fragments" }, >>>>> }; >>>>> >>>>> static int erofsdump_readdir(struct erofs_dir_context *ctx); >>>>> @@ -285,10 +288,12 @@ static int erofsdump_readdir(struct erofs_dir_context *ctx) >>>>> } >>>>> >>>>> if (S_ISREG(vi.i_mode)) { >>>>> - stats.files_total_origin_size += vi.i_size; >>>>> - inc_file_extension_count(ctx->dname, ctx->de_namelen); >>>>> + if (!erofs_is_packed_inode(&vi)) { >>>>> + stats.files_total_origin_size += vi.i_size; >>>>> + inc_file_extension_count(ctx->dname, ctx->de_namelen); >>>>> + update_file_size_statistics(vi.i_size, true); >>>>> + } >>>>> stats.files_total_size += occupied_size; >>>>> - update_file_size_statistics(vi.i_size, true); >>>>> update_file_size_statistics(occupied_size, false); >>>>> } >>>>> >>>>> @@ -320,6 +325,10 @@ static void erofsdump_show_fileinfo(bool show_extent) >>>>> "%4d: %8" PRIu64 "..%8" PRIu64 " | %7" PRIu64 " : %10" PRIu64 "..%10" PRIu64 " | %7" PRIu64 "\n", >>>>> "%4d: %8" PRIu64 "..%8" PRIu64 " | %7" PRIu64 " : %10" PRIu64 "..%10" PRIu64 " | %7" PRIu64 " # device %u\n" >>>>> }; >>>>> + const char *frag_ext_fmt[] = { >>>>> + "%4d: %8" PRIu64 "..%8" PRIu64 " | %7" PRIu64 "\n", >>>>> + "%4d: %8" PRIu64 "..%8" PRIu64 " | %7" PRIu64 " # device %u\n" >>>>> + }; >>>> >>>> Why do we need another fmt rather than just fill fragment extent with >>> >>> I also plan to remove this fmt in v2. >>> >>>> physical addr 0? >>> >>> Okay. Just leave latter physical length as empty? >>> >>>> >>>>> int err, i; >>>>> erofs_off_t size; >>>>> u16 access_mode; >>>>> @@ -348,16 +357,31 @@ static void erofsdump_show_fileinfo(bool show_extent) >>>>> } >>>>> } >>>>> >>>>> + if (erofs_inode_is_data_compressed(inode.datalayout)) { >>>>> + err = z_erofs_fill_inode_lazy(&inode); >>>>> + if (err) { >>>>> + erofs_err("read inode map header failed @ nid %llu", >>>>> + inode.nid | 0ULL); >>>>> + return; >>>>> + } >>>>> + } >>>> >>>> Why do we need to call z_erofs_fill_inode_lazy here? >>> >>> Used to check if the file has fragment. If yes, i think 'Compression ratio' can not be showed >>> exactly. >>> >>> So, I'd like to show fragment information related instead as below: >>> >>> NID: 715 Links: 1 Layout: 1 Fragment: entire file >>> >>> NID: 715 Links: 1 Layout: 3 Fragment: tail of file >> >> I think no need to distinguish these two stuffs considering extra >> complexity. > > So, no need to check fragment or not? just keep the original look? because we could get such information by observing inode extents directly. > >> >>> >>>> >>>>> + >>>>> err = erofs_get_occupied_size(&inode, &size); >>>>> if (err) { >>>>> erofs_err("get file size failed @ nid %llu", inode.nid | 0ULL); >>>>> return; >>>>> } >>>>> >>>>> - err = erofs_get_pathname(inode.nid, path, sizeof(path)); >>>>> - if (err < 0) { >>>>> - erofs_err("file path not found @ nid %llu", inode.nid | 0ULL); >>>>> - return; >>>> >>>> Can we just ignore pathname if it doesn't exist? >>> >>> Okay. >>> >>>> >>>>> + if (erofs_is_packed_inode(&inode) { > + strncpy(path, EROFS_PACKED_INODE, sizeof(path) - 1); >>>>> + path[sizeof(path) - 1] = '\0'; >>>>> + } else { >>>>> + err = erofs_get_pathname(inode.nid, path, sizeof(path)); >>>>> + if (err < 0) { >>>>> + erofs_err("file path not found @ nid %llu", >>>>> + inode.nid | 0ULL); >>>>> + return; >>>>> + } >>>>> } >>>>> >>>>> strftime(timebuf, sizeof(timebuf), >>>>> @@ -372,9 +396,13 @@ static void erofsdump_show_fileinfo(bool show_extent) >>>>> file_category_types[erofs_mode_to_ftype(inode.i_mode)]); >>>>> fprintf(stdout, "NID: %" PRIu64 " ", inode.nid); >>>>> fprintf(stdout, "Links: %u ", inode.i_nlink); >>>>> - fprintf(stdout, "Layout: %d Compression ratio: %.2f%%\n", >>>>> - inode.datalayout, >>>>> - (double)(100 * size) / (double)(inode.i_size)); >>>>> + if (inode.z_advise & Z_EROFS_ADVISE_FRAGMENT_PCLUSTER) >>>>> + fprintf(stdout, "Layout: %d Fragment: %s\n", >>>>> + inode.datalayout, size ? "partial" : "full"); >>>>> + else >>>>> + fprintf(stdout, "Layout: %d Compression ratio: %.2f%%\n", >>>>> + inode.datalayout, >>>>> + (double)(100 * size) / (double)(inode.i_size)); >>>>> fprintf(stdout, "Inode size: %d ", inode.inode_isize); >>>>> fprintf(stdout, "Extent size: %u ", inode.extent_isize); >>>>> fprintf(stdout, "Xattr size: %u\n", inode.xattr_isize); >>>>> @@ -404,7 +432,8 @@ static void erofsdump_show_fileinfo(bool show_extent) >>>>> if (!dumpcfg.show_extent) >>>>> return; >>>>> >>>>> - fprintf(stdout, "\n Ext: logical offset | length : physical offset | length\n"); >>>>> + fprintf(stdout, "\n Ext: logical offset | length%s\n", >>>>> + size ? " : physical offset | length" : ""); >>>>> while (map.m_la < inode.i_size) { >>>>> struct erofs_map_dev mdev; >>>>> >>>>> @@ -425,10 +454,17 @@ static void erofsdump_show_fileinfo(bool show_extent) >>>>> return; >>>>> } >>>>> >>>>> - fprintf(stdout, ext_fmt[!!mdev.m_deviceid], extent_count++, >>>>> - map.m_la, map.m_la + map.m_llen, map.m_llen, >>>>> - mdev.m_pa, mdev.m_pa + map.m_plen, map.m_plen, >>>>> - mdev.m_deviceid); >>>>> + if (map.m_flags & EROFS_MAP_FRAGMENT) >>>>> + fprintf(stdout, frag_ext_fmt[!!mdev.m_deviceid], >>>>> + extent_count++, >>>>> + map.m_la, map.m_la + map.m_llen, map.m_llen, >>>>> + mdev.m_deviceid); >>>> >>>> except for the last fragment extent, the other extents all have physical >>>> addr and length... >>> >>> It's true. >>> >>> What bothers me is how to better show fragment extent based on current format (especially >>> no physical address and offset for fragment extent). >>> >>> Now, i think the fragment extent should be showed like below (just use a '0' to represent it)? >>> >>> a) the whole file is a fragment: >>> >>> Ext: logical offset | length : physical offset | length >>> 1: 0.. 1046 | 1046 : 0 >> >> >> Can we use >> Ext: logical offset | length : physical offset | length >> 1: 0.. 1046 | 1046 : 0..0 | 0 >> >>> >>> b) the tail of file is a fragment: >>> >>> Ext: logical offset | length : physical offset | length >>> 0: 0.. 19672 | 19672 : 1314816.. 1323008 | 8192 >>> ... >>> 13: 165989.. 182474 | 16485 : 1421312.. 1429504 | 8192 >>> 14: 182474.. 196435 | 13961 : 0 >> >> Here >> 14: 182474.. 196435 | 13961 : 0.. 0 | 0 >> >> as well? > > No problem now. > >> >>> >>>> >>>>> + else >>>>> + fprintf(stdout, ext_fmt[!!mdev.m_deviceid], >>>>> + extent_count++, >>>>> + map.m_la, map.m_la + map.m_llen, map.m_llen, >>>>> + mdev.m_pa, mdev.m_pa + map.m_plen, map.m_plen, >>>>> + mdev.m_deviceid); >>>>> map.m_la += map.m_llen; >>>>> } >>>>> fprintf(stdout, "%s: %d extents found\n", path, extent_count); >>>>> @@ -537,6 +573,15 @@ static void erofsdump_print_statistic(void) >>>>> erofs_err("read dir failed"); >>>>> return; >>>>> } >>>>> + if (erofs_sb_has_fragments()) { >>>>> + err = erofsdump_readdir(&(struct erofs_dir_context) { >>>>> + .de_nid = sbi.packed_nid >>>>> + }); >>>> >>>> why do we need this? >>> >>> Since some status need to be updated such as stats.files_total_size. >> >> why should packed file be recorded in files_total_size? > > Mainly related to compression ratio. files_total_size records on-disk (un)compressed size. > > 296 stats.files_total_size += occupied_size; > > 544 stats.compress_rate = (double)(100 * stats.files_total_size) / > 545 (double)(stats.files_total_origin_size); > > The file system compression ratio will be incorrect (since all compressed fragments are > missing). I really think erofsdump should deserve a deep cleanup first. we should count the correct compression ratio for the packed file. But I don't think we need: static int erofsdump_readdir(struct erofs_dir_context *ctx) { ... stats.files++; stats.file_category_stat[erofs_mode_to_ftype(vi.i_mode)]++; ... } Thanks, Gao Xiang > >> >> Thanks, >> Gao Xiang >> >>> >>>> >>>> Thanks, >>>> Gao Xiang ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2023-01-04 7:19 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-12-19 9:50 [RFC PATCH 1/2] erofs-utils: dump: cleanup update_file_size_statatics() Yue Hu 2022-12-19 9:50 ` [RFC PATCH 2/2] erofs-utils: dump: support fragments Yue Hu 2023-01-04 3:13 ` Xiang Gao 2023-01-04 6:16 ` Yue Hu 2023-01-04 6:44 ` Xiang Gao 2023-01-04 7:12 ` Yue Hu 2023-01-04 7:19 ` Xiang Gao
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox