linux-erofs.lists.ozlabs.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 1/7] erofs: on-disk format should have explicitly assigned numbers
@ 2019-08-30  3:00 Gao Xiang
  2019-08-30  3:00 ` [PATCH v2 2/7] erofs: some marcos are much more readable as a function Gao Xiang
                   ` (6 more replies)
  0 siblings, 7 replies; 38+ messages in thread
From: Gao Xiang @ 2019-08-30  3:00 UTC (permalink / raw)
  To: Chao Yu, Dan Carpenter, Christoph Hellwig, Joe Perches,
	Greg Kroah-Hartman, devel
  Cc: linux-erofs, LKML, weidu.du, Miao Xie

As Christoph claimed [1], on-disk format should have
explicitly assigned numbers. I have to change it.

[1] https://lore.kernel.org/r/20190829095954.GB20598@infradead.org/
Reported-by: Christoph Hellwig <hch@infradead.org>
Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>
---
v2: no change, just resend in case of dependency problem;

I have to say one word "all patches are trivial".

 fs/erofs/erofs_fs.h | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/fs/erofs/erofs_fs.h b/fs/erofs/erofs_fs.h
index afa7d45ca958..2447ad4d0920 100644
--- a/fs/erofs/erofs_fs.h
+++ b/fs/erofs/erofs_fs.h
@@ -52,10 +52,10 @@ struct erofs_super_block {
  * 4~7 - reserved
  */
 enum {
-	EROFS_INODE_FLAT_PLAIN,
-	EROFS_INODE_FLAT_COMPRESSION_LEGACY,
-	EROFS_INODE_FLAT_INLINE,
-	EROFS_INODE_FLAT_COMPRESSION,
+	EROFS_INODE_FLAT_PLAIN			= 0,
+	EROFS_INODE_FLAT_COMPRESSION_LEGACY	= 1,
+	EROFS_INODE_FLAT_INLINE			= 2,
+	EROFS_INODE_FLAT_COMPRESSION		= 3,
 	EROFS_INODE_LAYOUT_MAX
 };
 
@@ -181,7 +181,7 @@ struct erofs_xattr_entry {
 
 /* available compression algorithm types */
 enum {
-	Z_EROFS_COMPRESSION_LZ4,
+	Z_EROFS_COMPRESSION_LZ4	= 0,
 	Z_EROFS_COMPRESSION_MAX
 };
 
@@ -239,10 +239,10 @@ struct z_erofs_map_header {
  *                (di_advise could be 0, 1 or 2)
  */
 enum {
-	Z_EROFS_VLE_CLUSTER_TYPE_PLAIN,
-	Z_EROFS_VLE_CLUSTER_TYPE_HEAD,
-	Z_EROFS_VLE_CLUSTER_TYPE_NONHEAD,
-	Z_EROFS_VLE_CLUSTER_TYPE_RESERVED,
+	Z_EROFS_VLE_CLUSTER_TYPE_PLAIN		= 0,
+	Z_EROFS_VLE_CLUSTER_TYPE_HEAD		= 1,
+	Z_EROFS_VLE_CLUSTER_TYPE_NONHEAD	= 2,
+	Z_EROFS_VLE_CLUSTER_TYPE_RESERVED	= 3,
 	Z_EROFS_VLE_CLUSTER_TYPE_MAX
 };
 
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* [PATCH v2 2/7] erofs: some marcos are much more readable as a function
  2019-08-30  3:00 [PATCH v2 1/7] erofs: on-disk format should have explicitly assigned numbers Gao Xiang
@ 2019-08-30  3:00 ` Gao Xiang
  2019-08-30  3:16   ` Joe Perches
  2019-08-30  3:00 ` [PATCH v2 3/7] erofs: use a better form for complicated on-disk fields Gao Xiang
                   ` (5 subsequent siblings)
  6 siblings, 1 reply; 38+ messages in thread
From: Gao Xiang @ 2019-08-30  3:00 UTC (permalink / raw)
  To: Chao Yu, Dan Carpenter, Christoph Hellwig, Joe Perches,
	Greg Kroah-Hartman, devel
  Cc: linux-erofs, LKML, weidu.du, Miao Xie

As Christoph suggested [1], these marcos are much
more readable as a function.

[1] https://lore.kernel.org/r/20190829095954.GB20598@infradead.org/
Reported-by: Christoph Hellwig <hch@infradead.org>
Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>
---
v2: no change, just resend in case of dependency problem;

 fs/erofs/erofs_fs.h | 24 ++++++++++++++++--------
 fs/erofs/inode.c    |  4 ++--
 fs/erofs/xattr.c    |  2 +-
 3 files changed, 19 insertions(+), 11 deletions(-)

diff --git a/fs/erofs/erofs_fs.h b/fs/erofs/erofs_fs.h
index 2447ad4d0920..41e53b49a11b 100644
--- a/fs/erofs/erofs_fs.h
+++ b/fs/erofs/erofs_fs.h
@@ -168,16 +168,24 @@ struct erofs_xattr_entry {
 	char   e_name[0];       /* attribute name */
 } __packed;
 
-#define ondisk_xattr_ibody_size(count)	({\
-	u32 __count = le16_to_cpu(count); \
-	((__count) == 0) ? 0 : \
-	sizeof(struct erofs_xattr_ibody_header) + \
-		sizeof(__u32) * ((__count) - 1); })
+static inline unsigned int erofs_xattr_ibody_size(__le16 d_icount)
+{
+	unsigned int icount = le16_to_cpu(d_icount);
+
+	if (!icount)
+		return 0;
+
+	return sizeof(struct erofs_xattr_ibody_header) +
+		sizeof(__u32) * (icount - 1);
+}
 
 #define EROFS_XATTR_ALIGN(size) round_up(size, sizeof(struct erofs_xattr_entry))
-#define EROFS_XATTR_ENTRY_SIZE(entry) EROFS_XATTR_ALIGN( \
-	sizeof(struct erofs_xattr_entry) + \
-	(entry)->e_name_len + le16_to_cpu((entry)->e_value_size))
+
+static inline unsigned int erofs_xattr_entry_size(struct erofs_xattr_entry *e)
+{
+	return EROFS_XATTR_ALIGN(sizeof(struct erofs_xattr_entry) +
+				 e->e_name_len + le16_to_cpu(e->e_value_size));
+}
 
 /* available compression algorithm types */
 enum {
diff --git a/fs/erofs/inode.c b/fs/erofs/inode.c
index 80f4fe919ee7..cf31554075c9 100644
--- a/fs/erofs/inode.c
+++ b/fs/erofs/inode.c
@@ -29,7 +29,7 @@ static int read_inode(struct inode *inode, void *data)
 		struct erofs_inode_v2 *v2 = data;
 
 		vi->inode_isize = sizeof(struct erofs_inode_v2);
-		vi->xattr_isize = ondisk_xattr_ibody_size(v2->i_xattr_icount);
+		vi->xattr_isize = erofs_xattr_ibody_size(v2->i_xattr_icount);
 
 		inode->i_mode = le16_to_cpu(v2->i_mode);
 		if (S_ISREG(inode->i_mode) || S_ISDIR(inode->i_mode) ||
@@ -62,7 +62,7 @@ static int read_inode(struct inode *inode, void *data)
 		struct erofs_sb_info *sbi = EROFS_SB(inode->i_sb);
 
 		vi->inode_isize = sizeof(struct erofs_inode_v1);
-		vi->xattr_isize = ondisk_xattr_ibody_size(v1->i_xattr_icount);
+		vi->xattr_isize = erofs_xattr_ibody_size(v1->i_xattr_icount);
 
 		inode->i_mode = le16_to_cpu(v1->i_mode);
 		if (S_ISREG(inode->i_mode) || S_ISDIR(inode->i_mode) ||
diff --git a/fs/erofs/xattr.c b/fs/erofs/xattr.c
index a8286998a079..7ef8d4bb45cd 100644
--- a/fs/erofs/xattr.c
+++ b/fs/erofs/xattr.c
@@ -231,7 +231,7 @@ static int xattr_foreach(struct xattr_iter *it,
 	 */
 	entry = *(struct erofs_xattr_entry *)(it->kaddr + it->ofs);
 	if (tlimit) {
-		unsigned int entry_sz = EROFS_XATTR_ENTRY_SIZE(&entry);
+		unsigned int entry_sz = erofs_xattr_entry_size(&entry);
 
 		/* xattr on-disk corruption: xattr entry beyond xattr_isize */
 		if (unlikely(*tlimit < entry_sz)) {
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* [PATCH v2 3/7] erofs: use a better form for complicated on-disk fields
  2019-08-30  3:00 [PATCH v2 1/7] erofs: on-disk format should have explicitly assigned numbers Gao Xiang
  2019-08-30  3:00 ` [PATCH v2 2/7] erofs: some marcos are much more readable as a function Gao Xiang
@ 2019-08-30  3:00 ` Gao Xiang
  2019-08-30  3:29   ` Chao Yu
  2019-08-30  3:00 ` [PATCH v2 4/7] erofs: kill __packed for on-disk structures Gao Xiang
                   ` (4 subsequent siblings)
  6 siblings, 1 reply; 38+ messages in thread
From: Gao Xiang @ 2019-08-30  3:00 UTC (permalink / raw)
  To: Chao Yu, Dan Carpenter, Christoph Hellwig, Joe Perches,
	Greg Kroah-Hartman, devel
  Cc: linux-erofs, LKML, weidu.du, Miao Xie

As Joe Perches [1] suggested, let's use a better
form to describe complicated on-disk fields.

p.s. it has different tab alignment looking between
     the real file and patch file.
p.p.s. due to changing a different form, some lines
       have to exceed 80 characters.
[1] https://lore.kernel.org/r/67d6efbbc9ac6db23215660cb970b7ef29dc0c1d.camel@perches.com/
Reported-by: Joe Perches <joe@perches.com>
Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>
---
new patch.

 fs/erofs/erofs_fs.h | 100 ++++++++++++++++++++++----------------------
 1 file changed, 50 insertions(+), 50 deletions(-)

diff --git a/fs/erofs/erofs_fs.h b/fs/erofs/erofs_fs.h
index 41e53b49a11b..76edc595cc4a 100644
--- a/fs/erofs/erofs_fs.h
+++ b/fs/erofs/erofs_fs.h
@@ -17,27 +17,27 @@
 #define EROFS_REQUIREMENT_LZ4_0PADDING	0x00000001
 #define EROFS_ALL_REQUIREMENTS		EROFS_REQUIREMENT_LZ4_0PADDING
 
-struct erofs_super_block {
-/*  0 */__le32 magic;           /* in the little endian */
-/*  4 */__le32 checksum;        /* crc32c(super_block) */
-/*  8 */__le32 features;        /* (aka. feature_compat) */
-/* 12 */__u8 blkszbits;         /* support block_size == PAGE_SIZE only */
-/* 13 */__u8 reserved;
-
-/* 14 */__le16 root_nid;
-/* 16 */__le64 inos;            /* total valid ino # (== f_files - f_favail) */
-
-/* 24 */__le64 build_time;      /* inode v1 time derivation */
-/* 32 */__le32 build_time_nsec;
-/* 36 */__le32 blocks;          /* used for statfs */
-/* 40 */__le32 meta_blkaddr;
-/* 44 */__le32 xattr_blkaddr;
-/* 48 */__u8 uuid[16];          /* 128-bit uuid for volume */
-/* 64 */__u8 volume_name[16];   /* volume name */
-/* 80 */__le32 requirements;    /* (aka. feature_incompat) */
-
-/* 84 */__u8 reserved2[44];
-} __packed;                     /* 128 bytes */
+struct erofs_super_block {	/* off description */
+	__le32 magic;		/*  0  file system magic number */
+	__le32 checksum;	/*  4  crc32c(super_block) */
+	__le32 features;	/*  8  (aka. feature_compat) */
+	__u8 blkszbits;		/* 12  support PAGE_SIZE only currently */
+	__u8 reserved;		/* 13  */
+
+	__le16 root_nid;	/* 14  nid of root directory */
+	__le64 inos;		/* 16  total valid ino # (== f_files - f_favail) */
+
+	__le64 build_time;	/* 24  inode v1 time derivation */
+	__le32 build_time_nsec;	/* 32  inode v1 time derivation in nano scale */
+	__le32 blocks;		/* 36  used for statfs */
+	__le32 meta_blkaddr;	/* 40  start block address of metadata area */
+	__le32 xattr_blkaddr;	/* 44  start block address of shared xattr area */
+	__u8 uuid[16];		/* 48  128-bit uuid for volume */
+	__u8 volume_name[16];	/* 64  volume name */
+	__le32 requirements;	/* 80  (aka. feature_incompat) */
+
+	__u8 reserved2[44];	/* 84 */
+} __packed;			/* 128 bytes */
 
 /*
  * erofs inode data mapping:
@@ -73,16 +73,16 @@ static inline bool erofs_inode_is_data_compressed(unsigned int datamode)
 #define EROFS_I_VERSION_BIT             0
 #define EROFS_I_DATA_MAPPING_BIT        1
 
-struct erofs_inode_v1 {
-/*  0 */__le16 i_advise;
+struct erofs_inode_v1 {		/* off description */
+	__le16 i_advise;	/*  0  file hints */
 
 /* 1 header + n-1 * 4 bytes inline xattr to keep continuity */
-/*  2 */__le16 i_xattr_icount;
-/*  4 */__le16 i_mode;
-/*  6 */__le16 i_nlink;
-/*  8 */__le32 i_size;
-/* 12 */__le32 i_reserved;
-/* 16 */union {
+	__le16 i_xattr_icount;	/*  2  encoding for xattr ibody size */
+	__le16 i_mode;		/*  4 */
+	__le16 i_nlink;		/*  6 */
+	__le32 i_size;		/*  8 */
+	__le32 i_reserved;	/* 12 */
+	union {			/* 16 */
 		/* file total compressed blocks for data mapping 1 */
 		__le32 compressed_blocks;
 		__le32 raw_blkaddr;
@@ -90,26 +90,26 @@ struct erofs_inode_v1 {
 		/* for device files, used to indicate old/new device # */
 		__le32 rdev;
 	} i_u __packed;
-/* 20 */__le32 i_ino;           /* only used for 32-bit stat compatibility */
-/* 24 */__le16 i_uid;
-/* 26 */__le16 i_gid;
-/* 28 */__le32 i_reserved2;
-} __packed;
+	__le32 i_ino;		/* 20 only used for 32-bit stat compatibility */
+	__le16 i_uid;		/* 24 */
+	__le16 i_gid;		/* 26 */
+	__le32 i_reserved2;	/* 28 */
+} __packed;			/* 32 bytes */
 
 /* 32 bytes on-disk inode */
 #define EROFS_INODE_LAYOUT_V1   0
 /* 64 bytes on-disk inode */
 #define EROFS_INODE_LAYOUT_V2   1
 
-struct erofs_inode_v2 {
-/*  0 */__le16 i_advise;
+struct erofs_inode_v2 {		/* off description */
+	__le16 i_advise;	/*  0  file hints */
 
 /* 1 header + n-1 * 4 bytes inline xattr to keep continuity */
-/*  2 */__le16 i_xattr_icount;
-/*  4 */__le16 i_mode;
-/*  6 */__le16 i_reserved;
-/*  8 */__le64 i_size;
-/* 16 */union {
+	__le16 i_xattr_icount;	/*  2  encoding for xattr ibody size */
+	__le16 i_mode;		/*  4 */
+	__le16 i_reserved;	/*  6 */
+	__le64 i_size;		/*  8 */
+	union {			/* 16 */
 		/* file total compressed blocks for data mapping 1 */
 		__le32 compressed_blocks;
 		__le32 raw_blkaddr;
@@ -119,15 +119,15 @@ struct erofs_inode_v2 {
 	} i_u __packed;
 
 	/* only used for 32-bit stat compatibility */
-/* 20 */__le32 i_ino;
-
-/* 24 */__le32 i_uid;
-/* 28 */__le32 i_gid;
-/* 32 */__le64 i_ctime;
-/* 40 */__le32 i_ctime_nsec;
-/* 44 */__le32 i_nlink;
-/* 48 */__u8   i_reserved2[16];
-} __packed;                     /* 64 bytes */
+	__le32 i_ino;		/* 20 only used for 32-bit stat compatibility */
+
+	__le32 i_uid;		/* 24 */
+	__le32 i_gid;		/* 28 */
+	__le64 i_ctime;		/* 32 */
+	__le32 i_ctime_nsec;	/* 40 */
+	__le32 i_nlink;		/* 44 */
+	__u8   i_reserved2[16];	/* 48 */
+} __packed;			/* 64 bytes */
 
 #define EROFS_MAX_SHARED_XATTRS         (128)
 /* h_shared_count between 129 ... 255 are special # */
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* [PATCH v2 4/7] erofs: kill __packed for on-disk structures
  2019-08-30  3:00 [PATCH v2 1/7] erofs: on-disk format should have explicitly assigned numbers Gao Xiang
  2019-08-30  3:00 ` [PATCH v2 2/7] erofs: some marcos are much more readable as a function Gao Xiang
  2019-08-30  3:00 ` [PATCH v2 3/7] erofs: use a better form for complicated on-disk fields Gao Xiang
@ 2019-08-30  3:00 ` Gao Xiang
  2019-08-30  3:00 ` [PATCH v2 5/7] erofs: kill erofs_{init,exit}_inode_cache Gao Xiang
                   ` (3 subsequent siblings)
  6 siblings, 0 replies; 38+ messages in thread
From: Gao Xiang @ 2019-08-30  3:00 UTC (permalink / raw)
  To: Chao Yu, Dan Carpenter, Christoph Hellwig, Joe Perches,
	Greg Kroah-Hartman, devel
  Cc: linux-erofs, LKML, weidu.du, Miao Xie

As Christoph claimed "Please don't add __packed" [1],
I have to remove all __packed except struct erofs_dirent here.

Note that all on-disk fields except struct erofs_dirent
(12 bytes with a 8-byte nid) in EROFS are naturally aligned.

[1] https://lore.kernel.org/lkml/20190829095954.GB20598@infradead.org/
Reported-by: Christoph Hellwig <hch@infradead.org>
Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>
---
new patch.

 fs/erofs/erofs_fs.h | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/fs/erofs/erofs_fs.h b/fs/erofs/erofs_fs.h
index 76edc595cc4a..43e427d28b1d 100644
--- a/fs/erofs/erofs_fs.h
+++ b/fs/erofs/erofs_fs.h
@@ -37,7 +37,7 @@ struct erofs_super_block {	/* off description */
 	__le32 requirements;	/* 80  (aka. feature_incompat) */
 
 	__u8 reserved2[44];	/* 84 */
-} __packed;			/* 128 bytes */
+};				/* 128 bytes */
 
 /*
  * erofs inode data mapping:
@@ -89,12 +89,12 @@ struct erofs_inode_v1 {		/* off description */
 
 		/* for device files, used to indicate old/new device # */
 		__le32 rdev;
-	} i_u __packed;
+	} i_u;
 	__le32 i_ino;		/* 20 only used for 32-bit stat compatibility */
 	__le16 i_uid;		/* 24 */
 	__le16 i_gid;		/* 26 */
 	__le32 i_reserved2;	/* 28 */
-} __packed;			/* 32 bytes */
+};				/* 32 bytes */
 
 /* 32 bytes on-disk inode */
 #define EROFS_INODE_LAYOUT_V1   0
@@ -116,7 +116,7 @@ struct erofs_inode_v2 {		/* off description */
 
 		/* for device files, used to indicate old/new device # */
 		__le32 rdev;
-	} i_u __packed;
+	} i_u;
 
 	/* only used for 32-bit stat compatibility */
 	__le32 i_ino;		/* 20 only used for 32-bit stat compatibility */
@@ -127,7 +127,7 @@ struct erofs_inode_v2 {		/* off description */
 	__le32 i_ctime_nsec;	/* 40 */
 	__le32 i_nlink;		/* 44 */
 	__u8   i_reserved2[16];	/* 48 */
-} __packed;			/* 64 bytes */
+};				/* 64 bytes */
 
 #define EROFS_MAX_SHARED_XATTRS         (128)
 /* h_shared_count between 129 ... 255 are special # */
@@ -149,7 +149,7 @@ struct erofs_xattr_ibody_header {
 	__u8   h_shared_count;
 	__u8   h_reserved2[7];
 	__le32 h_shared_xattrs[0];      /* shared xattr id array */
-} __packed;
+};
 
 /* Name indexes */
 #define EROFS_XATTR_INDEX_USER              1
