[PATCH 0/8] staging: erofs: error handing and more tracepoints

[PATCH 0/8] staging: erofs: error handing and more tracepoints

[Gao Xiang]

In order to avoid conflicts with cleanup patches, Chao and I think
it is better to send reviewed preview patches in the erofs mailing list
to the community in time.

So here is reviewed & tested patches right now, which clean up and
enhance the error handing and add some tracepoints for decompression.

Note that in this patchset, bare use of 'unsigned' and NULL comparison are
also fixed compared with the preview patches according to the previous
discussion in the staging mailing list.

Thanks,
Gao Xiang

Chen Gong (2):
  staging: erofs: add trace points for reading zipped data
  staging: erofs: replace BUG_ON with DBG_BUGON in data.c

Gao Xiang (6):
  staging: erofs: fix a missing endian conversion
  staging: erofs: clean up z_erofs_map_blocks_iter
  staging: erofs: complete error handing of z_erofs_map_blocks_iter
  staging: erofs: fix a bug when appling cache strategy
  staging: erofs: complete error handing of z_erofs_do_read_page
  staging: erofs: avoid magic constants when initializing clusterbits

 drivers/staging/erofs/data.c                       |  31 ++--
 drivers/staging/erofs/include/trace/events/erofs.h |  20 ++-
 drivers/staging/erofs/super.c                      |   5 +-
 drivers/staging/erofs/unzip_vle.c                  | 195 +++++++++++++--------
 4 files changed, 162 insertions(+), 89 deletions(-)

-- 
1.9.1

[PATCH 1/8] staging: erofs: fix a missing endian conversion

[Gao Xiang]

This patch fixes a missing endian conversion in
vle_get_logical_extent_head.

Reviewed-by: Chao Yu <yuchao0@huawei.com>
Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>
---
 drivers/staging/erofs/unzip_vle.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/erofs/unzip_vle.c b/drivers/staging/erofs/unzip_vle.c
