[PATCH v2 1/3] erofs-utils: lib: export parts of erofs_pread()

[PATCH v2 1/3] erofs-utils: lib: export parts of erofs_pread()

[Yue Hu]

From: Yue Hu <huyue2@coolpad.com>

Export parts of erofs_pread() to avoid duplicated code in
erofs_verify_inode_data(). Let's make two helpers for this.

Signed-off-by: Yue Hu <huyue2@coolpad.com>
---
v2: use parameter trimmed instead of partial

 include/erofs/internal.h |   5 ++
 lib/data.c               | 154 ++++++++++++++++++++++-----------------
 2 files changed, 92 insertions(+), 67 deletions(-)

diff --git a/include/erofs/internal.h b/include/erofs/internal.h
index 206913c..47240f5 100644
--- a/include/erofs/internal.h
+++ b/include/erofs/internal.h
@@ -355,6 +355,11 @@ int erofs_pread(struct erofs_inode *inode, char *buf,
 int erofs_map_blocks(struct erofs_inode *inode,
 		struct erofs_map_blocks *map, int flags);
 int erofs_map_dev(struct erofs_sb_info *sbi, struct erofs_map_dev *map);
+int erofs_read_raw_data_mapped(struct erofs_map_blocks *map, char *buffer,
+			       u64 offset, size_t len);
+int z_erofs_read_data_mapped(struct erofs_inode *inode,
+			struct erofs_map_blocks *map, char *raw, char *buffer,
+			erofs_off_t skip, erofs_off_t length, bool trimmed);
 
 static inline int erofs_get_occupied_size(const struct erofs_inode *inode,
 					  erofs_off_t *size)
diff --git a/lib/data.c b/lib/data.c
index 76a6677..d8c6076 100644
--- a/lib/data.c
+++ b/lib/data.c
@@ -158,19 +158,38 @@ int erofs_map_dev(struct erofs_sb_info *sbi, struct erofs_map_dev *map)
 	return 0;
 }
 