@@ -166,7 +166,7 @@ struct erofs_xattr_entry {
 	__le16 e_value_size;    /* size of attribute value */
 	/* followed by e_name and e_value */
 	char   e_name[0];       /* attribute name */
-} __packed;
+};
 
 static inline unsigned int erofs_xattr_ibody_size(__le16 d_icount)
 {
@@ -272,8 +272,8 @@ struct z_erofs_vle_decompressed_index {
 		 * [1] - pointing to the tail cluster
 		 */
 		__le16 delta[2];
-	} di_u __packed;		/* 8 bytes */
-} __packed;
+	} di_u;			/* 8 bytes */
+};
 
 #define Z_EROFS_VLE_LEGACY_INDEX_ALIGN(size) \
 	(round_up(size, sizeof(struct z_erofs_vle_decompressed_index)) + \
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* [PATCH v2 5/7] erofs: kill erofs_{init,exit}_inode_cache
  2019-08-30  3:00 [PATCH v2 1/7] erofs: on-disk format should have explicitly assigned numbers Gao Xiang
                   ` (2 preceding siblings ...)
  2019-08-30  3:00 ` [PATCH v2 4/7] erofs: kill __packed for on-disk structures Gao Xiang
@ 2019-08-30  3:00 ` Gao Xiang
  2019-08-30  3:00 ` [PATCH v2 6/7] erofs: remove all likely/unlikely annotations Gao Xiang
                   ` (2 subsequent siblings)
  6 siblings, 0 replies; 38+ messages in thread
From: Gao Xiang @ 2019-08-30  3:00 UTC (permalink / raw)
  To: Chao Yu, Dan Carpenter, Christoph Hellwig, Joe Perches,
	Greg Kroah-Hartman, devel
  Cc: linux-erofs, LKML, weidu.du, Miao Xie

As Christoph said [1] "having this function
seems entirely pointless", I have to kill those.

filesystem                              function name
ext2,f2fs,ext4,isofs,squashfs,cifs,...  init_inodecache

In addition, add a "rcu_barrier()" when exit_fs();

[1] https://lore.kernel.org/r/20190829101545.GC20598@infradead.org/
Reported-by: Christoph Hellwig <hch@infradead.org>
Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>
---
new patch.

 fs/erofs/super.c | 31 ++++++++++++-------------------
 1 file changed, 12 insertions(+), 19 deletions(-)

diff --git a/fs/erofs/super.c b/fs/erofs/super.c
index 6d3a9bcb8daa..556aae5426d6 100644
--- a/fs/erofs/super.c
+++ b/fs/erofs/super.c
@@ -23,21 +23,6 @@ static void init_once(void *ptr)
 	inode_init_once(&vi->vfs_inode);
 }
 
-static int __init erofs_init_inode_cache(void)
-{
-	erofs_inode_cachep = kmem_cache_create("erofs_inode",
-					       sizeof(struct erofs_vnode), 0,
-					       SLAB_RECLAIM_ACCOUNT,
-					       init_once);
-
-	return erofs_inode_cachep ? 0 : -ENOMEM;
-}
-
-static void erofs_exit_inode_cache(void)
-{
-	kmem_cache_destroy(erofs_inode_cachep);
-}
-
 static struct inode *alloc_inode(struct super_block *sb)
 {
 	struct erofs_vnode *vi =
@@ -531,9 +516,14 @@ static int __init erofs_module_init(void)
 	erofs_check_ondisk_layout_definitions();
 	infoln("initializing erofs " EROFS_VERSION);
 
-	err = erofs_init_inode_cache();
-	if (err)
+	erofs_inode_cachep = kmem_cache_create("erofs_inode",
+					       sizeof(struct erofs_vnode), 0,
+					       SLAB_RECLAIM_ACCOUNT,
+					       init_once);
+	if (!erofs_inode_cachep) {
+		err = -ENOMEM;
 		goto icache_err;
+	}
 
 	err = erofs_init_shrinker();
 	if (err)
@@ -555,7 +545,7 @@ static int __init erofs_module_init(void)
 zip_err:
 	erofs_exit_shrinker();
 shrinker_err:
-	erofs_exit_inode_cache();
+	kmem_cache_destroy(erofs_inode_cachep);
 icache_err:
 	return err;
 }
@@ -565,7 +555,10 @@ static void __exit erofs_module_exit(void)
 	unregister_filesystem(&erofs_fs_type);
 	z_erofs_exit_zip_subsystem();
 	erofs_exit_shrinker();
-	erofs_exit_inode_cache();
+
+	/* Ensure all RCU free inodes are safe before cache is destroyed. */
+	rcu_barrier();
+	kmem_cache_destroy(erofs_inode_cachep);
 	infoln("successfully finalize erofs");
 }
 
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* [PATCH v2 6/7] erofs: remove all likely/unlikely annotations
  2019-08-30  3:00 [PATCH v2 1/7] erofs: on-disk format should have explicitly assigned numbers Gao Xiang
                   ` (3 preceding siblings ...)
  2019-08-30  3:00 ` [PATCH v2 5/7] erofs: kill erofs_{init,exit}_inode_cache Gao Xiang
@ 2019-08-30  3:00 ` Gao Xiang
  2019-08-30  3:00 ` [PATCH v2 7/7] erofs: reduntant assignment in __erofs_get_meta_page() Gao Xiang
  2019-08-30  3:28 ` [PATCH v2 1/7] erofs: on-disk format should have explicitly assigned numbers Chao Yu
  6 siblings, 0 replies; 38+ messages in thread
From: Gao Xiang @ 2019-08-30  3:00 UTC (permalink / raw)
  To: Chao Yu, Dan Carpenter, Christoph Hellwig, Joe Perches,
	Greg Kroah-Hartman, devel
  Cc: linux-erofs, LKML, weidu.du, Miao Xie

As Dan Carpenter suggested [1], I have to remove
all erofs likely/unlikely annotations.

[1] https://lore.kernel.org/linux-fsdevel/20190829154346.GK23584@kadam/
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>
---
v2: no change, just resend in case of dependency problem;

 fs/erofs/data.c         | 22 ++++++++++-----------
 fs/erofs/decompressor.c |  2 +-
 fs/erofs/dir.c          | 14 ++++++-------
 fs/erofs/inode.c        | 10 +++++-----
 fs/erofs/internal.h     |  4 ++--
 fs/erofs/namei.c        | 14 ++++++-------
 fs/erofs/super.c        | 16 +++++++--------
 fs/erofs/utils.c        | 12 +++++------
 fs/erofs/xattr.c        | 12 +++++------
 fs/erofs/zdata.c        | 44 ++++++++++++++++++++---------------------
 fs/erofs/zmap.c         |  8 ++++----
 fs/erofs/zpvec.h        |  6 +++---
 12 files changed, 82 insertions(+), 82 deletions(-)

diff --git a/fs/erofs/data.c b/fs/erofs/data.c
index fda16ec8863e..0f2f1a839372 100644
--- a/fs/erofs/data.c
+++ b/fs/erofs/data.c
@@ -27,7 +27,7 @@ static inline void read_endio(struct bio *bio)
 		/* page is already locked */
 		DBG_BUGON(PageUptodate(page));
 
-		if (unlikely(err))
+		if (err)
 			SetPageError(page);
 		else
 			SetPageUptodate(page);
@@ -53,7 +53,7 @@ struct page *__erofs_get_meta_page(struct super_block *sb,
 
 repeat:
 	page = find_or_create_page(mapping, blkaddr, gfp);
-	if (unlikely(!page)) {
+	if (!page) {
 		DBG_BUGON(nofail);
 		return ERR_PTR(-ENOMEM);
 	}
@@ -70,7 +70,7 @@ struct page *__erofs_get_meta_page(struct super_block *sb,
 		}
 
 		err = bio_add_page(bio, page, PAGE_SIZE, 0);
-		if (unlikely(err != PAGE_SIZE)) {
+		if (err != PAGE_SIZE) {
 			err = -EFAULT;
 			goto err_out;
 		}
@@ -81,7 +81,7 @@ struct page *__erofs_get_meta_page(struct super_block *sb,
 		lock_page(page);
 
 		/* this page has been truncated by others */
-		if (unlikely(page->mapping != mapping)) {
+		if (page->mapping != mapping) {
 unlock_repeat:
 			unlock_page(page);
 			put_page(page);
@@ -89,7 +89,7 @@ struct page *__erofs_get_meta_page(struct super_block *sb,
 		}
 
 		/* more likely a read error */
-		if (unlikely(!PageUptodate(page))) {
+		if (!PageUptodate(page)) {
 			if (io_retries) {
 				--io_retries;
 				goto unlock_repeat;
@@ -120,7 +120,7 @@ static int erofs_map_blocks_flatmode(struct inode *inode,
 	nblocks = DIV_ROUND_UP(inode->i_size, PAGE_SIZE);
 	lastblk = nblocks - is_inode_flat_inline(inode);
 
-	if (unlikely(offset >= inode->i_size)) {
+	if (offset >= inode->i_size) {
 		/* leave out-of-bound access unmapped */
 		map->m_flags = 0;
 		map->m_plen = 0;
@@ -170,7 +170,7 @@ static int erofs_map_blocks_flatmode(struct inode *inode,
 int erofs_map_blocks(struct inode *inode,
 		     struct erofs_map_blocks *map, int flags)
 {
-	if (unlikely(is_inode_layout_compression(inode))) {
+	if (is_inode_layout_compression(inode)) {
 		int err = z_erofs_map_blocks_iter(inode, map, flags);
 
 		if (map->mpage) {
@@ -218,11 +218,11 @@ static inline struct bio *erofs_read_raw_page(struct bio *bio,
 		unsigned int blkoff;
 
 		err = erofs_map_blocks(inode, &map, EROFS_GET_BLOCKS_RAW);
-		if (unlikely(err))
+		if (err)
 			goto err_out;
 
 		/* zero out the holed page */
-		if (unlikely(!(map.m_flags & EROFS_MAP_MAPPED))) {
+		if (!(map.m_flags & EROFS_MAP_MAPPED)) {
 			zero_user_segment(page, 0, PAGE_SIZE);
 			SetPageUptodate(page);
 
@@ -315,7 +315,7 @@ static inline struct bio *erofs_read_raw_page(struct bio *bio,
 submit_bio_out:
 		__submit_bio(bio, REQ_OP_READ, 0);
 
-	return unlikely(err) ? ERR_PTR(err) : NULL;
+	return err ? ERR_PTR(err) : NULL;
 }
 
 /*
@@ -377,7 +377,7 @@ static int erofs_raw_access_readpages(struct file *filp,
 	DBG_BUGON(!list_empty(pages));
 
 	/* the rare case (end in gaps) */
-	if (unlikely(bio))
+	if (bio)
 		__submit_bio(bio, REQ_OP_READ, 0);
 	return 0;
 }
diff --git a/fs/erofs/decompressor.c b/fs/erofs/decompressor.c
index 5f4b7f302863..df349888f911 100644
--- a/fs/erofs/decompressor.c
+++ b/fs/erofs/decompressor.c
@@ -78,7 +78,7 @@ static int lz4_prepare_destpages(struct z_erofs_decompress_req *rq,
 			get_page(victim);
 		} else {
 			victim = erofs_allocpage(pagepool, GFP_KERNEL, false);
-			if (unlikely(!victim))
+			if (!victim)
 				return -ENOMEM;
 			victim->mapping = Z_EROFS_MAPPING_STAGING;
 		}
diff --git a/fs/erofs/dir.c b/fs/erofs/dir.c
index 1976e60e5174..6a5b43f7fb29 100644
--- a/fs/erofs/dir.c
+++ b/fs/erofs/dir.c
@@ -45,8 +45,8 @@ static int erofs_fill_dentries(struct inode *dir, struct dir_context *ctx,
 			de_namelen = le16_to_cpu(de[1].nameoff) - nameoff;
 
 		/* a corrupted entry is found */
-		if (unlikely(nameoff + de_namelen > maxsize ||
-			     de_namelen > EROFS_NAME_LEN)) {
+		if (nameoff + de_namelen > maxsize ||
+		    de_namelen > EROFS_NAME_LEN) {
 			errln("bogus dirent @ nid %llu", EROFS_V(dir)->nid);
 			DBG_BUGON(1);
 			return -EFSCORRUPTED;
@@ -94,8 +94,8 @@ static int erofs_readdir(struct file *f, struct dir_context *ctx)
 
 		nameoff = le16_to_cpu(de->nameoff);
 
-		if (unlikely(nameoff < sizeof(struct erofs_dirent) ||
-			     nameoff >= PAGE_SIZE)) {
+		if (nameoff < sizeof(struct erofs_dirent) ||
+		    nameoff >= PAGE_SIZE) {
 			errln("%s, invalid de[0].nameoff %u @ nid %llu",
 			      __func__, nameoff, EROFS_V(dir)->nid);
 			err = -EFSCORRUPTED;
@@ -106,11 +106,11 @@ static int erofs_readdir(struct file *f, struct dir_context *ctx)
 				dirsize - ctx->pos + ofs, PAGE_SIZE);
 
 		/* search dirents at the arbitrary position */
-		if (unlikely(initial)) {
+		if (initial) {
 			initial = false;
 
 			ofs = roundup(ofs, sizeof(struct erofs_dirent));
-			if (unlikely(ofs >= nameoff))
+			if (ofs >= nameoff)
 				goto skip_this;
 		}
 
@@ -123,7 +123,7 @@ static int erofs_readdir(struct file *f, struct dir_context *ctx)
 
 		ctx->pos = blknr_to_addr(i) + ofs;
 
-		if (unlikely(err))
+		if (err)
 			break;
 		++i;
 		ofs = 0;
diff --git a/fs/erofs/inode.c b/fs/erofs/inode.c
index cf31554075c9..871a6d8ed6f9 100644
--- a/fs/erofs/inode.c
+++ b/fs/erofs/inode.c
@@ -18,7 +18,7 @@ static int read_inode(struct inode *inode, void *data)
 
 	vi->datamode = __inode_data_mapping(advise);
 
-	if (unlikely(vi->datamode >= EROFS_INODE_LAYOUT_MAX)) {
+	if (vi->datamode >= EROFS_INODE_LAYOUT_MAX) {
 		errln("unsupported data mapping %u of nid %llu",
 		      vi->datamode, vi->nid);
 		DBG_BUGON(1);
@@ -133,13 +133,13 @@ static int fill_inline_data(struct inode *inode, void *data,
 	if (S_ISLNK(inode->i_mode) && inode->i_size < PAGE_SIZE) {
 		char *lnk = erofs_kmalloc(sbi, inode->i_size + 1, GFP_KERNEL);
 
-		if (unlikely(!lnk))
+		if (!lnk)
 			return -ENOMEM;
 
 		m_pofs += vi->inode_isize + vi->xattr_isize;
 
 		/* inline symlink data shouldn't across page boundary as well */
-		if (unlikely(m_pofs + inode->i_size > PAGE_SIZE)) {
+		if (m_pofs + inode->i_size > PAGE_SIZE) {
 			kfree(lnk);
 			errln("inline data cross block boundary @ nid %llu",
 			      vi->nid);
@@ -268,7 +268,7 @@ struct inode *erofs_iget(struct super_block *sb,
 {
 	struct inode *inode = erofs_iget_locked(sb, nid);
 
-	if (unlikely(!inode))
+	if (!inode)
 		return ERR_PTR(-ENOMEM);
 
 	if (inode->i_state & I_NEW) {
@@ -278,7 +278,7 @@ struct inode *erofs_iget(struct super_block *sb,
 		vi->nid = nid;
 
 		err = fill_inode(inode, isdir);
-		if (likely(!err))
+		if (!err)
 			unlock_new_inode(inode);
 		else {
 			iget_failed(inode);
diff --git a/fs/erofs/internal.h b/fs/erofs/internal.h
index 620b73fcc416..141ea424587d 100644
--- a/fs/erofs/internal.h
+++ b/fs/erofs/internal.h
@@ -424,7 +424,7 @@ static inline struct bio *erofs_grab_bio(struct super_block *sb,
 	do {
 		if (nr_pages == 1) {
 			bio = bio_alloc(gfp | (nofail ? __GFP_NOFAIL : 0), 1);
-			if (unlikely(!bio)) {
+			if (!bio) {
 				DBG_BUGON(nofail);
 				return ERR_PTR(-ENOMEM);
 			}
@@ -432,7 +432,7 @@ static inline struct bio *erofs_grab_bio(struct super_block *sb,
 		}
 		bio = bio_alloc(gfp, nr_pages);
 		nr_pages /= 2;
-	} while (unlikely(!bio));
+	} while (!bio);
 
 	bio->bi_end_io = endio;
 	bio_set_dev(bio, sb->s_bdev);
diff --git a/fs/erofs/namei.c b/fs/erofs/namei.c
index 8832b5d95d91..c1068ad0535e 100644
--- a/fs/erofs/namei.c
+++ b/fs/erofs/namei.c
@@ -64,7 +64,7 @@ static struct erofs_dirent *find_target_dirent(struct erofs_qstr *name,
 		unsigned int matched = min(startprfx, endprfx);
 		struct erofs_qstr dname = {
 			.name = data + nameoff,
-			.end = unlikely(mid >= ndirents - 1) ?
+			.end = mid >= ndirents - 1 ?
 				data + dirblksize :
 				data + nameoff_from_disk(de[mid + 1].nameoff,
 							 dirblksize)
@@ -73,7 +73,7 @@ static struct erofs_dirent *find_target_dirent(struct erofs_qstr *name,
 		/* string comparison without already matched prefix */
 		int ret = dirnamecmp(name, &dname, &matched);
 
-		if (unlikely(!ret)) {
+		if (!ret) {
 			return de + mid;
 		} else if (ret > 0) {
 			head = mid + 1;
@@ -113,7 +113,7 @@ static struct page *find_target_block_classic(struct inode *dir,
 			unsigned int matched;
 			struct erofs_qstr dname;
 
-			if (unlikely(!ndirents)) {
+			if (!ndirents) {
 				kunmap_atomic(de);
 				put_page(page);
 				errln("corrupted dir block %d @ nid %llu",
@@ -137,7 +137,7 @@ static struct page *find_target_block_classic(struct inode *dir,
 			diff = dirnamecmp(name, &dname, &matched);
 			kunmap_atomic(de);
 
-			if (unlikely(!diff)) {
+			if (!diff) {
 				*_ndirents = 0;
 				goto out;
 			} else if (diff > 0) {
@@ -174,7 +174,7 @@ int erofs_namei(struct inode *dir,
 	struct erofs_dirent *de;
 	struct erofs_qstr qn;
 
-	if (unlikely(!dir->i_size))
+	if (!dir->i_size)
 		return -ENOENT;
 
 	qn.name = name->name;
@@ -221,7 +221,7 @@ static struct dentry *erofs_lookup(struct inode *dir,
 	trace_erofs_lookup(dir, dentry, flags);
 
 	/* file name exceeds fs limit */
-	if (unlikely(dentry->d_name.len > EROFS_NAME_LEN))
+	if (dentry->d_name.len > EROFS_NAME_LEN)
 		return ERR_PTR(-ENAMETOOLONG);
 
 	/* false uninitialized warnings on gcc 4.8.x */
@@ -230,7 +230,7 @@ static struct dentry *erofs_lookup(struct inode *dir,
 	if (err == -ENOENT) {
 		/* negative dentry */
 		inode = NULL;
-	} else if (unlikely(err)) {
+	} else if (err) {
 		inode = ERR_PTR(err);
 	} else {
 		debugln("%s, %s (nid %llu) found, d_type %u", __func__,
diff --git a/fs/erofs/super.c b/fs/erofs/super.c
index 556aae5426d6..0c412de33315 100644
--- a/fs/erofs/super.c
+++ b/fs/erofs/super.c
@@ -92,7 +92,7 @@ static int superblock_read(struct super_block *sb)
 
 	blkszbits = layout->blkszbits;
 	/* 9(512 bytes) + LOG_SECTORS_PER_BLOCK == LOG_BLOCK_SIZE */
-	if (unlikely(blkszbits != LOG_BLOCK_SIZE)) {
+	if (blkszbits != LOG_BLOCK_SIZE) {
 		errln("blksize %u isn't supported on this platform",
 		      1 << blkszbits);
 		goto out;
@@ -364,7 +364,7 @@ static int erofs_init_managed_cache(struct super_block *sb)
 	struct erofs_sb_info *const sbi = EROFS_SB(sb);
 	struct inode *const inode = new_inode(sb);
 
-	if (unlikely(!inode))
+	if (!inode)
 		return -ENOMEM;
 
 	set_nlink(inode, 1);
@@ -391,13 +391,13 @@ static int erofs_fill_super(struct super_block *sb, void *data, int silent)
 
 	sb->s_magic = EROFS_SUPER_MAGIC;
 
-	if (unlikely(!sb_set_blocksize(sb, EROFS_BLKSIZ))) {
+	if (!sb_set_blocksize(sb, EROFS_BLKSIZ)) {
 		errln("failed to set erofs blksize");
 		return -EINVAL;
 	}
 
 	sbi = kzalloc(sizeof(*sbi), GFP_KERNEL);
-	if (unlikely(!sbi))
+	if (!sbi)
 		return -ENOMEM;
 
 	sb->s_fs_info = sbi;
@@ -418,7 +418,7 @@ static int erofs_fill_super(struct super_block *sb, void *data, int silent)
 	default_options(sbi);
 
 	err = parse_options(sb, data);
-	if (unlikely(err))
+	if (err)
 		return err;
 
 	if (!silent)
@@ -438,7 +438,7 @@ static int erofs_fill_super(struct super_block *sb, void *data, int silent)
 	if (IS_ERR(inode))
 		return PTR_ERR(inode);
 
-	if (unlikely(!S_ISDIR(inode->i_mode))) {
+	if (!S_ISDIR(inode->i_mode)) {
 		errln("rootino(nid %llu) is not a directory(i_mode %o)",
 		      ROOT_NID(sbi), inode->i_mode);
 		iput(inode);
@@ -446,13 +446,13 @@ static int erofs_fill_super(struct super_block *sb, void *data, int silent)
 	}
 
 	sb->s_root = d_make_root(inode);
-	if (unlikely(!sb->s_root))
+	if (!sb->s_root)
 		return -ENOMEM;
 
 	erofs_shrinker_register(sb);
 	/* sb->s_umount is already locked, SB_ACTIVE and SB_BORN are not set */
 	err = erofs_init_managed_cache(sb);
-	if (unlikely(err))
+	if (err)
 		return err;
 
 	if (!silent)
diff --git a/fs/erofs/utils.c b/fs/erofs/utils.c
index 1dd041aa0f5a..d92b3e753a6f 100644
--- a/fs/erofs/utils.c
+++ b/fs/erofs/utils.c
@@ -46,14 +46,14 @@ static int erofs_workgroup_get(struct erofs_workgroup *grp)
 
 repeat:
 	o = erofs_wait_on_workgroup_freezed(grp);
-	if (unlikely(o <= 0))
+	if (o <= 0)
 		return -1;
 
-	if (unlikely(atomic_cmpxchg(&grp->refcount, o, o + 1) != o))
+	if (atomic_cmpxchg(&grp->refcount, o, o + 1) != o)
 		goto repeat;
 
 	/* decrease refcount paired by erofs_workgroup_put */
-	if (unlikely(o == 1))
+	if (o == 1)
 		atomic_long_dec(&erofs_global_shrink_cnt);
 	return 0;
 }
@@ -91,7 +91,7 @@ int erofs_register_workgroup(struct super_block *sb,
 	int err;
 
 	/* grp shouldn't be broken or used before */
-	if (unlikely(atomic_read(&grp->refcount) != 1)) {
+	if (atomic_read(&grp->refcount) != 1) {
 		DBG_BUGON(1);
 		return -EINVAL;
 	}
@@ -113,7 +113,7 @@ int erofs_register_workgroup(struct super_block *sb,
 	__erofs_workgroup_get(grp);
 
 	err = radix_tree_insert(&sbi->workstn_tree, grp->index, grp);
-	if (unlikely(err))
+	if (err)
 		/*
 		 * it's safe to decrease since the workgroup isn't visible
 		 * and refcount >= 2 (cannot be freezed).
@@ -212,7 +212,7 @@ static unsigned long erofs_shrink_workstation(struct erofs_sb_info *sbi,
 			continue;
 
 		++freed;
-		if (unlikely(!--nr_shrink))
+		if (!--nr_shrink)
 			break;
 	}
 	xa_unlock(&sbi->workstn_tree);
diff --git a/fs/erofs/xattr.c b/fs/erofs/xattr.c
index 7ef8d4bb45cd..620cbc15f4d0 100644
--- a/fs/erofs/xattr.c
+++ b/fs/erofs/xattr.c
@@ -19,7 +19,7 @@ struct xattr_iter {
 static inline void xattr_iter_end(struct xattr_iter *it, bool atomic)
 {
 	/* the only user of kunmap() is 'init_inode_xattrs' */
-	if (unlikely(!atomic))
+	if (!atomic)
 		kunmap(it->page);
 	else
 		kunmap_atomic(it->kaddr);
@@ -72,7 +72,7 @@ static int init_inode_xattrs(struct inode *inode)
 		ret = -EOPNOTSUPP;
 		goto out_unlock;
 	} else if (vi->xattr_isize < sizeof(struct erofs_xattr_ibody_header)) {
-		if (unlikely(vi->xattr_isize)) {
+		if (vi->xattr_isize) {
 			errln("bogus xattr ibody @ nid %llu", vi->nid);
 			DBG_BUGON(1);
 			ret = -EFSCORRUPTED;
@@ -112,7 +112,7 @@ static int init_inode_xattrs(struct inode *inode)
 	it.ofs += sizeof(struct erofs_xattr_ibody_header);
 
 	for (i = 0; i < vi->xattr_shared_count; ++i) {
-		if (unlikely(it.ofs >= EROFS_BLKSIZ)) {
+		if (it.ofs >= EROFS_BLKSIZ) {
 			/* cannot be unaligned */
 			DBG_BUGON(it.ofs != EROFS_BLKSIZ);
 			xattr_iter_end(&it, atomic_map);
@@ -189,7 +189,7 @@ static int inline_xattr_iter_begin(struct xattr_iter *it,
 	unsigned int xattr_header_sz, inline_xattr_ofs;
 
 	xattr_header_sz = inlinexattr_header_size(inode);
-	if (unlikely(xattr_header_sz >= vi->xattr_isize)) {
+	if (xattr_header_sz >= vi->xattr_isize) {
 		DBG_BUGON(xattr_header_sz > vi->xattr_isize);
 		return -ENOATTR;
 	}
@@ -234,7 +234,7 @@ static int xattr_foreach(struct xattr_iter *it,
 		unsigned int entry_sz = erofs_xattr_entry_size(&entry);
 
 		/* xattr on-disk corruption: xattr entry beyond xattr_isize */
-		if (unlikely(*tlimit < entry_sz)) {
+		if (*tlimit < entry_sz) {
 			DBG_BUGON(1);
 			return -EFSCORRUPTED;
 		}
@@ -436,7 +436,7 @@ int erofs_getxattr(struct inode *inode, int index,
 	int ret;
 	struct getxattr_iter it;
 
-	if (unlikely(!name))
+	if (!name)
 		return -EINVAL;
 
 	ret = init_inode_xattrs(inode);
diff --git a/fs/erofs/zdata.c b/fs/erofs/zdata.c
index b32ad585237c..653bde0a619a 100644
--- a/fs/erofs/zdata.c
+++ b/fs/erofs/zdata.c
@@ -230,7 +230,7 @@ int erofs_try_to_free_all_cached_pages(struct erofs_sb_info *sbi,
 		if (!trylock_page(page))
 			return -EBUSY;
 
-		if (unlikely(page->mapping != mapping))
+		if (page->mapping != mapping)
 			continue;
 
 		/* barrier is implied in the following 'unlock_page' */
@@ -358,7 +358,7 @@ static struct z_erofs_collection *cllookup(struct z_erofs_collector *clt,
 	}
 
 	cl = z_erofs_primarycollection(pcl);
-	if (unlikely(cl->pageofs != (map->m_la & ~PAGE_MASK))) {
+	if (cl->pageofs != (map->m_la & ~PAGE_MASK)) {
 		DBG_BUGON(1);
 		erofs_workgroup_put(grp);
 		return ERR_PTR(-EFSCORRUPTED);
@@ -406,7 +406,7 @@ static struct z_erofs_collection *clregister(struct z_erofs_collector *clt,
 
 	/* no available workgroup, let's allocate one */
 	pcl = kmem_cache_alloc(pcluster_cachep, GFP_NOFS);
-	if (unlikely(!pcl))
+	if (!pcl)
 		return ERR_PTR(-ENOMEM);
 
 	init_always(pcl);
@@ -474,7 +474,7 @@ static int z_erofs_collector_begin(struct z_erofs_collector *clt,
 	if (!cl) {
 		cl = clregister(clt, inode, map);
 
-		if (unlikely(cl == ERR_PTR(-EAGAIN)))
+		if (cl == ERR_PTR(-EAGAIN))
 			goto repeat;
 	}
 
@@ -607,15 +607,15 @@ static int z_erofs_do_read_page(struct z_erofs_decompress_frontend *fe,
 	map->m_la = offset + cur;
 	map->m_llen = 0;
 	err = z_erofs_map_blocks_iter(inode, map, 0);
-	if (unlikely(err))
+	if (err)
 		goto err_out;
 
 restart_now:
-	if (unlikely(!(map->m_flags & EROFS_MAP_MAPPED)))
+	if (!(map->m_flags & EROFS_MAP_MAPPED))
 		goto hitted;
 
 	err = z_erofs_collector_begin(clt, inode, map);
-	if (unlikely(err))
+	if (err)
 		goto err_out;
 
 	/* preload all compressed pages (maybe downgrade role if necessary) */
@@ -630,7 +630,7 @@ static int z_erofs_do_read_page(struct z_erofs_decompress_frontend *fe,
 	tight &= (clt->mode >= COLLECT_PRIMARY_HOOKED);
 hitted:
 	cur = end - min_t(unsigned int, offset + end - map->m_la, end);
-	if (unlikely(!(map->m_flags & EROFS_MAP_MAPPED))) {
+	if (!(map->m_flags & EROFS_MAP_MAPPED)) {
 		zero_user_segment(page, cur, end);
 		goto next_part;
 	}
@@ -653,11 +653,11 @@ static int z_erofs_do_read_page(struct z_erofs_decompress_frontend *fe,
 
 		err = z_erofs_attach_page(clt, newpage,
 					  Z_EROFS_PAGE_TYPE_EXCLUSIVE);
-		if (likely(!err))
+		if (!err)
 			goto retry;
 	}
 
-	if (unlikely(err))
+	if (err)
 		goto err_out;
 
 	index = page->index - (map->m_la >> PAGE_SHIFT);
@@ -723,7 +723,7 @@ static inline void z_erofs_vle_read_endio(struct bio *bio)
 		DBG_BUGON(PageUptodate(page));
 		DBG_BUGON(!page->mapping);
 
-		if (unlikely(!sbi && !z_erofs_page_is_staging(page))) {
+		if (!sbi && !z_erofs_page_is_staging(page)) {
 			sbi = EROFS_SB(page->mapping->host->i_sb);
 
 			if (time_to_inject(sbi, FAULT_READ_IO)) {
@@ -736,7 +736,7 @@ static inline void z_erofs_vle_read_endio(struct bio *bio)
 		if (sbi)
 			cachemngd = erofs_page_is_managed(sbi, page);
 
-		if (unlikely(err))
+		if (err)
 			SetPageError(page);
 		else if (cachemngd)
 			SetPageUptodate(page);
@@ -772,7 +772,7 @@ static int z_erofs_decompress_pcluster(struct super_block *sb,
 	mutex_lock(&cl->lock);
 	nr_pages = cl->nr_pages;
 
-	if (likely(nr_pages <= Z_EROFS_VMAP_ONSTACK_PAGES)) {
+	if (nr_pages <= Z_EROFS_VMAP_ONSTACK_PAGES) {
 		pages = pages_onstack;
 	} else if (nr_pages <= Z_EROFS_VMAP_GLOBAL_PAGES &&
 		   mutex_trylock(&z_pagemap_global_lock)) {
@@ -787,7 +787,7 @@ static int z_erofs_decompress_pcluster(struct super_block *sb,
 				       gfp_flags);
 
 		/* fallback to global pagemap for the lowmem scenario */
-		if (unlikely(!pages)) {
+		if (!pages) {
 			mutex_lock(&z_pagemap_global_lock);
 			pages = z_pagemap_global;
 		}
@@ -823,7 +823,7 @@ static int z_erofs_decompress_pcluster(struct super_block *sb,
 		 * currently EROFS doesn't support multiref(dedup),
 		 * so here erroring out one multiref page.
 		 */
-		if (unlikely(pages[pagenr])) {
+		if (pages[pagenr]) {
 			DBG_BUGON(1);
 			SetPageError(pages[pagenr]);
 			z_erofs_onlinepage_endio(pages[pagenr]);
@@ -847,7 +847,7 @@ static int z_erofs_decompress_pcluster(struct super_block *sb,
 
 		if (!z_erofs_page_is_staging(page)) {
 			if (erofs_page_is_managed(sbi, page)) {
-				if (unlikely(!PageUptodate(page)))
+				if (!PageUptodate(page))
 					err = -EIO;
 				continue;
 			}
@@ -859,7 +859,7 @@ static int z_erofs_decompress_pcluster(struct super_block *sb,
 			pagenr = z_erofs_onlinepage_index(page);
 
 			DBG_BUGON(pagenr >= nr_pages);
-			if (unlikely(pages[pagenr])) {
+			if (pages[pagenr]) {
 				DBG_BUGON(1);
 				SetPageError(pages[pagenr]);
 				z_erofs_onlinepage_endio(pages[pagenr]);
@@ -871,13 +871,13 @@ static int z_erofs_decompress_pcluster(struct super_block *sb,
 		}
 
 		/* PG_error needs checking for inplaced and staging pages */
-		if (unlikely(PageError(page))) {
+		if (PageError(page)) {
 			DBG_BUGON(PageUptodate(page));
 			err = -EIO;
 		}
 	}
 
-	if (unlikely(err))
+	if (err)
 		goto out;
 
 	llen = pcl->length >> Z_EROFS_PCLUSTER_LENGTH_BIT;
@@ -926,7 +926,7 @@ static int z_erofs_decompress_pcluster(struct super_block *sb,
 		if (z_erofs_put_stagingpage(pagepool, page))
 			continue;
 
-		if (unlikely(err < 0))
+		if (err < 0)
 			SetPageError(page);
 
 		z_erofs_onlinepage_endio(page);
@@ -934,7 +934,7 @@ static int z_erofs_decompress_pcluster(struct super_block *sb,
 
 	if (pages == z_pagemap_global)
 		mutex_unlock(&z_pagemap_global_lock);
-	else if (unlikely(pages != pages_onstack))
+	else if (pages != pages_onstack)
 		kvfree(pages);
 
 	cl->nr_pages = 0;
@@ -1212,7 +1212,7 @@ static bool z_erofs_vle_submit_all(struct super_block *sb,
 	bool force_submit = false;
 	unsigned int nr_bios;
 
-	if (unlikely(owned_head == Z_EROFS_PCLUSTER_TAIL))
+	if (owned_head == Z_EROFS_PCLUSTER_TAIL)
 		return false;
 
 	force_submit = false;
diff --git a/fs/erofs/zmap.c b/fs/erofs/zmap.c
index 4dc9cec01297..850e0e3d57a8 100644
--- a/fs/erofs/zmap.c
+++ b/fs/erofs/zmap.c
@@ -348,7 +348,7 @@ static int vle_extent_lookback(struct z_erofs_maprecorder *m,
 
 	switch (m->type) {
 	case Z_EROFS_VLE_CLUSTER_TYPE_NONHEAD:
-		if (unlikely(!m->delta[0])) {
+		if (!m->delta[0]) {
 			errln("invalid lookback distance 0 at nid %llu",
 			      vi->nid);
 			DBG_BUGON(1);
@@ -386,7 +386,7 @@ int z_erofs_map_blocks_iter(struct inode *inode,
 	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)) {
+	if (map->m_la >= inode->i_size) {
 		map->m_llen = map->m_la + 1 - inode->i_size;
 		map->m_la = inode->i_size;
 		map->m_flags = 0;
@@ -420,7 +420,7 @@ int z_erofs_map_blocks_iter(struct inode *inode,
 			break;
 		}
 		/* m.lcn should be >= 1 if endoff < m.clusterofs */
-		if (unlikely(!m.lcn)) {
+		if (!m.lcn) {
 			errln("invalid logical cluster 0 at nid %llu",
 			      vi->nid);
 			err = -EFSCORRUPTED;
@@ -433,7 +433,7 @@ int z_erofs_map_blocks_iter(struct inode *inode,
 	case Z_EROFS_VLE_CLUSTER_TYPE_NONHEAD:
 		/* get the correspoinding first chunk */
 		err = vle_extent_lookback(&m, m.delta[0]);
-		if (unlikely(err))
+		if (err)
 			goto unmap_out;
 		break;
 	default:
diff --git a/fs/erofs/zpvec.h b/fs/erofs/zpvec.h
index bd3cee16491c..58556903aa94 100644
--- a/fs/erofs/zpvec.h
+++ b/fs/erofs/zpvec.h
@@ -111,11 +111,11 @@ static inline bool z_erofs_pagevec_enqueue(struct z_erofs_pagevec_ctor *ctor,
 					   bool *occupied)
 {
 	*occupied = false;
-	if (unlikely(!ctor->next && type))
+	if (!ctor->next && type)
 		if (ctor->index + 1 == ctor->nr)
 			return false;
 
-	if (unlikely(ctor->index >= ctor->nr))
+	if (ctor->index >= ctor->nr)
 		z_erofs_pagevec_ctor_pagedown(ctor, false);
 
 	/* exclusive page type must be 0 */
@@ -137,7 +137,7 @@ z_erofs_pagevec_dequeue(struct z_erofs_pagevec_ctor *ctor,
 {
 	erofs_vtptr_t t;
 
-	if (unlikely(ctor->index >= ctor->nr)) {
+	if (ctor->index >= ctor->nr) {
 		DBG_BUGON(!ctor->next);
 		z_erofs_pagevec_ctor_pagedown(ctor, true);
 	}
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* [PATCH v2 7/7] erofs: reduntant assignment in __erofs_get_meta_page()
  2019-08-30  3:00 [PATCH v2 1/7] erofs: on-disk format should have explicitly assigned numbers Gao Xiang
                   ` (4 preceding siblings ...)
  2019-08-30  3:00 ` [PATCH v2 6/7] erofs: remove all likely/unlikely annotations Gao Xiang
@ 2019-08-30  3:00 ` Gao Xiang
  2019-08-30  3:28 ` [PATCH v2 1/7] erofs: on-disk format should have explicitly assigned numbers Chao Yu
  6 siblings, 0 replies; 38+ messages in thread
From: Gao Xiang @ 2019-08-30  3:00 UTC (permalink / raw)
  To: Chao Yu, Dan Carpenter, Christoph Hellwig, Joe Perches,
	Greg Kroah-Hartman, devel
  Cc: linux-erofs, LKML, weidu.du, Miao Xie

As Joe Perches suggested [1],
 		err = bio_add_page(bio, page, PAGE_SIZE, 0);
-		if (unlikely(err != PAGE_SIZE)) {
+		if (err != PAGE_SIZE) {
 			err = -EFAULT;
 			goto err_out;
 		}

The initial assignment to err is odd as it's not
actually an error value -E<FOO> but a int size
from a unsigned int len.

Here the return is either 0 or PAGE_SIZE.

This would be more legible to me as:

		if (bio_add_page(bio, page, PAGE_SIZE, 0) != PAGE_SIZE) {
			err = -EFAULT;
			goto err_out;
		}

[1] https://lore.kernel.org/r/74c4784319b40deabfbaea92468f7e3ef44f1c96.camel@perches.com/
Reported-by: Joe Perches <joe@perches.com>
Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>
---
v2: no change, just resend in case of dependency problem;

 fs/erofs/data.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/fs/erofs/data.c b/fs/erofs/data.c
index 0f2f1a839372..0983807737fd 100644
--- a/fs/erofs/data.c
+++ b/fs/erofs/data.c
@@ -69,8 +69,7 @@ struct page *__erofs_get_meta_page(struct super_block *sb,
 			goto err_out;
 		}
 
-		err = bio_add_page(bio, page, PAGE_SIZE, 0);
-		if (err != PAGE_SIZE) {
+		if (bio_add_page(bio, page, PAGE_SIZE, 0) != PAGE_SIZE) {
 			err = -EFAULT;
 			goto err_out;
 		}
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* Re: [PATCH v2 2/7] erofs: some marcos are much more readable as a function
  2019-08-30  3:00 ` [PATCH v2 2/7] erofs: some marcos are much more readable as a function Gao Xiang
@ 2019-08-30  3:16   ` Joe Perches
  2019-08-30  3:20     ` Gao Xiang
  2019-08-30 15:45     ` [PATCH v2 2/7] erofs: some marcos are much more readable as a function Christoph Hellwig
  0 siblings, 2 replies; 38+ messages in thread
From: Joe Perches @ 2019-08-30  3:16 UTC (permalink / raw)
  To: Gao Xiang, Chao Yu, Dan Carpenter, Christoph Hellwig,
	Greg Kroah-Hartman, devel
  Cc: linux-erofs, LKML, weidu.du, Miao Xie

On Fri, 2019-08-30 at 11:00 +0800, Gao Xiang wrote:
> As Christoph suggested [1], these marcos are much
> more readable as a function

s/marcos/macros/
.
[]
> diff --git a/fs/erofs/erofs_fs.h b/fs/erofs/erofs_fs.h
[]
> @@ -168,16 +168,24 @@ struct erofs_xattr_entry {
>  	char   e_name[0];       /* attribute name */
>  } __packed;
>  
> -#define ondisk_xattr_ibody_size(count)	({\
> -	u32 __count = le16_to_cpu(count); \
> -	((__count) == 0) ? 0 : \
> -	sizeof(struct erofs_xattr_ibody_header) + \
> -		sizeof(__u32) * ((__count) - 1); })
> +static inline unsigned int erofs_xattr_ibody_size(__le16 d_icount)
> +{
> +	unsigned int icount = le16_to_cpu(d_icount);
> +
> +	if (!icount)
> +		return 0;
> +
> +	return sizeof(struct erofs_xattr_ibody_header) +
> +		sizeof(__u32) * (icount - 1);

Maybe use struct_size()?

{
	struct erofs_xattr_ibody_header *ibh;
	unsigned int icount = le16_to_cpu(d_icount);

	if (!icount)
		return 0;

	return struct_size(ibh, h_shared_xattrs, icount - 1);
}


^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v2 2/7] erofs: some marcos are much more readable as a function
  2019-08-30  3:16   ` Joe Perches
@ 2019-08-30  3:20     ` Gao Xiang
  2019-08-30  3:36       ` [PATCH v3 1/7] erofs: on-disk format should have explicitly assigned numbers Gao Xiang
  2019-08-30 15:45     ` [PATCH v2 2/7] erofs: some marcos are much more readable as a function Christoph Hellwig
  1 sibling, 1 reply; 38+ messages in thread
From: Gao Xiang @ 2019-08-30  3:20 UTC (permalink / raw)
  To: Joe Perches
  Cc: devel, Greg Kroah-Hartman, Miao Xie, LKML, Christoph Hellwig,
	weidu.du, linux-erofs, Dan Carpenter

Hi Joe,

On Thu, Aug 29, 2019 at 08:16:27PM -0700, Joe Perches wrote:
> On Fri, 2019-08-30 at 11:00 +0800, Gao Xiang wrote:
> > As Christoph suggested [1], these marcos are much
> > more readable as a function
> 
> s/marcos/macros/
> .
> []
> > diff --git a/fs/erofs/erofs_fs.h b/fs/erofs/erofs_fs.h
> []
> > @@ -168,16 +168,24 @@ struct erofs_xattr_entry {
> >  	char   e_name[0];       /* attribute name */
> >  } __packed;
> >  
> > -#define ondisk_xattr_ibody_size(count)	({\
> > -	u32 __count = le16_to_cpu(count); \
> > -	((__count) == 0) ? 0 : \
> > -	sizeof(struct erofs_xattr_ibody_header) + \
> > -		sizeof(__u32) * ((__count) - 1); })
> > +static inline unsigned int erofs_xattr_ibody_size(__le16 d_icount)
> > +{
> > +	unsigned int icount = le16_to_cpu(d_icount);
> > +
> > +	if (!icount)
> > +		return 0;
> > +
> > +	return sizeof(struct erofs_xattr_ibody_header) +
> > +		sizeof(__u32) * (icount - 1);
> 
> Maybe use struct_size()?
> 
> {
> 	struct erofs_xattr_ibody_header *ibh;
> 	unsigned int icount = le16_to_cpu(d_icount);
> 
> 	if (!icount)
> 		return 0;
> 
> 	return struct_size(ibh, h_shared_xattrs, icount - 1);
> }

Okay, That is fine, will resend this patch.

Thanks,
Gao Xiang

> 

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v2 1/7] erofs: on-disk format should have explicitly assigned numbers
  2019-08-30  3:00 [PATCH v2 1/7] erofs: on-disk format should have explicitly assigned numbers Gao Xiang
                   ` (5 preceding siblings ...)
  2019-08-30  3:00 ` [PATCH v2 7/7] erofs: reduntant assignment in __erofs_get_meta_page() Gao Xiang
@ 2019-08-30  3:28 ` Chao Yu
  6 siblings, 0 replies; 38+ messages in thread
From: Chao Yu @ 2019-08-30  3:28 UTC (permalink / raw)
  To: Gao Xiang, Dan Carpenter, Christoph Hellwig, Joe Perches,
	Greg Kroah-Hartman, devel
  Cc: linux-erofs, LKML, weidu.du, Miao Xie

On 2019/8/30 11:00, Gao Xiang wrote:
> As Christoph claimed [1], on-disk format should have
> explicitly assigned numbers. I have to change it.
> 
> [1] https://lore.kernel.org/r/20190829095954.GB20598@infradead.org/
> Reported-by: Christoph Hellwig <hch@infradead.org>
> Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>

Reviewed-by: Chao Yu <yuchao0@huawei.com>

Thanks,

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v2 3/7] erofs: use a better form for complicated on-disk fields
  2019-08-30  3:00 ` [PATCH v2 3/7] erofs: use a better form for complicated on-disk fields Gao Xiang
@ 2019-08-30  3:29   ` Chao Yu
  0 siblings, 0 replies; 38+ messages in thread
From: Chao Yu @ 2019-08-30  3:29 UTC (permalink / raw)
  To: Gao Xiang, Dan Carpenter, Christoph Hellwig, Joe Perches,
	Greg Kroah-Hartman, devel
  Cc: linux-erofs, LKML, weidu.du, Miao Xie

On 2019/8/30 11:00, Gao Xiang wrote:
> As Joe Perches [1] suggested, let's use a better
> form to describe complicated on-disk fields.
> 
> p.s. it has different tab alignment looking between
>      the real file and patch file.
> p.p.s. due to changing a different form, some lines
>        have to exceed 80 characters.
> [1] https://lore.kernel.org/r/67d6efbbc9ac6db23215660cb970b7ef29dc0c1d.camel@perches.com/
> Reported-by: Joe Perches <joe@perches.com>
> Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>

Reviewed-by: Chao Yu <yuchao0@huawei.com>

Thanks,

^ permalink raw reply	[flat|nested] 38+ messages in thread

* [PATCH v3 1/7] erofs: on-disk format should have explicitly assigned numbers
  2019-08-30  3:20     ` Gao Xiang
@ 2019-08-30  3:36       ` Gao Xiang
  2019-08-30  3:36         ` [PATCH v3 2/7] erofs: some macros are much more readable as a function Gao Xiang
                           ` (6 more replies)
  0 siblings, 7 replies; 38+ messages in thread
From: Gao Xiang @ 2019-08-30  3:36 UTC (permalink / raw)
  To: Chao Yu, Dan Carpenter, Christoph Hellwig, Joe Perches,
	Greg Kroah-Hartman, devel
  Cc: linux-erofs, LKML, weidu.du, Miao Xie

As Christoph claimed [1], on-disk format should have
explicitly assigned numbers. I have to change it.

[1] https://lore.kernel.org/r/20190829095954.GB20598@infradead.org/
Reported-by: Christoph Hellwig <hch@infradead.org>
Reviewed-by: Chao Yu <yuchao0@huawei.com>
Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>
---
no change

 fs/erofs/erofs_fs.h | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/fs/erofs/erofs_fs.h b/fs/erofs/erofs_fs.h
index afa7d45ca958..2447ad4d0920 100644
--- a/fs/erofs/erofs_fs.h
+++ b/fs/erofs/erofs_fs.h
@@ -52,10 +52,10 @@ struct erofs_super_block {
  * 4~7 - reserved
  */
 enum {
-	EROFS_INODE_FLAT_PLAIN,
-	EROFS_INODE_FLAT_COMPRESSION_LEGACY,
-	EROFS_INODE_FLAT_INLINE,
-	EROFS_INODE_FLAT_COMPRESSION,
+	EROFS_INODE_FLAT_PLAIN			= 0,
+	EROFS_INODE_FLAT_COMPRESSION_LEGACY	= 1,
+	EROFS_INODE_FLAT_INLINE			= 2,
+	EROFS_INODE_FLAT_COMPRESSION		= 3,
 	EROFS_INODE_LAYOUT_MAX
 };
 
@@ -181,7 +181,7 @@ struct erofs_xattr_entry {
 
 /* available compression algorithm types */
 enum {
-	Z_EROFS_COMPRESSION_LZ4,
+	Z_EROFS_COMPRESSION_LZ4	= 0,
 	Z_EROFS_COMPRESSION_MAX
 };
 
@@ -239,10 +239,10 @@ struct z_erofs_map_header {
  *                (di_advise could be 0, 1 or 2)
  */
 enum {
-	Z_EROFS_VLE_CLUSTER_TYPE_PLAIN,
-	Z_EROFS_VLE_CLUSTER_TYPE_HEAD,
-	Z_EROFS_VLE_CLUSTER_TYPE_NONHEAD,
-	Z_EROFS_VLE_CLUSTER_TYPE_RESERVED,
+	Z_EROFS_VLE_CLUSTER_TYPE_PLAIN		= 0,
+	Z_EROFS_VLE_CLUSTER_TYPE_HEAD		= 1,
+	Z_EROFS_VLE_CLUSTER_TYPE_NONHEAD	= 2,
+	Z_EROFS_VLE_CLUSTER_TYPE_RESERVED	= 3,
 	Z_EROFS_VLE_CLUSTER_TYPE_MAX
 };
 
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* [PATCH v3 2/7] erofs: some macros are much more readable as a function
  2019-08-30  3:36       ` [PATCH v3 1/7] erofs: on-disk format should have explicitly assigned numbers Gao Xiang
@ 2019-08-30  3:36         ` Gao Xiang
  2019-08-30  3:38           ` [PATCH v4 " Gao Xiang
  2019-08-30  6:11           ` [PATCH v3 " Chao Yu
  2019-08-30  3:36         ` [PATCH v3 3/7] erofs: use a better form for complicated on-disk fields Gao Xiang
                           ` (5 subsequent siblings)
  6 siblings, 2 replies; 38+ messages in thread
From: Gao Xiang @ 2019-08-30  3:36 UTC (permalink / raw)
  To: Chao Yu, Dan Carpenter, Christoph Hellwig, Joe Perches,
	Greg Kroah-Hartman, devel
  Cc: linux-erofs, LKML, weidu.du, Miao Xie

As Christoph suggested [1], these marcos are much
more readable as a function.

[1] https://lore.kernel.org/r/20190829095954.GB20598@infradead.org/
Reported-by: Christoph Hellwig <hch@infradead.org>
Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>
---
v3: change as Joe suggested,
 https://lore.kernel.org/r/5b2ecf5cec1a6aa3834e9af41886a7fcb18ae86a.camel@perches.com/

 fs/erofs/erofs_fs.h | 24 ++++++++++++++++--------
 fs/erofs/inode.c    |  4 ++--
 fs/erofs/xattr.c    |  2 +-
 3 files changed, 19 insertions(+), 11 deletions(-)

diff --git a/fs/erofs/erofs_fs.h b/fs/erofs/erofs_fs.h
index 2447ad4d0920..0782ba9da623 100644
--- a/fs/erofs/erofs_fs.h
+++ b/fs/erofs/erofs_fs.h
@@ -168,16 +168,24 @@ struct erofs_xattr_entry {
 	char   e_name[0];       /* attribute name */
 } __packed;
 
-#define ondisk_xattr_ibody_size(count)	({\
-	u32 __count = le16_to_cpu(count); \
-	((__count) == 0) ? 0 : \
-	sizeof(struct erofs_xattr_ibody_header) + \
-		sizeof(__u32) * ((__count) - 1); })
+static inline unsigned int erofs_xattr_ibody_size(__le16 d_icount)
+{
+	struct erofs_xattr_ibody_header *ibh;
+	unsigned int icount = le16_to_cpu(d_icount);
+
+	if (!icount)
+		return 0;
+
+	return struct_size(ibh, h_shared_xattrs, icount - 1);
+}
 
 #define EROFS_XATTR_ALIGN(size) round_up(size, sizeof(struct erofs_xattr_entry))
-#define EROFS_XATTR_ENTRY_SIZE(entry) EROFS_XATTR_ALIGN( \
-	sizeof(struct erofs_xattr_entry) + \
-	(entry)->e_name_len + le16_to_cpu((entry)->e_value_size))
+
+static inline unsigned int erofs_xattr_entry_size(struct erofs_xattr_entry *e)
+{
+	return EROFS_XATTR_ALIGN(sizeof(struct erofs_xattr_entry) +
+				 e->e_name_len + le16_to_cpu(e->e_value_size));
+}
 
 /* available compression algorithm types */
 enum {
diff --git a/fs/erofs/inode.c b/fs/erofs/inode.c
index 80f4fe919ee7..cf31554075c9 100644
--- a/fs/erofs/inode.c
+++ b/fs/erofs/inode.c
@@ -29,7 +29,7 @@ static int read_inode(struct inode *inode, void *data)
 		struct erofs_inode_v2 *v2 = data;
 
 		vi->inode_isize = sizeof(struct erofs_inode_v2);
-		vi->xattr_isize = ondisk_xattr_ibody_size(v2->i_xattr_icount);
+		vi->xattr_isize = erofs_xattr_ibody_size(v2->i_xattr_icount);
 
 		inode->i_mode = le16_to_cpu(v2->i_mode);
 		if (S_ISREG(inode->i_mode) || S_ISDIR(inode->i_mode) ||
@@ -62,7 +62,7 @@ static int read_inode(struct inode *inode, void *data)
 		struct erofs_sb_info *sbi = EROFS_SB(inode->i_sb);
 
 		vi->inode_isize = sizeof(struct erofs_inode_v1);
-		vi->xattr_isize = ondisk_xattr_ibody_size(v1->i_xattr_icount);
+		vi->xattr_isize = erofs_xattr_ibody_size(v1->i_xattr_icount);
 
 		inode->i_mode = le16_to_cpu(v1->i_mode);
 		if (S_ISREG(inode->i_mode) || S_ISDIR(inode->i_mode) ||
diff --git a/fs/erofs/xattr.c b/fs/erofs/xattr.c
index a8286998a079..7ef8d4bb45cd 100644
--- a/fs/erofs/xattr.c
+++ b/fs/erofs/xattr.c
@@ -231,7 +231,7 @@ static int xattr_foreach(struct xattr_iter *it,
 	 */
 	entry = *(struct erofs_xattr_entry *)(it->kaddr + it->ofs);
 	if (tlimit) {
-		unsigned int entry_sz = EROFS_XATTR_ENTRY_SIZE(&entry);
+		unsigned int entry_sz = erofs_xattr_entry_size(&entry);
 
 		/* xattr on-disk corruption: xattr entry beyond xattr_isize */
 		if (unlikely(*tlimit < entry_sz)) {
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* [PATCH v3 3/7] erofs: use a better form for complicated on-disk fields
  2019-08-30  3:36       ` [PATCH v3 1/7] erofs: on-disk format should have explicitly assigned numbers Gao Xiang
  2019-08-30  3:36         ` [PATCH v3 2/7] erofs: some macros are much more readable as a function Gao Xiang
@ 2019-08-30  3:36         ` Gao Xiang
  2019-08-30  3:36         ` [PATCH v3 4/7] erofs: kill __packed for on-disk structures Gao Xiang
                           ` (4 subsequent siblings)
  6 siblings, 0 replies; 38+ messages in thread
From: Gao Xiang @ 2019-08-30  3:36 UTC (permalink / raw)
  To: Chao Yu, Dan Carpenter, Christoph Hellwig, Joe Perches,
	Greg Kroah-Hartman, devel
  Cc: linux-erofs, LKML, weidu.du, Miao Xie

As Joe Perches [1] suggested, let's use a better
form to describe complicated on-disk fields.

p.s. it has different tab alignment looking between
     the real file and patch file.
p.p.s. due to changing a different form, some lines
       have to exceed 80 characters.
[1] https://lore.kernel.org/r/67d6efbbc9ac6db23215660cb970b7ef29dc0c1d.camel@perches.com/
Reported-by: Joe Perches <joe@perches.com>
Reviewed-by: Chao Yu <yuchao0@huawei.com>
Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>
---
no change.

 fs/erofs/erofs_fs.h | 100 ++++++++++++++++++++++----------------------
 1 file changed, 50 insertions(+), 50 deletions(-)

diff --git a/fs/erofs/erofs_fs.h b/fs/erofs/erofs_fs.h
index 0782ba9da623..fbdaf873d736 100644
--- a/fs/erofs/erofs_fs.h
+++ b/fs/erofs/erofs_fs.h
@@ -17,27 +17,27 @@
 #define EROFS_REQUIREMENT_LZ4_0PADDING	0x00000001
 #define EROFS_ALL_REQUIREMENTS		EROFS_REQUIREMENT_LZ4_0PADDING
 
-struct erofs_super_block {
-/*  0 */__le32 magic;           /* in the little endian */
-/*  4 */__le32 checksum;        /* crc32c(super_block) */
-/*  8 */__le32 features;        /* (aka. feature_compat) */
-/* 12 */__u8 blkszbits;         /* support block_size == PAGE_SIZE only */
-/* 13 */__u8 reserved;
-
-/* 14 */__le16 root_nid;
-/* 16 */__le64 inos;            /* total valid ino # (== f_files - f_favail) */
-
-/* 24 */__le64 build_time;      /* inode v1 time derivation */
-/* 32 */__le32 build_time_nsec;
-/* 36 */__le32 blocks;          /* used for statfs */
-/* 40 */__le32 meta_blkaddr;
-/* 44 */__le32 xattr_blkaddr;
-/* 48 */__u8 uuid[16];          /* 128-bit uuid for volume */
-/* 64 */__u8 volume_name[16];   /* volume name */
-/* 80 */__le32 requirements;    /* (aka. feature_incompat) */
-
-/* 84 */__u8 reserved2[44];
-} __packed;                     /* 128 bytes */
+struct erofs_super_block {	/* off description */
+	__le32 magic;		/*  0  file system magic number */
+	__le32 checksum;	/*  4  crc32c(super_block) */
+	__le32 features;	/*  8  (aka. feature_compat) */
+	__u8 blkszbits;		/* 12  support PAGE_SIZE only currently */
+	__u8 reserved;		/* 13  */
+
+	__le16 root_nid;	/* 14  nid of root directory */
+	__le64 inos;		/* 16  total valid ino # (== f_files - f_favail) */
+
+	__le64 build_time;	/* 24  inode v1 time derivation */
+	__le32 build_time_nsec;	/* 32  inode v1 time derivation in nano scale */
+	__le32 blocks;		/* 36  used for statfs */
+	__le32 meta_blkaddr;	/* 40  start block address of metadata area */
+	__le32 xattr_blkaddr;	/* 44  start block address of shared xattr area */
+	__u8 uuid[16];		/* 48  128-bit uuid for volume */
+	__u8 volume_name[16];	/* 64  volume name */
+	__le32 requirements;	/* 80  (aka. feature_incompat) */
+
+	__u8 reserved2[44];	/* 84 */
+} __packed;			/* 128 bytes */
 
 /*
  * erofs inode data mapping:
@@ -73,16 +73,16 @@ static inline bool erofs_inode_is_data_compressed(unsigned int datamode)
 #define EROFS_I_VERSION_BIT             0
 #define EROFS_I_DATA_MAPPING_BIT        1
 
-struct erofs_inode_v1 {
-/*  0 */__le16 i_advise;
+struct erofs_inode_v1 {		/* off description */
+	__le16 i_advise;	/*  0  file hints */
 
 /* 1 header + n-1 * 4 bytes inline xattr to keep continuity */
-/*  2 */__le16 i_xattr_icount;
-/*  4 */__le16 i_mode;
-/*  6 */__le16 i_nlink;
-/*  8 */__le32 i_size;
-/* 12 */__le32 i_reserved;
-/* 16 */union {
+	__le16 i_xattr_icount;	/*  2  encoding for xattr ibody size */
+	__le16 i_mode;		/*  4 */
+	__le16 i_nlink;		/*  6 */
+	__le32 i_size;		/*  8 */
+	__le32 i_reserved;	/* 12 */
+	union {			/* 16 */
 		/* file total compressed blocks for data mapping 1 */
 		__le32 compressed_blocks;
 		__le32 raw_blkaddr;
@@ -90,26 +90,26 @@ struct erofs_inode_v1 {
 		/* for device files, used to indicate old/new device # */
 		__le32 rdev;
 	} i_u __packed;
-/* 20 */__le32 i_ino;           /* only used for 32-bit stat compatibility */
-/* 24 */__le16 i_uid;
-/* 26 */__le16 i_gid;
-/* 28 */__le32 i_reserved2;
-} __packed;
+	__le32 i_ino;		/* 20 only used for 32-bit stat compatibility */
+	__le16 i_uid;		/* 24 */
+	__le16 i_gid;		/* 26 */
+	__le32 i_reserved2;	/* 28 */
+} __packed;			/* 32 bytes */
 
 /* 32 bytes on-disk inode */
 #define EROFS_INODE_LAYOUT_V1   0
 /* 64 bytes on-disk inode */
 #define EROFS_INODE_LAYOUT_V2   1
 
-struct erofs_inode_v2 {
-/*  0 */__le16 i_advise;
+struct erofs_inode_v2 {		/* off description */
+	__le16 i_advise;	/*  0  file hints */
 
 /* 1 header + n-1 * 4 bytes inline xattr to keep continuity */
-/*  2 */__le16 i_xattr_icount;
-/*  4 */__le16 i_mode;
-/*  6 */__le16 i_reserved;
-/*  8 */__le64 i_size;
-/* 16 */union {
+	__le16 i_xattr_icount;	/*  2  encoding for xattr ibody size */
+	__le16 i_mode;		/*  4 */
+	__le16 i_reserved;	/*  6 */
+	__le64 i_size;		/*  8 */
+	union {			/* 16 */
 		/* file total compressed blocks for data mapping 1 */
 		__le32 compressed_blocks;
 		__le32 raw_blkaddr;
@@ -119,15 +119,15 @@ struct erofs_inode_v2 {
 	} i_u __packed;
 
 	/* only used for 32-bit stat compatibility */
-/* 20 */__le32 i_ino;
-
-/* 24 */__le32 i_uid;
-/* 28 */__le32 i_gid;
-/* 32 */__le64 i_ctime;
-/* 40 */__le32 i_ctime_nsec;
-/* 44 */__le32 i_nlink;
-/* 48 */__u8   i_reserved2[16];
-} __packed;                     /* 64 bytes */
+	__le32 i_ino;		/* 20 only used for 32-bit stat compatibility */
+
+	__le32 i_uid;		/* 24 */
+	__le32 i_gid;		/* 28 */
+	__le64 i_ctime;		/* 32 */
+	__le32 i_ctime_nsec;	/* 40 */
+	__le32 i_nlink;		/* 44 */
+	__u8   i_reserved2[16];	/* 48 */
+} __packed;			/* 64 bytes */
 
 #define EROFS_MAX_SHARED_XATTRS         (128)
 /* h_shared_count between 129 ... 255 are special # */
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* [PATCH v3 4/7] erofs: kill __packed for on-disk structures
  2019-08-30  3:36       ` [PATCH v3 1/7] erofs: on-disk format should have explicitly assigned numbers Gao Xiang
  2019-08-30  3:36         ` [PATCH v3 2/7] erofs: some macros are much more readable as a function Gao Xiang
  2019-08-30  3:36         ` [PATCH v3 3/7] erofs: use a better form for complicated on-disk fields Gao Xiang
@ 2019-08-30  3:36         ` Gao Xiang
  2019-08-30  6:16           ` Chao Yu
  2019-08-30  3:36         ` [PATCH v3 5/7] erofs: kill erofs_{init,exit}_inode_cache Gao Xiang
                           ` (3 subsequent siblings)
  6 siblings, 1 reply; 38+ messages in thread
From: Gao Xiang @ 2019-08-30  3:36 UTC (permalink / raw)
  To: Chao Yu, Dan Carpenter, Christoph Hellwig, Joe Perches,
	Greg Kroah-Hartman, devel
  Cc: linux-erofs, LKML, weidu.du, Miao Xie

As Christoph claimed "Please don't add __packed" [1],
I have to remove all __packed except struct erofs_dirent here.

Note that all on-disk fields except struct erofs_dirent
(12 bytes with a 8-byte nid) in EROFS are naturally aligned.

[1] https://lore.kernel.org/lkml/20190829095954.GB20598@infradead.org/
Reported-by: Christoph Hellwig <hch@infradead.org>
Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>
---
no change.

 fs/erofs/erofs_fs.h | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/fs/erofs/erofs_fs.h b/fs/erofs/erofs_fs.h
index fbdaf873d736..b07984a17f11 100644
--- a/fs/erofs/erofs_fs.h
+++ b/fs/erofs/erofs_fs.h
@@ -37,7 +37,7 @@ struct erofs_super_block {	/* off description */
 	__le32 requirements;	/* 80  (aka. feature_incompat) */
 
 	__u8 reserved2[44];	/* 84 */
-} __packed;			/* 128 bytes */
+};				/* 128 bytes */
 
 /*
  * erofs inode data mapping:
@@ -89,12 +89,12 @@ struct erofs_inode_v1 {		/* off description */
 
 		/* for device files, used to indicate old/new device # */
 		__le32 rdev;
-	} i_u __packed;
+	} i_u;
 	__le32 i_ino;		/* 20 only used for 32-bit stat compatibility */
 	__le16 i_uid;		/* 24 */
 	__le16 i_gid;		/* 26 */
 	__le32 i_reserved2;	/* 28 */
-} __packed;			/* 32 bytes */
+};				/* 32 bytes */
 
 /* 32 bytes on-disk inode */
 #define EROFS_INODE_LAYOUT_V1   0
@@ -116,7 +116,7 @@ struct erofs_inode_v2 {		/* off description */
 
 		/* for device files, used to indicate old/new device # */
 		__le32 rdev;
-	} i_u __packed;
+	} i_u;
 
 	/* only used for 32-bit stat compatibility */
 	__le32 i_ino;		/* 20 only used for 32-bit stat compatibility */
@@ -127,7 +127,7 @@ struct erofs_inode_v2 {		/* off description */
 	__le32 i_ctime_nsec;	/* 40 */
 	__le32 i_nlink;		/* 44 */
 	__u8   i_reserved2[16];	/* 48 */
-} __packed;			/* 64 bytes */
+};				/* 64 bytes */
 
 #define EROFS_MAX_SHARED_XATTRS         (128)
 /* h_shared_count between 129 ... 255 are special # */
@@ -149,7 +149,7 @@ struct erofs_xattr_ibody_header {
 	__u8   h_shared_count;
 	__u8   h_reserved2[7];
 	__le32 h_shared_xattrs[0];      /* shared xattr id array */
-} __packed;
+};
 
 /* Name indexes */
 #define EROFS_XATTR_INDEX_USER              1
@@ -166,7 +166,7 @@ struct erofs_xattr_entry {
 	__le16 e_value_size;    /* size of attribute value */
 	/* followed by e_name and e_value */
 	char   e_name[0];       /* attribute name */
-} __packed;
+};
 
 static inline unsigned int erofs_xattr_ibody_size(__le16 d_icount)
 {
@@ -272,8 +272,8 @@ struct z_erofs_vle_decompressed_index {
 		 * [1] - pointing to the tail cluster
 		 */
 		__le16 delta[2];
-	} di_u __packed;		/* 8 bytes */
-} __packed;
+	} di_u;			/* 8 bytes */
+};
 
 #define Z_EROFS_VLE_LEGACY_INDEX_ALIGN(size) \
 	(round_up(size, sizeof(struct z_erofs_vle_decompressed_index)) + \
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* [PATCH v3 5/7] erofs: kill erofs_{init,exit}_inode_cache
  2019-08-30  3:36       ` [PATCH v3 1/7] erofs: on-disk format should have explicitly assigned numbers Gao Xiang
                           ` (2 preceding siblings ...)
  2019-08-30  3:36         ` [PATCH v3 4/7] erofs: kill __packed for on-disk structures Gao Xiang
@ 2019-08-30  3:36         ` Gao Xiang
  2019-08-30  6:17           ` Chao Yu
  2019-08-30  3:36         ` [PATCH v3 6/7] erofs: remove all likely/unlikely annotations Gao Xiang
                           ` (2 subsequent siblings)
  6 siblings, 1 reply; 38+ messages in thread
From: Gao Xiang @ 2019-08-30  3:36 UTC (permalink / raw)
  To: Chao Yu, Dan Carpenter, Christoph Hellwig, Joe Perches,
	Greg Kroah-Hartman, devel
  Cc: linux-erofs, LKML, weidu.du, Miao Xie

As Christoph said [1] "having this function
seems entirely pointless", I have to kill those.

filesystem                              function name
ext2,f2fs,ext4,isofs,squashfs,cifs,...  init_inodecache

In addition, add a "rcu_barrier()" when exit_fs();

[1] https://lore.kernel.org/r/20190829101545.GC20598@infradead.org/
Reported-by: Christoph Hellwig <hch@infradead.org>
Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>
---
no change.

 fs/erofs/super.c | 31 ++++++++++++-------------------
 1 file changed, 12 insertions(+), 19 deletions(-)

diff --git a/fs/erofs/super.c b/fs/erofs/super.c
index 6d3a9bcb8daa..556aae5426d6 100644
--- a/fs/erofs/super.c
+++ b/fs/erofs/super.c
@@ -23,21 +23,6 @@ static void init_once(void *ptr)
 	inode_init_once(&vi->vfs_inode);
 }
 
-static int __init erofs_init_inode_cache(void)
-{
-	erofs_inode_cachep = kmem_cache_create("erofs_inode",
-					       sizeof(struct erofs_vnode), 0,
-					       SLAB_RECLAIM_ACCOUNT,
-					       init_once);
-
-	return erofs_inode_cachep ? 0 : -ENOMEM;
-}
-
-static void erofs_exit_inode_cache(void)
-{
-	kmem_cache_destroy(erofs_inode_cachep);
-}
-
 static struct inode *alloc_inode(struct super_block *sb)
 {
 	struct erofs_vnode *vi =
@@ -531,9 +516,14 @@ static int __init erofs_module_init(void)
 	erofs_check_ondisk_layout_definitions();
 	infoln("initializing erofs " EROFS_VERSION);
 
-	err = erofs_init_inode_cache();
-	if (err)
+	erofs_inode_cachep = kmem_cache_create("erofs_inode",
+					       sizeof(struct erofs_vnode), 0,
+					       SLAB_RECLAIM_ACCOUNT,
+					       init_once);
+	if (!erofs_inode_cachep) {
+		err = -ENOMEM;
 		goto icache_err;
+	}
 
 	err = erofs_init_shrinker();
 	if (err)
@@ -555,7 +545,7 @@ static int __init erofs_module_init(void)
 zip_err:
 	erofs_exit_shrinker();
 shrinker_err:
-	erofs_exit_inode_cache();
+	kmem_cache_destroy(erofs_inode_cachep);
 icache_err:
 	return err;
 }
@@ -565,7 +555,10 @@ static void __exit erofs_module_exit(void)
 	unregister_filesystem(&erofs_fs_type);
 	z_erofs_exit_zip_subsystem();
 	erofs_exit_shrinker();
-	erofs_exit_inode_cache();
+
+	/* Ensure all RCU free inodes are safe before cache is destroyed. */
+	rcu_barrier();
+	kmem_cache_destroy(erofs_inode_cachep);
 	infoln("successfully finalize erofs");
 }
 
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* [PATCH v3 6/7] erofs: remove all likely/unlikely annotations
  2019-08-30  3:36       ` [PATCH v3 1/7] erofs: on-disk format should have explicitly assigned numbers Gao Xiang
                           ` (3 preceding siblings ...)
  2019-08-30  3:36         ` [PATCH v3 5/7] erofs: kill erofs_{init,exit}_inode_cache Gao Xiang
@ 2019-08-30  3:36         ` Gao Xiang
  2019-08-30  6:25           ` Chao Yu
                             ` (2 more replies)
  2019-08-30  3:36         ` [PATCH v3 7/7] erofs: redundant assignment in __erofs_get_meta_page() Gao Xiang
  2019-08-30 15:25         ` [PATCH v3 1/7] erofs: on-disk format should have explicitly assigned numbers Gao Xiang
  6 siblings, 3 replies; 38+ messages in thread
From: Gao Xiang @ 2019-08-30  3:36 UTC (permalink / raw)
  To: Chao Yu, Dan Carpenter, Christoph Hellwig, Joe Perches,
	Greg Kroah-Hartman, devel
  Cc: linux-erofs, LKML, weidu.du, Miao Xie

As Dan Carpenter suggested [1], I have to remove
all erofs likely/unlikely annotations.

[1] https://lore.kernel.org/linux-fsdevel/20190829154346.GK23584@kadam/
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>
---
no change.

 fs/erofs/data.c         | 22 ++++++++++-----------
 fs/erofs/decompressor.c |  2 +-
 fs/erofs/dir.c          | 14 ++++++-------
 fs/erofs/inode.c        | 10 +++++-----
 fs/erofs/internal.h     |  4 ++--
 fs/erofs/namei.c        | 14 ++++++-------
 fs/erofs/super.c        | 16 +++++++--------
 fs/erofs/utils.c        | 12 +++++------
 fs/erofs/xattr.c        | 12 +++++------
 fs/erofs/zdata.c        | 44 ++++++++++++++++++++---------------------
 fs/erofs/zmap.c         |  8 ++++----
 fs/erofs/zpvec.h        |  6 +++---
 12 files changed, 82 insertions(+), 82 deletions(-)

diff --git a/fs/erofs/data.c b/fs/erofs/data.c
index fda16ec8863e..0f2f1a839372 100644
--- a/fs/erofs/data.c
+++ b/fs/erofs/data.c
@@ -27,7 +27,7 @@ static inline void read_endio(struct bio *bio)
 		/* page is already locked */
 		DBG_BUGON(PageUptodate(page));
 
-		if (unlikely(err))
+		if (err)
 			SetPageError(page);
 		else
 			SetPageUptodate(page);
@@ -53,7 +53,7 @@ struct page *__erofs_get_meta_page(struct super_block *sb,
 
 repeat:
 	page = find_or_create_page(mapping, blkaddr, gfp);
-	if (unlikely(!page)) {
+	if (!page) {
 		DBG_BUGON(nofail);
 		return ERR_PTR(-ENOMEM);
 	}
@@ -70,7 +70,7 @@ struct page *__erofs_get_meta_page(struct super_block *sb,
 		}
 
 		err = bio_add_page(bio, page, PAGE_SIZE, 0);
-		if (unlikely(err != PAGE_SIZE)) {
+		if (err != PAGE_SIZE) {
 			err = -EFAULT;
 			goto err_out;
 		}
@@ -81,7 +81,7 @@ struct page *__erofs_get_meta_page(struct super_block *sb,
 		lock_page(page);
 
 		/* this page has been truncated by others */
-		if (unlikely(page->mapping != mapping)) {
+		if (page->mapping != mapping) {
 unlock_repeat:
 			unlock_page(page);
 			put_page(page);
@@ -89,7 +89,7 @@ struct page *__erofs_get_meta_page(struct super_block *sb,
 		}
 
 		/* more likely a read error */
-		if (unlikely(!PageUptodate(page))) {
+		if (!PageUptodate(page)) {
 			if (io_retries) {
 				--io_retries;
 				goto unlock_repeat;
@@ -120,7 +120,7 @@ static int erofs_map_blocks_flatmode(struct inode *inode,
 	nblocks = DIV_ROUND_UP(inode->i_size, PAGE_SIZE);
 	lastblk = nblocks - is_inode_flat_inline(inode);
 
-	if (unlikely(offset >= inode->i_size)) {
+	if (offset >= inode->i_size) {
 		/* leave out-of-bound access unmapped */
 		map->m_flags = 0;
 		map->m_plen = 0;
@@ -170,7 +170,7 @@ static int erofs_map_blocks_flatmode(struct inode *inode,
 int erofs_map_blocks(struct inode *inode,
 		     struct erofs_map_blocks *map, int flags)
 {
-	if (unlikely(is_inode_layout_compression(inode))) {
+	if (is_inode_layout_compression(inode)) {
 		int err = z_erofs_map_blocks_iter(inode, map, flags);
 
 		if (map->mpage) {
@@ -218,11 +218,11 @@ static inline struct bio *erofs_read_raw_page(struct bio *bio,
 		unsigned int blkoff;
 
 		err = erofs_map_blocks(inode, &map, EROFS_GET_BLOCKS_RAW);
-		if (unlikely(err))
+		if (err)
 			goto err_out;
 
 		/* zero out the holed page */
-		if (unlikely(!(map.m_flags & EROFS_MAP_MAPPED))) {
+		if (!(map.m_flags & EROFS_MAP_MAPPED)) {
 			zero_user_segment(page, 0, PAGE_SIZE);
 			SetPageUptodate(page);
 
@@ -315,7 +315,7 @@ static inline struct bio *erofs_read_raw_page(struct bio *bio,
 submit_bio_out:
 		__submit_bio(bio, REQ_OP_READ, 0);
 
-	return unlikely(err) ? ERR_PTR(err) : NULL;
+	return err ? ERR_PTR(err) : NULL;
 }
 
 /*
@@ -377,7 +377,7 @@ static int erofs_raw_access_readpages(struct file *filp,
 	DBG_BUGON(!list_empty(pages));
 
 	/* the rare case (end in gaps) */
-	if (unlikely(bio))
+	if (bio)
 		__submit_bio(bio, REQ_OP_READ, 0);
 	return 0;
 }
diff --git a/fs/erofs/decompressor.c b/fs/erofs/decompressor.c
index 5f4b7f302863..df349888f911 100644
--- a/fs/erofs/decompressor.c
+++ b/fs/erofs/decompressor.c
@@ -78,7 +78,7 @@ static int lz4_prepare_destpages(struct z_erofs_decompress_req *rq,
 			get_page(victim);
 		} else {
 			victim = erofs_allocpage(pagepool, GFP_KERNEL, false);
-			if (unlikely(!victim))
+			if (!victim)
 				return -ENOMEM;
 			victim->mapping = Z_EROFS_MAPPING_STAGING;
 		}
diff --git a/fs/erofs/dir.c b/fs/erofs/dir.c
index 1976e60e5174..6a5b43f7fb29 100644
--- a/fs/erofs/dir.c
+++ b/fs/erofs/dir.c
@@ -45,8 +45,8 @@ static int erofs_fill_dentries(struct inode *dir, struct dir_context *ctx,
 			de_namelen = le16_to_cpu(de[1].nameoff) - nameoff;
 
 		/* a corrupted entry is found */
-		if (unlikely(nameoff + de_namelen > maxsize ||
-			     de_namelen > EROFS_NAME_LEN)) {
+		if (nameoff + de_namelen > maxsize ||
+		    de_namelen > EROFS_NAME_LEN) {
 			errln("bogus dirent @ nid %llu", EROFS_V(dir)->nid);
 			DBG_BUGON(1);
 			return -EFSCORRUPTED;
@@ -94,8 +94,8 @@ static int erofs_readdir(struct file *f, struct dir_context *ctx)
 
 		nameoff = le16_to_cpu(de->nameoff);
 
-		if (unlikely(nameoff < sizeof(struct erofs_dirent) ||
-			     nameoff >= PAGE_SIZE)) {
+		if (nameoff < sizeof(struct erofs_dirent) ||
+		    nameoff >= PAGE_SIZE) {
 			errln("%s, invalid de[0].nameoff %u @ nid %llu",
 			      __func__, nameoff, EROFS_V(dir)->nid);
 			err = -EFSCORRUPTED;
@@ -106,11 +106,11 @@ static int erofs_readdir(struct file *f, struct dir_context *ctx)
 				dirsize - ctx->pos + ofs, PAGE_SIZE);
 
 		/* search dirents at the arbitrary position */
-		if (unlikely(initial)) {
+		if (initial) {
 			initial = false;
 
 			ofs = roundup(ofs, sizeof(struct erofs_dirent));
-			if (unlikely(ofs >= nameoff))
+			if (ofs >= nameoff)
 				goto skip_this;
 		}
 
@@ -123,7 +123,7 @@ static int erofs_readdir(struct file *f, struct dir_context *ctx)
 
 		ctx->pos = blknr_to_addr(i) + ofs;
 
-		if (unlikely(err))
+		if (err)
 			break;
 		++i;
 		ofs = 0;
diff --git a/fs/erofs/inode.c b/fs/erofs/inode.c
index cf31554075c9..871a6d8ed6f9 100644
--- a/fs/erofs/inode.c
+++ b/fs/erofs/inode.c
@@ -18,7 +18,7 @@ static int read_inode(struct inode *inode, void *data)
 
 	vi->datamode = __inode_data_mapping(advise);
 
-	if (unlikely(vi->datamode >= EROFS_INODE_LAYOUT_MAX)) {
+	if (vi->datamode >= EROFS_INODE_LAYOUT_MAX) {
 		errln("unsupported data mapping %u of nid %llu",
 		      vi->datamode, vi->nid);
 		DBG_BUGON(1);
@@ -133,13 +133,13 @@ static int fill_inline_data(struct inode *inode, void *data,
 	if (S_ISLNK(inode->i_mode) && inode->i_size < PAGE_SIZE) {
 		char *lnk = erofs_kmalloc(sbi, inode->i_size + 1, GFP_KERNEL);
 
-		if (unlikely(!lnk))
+		if (!lnk)
 			return -ENOMEM;
 
 		m_pofs += vi->inode_isize + vi->xattr_isize;
 
 		/* inline symlink data shouldn't across page boundary as well */
-		if (unlikely(m_pofs + inode->i_size > PAGE_SIZE)) {
+		if (m_pofs + inode->i_size > PAGE_SIZE) {
 			kfree(lnk);
 			errln("inline data cross block boundary @ nid %llu",
 			      vi->nid);
@@ -268,7 +268,7 @@ struct inode *erofs_iget(struct super_block *sb,
 {
 	struct inode *inode = erofs_iget_locked(sb, nid);
 
-	if (unlikely(!inode))
+	if (!inode)
 		return ERR_PTR(-ENOMEM);
 
 	if (inode->i_state & I_NEW) {
@@ -278,7 +278,7 @@ struct inode *erofs_iget(struct super_block *sb,
 		vi->nid = nid;
 
 		err = fill_inode(inode, isdir);
-		if (likely(!err))
+		if (!err)
 			unlock_new_inode(inode);
 		else {
 			iget_failed(inode);
diff --git a/fs/erofs/internal.h b/fs/erofs/internal.h
index 620b73fcc416..141ea424587d 100644
--- a/fs/erofs/internal.h
+++ b/fs/erofs/internal.h
@@ -424,7 +424,7 @@ static inline struct bio *erofs_grab_bio(struct super_block *sb,
 	do {
 		if (nr_pages == 1) {
 			bio = bio_alloc(gfp | (nofail ? __GFP_NOFAIL : 0), 1);
-			if (unlikely(!bio)) {
+			if (!bio) {
 				DBG_BUGON(nofail);
 				return ERR_PTR(-ENOMEM);
 			}
@@ -432,7 +432,7 @@ static inline struct bio *erofs_grab_bio(struct super_block *sb,
 		}
 		bio = bio_alloc(gfp, nr_pages);
 		nr_pages /= 2;
-	} while (unlikely(!bio));
+	} while (!bio);
 
 	bio->bi_end_io = endio;
 	bio_set_dev(bio, sb->s_bdev);
diff --git a/fs/erofs/namei.c b/fs/erofs/namei.c
index 8832b5d95d91..c1068ad0535e 100644
--- a/fs/erofs/namei.c
+++ b/fs/erofs/namei.c
@@ -64,7 +64,7 @@ static struct erofs_dirent *find_target_dirent(struct erofs_qstr *name,
 		unsigned int matched = min(startprfx, endprfx);
 		struct erofs_qstr dname = {
 			.name = data + nameoff,
-			.end = unlikely(mid >= ndirents - 1) ?
+			.end = mid >= ndirents - 1 ?
 				data + dirblksize :
 				data + nameoff_from_disk(de[mid + 1].nameoff,
 							 dirblksize)
@@ -73,7 +73,7 @@ static struct erofs_dirent *find_target_dirent(struct erofs_qstr *name,
 		/* string comparison without already matched prefix */
 		int ret = dirnamecmp(name, &dname, &matched);
 
-		if (unlikely(!ret)) {
+		if (!ret) {
 			return de + mid;
 		} else if (ret > 0) {
 			head = mid + 1;
@@ -113,7 +113,7 @@ static struct page *find_target_block_classic(struct inode *dir,
 			unsigned int matched;
 			struct erofs_qstr dname;
 
-			if (unlikely(!ndirents)) {
+			if (!ndirents) {
 				kunmap_atomic(de);
 				put_page(page);
 				errln("corrupted dir block %d @ nid %llu",
@@ -137,7 +137,7 @@ static struct page *find_target_block_classic(struct inode *dir,
 			diff = dirnamecmp(name, &dname, &matched);
 			kunmap_atomic(de);
 
-			if (unlikely(!diff)) {
+			if (!diff) {
 				*_ndirents = 0;
 				goto out;
 			} else if (diff > 0) {
@@ -174,7 +174,7 @@ int erofs_namei(struct inode *dir,
 	struct erofs_dirent *de;
 	struct erofs_qstr qn;
 
-	if (unlikely(!dir->i_size))
+	if (!dir->i_size)
 		return -ENOENT;
 
 	qn.name = name->name;
@@ -221,7 +221,7 @@ static struct dentry *erofs_lookup(struct inode *dir,
 	trace_erofs_lookup(dir, dentry, flags);
 
 	/* file name exceeds fs limit */
-	if (unlikely(dentry->d_name.len > EROFS_NAME_LEN))
+	if (dentry->d_name.len > EROFS_NAME_LEN)
 		return ERR_PTR(-ENAMETOOLONG);
 
 	/* false uninitialized warnings on gcc 4.8.x */
@@ -230,7 +230,7 @@ static struct dentry *erofs_lookup(struct inode *dir,
 	if (err == -ENOENT) {
 		/* negative dentry */
 		inode = NULL;
-	} else if (unlikely(err)) {
+	} else if (err) {
 		inode = ERR_PTR(err);
 	} else {
 		debugln("%s, %s (nid %llu) found, d_type %u", __func__,
diff --git a/fs/erofs/super.c b/fs/erofs/super.c
index 556aae5426d6..0c412de33315 100644
--- a/fs/erofs/super.c
+++ b/fs/erofs/super.c
@@ -92,7 +92,7 @@ static int superblock_read(struct super_block *sb)
 
 	blkszbits = layout->blkszbits;
 	/* 9(512 bytes) + LOG_SECTORS_PER_BLOCK == LOG_BLOCK_SIZE */
-	if (unlikely(blkszbits != LOG_BLOCK_SIZE)) {
+	if (blkszbits != LOG_BLOCK_SIZE) {
 		errln("blksize %u isn't supported on this platform",
 		      1 << blkszbits);
 		goto out;
@@ -364,7 +364,7 @@ static int erofs_init_managed_cache(struct super_block *sb)
 	struct erofs_sb_info *const sbi = EROFS_SB(sb);
 	struct inode *const inode = new_inode(sb);
 
-	if (unlikely(!inode))
+	if (!inode)
 		return -ENOMEM;
 
 	set_nlink(inode, 1);
@@ -391,13 +391,13 @@ static int erofs_fill_super(struct super_block *sb, void *data, int silent)
 
 	sb->s_magic = EROFS_SUPER_MAGIC;
 
-	if (unlikely(!sb_set_blocksize(sb, EROFS_BLKSIZ))) {
+	if (!sb_set_blocksize(sb, EROFS_BLKSIZ)) {
 		errln("failed to set erofs blksize");
 		return -EINVAL;
 	}
 
 	sbi = kzalloc(sizeof(*sbi), GFP_KERNEL);
-	if (unlikely(!sbi))
+	if (!sbi)
 		return -ENOMEM;
 
 	sb->s_fs_info = sbi;
@@ -418,7 +418,7 @@ static int erofs_fill_super(struct super_block *sb, void *data, int silent)
 	default_options(sbi);
 
 	err = parse_options(sb, data);
-	if (unlikely(err))
+	if (err)
 		return err;
 
 	if (!silent)
@@ -438,7 +438,7 @@ static int erofs_fill_super(struct super_block *sb, void *data, int silent)
 	if (IS_ERR(inode))
 		return PTR_ERR(inode);
 
-	if (unlikely(!S_ISDIR(inode->i_mode))) {
+	if (!S_ISDIR(inode->i_mode)) {
 		errln("rootino(nid %llu) is not a directory(i_mode %o)",
 		      ROOT_NID(sbi), inode->i_mode);
 		iput(inode);
@@ -446,13 +446,13 @@ static int erofs_fill_super(struct super_block *sb, void *data, int silent)
 	}
 
 	sb->s_root = d_make_root(inode);
-	if (unlikely(!sb->s_root))
+	if (!sb->s_root)
 		return -ENOMEM;
 
 	erofs_shrinker_register(sb);
 	/* sb->s_umount is already locked, SB_ACTIVE and SB_BORN are not set */
 	err = erofs_init_managed_cache(sb);
-	if (unlikely(err))
+	if (err)
 		return err;
 
 	if (!silent)
diff --git a/fs/erofs/utils.c b/fs/erofs/utils.c
index 1dd041aa0f5a..d92b3e753a6f 100644
--- a/fs/erofs/utils.c
+++ b/fs/erofs/utils.c
@@ -46,14 +46,14 @@ static int erofs_workgroup_get(struct erofs_workgroup *grp)
 
 repeat:
 	o = erofs_wait_on_workgroup_freezed(grp);
-	if (unlikely(o <= 0))
+	if (o <= 0)
 		return -1;
 
-	if (unlikely(atomic_cmpxchg(&grp->refcount, o, o + 1) != o))
+	if (atomic_cmpxchg(&grp->refcount, o, o + 1) != o)
 		goto repeat;
 
 	/* decrease refcount paired by erofs_workgroup_put */
-	if (unlikely(o == 1))
+	if (o == 1)
 		atomic_long_dec(&erofs_global_shrink_cnt);
 	return 0;
 }
@@ -91,7 +91,7 @@ int erofs_register_workgroup(struct super_block *sb,
 	int err;
 
 	/* grp shouldn't be broken or used before */
-	if (unlikely(atomic_read(&grp->refcount) != 1)) {
+	if (atomic_read(&grp->refcount) != 1) {
 		DBG_BUGON(1);
 		return -EINVAL;
 	}
@@ -113,7 +113,7 @@ int erofs_register_workgroup(struct super_block *sb,
 	__erofs_workgroup_get(grp);
 
 	err = radix_tree_insert(&sbi->workstn_tree, grp->index, grp);
-	if (unlikely(err))
+	if (err)
 		/*
 		 * it's safe to decrease since the workgroup isn't visible
 		 * and refcount >= 2 (cannot be freezed).
@@ -212,7 +212,7 @@ static unsigned long erofs_shrink_workstation(struct erofs_sb_info *sbi,
 			continue;
 
 		++freed;
-		if (unlikely(!--nr_shrink))
+		if (!--nr_shrink)
 			break;
 	}
 	xa_unlock(&sbi->workstn_tree);
diff --git a/fs/erofs/xattr.c b/fs/erofs/xattr.c
index 7ef8d4bb45cd..620cbc15f4d0 100644
--- a/fs/erofs/xattr.c
+++ b/fs/erofs/xattr.c
@@ -19,7 +19,7 @@ struct xattr_iter {
 static inline void xattr_iter_end(struct xattr_iter *it, bool atomic)
 {
 	/* the only user of kunmap() is 'init_inode_xattrs' */
-	if (unlikely(!atomic))
+	if (!atomic)
 		kunmap(it->page);
 	else
 		kunmap_atomic(it->kaddr);
@@ -72,7 +72,7 @@ static int init_inode_xattrs(struct inode *inode)
 		ret = -EOPNOTSUPP;
 		goto out_unlock;
 	} else if (vi->xattr_isize < sizeof(struct erofs_xattr_ibody_header)) {
-		if (unlikely(vi->xattr_isize)) {
+		if (vi->xattr_isize) {
 			errln("bogus xattr ibody @ nid %llu", vi->nid);
 			DBG_BUGON(1);
 			ret = -EFSCORRUPTED;
@@ -112,7 +112,7 @@ static int init_inode_xattrs(struct inode *inode)
 	it.ofs += sizeof(struct erofs_xattr_ibody_header);
 
 	for (i = 0; i < vi->xattr_shared_count; ++i) {
-		if (unlikely(it.ofs >= EROFS_BLKSIZ)) {
+		if (it.ofs >= EROFS_BLKSIZ) {
 			/* cannot be unaligned */
 			DBG_BUGON(it.ofs != EROFS_BLKSIZ);
 			xattr_iter_end(&it, atomic_map);
@@ -189,7 +189,7 @@ static int inline_xattr_iter_begin(struct xattr_iter *it,
 	unsigned int xattr_header_sz, inline_xattr_ofs;
 
 	xattr_header_sz = inlinexattr_header_size(inode);
-	if (unlikely(xattr_header_sz >= vi->xattr_isize)) {
+	if (xattr_header_sz >= vi->xattr_isize) {
 		DBG_BUGON(xattr_header_sz > vi->xattr_isize);
 		return -ENOATTR;
 	}
@@ -234,7 +234,7 @@ static int xattr_foreach(struct xattr_iter *it,
 		unsigned int entry_sz = erofs_xattr_entry_size(&entry);
 
 		/* xattr on-disk corruption: xattr entry beyond xattr_isize */
-		if (unlikely(*tlimit < entry_sz)) {
+		if (*tlimit < entry_sz) {
 			DBG_BUGON(1);
 			return -EFSCORRUPTED;
 		}
@@ -436,7 +436,7 @@ int erofs_getxattr(struct inode *inode, int index,
 	int ret;
 	struct getxattr_iter it;
 
-	if (unlikely(!name))
+	if (!name)
 		return -EINVAL;
 
 	ret = init_inode_xattrs(inode);
diff --git a/fs/erofs/zdata.c b/fs/erofs/zdata.c
index b32ad585237c..653bde0a619a 100644
--- a/fs/erofs/zdata.c
+++ b/fs/erofs/zdata.c
@@ -230,7 +230,7 @@ int erofs_try_to_free_all_cached_pages(struct erofs_sb_info *sbi,
 		if (!trylock_page(page))
 			return -EBUSY;
 
-		if (unlikely(page->mapping != mapping))
+		if (page->mapping != mapping)
 			continue;
 
 		/* barrier is implied in the following 'unlock_page' */
@@ -358,7 +358,7 @@ static struct z_erofs_collection *cllookup(struct z_erofs_collector *clt,
 	}
 
 	cl = z_erofs_primarycollection(pcl);
-	if (unlikely(cl->pageofs != (map->m_la & ~PAGE_MASK))) {
+	if (cl->pageofs != (map->m_la & ~PAGE_MASK)) {
 		DBG_BUGON(1);
 		erofs_workgroup_put(grp);
 		return ERR_PTR(-EFSCORRUPTED);
@@ -406,7 +406,7 @@ static struct z_erofs_collection *clregister(struct z_erofs_collector *clt,
 
 	/* no available workgroup, let's allocate one */
 	pcl = kmem_cache_alloc(pcluster_cachep, GFP_NOFS);
-	if (unlikely(!pcl))
+	if (!pcl)
 		return ERR_PTR(-ENOMEM);
 
 	init_always(pcl);
@@ -474,7 +474,7 @@ static int z_erofs_collector_begin(struct z_erofs_collector *clt,
 	if (!cl) {
 		cl = clregister(clt, inode, map);
 
-		if (unlikely(cl == ERR_PTR(-EAGAIN)))
+		if (cl == ERR_PTR(-EAGAIN))
 			goto repeat;
 	}
 
@@ -607,15 +607,15 @@ static int z_erofs_do_read_page(struct z_erofs_decompress_frontend *fe,
 	map->m_la = offset + cur;
 	map->m_llen = 0;
 	err = z_erofs_map_blocks_iter(inode, map, 0);
-	if (unlikely(err))
+	if (err)
 		goto err_out;
 
 restart_now:
-	if (unlikely(!(map->m_flags & EROFS_MAP_MAPPED)))
+	if (!(map->m_flags & EROFS_MAP_MAPPED))
 		goto hitted;
 
 	err = z_erofs_collector_begin(clt, inode, map);
-	if (unlikely(err))
+	if (err)
 		goto err_out;
 
 	/* preload all compressed pages (maybe downgrade role if necessary) */
@@ -630,7 +630,7 @@ static int z_erofs_do_read_page(struct z_erofs_decompress_frontend *fe,
 	tight &= (clt->mode >= COLLECT_PRIMARY_HOOKED);
 hitted:
 	cur = end - min_t(unsigned int, offset + end - map->m_la, end);
-	if (unlikely(!(map->m_flags & EROFS_MAP_MAPPED))) {
+	if (!(map->m_flags & EROFS_MAP_MAPPED)) {
 		zero_user_segment(page, cur, end);
 		goto next_part;
 	}
@@ -653,11 +653,11 @@ static int z_erofs_do_read_page(struct z_erofs_decompress_frontend *fe,
 
 		err = z_erofs_attach_page(clt, newpage,
 					  Z_EROFS_PAGE_TYPE_EXCLUSIVE);
-		if (likely(!err))
+		if (!err)
 			goto retry;
 	}
 
-	if (unlikely(err))
+	if (err)
 		goto err_out;
 
 	index = page->index - (map->m_la >> PAGE_SHIFT);
@@ -723,7 +723,7 @@ static inline void z_erofs_vle_read_endio(struct bio *bio)
 		DBG_BUGON(PageUptodate(page));
 		DBG_BUGON(!page->mapping);
 
-		if (unlikely(!sbi && !z_erofs_page_is_staging(page))) {
+		if (!sbi && !z_erofs_page_is_staging(page)) {
 			sbi = EROFS_SB(page->mapping->host->i_sb);
 
 			if (time_to_inject(sbi, FAULT_READ_IO)) {
@@ -736,7 +736,7 @@ static inline void z_erofs_vle_read_endio(struct bio *bio)
 		if (sbi)
 			cachemngd = erofs_page_is_managed(sbi, page);
 
-		if (unlikely(err))
+		if (err)
 			SetPageError(page);
 		else if (cachemngd)
 			SetPageUptodate(page);
@@ -772,7 +772,7 @@ static int z_erofs_decompress_pcluster(struct super_block *sb,
 	mutex_lock(&cl->lock);
 	nr_pages = cl->nr_pages;
 
-	if (likely(nr_pages <= Z_EROFS_VMAP_ONSTACK_PAGES)) {
+	if (nr_pages <= Z_EROFS_VMAP_ONSTACK_PAGES) {
 		pages = pages_onstack;
 	} else if (nr_pages <= Z_EROFS_VMAP_GLOBAL_PAGES &&
 		   mutex_trylock(&z_pagemap_global_lock)) {
@@ -787,7 +787,7 @@ static int z_erofs_decompress_pcluster(struct super_block *sb,
 				       gfp_flags);
 
 		/* fallback to global pagemap for the lowmem scenario */
-		if (unlikely(!pages)) {
+		if (!pages) {
 			mutex_lock(&z_pagemap_global_lock);
 			pages = z_pagemap_global;
 		}
@@ -823,7 +823,7 @@ static int z_erofs_decompress_pcluster(struct super_block *sb,
 		 * currently EROFS doesn't support multiref(dedup),
 		 * so here erroring out one multiref page.
 		 */
-		if (unlikely(pages[pagenr])) {
+		if (pages[pagenr]) {
 			DBG_BUGON(1);
 			SetPageError(pages[pagenr]);
 			z_erofs_onlinepage_endio(pages[pagenr]);
@@ -847,7 +847,7 @@ static int z_erofs_decompress_pcluster(struct super_block *sb,
 
 		if (!z_erofs_page_is_staging(page)) {
 			if (erofs_page_is_managed(sbi, page)) {
-				if (unlikely(!PageUptodate(page)))
+				if (!PageUptodate(page))
 					err = -EIO;
 				continue;
 			}
@@ -859,7 +859,7 @@ static int z_erofs_decompress_pcluster(struct super_block *sb,
 			pagenr = z_erofs_onlinepage_index(page);
 
 			DBG_BUGON(pagenr >= nr_pages);
-			if (unlikely(pages[pagenr])) {
+			if (pages[pagenr]) {
 				DBG_BUGON(1);
 				SetPageError(pages[pagenr]);
 				z_erofs_onlinepage_endio(pages[pagenr]);
@@ -871,13 +871,13 @@ static int z_erofs_decompress_pcluster(struct super_block *sb,
 		}
 
 		/* PG_error needs checking for inplaced and staging pages */
-		if (unlikely(PageError(page))) {
+		if (PageError(page)) {
 			DBG_BUGON(PageUptodate(page));
 			err = -EIO;
 		}
 	}
 
-	if (unlikely(err))
+	if (err)
 		goto out;
 
 	llen = pcl->length >> Z_EROFS_PCLUSTER_LENGTH_BIT;
@@ -926,7 +926,7 @@ static int z_erofs_decompress_pcluster(struct super_block *sb,
 		if (z_erofs_put_stagingpage(pagepool, page))
 			continue;
 
-		if (unlikely(err < 0))
+		if (err < 0)
 			SetPageError(page);
 
 		z_erofs_onlinepage_endio(page);
@@ -934,7 +934,7 @@ static int z_erofs_decompress_pcluster(struct super_block *sb,
 
 	if (pages == z_pagemap_global)
 		mutex_unlock(&z_pagemap_global_lock);
-	else if (unlikely(pages != pages_onstack))
+	else if (pages != pages_onstack)
 		kvfree(pages);
 
 	cl->nr_pages = 0;
@@ -1212,7 +1212,7 @@ static bool z_erofs_vle_submit_all(struct super_block *sb,
 	bool force_submit = false;
 	unsigned int nr_bios;
 
-	if (unlikely(owned_head == Z_EROFS_PCLUSTER_TAIL))
+	if (owned_head == Z_EROFS_PCLUSTER_TAIL)
 		return false;
 
 	force_submit = false;
diff --git a/fs/erofs/zmap.c b/fs/erofs/zmap.c
index 4dc9cec01297..850e0e3d57a8 100644
--- a/fs/erofs/zmap.c
+++ b/fs/erofs/zmap.c
@@ -348,7 +348,7 @@ static int vle_extent_lookback(struct z_erofs_maprecorder *m,
 
 	switch (m->type) {
 	case Z_EROFS_VLE_CLUSTER_TYPE_NONHEAD:
-		if (unlikely(!m->delta[0])) {
+		if (!m->delta[0]) {
 			errln("invalid lookback distance 0 at nid %llu",
 			      vi->nid);
 			DBG_BUGON(1);
@@ -386,7 +386,7 @@ int z_erofs_map_blocks_iter(struct inode *inode,
 	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)) {
+	if (map->m_la >= inode->i_size) {
 		map->m_llen = map->m_la + 1 - inode->i_size;
 		map->m_la = inode->i_size;
 		map->m_flags = 0;
@@ -420,7 +420,7 @@ int z_erofs_map_blocks_iter(struct inode *inode,
 			break;
 		}
 		/* m.lcn should be >= 1 if endoff < m.clusterofs */
-		if (unlikely(!m.lcn)) {
+		if (!m.lcn) {
 			errln("invalid logical cluster 0 at nid %llu",
 			      vi->nid);
 			err = -EFSCORRUPTED;
@@ -433,7 +433,7 @@ int z_erofs_map_blocks_iter(struct inode *inode,
 	case Z_EROFS_VLE_CLUSTER_TYPE_NONHEAD:
 		/* get the correspoinding first chunk */
 		err = vle_extent_lookback(&m, m.delta[0]);
-		if (unlikely(err))
+		if (err)
 			goto unmap_out;
 		break;
 	default:
diff --git a/fs/erofs/zpvec.h b/fs/erofs/zpvec.h
index bd3cee16491c..58556903aa94 100644
--- a/fs/erofs/zpvec.h
+++ b/fs/erofs/zpvec.h
@@ -111,11 +111,11 @@ static inline bool z_erofs_pagevec_enqueue(struct z_erofs_pagevec_ctor *ctor,
 					   bool *occupied)
 {
 	*occupied = false;
-	if (unlikely(!ctor->next && type))
+	if (!ctor->next && type)
 		if (ctor->index + 1 == ctor->nr)
 			return false;
 
-	if (unlikely(ctor->index >= ctor->nr))
+	if (ctor->index >= ctor->nr)
 		z_erofs_pagevec_ctor_pagedown(ctor, false);
 
 	/* exclusive page type must be 0 */
@@ -137,7 +137,7 @@ z_erofs_pagevec_dequeue(struct z_erofs_pagevec_ctor *ctor,
 {
 	erofs_vtptr_t t;
 
-	if (unlikely(ctor->index >= ctor->nr)) {
+	if (ctor->index >= ctor->nr) {
 		DBG_BUGON(!ctor->next);
 		z_erofs_pagevec_ctor_pagedown(ctor, true);
 	}
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* [PATCH v3 7/7] erofs: redundant assignment in __erofs_get_meta_page()
  2019-08-30  3:36       ` [PATCH v3 1/7] erofs: on-disk format should have explicitly assigned numbers Gao Xiang
                           ` (4 preceding siblings ...)
  2019-08-30  3:36         ` [PATCH v3 6/7] erofs: remove all likely/unlikely annotations Gao Xiang
@ 2019-08-30  3:36         ` Gao Xiang
  2019-08-30  6:25           ` Chao Yu
  2019-08-30 16:28           ` Christoph Hellwig
  2019-08-30 15:25         ` [PATCH v3 1/7] erofs: on-disk format should have explicitly assigned numbers Gao Xiang
  6 siblings, 2 replies; 38+ messages in thread
From: Gao Xiang @ 2019-08-30  3:36 UTC (permalink / raw)
  To: Chao Yu, Dan Carpenter, Christoph Hellwig, Joe Perches,
	Greg Kroah-Hartman, devel
  Cc: linux-erofs, LKML, weidu.du, Miao Xie

As Joe Perches suggested [1],
 		err = bio_add_page(bio, page, PAGE_SIZE, 0);
-		if (unlikely(err != PAGE_SIZE)) {
+		if (err != PAGE_SIZE) {
 			err = -EFAULT;
 			goto err_out;
 		}

The initial assignment to err is odd as it's not
actually an error value -E<FOO> but a int size
from a unsigned int len.

Here the return is either 0 or PAGE_SIZE.

This would be more legible to me as:

		if (bio_add_page(bio, page, PAGE_SIZE, 0) != PAGE_SIZE) {
			err = -EFAULT;
			goto err_out;
		}

[1] https://lore.kernel.org/r/74c4784319b40deabfbaea92468f7e3ef44f1c96.camel@perches.com/
Reported-by: Joe Perches <joe@perches.com>
Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>
---
v3: fix a typo in subject line.

 fs/erofs/data.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/fs/erofs/data.c b/fs/erofs/data.c
index 0f2f1a839372..0983807737fd 100644
--- a/fs/erofs/data.c
+++ b/fs/erofs/data.c
@@ -69,8 +69,7 @@ struct page *__erofs_get_meta_page(struct super_block *sb,
 			goto err_out;
 		}
 
-		err = bio_add_page(bio, page, PAGE_SIZE, 0);
-		if (err != PAGE_SIZE) {
+		if (bio_add_page(bio, page, PAGE_SIZE, 0) != PAGE_SIZE) {
 			err = -EFAULT;
 			goto err_out;
 		}
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* [PATCH v4 2/7] erofs: some macros are much more readable as a function
  2019-08-30  3:36         ` [PATCH v3 2/7] erofs: some macros are much more readable as a function Gao Xiang
@ 2019-08-30  3:38           ` Gao Xiang
  2019-08-30  6:11           ` [PATCH v3 " Chao Yu
  1 sibling, 0 replies; 38+ messages in thread
From: Gao Xiang @ 2019-08-30  3:38 UTC (permalink / raw)
  To: Chao Yu, Dan Carpenter, Christoph Hellwig, Joe Perches,
	Greg Kroah-Hartman, devel
  Cc: linux-erofs, LKML, weidu.du, Miao Xie

As Christoph suggested [1], these macros are much
more readable as a function.

[1] https://lore.kernel.org/r/20190829095954.GB20598@infradead.org/
Reported-by: Christoph Hellwig <hch@infradead.org>
Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>
---
v4: a type fix in commit message.
v3: change as Joe suggested,
 https://lore.kernel.org/r/5b2ecf5cec1a6aa3834e9af41886a7fcb18ae86a.camel@perches.com/

 fs/erofs/erofs_fs.h | 24 ++++++++++++++++--------
 fs/erofs/inode.c    |  4 ++--
 fs/erofs/xattr.c    |  2 +-
 3 files changed, 19 insertions(+), 11 deletions(-)

diff --git a/fs/erofs/erofs_fs.h b/fs/erofs/erofs_fs.h
index 2447ad4d0920..0782ba9da623 100644
--- a/fs/erofs/erofs_fs.h
+++ b/fs/erofs/erofs_fs.h
@@ -168,16 +168,24 @@ struct erofs_xattr_entry {
 	char   e_name[0];       /* attribute name */
 } __packed;
 
-#define ondisk_xattr_ibody_size(count)	({\
-	u32 __count = le16_to_cpu(count); \
-	((__count) == 0) ? 0 : \
-	sizeof(struct erofs_xattr_ibody_header) + \
-		sizeof(__u32) * ((__count) - 1); })
+static inline unsigned int erofs_xattr_ibody_size(__le16 d_icount)
+{
+	struct erofs_xattr_ibody_header *ibh;
+	unsigned int icount = le16_to_cpu(d_icount);
+
+	if (!icount)
+		return 0;
+
+	return struct_size(ibh, h_shared_xattrs, icount - 1);
+}
 
 #define EROFS_XATTR_ALIGN(size) round_up(size, sizeof(struct erofs_xattr_entry))
-#define EROFS_XATTR_ENTRY_SIZE(entry) EROFS_XATTR_ALIGN( \
-	sizeof(struct erofs_xattr_entry) + \
-	(entry)->e_name_len + le16_to_cpu((entry)->e_value_size))
+
+static inline unsigned int erofs_xattr_entry_size(struct erofs_xattr_entry *e)
+{
+	return EROFS_XATTR_ALIGN(sizeof(struct erofs_xattr_entry) +
+				 e->e_name_len + le16_to_cpu(e->e_value_size));
+}
 
 /* available compression algorithm types */
 enum {
diff --git a/fs/erofs/inode.c b/fs/erofs/inode.c
index 80f4fe919ee7..cf31554075c9 100644
--- a/fs/erofs/inode.c
+++ b/fs/erofs/inode.c
@@ -29,7 +29,7 @@ static int read_inode(struct inode *inode, void *data)
 		struct erofs_inode_v2 *v2 = data;
 
 		vi->inode_isize = sizeof(struct erofs_inode_v2);
-		vi->xattr_isize = ondisk_xattr_ibody_size(v2->i_xattr_icount);
+		vi->xattr_isize = erofs_xattr_ibody_size(v2->i_xattr_icount);
 
 		inode->i_mode = le16_to_cpu(v2->i_mode);
 		if (S_ISREG(inode->i_mode) || S_ISDIR(inode->i_mode) ||
@@ -62,7 +62,7 @@ static int read_inode(struct inode *inode, void *data)
 		struct erofs_sb_info *sbi = EROFS_SB(inode->i_sb);
 
 		vi->inode_isize = sizeof(struct erofs_inode_v1);
-		vi->xattr_isize = ondisk_xattr_ibody_size(v1->i_xattr_icount);
+		vi->xattr_isize = erofs_xattr_ibody_size(v1->i_xattr_icount);
 
 		inode->i_mode = le16_to_cpu(v1->i_mode);
 		if (S_ISREG(inode->i_mode) || S_ISDIR(inode->i_mode) ||
diff --git a/fs/erofs/xattr.c b/fs/erofs/xattr.c
index a8286998a079..7ef8d4bb45cd 100644
--- a/fs/erofs/xattr.c
+++ b/fs/erofs/xattr.c
@@ -231,7 +231,7 @@ static int xattr_foreach(struct xattr_iter *it,
 	 */
 	entry = *(struct erofs_xattr_entry *)(it->kaddr + it->ofs);
 	if (tlimit) {
-		unsigned int entry_sz = EROFS_XATTR_ENTRY_SIZE(&entry);
+		unsigned int entry_sz = erofs_xattr_entry_size(&entry);
 
 		/* xattr on-disk corruption: xattr entry beyond xattr_isize */
 		if (unlikely(*tlimit < entry_sz)) {
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* Re: [PATCH v3 2/7] erofs: some macros are much more readable as a function
  2019-08-30  3:36         ` [PATCH v3 2/7] erofs: some macros are much more readable as a function Gao Xiang
  2019-08-30  3:38           ` [PATCH v4 " Gao Xiang
@ 2019-08-30  6:11           ` Chao Yu
  1 sibling, 0 replies; 38+ messages in thread
From: Chao Yu @ 2019-08-30  6:11 UTC (permalink / raw)
  To: Gao Xiang, Dan Carpenter, Christoph Hellwig, Joe Perches,
	Greg Kroah-Hartman, devel
  Cc: linux-erofs, LKML, weidu.du, Miao Xie

On 2019/8/30 11:36, Gao Xiang wrote:
> As Christoph suggested [1], these marcos are much
> more readable as a function.
> 
> [1] https://lore.kernel.org/r/20190829095954.GB20598@infradead.org/
> Reported-by: Christoph Hellwig <hch@infradead.org>
> Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>

Reviewed-by: Chao Yu <yuchao0@huawei.com>

Thanks,

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v3 4/7] erofs: kill __packed for on-disk structures
  2019-08-30  3:36         ` [PATCH v3 4/7] erofs: kill __packed for on-disk structures Gao Xiang
@ 2019-08-30  6:16           ` Chao Yu
  0 siblings, 0 replies; 38+ messages in thread
From: Chao Yu @ 2019-08-30  6:16 UTC (permalink / raw)
  To: Gao Xiang, Dan Carpenter, Christoph Hellwig, Joe Perches,
	Greg Kroah-Hartman, devel
  Cc: linux-erofs, LKML, weidu.du, Miao Xie

On 2019/8/30 11:36, Gao Xiang wrote:
> As Christoph claimed "Please don't add __packed" [1],
> I have to remove all __packed except struct erofs_dirent here.
> 
> Note that all on-disk fields except struct erofs_dirent
> (12 bytes with a 8-byte nid) in EROFS are naturally aligned.
> 
> [1] https://lore.kernel.org/lkml/20190829095954.GB20598@infradead.org/
> Reported-by: Christoph Hellwig <hch@infradead.org>
> Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>

Reviewed-by: Chao Yu <yuchao0@huawei.com>

Thanks,

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v3 5/7] erofs: kill erofs_{init,exit}_inode_cache
  2019-08-30  3:36         ` [PATCH v3 5/7] erofs: kill erofs_{init,exit}_inode_cache Gao Xiang
@ 2019-08-30  6:17           ` Chao Yu
  0 siblings, 0 replies; 38+ messages in thread
From: Chao Yu @ 2019-08-30  6:17 UTC (permalink / raw)
  To: Gao Xiang, Dan Carpenter, Christoph Hellwig, Joe Perches,
	Greg Kroah-Hartman, devel
  Cc: linux-erofs, LKML, weidu.du, Miao Xie

On 2019/8/30 11:36, Gao Xiang wrote:
> As Christoph said [1] "having this function
> seems entirely pointless", I have to kill those.
> 
> filesystem                              function name
> ext2,f2fs,ext4,isofs,squashfs,cifs,...  init_inodecache
> 
> In addition, add a "rcu_barrier()" when exit_fs();
> 
> [1] https://lore.kernel.org/r/20190829101545.GC20598@infradead.org/
> Reported-by: Christoph Hellwig <hch@infradead.org>
> Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>

Reviewed-by: Chao Yu <yuchao0@huawei.com>

Thanks,

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v3 6/7] erofs: remove all likely/unlikely annotations
  2019-08-30  3:36         ` [PATCH v3 6/7] erofs: remove all likely/unlikely annotations Gao Xiang
@ 2019-08-30  6:25           ` Chao Yu
  2019-08-30  6:31             ` Gao Xiang
  2019-08-30 11:55             ` Dan Carpenter
  2019-08-30 11:30           ` Dan Carpenter
  2019-08-30 15:46           ` Christoph Hellwig
  2 siblings, 2 replies; 38+ messages in thread
From: Chao Yu @ 2019-08-30  6:25 UTC (permalink / raw)
  To: Gao Xiang, Dan Carpenter, Christoph Hellwig, Joe Perches,
	Greg Kroah-Hartman, devel
  Cc: linux-erofs, LKML, weidu.du, Miao Xie

On 2019/8/30 11:36, Gao Xiang wrote:
> As Dan Carpenter suggested [1], I have to remove
> all erofs likely/unlikely annotations.
> 
> [1] https://lore.kernel.org/linux-fsdevel/20190829154346.GK23584@kadam/
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>

I suggest we can modify this by following good example rather than removing them
all, at least, the fuzzed random fields of disk layout handling should be very
rare case, I guess it's fine to use unlikely.

To Dan, thoughts?

Thanks,

> ---
> no change.
> 
>  fs/erofs/data.c         | 22 ++++++++++-----------
>  fs/erofs/decompressor.c |  2 +-
>  fs/erofs/dir.c          | 14 ++++++-------
>  fs/erofs/inode.c        | 10 +++++-----
>  fs/erofs/internal.h     |  4 ++--
>  fs/erofs/namei.c        | 14 ++++++-------
>  fs/erofs/super.c        | 16 +++++++--------
>  fs/erofs/utils.c        | 12 +++++------
>  fs/erofs/xattr.c        | 12 +++++------
>  fs/erofs/zdata.c        | 44 ++++++++++++++++++++---------------------
>  fs/erofs/zmap.c         |  8 ++++----
>  fs/erofs/zpvec.h        |  6 +++---
>  12 files changed, 82 insertions(+), 82 deletions(-)
> 
> diff --git a/fs/erofs/data.c b/fs/erofs/data.c
> index fda16ec8863e..0f2f1a839372 100644
> --- a/fs/erofs/data.c
> +++ b/fs/erofs/data.c
> @@ -27,7 +27,7 @@ static inline void read_endio(struct bio *bio)
>  		/* page is already locked */
>  		DBG_BUGON(PageUptodate(page));
>  
> -		if (unlikely(err))
> +		if (err)
>  			SetPageError(page);
>  		else
>  			SetPageUptodate(page);
> @@ -53,7 +53,7 @@ struct page *__erofs_get_meta_page(struct super_block *sb,
>  
>  repeat:
>  	page = find_or_create_page(mapping, blkaddr, gfp);
> -	if (unlikely(!page)) {
> +	if (!page) {
>  		DBG_BUGON(nofail);
>  		return ERR_PTR(-ENOMEM);
>  	}
> @@ -70,7 +70,7 @@ struct page *__erofs_get_meta_page(struct super_block *sb,
>  		}
>  
>  		err = bio_add_page(bio, page, PAGE_SIZE, 0);
> -		if (unlikely(err != PAGE_SIZE)) {
> +		if (err != PAGE_SIZE) {
>  			err = -EFAULT;
>  			goto err_out;
>  		}
> @@ -81,7 +81,7 @@ struct page *__erofs_get_meta_page(struct super_block *sb,
>  		lock_page(page);
>  
>  		/* this page has been truncated by others */
> -		if (unlikely(page->mapping != mapping)) {
> +		if (page->mapping != mapping) {
>  unlock_repeat:
>  			unlock_page(page);
>  			put_page(page);
> @@ -89,7 +89,7 @@ struct page *__erofs_get_meta_page(struct super_block *sb,
>  		}
>  
>  		/* more likely a read error */
> -		if (unlikely(!PageUptodate(page))) {
> +		if (!PageUptodate(page)) {
>  			if (io_retries) {
>  				--io_retries;
>  				goto unlock_repeat;
> @@ -120,7 +120,7 @@ static int erofs_map_blocks_flatmode(struct inode *inode,
>  	nblocks = DIV_ROUND_UP(inode->i_size, PAGE_SIZE);
>  	lastblk = nblocks - is_inode_flat_inline(inode);
>  
> -	if (unlikely(offset >= inode->i_size)) {
> +	if (offset >= inode->i_size) {
>  		/* leave out-of-bound access unmapped */
>  		map->m_flags = 0;
>  		map->m_plen = 0;
> @@ -170,7 +170,7 @@ static int erofs_map_blocks_flatmode(struct inode *inode,
>  int erofs_map_blocks(struct inode *inode,
>  		     struct erofs_map_blocks *map, int flags)
>  {
> -	if (unlikely(is_inode_layout_compression(inode))) {
> +	if (is_inode_layout_compression(inode)) {
>  		int err = z_erofs_map_blocks_iter(inode, map, flags);
>  
>  		if (map->mpage) {
> @@ -218,11 +218,11 @@ static inline struct bio *erofs_read_raw_page(struct bio *bio,
>  		unsigned int blkoff;
>  
>  		err = erofs_map_blocks(inode, &map, EROFS_GET_BLOCKS_RAW);
> -		if (unlikely(err))
> +		if (err)
>  			goto err_out;
>  
>  		/* zero out the holed page */
> -		if (unlikely(!(map.m_flags & EROFS_MAP_MAPPED))) {
> +		if (!(map.m_flags & EROFS_MAP_MAPPED)) {
>  			zero_user_segment(page, 0, PAGE_SIZE);
>  			SetPageUptodate(page);
>  
> @@ -315,7 +315,7 @@ static inline struct bio *erofs_read_raw_page(struct bio *bio,
>  submit_bio_out:
>  		__submit_bio(bio, REQ_OP_READ, 0);
>  
> -	return unlikely(err) ? ERR_PTR(err) : NULL;
> +	return err ? ERR_PTR(err) : NULL;
>  }
>  
>  /*
> @@ -377,7 +377,7 @@ static int erofs_raw_access_readpages(struct file *filp,
>  	DBG_BUGON(!list_empty(pages));
>  
>  	/* the rare case (end in gaps) */
> -	if (unlikely(bio))
> +	if (bio)
>  		__submit_bio(bio, REQ_OP_READ, 0);
>  	return 0;
>  }
> diff --git a/fs/erofs/decompressor.c b/fs/erofs/decompressor.c
> index 5f4b7f302863..df349888f911 100644
> --- a/fs/erofs/decompressor.c
> +++ b/fs/erofs/decompressor.c
> @@ -78,7 +78,7 @@ static int lz4_prepare_destpages(struct z_erofs_decompress_req *rq,
>  			get_page(victim);
>  		} else {
>  			victim = erofs_allocpage(pagepool, GFP_KERNEL, false);
> -			if (unlikely(!victim))
> +			if (!victim)
>  				return -ENOMEM;
>  			victim->mapping = Z_EROFS_MAPPING_STAGING;
>  		}
> diff --git a/fs/erofs/dir.c b/fs/erofs/dir.c
> index 1976e60e5174..6a5b43f7fb29 100644
> --- a/fs/erofs/dir.c
> +++ b/fs/erofs/dir.c
> @@ -45,8 +45,8 @@ static int erofs_fill_dentries(struct inode *dir, struct dir_context *ctx,
>  			de_namelen = le16_to_cpu(de[1].nameoff) - nameoff;
>  
>  		/* a corrupted entry is found */
> -		if (unlikely(nameoff + de_namelen > maxsize ||
> -			     de_namelen > EROFS_NAME_LEN)) {
> +		if (nameoff + de_namelen > maxsize ||
> +		    de_namelen > EROFS_NAME_LEN) {
>  			errln("bogus dirent @ nid %llu", EROFS_V(dir)->nid);
>  			DBG_BUGON(1);
>  			return -EFSCORRUPTED;
> @@ -94,8 +94,8 @@ static int erofs_readdir(struct file *f, struct dir_context *ctx)
>  
>  		nameoff = le16_to_cpu(de->nameoff);
>  
> -		if (unlikely(nameoff < sizeof(struct erofs_dirent) ||
> -			     nameoff >= PAGE_SIZE)) {
> +		if (nameoff < sizeof(struct erofs_dirent) ||
> +		    nameoff >= PAGE_SIZE) {
>  			errln("%s, invalid de[0].nameoff %u @ nid %llu",
>  			      __func__, nameoff, EROFS_V(dir)->nid);
>  			err = -EFSCORRUPTED;
> @@ -106,11 +106,11 @@ static int erofs_readdir(struct file *f, struct dir_context *ctx)
>  				dirsize - ctx->pos + ofs, PAGE_SIZE);
>  
>  		/* search dirents at the arbitrary position */
> -		if (unlikely(initial)) {
> +		if (initial) {
>  			initial = false;
>  
>  			ofs = roundup(ofs, sizeof(struct erofs_dirent));
> -			if (unlikely(ofs >= nameoff))
> +			if (ofs >= nameoff)
>  				goto skip_this;
>  		}
>  
> @@ -123,7 +123,7 @@ static int erofs_readdir(struct file *f, struct dir_context *ctx)
>  
>  		ctx->pos = blknr_to_addr(i) + ofs;
>  
> -		if (unlikely(err))
> +		if (err)
>  			break;
>  		++i;
>  		ofs = 0;
> diff --git a/fs/erofs/inode.c b/fs/erofs/inode.c
> index cf31554075c9..871a6d8ed6f9 100644
> --- a/fs/erofs/inode.c
> +++ b/fs/erofs/inode.c
> @@ -18,7 +18,7 @@ static int read_inode(struct inode *inode, void *data)
>  
>  	vi->datamode = __inode_data_mapping(advise);
>  
> -	if (unlikely(vi->datamode >= EROFS_INODE_LAYOUT_MAX)) {
> +	if (vi->datamode >= EROFS_INODE_LAYOUT_MAX) {
>  		errln("unsupported data mapping %u of nid %llu",
>  		      vi->datamode, vi->nid);
>  		DBG_BUGON(1);
> @@ -133,13 +133,13 @@ static int fill_inline_data(struct inode *inode, void *data,
>  	if (S_ISLNK(inode->i_mode) && inode->i_size < PAGE_SIZE) {
>  		char *lnk = erofs_kmalloc(sbi, inode->i_size + 1, GFP_KERNEL);
>  
> -		if (unlikely(!lnk))
> +		if (!lnk)
>  			return -ENOMEM;
>  
>  		m_pofs += vi->inode_isize + vi->xattr_isize;
>  
>  		/* inline symlink data shouldn't across page boundary as well */
> -		if (unlikely(m_pofs + inode->i_size > PAGE_SIZE)) {
> +		if (m_pofs + inode->i_size > PAGE_SIZE) {
>  			kfree(lnk);
>  			errln("inline data cross block boundary @ nid %llu",
>  			      vi->nid);
> @@ -268,7 +268,7 @@ struct inode *erofs_iget(struct super_block *sb,
>  {
>  	struct inode *inode = erofs_iget_locked(sb, nid);
>  
> -	if (unlikely(!inode))
> +	if (!inode)
>  		return ERR_PTR(-ENOMEM);
>  
>  	if (inode->i_state & I_NEW) {
> @@ -278,7 +278,7 @@ struct inode *erofs_iget(struct super_block *sb,
>  		vi->nid = nid;
>  
>  		err = fill_inode(inode, isdir);
> -		if (likely(!err))
> +		if (!err)
>  			unlock_new_inode(inode);
>  		else {
>  			iget_failed(inode);
> diff --git a/fs/erofs/internal.h b/fs/erofs/internal.h
> index 620b73fcc416..141ea424587d 100644
> --- a/fs/erofs/internal.h
> +++ b/fs/erofs/internal.h
> @@ -424,7 +424,7 @@ static inline struct bio *erofs_grab_bio(struct super_block *sb,
>  	do {
>  		if (nr_pages == 1) {
>  			bio = bio_alloc(gfp | (nofail ? __GFP_NOFAIL : 0), 1);
> -			if (unlikely(!bio)) {
> +			if (!bio) {
>  				DBG_BUGON(nofail);
>  				return ERR_PTR(-ENOMEM);
>  			}
> @@ -432,7 +432,7 @@ static inline struct bio *erofs_grab_bio(struct super_block *sb,
>  		}
>  		bio = bio_alloc(gfp, nr_pages);
>  		nr_pages /= 2;
> -	} while (unlikely(!bio));
> +	} while (!bio);
>  
>  	bio->bi_end_io = endio;
>  	bio_set_dev(bio, sb->s_bdev);
> diff --git a/fs/erofs/namei.c b/fs/erofs/namei.c
> index 8832b5d95d91..c1068ad0535e 100644
> --- a/fs/erofs/namei.c
> +++ b/fs/erofs/namei.c
> @@ -64,7 +64,7 @@ static struct erofs_dirent *find_target_dirent(struct erofs_qstr *name,
>  		unsigned int matched = min(startprfx, endprfx);
>  		struct erofs_qstr dname = {
>  			.name = data + nameoff,
> -			.end = unlikely(mid >= ndirents - 1) ?
> +			.end = mid >= ndirents - 1 ?
>  				data + dirblksize :
>  				data + nameoff_from_disk(de[mid + 1].nameoff,
>  							 dirblksize)
> @@ -73,7 +73,7 @@ static struct erofs_dirent *find_target_dirent(struct erofs_qstr *name,
>  		/* string comparison without already matched prefix */
>  		int ret = dirnamecmp(name, &dname, &matched);
>  
> -		if (unlikely(!ret)) {
> +		if (!ret) {
>  			return de + mid;
>  		} else if (ret > 0) {
>  			head = mid + 1;
> @@ -113,7 +113,7 @@ static struct page *find_target_block_classic(struct inode *dir,
>  			unsigned int matched;
>  			struct erofs_qstr dname;
>  
> -			if (unlikely(!ndirents)) {
> +			if (!ndirents) {
>  				kunmap_atomic(de);
>  				put_page(page);
>  				errln("corrupted dir block %d @ nid %llu",
> @@ -137,7 +137,7 @@ static struct page *find_target_block_classic(struct inode *dir,
>  			diff = dirnamecmp(name, &dname, &matched);
>  			kunmap_atomic(de);
>  
> -			if (unlikely(!diff)) {
> +			if (!diff) {
>  				*_ndirents = 0;
>  				goto out;
>  			} else if (diff > 0) {
> @@ -174,7 +174,7 @@ int erofs_namei(struct inode *dir,
>  	struct erofs_dirent *de;
>  	struct erofs_qstr qn;
>  
> -	if (unlikely(!dir->i_size))
> +	if (!dir->i_size)
>  		return -ENOENT;
>  
>  	qn.name = name->name;
> @@ -221,7 +221,7 @@ static struct dentry *erofs_lookup(struct inode *dir,
>  	trace_erofs_lookup(dir, dentry, flags);
>  
>  	/* file name exceeds fs limit */
> -	if (unlikely(dentry->d_name.len > EROFS_NAME_LEN))
> +	if (dentry->d_name.len > EROFS_NAME_LEN)
>  		return ERR_PTR(-ENAMETOOLONG);
>  
>  	/* false uninitialized warnings on gcc 4.8.x */
> @@ -230,7 +230,7 @@ static struct dentry *erofs_lookup(struct inode *dir,
>  	if (err == -ENOENT) {
>  		/* negative dentry */
>  		inode = NULL;
> -	} else if (unlikely(err)) {
> +	} else if (err) {
>  		inode = ERR_PTR(err);
>  	} else {
>  		debugln("%s, %s (nid %llu) found, d_type %u", __func__,
> diff --git a/fs/erofs/super.c b/fs/erofs/super.c
> index 556aae5426d6..0c412de33315 100644
> --- a/fs/erofs/super.c
> +++ b/fs/erofs/super.c
> @@ -92,7 +92,7 @@ static int superblock_read(struct super_block *sb)
>  
>  	blkszbits = layout->blkszbits;
>  	/* 9(512 bytes) + LOG_SECTORS_PER_BLOCK == LOG_BLOCK_SIZE */
> -	if (unlikely(blkszbits != LOG_BLOCK_SIZE)) {
> +	if (blkszbits != LOG_BLOCK_SIZE) {
>  		errln("blksize %u isn't supported on this platform",
>  		      1 << blkszbits);
>  		goto out;
> @@ -364,7 +364,7 @@ static int erofs_init_managed_cache(struct super_block *sb)
>  	struct erofs_sb_info *const sbi = EROFS_SB(sb);
>  	struct inode *const inode = new_inode(sb);
>  
> -	if (unlikely(!inode))
> +	if (!inode)
>  		return -ENOMEM;
>  
>  	set_nlink(inode, 1);
> @@ -391,13 +391,13 @@ static int erofs_fill_super(struct super_block *sb, void *data, int silent)
>  
>  	sb->s_magic = EROFS_SUPER_MAGIC;
>  
> -	if (unlikely(!sb_set_blocksize(sb, EROFS_BLKSIZ))) {
> +	if (!sb_set_blocksize(sb, EROFS_BLKSIZ)) {
>  		errln("failed to set erofs blksize");
>  		return -EINVAL;
>  	}
>  
>  	sbi = kzalloc(sizeof(*sbi), GFP_KERNEL);
> -	if (unlikely(!sbi))
> +	if (!sbi)
>  		return -ENOMEM;
>  
>  	sb->s_fs_info = sbi;
> @@ -418,7 +418,7 @@ static int erofs_fill_super(struct super_block *sb, void *data, int silent)
>  	default_options(sbi);
>  
>  	err = parse_options(sb, data);
> -	if (unlikely(err))
> +	if (err)
>  		return err;
>  
>  	if (!silent)
> @@ -438,7 +438,7 @@ static int erofs_fill_super(struct super_block *sb, void *data, int silent)
>  	if (IS_ERR(inode))
>  		return PTR_ERR(inode);
>  
> -	if (unlikely(!S_ISDIR(inode->i_mode))) {
> +	if (!S_ISDIR(inode->i_mode)) {
>  		errln("rootino(nid %llu) is not a directory(i_mode %o)",
>  		      ROOT_NID(sbi), inode->i_mode);
>  		iput(inode);
> @@ -446,13 +446,13 @@ static int erofs_fill_super(struct super_block *sb, void *data, int silent)
>  	}
>  
>  	sb->s_root = d_make_root(inode);
> -	if (unlikely(!sb->s_root))
> +	if (!sb->s_root)
>  		return -ENOMEM;
>  
>  	erofs_shrinker_register(sb);
>  	/* sb->s_umount is already locked, SB_ACTIVE and SB_BORN are not set */
>  	err = erofs_init_managed_cache(sb);
> -	if (unlikely(err))
> +	if (err)
>  		return err;
>  
>  	if (!silent)
> diff --git a/fs/erofs/utils.c b/fs/erofs/utils.c
> index 1dd041aa0f5a..d92b3e753a6f 100644
> --- a/fs/erofs/utils.c
> +++ b/fs/erofs/utils.c
> @@ -46,14 +46,14 @@ static int erofs_workgroup_get(struct erofs_workgroup *grp)
>  
>  repeat:
>  	o = erofs_wait_on_workgroup_freezed(grp);
> -	if (unlikely(o <= 0))
> +	if (o <= 0)
>  		return -1;
>  
> -	if (unlikely(atomic_cmpxchg(&grp->refcount, o, o + 1) != o))
> +	if (atomic_cmpxchg(&grp->refcount, o, o + 1) != o)
>  		goto repeat;
>  
>  	/* decrease refcount paired by erofs_workgroup_put */
> -	if (unlikely(o == 1))
> +	if (o == 1)
>  		atomic_long_dec(&erofs_global_shrink_cnt);
>  	return 0;
>  }
> @@ -91,7 +91,7 @@ int erofs_register_workgroup(struct super_block *sb,
>  	int err;
>  
>  	/* grp shouldn't be broken or used before */
> -	if (unlikely(atomic_read(&grp->refcount) != 1)) {
> +	if (atomic_read(&grp->refcount) != 1) {
>  		DBG_BUGON(1);
>  		return -EINVAL;
>  	}
> @@ -113,7 +113,7 @@ int erofs_register_workgroup(struct super_block *sb,
>  	__erofs_workgroup_get(grp);
>  
>  	err = radix_tree_insert(&sbi->workstn_tree, grp->index, grp);
> -	if (unlikely(err))
> +	if (err)
>  		/*
>  		 * it's safe to decrease since the workgroup isn't visible
>  		 * and refcount >= 2 (cannot be freezed).
> @@ -212,7 +212,7 @@ static unsigned long erofs_shrink_workstation(struct erofs_sb_info *sbi,
>  			continue;
>  
>  		++freed;
> -		if (unlikely(!--nr_shrink))
> +		if (!--nr_shrink)
>  			break;
>  	}
>  	xa_unlock(&sbi->workstn_tree);
> diff --git a/fs/erofs/xattr.c b/fs/erofs/xattr.c
> index 7ef8d4bb45cd..620cbc15f4d0 100644
> --- a/fs/erofs/xattr.c
> +++ b/fs/erofs/xattr.c
> @@ -19,7 +19,7 @@ struct xattr_iter {
>  static inline void xattr_iter_end(struct xattr_iter *it, bool atomic)
>  {
>  	/* the only user of kunmap() is 'init_inode_xattrs' */
> -	if (unlikely(!atomic))
> +	if (!atomic)
>  		kunmap(it->page);
>  	else
>  		kunmap_atomic(it->kaddr);
> @@ -72,7 +72,7 @@ static int init_inode_xattrs(struct inode *inode)
>  		ret = -EOPNOTSUPP;
>  		goto out_unlock;
>  	} else if (vi->xattr_isize < sizeof(struct erofs_xattr_ibody_header)) {
> -		if (unlikely(vi->xattr_isize)) {
> +		if (vi->xattr_isize) {
>  			errln("bogus xattr ibody @ nid %llu", vi->nid);
>  			DBG_BUGON(1);
>  			ret = -EFSCORRUPTED;
> @@ -112,7 +112,7 @@ static int init_inode_xattrs(struct inode *inode)
>  	it.ofs += sizeof(struct erofs_xattr_ibody_header);
>  
>  	for (i = 0; i < vi->xattr_shared_count; ++i) {
> -		if (unlikely(it.ofs >= EROFS_BLKSIZ)) {
> +		if (it.ofs >= EROFS_BLKSIZ) {
>  			/* cannot be unaligned */
>  			DBG_BUGON(it.ofs != EROFS_BLKSIZ);
>  			xattr_iter_end(&it, atomic_map);
> @@ -189,7 +189,7 @@ static int inline_xattr_iter_begin(struct xattr_iter *it,
>  	unsigned int xattr_header_sz, inline_xattr_ofs;
>  
>  	xattr_header_sz = inlinexattr_header_size(inode);
> -	if (unlikely(xattr_header_sz >= vi->xattr_isize)) {
> +	if (xattr_header_sz >= vi->xattr_isize) {
>  		DBG_BUGON(xattr_header_sz > vi->xattr_isize);
>  		return -ENOATTR;
>  	}
> @@ -234,7 +234,7 @@ static int xattr_foreach(struct xattr_iter *it,
>  		unsigned int entry_sz = erofs_xattr_entry_size(&entry);
>  
>  		/* xattr on-disk corruption: xattr entry beyond xattr_isize */
> -		if (unlikely(*tlimit < entry_sz)) {
> +		if (*tlimit < entry_sz) {
>  			DBG_BUGON(1);
>  			return -EFSCORRUPTED;
>  		}
> @@ -436,7 +436,7 @@ int erofs_getxattr(struct inode *inode, int index,
>  	int ret;
>  	struct getxattr_iter it;
>  
> -	if (unlikely(!name))
> +	if (!name)
>  		return -EINVAL;
>  
>  	ret = init_inode_xattrs(inode);
> diff --git a/fs/erofs/zdata.c b/fs/erofs/zdata.c
> index b32ad585237c..653bde0a619a 100644
> --- a/fs/erofs/zdata.c
> +++ b/fs/erofs/zdata.c
> @@ -230,7 +230,7 @@ int erofs_try_to_free_all_cached_pages(struct erofs_sb_info *sbi,
>  		if (!trylock_page(page))
>  			return -EBUSY;
>  
> -		if (unlikely(page->mapping != mapping))
> +		if (page->mapping != mapping)
>  			continue;
>  
>  		/* barrier is implied in the following 'unlock_page' */
> @@ -358,7 +358,7 @@ static struct z_erofs_collection *cllookup(struct z_erofs_collector *clt,
>  	}
>  
>  	cl = z_erofs_primarycollection(pcl);
> -	if (unlikely(cl->pageofs != (map->m_la & ~PAGE_MASK))) {
> +	if (cl->pageofs != (map->m_la & ~PAGE_MASK)) {
>  		DBG_BUGON(1);
>  		erofs_workgroup_put(grp);
>  		return ERR_PTR(-EFSCORRUPTED);
> @@ -406,7 +406,7 @@ static struct z_erofs_collection *clregister(struct z_erofs_collector *clt,
>  
>  	/* no available workgroup, let's allocate one */
>  	pcl = kmem_cache_alloc(pcluster_cachep, GFP_NOFS);
> -	if (unlikely(!pcl))
> +	if (!pcl)
>  		return ERR_PTR(-ENOMEM);
>  
>  	init_always(pcl);
> @@ -474,7 +474,7 @@ static int z_erofs_collector_begin(struct z_erofs_collector *clt,
>  	if (!cl) {
>  		cl = clregister(clt, inode, map);
>  
> -		if (unlikely(cl == ERR_PTR(-EAGAIN)))
> +		if (cl == ERR_PTR(-EAGAIN))
>  			goto repeat;
>  	}
>  
> @@ -607,15 +607,15 @@ static int z_erofs_do_read_page(struct z_erofs_decompress_frontend *fe,
>  	map->m_la = offset + cur;
>  	map->m_llen = 0;
>  	err = z_erofs_map_blocks_iter(inode, map, 0);
> -	if (unlikely(err))
> +	if (err)
>  		goto err_out;
>  
>  restart_now:
> -	if (unlikely(!(map->m_flags & EROFS_MAP_MAPPED)))
> +	if (!(map->m_flags & EROFS_MAP_MAPPED))
>  		goto hitted;
>  
>  	err = z_erofs_collector_begin(clt, inode, map);
> -	if (unlikely(err))
> +	if (err)
>  		goto err_out;
>  
>  	/* preload all compressed pages (maybe downgrade role if necessary) */
> @@ -630,7 +630,7 @@ static int z_erofs_do_read_page(struct z_erofs_decompress_frontend *fe,
>  	tight &= (clt->mode >= COLLECT_PRIMARY_HOOKED);
>  hitted:
>  	cur = end - min_t(unsigned int, offset + end - map->m_la, end);
> -	if (unlikely(!(map->m_flags & EROFS_MAP_MAPPED))) {
> +	if (!(map->m_flags & EROFS_MAP_MAPPED)) {
>  		zero_user_segment(page, cur, end);
>  		goto next_part;
>  	}
> @@ -653,11 +653,11 @@ static int z_erofs_do_read_page(struct z_erofs_decompress_frontend *fe,
>  
>  		err = z_erofs_attach_page(clt, newpage,
>  					  Z_EROFS_PAGE_TYPE_EXCLUSIVE);
> -		if (likely(!err))
> +		if (!err)
>  			goto retry;
>  	}
>  
> -	if (unlikely(err))
> +	if (err)
>  		goto err_out;
>  
>  	index = page->index - (map->m_la >> PAGE_SHIFT);
> @@ -723,7 +723,7 @@ static inline void z_erofs_vle_read_endio(struct bio *bio)
>  		DBG_BUGON(PageUptodate(page));
>  		DBG_BUGON(!page->mapping);
>  
> -		if (unlikely(!sbi && !z_erofs_page_is_staging(page))) {
> +		if (!sbi && !z_erofs_page_is_staging(page)) {
>  			sbi = EROFS_SB(page->mapping->host->i_sb);
>  
>  			if (time_to_inject(sbi, FAULT_READ_IO)) {
> @@ -736,7 +736,7 @@ static inline void z_erofs_vle_read_endio(struct bio *bio)
>  		if (sbi)
>  			cachemngd = erofs_page_is_managed(sbi, page);
>  
> -		if (unlikely(err))
> +		if (err)
>  			SetPageError(page);
>  		else if (cachemngd)
>  			SetPageUptodate(page);
> @@ -772,7 +772,7 @@ static int z_erofs_decompress_pcluster(struct super_block *sb,
>  	mutex_lock(&cl->lock);
>  	nr_pages = cl->nr_pages;
>  
> -	if (likely(nr_pages <= Z_EROFS_VMAP_ONSTACK_PAGES)) {
> +	if (nr_pages <= Z_EROFS_VMAP_ONSTACK_PAGES) {
>  		pages = pages_onstack;
>  	} else if (nr_pages <= Z_EROFS_VMAP_GLOBAL_PAGES &&
>  		   mutex_trylock(&z_pagemap_global_lock)) {
> @@ -787,7 +787,7 @@ static int z_erofs_decompress_pcluster(struct super_block *sb,
>  				       gfp_flags);
>  
>  		/* fallback to global pagemap for the lowmem scenario */
> -		if (unlikely(!pages)) {
> +		if (!pages) {
>  			mutex_lock(&z_pagemap_global_lock);
>  			pages = z_pagemap_global;
>  		}
> @@ -823,7 +823,7 @@ static int z_erofs_decompress_pcluster(struct super_block *sb,
>  		 * currently EROFS doesn't support multiref(dedup),
>  		 * so here erroring out one multiref page.
>  		 */
> -		if (unlikely(pages[pagenr])) {
> +		if (pages[pagenr]) {
>  			DBG_BUGON(1);
>  			SetPageError(pages[pagenr]);
>  			z_erofs_onlinepage_endio(pages[pagenr]);
> @@ -847,7 +847,7 @@ static int z_erofs_decompress_pcluster(struct super_block *sb,
>  
>  		if (!z_erofs_page_is_staging(page)) {
>  			if (erofs_page_is_managed(sbi, page)) {
> -				if (unlikely(!PageUptodate(page)))
> +				if (!PageUptodate(page))
>  					err = -EIO;
>  				continue;
>  			}
> @@ -859,7 +859,7 @@ static int z_erofs_decompress_pcluster(struct super_block *sb,
>  			pagenr = z_erofs_onlinepage_index(page);
>  
>  			DBG_BUGON(pagenr >= nr_pages);
> -			if (unlikely(pages[pagenr])) {
> +			if (pages[pagenr]) {
>  				DBG_BUGON(1);
>  				SetPageError(pages[pagenr]);
>  				z_erofs_onlinepage_endio(pages[pagenr]);
> @@ -871,13 +871,13 @@ static int z_erofs_decompress_pcluster(struct super_block *sb,
>  		}
>  
>  		/* PG_error needs checking for inplaced and staging pages */
> -		if (unlikely(PageError(page))) {
> +		if (PageError(page)) {
>  			DBG_BUGON(PageUptodate(page));
>  			err = -EIO;
>  		}
>  	}
>  
> -	if (unlikely(err))
> +	if (err)
>  		goto out;
>  
>  	llen = pcl->length >> Z_EROFS_PCLUSTER_LENGTH_BIT;
> @@ -926,7 +926,7 @@ static int z_erofs_decompress_pcluster(struct super_block *sb,
>  		if (z_erofs_put_stagingpage(pagepool, page))
>  			continue;
>  
> -		if (unlikely(err < 0))
> +		if (err < 0)
>  			SetPageError(page);
>  
>  		z_erofs_onlinepage_endio(page);
> @@ -934,7 +934,7 @@ static int z_erofs_decompress_pcluster(struct super_block *sb,
>  
>  	if (pages == z_pagemap_global)
>  		mutex_unlock(&z_pagemap_global_lock);
> -	else if (unlikely(pages != pages_onstack))
> +	else if (pages != pages_onstack)
>  		kvfree(pages);
>  
>  	cl->nr_pages = 0;
> @@ -1212,7 +1212,7 @@ static bool z_erofs_vle_submit_all(struct super_block *sb,
>  	bool force_submit = false;
>  	unsigned int nr_bios;
>  
> -	if (unlikely(owned_head == Z_EROFS_PCLUSTER_TAIL))
> +	if (owned_head == Z_EROFS_PCLUSTER_TAIL)
>  		return false;
>  
>  	force_submit = false;
> diff --git a/fs/erofs/zmap.c b/fs/erofs/zmap.c
> index 4dc9cec01297..850e0e3d57a8 100644
> --- a/fs/erofs/zmap.c
> +++ b/fs/erofs/zmap.c
> @@ -348,7 +348,7 @@ static int vle_extent_lookback(struct z_erofs_maprecorder *m,
>  
>  	switch (m->type) {
>  	case Z_EROFS_VLE_CLUSTER_TYPE_NONHEAD:
> -		if (unlikely(!m->delta[0])) {
> +		if (!m->delta[0]) {
>  			errln("invalid lookback distance 0 at nid %llu",
>  			      vi->nid);
>  			DBG_BUGON(1);
> @@ -386,7 +386,7 @@ int z_erofs_map_blocks_iter(struct inode *inode,
>  	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)) {
> +	if (map->m_la >= inode->i_size) {
>  		map->m_llen = map->m_la + 1 - inode->i_size;
>  		map->m_la = inode->i_size;
>  		map->m_flags = 0;
> @@ -420,7 +420,7 @@ int z_erofs_map_blocks_iter(struct inode *inode,
>  			break;
>  		}
>  		/* m.lcn should be >= 1 if endoff < m.clusterofs */
> -		if (unlikely(!m.lcn)) {
> +		if (!m.lcn) {
>  			errln("invalid logical cluster 0 at nid %llu",
>  			      vi->nid);
>  			err = -EFSCORRUPTED;
> @@ -433,7 +433,7 @@ int z_erofs_map_blocks_iter(struct inode *inode,
>  	case Z_EROFS_VLE_CLUSTER_TYPE_NONHEAD:
>  		/* get the correspoinding first chunk */
>  		err = vle_extent_lookback(&m, m.delta[0]);
> -		if (unlikely(err))
> +		if (err)
>  			goto unmap_out;
>  		break;
>  	default:
> diff --git a/fs/erofs/zpvec.h b/fs/erofs/zpvec.h
> index bd3cee16491c..58556903aa94 100644
> --- a/fs/erofs/zpvec.h
> +++ b/fs/erofs/zpvec.h
> @@ -111,11 +111,11 @@ static inline bool z_erofs_pagevec_enqueue(struct z_erofs_pagevec_ctor *ctor,
>  					   bool *occupied)
>  {
>  	*occupied = false;
> -	if (unlikely(!ctor->next && type))
> +	if (!ctor->next && type)
>  		if (ctor->index + 1 == ctor->nr)
>  			return false;
>  
> -	if (unlikely(ctor->index >= ctor->nr))
> +	if (ctor->index >= ctor->nr)
>  		z_erofs_pagevec_ctor_pagedown(ctor, false);
>  
>  	/* exclusive page type must be 0 */
> @@ -137,7 +137,7 @@ z_erofs_pagevec_dequeue(struct z_erofs_pagevec_ctor *ctor,
>  {
>  	erofs_vtptr_t t;
>  
> -	if (unlikely(ctor->index >= ctor->nr)) {
> +	if (ctor->index >= ctor->nr) {
>  		DBG_BUGON(!ctor->next);
>  		z_erofs_pagevec_ctor_pagedown(ctor, true);
>  	}
> 

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v3 7/7] erofs: redundant assignment in __erofs_get_meta_page()
  2019-08-30  3:36         ` [PATCH v3 7/7] erofs: redundant assignment in __erofs_get_meta_page() Gao Xiang
@ 2019-08-30  6:25           ` Chao Yu
  2019-08-30 16:28           ` Christoph Hellwig
  1 sibling, 0 replies; 38+ messages in thread
From: Chao Yu @ 2019-08-30  6:25 UTC (permalink / raw)
  To: Gao Xiang, Dan Carpenter, Christoph Hellwig, Joe Perches,
	Greg Kroah-Hartman, devel
  Cc: linux-erofs, LKML, weidu.du, Miao Xie

On 2019/8/30 11:36, Gao Xiang wrote:
> As Joe Perches suggested [1],
>  		err = bio_add_page(bio, page, PAGE_SIZE, 0);
> -		if (unlikely(err != PAGE_SIZE)) {
> +		if (err != PAGE_SIZE) {
>  			err = -EFAULT;
>  			goto err_out;
>  		}
> 
> The initial assignment to err is odd as it's not
> actually an error value -E<FOO> but a int size
> from a unsigned int len.
> 
> Here the return is either 0 or PAGE_SIZE.
> 
> This would be more legible to me as:
> 
> 		if (bio_add_page(bio, page, PAGE_SIZE, 0) != PAGE_SIZE) {
> 			err = -EFAULT;
> 			goto err_out;
> 		}
> 
> [1] https://lore.kernel.org/r/74c4784319b40deabfbaea92468f7e3ef44f1c96.camel@perches.com/
> Reported-by: Joe Perches <joe@perches.com>
> Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>

Reviewed-by: Chao Yu <yuchao0@huawei.com>

Thanks,

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v3 6/7] erofs: remove all likely/unlikely annotations
  2019-08-30  6:25           ` Chao Yu
@ 2019-08-30  6:31             ` Gao Xiang
  2019-08-30  6:43               ` Chao Yu
  2019-08-30 11:55             ` Dan Carpenter
  1 sibling, 1 reply; 38+ messages in thread
From: Gao Xiang @ 2019-08-30  6:31 UTC (permalink / raw)
  To: Chao Yu
  Cc: devel, Greg Kroah-Hartman, Miao Xie, LKML, Christoph Hellwig,
	weidu.du, Joe Perches, linux-erofs, Dan Carpenter

Hi Chao,

On Fri, Aug 30, 2019 at 02:25:13PM +0800, Chao Yu wrote:
> On 2019/8/30 11:36, Gao Xiang wrote:
> > As Dan Carpenter suggested [1], I have to remove
> > all erofs likely/unlikely annotations.
> > 
> > [1] https://lore.kernel.org/linux-fsdevel/20190829154346.GK23584@kadam/
> > Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> > Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>
> 
> I suggest we can modify this by following good example rather than removing them
> all, at least, the fuzzed random fields of disk layout handling should be very
> rare case, I guess it's fine to use unlikely.
> 
> To Dan, thoughts?

I think let's stop talking this anymore, I think we can do such
a commit now and I think "folks have their own thoughts on it".

Could you kindly review and check this one? I did it by 'sed', could
you double check it?

Thanks,
Gao Xiang

> 
> Thanks,
> 
> > ---
> > no change.
> > 
> >  fs/erofs/data.c         | 22 ++++++++++-----------
> >  fs/erofs/decompressor.c |  2 +-
> >  fs/erofs/dir.c          | 14 ++++++-------
> >  fs/erofs/inode.c        | 10 +++++-----
> >  fs/erofs/internal.h     |  4 ++--
> >  fs/erofs/namei.c        | 14 ++++++-------
> >  fs/erofs/super.c        | 16 +++++++--------
> >  fs/erofs/utils.c        | 12 +++++------
> >  fs/erofs/xattr.c        | 12 +++++------
> >  fs/erofs/zdata.c        | 44 ++++++++++++++++++++---------------------
> >  fs/erofs/zmap.c         |  8 ++++----
> >  fs/erofs/zpvec.h        |  6 +++---
> >  12 files changed, 82 insertions(+), 82 deletions(-)
> > 
> > diff --git a/fs/erofs/data.c b/fs/erofs/data.c
> > index fda16ec8863e..0f2f1a839372 100644
> > --- a/fs/erofs/data.c
> > +++ b/fs/erofs/data.c
> > @@ -27,7 +27,7 @@ static inline void read_endio(struct bio *bio)
> >  		/* page is already locked */
> >  		DBG_BUGON(PageUptodate(page));
> >  
> > -		if (unlikely(err))
> > +		if (err)
> >  			SetPageError(page);
> >  		else
> >  			SetPageUptodate(page);
> > @@ -53,7 +53,7 @@ struct page *__erofs_get_meta_page(struct super_block *sb,
> >  
> >  repeat:
> >  	page = find_or_create_page(mapping, blkaddr, gfp);
> > -	if (unlikely(!page)) {
> > +	if (!page) {
> >  		DBG_BUGON(nofail);
> >  		return ERR_PTR(-ENOMEM);
> >  	}
> > @@ -70,7 +70,7 @@ struct page *__erofs_get_meta_page(struct super_block *sb,
> >  		}
> >  
> >  		err = bio_add_page(bio, page, PAGE_SIZE, 0);
> > -		if (unlikely(err != PAGE_SIZE)) {
> > +		if (err != PAGE_SIZE) {
> >  			err = -EFAULT;
> >  			goto err_out;
> >  		}
> > @@ -81,7 +81,7 @@ struct page *__erofs_get_meta_page(struct super_block *sb,
> >  		lock_page(page);
> >  
> >  		/* this page has been truncated by others */
> > -		if (unlikely(page->mapping != mapping)) {
> > +		if (page->mapping != mapping) {
> >  unlock_repeat:
> >  			unlock_page(page);
> >  			put_page(page);
> > @@ -89,7 +89,7 @@ struct page *__erofs_get_meta_page(struct super_block *sb,
> >  		}
> >  
> >  		/* more likely a read error */
> > -		if (unlikely(!PageUptodate(page))) {
> > +		if (!PageUptodate(page)) {
> >  			if (io_retries) {
> >  				--io_retries;
> >  				goto unlock_repeat;
> > @@ -120,7 +120,7 @@ static int erofs_map_blocks_flatmode(struct inode *inode,
> >  	nblocks = DIV_ROUND_UP(inode->i_size, PAGE_SIZE);
> >  	lastblk = nblocks - is_inode_flat_inline(inode);
> >  
> > -	if (unlikely(offset >= inode->i_size)) {
> > +	if (offset >= inode->i_size) {
> >  		/* leave out-of-bound access unmapped */
> >  		map->m_flags = 0;
> >  		map->m_plen = 0;
> > @@ -170,7 +170,7 @@ static int erofs_map_blocks_flatmode(struct inode *inode,
> >  int erofs_map_blocks(struct inode *inode,
> >  		     struct erofs_map_blocks *map, int flags)
> >  {
> > -	if (unlikely(is_inode_layout_compression(inode))) {
> > +	if (is_inode_layout_compression(inode)) {
> >  		int err = z_erofs_map_blocks_iter(inode, map, flags);
> >  
> >  		if (map->mpage) {
> > @@ -218,11 +218,11 @@ static inline struct bio *erofs_read_raw_page(struct bio *bio,
> >  		unsigned int blkoff;
> >  
> >  		err = erofs_map_blocks(inode, &map, EROFS_GET_BLOCKS_RAW);
> > -		if (unlikely(err))
> > +		if (err)
> >  			goto err_out;
> >  
> >  		/* zero out the holed page */
> > -		if (unlikely(!(map.m_flags & EROFS_MAP_MAPPED))) {
> > +		if (!(map.m_flags & EROFS_MAP_MAPPED)) {
> >  			zero_user_segment(page, 0, PAGE_SIZE);
> >  			SetPageUptodate(page);
> >  
> > @@ -315,7 +315,7 @@ static inline struct bio *erofs_read_raw_page(struct bio *bio,
> >  submit_bio_out:
> >  		__submit_bio(bio, REQ_OP_READ, 0);
> >  
> > -	return unlikely(err) ? ERR_PTR(err) : NULL;
> > +	return err ? ERR_PTR(err) : NULL;
> >  }
> >  
> >  /*
> > @@ -377,7 +377,7 @@ static int erofs_raw_access_readpages(struct file *filp,
> >  	DBG_BUGON(!list_empty(pages));
> >  
> >  	/* the rare case (end in gaps) */
> > -	if (unlikely(bio))
> > +	if (bio)
> >  		__submit_bio(bio, REQ_OP_READ, 0);
> >  	return 0;
> >  }
> > diff --git a/fs/erofs/decompressor.c b/fs/erofs/decompressor.c
> > index 5f4b7f302863..df349888f911 100644
> > --- a/fs/erofs/decompressor.c
> > +++ b/fs/erofs/decompressor.c
> > @@ -78,7 +78,7 @@ static int lz4_prepare_destpages(struct z_erofs_decompress_req *rq,
> >  			get_page(victim);
> >  		} else {
> >  			victim = erofs_allocpage(pagepool, GFP_KERNEL, false);
> > -			if (unlikely(!victim))
> > +			if (!victim)
> >  				return -ENOMEM;
> >  			victim->mapping = Z_EROFS_MAPPING_STAGING;
> >  		}
> > diff --git a/fs/erofs/dir.c b/fs/erofs/dir.c
> > index 1976e60e5174..6a5b43f7fb29 100644
> > --- a/fs/erofs/dir.c
> > +++ b/fs/erofs/dir.c
> > @@ -45,8 +45,8 @@ static int erofs_fill_dentries(struct inode *dir, struct dir_context *ctx,
> >  			de_namelen = le16_to_cpu(de[1].nameoff) - nameoff;
> >  
> >  		/* a corrupted entry is found */
> > -		if (unlikely(nameoff + de_namelen > maxsize ||
> > -			     de_namelen > EROFS_NAME_LEN)) {
> > +		if (nameoff + de_namelen > maxsize ||
> > +		    de_namelen > EROFS_NAME_LEN) {
> >  			errln("bogus dirent @ nid %llu", EROFS_V(dir)->nid);
> >  			DBG_BUGON(1);
> >  			return -EFSCORRUPTED;
> > @@ -94,8 +94,8 @@ static int erofs_readdir(struct file *f, struct dir_context *ctx)
> >  
> >  		nameoff = le16_to_cpu(de->nameoff);
> >  
> > -		if (unlikely(nameoff < sizeof(struct erofs_dirent) ||
> > -			     nameoff >= PAGE_SIZE)) {
> > +		if (nameoff < sizeof(struct erofs_dirent) ||
> > +		    nameoff >= PAGE_SIZE) {
> >  			errln("%s, invalid de[0].nameoff %u @ nid %llu",
> >  			      __func__, nameoff, EROFS_V(dir)->nid);
> >  			err = -EFSCORRUPTED;
> > @@ -106,11 +106,11 @@ static int erofs_readdir(struct file *f, struct dir_context *ctx)
> >  				dirsize - ctx->pos + ofs, PAGE_SIZE);
> >  
> >  		/* search dirents at the arbitrary position */
> > -		if (unlikely(initial)) {
> > +		if (initial) {
> >  			initial = false;
> >  
> >  			ofs = roundup(ofs, sizeof(struct erofs_dirent));
> > -			if (unlikely(ofs >= nameoff))
> > +			if (ofs >= nameoff)
> >  				goto skip_this;
> >  		}
> >  
> > @@ -123,7 +123,7 @@ static int erofs_readdir(struct file *f, struct dir_context *ctx)
> >  
> >  		ctx->pos = blknr_to_addr(i) + ofs;
> >  
> > -		if (unlikely(err))
> > +		if (err)
> >  			break;
> >  		++i;
> >  		ofs = 0;
> > diff --git a/fs/erofs/inode.c b/fs/erofs/inode.c
> > index cf31554075c9..871a6d8ed6f9 100644
> > --- a/fs/erofs/inode.c
> > +++ b/fs/erofs/inode.c
> > @@ -18,7 +18,7 @@ static int read_inode(struct inode *inode, void *data)
> >  
> >  	vi->datamode = __inode_data_mapping(advise);
> >  
> > -	if (unlikely(vi->datamode >= EROFS_INODE_LAYOUT_MAX)) {
> > +	if (vi->datamode >= EROFS_INODE_LAYOUT_MAX) {
> >  		errln("unsupported data mapping %u of nid %llu",
> >  		      vi->datamode, vi->nid);
> >  		DBG_BUGON(1);
> > @@ -133,13 +133,13 @@ static int fill_inline_data(struct inode *inode, void *data,
> >  	if (S_ISLNK(inode->i_mode) && inode->i_size < PAGE_SIZE) {
> >  		char *lnk = erofs_kmalloc(sbi, inode->i_size + 1, GFP_KERNEL);
> >  
> > -		if (unlikely(!lnk))
> > +		if (!lnk)
> >  			return -ENOMEM;
> >  
> >  		m_pofs += vi->inode_isize + vi->xattr_isize;
> >  
> >  		/* inline symlink data shouldn't across page boundary as well */
> > -		if (unlikely(m_pofs + inode->i_size > PAGE_SIZE)) {
> > +		if (m_pofs + inode->i_size > PAGE_SIZE) {
> >  			kfree(lnk);
> >  			errln("inline data cross block boundary @ nid %llu",
> >  			      vi->nid);
> > @@ -268,7 +268,7 @@ struct inode *erofs_iget(struct super_block *sb,
> >  {
> >  	struct inode *inode = erofs_iget_locked(sb, nid);
> >  
> > -	if (unlikely(!inode))
> > +	if (!inode)
> >  		return ERR_PTR(-ENOMEM);
> >  
> >  	if (inode->i_state & I_NEW) {
> > @@ -278,7 +278,7 @@ struct inode *erofs_iget(struct super_block *sb,
> >  		vi->nid = nid;
> >  
> >  		err = fill_inode(inode, isdir);
> > -		if (likely(!err))
> > +		if (!err)
> >  			unlock_new_inode(inode);
> >  		else {
> >  			iget_failed(inode);
> > diff --git a/fs/erofs/internal.h b/fs/erofs/internal.h
> > index 620b73fcc416..141ea424587d 100644
> > --- a/fs/erofs/internal.h
> > +++ b/fs/erofs/internal.h
> > @@ -424,7 +424,7 @@ static inline struct bio *erofs_grab_bio(struct super_block *sb,
> >  	do {
> >  		if (nr_pages == 1) {
> >  			bio = bio_alloc(gfp | (nofail ? __GFP_NOFAIL : 0), 1);
> > -			if (unlikely(!bio)) {
> > +			if (!bio) {
> >  				DBG_BUGON(nofail);
> >  				return ERR_PTR(-ENOMEM);
> >  			}
> > @@ -432,7 +432,7 @@ static inline struct bio *erofs_grab_bio(struct super_block *sb,
> >  		}
> >  		bio = bio_alloc(gfp, nr_pages);
> >  		nr_pages /= 2;
> > -	} while (unlikely(!bio));
> > +	} while (!bio);
> >  
> >  	bio->bi_end_io = endio;
> >  	bio_set_dev(bio, sb->s_bdev);
> > diff --git a/fs/erofs/namei.c b/fs/erofs/namei.c
> > index 8832b5d95d91..c1068ad0535e 100644
> > --- a/fs/erofs/namei.c
> > +++ b/fs/erofs/namei.c
> > @@ -64,7 +64,7 @@ static struct erofs_dirent *find_target_dirent(struct erofs_qstr *name,
> >  		unsigned int matched = min(startprfx, endprfx);
> >  		struct erofs_qstr dname = {
> >  			.name = data + nameoff,
> > -			.end = unlikely(mid >= ndirents - 1) ?
> > +			.end = mid >= ndirents - 1 ?
> >  				data + dirblksize :
> >  				data + nameoff_from_disk(de[mid + 1].nameoff,
> >  							 dirblksize)
> > @@ -73,7 +73,7 @@ static struct erofs_dirent *find_target_dirent(struct erofs_qstr *name,
> >  		/* string comparison without already matched prefix */
> >  		int ret = dirnamecmp(name, &dname, &matched);
> >  
> > -		if (unlikely(!ret)) {
> > +		if (!ret) {
> >  			return de + mid;
> >  		} else if (ret > 0) {
> >  			head = mid + 1;
> > @@ -113,7 +113,7 @@ static struct page *find_target_block_classic(struct inode *dir,
> >  			unsigned int matched;
> >  			struct erofs_qstr dname;
> >  
> > -			if (unlikely(!ndirents)) {
> > +			if (!ndirents) {
> >  				kunmap_atomic(de);
> >  				put_page(page);
> >  				errln("corrupted dir block %d @ nid %llu",
> > @@ -137,7 +137,7 @@ static struct page *find_target_block_classic(struct inode *dir,
> >  			diff = dirnamecmp(name, &dname, &matched);
> >  			kunmap_atomic(de);
> >  
> > -			if (unlikely(!diff)) {
> > +			if (!diff) {
> >  				*_ndirents = 0;
> >  				goto out;
> >  			} else if (diff > 0) {
> > @@ -174,7 +174,7 @@ int erofs_namei(struct inode *dir,
> >  	struct erofs_dirent *de;
> >  	struct erofs_qstr qn;
> >  
> > -	if (unlikely(!dir->i_size))
> > +	if (!dir->i_size)
> >  		return -ENOENT;
> >  
> >  	qn.name = name->name;
> > @@ -221,7 +221,7 @@ static struct dentry *erofs_lookup(struct inode *dir,
> >  	trace_erofs_lookup(dir, dentry, flags);
> >  
> >  	/* file name exceeds fs limit */
> > -	if (unlikely(dentry->d_name.len > EROFS_NAME_LEN))
> > +	if (dentry->d_name.len > EROFS_NAME_LEN)
> >  		return ERR_PTR(-ENAMETOOLONG);
> >  
> >  	/* false uninitialized warnings on gcc 4.8.x */
> > @@ -230,7 +230,7 @@ static struct dentry *erofs_lookup(struct inode *dir,
> >  	if (err == -ENOENT) {
> >  		/* negative dentry */
> >  		inode = NULL;
> > -	} else if (unlikely(err)) {
> > +	} else if (err) {
> >  		inode = ERR_PTR(err);
> >  	} else {
> >  		debugln("%s, %s (nid %llu) found, d_type %u", __func__,
> > diff --git a/fs/erofs/super.c b/fs/erofs/super.c
> > index 556aae5426d6..0c412de33315 100644
> > --- a/fs/erofs/super.c
> > +++ b/fs/erofs/super.c
> > @@ -92,7 +92,7 @@ static int superblock_read(struct super_block *sb)
> >  
> >  	blkszbits = layout->blkszbits;
> >  	/* 9(512 bytes) + LOG_SECTORS_PER_BLOCK == LOG_BLOCK_SIZE */
> > -	if (unlikely(blkszbits != LOG_BLOCK_SIZE)) {
> > +	if (blkszbits != LOG_BLOCK_SIZE) {
> >  		errln("blksize %u isn't supported on this platform",
> >  		      1 << blkszbits);
> >  		goto out;
> > @@ -364,7 +364,7 @@ static int erofs_init_managed_cache(struct super_block *sb)
> >  	struct erofs_sb_info *const sbi = EROFS_SB(sb);
> >  	struct inode *const inode = new_inode(sb);
> >  
> > -	if (unlikely(!inode))
> > +	if (!inode)
> >  		return -ENOMEM;
> >  
> >  	set_nlink(inode, 1);
> > @@ -391,13 +391,13 @@ static int erofs_fill_super(struct super_block *sb, void *data, int silent)
> >  
> >  	sb->s_magic = EROFS_SUPER_MAGIC;
> >  
> > -	if (unlikely(!sb_set_blocksize(sb, EROFS_BLKSIZ))) {
> > +	if (!sb_set_blocksize(sb, EROFS_BLKSIZ)) {
> >  		errln("failed to set erofs blksize");
> >  		return -EINVAL;
> >  	}
> >  
> >  	sbi = kzalloc(sizeof(*sbi), GFP_KERNEL);
> > -	if (unlikely(!sbi))
> > +	if (!sbi)
> >  		return -ENOMEM;
> >  
> >  	sb->s_fs_info = sbi;
> > @@ -418,7 +418,7 @@ static int erofs_fill_super(struct super_block *sb, void *data, int silent)
> >  	default_options(sbi);
> >  
> >  	err = parse_options(sb, data);
> > -	if (unlikely(err))
> > +	if (err)
> >  		return err;
> >  
> >  	if (!silent)
> > @@ -438,7 +438,7 @@ static int erofs_fill_super(struct super_block *sb, void *data, int silent)
> >  	if (IS_ERR(inode))
> >  		return PTR_ERR(inode);
> >  
> > -	if (unlikely(!S_ISDIR(inode->i_mode))) {
> > +	if (!S_ISDIR(inode->i_mode)) {
> >  		errln("rootino(nid %llu) is not a directory(i_mode %o)",
> >  		      ROOT_NID(sbi), inode->i_mode);
> >  		iput(inode);
> > @@ -446,13 +446,13 @@ static int erofs_fill_super(struct super_block *sb, void *data, int silent)
> >  	}
> >  
> >  	sb->s_root = d_make_root(inode);
> > -	if (unlikely(!sb->s_root))
> > +	if (!sb->s_root)
> >  		return -ENOMEM;
> >  
> >  	erofs_shrinker_register(sb);
> >  	/* sb->s_umount is already locked, SB_ACTIVE and SB_BORN are not set */
> >  	err = erofs_init_managed_cache(sb);
> > -	if (unlikely(err))
> > +	if (err)
> >  		return err;
> >  
> >  	if (!silent)
> > diff --git a/fs/erofs/utils.c b/fs/erofs/utils.c
> > index 1dd041aa0f5a..d92b3e753a6f 100644
> > --- a/fs/erofs/utils.c
> > +++ b/fs/erofs/utils.c
> > @@ -46,14 +46,14 @@ static int erofs_workgroup_get(struct erofs_workgroup *grp)
> >  
> >  repeat:
> >  	o = erofs_wait_on_workgroup_freezed(grp);
> > -	if (unlikely(o <= 0))
> > +	if (o <= 0)
> >  		return -1;
> >  
> > -	if (unlikely(atomic_cmpxchg(&grp->refcount, o, o + 1) != o))
> > +	if (atomic_cmpxchg(&grp->refcount, o, o + 1) != o)
> >  		goto repeat;
> >  
> >  	/* decrease refcount paired by erofs_workgroup_put */
> > -	if (unlikely(o == 1))
> > +	if (o == 1)
> >  		atomic_long_dec(&erofs_global_shrink_cnt);
> >  	return 0;
> >  }
> > @@ -91,7 +91,7 @@ int erofs_register_workgroup(struct super_block *sb,
> >  	int err;
> >  
> >  	/* grp shouldn't be broken or used before */
> > -	if (unlikely(atomic_read(&grp->refcount) != 1)) {
> > +	if (atomic_read(&grp->refcount) != 1) {
> >  		DBG_BUGON(1);
> >  		return -EINVAL;
> >  	}
> > @@ -113,7 +113,7 @@ int erofs_register_workgroup(struct super_block *sb,
> >  	__erofs_workgroup_get(grp);
> >  
> >  	err = radix_tree_insert(&sbi->workstn_tree, grp->index, grp);
> > -	if (unlikely(err))
> > +	if (err)
> >  		/*
> >  		 * it's safe to decrease since the workgroup isn't visible
> >  		 * and refcount >= 2 (cannot be freezed).
> > @@ -212,7 +212,7 @@ static unsigned long erofs_shrink_workstation(struct erofs_sb_info *sbi,
> >  			continue;
> >  
> >  		++freed;
> > -		if (unlikely(!--nr_shrink))
> > +		if (!--nr_shrink)
> >  			break;
> >  	}
> >  	xa_unlock(&sbi->workstn_tree);
> > diff --git a/fs/erofs/xattr.c b/fs/erofs/xattr.c
> > index 7ef8d4bb45cd..620cbc15f4d0 100644
> > --- a/fs/erofs/xattr.c
> > +++ b/fs/erofs/xattr.c
> > @@ -19,7 +19,7 @@ struct xattr_iter {
> >  static inline void xattr_iter_end(struct xattr_iter *it, bool atomic)
> >  {
> >  	/* the only user of kunmap() is 'init_inode_xattrs' */
> > -	if (unlikely(!atomic))
> > +	if (!atomic)
> >  		kunmap(it->page);
> >  	else
> >  		kunmap_atomic(it->kaddr);
> > @@ -72,7 +72,7 @@ static int init_inode_xattrs(struct inode *inode)
> >  		ret = -EOPNOTSUPP;
> >  		goto out_unlock;
> >  	} else if (vi->xattr_isize < sizeof(struct erofs_xattr_ibody_header)) {
> > -		if (unlikely(vi->xattr_isize)) {
> > +		if (vi->xattr_isize) {
> >  			errln("bogus xattr ibody @ nid %llu", vi->nid);
> >  			DBG_BUGON(1);
> >  			ret = -EFSCORRUPTED;
> > @@ -112,7 +112,7 @@ static int init_inode_xattrs(struct inode *inode)
> >  	it.ofs += sizeof(struct erofs_xattr_ibody_header);
> >  
> >  	for (i = 0; i < vi->xattr_shared_count; ++i) {
> > -		if (unlikely(it.ofs >= EROFS_BLKSIZ)) {
> > +		if (it.ofs >= EROFS_BLKSIZ) {
> >  			/* cannot be unaligned */
> >  			DBG_BUGON(it.ofs != EROFS_BLKSIZ);
> >  			xattr_iter_end(&it, atomic_map);
> > @@ -189,7 +189,7 @@ static int inline_xattr_iter_begin(struct xattr_iter *it,
> >  	unsigned int xattr_header_sz, inline_xattr_ofs;
> >  
> >  	xattr_header_sz = inlinexattr_header_size(inode);
> > -	if (unlikely(xattr_header_sz >= vi->xattr_isize)) {
> > +	if (xattr_header_sz >= vi->xattr_isize) {
> >  		DBG_BUGON(xattr_header_sz > vi->xattr_isize);
> >  		return -ENOATTR;
> >  	}
> > @@ -234,7 +234,7 @@ static int xattr_foreach(struct xattr_iter *it,
> >  		unsigned int entry_sz = erofs_xattr_entry_size(&entry);
> >  
> >  		/* xattr on-disk corruption: xattr entry beyond xattr_isize */
> > -		if (unlikely(*tlimit < entry_sz)) {
> > +		if (*tlimit < entry_sz) {
> >  			DBG_BUGON(1);
> >  			return -EFSCORRUPTED;
> >  		}
> > @@ -436,7 +436,7 @@ int erofs_getxattr(struct inode *inode, int index,
> >  	int ret;
> >  	struct getxattr_iter it;
> >  
> > -	if (unlikely(!name))
> > +	if (!name)
> >  		return -EINVAL;
> >  
> >  	ret = init_inode_xattrs(inode);
> > diff --git a/fs/erofs/zdata.c b/fs/erofs/zdata.c
> > index b32ad585237c..653bde0a619a 100644
> > --- a/fs/erofs/zdata.c
> > +++ b/fs/erofs/zdata.c
> > @@ -230,7 +230,7 @@ int erofs_try_to_free_all_cached_pages(struct erofs_sb_info *sbi,
> >  		if (!trylock_page(page))
> >  			return -EBUSY;
> >  
> > -		if (unlikely(page->mapping != mapping))
> > +		if (page->mapping != mapping)
> >  			continue;
> >  
> >  		/* barrier is implied in the following 'unlock_page' */
> > @@ -358,7 +358,7 @@ static struct z_erofs_collection *cllookup(struct z_erofs_collector *clt,
> >  	}
> >  
> >  	cl = z_erofs_primarycollection(pcl);
> > -	if (unlikely(cl->pageofs != (map->m_la & ~PAGE_MASK))) {
> > +	if (cl->pageofs != (map->m_la & ~PAGE_MASK)) {
> >  		DBG_BUGON(1);
> >  		erofs_workgroup_put(grp);
> >  		return ERR_PTR(-EFSCORRUPTED);
> > @@ -406,7 +406,7 @@ static struct z_erofs_collection *clregister(struct z_erofs_collector *clt,
> >  
> >  	/* no available workgroup, let's allocate one */
> >  	pcl = kmem_cache_alloc(pcluster_cachep, GFP_NOFS);
> > -	if (unlikely(!pcl))
> > +	if (!pcl)
> >  		return ERR_PTR(-ENOMEM);
> >  
> >  	init_always(pcl);
> > @@ -474,7 +474,7 @@ static int z_erofs_collector_begin(struct z_erofs_collector *clt,
> >  	if (!cl) {
> >  		cl = clregister(clt, inode, map);
> >  
> > -		if (unlikely(cl == ERR_PTR(-EAGAIN)))
> > +		if (cl == ERR_PTR(-EAGAIN))
> >  			goto repeat;
> >  	}
> >  
> > @@ -607,15 +607,15 @@ static int z_erofs_do_read_page(struct z_erofs_decompress_frontend *fe,
> >  	map->m_la = offset + cur;
> >  	map->m_llen = 0;
> >  	err = z_erofs_map_blocks_iter(inode, map, 0);
> > -	if (unlikely(err))
> > +	if (err)
> >  		goto err_out;
> >  
> >  restart_now:
> > -	if (unlikely(!(map->m_flags & EROFS_MAP_MAPPED)))
> > +	if (!(map->m_flags & EROFS_MAP_MAPPED))
> >  		goto hitted;
> >  
> >  	err = z_erofs_collector_begin(clt, inode, map);
> > -	if (unlikely(err))
> > +	if (err)
> >  		goto err_out;
> >  
> >  	/* preload all compressed pages (maybe downgrade role if necessary) */
> > @@ -630,7 +630,7 @@ static int z_erofs_do_read_page(struct z_erofs_decompress_frontend *fe,
> >  	tight &= (clt->mode >= COLLECT_PRIMARY_HOOKED);
> >  hitted:
> >  	cur = end - min_t(unsigned int, offset + end - map->m_la, end);
> > -	if (unlikely(!(map->m_flags & EROFS_MAP_MAPPED))) {
> > +	if (!(map->m_flags & EROFS_MAP_MAPPED)) {
> >  		zero_user_segment(page, cur, end);
> >  		goto next_part;
> >  	}
> > @@ -653,11 +653,11 @@ static int z_erofs_do_read_page(struct z_erofs_decompress_frontend *fe,
> >  
> >  		err = z_erofs_attach_page(clt, newpage,
> >  					  Z_EROFS_PAGE_TYPE_EXCLUSIVE);
> > -		if (likely(!err))
> > +		if (!err)
> >  			goto retry;
> >  	}
> >  
> > -	if (unlikely(err))
> > +	if (err)
> >  		goto err_out;
> >  
> >  	index = page->index - (map->m_la >> PAGE_SHIFT);
> > @@ -723,7 +723,7 @@ static inline void z_erofs_vle_read_endio(struct bio *bio)
> >  		DBG_BUGON(PageUptodate(page));
> >  		DBG_BUGON(!page->mapping);
> >  
> > -		if (unlikely(!sbi && !z_erofs_page_is_staging(page))) {
> > +		if (!sbi && !z_erofs_page_is_staging(page)) {
> >  			sbi = EROFS_SB(page->mapping->host->i_sb);
> >  
> >  			if (time_to_inject(sbi, FAULT_READ_IO)) {
> > @@ -736,7 +736,7 @@ static inline void z_erofs_vle_read_endio(struct bio *bio)
> >  		if (sbi)
> >  			cachemngd = erofs_page_is_managed(sbi, page);
> >  
> > -		if (unlikely(err))
> > +		if (err)
> >  			SetPageError(page);
> >  		else if (cachemngd)
> >  			SetPageUptodate(page);
> > @@ -772,7 +772,7 @@ static int z_erofs_decompress_pcluster(struct super_block *sb,
> >  	mutex_lock(&cl->lock);
> >  	nr_pages = cl->nr_pages;
> >  
> > -	if (likely(nr_pages <= Z_EROFS_VMAP_ONSTACK_PAGES)) {
> > +	if (nr_pages <= Z_EROFS_VMAP_ONSTACK_PAGES) {
> >  		pages = pages_onstack;
> >  	} else if (nr_pages <= Z_EROFS_VMAP_GLOBAL_PAGES &&
> >  		   mutex_trylock(&z_pagemap_global_lock)) {
> > @@ -787,7 +787,7 @@ static int z_erofs_decompress_pcluster(struct super_block *sb,
> >  				       gfp_flags);
> >  
> >  		/* fallback to global pagemap for the lowmem scenario */
> > -		if (unlikely(!pages)) {
> > +		if (!pages) {
> >  			mutex_lock(&z_pagemap_global_lock);
> >  			pages = z_pagemap_global;
> >  		}
> > @@ -823,7 +823,7 @@ static int z_erofs_decompress_pcluster(struct super_block *sb,
> >  		 * currently EROFS doesn't support multiref(dedup),
> >  		 * so here erroring out one multiref page.
> >  		 */
> > -		if (unlikely(pages[pagenr])) {
> > +		if (pages[pagenr]) {
> >  			DBG_BUGON(1);
> >  			SetPageError(pages[pagenr]);
> >  			z_erofs_onlinepage_endio(pages[pagenr]);
> > @@ -847,7 +847,7 @@ static int z_erofs_decompress_pcluster(struct super_block *sb,
> >  
> >  		if (!z_erofs_page_is_staging(page)) {
> >  			if (erofs_page_is_managed(sbi, page)) {
> > -				if (unlikely(!PageUptodate(page)))
> > +				if (!PageUptodate(page))
> >  					err = -EIO;
> >  				continue;
> >  			}
> > @@ -859,7 +859,7 @@ static int z_erofs_decompress_pcluster(struct super_block *sb,
> >  			pagenr = z_erofs_onlinepage_index(page);
> >  
> >  			DBG_BUGON(pagenr >= nr_pages);
> > -			if (unlikely(pages[pagenr])) {
> > +			if (pages[pagenr]) {
> >  				DBG_BUGON(1);
> >  				SetPageError(pages[pagenr]);
> >  				z_erofs_onlinepage_endio(pages[pagenr]);
> > @@ -871,13 +871,13 @@ static int z_erofs_decompress_pcluster(struct super_block *sb,
> >  		}
> >  
> >  		/* PG_error needs checking for inplaced and staging pages */
> > -		if (unlikely(PageError(page))) {
> > +		if (PageError(page)) {
> >  			DBG_BUGON(PageUptodate(page));
> >  			err = -EIO;
> >  		}
> >  	}
> >  
> > -	if (unlikely(err))
> > +	if (err)
> >  		goto out;
> >  
> >  	llen = pcl->length >> Z_EROFS_PCLUSTER_LENGTH_BIT;
> > @@ -926,7 +926,7 @@ static int z_erofs_decompress_pcluster(struct super_block *sb,
> >  		if (z_erofs_put_stagingpage(pagepool, page))
> >  			continue;
> >  
> > -		if (unlikely(err < 0))
> > +		if (err < 0)
> >  			SetPageError(page);
> >  
> >  		z_erofs_onlinepage_endio(page);
> > @@ -934,7 +934,7 @@ static int z_erofs_decompress_pcluster(struct super_block *sb,
> >  
> >  	if (pages == z_pagemap_global)
> >  		mutex_unlock(&z_pagemap_global_lock);
> > -	else if (unlikely(pages != pages_onstack))
> > +	else if (pages != pages_onstack)
> >  		kvfree(pages);
> >  
> >  	cl->nr_pages = 0;
> > @@ -1212,7 +1212,7 @@ static bool z_erofs_vle_submit_all(struct super_block *sb,
> >  	bool force_submit = false;
> >  	unsigned int nr_bios;
> >  
> > -	if (unlikely(owned_head == Z_EROFS_PCLUSTER_TAIL))
> > +	if (owned_head == Z_EROFS_PCLUSTER_TAIL)
> >  		return false;
> >  
> >  	force_submit = false;
> > diff --git a/fs/erofs/zmap.c b/fs/erofs/zmap.c
> > index 4dc9cec01297..850e0e3d57a8 100644
> > --- a/fs/erofs/zmap.c
> > +++ b/fs/erofs/zmap.c
> > @@ -348,7 +348,7 @@ static int vle_extent_lookback(struct z_erofs_maprecorder *m,
> >  
> >  	switch (m->type) {
> >  	case Z_EROFS_VLE_CLUSTER_TYPE_NONHEAD:
> > -		if (unlikely(!m->delta[0])) {
> > +		if (!m->delta[0]) {
> >  			errln("invalid lookback distance 0 at nid %llu",
> >  			      vi->nid);
> >  			DBG_BUGON(1);
> > @@ -386,7 +386,7 @@ int z_erofs_map_blocks_iter(struct inode *inode,
> >  	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)) {
> > +	if (map->m_la >= inode->i_size) {
> >  		map->m_llen = map->m_la + 1 - inode->i_size;
> >  		map->m_la = inode->i_size;
> >  		map->m_flags = 0;
> > @@ -420,7 +420,7 @@ int z_erofs_map_blocks_iter(struct inode *inode,
> >  			break;
> >  		}
> >  		/* m.lcn should be >= 1 if endoff < m.clusterofs */
> > -		if (unlikely(!m.lcn)) {
> > +		if (!m.lcn) {
> >  			errln("invalid logical cluster 0 at nid %llu",
> >  			      vi->nid);
> >  			err = -EFSCORRUPTED;
> > @@ -433,7 +433,7 @@ int z_erofs_map_blocks_iter(struct inode *inode,
> >  	case Z_EROFS_VLE_CLUSTER_TYPE_NONHEAD:
> >  		/* get the correspoinding first chunk */
> >  		err = vle_extent_lookback(&m, m.delta[0]);
> > -		if (unlikely(err))
> > +		if (err)
> >  			goto unmap_out;
> >  		break;
> >  	default:
> > diff --git a/fs/erofs/zpvec.h b/fs/erofs/zpvec.h
> > index bd3cee16491c..58556903aa94 100644
> > --- a/fs/erofs/zpvec.h
> > +++ b/fs/erofs/zpvec.h
> > @@ -111,11 +111,11 @@ static inline bool z_erofs_pagevec_enqueue(struct z_erofs_pagevec_ctor *ctor,
> >  					   bool *occupied)
> >  {
> >  	*occupied = false;
> > -	if (unlikely(!ctor->next && type))
> > +	if (!ctor->next && type)
> >  		if (ctor->index + 1 == ctor->nr)
> >  			return false;
> >  
> > -	if (unlikely(ctor->index >= ctor->nr))
> > +	if (ctor->index >= ctor->nr)
> >  		z_erofs_pagevec_ctor_pagedown(ctor, false);
> >  
> >  	/* exclusive page type must be 0 */
> > @@ -137,7 +137,7 @@ z_erofs_pagevec_dequeue(struct z_erofs_pagevec_ctor *ctor,
> >  {
> >  	erofs_vtptr_t t;
> >  
> > -	if (unlikely(ctor->index >= ctor->nr)) {
> > +	if (ctor->index >= ctor->nr) {
> >  		DBG_BUGON(!ctor->next);
> >  		z_erofs_pagevec_ctor_pagedown(ctor, true);
> >  	}
> > 

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v3 6/7] erofs: remove all likely/unlikely annotations
  2019-08-30  6:31             ` Gao Xiang
@ 2019-08-30  6:43               ` Chao Yu
  0 siblings, 0 replies; 38+ messages in thread
From: Chao Yu @ 2019-08-30  6:43 UTC (permalink / raw)
  To: Gao Xiang
  Cc: devel, Greg Kroah-Hartman, Miao Xie, LKML, Christoph Hellwig,
	weidu.du, Joe Perches, linux-erofs, Dan Carpenter

Xiang, the code itself looks clean to me.

Reviewed-by: Chao Yu <yuchao0@huawei.com>

Thanks,

On 2019/8/30 14:31, Gao Xiang wrote:
> Hi Chao,
> 
> On Fri, Aug 30, 2019 at 02:25:13PM +0800, Chao Yu wrote:
>> On 2019/8/30 11:36, Gao Xiang wrote:
>>> As Dan Carpenter suggested [1], I have to remove
>>> all erofs likely/unlikely annotations.
>>>
>>> [1] https://lore.kernel.org/linux-fsdevel/20190829154346.GK23584@kadam/
>>> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
>>> Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>
>>
>> I suggest we can modify this by following good example rather than removing them
>> all, at least, the fuzzed random fields of disk layout handling should be very
>> rare case, I guess it's fine to use unlikely.
>>
>> To Dan, thoughts?
> 
> I think let's stop talking this anymore, I think we can do such
> a commit now and I think "folks have their own thoughts on it".
> 
> Could you kindly review and check this one? I did it by 'sed', could
> you double check it?
> 
> Thanks,
> Gao Xiang
> 
>>
>> Thanks,
>>
>>> ---
>>> no change.
>>>
>>>  fs/erofs/data.c         | 22 ++++++++++-----------
>>>  fs/erofs/decompressor.c |  2 +-
>>>  fs/erofs/dir.c          | 14 ++++++-------
>>>  fs/erofs/inode.c        | 10 +++++-----
>>>  fs/erofs/internal.h     |  4 ++--
>>>  fs/erofs/namei.c        | 14 ++++++-------
>>>  fs/erofs/super.c        | 16 +++++++--------
>>>  fs/erofs/utils.c        | 12 +++++------
>>>  fs/erofs/xattr.c        | 12 +++++------
>>>  fs/erofs/zdata.c        | 44 ++++++++++++++++++++---------------------
>>>  fs/erofs/zmap.c         |  8 ++++----
>>>  fs/erofs/zpvec.h        |  6 +++---
>>>  12 files changed, 82 insertions(+), 82 deletions(-)
>>>
>>> diff --git a/fs/erofs/data.c b/fs/erofs/data.c
>>> index fda16ec8863e..0f2f1a839372 100644
>>> --- a/fs/erofs/data.c
>>> +++ b/fs/erofs/data.c
>>> @@ -27,7 +27,7 @@ static inline void read_endio(struct bio *bio)
>>>  		/* page is already locked */
>>>  		DBG_BUGON(PageUptodate(page));
>>>  
>>> -		if (unlikely(err))
>>> +		if (err)
>>>  			SetPageError(page);
>>>  		else
>>>  			SetPageUptodate(page);
>>> @@ -53,7 +53,7 @@ struct page *__erofs_get_meta_page(struct super_block *sb,
>>>  
>>>  repeat:
>>>  	page = find_or_create_page(mapping, blkaddr, gfp);
>>> -	if (unlikely(!page)) {
>>> +	if (!page) {
>>>  		DBG_BUGON(nofail);
>>>  		return ERR_PTR(-ENOMEM);
>>>  	}
>>> @@ -70,7 +70,7 @@ struct page *__erofs_get_meta_page(struct super_block *sb,
>>>  		}
>>>  
>>>  		err = bio_add_page(bio, page, PAGE_SIZE, 0);
>>> -		if (unlikely(err != PAGE_SIZE)) {
>>> +		if (err != PAGE_SIZE) {
>>>  			err = -EFAULT;
>>>  			goto err_out;
>>>  		}
>>> @@ -81,7 +81,7 @@ struct page *__erofs_get_meta_page(struct super_block *sb,
>>>  		lock_page(page);
>>>  
>>>  		/* this page has been truncated by others */
>>> -		if (unlikely(page->mapping != mapping)) {
>>> +		if (page->mapping != mapping) {
>>>  unlock_repeat:
>>>  			unlock_page(page);
>>>  			put_page(page);
>>> @@ -89,7 +89,7 @@ struct page *__erofs_get_meta_page(struct super_block *sb,
>>>  		}
>>>  
>>>  		/* more likely a read error */
>>> -		if (unlikely(!PageUptodate(page))) {
>>> +		if (!PageUptodate(page)) {
>>>  			if (io_retries) {
>>>  				--io_retries;
>>>  				goto unlock_repeat;
>>> @@ -120,7 +120,7 @@ static int erofs_map_blocks_flatmode(struct inode *inode,
>>>  	nblocks = DIV_ROUND_UP(inode->i_size, PAGE_SIZE);
>>>  	lastblk = nblocks - is_inode_flat_inline(inode);
>>>  
>>> -	if (unlikely(offset >= inode->i_size)) {
>>> +	if (offset >= inode->i_size) {
>>>  		/* leave out-of-bound access unmapped */
>>>  		map->m_flags = 0;
>>>  		map->m_plen = 0;
>>> @@ -170,7 +170,7 @@ static int erofs_map_blocks_flatmode(struct inode *inode,
>>>  int erofs_map_blocks(struct inode *inode,
>>>  		     struct erofs_map_blocks *map, int flags)
>>>  {
>>> -	if (unlikely(is_inode_layout_compression(inode))) {
>>> +	if (is_inode_layout_compression(inode)) {
>>>  		int err = z_erofs_map_blocks_iter(inode, map, flags);
>>>  
>>>  		if (map->mpage) {
>>> @@ -218,11 +218,11 @@ static inline struct bio *erofs_read_raw_page(struct bio *bio,
>>>  		unsigned int blkoff;
>>>  
>>>  		err = erofs_map_blocks(inode, &map, EROFS_GET_BLOCKS_RAW);
>>> -		if (unlikely(err))
>>> +		if (err)
>>>  			goto err_out;
>>>  
>>>  		/* zero out the holed page */
>>> -		if (unlikely(!(map.m_flags & EROFS_MAP_MAPPED))) {
>>> +		if (!(map.m_flags & EROFS_MAP_MAPPED)) {
>>>  			zero_user_segment(page, 0, PAGE_SIZE);
>>>  			SetPageUptodate(page);
>>>  
>>> @@ -315,7 +315,7 @@ static inline struct bio *erofs_read_raw_page(struct bio *bio,
>>>  submit_bio_out:
>>>  		__submit_bio(bio, REQ_OP_READ, 0);
>>>  
>>> -	return unlikely(err) ? ERR_PTR(err) : NULL;
>>> +	return err ? ERR_PTR(err) : NULL;
>>>  }
>>>  
>>>  /*
>>> @@ -377,7 +377,7 @@ static int erofs_raw_access_readpages(struct file *filp,
>>>  	DBG_BUGON(!list_empty(pages));
>>>  
>>>  	/* the rare case (end in gaps) */
>>> -	if (unlikely(bio))
>>> +	if (bio)
>>>  		__submit_bio(bio, REQ_OP_READ, 0);
>>>  	return 0;
>>>  }
>>> diff --git a/fs/erofs/decompressor.c b/fs/erofs/decompressor.c
>>> index 5f4b7f302863..df349888f911 100644
>>> --- a/fs/erofs/decompressor.c
>>> +++ b/fs/erofs/decompressor.c
>>> @@ -78,7 +78,7 @@ static int lz4_prepare_destpages(struct z_erofs_decompress_req *rq,
>>>  			get_page(victim);
>>>  		} else {
>>>  			victim = erofs_allocpage(pagepool, GFP_KERNEL, false);
>>> -			if (unlikely(!victim))
>>> +			if (!victim)
>>>  				return -ENOMEM;
>>>  			victim->mapping = Z_EROFS_MAPPING_STAGING;
>>>  		}
>>> diff --git a/fs/erofs/dir.c b/fs/erofs/dir.c
>>> index 1976e60e5174..6a5b43f7fb29 100644
>>> --- a/fs/erofs/dir.c
>>> +++ b/fs/erofs/dir.c
>>> @@ -45,8 +45,8 @@ static int erofs_fill_dentries(struct inode *dir, struct dir_context *ctx,
>>>  			de_namelen = le16_to_cpu(de[1].nameoff) - nameoff;
>>>  
>>>  		/* a corrupted entry is found */
>>> -		if (unlikely(nameoff + de_namelen > maxsize ||
>>> -			     de_namelen > EROFS_NAME_LEN)) {
>>> +		if (nameoff + de_namelen > maxsize ||
>>> +		    de_namelen > EROFS_NAME_LEN) {
>>>  			errln("bogus dirent @ nid %llu", EROFS_V(dir)->nid);
>>>  			DBG_BUGON(1);
>>>  			return -EFSCORRUPTED;
>>> @@ -94,8 +94,8 @@ static int erofs_readdir(struct file *f, struct dir_context *ctx)
>>>  
>>>  		nameoff = le16_to_cpu(de->nameoff);
>>>  
>>> -		if (unlikely(nameoff < sizeof(struct erofs_dirent) ||
>>> -			     nameoff >= PAGE_SIZE)) {
>>> +		if (nameoff < sizeof(struct erofs_dirent) ||
>>> +		    nameoff >= PAGE_SIZE) {
>>>  			errln("%s, invalid de[0].nameoff %u @ nid %llu",
>>>  			      __func__, nameoff, EROFS_V(dir)->nid);
>>>  			err = -EFSCORRUPTED;
>>> @@ -106,11 +106,11 @@ static int erofs_readdir(struct file *f, struct dir_context *ctx)
>>>  				dirsize - ctx->pos + ofs, PAGE_SIZE);
>>>  
>>>  		/* search dirents at the arbitrary position */
>>> -		if (unlikely(initial)) {
>>> +		if (initial) {
>>>  			initial = false;
>>>  
>>>  			ofs = roundup(ofs, sizeof(struct erofs_dirent));
>>> -			if (unlikely(ofs >= nameoff))
>>> +			if (ofs >= nameoff)
>>>  				goto skip_this;
>>>  		}
>>>  
>>> @@ -123,7 +123,7 @@ static int erofs_readdir(struct file *f, struct dir_context *ctx)
>>>  
>>>  		ctx->pos = blknr_to_addr(i) + ofs;
>>>  
>>> -		if (unlikely(err))
>>> +		if (err)
>>>  			break;
>>>  		++i;
>>>  		ofs = 0;
>>> diff --git a/fs/erofs/inode.c b/fs/erofs/inode.c
>>> index cf31554075c9..871a6d8ed6f9 100644
>>> --- a/fs/erofs/inode.c
>>> +++ b/fs/erofs/inode.c
>>> @@ -18,7 +18,7 @@ static int read_inode(struct inode *inode, void *data)
>>>  
>>>  	vi->datamode = __inode_data_mapping(advise);
>>>  
>>> -	if (unlikely(vi->datamode >= EROFS_INODE_LAYOUT_MAX)) {
>>> +	if (vi->datamode >= EROFS_INODE_LAYOUT_MAX) {
>>>  		errln("unsupported data mapping %u of nid %llu",
>>>  		      vi->datamode, vi->nid);
>>>  		DBG_BUGON(1);
>>> @@ -133,13 +133,13 @@ static int fill_inline_data(struct inode *inode, void *data,
>>>  	if (S_ISLNK(inode->i_mode) && inode->i_size < PAGE_SIZE) {
>>>  		char *lnk = erofs_kmalloc(sbi, inode->i_size + 1, GFP_KERNEL);
>>>  
>>> -		if (unlikely(!lnk))
>>> +		if (!lnk)
>>>  			return -ENOMEM;
>>>  
>>>  		m_pofs += vi->inode_isize + vi->xattr_isize;
>>>  
>>>  		/* inline symlink data shouldn't across page boundary as well */
>>> -		if (unlikely(m_pofs + inode->i_size > PAGE_SIZE)) {
>>> +		if (m_pofs + inode->i_size > PAGE_SIZE) {
>>>  			kfree(lnk);
>>>  			errln("inline data cross block boundary @ nid %llu",
>>>  			      vi->nid);
>>> @@ -268,7 +268,7 @@ struct inode *erofs_iget(struct super_block *sb,
>>>  {
>>>  	struct inode *inode = erofs_iget_locked(sb, nid);
>>>  
>>> -	if (unlikely(!inode))
>>> +	if (!inode)
>>>  		return ERR_PTR(-ENOMEM);
>>>  
>>>  	if (inode->i_state & I_NEW) {
>>> @@ -278,7 +278,7 @@ struct inode *erofs_iget(struct super_block *sb,
>>>  		vi->nid = nid;
>>>  
>>>  		err = fill_inode(inode, isdir);
>>> -		if (likely(!err))
>>> +		if (!err)
>>>  			unlock_new_inode(inode);
>>>  		else {
>>>  			iget_failed(inode);
>>> diff --git a/fs/erofs/internal.h b/fs/erofs/internal.h
>>> index 620b73fcc416..141ea424587d 100644
>>> --- a/fs/erofs/internal.h
>>> +++ b/fs/erofs/internal.h
>>> @@ -424,7 +424,7 @@ static inline struct bio *erofs_grab_bio(struct super_block *sb,
>>>  	do {
>>>  		if (nr_pages == 1) {
>>>  			bio = bio_alloc(gfp | (nofail ? __GFP_NOFAIL : 0), 1);
>>> -			if (unlikely(!bio)) {
>>> +			if (!bio) {
>>>  				DBG_BUGON(nofail);
>>>  				return ERR_PTR(-ENOMEM);
>>>  			}
>>> @@ -432,7 +432,7 @@ static inline struct bio *erofs_grab_bio(struct super_block *sb,
>>>  		}
>>>  		bio = bio_alloc(gfp, nr_pages);
>>>  		nr_pages /= 2;
>>> -	} while (unlikely(!bio));
>>> +	} while (!bio);
>>>  
>>>  	bio->bi_end_io = endio;
>>>  	bio_set_dev(bio, sb->s_bdev);
>>> diff --git a/fs/erofs/namei.c b/fs/erofs/namei.c
>>> index 8832b5d95d91..c1068ad0535e 100644
>>> --- a/fs/erofs/namei.c
>>> +++ b/fs/erofs/namei.c
>>> @@ -64,7 +64,7 @@ static struct erofs_dirent *find_target_dirent(struct erofs_qstr *name,
>>>  		unsigned int matched = min(startprfx, endprfx);
>>>  		struct erofs_qstr dname = {
>>>  			.name = data + nameoff,
>>> -			.end = unlikely(mid >= ndirents - 1) ?
>>> +			.end = mid >= ndirents - 1 ?
>>>  				data + dirblksize :
>>>  				data + nameoff_from_disk(de[mid + 1].nameoff,
>>>  							 dirblksize)
>>> @@ -73,7 +73,7 @@ static struct erofs_dirent *find_target_dirent(struct erofs_qstr *name,
>>>  		/* string comparison without already matched prefix */
>>>  		int ret = dirnamecmp(name, &dname, &matched);
>>>  
>>> -		if (unlikely(!ret)) {
>>> +		if (!ret) {
>>>  			return de + mid;
>>>  		} else if (ret > 0) {
>>>  			head = mid + 1;
>>> @@ -113,7 +113,7 @@ static struct page *find_target_block_classic(struct inode *dir,
>>>  			unsigned int matched;
>>>  			struct erofs_qstr dname;
>>>  
>>> -			if (unlikely(!ndirents)) {
>>> +			if (!ndirents) {
>>>  				kunmap_atomic(de);
>>>  				put_page(page);
>>>  				errln("corrupted dir block %d @ nid %llu",
>>> @@ -137,7 +137,7 @@ static struct page *find_target_block_classic(struct inode *dir,
>>>  			diff = dirnamecmp(name, &dname, &matched);
>>>  			kunmap_atomic(de);
>>>  
>>> -			if (unlikely(!diff)) {
>>> +			if (!diff) {
>>>  				*_ndirents = 0;
>>>  				goto out;
>>>  			} else if (diff > 0) {
>>> @@ -174,7 +174,7 @@ int erofs_namei(struct inode *dir,
>>>  	struct erofs_dirent *de;
>>>  	struct erofs_qstr qn;
>>>  
>>> -	if (unlikely(!dir->i_size))
>>> +	if (!dir->i_size)
>>>  		return -ENOENT;
>>>  
>>>  	qn.name = name->name;
>>> @@ -221,7 +221,7 @@ static struct dentry *erofs_lookup(struct inode *dir,
>>>  	trace_erofs_lookup(dir, dentry, flags);
>>>  
>>>  	/* file name exceeds fs limit */
>>> -	if (unlikely(dentry->d_name.len > EROFS_NAME_LEN))
>>> +	if (dentry->d_name.len > EROFS_NAME_LEN)
>>>  		return ERR_PTR(-ENAMETOOLONG);
>>>  
>>>  	/* false uninitialized warnings on gcc 4.8.x */
>>> @@ -230,7 +230,7 @@ static struct dentry *erofs_lookup(struct inode *dir,
>>>  	if (err == -ENOENT) {
>>>  		/* negative dentry */
>>>  		inode = NULL;
>>> -	} else if (unlikely(err)) {
>>> +	} else if (err) {
>>>  		inode = ERR_PTR(err);
>>>  	} else {
>>>  		debugln("%s, %s (nid %llu) found, d_type %u", __func__,
>>> diff --git a/fs/erofs/super.c b/fs/erofs/super.c
>>> index 556aae5426d6..0c412de33315 100644
>>> --- a/fs/erofs/super.c
>>> +++ b/fs/erofs/super.c
>>> @@ -92,7 +92,7 @@ static int superblock_read(struct super_block *sb)
>>>  
>>>  	blkszbits = layout->blkszbits;
>>>  	/* 9(512 bytes) + LOG_SECTORS_PER_BLOCK == LOG_BLOCK_SIZE */
>>> -	if (unlikely(blkszbits != LOG_BLOCK_SIZE)) {
>>> +	if (blkszbits != LOG_BLOCK_SIZE) {
>>>  		errln("blksize %u isn't supported on this platform",
>>>  		      1 << blkszbits);
>>>  		goto out;
>>> @@ -364,7 +364,7 @@ static int erofs_init_managed_cache(struct super_block *sb)
>>>  	struct erofs_sb_info *const sbi = EROFS_SB(sb);
>>>  	struct inode *const inode = new_inode(sb);
>>>  
>>> -	if (unlikely(!inode))
>>> +	if (!inode)
>>>  		return -ENOMEM;
>>>  
>>>  	set_nlink(inode, 1);
>>> @@ -391,13 +391,13 @@ static int erofs_fill_super(struct super_block *sb, void *data, int silent)
>>>  
>>>  	sb->s_magic = EROFS_SUPER_MAGIC;
>>>  
>>> -	if (unlikely(!sb_set_blocksize(sb, EROFS_BLKSIZ))) {
>>> +	if (!sb_set_blocksize(sb, EROFS_BLKSIZ)) {
>>>  		errln("failed to set erofs blksize");
>>>  		return -EINVAL;
>>>  	}
>>>  
>>>  	sbi = kzalloc(sizeof(*sbi), GFP_KERNEL);
>>> -	if (unlikely(!sbi))
>>> +	if (!sbi)
>>>  		return -ENOMEM;
>>>  
>>>  	sb->s_fs_info = sbi;
>>> @@ -418,7 +418,7 @@ static int erofs_fill_super(struct super_block *sb, void *data, int silent)
>>>  	default_options(sbi);
>>>  
>>>  	err = parse_options(sb, data);
>>> -	if (unlikely(err))
>>> +	if (err)
>>>  		return err;
>>>  
>>>  	if (!silent)
>>> @@ -438,7 +438,7 @@ static int erofs_fill_super(struct super_block *sb, void *data, int silent)
>>>  	if (IS_ERR(inode))
>>>  		return PTR_ERR(inode);
>>>  
>>> -	if (unlikely(!S_ISDIR(inode->i_mode))) {
>>> +	if (!S_ISDIR(inode->i_mode)) {
>>>  		errln("rootino(nid %llu) is not a directory(i_mode %o)",
>>>  		      ROOT_NID(sbi), inode->i_mode);
>>>  		iput(inode);
>>> @@ -446,13 +446,13 @@ static int erofs_fill_super(struct super_block *sb, void *data, int silent)
>>>  	}
>>>  
>>>  	sb->s_root = d_make_root(inode);
>>> -	if (unlikely(!sb->s_root))
>>> +	if (!sb->s_root)
>>>  		return -ENOMEM;
>>>  
>>>  	erofs_shrinker_register(sb);
>>>  	/* sb->s_umount is already locked, SB_ACTIVE and SB_BORN are not set */
>>>  	err = erofs_init_managed_cache(sb);
>>> -	if (unlikely(err))
>>> +	if (err)
>>>  		return err;
>>>  
>>>  	if (!silent)
>>> diff --git a/fs/erofs/utils.c b/fs/erofs/utils.c
>>> index 1dd041aa0f5a..d92b3e753a6f 100644
>>> --- a/fs/erofs/utils.c
>>> +++ b/fs/erofs/utils.c
>>> @@ -46,14 +46,14 @@ static int erofs_workgroup_get(struct erofs_workgroup *grp)
>>>  
>>>  repeat:
>>>  	o = erofs_wait_on_workgroup_freezed(grp);
>>> -	if (unlikely(o <= 0))
>>> +	if (o <= 0)
>>>  		return -1;
>>>  
>>> -	if (unlikely(atomic_cmpxchg(&grp->refcount, o, o + 1) != o))
>>> +	if (atomic_cmpxchg(&grp->refcount, o, o + 1) != o)
>>>  		goto repeat;
>>>  
>>>  	/* decrease refcount paired by erofs_workgroup_put */
>>> -	if (unlikely(o == 1))
>>> +	if (o == 1)
>>>  		atomic_long_dec(&erofs_global_shrink_cnt);
>>>  	return 0;
>>>  }
>>> @@ -91,7 +91,7 @@ int erofs_register_workgroup(struct super_block *sb,
>>>  	int err;
>>>  
>>>  	/* grp shouldn't be broken or used before */
>>> -	if (unlikely(atomic_read(&grp->refcount) != 1)) {
>>> +	if (atomic_read(&grp->refcount) != 1) {
>>>  		DBG_BUGON(1);
>>>  		return -EINVAL;
>>>  	}
>>> @@ -113,7 +113,7 @@ int erofs_register_workgroup(struct super_block *sb,
>>>  	__erofs_workgroup_get(grp);
>>>  
>>>  	err = radix_tree_insert(&sbi->workstn_tree, grp->index, grp);
>>> -	if (unlikely(err))
>>> +	if (err)
>>>  		/*
>>>  		 * it's safe to decrease since the workgroup isn't visible
>>>  		 * and refcount >= 2 (cannot be freezed).
>>> @@ -212,7 +212,7 @@ static unsigned long erofs_shrink_workstation(struct erofs_sb_info *sbi,
>>>  			continue;
>>>  
>>>  		++freed;
>>> -		if (unlikely(!--nr_shrink))
>>> +		if (!--nr_shrink)
>>>  			break;
>>>  	}
>>>  	xa_unlock(&sbi->workstn_tree);
>>> diff --git a/fs/erofs/xattr.c b/fs/erofs/xattr.c
>>> index 7ef8d4bb45cd..620cbc15f4d0 100644
>>> --- a/fs/erofs/xattr.c
>>> +++ b/fs/erofs/xattr.c
>>> @@ -19,7 +19,7 @@ struct xattr_iter {
>>>  static inline void xattr_iter_end(struct xattr_iter *it, bool atomic)
>>>  {
>>>  	/* the only user of kunmap() is 'init_inode_xattrs' */
>>> -	if (unlikely(!atomic))
>>> +	if (!atomic)
>>>  		kunmap(it->page);
>>>  	else
>>>  		kunmap_atomic(it->kaddr);
>>> @@ -72,7 +72,7 @@ static int init_inode_xattrs(struct inode *inode)
>>>  		ret = -EOPNOTSUPP;
>>>  		goto out_unlock;
>>>  	} else if (vi->xattr_isize < sizeof(struct erofs_xattr_ibody_header)) {
>>> -		if (unlikely(vi->xattr_isize)) {
>>> +		if (vi->xattr_isize) {
>>>  			errln("bogus xattr ibody @ nid %llu", vi->nid);
>>>  			DBG_BUGON(1);
>>>  			ret = -EFSCORRUPTED;
>>> @@ -112,7 +112,7 @@ static int init_inode_xattrs(struct inode *inode)
>>>  	it.ofs += sizeof(struct erofs_xattr_ibody_header);
>>>  
>>>  	for (i = 0; i < vi->xattr_shared_count; ++i) {
>>> -		if (unlikely(it.ofs >= EROFS_BLKSIZ)) {
>>> +		if (it.ofs >= EROFS_BLKSIZ) {
>>>  			/* cannot be unaligned */
>>>  			DBG_BUGON(it.ofs != EROFS_BLKSIZ);
>>>  			xattr_iter_end(&it, atomic_map);
>>> @@ -189,7 +189,7 @@ static int inline_xattr_iter_begin(struct xattr_iter *it,
>>>  	unsigned int xattr_header_sz, inline_xattr_ofs;
>>>  
>>>  	xattr_header_sz = inlinexattr_header_size(inode);
>>> -	if (unlikely(xattr_header_sz >= vi->xattr_isize)) {
>>> +	if (xattr_header_sz >= vi->xattr_isize) {
>>>  		DBG_BUGON(xattr_header_sz > vi->xattr_isize);
>>>  		return -ENOATTR;
>>>  	}
>>> @@ -234,7 +234,7 @@ static int xattr_foreach(struct xattr_iter *it,
>>>  		unsigned int entry_sz = erofs_xattr_entry_size(&entry);
>>>  
>>>  		/* xattr on-disk corruption: xattr entry beyond xattr_isize */
>>> -		if (unlikely(*tlimit < entry_sz)) {
>>> +		if (*tlimit < entry_sz) {
>>>  			DBG_BUGON(1);
>>>  			return -EFSCORRUPTED;
>>>  		}
>>> @@ -436,7 +436,7 @@ int erofs_getxattr(struct inode *inode, int index,
>>>  	int ret;
>>>  	struct getxattr_iter it;
>>>  
>>> -	if (unlikely(!name))
>>> +	if (!name)
>>>  		return -EINVAL;
>>>  
>>>  	ret = init_inode_xattrs(inode);
>>> diff --git a/fs/erofs/zdata.c b/fs/erofs/zdata.c
>>> index b32ad585237c..653bde0a619a 100644
>>> --- a/fs/erofs/zdata.c
>>> +++ b/fs/erofs/zdata.c
>>> @@ -230,7 +230,7 @@ int erofs_try_to_free_all_cached_pages(struct erofs_sb_info *sbi,
>>>  		if (!trylock_page(page))
>>>  			return -EBUSY;
>>>  
>>> -		if (unlikely(page->mapping != mapping))
>>> +		if (page->mapping != mapping)
>>>  			continue;
>>>  
>>>  		/* barrier is implied in the following 'unlock_page' */
>>> @@ -358,7 +358,7 @@ static struct z_erofs_collection *cllookup(struct z_erofs_collector *clt,
>>>  	}
>>>  
>>>  	cl = z_erofs_primarycollection(pcl);
>>> -	if (unlikely(cl->pageofs != (map->m_la & ~PAGE_MASK))) {
>>> +	if (cl->pageofs != (map->m_la & ~PAGE_MASK)) {
>>>  		DBG_BUGON(1);
>>>  		erofs_workgroup_put(grp);
>>>  		return ERR_PTR(-EFSCORRUPTED);
>>> @@ -406,7 +406,7 @@ static struct z_erofs_collection *clregister(struct z_erofs_collector *clt,
>>>  
>>>  	/* no available workgroup, let's allocate one */
>>>  	pcl = kmem_cache_alloc(pcluster_cachep, GFP_NOFS);
>>> -	if (unlikely(!pcl))
>>> +	if (!pcl)
>>>  		return ERR_PTR(-ENOMEM);
>>>  
>>>  	init_always(pcl);
>>> @@ -474,7 +474,7 @@ static int z_erofs_collector_begin(struct z_erofs_collector *clt,
>>>  	if (!cl) {
>>>  		cl = clregister(clt, inode, map);
>>>  
>>> -		if (unlikely(cl == ERR_PTR(-EAGAIN)))
>>> +		if (cl == ERR_PTR(-EAGAIN))
>>>  			goto repeat;
>>>  	}
>>>  
>>> @@ -607,15 +607,15 @@ static int z_erofs_do_read_page(struct z_erofs_decompress_frontend *fe,
>>>  	map->m_la = offset + cur;
>>>  	map->m_llen = 0;
>>>  	err = z_erofs_map_blocks_iter(inode, map, 0);
>>> -	if (unlikely(err))
>>> +	if (err)
>>>  		goto err_out;
>>>  
>>>  restart_now:
>>> -	if (unlikely(!(map->m_flags & EROFS_MAP_MAPPED)))
>>> +	if (!(map->m_flags & EROFS_MAP_MAPPED))
>>>  		goto hitted;
>>>  
>>>  	err = z_erofs_collector_begin(clt, inode, map);
>>> -	if (unlikely(err))
>>> +	if (err)
>>>  		goto err_out;
>>>  
>>>  	/* preload all compressed pages (maybe downgrade role if necessary) */
>>> @@ -630,7 +630,7 @@ static int z_erofs_do_read_page(struct z_erofs_decompress_frontend *fe,
>>>  	tight &= (clt->mode >= COLLECT_PRIMARY_HOOKED);
>>>  hitted:
>>>  	cur = end - min_t(unsigned int, offset + end - map->m_la, end);
>>> -	if (unlikely(!(map->m_flags & EROFS_MAP_MAPPED))) {
>>> +	if (!(map->m_flags & EROFS_MAP_MAPPED)) {
>>>  		zero_user_segment(page, cur, end);
>>>  		goto next_part;
>>>  	}
>>> @@ -653,11 +653,11 @@ static int z_erofs_do_read_page(struct z_erofs_decompress_frontend *fe,
>>>  
>>>  		err = z_erofs_attach_page(clt, newpage,
>>>  					  Z_EROFS_PAGE_TYPE_EXCLUSIVE);
>>> -		if (likely(!err))
>>> +		if (!err)
>>>  			goto retry;
>>>  	}
>>>  
>>> -	if (unlikely(err))
>>> +	if (err)
>>>  		goto err_out;
>>>  
>>>  	index = page->index - (map->m_la >> PAGE_SHIFT);
>>> @@ -723,7 +723,7 @@ static inline void z_erofs_vle_read_endio(struct bio *bio)
>>>  		DBG_BUGON(PageUptodate(page));
>>>  		DBG_BUGON(!page->mapping);
>>>  
>>> -		if (unlikely(!sbi && !z_erofs_page_is_staging(page))) {
>>> +		if (!sbi && !z_erofs_page_is_staging(page)) {
>>>  			sbi = EROFS_SB(page->mapping->host->i_sb);
>>>  
>>>  			if (time_to_inject(sbi, FAULT_READ_IO)) {
>>> @@ -736,7 +736,7 @@ static inline void z_erofs_vle_read_endio(struct bio *bio)
>>>  		if (sbi)
>>>  			cachemngd = erofs_page_is_managed(sbi, page);
>>>  
>>> -		if (unlikely(err))
>>> +		if (err)
>>>  			SetPageError(page);
>>>  		else if (cachemngd)
>>>  			SetPageUptodate(page);
>>> @@ -772,7 +772,7 @@ static int z_erofs_decompress_pcluster(struct super_block *sb,
>>>  	mutex_lock(&cl->lock);
>>>  	nr_pages = cl->nr_pages;
>>>  
>>> -	if (likely(nr_pages <= Z_EROFS_VMAP_ONSTACK_PAGES)) {
>>> +	if (nr_pages <= Z_EROFS_VMAP_ONSTACK_PAGES) {
>>>  		pages = pages_onstack;
>>>  	} else if (nr_pages <= Z_EROFS_VMAP_GLOBAL_PAGES &&
>>>  		   mutex_trylock(&z_pagemap_global_lock)) {
>>> @@ -787,7 +787,7 @@ static int z_erofs_decompress_pcluster(struct super_block *sb,
>>>  				       gfp_flags);
>>>  
>>>  		/* fallback to global pagemap for the lowmem scenario */
>>> -		if (unlikely(!pages)) {
>>> +		if (!pages) {
>>>  			mutex_lock(&z_pagemap_global_lock);
>>>  			pages = z_pagemap_global;
>>>  		}
>>> @@ -823,7 +823,7 @@ static int z_erofs_decompress_pcluster(struct super_block *sb,
>>>  		 * currently EROFS doesn't support multiref(dedup),
>>>  		 * so here erroring out one multiref page.
>>>  		 */
>>> -		if (unlikely(pages[pagenr])) {
>>> +		if (pages[pagenr]) {
>>>  			DBG_BUGON(1);
>>>  			SetPageError(pages[pagenr]);
>>>  			z_erofs_onlinepage_endio(pages[pagenr]);
>>> @@ -847,7 +847,7 @@ static int z_erofs_decompress_pcluster(struct super_block *sb,
>>>  
>>>  		if (!z_erofs_page_is_staging(page)) {
>>>  			if (erofs_page_is_managed(sbi, page)) {
>>> -				if (unlikely(!PageUptodate(page)))
>>> +				if (!PageUptodate(page))
>>>  					err = -EIO;
>>>  				continue;
>>>  			}
>>> @@ -859,7 +859,7 @@ static int z_erofs_decompress_pcluster(struct super_block *sb,
>>>  			pagenr = z_erofs_onlinepage_index(page);
>>>  
>>>  			DBG_BUGON(pagenr >= nr_pages);
>>> -			if (unlikely(pages[pagenr])) {
>>> +			if (pages[pagenr]) {
>>>  				DBG_BUGON(1);
>>>  				SetPageError(pages[pagenr]);
>>>  				z_erofs_onlinepage_endio(pages[pagenr]);
>>> @@ -871,13 +871,13 @@ static int z_erofs_decompress_pcluster(struct super_block *sb,
>>>  		}
>>>  
>>>  		/* PG_error needs checking for inplaced and staging pages */
>>> -		if (unlikely(PageError(page))) {
>>> +		if (PageError(page)) {
>>>  			DBG_BUGON(PageUptodate(page));
>>>  			err = -EIO;
>>>  		}
>>>  	}
>>>  
>>> -	if (unlikely(err))
>>> +	if (err)
>>>  		goto out;
>>>  
>>>  	llen = pcl->length >> Z_EROFS_PCLUSTER_LENGTH_BIT;
>>> @@ -926,7 +926,7 @@ static int z_erofs_decompress_pcluster(struct super_block *sb,
>>>  		if (z_erofs_put_stagingpage(pagepool, page))
>>>  			continue;
>>>  
>>> -		if (unlikely(err < 0))
>>> +		if (err < 0)
>>>  			SetPageError(page);
>>>  
>>>  		z_erofs_onlinepage_endio(page);
>>> @@ -934,7 +934,7 @@ static int z_erofs_decompress_pcluster(struct super_block *sb,
>>>  
>>>  	if (pages == z_pagemap_global)
>>>  		mutex_unlock(&z_pagemap_global_lock);
>>> -	else if (unlikely(pages != pages_onstack))
>>> +	else if (pages != pages_onstack)
>>>  		kvfree(pages);
>>>  
>>>  	cl->nr_pages = 0;
>>> @@ -1212,7 +1212,7 @@ static bool z_erofs_vle_submit_all(struct super_block *sb,
>>>  	bool force_submit = false;
>>>  	unsigned int nr_bios;
>>>  
>>> -	if (unlikely(owned_head == Z_EROFS_PCLUSTER_TAIL))
>>> +	if (owned_head == Z_EROFS_PCLUSTER_TAIL)
>>>  		return false;
>>>  
>>>  	force_submit = false;
>>> diff --git a/fs/erofs/zmap.c b/fs/erofs/zmap.c
>>> index 4dc9cec01297..850e0e3d57a8 100644
>>> --- a/fs/erofs/zmap.c
>>> +++ b/fs/erofs/zmap.c
>>> @@ -348,7 +348,7 @@ static int vle_extent_lookback(struct z_erofs_maprecorder *m,
>>>  
>>>  	switch (m->type) {
>>>  	case Z_EROFS_VLE_CLUSTER_TYPE_NONHEAD:
>>> -		if (unlikely(!m->delta[0])) {
>>> +		if (!m->delta[0]) {
>>>  			errln("invalid lookback distance 0 at nid %llu",
>>>  			      vi->nid);
>>>  			DBG_BUGON(1);
>>> @@ -386,7 +386,7 @@ int z_erofs_map_blocks_iter(struct inode *inode,
>>>  	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)) {
>>> +	if (map->m_la >= inode->i_size) {
>>>  		map->m_llen = map->m_la + 1 - inode->i_size;
>>>  		map->m_la = inode->i_size;
>>>  		map->m_flags = 0;
>>> @@ -420,7 +420,7 @@ int z_erofs_map_blocks_iter(struct inode *inode,
>>>  			break;
>>>  		}
>>>  		/* m.lcn should be >= 1 if endoff < m.clusterofs */
>>> -		if (unlikely(!m.lcn)) {
>>> +		if (!m.lcn) {
>>>  			errln("invalid logical cluster 0 at nid %llu",
>>>  			      vi->nid);
>>>  			err = -EFSCORRUPTED;
>>> @@ -433,7 +433,7 @@ int z_erofs_map_blocks_iter(struct inode *inode,
>>>  	case Z_EROFS_VLE_CLUSTER_TYPE_NONHEAD:
>>>  		/* get the correspoinding first chunk */
>>>  		err = vle_extent_lookback(&m, m.delta[0]);
>>> -		if (unlikely(err))
>>> +		if (err)
>>>  			goto unmap_out;
>>>  		break;
>>>  	default:
>>> diff --git a/fs/erofs/zpvec.h b/fs/erofs/zpvec.h
>>> index bd3cee16491c..58556903aa94 100644
>>> --- a/fs/erofs/zpvec.h
>>> +++ b/fs/erofs/zpvec.h
>>> @@ -111,11 +111,11 @@ static inline bool z_erofs_pagevec_enqueue(struct z_erofs_pagevec_ctor *ctor,
>>>  					   bool *occupied)
>>>  {
>>>  	*occupied = false;
>>> -	if (unlikely(!ctor->next && type))
>>> +	if (!ctor->next && type)
>>>  		if (ctor->index + 1 == ctor->nr)
>>>  			return false;
>>>  
>>> -	if (unlikely(ctor->index >= ctor->nr))
>>> +	if (ctor->index >= ctor->nr)
>>>  		z_erofs_pagevec_ctor_pagedown(ctor, false);
>>>  
>>>  	/* exclusive page type must be 0 */
>>> @@ -137,7 +137,7 @@ z_erofs_pagevec_dequeue(struct z_erofs_pagevec_ctor *ctor,
>>>  {
>>>  	erofs_vtptr_t t;
>>>  
>>> -	if (unlikely(ctor->index >= ctor->nr)) {
>>> +	if (ctor->index >= ctor->nr) {
>>>  		DBG_BUGON(!ctor->next);
>>>  		z_erofs_pagevec_ctor_pagedown(ctor, true);
>>>  	}
>>>
> .
> 

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v3 6/7] erofs: remove all likely/unlikely annotations
  2019-08-30  3:36         ` [PATCH v3 6/7] erofs: remove all likely/unlikely annotations Gao Xiang
  2019-08-30  6:25           ` Chao Yu
@ 2019-08-30 11:30           ` Dan Carpenter
  2019-08-30 12:06             ` Gao Xiang via Linux-erofs
  2019-08-30 15:46           ` Christoph Hellwig
  2 siblings, 1 reply; 38+ messages in thread
From: Dan Carpenter @ 2019-08-30 11:30 UTC (permalink / raw)
  To: Gao Xiang
  Cc: devel, Greg Kroah-Hartman, Miao Xie, LKML, Christoph Hellwig,
	weidu.du, Joe Perches, linux-erofs

On Fri, Aug 30, 2019 at 11:36:42AM +0800, Gao Xiang wrote:
> As Dan Carpenter suggested [1], I have to remove
> all erofs likely/unlikely annotations.
> 
> [1] https://lore.kernel.org/linux-fsdevel/20190829154346.GK23584@kadam/
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>
> ---

Thanks!

This is a nice readability improvement and I'm so sure it won't impact
benchmarking at all.

Acked-by: Dan Carpenter <dan.carpenter@oracle.com>

regards,
dan carpenter


^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v3 6/7] erofs: remove all likely/unlikely annotations
  2019-08-30  6:25           ` Chao Yu
  2019-08-30  6:31             ` Gao Xiang
@ 2019-08-30 11:55             ` Dan Carpenter
  1 sibling, 0 replies; 38+ messages in thread
From: Dan Carpenter @ 2019-08-30 11:55 UTC (permalink / raw)
  To: Chao Yu
  Cc: devel, Greg Kroah-Hartman, Miao Xie, LKML, Christoph Hellwig,
	weidu.du, Joe Perches, linux-erofs

On Fri, Aug 30, 2019 at 02:25:13PM +0800, Chao Yu wrote:
> On 2019/8/30 11:36, Gao Xiang wrote:
> > As Dan Carpenter suggested [1], I have to remove
> > all erofs likely/unlikely annotations.
> > 
> > [1] https://lore.kernel.org/linux-fsdevel/20190829154346.GK23584@kadam/
> > Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> > Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>
> 
> I suggest we can modify this by following good example rather than removing them
> all, at least, the fuzzed random fields of disk layout handling should be very
> rare case, I guess it's fine to use unlikely.

No, no...  It's the opposite.  Only use those annotations on fast paths
where it's going to show up in benchmarks.  On fast paths then remove
all the debug code and really optimize the heck out of the code.  We
sacrifice readability for speed in places where it matters.

regards,
dan carpenter


^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v3 6/7] erofs: remove all likely/unlikely annotations
  2019-08-30 11:30           ` Dan Carpenter
@ 2019-08-30 12:06             ` Gao Xiang via Linux-erofs
  0 siblings, 0 replies; 38+ messages in thread
From: Gao Xiang via Linux-erofs @ 2019-08-30 12:06 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: devel, Greg Kroah-Hartman, linux-erofs, LKML, Christoph Hellwig,
	weidu.du, Joe Perches, Miao Xie

Hi Dan,

On Fri, Aug 30, 2019 at 02:30:47PM +0300, Dan Carpenter wrote:
> On Fri, Aug 30, 2019 at 11:36:42AM +0800, Gao Xiang wrote:
> > As Dan Carpenter suggested [1], I have to remove
> > all erofs likely/unlikely annotations.
> > 
> > [1] https://lore.kernel.org/linux-fsdevel/20190829154346.GK23584@kadam/
> > Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> > Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>
> > ---
> 
> Thanks!
> 
> This is a nice readability improvement and I'm so sure it won't impact
> benchmarking at all.
> 
> Acked-by: Dan Carpenter <dan.carpenter@oracle.com>

It seems Greg merged another version... I have no idea but thanks for
your acked-by :)
	https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git/commit/?h=staging-testing&id=8d8a09b093d7073465c824f74caf315c073d3875

THanks,
Gao Xiang

> 
> regards,
> dan carpenter
>
 

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v3 1/7] erofs: on-disk format should have explicitly assigned numbers
  2019-08-30  3:36       ` [PATCH v3 1/7] erofs: on-disk format should have explicitly assigned numbers Gao Xiang
                           ` (5 preceding siblings ...)
  2019-08-30  3:36         ` [PATCH v3 7/7] erofs: redundant assignment in __erofs_get_meta_page() Gao Xiang
@ 2019-08-30 15:25         ` Gao Xiang
  6 siblings, 0 replies; 38+ messages in thread
From: Gao Xiang @ 2019-08-30 15:25 UTC (permalink / raw)
  To: Chao Yu, Dan Carpenter, Christoph Hellwig, Joe Perches,
	Greg Kroah-Hartman, devel
  Cc: weidu.du, Miao Xie, linux-erofs, LKML

On Fri, Aug 30, 2019 at 11:36:37AM +0800, Gao Xiang wrote:
> As Christoph claimed [1], on-disk format should have
> explicitly assigned numbers. I have to change it.
> 
> [1] https://lore.kernel.org/r/20190829095954.GB20598@infradead.org/
> Reported-by: Christoph Hellwig <hch@infradead.org>
> Reviewed-by: Chao Yu <yuchao0@huawei.com>
> Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>

...ignore this patchset. I will resend another incremental
patchset to address what Christoph suggested yesterday...

Thanks,
Gao Xiang

> ---
> no change
> 
>  fs/erofs/erofs_fs.h | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/erofs/erofs_fs.h b/fs/erofs/erofs_fs.h
> index afa7d45ca958..2447ad4d0920 100644
> --- a/fs/erofs/erofs_fs.h
> +++ b/fs/erofs/erofs_fs.h
> @@ -52,10 +52,10 @@ struct erofs_super_block {
>   * 4~7 - reserved
>   */
>  enum {
> -	EROFS_INODE_FLAT_PLAIN,
> -	EROFS_INODE_FLAT_COMPRESSION_LEGACY,
> -	EROFS_INODE_FLAT_INLINE,
> -	EROFS_INODE_FLAT_COMPRESSION,
> +	EROFS_INODE_FLAT_PLAIN			= 0,
> +	EROFS_INODE_FLAT_COMPRESSION_LEGACY	= 1,
> +	EROFS_INODE_FLAT_INLINE			= 2,
> +	EROFS_INODE_FLAT_COMPRESSION		= 3,
>  	EROFS_INODE_LAYOUT_MAX
>  };
>  
> @@ -181,7 +181,7 @@ struct erofs_xattr_entry {
>  
>  /* available compression algorithm types */
>  enum {
> -	Z_EROFS_COMPRESSION_LZ4,
> +	Z_EROFS_COMPRESSION_LZ4	= 0,
>  	Z_EROFS_COMPRESSION_MAX
>  };
>  
> @@ -239,10 +239,10 @@ struct z_erofs_map_header {
>   *                (di_advise could be 0, 1 or 2)
>   */
>  enum {
> -	Z_EROFS_VLE_CLUSTER_TYPE_PLAIN,
> -	Z_EROFS_VLE_CLUSTER_TYPE_HEAD,
> -	Z_EROFS_VLE_CLUSTER_TYPE_NONHEAD,
> -	Z_EROFS_VLE_CLUSTER_TYPE_RESERVED,
> +	Z_EROFS_VLE_CLUSTER_TYPE_PLAIN		= 0,
> +	Z_EROFS_VLE_CLUSTER_TYPE_HEAD		= 1,
> +	Z_EROFS_VLE_CLUSTER_TYPE_NONHEAD	= 2,
> +	Z_EROFS_VLE_CLUSTER_TYPE_RESERVED	= 3,
>  	Z_EROFS_VLE_CLUSTER_TYPE_MAX
>  };
>  
> -- 
> 2.17.1
> 

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v2 2/7] erofs: some marcos are much more readable as a function
  2019-08-30  3:16   ` Joe Perches
  2019-08-30  3:20     ` Gao Xiang
@ 2019-08-30 15:45     ` Christoph Hellwig
  2019-08-30 15:52       ` Gao Xiang
  1 sibling, 1 reply; 38+ messages in thread
From: Christoph Hellwig @ 2019-08-30 15:45 UTC (permalink / raw)
  To: Joe Perches
  Cc: devel, Greg Kroah-Hartman, Miao Xie, LKML, Christoph Hellwig,
	weidu.du, linux-erofs, Dan Carpenter

On Thu, Aug 29, 2019 at 08:16:27PM -0700, Joe Perches wrote:
> > -		sizeof(__u32) * ((__count) - 1); })
> > +static inline unsigned int erofs_xattr_ibody_size(__le16 d_icount)
> > +{
> > +	unsigned int icount = le16_to_cpu(d_icount);
> > +
> > +	if (!icount)
> > +		return 0;
> > +
> > +	return sizeof(struct erofs_xattr_ibody_header) +
> > +		sizeof(__u32) * (icount - 1);
> 
> Maybe use struct_size()?

Declaring a variable that is only used for struct_size is rather ugly.
But while we are nitpicking: you don't need to byteswap to check for 0,
so the local variable could be avoided.

Also what is that magic -1 for?  Normally we use that for the
deprecated style where a variable size array is declared using
varname[1], but that doesn't seem to be the case for erofs.

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v3 6/7] erofs: remove all likely/unlikely annotations
  2019-08-30  3:36         ` [PATCH v3 6/7] erofs: remove all likely/unlikely annotations Gao Xiang
  2019-08-30  6:25           ` Chao Yu
  2019-08-30 11:30           ` Dan Carpenter
@ 2019-08-30 15:46           ` Christoph Hellwig
  2019-08-30 16:04             ` Gao Xiang
  2 siblings, 1 reply; 38+ messages in thread
From: Christoph Hellwig @ 2019-08-30 15:46 UTC (permalink / raw)
  To: Gao Xiang
  Cc: devel, Greg Kroah-Hartman, Miao Xie, LKML, Christoph Hellwig,
	weidu.du, Joe Perches, linux-erofs, Dan Carpenter

On Fri, Aug 30, 2019 at 11:36:42AM +0800, Gao Xiang wrote:
> As Dan Carpenter suggested [1], I have to remove
> all erofs likely/unlikely annotations.

Do you have to remove all of them, or just those where you don't have
a particularly good reason why you think in this particular case they
might actually matter?

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v2 2/7] erofs: some marcos are much more readable as a function
  2019-08-30 15:45     ` [PATCH v2 2/7] erofs: some marcos are much more readable as a function Christoph Hellwig
@ 2019-08-30 15:52       ` Gao Xiang
  2019-08-30 15:56         ` Gao Xiang
  0 siblings, 1 reply; 38+ messages in thread
From: Gao Xiang @ 2019-08-30 15:52 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: devel, Greg Kroah-Hartman, Miao Xie, LKML, weidu.du, Joe Perches,
	linux-erofs, Dan Carpenter

Hi Christoph,

On Fri, Aug 30, 2019 at 08:45:51AM -0700, Christoph Hellwig wrote:
> On Thu, Aug 29, 2019 at 08:16:27PM -0700, Joe Perches wrote:
> > > -		sizeof(__u32) * ((__count) - 1); })
> > > +static inline unsigned int erofs_xattr_ibody_size(__le16 d_icount)
> > > +{
> > > +	unsigned int icount = le16_to_cpu(d_icount);
> > > +
> > > +	if (!icount)
> > > +		return 0;
> > > +
> > > +	return sizeof(struct erofs_xattr_ibody_header) +
> > > +		sizeof(__u32) * (icount - 1);
> > 
> > Maybe use struct_size()?
> 
> Declaring a variable that is only used for struct_size is rather ugly.
> But while we are nitpicking: you don't need to byteswap to check for 0,
> so the local variable could be avoided.
> 
> Also what is that magic -1 for?  Normally we use that for the
> deprecated style where a variable size array is declared using
> varname[1], but that doesn't seem to be the case for erofs.

I have to explain more about this (sorry about my awkward English)
here i_xattr_icount is to represent the size of xattr field of erofs, as follows:
 0 - no xattr at all (no erofs_xattr_ibody_header)
  _______
 | inode |
 |_______|

 1 - a erofs_xattr_ibody_header (12 byte) + 4-byte (shared + inline) xattrs
 2 - a erofs_xattr_ibody_header (12 byte) + 8-byte (shared + inline) xattrs
 ....
 (that is the magic -1 means...)

In order to keep the number continuously, actually the content could be
 an array of shared_xattr_id and
 an inline xattr combination (struct erofs_xattr_entry + name + value)

Thanks,
Gao Xiang


^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v2 2/7] erofs: some marcos are much more readable as a function
  2019-08-30 15:52       ` Gao Xiang
@ 2019-08-30 15:56         ` Gao Xiang
  0 siblings, 0 replies; 38+ messages in thread
From: Gao Xiang @ 2019-08-30 15:56 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: devel, Greg Kroah-Hartman, linux-erofs, LKML, weidu.du,
	Joe Perches, Miao Xie, Dan Carpenter

On Fri, Aug 30, 2019 at 11:52:23PM +0800, Gao Xiang wrote:
> Hi Christoph,
> 
> On Fri, Aug 30, 2019 at 08:45:51AM -0700, Christoph Hellwig wrote:
> > On Thu, Aug 29, 2019 at 08:16:27PM -0700, Joe Perches wrote:
> > > > -		sizeof(__u32) * ((__count) - 1); })
> > > > +static inline unsigned int erofs_xattr_ibody_size(__le16 d_icount)
> > > > +{
> > > > +	unsigned int icount = le16_to_cpu(d_icount);
> > > > +
> > > > +	if (!icount)
> > > > +		return 0;
> > > > +
> > > > +	return sizeof(struct erofs_xattr_ibody_header) +
> > > > +		sizeof(__u32) * (icount - 1);
> > > 
> > > Maybe use struct_size()?
> > 
> > Declaring a variable that is only used for struct_size is rather ugly.
> > But while we are nitpicking: you don't need to byteswap to check for 0,
> > so the local variable could be avoided.
> > 
> > Also what is that magic -1 for?  Normally we use that for the
> > deprecated style where a variable size array is declared using
> > varname[1], but that doesn't seem to be the case for erofs.
> 
> I have to explain more about this (sorry about my awkward English)
> here i_xattr_icount is to represent the size of xattr field of erofs, as follows:
>  0 - no xattr at all (no erofs_xattr_ibody_header)
>   _______
>  | inode |
>  |_______|
> 
>  1 - a erofs_xattr_ibody_header (12 byte) + 4-byte (shared + inline) xattrs
>  2 - a erofs_xattr_ibody_header (12 byte) + 8-byte (shared + inline) xattrs
>  ....
>  (that is the magic -1 means...)
> 
> In order to keep the number continuously, actually the content could be
>  an array of shared_xattr_id and
>  an inline xattr combination (struct erofs_xattr_entry + name + value)

...Add a word, large xattrs should use shared xattr (which save xattrs
in another area) rather than inline xattr, shared xattr stores xattr_id
just after erofs_xattr_ibody_header and before inline xattrs...

Thanks,
Gao Xiang

> 
> Thanks,
> Gao Xiang
> 

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v3 6/7] erofs: remove all likely/unlikely annotations
  2019-08-30 15:46           ` Christoph Hellwig
@ 2019-08-30 16:04             ` Gao Xiang
  2019-08-31 10:57               ` Dan Carpenter
  0 siblings, 1 reply; 38+ messages in thread
From: Gao Xiang @ 2019-08-30 16:04 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: devel, Greg Kroah-Hartman, Miao Xie, LKML, weidu.du, Joe Perches,
	linux-erofs, Dan Carpenter

Hi Christoph,

On Fri, Aug 30, 2019 at 08:46:50AM -0700, Christoph Hellwig wrote:
> On Fri, Aug 30, 2019 at 11:36:42AM +0800, Gao Xiang wrote:
> > As Dan Carpenter suggested [1], I have to remove
> > all erofs likely/unlikely annotations.
> 
> Do you have to remove all of them, or just those where you don't have
> a particularly good reason why you think in this particular case they
> might actually matter?

I just added unlikely/likely for all erofs error handling paths or
rare happened cases at first... (That is all in my thought...)

I don't have some benchmark data for each unlikely/likely case (and I have
no idea "is that worth to take time to benchmark rather than do another more
useful stuffs"), so..I have to kill them all...

Thanks,
Gao Xiang



^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v3 7/7] erofs: redundant assignment in __erofs_get_meta_page()
  2019-08-30  3:36         ` [PATCH v3 7/7] erofs: redundant assignment in __erofs_get_meta_page() Gao Xiang
  2019-08-30  6:25           ` Chao Yu
@ 2019-08-30 16:28           ` Christoph Hellwig
  2019-08-30 16:48             ` Gao Xiang
  1 sibling, 1 reply; 38+ messages in thread
From: Christoph Hellwig @ 2019-08-30 16:28 UTC (permalink / raw)
  To: Gao Xiang
  Cc: devel, Greg Kroah-Hartman, Miao Xie, LKML, Christoph Hellwig,
	weidu.du, Joe Perches, linux-erofs, Dan Carpenter

> -		err = bio_add_page(bio, page, PAGE_SIZE, 0);
> -		if (err != PAGE_SIZE) {
> +		if (bio_add_page(bio, page, PAGE_SIZE, 0) != PAGE_SIZE) {
>  			err = -EFAULT;
>  			goto err_out;
>  		}

This patch looks like an improvement.  But looking at that whole
area just makes me cringe.

Why is there __erofs_get_meta_page with the two weird booleans instead
of a single erofs_get_meta_page that gets and gfp_t for additional
flags and an unsigned int for additional bio op flags.

Why do need ioprio support to start with?  Seeing that in a new
fs look kinda odd.  Do you have benchmarks that show the difference?

That function then calls erofs_grab_bio, which tries to handle a
bio_alloc failure, except that the function will not actually fail
due the mempool backing it.  It also seems like and awfully
huge function to inline.

Why is there __submit_bio which really just obsfucates what is
going on?  Also why is __submit_bio using bio_set_op_attrs instead
of opencode it as the comment right next to it asks you to?

Also I really don't understand why you can't just use read_cache_page
or even read_cache_page_gfp instead of __erofs_get_meta_page.
That function is a whole lot of duplication of functionality shared
by a lot of other file systems.

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v3 7/7] erofs: redundant assignment in __erofs_get_meta_page()
  2019-08-30 16:28           ` Christoph Hellwig
@ 2019-08-30 16:48             ` Gao Xiang
  0 siblings, 0 replies; 38+ messages in thread
From: Gao Xiang @ 2019-08-30 16:48 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: devel, Greg Kroah-Hartman, Miao Xie, LKML, weidu.du, Joe Perches,
	linux-erofs, Dan Carpenter

Hi Christoph,

On Fri, Aug 30, 2019 at 09:28:12AM -0700, Christoph Hellwig wrote:
> > -		err = bio_add_page(bio, page, PAGE_SIZE, 0);
> > -		if (err != PAGE_SIZE) {
> > +		if (bio_add_page(bio, page, PAGE_SIZE, 0) != PAGE_SIZE) {
> >  			err = -EFAULT;
> >  			goto err_out;
> >  		}
> 
> This patch looks like an improvement.  But looking at that whole
> area just makes me cringe.

OK, I agree with you, I will improve it or just kill them all with
new iomap approach after it supports tail-end packing inline.

> 
> Why is there __erofs_get_meta_page with the two weird booleans instead
> of a single erofs_get_meta_page that gets and gfp_t for additional
> flags and an unsigned int for additional bio op flags.

I agree with you. Thanks for your suggestion.

> 
> Why do need ioprio support to start with?  Seeing that in a new
> fs look kinda odd.  Do you have benchmarks that show the difference?

I don't have some benchmark for all of these, can I just set
REQ_PRIO for all metadata? is that reasonable?
Could you kindly give some suggestion on this?

> 
> That function then calls erofs_grab_bio, which tries to handle a
> bio_alloc failure, except that the function will not actually fail
> due the mempool backing it.  It also seems like and awfully
> huge function to inline.

OK, I will simplify it. Thanks for your suggestion.

> 
> Why is there __submit_bio which really just obsfucates what is
> going on?  Also why is __submit_bio using bio_set_op_attrs instead
> of opencode it as the comment right next to it asks you to?

Originally, mainly due to backport consideration since some
of our smartphones use 3.x kernel as well...

> 
> Also I really don't understand why you can't just use read_cache_page
> or even read_cache_page_gfp instead of __erofs_get_meta_page.
> That function is a whole lot of duplication of functionality shared
> by a lot of other file systems.

OK, I have to admit, that code was originally just copied from f2fs
with some modification (maybe it's not a good example for us).

Thanks,
Gao Xiang


^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v3 6/7] erofs: remove all likely/unlikely annotations
  2019-08-30 16:04             ` Gao Xiang
@ 2019-08-31 10:57               ` Dan Carpenter
  0 siblings, 0 replies; 38+ messages in thread
From: Dan Carpenter @ 2019-08-31 10:57 UTC (permalink / raw)
  To: Gao Xiang
  Cc: devel, Greg Kroah-Hartman, Miao Xie, LKML, Christoph Hellwig,
	weidu.du, Joe Perches, linux-erofs

On Sat, Aug 31, 2019 at 12:04:20AM +0800, Gao Xiang wrote:
> I don't have some benchmark data for each unlikely/likely case (and I have
> no idea "is that worth to take time to benchmark rather than do another more
> useful stuffs"), so..I have to kill them all...

We don't really require benchmarks, just that a reasonable person would
think it might make a difference.

regards,
dan carpenter

^ permalink raw reply	[flat|nested] 38+ messages in thread

end of thread, other threads:[~2019-08-31 10:58 UTC | newest]

Thread overview: 38+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-30  3:00 [PATCH v2 1/7] erofs: on-disk format should have explicitly assigned numbers Gao Xiang
2019-08-30  3:00 ` [PATCH v2 2/7] erofs: some marcos are much more readable as a function Gao Xiang
2019-08-30  3:16   ` Joe Perches
2019-08-30  3:20     ` Gao Xiang
2019-08-30  3:36       ` [PATCH v3 1/7] erofs: on-disk format should have explicitly assigned numbers Gao Xiang
2019-08-30  3:36         ` [PATCH v3 2/7] erofs: some macros are much more readable as a function Gao Xiang
2019-08-30  3:38           ` [PATCH v4 " Gao Xiang
2019-08-30  6:11           ` [PATCH v3 " Chao Yu
2019-08-30  3:36         ` [PATCH v3 3/7] erofs: use a better form for complicated on-disk fields Gao Xiang
2019-08-30  3:36         ` [PATCH v3 4/7] erofs: kill __packed for on-disk structures Gao Xiang
2019-08-30  6:16           ` Chao Yu
2019-08-30  3:36         ` [PATCH v3 5/7] erofs: kill erofs_{init,exit}_inode_cache Gao Xiang
2019-08-30  6:17           ` Chao Yu
2019-08-30  3:36         ` [PATCH v3 6/7] erofs: remove all likely/unlikely annotations Gao Xiang
2019-08-30  6:25           ` Chao Yu
2019-08-30  6:31             ` Gao Xiang
2019-08-30  6:43               ` Chao Yu
2019-08-30 11:55             ` Dan Carpenter
2019-08-30 11:30           ` Dan Carpenter
2019-08-30 12:06             ` Gao Xiang via Linux-erofs
2019-08-30 15:46           ` Christoph Hellwig
2019-08-30 16:04             ` Gao Xiang
2019-08-31 10:57               ` Dan Carpenter
2019-08-30  3:36         ` [PATCH v3 7/7] erofs: redundant assignment in __erofs_get_meta_page() Gao Xiang
2019-08-30  6:25           ` Chao Yu
2019-08-30 16:28           ` Christoph Hellwig
2019-08-30 16:48             ` Gao Xiang
2019-08-30 15:25         ` [PATCH v3 1/7] erofs: on-disk format should have explicitly assigned numbers Gao Xiang
2019-08-30 15:45     ` [PATCH v2 2/7] erofs: some marcos are much more readable as a function Christoph Hellwig
2019-08-30 15:52       ` Gao Xiang
2019-08-30 15:56         ` Gao Xiang
2019-08-30  3:00 ` [PATCH v2 3/7] erofs: use a better form for complicated on-disk fields Gao Xiang
2019-08-30  3:29   ` Chao Yu
2019-08-30  3:00 ` [PATCH v2 4/7] erofs: kill __packed for on-disk structures Gao Xiang
2019-08-30  3:00 ` [PATCH v2 5/7] erofs: kill erofs_{init,exit}_inode_cache Gao Xiang
2019-08-30  3:00 ` [PATCH v2 6/7] erofs: remove all likely/unlikely annotations Gao Xiang
2019-08-30  3:00 ` [PATCH v2 7/7] erofs: reduntant assignment in __erofs_get_meta_page() Gao Xiang
2019-08-30  3:28 ` [PATCH v2 1/7] erofs: on-disk format should have explicitly assigned numbers Chao Yu

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).