[RFC PATCH 1/2] erofs-utils: dump: cleanup update_file_size_statatics()

[RFC PATCH 1/2] erofs-utils: dump: cleanup update_file_size_statatics()

[Yue Hu]

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

[RFC PATCH 2/2] erofs-utils: dump: support fragments

[Yue Hu]

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

Re: [RFC PATCH 2/2] erofs-utils: dump: support fragments

[Xiang Gao]

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;

Re: [RFC PATCH 2/2] erofs-utils: dump: support fragments

[Yue Hu]

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;  

Re: [RFC PATCH 2/2] erofs-utils: dump: support fragments

[Xiang Gao]

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

Re: [RFC PATCH 2/2] erofs-utils: dump: support fragments

[Yue Hu]

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  

Re: [RFC PATCH 2/2] erofs-utils: dump: support fragments

[Xiang Gao]

(+ 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