index 21874b7..d16e3dc 100644
--- a/drivers/staging/erofs/unzip_vle.c
+++ b/drivers/staging/erofs/unzip_vle.c
@@ -1488,6 +1488,7 @@ static erofs_off_t vle_get_logical_extent_head(
 	struct super_block *const sb = inode->i_sb;
 	const unsigned int clusterbits = EROFS_SB(sb)->clusterbits;
 	const unsigned int clustersize = 1 << clusterbits;
+	unsigned int delta0;
 
 	if (page->index != blkaddr) {
 		kunmap_atomic(*kaddr_iter);
@@ -1502,12 +1503,13 @@ static erofs_off_t vle_get_logical_extent_head(
 	di = *kaddr_iter + vle_extent_blkoff(inode, lcn);
 	switch (vle_cluster_type(di)) {
 	case Z_EROFS_VLE_CLUSTER_TYPE_NONHEAD:
-		BUG_ON(!di->di_u.delta[0]);
-		BUG_ON(lcn < di->di_u.delta[0]);
+		delta0 = le16_to_cpu(di->di_u.delta[0]);
+		DBG_BUGON(!delta0);
+		DBG_BUGON(lcn < delta0);
 
 		ofs = vle_get_logical_extent_head(inode,
 			page_iter, kaddr_iter,
-			lcn - di->di_u.delta[0], pcn, flags);
+			lcn - delta0, pcn, flags);
 		break;
 	case Z_EROFS_VLE_CLUSTER_TYPE_PLAIN:
 		*flags ^= EROFS_MAP_ZIPPED;
-- 
1.9.1

[PATCH 2/8] staging: erofs: clean up z_erofs_map_blocks_iter

[Gao Xiang]

This patch mainly introduces `vle_map_blocks_iter_ctx' to clean up
z_erofs_map_blocks_iter and vle_get_logical_extent_head.

It changes the return value of `vle_get_logical_extent_head' to int
for the later error handing. In addition, it also renames `pcn' to
`pblk' since only `pblk' exists in erofs compression ondisk format.

Reviewed-by: Chao Yu <yuchao0@huawei.com>
Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>
---
 drivers/staging/erofs/unzip_vle.c | 139 +++++++++++++++++++++-----------------
 1 file changed, 77 insertions(+), 62 deletions(-)

diff --git a/drivers/staging/erofs/unzip_vle.c b/drivers/staging/erofs/unzip_vle.c
index d16e3dc..d6e5956 100644
--- a/drivers/staging/erofs/unzip_vle.c
+++ b/drivers/staging/erofs/unzip_vle.c
@@ -1410,6 +1410,13 @@ static int z_erofs_vle_normalaccess_readpages(
 	.readpages = z_erofs_vle_normalaccess_readpages,
 };
 
+/*
+ * Variable-sized Logical Extent (Fixed Physical Cluster) Compression Mode
+ * ---
+ * VLE compression mode attempts to compress a number of logical data into
+ * a physical cluster with a fixed size.
+ * VLE compression mode uses "struct z_erofs_vle_decompressed_index".
+ */
 #define __vle_cluster_advise(x, bit, bits) \
 	((le16_to_cpu(x) >> (bit)) & ((1 << (bits)) - 1))
 
@@ -1465,90 +1472,95 @@ static int z_erofs_vle_normalaccess_readpages(
 	return erofs_blkoff(iloc(sbi, vi->nid) + ofs);
 }
 
-/*
- * Variable-sized Logical Extent (Fixed Physical Cluster) Compression Mode
- * ---
- * VLE compression mode attempts to compress a number of logical data into
- * a physical cluster with a fixed size.
- * VLE compression mode uses "struct z_erofs_vle_decompressed_index".
- */
-static erofs_off_t vle_get_logical_extent_head(
-	struct inode *inode,
-	struct page **page_iter,
-	void **kaddr_iter,
-	unsigned int lcn,	/* logical cluster number */
-	erofs_blk_t *pcn,
-	unsigned int *flags)
+struct vle_map_blocks_iter_ctx {
+	struct inode *inode;
+	struct super_block *sb;
+	unsigned int clusterbits;
+
+	struct page **mpage_ret;
+	void **kaddr_ret;
+};
+
+static int
+vle_get_logical_extent_head(const struct vle_map_blocks_iter_ctx *ctx,
+			    unsigned int lcn,	/* logical cluster number */
+			    unsigned long long *ofs,
+			    erofs_blk_t *pblk,
+			    unsigned int *flags)
 {
-	/* for extent meta */
-	struct page *page = *page_iter;
-	erofs_blk_t blkaddr = vle_extent_blkaddr(inode, lcn);
+	const unsigned int clustersize = 1 << ctx->clusterbits;
+	const erofs_blk_t mblk = vle_extent_blkaddr(ctx->inode, lcn);
+	struct page *mpage = *ctx->mpage_ret;	/* extent metapage */
+
 	struct z_erofs_vle_decompressed_index *di;
-	unsigned long long ofs;
-	struct super_block *const sb = inode->i_sb;
-	const unsigned int clusterbits = EROFS_SB(sb)->clusterbits;
-	const unsigned int clustersize = 1 << clusterbits;
-	unsigned int delta0;
-
-	if (page->index != blkaddr) {
-		kunmap_atomic(*kaddr_iter);
-		unlock_page(page);
-		put_page(page);
+	unsigned int cluster_type, delta0;
 
-		page = erofs_get_meta_page_nofail(sb, blkaddr, false);
-		*page_iter = page;
-		*kaddr_iter = kmap_atomic(page);
+	if (mpage->index != mblk) {
+		kunmap_atomic(*ctx->kaddr_ret);
+		unlock_page(mpage);
+		put_page(mpage);
+
+		mpage = erofs_get_meta_page_nofail(ctx->sb, mblk, false);
+		*ctx->mpage_ret = mpage;
+		*ctx->kaddr_ret = kmap_atomic(mpage);
 	}
 
-	di = *kaddr_iter + vle_extent_blkoff(inode, lcn);
-	switch (vle_cluster_type(di)) {
+	di = *ctx->kaddr_ret + vle_extent_blkoff(ctx->inode, lcn);
+
+	cluster_type = vle_cluster_type(di);
+	switch (cluster_type) {
 	case Z_EROFS_VLE_CLUSTER_TYPE_NONHEAD:
 		delta0 = le16_to_cpu(di->di_u.delta[0]);
 		DBG_BUGON(!delta0);
 		DBG_BUGON(lcn < delta0);
 
-		ofs = vle_get_logical_extent_head(inode,
-			page_iter, kaddr_iter,
-			lcn - delta0, pcn, flags);
-		break;
+		return vle_get_logical_extent_head(ctx,
+			lcn - delta0, ofs, pblk, flags);
 	case Z_EROFS_VLE_CLUSTER_TYPE_PLAIN:
 		*flags ^= EROFS_MAP_ZIPPED;
+		/* fallthrough */
 	case Z_EROFS_VLE_CLUSTER_TYPE_HEAD:
 		/* clustersize should be a power of two */
-		ofs = ((u64)lcn << clusterbits) +
+		*ofs = ((u64)lcn << ctx->clusterbits) +
 			(le16_to_cpu(di->di_clusterofs) & (clustersize - 1));
-		*pcn = le32_to_cpu(di->di_u.blkaddr);
+		*pblk = le32_to_cpu(di->di_u.blkaddr);
 		break;
 	default:
 		BUG_ON(1);
 	}
-	return ofs;
+	return 0;
 }
 
 int z_erofs_map_blocks_iter(struct inode *inode,
 	struct erofs_map_blocks *map,
 	struct page **mpage_ret, int flags)
 {
+	void *kaddr;
+	const struct vle_map_blocks_iter_ctx ctx = {
+		.inode = inode,
+		.sb = inode->i_sb,
+		.clusterbits = EROFS_I_SB(inode)->clusterbits,
+		.mpage_ret = mpage_ret,
+		.kaddr_ret = &kaddr
+	};
+	const unsigned int clustersize = 1 << ctx.clusterbits;
+	/* if both m_(l,p)len are 0, regularize l_lblk, l_lofs, etc... */
+	const bool initial = !map->m_llen;
+
 	/* logicial extent (start, end) offset */
 	unsigned long long ofs, end;
-	struct z_erofs_vle_decompressed_index *di;
-	erofs_blk_t e_blkaddr, pcn;
-	unsigned int lcn, logical_cluster_ofs, cluster_type;
+	unsigned int lcn;
 	u32 ofs_rem;
+
+	erofs_blk_t mblk, pblk;
 	struct page *mpage = *mpage_ret;
-	void *kaddr;
-	bool initial;
-	struct super_block *const sb = inode->i_sb;
-	const unsigned int clusterbits = EROFS_SB(sb)->clusterbits;
-	const unsigned int clustersize = 1 << clusterbits;
+	struct z_erofs_vle_decompressed_index *di;
+	unsigned int cluster_type, logical_cluster_ofs;
 	int err = 0;
 
-	/* if both m_(l,p)len are 0, regularize l_lblk, l_lofs, etc... */
-	initial = !map->m_llen;
-
 	/* when trying to read beyond EOF, leave it unmapped */
 	if (unlikely(map->m_la >= inode->i_size)) {
-		BUG_ON(!initial);
+		DBG_BUGON(!initial);
 		map->m_llen = map->m_la + 1 - inode->i_size;
 		map->m_la = inode->i_size;
 		map->m_flags = 0;
@@ -1561,16 +1573,16 @@ int z_erofs_map_blocks_iter(struct inode *inode,
 	ofs = map->m_la + map->m_llen;
 
 	/* clustersize should be power of two */
-	lcn = ofs >> clusterbits;
+	lcn = ofs >> ctx.clusterbits;
 	ofs_rem = ofs & (clustersize - 1);
 
-	e_blkaddr = vle_extent_blkaddr(inode, lcn);
+	mblk = vle_extent_blkaddr(inode, lcn);
 
-	if (mpage == NULL || mpage->index != e_blkaddr) {
+	if (!mpage || mpage->index != mblk) {
 		if (mpage != NULL)
 			put_page(mpage);
 
-		mpage = erofs_get_meta_page_nofail(sb, e_blkaddr, false);
+		mpage = erofs_get_meta_page_nofail(ctx.sb, mblk, false);
 		*mpage_ret = mpage;
 	} else {
 		lock_page(mpage);
@@ -1580,8 +1592,8 @@ int z_erofs_map_blocks_iter(struct inode *inode,
 	kaddr = kmap_atomic(mpage);
 	di = kaddr + vle_extent_blkoff(inode, lcn);
 
-	debugln("%s, lcn %u e_blkaddr %u e_blkoff %u", __func__, lcn,
-		e_blkaddr, vle_extent_blkoff(inode, lcn));
+	debugln("%s, lcn %u mblk %u e_blkoff %u", __func__, lcn,
+		mblk, vle_extent_blkoff(inode, lcn));
 
 	err = vle_decompressed_index_clusterofs(&logical_cluster_ofs,
 						clustersize, di);
@@ -1608,13 +1620,13 @@ int z_erofs_map_blocks_iter(struct inode *inode,
 		/* fallthrough */
 	case Z_EROFS_VLE_CLUSTER_TYPE_HEAD:
 		if (ofs_rem == logical_cluster_ofs) {
-			pcn = le32_to_cpu(di->di_u.blkaddr);
+			pblk = le32_to_cpu(di->di_u.blkaddr);
 			goto exact_hitted;
 		}
 
 		if (ofs_rem > logical_cluster_ofs) {
 			ofs = (u64)lcn * clustersize | logical_cluster_ofs;
-			pcn = le32_to_cpu(di->di_u.blkaddr);
+			pblk = le32_to_cpu(di->di_u.blkaddr);
 			break;
 		}
 
@@ -1629,9 +1641,12 @@ int z_erofs_map_blocks_iter(struct inode *inode,
 		/* fallthrough */
 	case Z_EROFS_VLE_CLUSTER_TYPE_NONHEAD:
 		/* get the correspoinding first chunk */
-		ofs = vle_get_logical_extent_head(inode, mpage_ret,
-			&kaddr, lcn, &pcn, &map->m_flags);
+		err = vle_get_logical_extent_head(&ctx, lcn, &ofs,
+						  &pblk, &map->m_flags);
 		mpage = *mpage_ret;
+
+		if (unlikely(err))
+			goto unmap_out;
 		break;
 	default:
 		errln("unknown cluster type %u at offset %llu of nid %llu",
@@ -1644,7 +1659,7 @@ int z_erofs_map_blocks_iter(struct inode *inode,
 exact_hitted:
 	map->m_llen = end - ofs;
 	map->m_plen = clustersize;
-	map->m_pa = blknr_to_addr(pcn);
+	map->m_pa = blknr_to_addr(pblk);
 	map->m_flags |= EROFS_MAP_MAPPED;
 unmap_out:
 	kunmap_atomic(kaddr);
-- 
1.9.1

[PATCH 3/8] staging: erofs: complete error handing of z_erofs_map_blocks_iter

[Gao Xiang]

This patch completes error handing of z_erofs_map_blocks_iter
and vle_get_logical_extent_head, including no memory and
io error cases.

Reviewed-by: Chao Yu <yuchao0@huawei.com>
Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>
---
 drivers/staging/erofs/unzip_vle.c | 35 ++++++++++++++++++++++++++---------
 1 file changed, 26 insertions(+), 9 deletions(-)

diff --git a/drivers/staging/erofs/unzip_vle.c b/drivers/staging/erofs/unzip_vle.c
index d6e5956..d4998fe 100644
--- a/drivers/staging/erofs/unzip_vle.c
+++ b/drivers/staging/erofs/unzip_vle.c
@@ -1500,7 +1500,11 @@ struct vle_map_blocks_iter_ctx {
 		unlock_page(mpage);
 		put_page(mpage);
 
-		mpage = erofs_get_meta_page_nofail(ctx->sb, mblk, false);
+		mpage = erofs_get_meta_page(ctx->sb, mblk, false);
+		if (IS_ERR(mpage)) {
+			*ctx->mpage_ret = NULL;
+			return PTR_ERR(mpage);
+		}
 		*ctx->mpage_ret = mpage;
 		*ctx->kaddr_ret = kmap_atomic(mpage);
 	}
@@ -1511,9 +1515,12 @@ struct vle_map_blocks_iter_ctx {
 	switch (cluster_type) {
 	case Z_EROFS_VLE_CLUSTER_TYPE_NONHEAD:
 		delta0 = le16_to_cpu(di->di_u.delta[0]);
-		DBG_BUGON(!delta0);
-		DBG_BUGON(lcn < delta0);
-
+		if (unlikely(!delta0 || delta0 > lcn)) {
+			errln("invalid NONHEAD dl0 %u at lcn %u of nid %llu",
+			      delta0, lcn, EROFS_V(ctx->inode)->nid);
+			DBG_BUGON(1);
+			return -EIO;
+		}
 		return vle_get_logical_extent_head(ctx,
 			lcn - delta0, ofs, pblk, flags);
 	case Z_EROFS_VLE_CLUSTER_TYPE_PLAIN:
@@ -1526,7 +1533,10 @@ struct vle_map_blocks_iter_ctx {
 		*pblk = le32_to_cpu(di->di_u.blkaddr);
 		break;
 	default:
-		BUG_ON(1);
+		errln("unknown cluster type %u at lcn %u of nid %llu",
+		      cluster_type, lcn, EROFS_V(ctx->inode)->nid);
+		DBG_BUGON(1);
+		return -EIO;
 	}
 	return 0;
 }
@@ -1582,7 +1592,11 @@ int z_erofs_map_blocks_iter(struct inode *inode,
 		if (mpage != NULL)
 			put_page(mpage);
 
-		mpage = erofs_get_meta_page_nofail(ctx.sb, mblk, false);
+		mpage = erofs_get_meta_page(ctx.sb, mblk, false);
+		if (IS_ERR(mpage)) {
+			err = PTR_ERR(mpage);
+			goto out;
+		}
 		*mpage_ret = mpage;
 	} else {
 		lock_page(mpage);
@@ -1645,8 +1659,11 @@ int z_erofs_map_blocks_iter(struct inode *inode,
 						  &pblk, &map->m_flags);
 		mpage = *mpage_ret;
 
-		if (unlikely(err))
-			goto unmap_out;
+		if (unlikely(err)) {
+			if (mpage)
+				goto unmap_out;
+			goto out;
+		}
 		break;
 	default:
 		errln("unknown cluster type %u at offset %llu of nid %llu",
@@ -1670,7 +1687,7 @@ int z_erofs_map_blocks_iter(struct inode *inode,
 		map->m_llen, map->m_plen, map->m_flags);
 
 	/* aggressively BUG_ON iff CONFIG_EROFS_FS_DEBUG is on */
-	DBG_BUGON(err < 0);
+	DBG_BUGON(err < 0 && err != -ENOMEM);
 	return err;
 }
 
-- 
1.9.1

[PATCH 4/8] staging: erofs: fix a bug when appling cache strategy

[Gao Xiang]

As described in Kconfig, the last compressed pack should be cached
for further reading for either `EROFS_FS_ZIP_CACHE_UNIPOLAR' or
`EROFS_FS_ZIP_CACHE_BIPOLAR' by design.

However, there is a bug in z_erofs_do_read_page, it will
switch `initial' to `false' at the very beginning before it decides
to cache the last compressed pack.

caching strategy should work properly after appling this patch.

Reviewed-by: Chao Yu <yuchao0@huawei.com>
Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>
---
 drivers/staging/erofs/unzip_vle.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/erofs/unzip_vle.c b/drivers/staging/erofs/unzip_vle.c
index d4998fe..0f2e180 100644
--- a/drivers/staging/erofs/unzip_vle.c
+++ b/drivers/staging/erofs/unzip_vle.c
@@ -629,7 +629,7 @@ static int z_erofs_do_read_page(struct z_erofs_vle_frontend *fe,
 	/* go ahead the next map_blocks */
 	debugln("%s: [out-of-range] pos %llu", __func__, offset + cur);
 
-	if (!z_erofs_vle_work_iter_end(builder))
+	if (z_erofs_vle_work_iter_end(builder))
 		fe->initial = false;
 
 	map->m_la = offset + cur;
-- 
1.9.1

[PATCH 5/8] staging: erofs: complete error handing of z_erofs_do_read_page

[Gao Xiang]

This patch completes error handing code of z_erofs_do_read_page.
PG_error will be set when some read error happens, therefore
z_erofs_onlinepage_endio will unlock this page without setting
PG_uptodate.

Reviewed-by: Chao Yu <yucxhao0@huawei.com>
Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>
---
 drivers/staging/erofs/unzip_vle.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/drivers/staging/erofs/unzip_vle.c b/drivers/staging/erofs/unzip_vle.c
index 0f2e180..15c2015 100644
--- a/drivers/staging/erofs/unzip_vle.c
+++ b/drivers/staging/erofs/unzip_vle.c
@@ -611,7 +611,7 @@ static int z_erofs_do_read_page(struct z_erofs_vle_frontend *fe,
 
 	enum z_erofs_page_type page_type;
 	unsigned int cur, end, spiltted, index;
-	int err;
+	int err = 0;
 
 	/* register locked file pages as online pages in pack */
 	z_erofs_onlinepage_init(page);
@@ -638,12 +638,11 @@ static int z_erofs_do_read_page(struct z_erofs_vle_frontend *fe,
 	if (unlikely(err))
 		goto err_out;
 
-	/* deal with hole (FIXME! broken now) */
 	if (unlikely(!(map->m_flags & EROFS_MAP_MAPPED)))
 		goto hitted;
 
 	DBG_BUGON(map->m_plen != 1 << sbi->clusterbits);
-	BUG_ON(erofs_blkoff(map->m_pa));
+	DBG_BUGON(erofs_blkoff(map->m_pa));
 
 	err = z_erofs_vle_work_iter_begin(builder, sb, map, &fe->owned_head);
 	if (unlikely(err))
@@ -688,7 +687,7 @@ static int z_erofs_do_read_page(struct z_erofs_vle_frontend *fe,
 
 		err = z_erofs_vle_work_add_page(builder,
 			newpage, Z_EROFS_PAGE_TYPE_EXCLUSIVE);
-		if (!err)
+		if (likely(!err))
 			goto retry;
 	}
 
@@ -699,9 +698,10 @@ static int z_erofs_do_read_page(struct z_erofs_vle_frontend *fe,
 
 	/* FIXME! avoid the last relundant fixup & endio */
 	z_erofs_onlinepage_fixup(page, index, true);
-	++spiltted;
 
-	/* also update nr_pages and increase queued_pages */
+	/* bump up the number of spiltted parts of a page */
+	++spiltted;
+	/* also update nr_pages */
 	work->nr_pages = max_t(pgoff_t, work->nr_pages, index + 1);
 next_part:
 	/* can be used for verification */
@@ -711,16 +711,18 @@ static int z_erofs_do_read_page(struct z_erofs_vle_frontend *fe,
 	if (end > 0)
 		goto repeat;
 
+out:
 	/* FIXME! avoid the last relundant fixup & endio */
 	z_erofs_onlinepage_endio(page);
 
 	debugln("%s, finish page: %pK spiltted: %u map->m_llen %llu",
 		__func__, page, spiltted, map->m_llen);
-	return 0;
+	return err;
 
+	/* if some error occurred while processing this page */
 err_out:
-	/* TODO: the missing error handing cases */
-	return err;
+	SetPageError(page);
+	goto out;
 }
 
 static void z_erofs_vle_unzip_kickoff(void *ptr, int bios)
-- 
1.9.1

[PATCH 6/8] staging: erofs: avoid magic constants when initializing clusterbits

[Gao Xiang]

Currently erofs only supports clustersize == blocksize.
and clustersize == 2^n * blocksize will be supported in the future.

Reviewed-by: Chao Yu <yuchao0@huawei.com>
Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>
---
 drivers/staging/erofs/super.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/erofs/super.c b/drivers/staging/erofs/super.c
index 2109b03..9e42153 100644
--- a/drivers/staging/erofs/super.c
+++ b/drivers/staging/erofs/super.c
@@ -116,9 +116,10 @@ static int superblock_read(struct super_block *sb)
 #endif
 	sbi->islotbits = ffs(sizeof(struct erofs_inode_v1)) - 1;
 #ifdef CONFIG_EROFS_FS_ZIP
-	sbi->clusterbits = 12;
+	/* TODO: clusterbits should be related to inode */
+	sbi->clusterbits = blkszbits;
 
-	if (1 << (sbi->clusterbits - 12) > Z_EROFS_CLUSTER_MAX_PAGES)
+	if (1 << (sbi->clusterbits - PAGE_SHIFT) > Z_EROFS_CLUSTER_MAX_PAGES)
 		errln("clusterbits %u is not supported on this kernel",
 			sbi->clusterbits);
 #endif
-- 
1.9.1

[PATCH 7/8] staging: erofs: add trace points for reading zipped data

[Gao Xiang]

From: Chen Gong <gongchen4@huawei.com>

This patch adds trace points for reading zipped data.

Signed-off-by: Chen Gong <gongchen4@huawei.com>
Reviewed-by: Chao Yu <yuchao0@huawei.com>
Reviewed-by: Gao Xiang <gaoxiang25@huawei.com>
Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>
---
 drivers/staging/erofs/include/trace/events/erofs.h | 20 ++++++++++++++++++--
 drivers/staging/erofs/unzip_vle.c                  | 11 +++++++++++
 2 files changed, 29 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/erofs/include/trace/events/erofs.h b/drivers/staging/erofs/include/trace/events/erofs.h
index 5aead93..660c92f 100644
--- a/drivers/staging/erofs/include/trace/events/erofs.h
+++ b/drivers/staging/erofs/include/trace/events/erofs.h
@@ -162,7 +162,8 @@
 
 	TP_printk("dev = (%d,%d), nid = %llu, la %llu llen %llu flags %s",
 		  show_dev_nid(__entry),
-		  __entry->la, __entry->llen, show_map_flags(__entry->flags))
+		  __entry->la, __entry->llen,
+		  __entry->flags ? show_map_flags(__entry->flags) : "NULL")
 );
 
 DEFINE_EVENT(erofs__map_blocks_enter, erofs_map_blocks_flatmode_enter,
@@ -172,6 +173,13 @@
 	TP_ARGS(inode, map, flags)
 );
 
+DEFINE_EVENT(erofs__map_blocks_enter, z_erofs_map_blocks_iter_enter,
+	TP_PROTO(struct inode *inode, struct erofs_map_blocks *map,
+		 unsigned int flags),
+
+	TP_ARGS(inode, map, flags)
+);
+
 DECLARE_EVENT_CLASS(erofs__map_blocks_exit,
 	TP_PROTO(struct inode *inode, struct erofs_map_blocks *map,
 		 unsigned int flags, int ret),
@@ -204,7 +212,8 @@
 
 	TP_printk("dev = (%d,%d), nid = %llu, flags %s "
 		  "la %llu pa %llu llen %llu plen %llu mflags %s ret %d",
-		  show_dev_nid(__entry), show_map_flags(__entry->flags),
+		  show_dev_nid(__entry),
+		  __entry->flags ? show_map_flags(__entry->flags) : "NULL",
 		  __entry->la, __entry->pa, __entry->llen, __entry->plen,
 		  show_mflags(__entry->mflags), __entry->ret)
 );
@@ -216,6 +225,13 @@
 	TP_ARGS(inode, map, flags, ret)
 );
 
+DEFINE_EVENT(erofs__map_blocks_exit, z_erofs_map_blocks_iter_exit,
+	TP_PROTO(struct inode *inode, struct erofs_map_blocks *map,
+		 unsigned int flags, int ret),
+
+	TP_ARGS(inode, map, flags, ret)
+);
+
 TRACE_EVENT(erofs_destroy_inode,
 	TP_PROTO(struct inode *inode),
 
diff --git a/drivers/staging/erofs/unzip_vle.c b/drivers/staging/erofs/unzip_vle.c
index 15c2015..ad3b7bb 100644
--- a/drivers/staging/erofs/unzip_vle.c
+++ b/drivers/staging/erofs/unzip_vle.c
@@ -13,6 +13,8 @@
 #include "unzip_vle.h"
 #include <linux/prefetch.h>
 
+#include <trace/events/erofs.h>
+
 static struct workqueue_struct *z_erofs_workqueue __read_mostly;
 static struct kmem_cache *z_erofs_workgroup_cachep __read_mostly;
 
@@ -613,6 +615,8 @@ static int z_erofs_do_read_page(struct z_erofs_vle_frontend *fe,
 	unsigned int cur, end, spiltted, index;
 	int err = 0;
 
+	trace_erofs_readpage(page, false);
+
 	/* register locked file pages as online pages in pack */
 	z_erofs_onlinepage_init(page);
 
@@ -1348,6 +1352,9 @@ static inline int __z_erofs_vle_normalaccess_readpages(
 	struct page *head = NULL;
 	LIST_HEAD(pagepool);
 
+	trace_erofs_readpages(mapping->host, lru_to_page(pages),
+			      nr_pages, false);
+
 #if (EROFS_FS_ZIP_CACHE_LVL >= 2)
 	f.cachedzone_la = (erofs_off_t)lru_to_page(pages)->index << PAGE_SHIFT;
 #endif
@@ -1570,6 +1577,8 @@ int z_erofs_map_blocks_iter(struct inode *inode,
 	unsigned int cluster_type, logical_cluster_ofs;
 	int err = 0;
 
+	trace_z_erofs_map_blocks_iter_enter(inode, map, flags);
+
 	/* when trying to read beyond EOF, leave it unmapped */
 	if (unlikely(map->m_la >= inode->i_size)) {
 		DBG_BUGON(!initial);
@@ -1688,6 +1697,8 @@ int z_erofs_map_blocks_iter(struct inode *inode,
 		__func__, map->m_la, map->m_pa,
 		map->m_llen, map->m_plen, map->m_flags);
 
+	trace_z_erofs_map_blocks_iter_exit(inode, map, flags, err);
+
 	/* aggressively BUG_ON iff CONFIG_EROFS_FS_DEBUG is on */
 	DBG_BUGON(err < 0 && err != -ENOMEM);
 	return err;
-- 
1.9.1

[PATCH 8/8] staging: erofs: replace BUG_ON with DBG_BUGON in data.c

[Gao Xiang]

From: Chen Gong <gongchen4@huawei.com>

This patch replace BUG_ON with DBG_BUGON in data.c, and add necessary
error handler.

Signed-off-by: Chen Gong <gongchen4@huawei.com>
Reviewed-by: Gao Xiang <gaoxiang25@huawei.com>
Reviewed-by: Chao Yu <yuchao0@huawei.com>
Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>
---
 drivers/staging/erofs/data.c | 31 ++++++++++++++++++++-----------
 1 file changed, 20 insertions(+), 11 deletions(-)

diff --git a/drivers/staging/erofs/data.c b/drivers/staging/erofs/data.c
index e191610..6384f73 100644
--- a/drivers/staging/erofs/data.c
+++ b/drivers/staging/erofs/data.c
@@ -25,7 +25,7 @@ static inline void read_endio(struct bio *bio)
 		struct page *page = bvec->bv_page;
 
 		/* page is already locked */
-		BUG_ON(PageUptodate(page));
+		DBG_BUGON(PageUptodate(page));
 
 		if (unlikely(err))
 			SetPageError(page);
@@ -110,12 +110,12 @@ static int erofs_map_blocks_flatmode(struct inode *inode,
 	struct erofs_map_blocks *map,
 	int flags)
 {
+	int err = 0;
 	erofs_blk_t nblocks, lastblk;
 	u64 offset = map->m_la;
 	struct erofs_vnode *vi = EROFS_V(inode);
 
 	trace_erofs_map_blocks_flatmode_enter(inode, map, flags);
-	BUG_ON(is_inode_layout_compression(inode));
 
 	nblocks = DIV_ROUND_UP(inode->i_size, PAGE_SIZE);
 	lastblk = nblocks - is_inode_layout_inline(inode);
@@ -142,18 +142,27 @@ static int erofs_map_blocks_flatmode(struct inode *inode,
 		map->m_plen = inode->i_size - offset;
 
 		/* inline data should locate in one meta block */
-		BUG_ON(erofs_blkoff(map->m_pa) + map->m_plen > PAGE_SIZE);
+		if (erofs_blkoff(map->m_pa) + map->m_plen > PAGE_SIZE) {
+			DBG_BUGON(1);
+			err = -EIO;
+			goto err_out;
+		}
+
 		map->m_flags |= EROFS_MAP_META;
 	} else {
 		errln("internal error @ nid: %llu (size %llu), m_la 0x%llx",
 			vi->nid, inode->i_size, map->m_la);
-		BUG();
+		DBG_BUGON(1);
+		err = -EIO;
+		goto err_out;
 	}
 
 out:
 	map->m_llen = map->m_plen;
+
+err_out:
 	trace_erofs_map_blocks_flatmode_exit(inode, map, flags, 0);
-	return 0;
+	return err;
 }
 
 #ifdef CONFIG_EROFS_FS_ZIP
@@ -209,7 +218,7 @@ static inline struct bio *erofs_read_raw_page(
 	erofs_off_t current_block = (erofs_off_t)page->index;
 	int err;
 
-	BUG_ON(!nblocks);
+	DBG_BUGON(!nblocks);
 
 	if (PageUptodate(page)) {
 		err = 0;
@@ -252,7 +261,7 @@ static inline struct bio *erofs_read_raw_page(
 		}
 
 		/* for RAW access mode, m_plen must be equal to m_llen */
-		BUG_ON(map.m_plen != map.m_llen);
+		DBG_BUGON(map.m_plen != map.m_llen);
 
 		blknr = erofs_blknr(map.m_pa);
 		blkoff = erofs_blkoff(map.m_pa);
@@ -262,7 +271,7 @@ static inline struct bio *erofs_read_raw_page(
 			void *vsrc, *vto;
 			struct page *ipage;
 
-			BUG_ON(map.m_plen > PAGE_SIZE);
+			DBG_BUGON(map.m_plen > PAGE_SIZE);
 
 			ipage = erofs_get_meta_page(inode->i_sb, blknr, 0);
 
@@ -289,7 +298,7 @@ static inline struct bio *erofs_read_raw_page(
 		}
 
 		/* pa must be block-aligned for raw reading */
-		BUG_ON(erofs_blkoff(map.m_pa) != 0);
+		DBG_BUGON(erofs_blkoff(map.m_pa));
 
 		/* max # of continuous pages */
 		if (nblocks > DIV_ROUND_UP(map.m_plen, PAGE_SIZE))
@@ -357,7 +366,7 @@ static int erofs_raw_access_readpage(struct file *file, struct page *page)
 	if (IS_ERR(bio))
 		return PTR_ERR(bio);
 
-	BUG_ON(bio != NULL);	/* since we have only one bio -- must be NULL */
+	DBG_BUGON(bio);	/* since we have only one bio -- must be NULL */
 	return 0;
 }
 
@@ -395,7 +404,7 @@ static int erofs_raw_access_readpages(struct file *filp,
 		/* pages could still be locked */
 		put_page(page);
 	}
-	BUG_ON(!list_empty(pages));
+	DBG_BUGON(!list_empty(pages));
 
 	/* the rare case (end in gaps) */
 	if (unlikely(bio != NULL))
-- 
1.9.1

Re: [PATCH 0/8] staging: erofs: error handing and more tracepoints

[Greg Kroah-Hartman]

On Fri, Sep 14, 2018 at 10:40:22PM +0800, Gao Xiang wrote:
> In order to avoid conflicts with cleanup patches, Chao and I think
> it is better to send reviewed preview patches in the erofs mailing list
> to the community in time.
> 
> So here is reviewed & tested patches right now, which clean up and
> enhance the error handing and add some tracepoints for decompression.
> 
> Note that in this patchset, bare use of 'unsigned' and NULL comparison are
> also fixed compared with the preview patches according to the previous
> discussion in the staging mailing list.

I applied this, but I need to go delete it as this patch series adds a
build warning to the system:

In file included from drivers/staging/erofs/unzip_vle.h:16:0,
                 from drivers/staging/erofs/unzip_vle.c:13:
drivers/staging/erofs/unzip_vle.c: In function ‘z_erofs_map_blocks_iter’:
drivers/staging/erofs/internal.h:303:34: warning: ‘pblk’ may be used uninitialized in this function [-Wmaybe-uninitialized]
 #define blknr_to_addr(nr)       ((erofs_off_t)(nr) * EROFS_BLKSIZ)
                                  ^
drivers/staging/erofs/unzip_vle.c:1574:20: note: ‘pblk’ was declared here
  erofs_blk_t mblk, pblk;
                    ^~~~

Please fix that up and resend.

thanks,

greg k-h

Re: [PATCH 0/8] staging: erofs: error handing and more tracepoints

[Gao Xiang]

Hi Greg,

On 2018/9/18 19:19, Greg Kroah-Hartman wrote:
> On Fri, Sep 14, 2018 at 10:40:22PM +0800, Gao Xiang wrote:
>> In order to avoid conflicts with cleanup patches, Chao and I think
>> it is better to send reviewed preview patches in the erofs mailing list
>> to the community in time.
>>
>> So here is reviewed & tested patches right now, which clean up and
>> enhance the error handing and add some tracepoints for decompression.
>>
>> Note that in this patchset, bare use of 'unsigned' and NULL comparison are
>> also fixed compared with the preview patches according to the previous
>> discussion in the staging mailing list.
> 
> I applied this, but I need to go delete it as this patch series adds a
> build warning to the system:
> 
> In file included from drivers/staging/erofs/unzip_vle.h:16:0,
>                  from drivers/staging/erofs/unzip_vle.c:13:
> drivers/staging/erofs/unzip_vle.c: In function ‘z_erofs_map_blocks_iter’:
> drivers/staging/erofs/internal.h:303:34: warning: ‘pblk’ may be used uninitialized in this function [-Wmaybe-uninitialized]
>  #define blknr_to_addr(nr)       ((erofs_off_t)(nr) * EROFS_BLKSIZ)
>                                   ^
> drivers/staging/erofs/unzip_vle.c:1574:20: note: ‘pblk’ was declared here
>   erofs_blk_t mblk, pblk;
>                     ^~~~
> 
> Please fix that up and resend.

strange... my compiler (4.8.4) and huawei internal CI don't report that,
and this patchset has been in Chao's tree for a while, I don't get any report so far...

I just looked into that code again and it seems a false warning since
1) this code is heavily running on the products and working fine till now.
2) pblk gets a proper value before unzip_vle.c:1690 map->m_pa = blknr_to_addr(pblk);

so I think I need to silence this warning for now and check if there is a really issue....

Thanks,
Gao Xiang

> 
> thanks,
> 
> greg k-h
> 

Re: [PATCH 0/8] staging: erofs: error handing and more tracepoints

[Greg Kroah-Hartman]

On Tue, Sep 18, 2018 at 08:02:30PM +0800, Gao Xiang wrote:
> Hi Greg,
> 
> On 2018/9/18 19:19, Greg Kroah-Hartman wrote:
> > On Fri, Sep 14, 2018 at 10:40:22PM +0800, Gao Xiang wrote:
> >> In order to avoid conflicts with cleanup patches, Chao and I think
> >> it is better to send reviewed preview patches in the erofs mailing list
> >> to the community in time.
> >>
> >> So here is reviewed & tested patches right now, which clean up and
> >> enhance the error handing and add some tracepoints for decompression.
> >>
> >> Note that in this patchset, bare use of 'unsigned' and NULL comparison are
> >> also fixed compared with the preview patches according to the previous
> >> discussion in the staging mailing list.
> > 
> > I applied this, but I need to go delete it as this patch series adds a
> > build warning to the system:
> > 
> > In file included from drivers/staging/erofs/unzip_vle.h:16:0,
> >                  from drivers/staging/erofs/unzip_vle.c:13:
> > drivers/staging/erofs/unzip_vle.c: In function ‘z_erofs_map_blocks_iter’:
> > drivers/staging/erofs/internal.h:303:34: warning: ‘pblk’ may be used uninitialized in this function [-Wmaybe-uninitialized]
> >  #define blknr_to_addr(nr)       ((erofs_off_t)(nr) * EROFS_BLKSIZ)
> >                                   ^
> > drivers/staging/erofs/unzip_vle.c:1574:20: note: ‘pblk’ was declared here
> >   erofs_blk_t mblk, pblk;
> >                     ^~~~
> > 
> > Please fix that up and resend.
> 
> strange... my compiler (4.8.4) and huawei internal CI don't report that,
> and this patchset has been in Chao's tree for a while, I don't get any report so far...
> 
> I just looked into that code again and it seems a false warning since
> 1) this code is heavily running on the products and working fine till now.
> 2) pblk gets a proper value before unzip_vle.c:1690 map->m_pa = blknr_to_addr(pblk);
> 
> so I think I need to silence this warning for now and check if there is a really issue....

Don't just silent the warning.  Usually gcc now properly detects where
there really is a problem, it should not be a false-positive.  And in
looking at the code, it does seem that pblk could not be set if one of
the different case statements is taken and then exact_hitted: is jumped
to, right?

Or is gcc just being "dumb" here?  What about clang, have you tried
building it with that compiler as well?

thanks,

greg k-h

Re: [PATCH 0/8] staging: erofs: error handing and more tracepoints

[Gao Xiang]

Hi Greg,

On 2018/9/18 20:14, Greg Kroah-Hartman wrote:
> On Tue, Sep 18, 2018 at 08:02:30PM +0800, Gao Xiang wrote:
>> Hi Greg,
>>
>> On 2018/9/18 19:19, Greg Kroah-Hartman wrote:
>>> On Fri, Sep 14, 2018 at 10:40:22PM +0800, Gao Xiang wrote:
>>>> In order to avoid conflicts with cleanup patches, Chao and I think
>>>> it is better to send reviewed preview patches in the erofs mailing list
>>>> to the community in time.
>>>>
>>>> So here is reviewed & tested patches right now, which clean up and
>>>> enhance the error handing and add some tracepoints for decompression.
>>>>
>>>> Note that in this patchset, bare use of 'unsigned' and NULL comparison are
>>>> also fixed compared with the preview patches according to the previous
>>>> discussion in the staging mailing list.
>>>
>>> I applied this, but I need to go delete it as this patch series adds a
>>> build warning to the system:
>>>
>>> In file included from drivers/staging/erofs/unzip_vle.h:16:0,
>>>                  from drivers/staging/erofs/unzip_vle.c:13:
>>> drivers/staging/erofs/unzip_vle.c: In function ‘z_erofs_map_blocks_iter’:
>>> drivers/staging/erofs/internal.h:303:34: warning: ‘pblk’ may be used uninitialized in this function [-Wmaybe-uninitialized]
>>>  #define blknr_to_addr(nr)       ((erofs_off_t)(nr) * EROFS_BLKSIZ)
>>>                                   ^
>>> drivers/staging/erofs/unzip_vle.c:1574:20: note: ‘pblk’ was declared here
>>>   erofs_blk_t mblk, pblk;
>>>                     ^~~~
>>>
>>> Please fix that up and resend.
>>
>> strange... my compiler (4.8.4) and huawei internal CI don't report that,
>> and this patchset has been in Chao's tree for a while, I don't get any report so far...
>>
>> I just looked into that code again and it seems a false warning since
>> 1) this code is heavily running on the products and working fine till now.
>> 2) pblk gets a proper value before unzip_vle.c:1690 map->m_pa = blknr_to_addr(pblk);
>>
>> so I think I need to silence this warning for now and check if there is a really issue....
> 
> Don't just silent the warning.  Usually gcc now properly detects where
> there really is a problem, it should not be a false-positive.  And in
> looking at the code, it does seem that pblk could not be set if one of
> the different case statements is taken and then exact_hitted: is jumped
> to, right?

pblk is assigned before each "goto exact_hitted" and "break;" here.

1641         switch (cluster_type) {
1642         case Z_EROFS_VLE_CLUSTER_TYPE_PLAIN:
1643                 if (ofs_rem >= logical_cluster_ofs)
1644                         map->m_flags ^= EROFS_MAP_ZIPPED;
1645                 /* fallthrough */
1646         case Z_EROFS_VLE_CLUSTER_TYPE_HEAD:
1647                 if (ofs_rem == logical_cluster_ofs) {
1648                         pblk = le32_to_cpu(di->di_u.blkaddr);
1649                         goto exact_hitted; <--- assigned
1650                 }
1651
1652                 if (ofs_rem > logical_cluster_ofs) {
1653                         ofs = (u64)lcn * clustersize | logical_cluster_ofs;
1654                         pblk = le32_to_cpu(di->di_u.blkaddr);
1655                         break; <--- assigned
1656                 }
1657
1658                 /* logical cluster number should be >= 1 */
1659                 if (unlikely(!lcn)) {
1660                         errln("invalid logical cluster 0 at nid %llu",
1661                                 EROFS_V(inode)->nid);
1662                         err = -EIO;
1663                         goto unmap_out;  <--- bypass no need it
1664                 }
1665                 end = ((u64)lcn-- * clustersize) | logical_cluster_ofs;
1666                 /* fallthrough */
1667         case Z_EROFS_VLE_CLUSTER_TYPE_NONHEAD:
1668                 /* get the correspoinding first chunk */
1669                 err = vle_get_logical_extent_head(&ctx, lcn, &ofs,
1670                                                   &pblk, &map->m_flags);
1671                 mpage = *mpage_ret;
1672
1673                 if (unlikely(err)) {
1674                         if (mpage)
1675                                 goto unmap_out; <--- bypass no need it
1676                         goto out;   <--- bypass no need it
1677                 }
1678                 break;           <-- pblk should be assigned in vle_get_logical_extent_head
                                          if no error occurred in vle_get_logical_extent_head
1679         default:
1680                 errln("unknown cluster type %u at offset %llu of nid %llu",
1681                         cluster_type, ofs, EROFS_V(inode)->nid);
1682                 err = -EIO;
1683                 goto unmap_out;   <-- bypass no need it
1684         }
1685
1686         map->m_la = ofs;
1687 exact_hitted:
1688         map->m_llen = end - ofs;
1689         map->m_plen = clustersize;
1690         map->m_pa = blknr_to_addr(pblk);   <--- pblk is used here
1691         map->m_flags |= EROFS_MAP_MAPPED;
1692 unmap_out:
1693         kunmap_atomic(kaddr);
1694         unlock_page(mpage);
1695 out:

and in vle_get_logical_extent_head
1493 static int
1494 vle_get_logical_extent_head(const struct vle_map_blocks_iter_ctx *ctx,
1495                             unsigned int lcn,   /* logical cluster number */
1496                             unsigned long long *ofs,
1497                             erofs_blk_t *pblk,
1498                             unsigned int *flags)
1499 {
1500         const unsigned int clustersize = 1 << ctx->clusterbits;
1501         const erofs_blk_t mblk = vle_extent_blkaddr(ctx->inode, lcn);
1502         struct page *mpage = *ctx->mpage_ret;   /* extent metapage */
1503
1504         struct z_erofs_vle_decompressed_index *di;
1505         unsigned int cluster_type, delta0;
1506
1507         if (mpage->index != mblk) {
1508                 kunmap_atomic(*ctx->kaddr_ret);
1509                 unlock_page(mpage);
1510                 put_page(mpage);
1511
1512                 mpage = erofs_get_meta_page(ctx->sb, mblk, false);
1513                 if (IS_ERR(mpage)) {
1514                         *ctx->mpage_ret = NULL;
1515                         return PTR_ERR(mpage);   <--- bypass pblk since err != 0
1516                 }
1517                 *ctx->mpage_ret = mpage;
1518                 *ctx->kaddr_ret = kmap_atomic(mpage);
1519         }
1520
1521         di = *ctx->kaddr_ret + vle_extent_blkoff(ctx->inode, lcn);
1522
1523         cluster_type = vle_cluster_type(di);
1524         switch (cluster_type) {
1525         case Z_EROFS_VLE_CLUSTER_TYPE_NONHEAD:
1526                 delta0 = le16_to_cpu(di->di_u.delta[0]);
1527                 if (unlikely(!delta0 || delta0 > lcn)) {
1528                         errln("invalid NONHEAD dl0 %u at lcn %u of nid %llu",
1529                               delta0, lcn, EROFS_V(ctx->inode)->nid);
1530                         DBG_BUGON(1);
1531                         return -EIO;   <--- bypass pblk since err != 0
1532                 }
1533                 return vle_get_logical_extent_head(ctx,
1534                         lcn - delta0, ofs, pblk, flags);  <---- recursive call (vle_get_logical_extent_head at most 2 times)
1535         case Z_EROFS_VLE_CLUSTER_TYPE_PLAIN:
1536                 *flags ^= EROFS_MAP_ZIPPED;
1537                 /* fallthrough */
1538         case Z_EROFS_VLE_CLUSTER_TYPE_HEAD:
1539                 /* clustersize should be a power of two */
1540                 *ofs = ((u64)lcn << ctx->clusterbits) +
1541                         (le16_to_cpu(di->di_clusterofs) & (clustersize - 1));
1542                 *pblk = le32_to_cpu(di->di_u.blkaddr);
1543                 break;      <--- assigned
1544         default:
1545                 errln("unknown cluster type %u at lcn %u of nid %llu",
1546                       cluster_type, lcn, EROFS_V(ctx->inode)->nid);
1547                 DBG_BUGON(1);
1548                 return -EIO;   <--- bypass pblk since err != 0
1549         }
1550         return 0;
1551 }

(I have no clang environment to build kernel yet... :( I will try later, but I have no idea why it happens,
and it seems a false warning).

Thanks,
Gao Xiang

> 
> Or is gcc just being "dumb" here?  What about clang, have you tried
> building it with that compiler as well?
> 
> thanks,
> 
> greg k-h
> 

Re: [PATCH 0/8] staging: erofs: error handing and more tracepoints

[Dan Carpenter]

On Tue, Sep 18, 2018 at 08:31:22PM +0800, Gao Xiang wrote:
> (I have no clang environment to build kernel yet... :( I will try later, but I have no idea why it happens,
> and it seems a false warning).
> 
> Thanks,
> Gao Xiang
> 
> > 
> > Or is gcc just being "dumb" here?  What about clang, have you tried
> > building it with that compiler as well?

Yeah.  Gcc is wrong.  Gcc used to be extra conservative and missed a
bunch of bugs, but I guess now they're going in to opposite direction?

Smatch gets this correct, but you have to rebuild the cross function DB
twice to make the warning go away.  It could be because it starts out
with the old vle_get_logical_extent_head() information so it thinks it
knows how that function works.

regards,
dan carpenter

Re: [PATCH 0/8] staging: erofs: error handing and more tracepoints

[Greg Kroah-Hartman]

On Tue, Sep 18, 2018 at 04:03:37PM +0300, Dan Carpenter wrote:
> On Tue, Sep 18, 2018 at 08:31:22PM +0800, Gao Xiang wrote:
> > (I have no clang environment to build kernel yet... :( I will try later, but I have no idea why it happens,
> > and it seems a false warning).
> > 
> > Thanks,
> > Gao Xiang
> > 
> > > 
> > > Or is gcc just being "dumb" here?  What about clang, have you tried
> > > building it with that compiler as well?
> 
> Yeah.  Gcc is wrong.  Gcc used to be extra conservative and missed a
> bunch of bugs, but I guess now they're going in to opposite direction?
> 
> Smatch gets this correct, but you have to rebuild the cross function DB
> twice to make the warning go away.  It could be because it starts out
> with the old vle_get_logical_extent_head() information so it thinks it
> knows how that function works.

Ok, thanks for checking (both of you).  Just initialize the variable to
keep gcc from printing foolish warnings.

thanks,

greg k-h

Re: [PATCH 0/8] staging: erofs: error handing and more tracepoints

[Gao Xiang]

On 2018/9/18 21:09, Greg Kroah-Hartman wrote:
> On Tue, Sep 18, 2018 at 04:03:37PM +0300, Dan Carpenter wrote:
>> On Tue, Sep 18, 2018 at 08:31:22PM +0800, Gao Xiang wrote:
>>> (I have no clang environment to build kernel yet... :( I will try later, but I have no idea why it happens,
>>> and it seems a false warning).
>>>
>>> Thanks,
>>> Gao Xiang
>>>
>>>>
>>>> Or is gcc just being "dumb" here?  What about clang, have you tried
>>>> building it with that compiler as well?
>>
>> Yeah.  Gcc is wrong.  Gcc used to be extra conservative and missed a
>> bunch of bugs, but I guess now they're going in to opposite direction?
>>
>> Smatch gets this correct, but you have to rebuild the cross function DB
>> twice to make the warning go away.  It could be because it starts out
>> with the old vle_get_logical_extent_head() information so it thinks it
>> knows how that function works.

Hi Dan,
Thanks for taking time to confirm this and the detailed explanation ;)

> 
> Ok, thanks for checking (both of you).  Just initialize the variable to
> keep gcc from printing foolish warnings.

Hi Greg,

Ok, if you don't tend to use `uninitialized_var(...)', I will initialize a 
dumb value...but I think it is really useless to initialize it... :(

I will resend the related false warning patch independently soon...

Thanks,
Gao Xiang

> 
> thanks,
> 
> greg k-h
> 

[PATCH] staging: erofs: initialize `pblk' with 0 first in `z_erofs_map_blocks_iter'

[Gao Xiang]

This commit message helps to understand why `pblk' is assigned with 0 here.

[ Greg reported a warning raised by gcc. ]
In file included from drivers/staging/erofs/unzip_vle.h:16:0,
                 from drivers/staging/erofs/unzip_vle.c:13:
drivers/staging/erofs/unzip_vle.c: In function ‘z_erofs_map_blocks_iter’:
drivers/staging/erofs/internal.h:303:34: warning: ‘pblk’ may be used uninitialized in this function [-Wmaybe-uninitialized]
 #define blknr_to_addr(nr)       ((erofs_off_t)(nr) * EROFS_BLKSIZ)
                                  ^
drivers/staging/erofs/unzip_vle.c:1574:20: note: ‘pblk’ was declared here
  erofs_blk_t mblk, pblk;
                    ^~~~

Actually, it is a false-positive warning when looking into that. [1]
Just initialize the variable to keep gcc from printing foolish warnings.

[1] https://lists.ozlabs.org/pipermail/linux-erofs/2018-September/000637.html

Reported-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Thanks-to: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>
---
 drivers/staging/erofs/unzip_vle.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/erofs/unzip_vle.c b/drivers/staging/erofs/unzip_vle.c
index ad3b7bb..3ff8adf 100644
--- a/drivers/staging/erofs/unzip_vle.c
+++ b/drivers/staging/erofs/unzip_vle.c
@@ -1571,7 +1571,7 @@ int z_erofs_map_blocks_iter(struct inode *inode,
 	unsigned int lcn;
 	u32 ofs_rem;
 
-	erofs_blk_t mblk, pblk;
+	erofs_blk_t mblk, pblk = 0;
 	struct page *mpage = *mpage_ret;
 	struct z_erofs_vle_decompressed_index *di;
 	unsigned int cluster_type, logical_cluster_ofs;
-- 
1.9.1

Re: [PATCH] staging: erofs: initialize `pblk' with 0 first in `z_erofs_map_blocks_iter'

[Greg Kroah-Hartman]

On Tue, Sep 18, 2018 at 09:44:36PM +0800, Gao Xiang wrote:
> This commit message helps to understand why `pblk' is assigned with 0 here.
> 
> [ Greg reported a warning raised by gcc. ]
> In file included from drivers/staging/erofs/unzip_vle.h:16:0,
>                  from drivers/staging/erofs/unzip_vle.c:13:
> drivers/staging/erofs/unzip_vle.c: In function ‘z_erofs_map_blocks_iter’:
> drivers/staging/erofs/internal.h:303:34: warning: ‘pblk’ may be used uninitialized in this function [-Wmaybe-uninitialized]
>  #define blknr_to_addr(nr)       ((erofs_off_t)(nr) * EROFS_BLKSIZ)
>                                   ^
> drivers/staging/erofs/unzip_vle.c:1574:20: note: ‘pblk’ was declared here
>   erofs_blk_t mblk, pblk;
>                     ^~~~
> 
> Actually, it is a false-positive warning when looking into that. [1]
> Just initialize the variable to keep gcc from printing foolish warnings.
> 
> [1] https://lists.ozlabs.org/pipermail/linux-erofs/2018-September/000637.html
> 
> Reported-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Thanks-to: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>
> ---
>  drivers/staging/erofs/unzip_vle.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Please roll this into the original patch.  I have dropped that whole
series from my tree, it is no longer present and needs to be resent in
order for me to be able to accept any of these.

thanks,

greg k-h

Re: [PATCH] staging: erofs: initialize `pblk' with 0 first in `z_erofs_map_blocks_iter'

[Gao Xiang]

Hi Greg,

On 2018/9/18 21:57, Greg Kroah-Hartman wrote:
> On Tue, Sep 18, 2018 at 09:44:36PM +0800, Gao Xiang wrote:
>> This commit message helps to understand why `pblk' is assigned with 0 here.
>>
>> [ Greg reported a warning raised by gcc. ]
>> In file included from drivers/staging/erofs/unzip_vle.h:16:0,
>>                  from drivers/staging/erofs/unzip_vle.c:13:
>> drivers/staging/erofs/unzip_vle.c: In function ‘z_erofs_map_blocks_iter’:
>> drivers/staging/erofs/internal.h:303:34: warning: ‘pblk’ may be used uninitialized in this function [-Wmaybe-uninitialized]
>>  #define blknr_to_addr(nr)       ((erofs_off_t)(nr) * EROFS_BLKSIZ)
>>                                   ^
>> drivers/staging/erofs/unzip_vle.c:1574:20: note: ‘pblk’ was declared here
>>   erofs_blk_t mblk, pblk;
>>                     ^~~~
>>
>> Actually, it is a false-positive warning when looking into that. [1]
>> Just initialize the variable to keep gcc from printing foolish warnings.
>>
>> [1] https://lists.ozlabs.org/pipermail/linux-erofs/2018-September/000637.html
>>
>> Reported-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> Thanks-to: Dan Carpenter <dan.carpenter@oracle.com>
>> Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>
>> ---
>>  drivers/staging/erofs/unzip_vle.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> Please roll this into the original patch.  I have dropped that whole
> series from my tree, it is no longer present and needs to be resent in
> order for me to be able to accept any of these.

OK, will send. it's my honor.

Thanks,
Gao Xiang

> 
> thanks,
> 
> greg k-h
>