+int erofs_read_raw_data_mapped(struct erofs_map_blocks *map, char *buffer,
+				u64 offset, size_t len)
+{
+	struct erofs_map_dev mdev;
+	int ret;
+
+	mdev = (struct erofs_map_dev) {
+		.m_deviceid = map->m_deviceid,
+		.m_pa = map->m_pa,
+	};
+	ret = erofs_map_dev(&sbi, &mdev);
+	if (ret)
+		return ret;
+
+	ret = dev_read(mdev.m_deviceid, buffer, mdev.m_pa + offset, len);
+	if (ret < 0)
+		return -EIO;
+	return 0;
+}
+
 static int erofs_read_raw_data(struct erofs_inode *inode, char *buffer,
 			       erofs_off_t size, erofs_off_t offset)
 {
 	struct erofs_map_blocks map = {
 		.index = UINT_MAX,
 	};
-	struct erofs_map_dev mdev;
 	int ret;
 	erofs_off_t ptr = offset;
 
 	while (ptr < offset + size) {
 		char *const estart = buffer + ptr - offset;
-		erofs_off_t eend;
+		erofs_off_t eend, moff = 0;
 
 		map.m_la = ptr;
 		ret = erofs_map_blocks(inode, &map, 0);
@@ -179,14 +198,6 @@ static int erofs_read_raw_data(struct erofs_inode *inode, char *buffer,
 
 		DBG_BUGON(map.m_plen != map.m_llen);
 
-		mdev = (struct erofs_map_dev) {
-			.m_deviceid = map.m_deviceid,
-			.m_pa = map.m_pa,
-		};
-		ret = erofs_map_dev(&sbi, &mdev);
-		if (ret)
-			return ret;
-
 		/* trim extent */
 		eend = min(offset + size, map.m_la + map.m_llen);
 		DBG_BUGON(ptr < map.m_la);
@@ -204,19 +215,74 @@ static int erofs_read_raw_data(struct erofs_inode *inode, char *buffer,
 		}
 
 		if (ptr > map.m_la) {
-			mdev.m_pa += ptr - map.m_la;
+			moff = ptr - map.m_la;
 			map.m_la = ptr;
 		}
 
-		ret = dev_read(mdev.m_deviceid, estart, mdev.m_pa,
-			       eend - map.m_la);
-		if (ret < 0)
-			return -EIO;
+		ret = erofs_read_raw_data_mapped(&map, estart, moff,
+						 eend - map.m_la);
+		if (ret)
+			return ret;
 		ptr = eend;
 	}
 	return 0;
 }
 
+int z_erofs_read_data_mapped(struct erofs_inode *inode,
+			struct erofs_map_blocks *map, char *raw, char *buffer,
+			erofs_off_t skip, erofs_off_t length, bool trimmed)
+{
+	struct erofs_map_dev mdev;
+	int ret = 0;
+
+	if (map->m_flags & EROFS_MAP_FRAGMENT) {
+		struct erofs_inode packed_inode = {
+			.nid = sbi.packed_nid,
+		};
+
+		ret = erofs_read_inode_from_disk(&packed_inode);
+		if (ret) {
+			erofs_err("failed to read packed inode from disk");
+			return ret;
+		}
+
+		return erofs_pread(&packed_inode, buffer, length - skip,
+				inode->fragmentoff + skip);
+	}
+
+	/* no device id here, thus it will always succeed */
+	mdev = (struct erofs_map_dev) {
+		.m_pa = map->m_pa,
+	};
+	ret = erofs_map_dev(&sbi, &mdev);
+	if (ret) {
+		DBG_BUGON(1);
+		return ret;
+	}
+
+	ret = dev_read(mdev.m_deviceid, raw, mdev.m_pa, map->m_plen);
+	if (ret < 0)
+		return ret;
+
+	ret = z_erofs_decompress(&(struct z_erofs_decompress_req) {
+			.in = raw,
+			.out = buffer,
+			.decodedskip = skip,
+			.interlaced_offset =
+				map->m_algorithmformat == Z_EROFS_COMPRESSION_INTERLACED ?
+					erofs_blkoff(map->m_la) : 0,
+			.inputsize = map->m_plen,
+			.decodedlength = length,
+			.alg = map->m_algorithmformat,
+			.partial_decoding = trimmed ? true :
+				!(map->m_flags & EROFS_MAP_FULL_MAPPED) ||
+					(map->m_flags & EROFS_MAP_PARTIAL_REF),
+			 });
+	if (ret < 0)
+		return ret;
+	return 0;
+}
+
 static int z_erofs_read_data(struct erofs_inode *inode, char *buffer,
 			     erofs_off_t size, erofs_off_t offset)
 {
@@ -224,9 +290,8 @@ static int z_erofs_read_data(struct erofs_inode *inode, char *buffer,
 	struct erofs_map_blocks map = {
 		.index = UINT_MAX,
 	};
-	struct erofs_map_dev mdev;
-	bool partial;
-	unsigned int bufsize = 0, interlaced_offset;
+	bool trimmed;
+	unsigned int bufsize = 0;
 	char *raw = NULL;
 	int ret = 0;
 
@@ -238,28 +303,17 @@ static int z_erofs_read_data(struct erofs_inode *inode, char *buffer,
 		if (ret)
 			break;
 
-		/* no device id here, thus it will always succeed */
-		mdev = (struct erofs_map_dev) {
-			.m_pa = map.m_pa,
-		};
-		ret = erofs_map_dev(&sbi, &mdev);
-		if (ret) {
-			DBG_BUGON(1);
-			break;
-		}
-
 		/*
 		 * trim to the needed size if the returned extent is quite
 		 * larger than requested, and set up partial flag as well.
 		 */
 		if (end < map.m_la + map.m_llen) {
 			length = end - map.m_la;
-			partial = true;
+			trimmed = true;
 		} else {
 			DBG_BUGON(end != map.m_la + map.m_llen);
 			length = map.m_llen;
-			partial = !(map.m_flags & EROFS_MAP_FULL_MAPPED) ||
-				(map.m_flags & EROFS_MAP_PARTIAL_REF);
+			trimmed = false;
 		}
 
 		if (map.m_la < offset) {
@@ -276,25 +330,6 @@ static int z_erofs_read_data(struct erofs_inode *inode, char *buffer,
 			continue;
 		}
 
-		if (map.m_flags & EROFS_MAP_FRAGMENT) {
-			struct erofs_inode packed_inode = {
-				.nid = sbi.packed_nid,
-			};
-
-			ret = erofs_read_inode_from_disk(&packed_inode);
-			if (ret) {
-				erofs_err("failed to read packed inode from disk");
-				return ret;
-			}
-
-			ret = z_erofs_read_data(&packed_inode,
-					buffer + end - offset, length - skip,
-					inode->fragmentoff + skip);
-			if (ret < 0)
-				break;
-			continue;
-		}
-
 		if (map.m_plen > bufsize) {
 			bufsize = map.m_plen;
 			raw = realloc(raw, bufsize);
@@ -303,24 +338,9 @@ static int z_erofs_read_data(struct erofs_inode *inode, char *buffer,
 				break;
 			}
 		}
-		ret = dev_read(mdev.m_deviceid, raw, mdev.m_pa, map.m_plen);
-		if (ret < 0)
-			break;
 
-		interlaced_offset = 0;
-		if (map.m_algorithmformat == Z_EROFS_COMPRESSION_INTERLACED)
-			interlaced_offset = erofs_blkoff(map.m_la);
-
-		ret = z_erofs_decompress(&(struct z_erofs_decompress_req) {
-					.in = raw,
-					.out = buffer + end - offset,
-					.decodedskip = skip,
-					.interlaced_offset = interlaced_offset,
-					.inputsize = map.m_plen,
-					.decodedlength = length,
-					.alg = map.m_algorithmformat,
-					.partial_decoding = partial
-					 });
+		ret = z_erofs_read_data_mapped(inode, &map, raw,
+				buffer + end - offset, skip, length, trimmed);
 		if (ret < 0)
 			break;
 	}
-- 
2.17.1

[PATCH v2 2/3] erofs-utils: fsck: cleanup erofs_verify_inode_data()

[Yue Hu]

From: Yue Hu <huyue2@coolpad.com>

Diretly call {z_}erofs_read_{raw_}data_mapped() to avoid duplicated
code. Accordingly, fragment and partial-referenced plusters are also
supported after this change.

Signed-off-by: Yue Hu <huyue2@coolpad.com>
---
v2: parameter '0' -> false and update commit message

 fsck/main.c | 57 +++++++++++------------------------------------------
 1 file changed, 11 insertions(+), 46 deletions(-)

diff --git a/fsck/main.c b/fsck/main.c
index 2a9c501..4dfab29 100644
--- a/fsck/main.c
+++ b/fsck/main.c
@@ -366,7 +366,6 @@ static int erofs_verify_inode_data(struct erofs_inode *inode, int outfd)
 	struct erofs_map_blocks map = {
 		.index = UINT_MAX,
 	};
-	struct erofs_map_dev mdev;
 	int ret = 0;
 	bool compressed;
 	erofs_off_t pos = 0;
@@ -427,54 +426,20 @@ static int erofs_verify_inode_data(struct erofs_inode *inode, int outfd)
 			BUG_ON(!raw);
 		}
 
-		mdev = (struct erofs_map_dev) {
-			.m_deviceid = map.m_deviceid,
-			.m_pa = map.m_pa,
-		};
-		ret = erofs_map_dev(&sbi, &mdev);
-		if (ret) {
-			erofs_err("failed to map device of m_pa %" PRIu64 ", m_deviceid %u @ nid %llu: %d",
-				  map.m_pa, map.m_deviceid, inode->nid | 0ULL,
-				  ret);
-			goto out;
-		}
-
-		if (compressed && map.m_llen > buffer_size) {
-			buffer_size = map.m_llen;
-			buffer = realloc(buffer, buffer_size);
-			BUG_ON(!buffer);
-		}
-
-		ret = dev_read(mdev.m_deviceid, raw, mdev.m_pa, map.m_plen);
-		if (ret < 0) {
-			erofs_err("failed to read data of m_pa %" PRIu64 ", m_plen %" PRIu64 " @ nid %llu: %d",
-				  mdev.m_pa, map.m_plen, inode->nid | 0ULL,
-				  ret);
-			goto out;
-		}
-
 		if (compressed) {
-			struct z_erofs_decompress_req rq = {
-				.in = raw,
-				.out = buffer,
-				.decodedskip = 0,
-				.interlaced_offset =
-					map.m_algorithmformat == Z_EROFS_COMPRESSION_INTERLACED ?
-						erofs_blkoff(map.m_la) : 0,
-				.inputsize = map.m_plen,
-				.decodedlength = map.m_llen,
-				.alg = map.m_algorithmformat,
-				.partial_decoding = 0
-			};
-
-			ret = z_erofs_decompress(&rq);
-			if (ret < 0) {
-				erofs_err("failed to decompress data of m_pa %" PRIu64 ", m_plen %" PRIu64 " @ nid %llu: %s",
-					  mdev.m_pa, map.m_plen,
-					  inode->nid | 0ULL, strerror(-ret));
-				goto out;
+			if (map.m_llen > buffer_size) {
+				buffer_size = map.m_llen;
+				buffer = realloc(buffer, buffer_size);
+				BUG_ON(!buffer);
 			}
+			ret = z_erofs_read_data_mapped(inode, &map, raw,
+						buffer, 0, map.m_llen, false);
+		} else {
+			ret = erofs_read_raw_data_mapped(&map, raw, 0,
+							 map.m_plen);
 		}
+		if (ret)
+			goto out;
 
 		if (outfd >= 0 && write(outfd, compressed ? buffer : raw,
 					map.m_llen) < 0) {
-- 
2.17.1

Re: [PATCH v2 1/3] erofs-utils: lib: export parts of erofs_pread()

[Xiang Gao]

Hi Yue,

The patch itself generally looks good to me:

On 2023/1/10 16:49, Yue Hu wrote:
> From: Yue Hu <huyue2@coolpad.com>
> 
> Export parts of erofs_pread() to avoid duplicated code in
> erofs_verify_inode_data(). Let's make two helpers for this.
> 
> Signed-off-by: Yue Hu <huyue2@coolpad.com>
> ---
> v2: use parameter trimmed instead of partial
> 
>   include/erofs/internal.h |   5 ++
>   lib/data.c               | 154 ++++++++++++++++++++++-----------------
>   2 files changed, 92 insertions(+), 67 deletions(-)
> 
> diff --git a/include/erofs/internal.h b/include/erofs/internal.h
> index 206913c..47240f5 100644
> --- a/include/erofs/internal.h
> +++ b/include/erofs/internal.h
> @@ -355,6 +355,11 @@ int erofs_pread(struct erofs_inode *inode, char *buf,
>   int erofs_map_blocks(struct erofs_inode *inode,
>   		struct erofs_map_blocks *map, int flags);
>   int erofs_map_dev(struct erofs_sb_info *sbi, struct erofs_map_dev *map);
> +int erofs_read_raw_data_mapped(struct erofs_map_blocks *map, char *buffer,
> +			       u64 offset, size_t len);
> +int z_erofs_read_data_mapped(struct erofs_inode *inode,
> +			struct erofs_map_blocks *map, char *raw, char *buffer,
> +			erofs_off_t skip, erofs_off_t length, bool trimmed);
>   
>   static inline int erofs_get_occupied_size(const struct erofs_inode *inode,
>   					  erofs_off_t *size)
> diff --git a/lib/data.c b/lib/data.c
> index 76a6677..d8c6076 100644
> --- a/lib/data.c
> +++ b/lib/data.c
> @@ -158,19 +158,38 @@ int erofs_map_dev(struct erofs_sb_info *sbi, struct erofs_map_dev *map)
>   	return 0;
>   }
>   
> +int erofs_read_raw_data_mapped(struct erofs_map_blocks *map, char *buffer,

erofs_read_one_data?

> +				u64 offset, size_t len)
> +{
> +	struct erofs_map_dev mdev;
> +	int ret;
> +
> +	mdev = (struct erofs_map_dev) {
> +		.m_deviceid = map->m_deviceid,
> +		.m_pa = map->m_pa,
> +	};
> +	ret = erofs_map_dev(&sbi, &mdev);
> +	if (ret)
> +		return ret;
> +
> +	ret = dev_read(mdev.m_deviceid, buffer, mdev.m_pa + offset, len);
> +	if (ret < 0)
> +		return -EIO;
> +	return 0;
> +}
> +
>   static int erofs_read_raw_data(struct erofs_inode *inode, char *buffer,
>   			       erofs_off_t size, erofs_off_t offset)
>   {
>   	struct erofs_map_blocks map = {
>   		.index = UINT_MAX,
>   	};
> -	struct erofs_map_dev mdev;
>   	int ret;
>   	erofs_off_t ptr = offset;
>   
>   	while (ptr < offset + size) {
>   		char *const estart = buffer + ptr - offset;
> -		erofs_off_t eend;
> +		erofs_off_t eend, moff = 0;
>   
>   		map.m_la = ptr;
>   		ret = erofs_map_blocks(inode, &map, 0);
> @@ -179,14 +198,6 @@ static int erofs_read_raw_data(struct erofs_inode *inode, char *buffer,
>   
>   		DBG_BUGON(map.m_plen != map.m_llen);
>   
> -		mdev = (struct erofs_map_dev) {
> -			.m_deviceid = map.m_deviceid,
> -			.m_pa = map.m_pa,
> -		};
> -		ret = erofs_map_dev(&sbi, &mdev);
> -		if (ret)
> -			return ret;
> -
>   		/* trim extent */
>   		eend = min(offset + size, map.m_la + map.m_llen);
>   		DBG_BUGON(ptr < map.m_la);
> @@ -204,19 +215,74 @@ static int erofs_read_raw_data(struct erofs_inode *inode, char *buffer,
>   		}
>   
>   		if (ptr > map.m_la) {
> -			mdev.m_pa += ptr - map.m_la;
> +			moff = ptr - map.m_la;
>   			map.m_la = ptr;
>   		}
>   
> -		ret = dev_read(mdev.m_deviceid, estart, mdev.m_pa,
> -			       eend - map.m_la);
> -		if (ret < 0)
> -			return -EIO;
> +		ret = erofs_read_raw_data_mapped(&map, estart, moff,
> +						 eend - map.m_la);
> +		if (ret)
> +			return ret;
>   		ptr = eend;
>   	}
>   	return 0;
>   }
>   
> +int z_erofs_read_data_mapped(struct erofs_inode *inode,

z_erofs_read_one_data?

Thanks,
Gao Xiang

Re: [PATCH v2 1/3] erofs-utils: lib: export parts of erofs_pread()

[Yue Hu]

On Tue, 10 Jan 2023 17:00:33 +0800
Xiang Gao <hsiangkao@linux.alibaba.com> wrote:

> Hi Yue,
> 
> The patch itself generally looks good to me:
> 
> On 2023/1/10 16:49, Yue Hu wrote:
> > From: Yue Hu <huyue2@coolpad.com>
> > 
> > Export parts of erofs_pread() to avoid duplicated code in
> > erofs_verify_inode_data(). Let's make two helpers for this.
> > 
> > Signed-off-by: Yue Hu <huyue2@coolpad.com>
> > ---
> > v2: use parameter trimmed instead of partial
> > 
> >   include/erofs/internal.h |   5 ++
> >   lib/data.c               | 154 ++++++++++++++++++++++-----------------
> >   2 files changed, 92 insertions(+), 67 deletions(-)
> > 
> > diff --git a/include/erofs/internal.h b/include/erofs/internal.h
> > index 206913c..47240f5 100644
> > --- a/include/erofs/internal.h
> > +++ b/include/erofs/internal.h
> > @@ -355,6 +355,11 @@ int erofs_pread(struct erofs_inode *inode, char *buf,
> >   int erofs_map_blocks(struct erofs_inode *inode,
> >   		struct erofs_map_blocks *map, int flags);
> >   int erofs_map_dev(struct erofs_sb_info *sbi, struct erofs_map_dev *map);
> > +int erofs_read_raw_data_mapped(struct erofs_map_blocks *map, char *buffer,
> > +			       u64 offset, size_t len);
> > +int z_erofs_read_data_mapped(struct erofs_inode *inode,
> > +			struct erofs_map_blocks *map, char *raw, char *buffer,
> > +			erofs_off_t skip, erofs_off_t length, bool trimmed);
> >   
> >   static inline int erofs_get_occupied_size(const struct erofs_inode *inode,
> >   					  erofs_off_t *size)
> > diff --git a/lib/data.c b/lib/data.c
> > index 76a6677..d8c6076 100644
> > --- a/lib/data.c
> > +++ b/lib/data.c
> > @@ -158,19 +158,38 @@ int erofs_map_dev(struct erofs_sb_info *sbi, struct erofs_map_dev *map)
> >   	return 0;
> >   }
> >   
> > +int erofs_read_raw_data_mapped(struct erofs_map_blocks *map, char *buffer,  
> 
> erofs_read_one_data?
> 
> > +				u64 offset, size_t len)
> > +{
> > +	struct erofs_map_dev mdev;
> > +	int ret;
> > +
> > +	mdev = (struct erofs_map_dev) {
> > +		.m_deviceid = map->m_deviceid,
> > +		.m_pa = map->m_pa,
> > +	};
> > +	ret = erofs_map_dev(&sbi, &mdev);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = dev_read(mdev.m_deviceid, buffer, mdev.m_pa + offset, len);
> > +	if (ret < 0)
> > +		return -EIO;
> > +	return 0;
> > +}
> > +
> >   static int erofs_read_raw_data(struct erofs_inode *inode, char *buffer,
> >   			       erofs_off_t size, erofs_off_t offset)
> >   {
> >   	struct erofs_map_blocks map = {
> >   		.index = UINT_MAX,
> >   	};
> > -	struct erofs_map_dev mdev;
> >   	int ret;
> >   	erofs_off_t ptr = offset;
> >   
> >   	while (ptr < offset + size) {
> >   		char *const estart = buffer + ptr - offset;
> > -		erofs_off_t eend;
> > +		erofs_off_t eend, moff = 0;
> >   
> >   		map.m_la = ptr;
> >   		ret = erofs_map_blocks(inode, &map, 0);
> > @@ -179,14 +198,6 @@ static int erofs_read_raw_data(struct erofs_inode *inode, char *buffer,
> >   
> >   		DBG_BUGON(map.m_plen != map.m_llen);
> >   
> > -		mdev = (struct erofs_map_dev) {
> > -			.m_deviceid = map.m_deviceid,
> > -			.m_pa = map.m_pa,
> > -		};
> > -		ret = erofs_map_dev(&sbi, &mdev);
> > -		if (ret)
> > -			return ret;
> > -
> >   		/* trim extent */
> >   		eend = min(offset + size, map.m_la + map.m_llen);
> >   		DBG_BUGON(ptr < map.m_la);
> > @@ -204,19 +215,74 @@ static int erofs_read_raw_data(struct erofs_inode *inode, char *buffer,
> >   		}
> >   
> >   		if (ptr > map.m_la) {
> > -			mdev.m_pa += ptr - map.m_la;
> > +			moff = ptr - map.m_la;
> >   			map.m_la = ptr;
> >   		}
> >   
> > -		ret = dev_read(mdev.m_deviceid, estart, mdev.m_pa,
> > -			       eend - map.m_la);
> > -		if (ret < 0)
> > -			return -EIO;
> > +		ret = erofs_read_raw_data_mapped(&map, estart, moff,
> > +						 eend - map.m_la);
> > +		if (ret)
> > +			return ret;
> >   		ptr = eend;
> >   	}
> >   	return 0;
> >   }
> >   
> > +int z_erofs_read_data_mapped(struct erofs_inode *inode,  
> 
> z_erofs_read_one_data?

Okay. Let me update these in v3.

> 
> Thanks,
> Gao Xiang