[PATCH v4 2/7] erofs: some macros are much more readable as a function
As Christoph suggested [1], these macros are much
more readable as a function.
[1] https://lore.kernel.org/r/20190829095954.gb20...@infradead.org/
Reported-by: Christoph Hellwig
Signed-off-by: Gao Xiang
---
v4: a type fix in commit message.
v3: change as Joe suggested,
https://lore.kernel.org/r/5b2ecf5cec1a6aa3834e9af41886a7fcb18ae86a.ca...@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();
+ unsigned int entry_sz = erofs_xattr_entry_size();
/* xattr on-disk corruption: xattr entry beyond xattr_isize */
if (unlikely(*tlimit < entry_sz)) {
--
2.17.1
[PATCH v3 3/7] erofs: use a better form for complicated on-disk fields
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.ca...@perches.com/
Reported-by: Joe Perches
Reviewed-by: Chao Yu
Signed-off-by: Gao Xiang
---
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 0x0001
#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_BIT1
-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
[PATCH v3 1/7] erofs: on-disk format should have explicitly assigned numbers
As Christoph claimed [1], on-disk format should have
explicitly assigned numbers. I have to change it.
[1] https://lore.kernel.org/r/20190829095954.gb20...@infradead.org/
Reported-by: Christoph Hellwig
Reviewed-by: Chao Yu
Signed-off-by: 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
[PATCH v3 4/7] erofs: kill __packed for on-disk structures
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.gb20...@infradead.org/
Reported-by: Christoph Hellwig
Signed-off-by: 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 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
[PATCH v3 7/7] erofs: redundant assignment in __erofs_get_meta_page()
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 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.ca...@perches.com/
Reported-by: Joe Perches
Signed-off-by: Gao Xiang
---
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
[PATCH v3 2/7] erofs: some macros are much more readable as a function
As Christoph suggested [1], these marcos are much
more readable as a function.
[1] https://lore.kernel.org/r/20190829095954.gb20...@infradead.org/
Reported-by: Christoph Hellwig
Signed-off-by: Gao Xiang
---
v3: change as Joe suggested,
https://lore.kernel.org/r/5b2ecf5cec1a6aa3834e9af41886a7fcb18ae86a.ca...@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();
+ unsigned int entry_sz = erofs_xattr_entry_size();
/* xattr on-disk corruption: xattr entry beyond xattr_isize */
if (unlikely(*tlimit < entry_sz)) {
--
2.17.1
[PATCH v3 5/7] erofs: kill erofs_{init,exit}_inode_cache
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.gc20...@infradead.org/
Reported-by: Christoph Hellwig
Signed-off-by: Gao Xiang
---
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(>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(_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
[PATCH v3 6/7] erofs: remove all likely/unlikely annotations
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
Signed-off-by: Gao Xiang
---
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, , 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);
-
Re: [PATCH v2 3/7] erofs: use a better form for complicated on-disk fields
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.ca...@perches.com/ > Reported-by: Joe Perches > Signed-off-by: Gao Xiang Reviewed-by: Chao Yu Thanks,
Re: [PATCH v2 1/7] erofs: on-disk format should have explicitly assigned numbers
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.gb20...@infradead.org/ > Reported-by: Christoph Hellwig > Signed-off-by: Gao Xiang Reviewed-by: Chao Yu Thanks,
Re: [PATCH v2 2/7] erofs: some marcos are much more readable as a function
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
>
Re: [PATCH v2 2/7] erofs: some marcos are much more readable as a function
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);
}
[PATCH v2 7/7] erofs: reduntant assignment in __erofs_get_meta_page()
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 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.ca...@perches.com/
Reported-by: Joe Perches
Signed-off-by: Gao Xiang
---
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
[PATCH v2 1/7] erofs: on-disk format should have explicitly assigned numbers
As Christoph claimed [1], on-disk format should have
explicitly assigned numbers. I have to change it.
[1] https://lore.kernel.org/r/20190829095954.gb20...@infradead.org/
Reported-by: Christoph Hellwig
Signed-off-by: Gao Xiang
---
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
[PATCH v2 4/7] erofs: kill __packed for on-disk structures
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.gb20...@infradead.org/
Reported-by: Christoph Hellwig
Signed-off-by: Gao Xiang
---
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
[PATCH v2 2/7] erofs: some marcos are much more readable as a function
As Christoph suggested [1], these marcos are much
more readable as a function.
[1] https://lore.kernel.org/r/20190829095954.gb20...@infradead.org/
Reported-by: Christoph Hellwig
Signed-off-by: Gao Xiang
---
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();
+ unsigned int entry_sz = erofs_xattr_entry_size();
/* xattr on-disk corruption: xattr entry beyond xattr_isize */
if (unlikely(*tlimit < entry_sz)) {
--
2.17.1
[PATCH v2 6/7] erofs: remove all likely/unlikely annotations
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
Signed-off-by: Gao Xiang
---
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, , 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 =
[PATCH v2 3/7] erofs: use a better form for complicated on-disk fields
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.ca...@perches.com/
Reported-by: Joe Perches
Signed-off-by: Gao Xiang
---
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 0x0001
#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_BIT1
-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
[PATCH v2 5/7] erofs: kill erofs_{init,exit}_inode_cache
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.gc20...@infradead.org/
Reported-by: Christoph Hellwig
Signed-off-by: Gao Xiang
---
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(>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(_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
Re: [PATCH v8] Add flags option to get xattr method paired to __vfs_getxattr
I added this patch series on top of Linux 5.3-rc6 and ran xfstests on orangefs with no regressions. Acked-by: Mike Marshall -Mike On Wed, Aug 28, 2019 at 10:40 AM Mark Salyzyn wrote: > > On 8/28/19 7:24 AM, Christoph Hellwig wrote: > > On Tue, Aug 27, 2019 at 08:05:15AM -0700, Mark Salyzyn wrote: > >> Replace arguments for get and set xattr methods, and __vfs_getxattr > >> and __vfs_setaxtr functions with a reference to the following now > >> common argument structure: > > Yikes. That looks like a mess. Why can't we pass a kernel-only > > flag in the existing flags field for ₋>set and add a flags field > > to ->get? Passing methods by structure always tends to be a mess. > > This was a response to GregKH@ criticism, an earlier patch set just > added a flag as you stated to get method, until complaints of an > excessively long argument list and fragility to add or change more > arguments. > > So many ways have been tried to skin this cat ... the risk was taken to > please some, and we now have hundreds of stakeholders, when the first > patch set was less than a dozen. A recipe for failure? > > -- Mark >
Re: [PATCH] staging: erofs: using switch-case while checking the inode type.
Hi Dan, On Thu, Aug 29, 2019 at 11:13:53PM +0800, Gao Xiang wrote: > Hi Dan, > > On Thu, Aug 29, 2019 at 06:04:36PM +0300, Dan Carpenter wrote: > > On Thu, Aug 29, 2019 at 10:15:22PM +0800, Gao Xiang wrote: > > > I am very happy that you send a patch about this, but we have > > > to take care of handling "fall through" properly at least, > > > and I don't want to introduce some extra compile warnings > > > instead at this time. > > > > I can't apply the patch so I maybe missed something. I don't see > > a fall through issue. We have the code so you could use to > > indicate which lines have a fall through problem. > > > > > > > > EROFS is sensitive for now and I have no idea what the "real" > > > point is. > > > > What does "sensitive" mean here? Now that it's out of staging we > > aren't applying clean up patches? Again, due to language obstacle, I have to give a detailed explanation of what "sensitive" meant here. I meant it seems all topic had no relationship with EROFS at all, but someone mentioned erofs in different topics, I have no idea what is wrong with EROFS, and I have no idea where it is like POS. As your unlikely/likely concern, I think I discussed with you earlier, in fact, most of EROFS unlikely is due to error handling paths, which are meaningful in my thought. If you argue that it has little performance difference, I think unlikely in IS_ERR can also be killed as well since for most use cases in Linux it is true that they are little performance difference at all. But I think totally it will have some impact. Anyway, I didn't find some formal standard about this, and I remove all of these as you like. Thanks, Gao Xiang > > Of course not, I mean we should avoid "fall through" problem > but I have no time to verify this patch since I am fixing what > hch said as well. > > Thanks, > Gao Xiang > > > > > regards, > > dan carpenter > >
Re: [PATCH v6 01/24] erofs: add on-disk layout
Hi Joe,
On Thu, Aug 29, 2019 at 08:58:17AM -0700, Joe Perches wrote:
> On Thu, 2019-08-29 at 18:32 +0800, Gao Xiang wrote:
> > Hi Christoph,
> >
> > On Thu, Aug 29, 2019 at 02:59:54AM -0700, Christoph Hellwig wrote:
> > > > --- /dev/null
> > > > +++ b/fs/erofs/erofs_fs.h
> > > > @@ -0,0 +1,316 @@
> > > > +/* SPDX-License-Identifier: GPL-2.0-only OR Apache-2.0 */
> > > > +/*
> > > > + * linux/fs/erofs/erofs_fs.h
> > >
> > > Please remove the pointless file names in the comment headers.
> >
> > Already removed in the latest version.
> >
> > > > +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 */
> > >
> > > Please remove all the byte offset comments. That is something that can
> > > easily be checked with gdb or pahole.
> >
> > I have no idea the actual issue here.
> > It will help all developpers better add fields or calculate
> > these offsets in their mind, and with care.
> >
> > Rather than they didn't run "gdb" or "pahole" and change it by mistake.
>
> I think Christoph is not right here.
>
> Using external tools for validation is extra work
> when necessary for understanding the code.
>
> The expected offset is somewhat valuable, but
> perhaps the form is a bit off given the visual
> run-in to the field types.
>
> The extra work with this form is manipulating all
> the offsets whenever a structure change occurs.
>
> The comments might be better with a form more like:
Thanks for your comment.
I will change those places as you suggested, that is fine.
Thanks,
Gao Xiang
>
> struct erofs_super_block {/* offset description */
> __le32 magic; /* 0 */
> __le32 checksum;/* 4 crc32c(super_block) */
> __le32 features;/* 8 (aka. feature_compat) */
> __u8 blkszbits; /* 12 support block_size == PAGE_SIZE only */
>
>
[PATCH] erofs: reduntant assignment in __erofs_get_meta_page()
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 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.ca...@perches.com/
Signed-off-by: Gao Xiang
---
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
Re: [PATCH v8 00/24] erofs: promote erofs from staging v8
on 2019/8/15 at 12:41, Gao Xiang wrote: > [I strip the previous cover letter, the old one can be found in v6: > https://lore.kernel.org/r/20190802125347.166018-1-gaoxian...@huawei.com/] > > We'd like to submit a formal moving patch applied to staging tree > for 5.4, before that we'd like to hear if there are some ACKs, > suggestions or NAKs, objections of EROFS. Therefore, we can improve > it in this round or rethink about the whole thing. ACK since it is stable enough and doesn't affect vfs or other filesystems. And then, it could attract more users, we can get more feedback, and they can help us to further improve it. Thanks Miao > > As related materials mentioned [1] [2], the goal of EROFS is to > save extra storage space with guaranteed end-to-end performance > for read-only files, which has better performance over exist Linux > compression filesystems based on fixed-sized output compression > and inplace decompression. It even has better performance in > a large compression ratio range compared with generic uncompressed > filesystems with proper CPU-storage combinations. And we think this > direction is correct and a dedicated kernel team is continuously / > actively working on improving it, enough testers and beta / end > users using it. > > EROFS has been applied to almost all in-service HUAWEI smartphones > (Yes, the number is still increasing by time) and it seems like > a success. It can be used in more wider scenarios. We think it's > useful for Linux / Android OS community and it's the time moving > out of staging. > > In order to get started, latest stable mkfs.erofs is available at > > git://git.kernel.org/pub/scm/linux/kernel/git/xiang/erofs-utils.git -b dev > > with README in the repository. > > We are still tuning sequential read performance for ultra-fast > speed NVME SSDs like Samsung 970PRO, but at least now you can > try on your PC with some data with proper compression ratio, > the latest Linux kernel, USB stick for convenience sake and > a not very old-fashioned CPU. There are also benchmarks available > in the above materials mentioned. > > EROFS is a self-contained filesystem driver. Although there are > still some TODOs to be more generic, we will actively keep on > developping / tuning EROFS with the evolution of Linux kernel > as the other in-kernel filesystems. > > As I mentioned before in LSF/MM 2019, in the future, we'd like > to generalize the decompression engine into a library for other > fses to use after the whole system is mature like fscrypt. > However, such metadata should be designed respectively for > each fs, and synchronous metadata read cost will be larger > than EROFS because of those ondisk limitation. Therefore EROFS > is still a better choice for read-only scenarios. > > EROFS is now ready for reviewing and moving, and the code is > already cleaned up as shiny floors... Please kindly take some > precious time, share your comments about EROFS and let us know > your opinion about this. It's really important for us since > generally speaking, we like to use Linux _in-tree_ stuffs rather > than lack of supported out-of-tree / orphan stuffs as well. > > Thank you in advance, > Gao Xiang > > [1] > https://kccncosschn19eng.sched.com/event/Nru2/erofs-an-introduction-and-our-smartphone-practice-xiang-gao-huawei > [2] https://www.usenix.org/conference/atc19/presentation/gao > > Changelog from v7: > o keep up with the latest staging tree in addition to >the latest staging patch: >https://lore.kernel.org/r/20190814103705.60698-1-gaoxian...@huawei.com/ >- use EUCLEAN for fs corruption cases suggested by Pavel; >- turn EIO into EOPNOTSUPP for unsupported on-disk format; >- fix all misused ENOTSUPP into EOPNOTSUPP pointed out by Chao; > o update cover letter > > It can also be found in git at tag "erofs_2019-08-15" (will be shown later) > at: > https://git.kernel.org/pub/scm/linux/kernel/git/xiang/linux.git/ > > and the latest fs code is available at: > > https://git.kernel.org/pub/scm/linux/kernel/git/xiang/linux.git/tree/fs/erofs?h=erofs-outofstaging > > Changelog from v6: > o keep up with the latest staging patchset > > https://lore.kernel.org/linux-fsdevel/20190813023054.73126-1-gaoxian...@huawei.com/ >in order to fix the following cases: >- inline erofs_inode_is_data_compressed() in erofs_fs.h; >- remove incomplete cleancache; >- remove all BUG_ON in EROFS. > o Removing the file names from the comments at the top of the files >suggested by Stephen will be applied to the real moving patch later. > > Changelog from v5: > o keep up with "[PATCH v2] staging: erofs: updates according to > erofs-outofstaging v4" > > https://lore.kernel.org/lkml/20190731155752.210602-1-gaoxian...@huawei.com/ >which mainly addresses review comments from Chao: > - keep the marco EROFS_IO_MAX_RETRIES_NOFAIL in internal.h; > - kill a redundant NULL check in "__stagingpage_alloc"; > - add some
[PATCH] erofs: remove all likely/unlikely annotations
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
Signed-off-by: Gao Xiang
---
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, , 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
Re: [PATCH v6 01/24] erofs: add on-disk layout
On Thu, 2019-08-29 at 18:32 +0800, Gao Xiang wrote:
> Hi Christoph,
>
> On Thu, Aug 29, 2019 at 02:59:54AM -0700, Christoph Hellwig wrote:
> > > --- /dev/null
> > > +++ b/fs/erofs/erofs_fs.h
> > > @@ -0,0 +1,316 @@
> > > +/* SPDX-License-Identifier: GPL-2.0-only OR Apache-2.0 */
> > > +/*
> > > + * linux/fs/erofs/erofs_fs.h
> >
> > Please remove the pointless file names in the comment headers.
>
> Already removed in the latest version.
>
> > > +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
> > > */
> >
> > Please remove all the byte offset comments. That is something that can
> > easily be checked with gdb or pahole.
>
> I have no idea the actual issue here.
> It will help all developpers better add fields or calculate
> these offsets in their mind, and with care.
>
> Rather than they didn't run "gdb" or "pahole" and change it by mistake.
I think Christoph is not right here.
Using external tools for validation is extra work
when necessary for understanding the code.
The expected offset is somewhat valuable, but
perhaps the form is a bit off given the visual
run-in to the field types.
The extra work with this form is manipulating all
the offsets whenever a structure change occurs.
The comments might be better with a form more like:
struct erofs_super_block { /* offset description */
__le32 magic; /* 0 */
__le32 checksum;/* 4 crc32c(super_block) */
__le32 features;/* 8 (aka. feature_compat) */
__u8 blkszbits; /* 12 support block_size == PAGE_SIZE only */
Re: [PATCH v6 01/24] erofs: add on-disk layout
Hi Christoph,
On Thu, Aug 29, 2019 at 02:59:54AM -0700, Christoph Hellwig wrote:
[]
>
> > +static bool erofs_inode_is_data_compressed(unsigned int datamode)
> > +{
> > + if (datamode == EROFS_INODE_FLAT_COMPRESSION)
> > + return true;
> > + return datamode == EROFS_INODE_FLAT_COMPRESSION_LEGACY;
> > +}
>
> This looks like a really obsfucated way to write:
>
> return datamode == EROFS_INODE_FLAT_COMPRESSION ||
> datamode == EROFS_INODE_FLAT_COMPRESSION_LEGACY;
Add a word about this, the above approach is not horrible if more
datamode add here and comments, e.g
static bool erofs_inode_is_data_compressed(unsigned int datamode)
{
/* has z_erofs_map_header */
if (datamode == EROFS_INODE_FLAT_COMPRESSION)
return true;
/* some blablabla */
if (datamode == (1) )
return true;
/* some blablablabla */
if (datamode == (2) )
return true;
/* no z_erofs_map_header */
return datamode == EROFS_INODE_FLAT_COMPRESSION_LEGACY;
}
vs.
static bool erofs_inode_is_data_compressed(unsigned int datamode)
{
/* has z_erofs_map_header */
return datamode == EROFS_INODE_FLAT_COMPRESSION ||
/* some blablabla */
datamode == (1) ||
/* some blablablabla */
datamode == (2) ||
/* no z_erofs_map_header */
datamode == EROFS_INODE_FLAT_COMPRESSION_LEGACY;
}
I have no idea which one is better.
Anyway, if you still like the form, I will change it.
Thanks,
Gao Xiang
[PATCH 1/2] erofs: on-disk format should have explicitly assigned numbers
As Christoph claimed [1], on-disk format should have
explicitly assigned numbers.
[1] https://lore.kernel.org/r/20190829095954.gb20...@infradead.org/
Reported-by: Christoph Hellwig
Signed-off-by: Gao Xiang
---
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
[PATCH 2/2] erofs: some marcos are much more readable as a function
As Christoph suggested [1], these marcos are much
more readable as a function.
[1] https://lore.kernel.org/r/20190829095954.gb20...@infradead.org/
Reported-by: Christoph Hellwig
Signed-off-by: Gao Xiang
---
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();
+ unsigned int entry_sz = erofs_xattr_entry_size();
/* xattr on-disk corruption: xattr entry beyond xattr_isize */
if (unlikely(*tlimit < entry_sz)) {
--
2.17.1
Re: [PATCH] staging: erofs: using switch-case while checking the inode type.
Hi Dan, On Thu, Aug 29, 2019 at 06:04:36PM +0300, Dan Carpenter wrote: > On Thu, Aug 29, 2019 at 10:15:22PM +0800, Gao Xiang wrote: > > I am very happy that you send a patch about this, but we have > > to take care of handling "fall through" properly at least, > > and I don't want to introduce some extra compile warnings > > instead at this time. > > I can't apply the patch so I maybe missed something. I don't see > a fall through issue. We have the code so you could use to > indicate which lines have a fall through problem. > > > > > EROFS is sensitive for now and I have no idea what the "real" > > point is. > > What does "sensitive" mean here? Now that it's out of staging we > aren't applying clean up patches? Of course not, I mean we should avoid "fall through" problem but I have no time to verify this patch since I am fixing what hch said as well. Thanks, Gao Xiang > > regards, > dan carpenter >
Re: [PATCH] staging: erofs: using switch-case while checking the inode type.
On Thu, Aug 29, 2019 at 10:15:22PM +0800, Gao Xiang wrote: > I am very happy that you send a patch about this, but we have > to take care of handling "fall through" properly at least, > and I don't want to introduce some extra compile warnings > instead at this time. I can't apply the patch so I maybe missed something. I don't see a fall through issue. We have the code so you could use to indicate which lines have a fall through problem. > > EROFS is sensitive for now and I have no idea what the "real" > point is. What does "sensitive" mean here? Now that it's out of staging we aren't applying clean up patches? regards, dan carpenter
Re: [PATCH] staging: erofs: using switch-case while checking the inode type.
On Thu, Aug 29, 2019 at 09:56:07PM +0800, Gao Xiang wrote: > Hi Pratik, > > On Thu, Aug 29, 2019 at 06:38:13PM +0530, Pratik Shinde wrote: > > while filling the linux inode, using switch-case statement to check > > the type of inode. > > switch-case statement looks more clean. > > > > Signed-off-by: Pratik Shinde > > No, that is not the case, see __ext4_iget() in fs/ext4/inode.c. Unnecessary complaining. > and could you write patches based on latest staging tree? > erofs is now in "fs/" rather than "drivers/staging". > and I will review it then. > > p.s. if someone argues here or there, there will be endless > issues since there is no standard at all. More complaining... It doesn't matter if you can find ext4 that looks like dog poop, that's irrelevant. regards, dan carpenter
Re: [PATCH] staging: erofs: using switch-case while checking the inode type.
On Thu, Aug 29, 2019 at 07:35:01PM +0530, Pratik Shinde wrote:
> Hi Gao,
>
> Sorry I didn't pull the latest tree. I will do the necessary.
> Anyways, don't you think it will be cleaner to have a switch case statement
> rather than if-else statement.
I think so, but that's another personal choise and no urgent
as well (It is just a cleanup to some extent).
I am very happy that you send a patch about this, but we have
to take care of handling "fall through" properly at least,
and I don't want to introduce some extra compile warnings
instead at this time.
EROFS is sensitive for now and I have no idea what the "real"
point is.
Thanks,
Gao Xiang
>
> --Pratik
>
>
>
> On Thu, 29 Aug, 2019, 7:27 PM Gao Xiang, wrote:
>
> > Hi Pratik,
> >
> > On Thu, Aug 29, 2019 at 06:38:13PM +0530, Pratik Shinde wrote:
> > > while filling the linux inode, using switch-case statement to check
> > > the type of inode.
> > > switch-case statement looks more clean.
> > >
> > > Signed-off-by: Pratik Shinde
> >
> > No, that is not the case, see __ext4_iget() in fs/ext4/inode.c.
> > and could you write patches based on latest staging tree?
> > erofs is now in "fs/" rather than "drivers/staging".
> > and I will review it then.
> >
> > p.s. if someone argues here or there, there will be endless
> > issues since there is no standard at all.
> >
> > Thanks,
> > Gao Xiang
> >
> > > ---
> > > drivers/staging/erofs/inode.c | 18 --
> > > 1 file changed, 12 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/staging/erofs/inode.c
> > b/drivers/staging/erofs/inode.c
> > > index 4c3d8bf..2d2d545 100644
> > > --- a/drivers/staging/erofs/inode.c
> > > +++ b/drivers/staging/erofs/inode.c
> > > @@ -190,22 +190,28 @@ static int fill_inode(struct inode *inode, int
> > isdir)
> > > err = read_inode(inode, data + ofs);
> > > if (!err) {
> > > /* setup the new inode */
> > > - if (S_ISREG(inode->i_mode)) {
> > > + switch (inode->i_mode & S_IFMT) {
> > > + case S_IFREG:
> > > inode->i_op = _generic_iops;
> > > inode->i_fop = _ro_fops;
> > > - } else if (S_ISDIR(inode->i_mode)) {
> > > + break;
> > > + case S_IFDIR:
> > > inode->i_op = _dir_iops;
> > > inode->i_fop = _dir_fops;
> > > - } else if (S_ISLNK(inode->i_mode)) {
> > > + break;
> > > + case S_IFLNK:
> > > /* by default, page_get_link is used for symlink */
> > > inode->i_op = _symlink_iops;
> > > inode_nohighmem(inode);
> > > - } else if (S_ISCHR(inode->i_mode) ||
> > S_ISBLK(inode->i_mode) ||
> > > - S_ISFIFO(inode->i_mode) ||
> > S_ISSOCK(inode->i_mode)) {
> > > + break;
> > > + case S_IFCHR:
> > > + case S_IFBLK:
> > > + case S_IFIFO:
> > > + case S_IFSOCK:
> > > inode->i_op = _generic_iops;
> > > init_special_inode(inode, inode->i_mode,
> > inode->i_rdev);
> > > goto out_unlock;
> > > - } else {
> > > + default:
> > > err = -EIO;
> > > goto out_unlock;
> > > }
> > > --
> > > 2.9.3
> > >
> >
Re: [PATCH] staging: erofs: using switch-case while checking the inode type.
Hi Gao,
Sorry I didn't pull the latest tree. I will do the necessary.
Anyways, don't you think it will be cleaner to have a switch case statement
rather than if-else statement.
--Pratik
On Thu, 29 Aug, 2019, 7:27 PM Gao Xiang, wrote:
> Hi Pratik,
>
> On Thu, Aug 29, 2019 at 06:38:13PM +0530, Pratik Shinde wrote:
> > while filling the linux inode, using switch-case statement to check
> > the type of inode.
> > switch-case statement looks more clean.
> >
> > Signed-off-by: Pratik Shinde
>
> No, that is not the case, see __ext4_iget() in fs/ext4/inode.c.
> and could you write patches based on latest staging tree?
> erofs is now in "fs/" rather than "drivers/staging".
> and I will review it then.
>
> p.s. if someone argues here or there, there will be endless
> issues since there is no standard at all.
>
> Thanks,
> Gao Xiang
>
> > ---
> > drivers/staging/erofs/inode.c | 18 --
> > 1 file changed, 12 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/staging/erofs/inode.c
> b/drivers/staging/erofs/inode.c
> > index 4c3d8bf..2d2d545 100644
> > --- a/drivers/staging/erofs/inode.c
> > +++ b/drivers/staging/erofs/inode.c
> > @@ -190,22 +190,28 @@ static int fill_inode(struct inode *inode, int
> isdir)
> > err = read_inode(inode, data + ofs);
> > if (!err) {
> > /* setup the new inode */
> > - if (S_ISREG(inode->i_mode)) {
> > + switch (inode->i_mode & S_IFMT) {
> > + case S_IFREG:
> > inode->i_op = _generic_iops;
> > inode->i_fop = _ro_fops;
> > - } else if (S_ISDIR(inode->i_mode)) {
> > + break;
> > + case S_IFDIR:
> > inode->i_op = _dir_iops;
> > inode->i_fop = _dir_fops;
> > - } else if (S_ISLNK(inode->i_mode)) {
> > + break;
> > + case S_IFLNK:
> > /* by default, page_get_link is used for symlink */
> > inode->i_op = _symlink_iops;
> > inode_nohighmem(inode);
> > - } else if (S_ISCHR(inode->i_mode) ||
> S_ISBLK(inode->i_mode) ||
> > - S_ISFIFO(inode->i_mode) ||
> S_ISSOCK(inode->i_mode)) {
> > + break;
> > + case S_IFCHR:
> > + case S_IFBLK:
> > + case S_IFIFO:
> > + case S_IFSOCK:
> > inode->i_op = _generic_iops;
> > init_special_inode(inode, inode->i_mode,
> inode->i_rdev);
> > goto out_unlock;
> > - } else {
> > + default:
> > err = -EIO;
> > goto out_unlock;
> > }
> > --
> > 2.9.3
> >
>
Re: [PATCH] staging: erofs: using switch-case while checking the inode type.
Hi Pratik,
On Thu, Aug 29, 2019 at 06:38:13PM +0530, Pratik Shinde wrote:
> while filling the linux inode, using switch-case statement to check
> the type of inode.
> switch-case statement looks more clean.
>
> Signed-off-by: Pratik Shinde
No, that is not the case, see __ext4_iget() in fs/ext4/inode.c.
and could you write patches based on latest staging tree?
erofs is now in "fs/" rather than "drivers/staging".
and I will review it then.
p.s. if someone argues here or there, there will be endless
issues since there is no standard at all.
Thanks,
Gao Xiang
> ---
> drivers/staging/erofs/inode.c | 18 --
> 1 file changed, 12 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/staging/erofs/inode.c b/drivers/staging/erofs/inode.c
> index 4c3d8bf..2d2d545 100644
> --- a/drivers/staging/erofs/inode.c
> +++ b/drivers/staging/erofs/inode.c
> @@ -190,22 +190,28 @@ static int fill_inode(struct inode *inode, int isdir)
> err = read_inode(inode, data + ofs);
> if (!err) {
> /* setup the new inode */
> - if (S_ISREG(inode->i_mode)) {
> + switch (inode->i_mode & S_IFMT) {
> + case S_IFREG:
> inode->i_op = _generic_iops;
> inode->i_fop = _ro_fops;
> - } else if (S_ISDIR(inode->i_mode)) {
> + break;
> + case S_IFDIR:
> inode->i_op = _dir_iops;
> inode->i_fop = _dir_fops;
> - } else if (S_ISLNK(inode->i_mode)) {
> + break;
> + case S_IFLNK:
> /* by default, page_get_link is used for symlink */
> inode->i_op = _symlink_iops;
> inode_nohighmem(inode);
> - } else if (S_ISCHR(inode->i_mode) || S_ISBLK(inode->i_mode) ||
> - S_ISFIFO(inode->i_mode) || S_ISSOCK(inode->i_mode)) {
> + break;
> + case S_IFCHR:
> + case S_IFBLK:
> + case S_IFIFO:
> + case S_IFSOCK:
> inode->i_op = _generic_iops;
> init_special_inode(inode, inode->i_mode, inode->i_rdev);
> goto out_unlock;
> - } else {
> + default:
> err = -EIO;
> goto out_unlock;
> }
> --
> 2.9.3
>
[PATCH] staging: erofs: using switch-case while checking the inode type.
while filling the linux inode, using switch-case statement to check
the type of inode.
switch-case statement looks more clean.
Signed-off-by: Pratik Shinde
---
drivers/staging/erofs/inode.c | 18 --
1 file changed, 12 insertions(+), 6 deletions(-)
diff --git a/drivers/staging/erofs/inode.c b/drivers/staging/erofs/inode.c
index 4c3d8bf..2d2d545 100644
--- a/drivers/staging/erofs/inode.c
+++ b/drivers/staging/erofs/inode.c
@@ -190,22 +190,28 @@ static int fill_inode(struct inode *inode, int isdir)
err = read_inode(inode, data + ofs);
if (!err) {
/* setup the new inode */
- if (S_ISREG(inode->i_mode)) {
+ switch (inode->i_mode & S_IFMT) {
+ case S_IFREG:
inode->i_op = _generic_iops;
inode->i_fop = _ro_fops;
- } else if (S_ISDIR(inode->i_mode)) {
+ break;
+ case S_IFDIR:
inode->i_op = _dir_iops;
inode->i_fop = _dir_fops;
- } else if (S_ISLNK(inode->i_mode)) {
+ break;
+ case S_IFLNK:
/* by default, page_get_link is used for symlink */
inode->i_op = _symlink_iops;
inode_nohighmem(inode);
- } else if (S_ISCHR(inode->i_mode) || S_ISBLK(inode->i_mode) ||
- S_ISFIFO(inode->i_mode) || S_ISSOCK(inode->i_mode)) {
+ break;
+ case S_IFCHR:
+ case S_IFBLK:
+ case S_IFIFO:
+ case S_IFSOCK:
inode->i_op = _generic_iops;
init_special_inode(inode, inode->i_mode, inode->i_rdev);
goto out_unlock;
- } else {
+ default:
err = -EIO;
goto out_unlock;
}
--
2.9.3
Re: [PATCH v6 05/24] erofs: add inode operations
On Thu, Aug 29, 2019 at 03:24:26AM -0700, Christoph Hellwig wrote:
[]
>
> > +
> > + /* fill last page if inline data is available */
> > + err = fill_inline_data(inode, data, ofs);
>
> Well, I think you should move the is_inode_flat_inline and
> (S_ISLNK(inode->i_mode) && inode->i_size < PAGE_SIZE) checks from that
> helper here, as otherwise you make everyone wonder why you'd always
> fill out the inline data.
Currently, fill_inline_data() only fills for fast symlink,
later we can fill any tail-end block (such as dir block)
for our requirements.
And I think that is minor.
>
> > +static inline struct inode *erofs_iget_locked(struct super_block *sb,
> > + erofs_nid_t nid)
> > +{
> > + const unsigned long hashval = erofs_inode_hash(nid);
> > +
> > +#if BITS_PER_LONG >= 64
> > + /* it is safe to use iget_locked for >= 64-bit platform */
> > + return iget_locked(sb, hashval);
> > +#else
> > + return iget5_locked(sb, hashval, erofs_ilookup_test_actor,
> > + erofs_iget_set_actor, );
> > +#endif
>
> Just use the slightly more complicated 32-bit version everywhere so that
> you have a single actually tested code path. And then remove this
> helper.
The consideration is simply because iget_locked performs better
than iget5_locked.
Thanks,
Gao Xiang
Re: [PATCH v6 04/24] erofs: add raw address_space operations
Hi Christoph, On Thu, Aug 29, 2019 at 03:17:21AM -0700, Christoph Hellwig wrote: > The actual address_space operations seem to largely duplicate > the iomap versions. Please use those instead. Also I don't think > any new file system should write up ->bmap these days. iomap doesn't support tail-end packing inline data till now, I think Chao and I told you and Andreas before [1]. Since EROFS keeps a self-contained driver for now, we will use iomap if it supports tail-end packing inline data later. [1] https://lore.kernel.org/linux-fsdevel/90fca1c4-c142-992d-ebf3-03c8017f9...@huawei.com/ Thanks, Gao Xiang
Re: [PATCH v6 08/24] erofs: add namei functions
On Thu, Aug 29, 2019 at 03:28:38AM -0700, Christoph Hellwig wrote:
> On Fri, Aug 02, 2019 at 08:53:31PM +0800, Gao Xiang wrote:
> > +struct erofs_qstr {
> > + const unsigned char *name;
> > + const unsigned char *end;
> > +};
>
> Maybe erofs_name? The q in qstr stands for quick, because of the
> existing hash and len, which this doesn't really provide.
>
> Also I don't really see why you don't just pass the actual qstr and
> just document that dirnamecmp does not look at the hash and thus
> doesn't require it to be filled out.
q in erofs_qstr also means quick substring.
If you have some time to look into it more, it uses a prefixed
binary search algorithm (rather than linear traversal), which
provides similar proformance with hashed approach but no need
to save such hash field and it's natively sorted in alphabet
order.
Thanks,
Gao Xiang
>
Re: [PATCH v6 01/24] erofs: add on-disk layout
Hi Christoph, On Thu, Aug 29, 2019 at 03:36:04AM -0700, Christoph Hellwig wrote: > On Thu, Aug 29, 2019 at 06:32:53PM +0800, Gao Xiang wrote: > > I can fix it up as you like but I still cannot get > > what is critical issues here. > > The problem is that the whole codebase is way substandard quality, > looking a lot like Linux code from 20 years ago. Yes, we already have > plenty of code of that standard in the tree, but we should not add more. I still cannot get your point what does your substandard quality mean, please refer to some thing critical in EROFS (and I noticed that your new code still has bug) rather than naming. Thanks, Gao Xiang
Re: [PATCH v6 03/24] erofs: add super block operations
Hi Christoph,
On Thu, Aug 29, 2019 at 03:15:45AM -0700, Christoph Hellwig wrote:
> On Fri, Aug 02, 2019 at 08:53:26PM +0800, Gao Xiang wrote:
> > +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;
>
> Please just use normal if/else. Also having this function seems
> entirely pointless.
>
> > +static void erofs_exit_inode_cache(void)
> > +{
> > + kmem_cache_destroy(erofs_inode_cachep);
> > +}
>
> Same for this one.
>
> > +static void free_inode(struct inode *inode)
>
> Please use an erofs_ prefix for all your functions.
It is already a static function, I have no idea what is wrong here.
>
> > +{
> > + struct erofs_vnode *vi = EROFS_V(inode);
>
> Why is this called vnode instead of inode? That seems like a rather
> odd naming for a Linux file system.
I don't know anything difference of that, it is just a naming.
>
> > +
> > + /* be careful RCU symlink path (see ext4_inode_info->i_data)! */
> > + if (is_inode_fast_symlink(inode))
> > + kfree(inode->i_link);
>
> is_inode_fast_symlink only shows up in a later patch. And really
> obsfucates the check here in the only caller as you can just do an
> unconditional kfree here - i_link will be NULL except for the case
> where you explicitly set it.
I cannot fully understand your point (sorry about my English),
I will reply you about this later.
>
> Also this code is nothing like ext4, so the code seems a little confusing.
>
> > +static bool check_layout_compatibility(struct super_block *sb,
> > + struct erofs_super_block *layout)
> > +{
> > + const unsigned int requirements = le32_to_cpu(layout->requirements);
>
> Why is the variable name for the on-disk subperblock layout? We usually
> still calls this something with sb in the name, e.g. dsb. for disk
> super block.
I can change it later, sbi and dsb (It has not good meaning in Chinese,
although).
>
> > + EROFS_SB(sb)->requirements = requirements;
> > +
> > + /* check if current kernel meets all mandatory requirements */
> > + if (requirements & (~EROFS_ALL_REQUIREMENTS)) {
> > + errln("unidentified requirements %x, please upgrade kernel
> > version",
> > + requirements & ~EROFS_ALL_REQUIREMENTS);
> > + return false;
> > + }
> > + return true;
>
> Note that normally we call this features, but that doesn't really
> matter too much.
>
> > +static int superblock_read(struct super_block *sb)
> > +{
> > + struct erofs_sb_info *sbi;
> > + struct buffer_head *bh;
> > + struct erofs_super_block *layout;
> > + unsigned int blkszbits;
> > + int ret;
> > +
> > + bh = sb_bread(sb, 0);
>
> Is there any good reasons to use buffer heads like this in new code
> vs directly using bios?
This page can save in bdev page cache, it contains not only the erofs
superblock so it can be fetched in page cache later.
>
> > +
> > + sbi->blocks = le32_to_cpu(layout->blocks);
> > + sbi->meta_blkaddr = le32_to_cpu(layout->meta_blkaddr);
> > + sbi->islotbits = ffs(sizeof(struct erofs_inode_v1)) - 1;
> > + sbi->root_nid = le16_to_cpu(layout->root_nid);
> > + sbi->inos = le64_to_cpu(layout->inos);
> > +
> > + sbi->build_time = le64_to_cpu(layout->build_time);
> > + sbi->build_time_nsec = le32_to_cpu(layout->build_time_nsec);
> > +
> > + memcpy(>s_uuid, layout->uuid, sizeof(layout->uuid));
> > + memcpy(sbi->volume_name, layout->volume_name,
> > + sizeof(layout->volume_name));
>
> s_uuid should preferably be a uuid_t (assuming it is a real BE uuid,
> if it is le it should be a guid_t).
I just copied it from f2fs, I have no idea which one is best and
which fs I could refer to.
>
> > +/* set up default EROFS parameters */
> > +static void default_options(struct erofs_sb_info *sbi)
> > +{
> > +}
>
> No need to add an empty function.
Later patch will fill this function.
>
> > +static int erofs_fill_super(struct super_block *sb, void *data, int silent)
> > +{
> > + struct inode *inode;
> > + struct erofs_sb_info *sbi;
> > + int err;
> > +
> > + infoln("fill_super, device -> %s", sb->s_id);
> > + infoln("options -> %s", (char *)data);
>
> That is some very verbose debug info. We usually don't add that and
> let people trace the function instead. Also you should probably
> implement the new mount API.
> new mount API.
Al think it is not urgent as well,
https://lore.kernel.org/driverdev-devel/20190721040547.gf17...@zeniv.linux.org.uk/
Al said,
>> I agree with you, it seems better to just use s_id in community and
>> delete erofs_mount_private stuffs...
>> Yet I don't look into how to use new fs_context, could I keep using
>> legacy mount
Re: [PATCH v6 01/24] erofs: add on-disk layout
On Thu, Aug 29, 2019 at 06:32:53PM +0800, Gao Xiang wrote: > I can fix it up as you like but I still cannot get > what is critical issues here. The problem is that the whole codebase is way substandard quality, looking a lot like Linux code from 20 years ago. Yes, we already have plenty of code of that standard in the tree, but we should not add more.
Re: [PATCH v6 01/24] erofs: add on-disk layout
Hi Christoph,
On Thu, Aug 29, 2019 at 02:59:54AM -0700, Christoph Hellwig wrote:
> > --- /dev/null
> > +++ b/fs/erofs/erofs_fs.h
> > @@ -0,0 +1,316 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only OR Apache-2.0 */
> > +/*
> > + * linux/fs/erofs/erofs_fs.h
>
> Please remove the pointless file names in the comment headers.
Already removed in the latest version.
>
> > +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 */
>
> Please remove all the byte offset comments. That is something that can
> easily be checked with gdb or pahole.
I have no idea the actual issue here.
It will help all developpers better add fields or calculate
these offsets in their mind, and with care.
Rather than they didn't run "gdb" or "pahole" and change it by mistake.
>
> > +/* 64 */__u8 volume_name[16]; /* volume name */
> > +/* 80 */__le32 requirements;/* (aka. feature_incompat) */
> > +
> > +/* 84 */__u8 reserved2[44];
> > +} __packed; /* 128 bytes */
>
> Please don't add __packed. In this case I think you don't need it
> (but double check with pahole), but even if you would need it using
> proper padding fields and making sure all fields are naturally aligned
> will give you much better code generation on architectures that don't
> support native unaligned access.
If you can see more, all on-disk fields in EROFS are naturally aligned,
I can remove all of these as you like, but I think that is not very urgent.
>
> > +/*
> > + * erofs inode data mapping:
> > + * 0 - inode plain without inline data A:
> > + * inode, [xattrs], ... | ... | no-holed data
> > + * 1 - inode VLE compression B (legacy):
> > + * inode, [xattrs], extents ... | ...
> > + * 2 - inode plain with inline data C:
> > + * inode, [xattrs], last_inline_data, ... | ... | no-holed data
> > + * 3 - inode compression D:
> > + * inode, [xattrs], map_header, extents ... | ...
> > + * 4~7 - reserved
> > + */
> > +enum {
> > + EROFS_INODE_FLAT_PLAIN,
>
> This one doesn't actually seem to be used.
It could be better has a name though, because erofs.mkfs uses it,
and we keep this on-disk file up with erofs-utils.
>
> > + EROFS_INODE_FLAT_COMPRESSION_LEGACY,
>
> why are we adding a legacy field to a brand new file system?
the difference is just EROFS_INODE_FLAT_COMPRESSION_LEGACY doesn't have
z_erofs_map_header, nothing special at all.
>
> > + EROFS_INODE_FLAT_INLINE,
> > + EROFS_INODE_FLAT_COMPRESSION,
> > + EROFS_INODE_LAYOUT_MAX
>
> It seems like these come from the on-disk format, in which case they
> should have explicit values assigned to them.
>
> Btw, I think it generally helps file system implementation quality
> if you use a separate header for the on-disk structures vs in-memory
> structures, as that keeps it clear in everyones mind what needs to
> stay persistent and what can be chenged easily.
All fields in this file are on-disk representation.
>
> > +static bool erofs_inode_is_data_compressed(unsigned int datamode)
> > +{
> > + if (datamode == EROFS_INODE_FLAT_COMPRESSION)
> > + return true;
> > + return datamode == EROFS_INODE_FLAT_COMPRESSION_LEGACY;
> > +}
>
> This looks like a really obsfucated way to write:
>
> return datamode == EROFS_INODE_FLAT_COMPRESSION ||
> datamode == EROFS_INODE_FLAT_COMPRESSION_LEGACY;
It depends on the personal choise, if you like, I will change into your form.
>
> > +/* 28 */__le32 i_reserved2;
> > +} __packed;
>
> Sane comment as above.
>
> > +
> > +/* 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;
>
> Why do we have two inode version in a newly added file system?
v2 is an exhanced on-disk inode form, it has 64 bytes,
v1 is more compacted one, which is already suitable
for Android use case of course.
There is no new and old, both are used for the current EROFS.
>
> > +#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); })
>
> This would be much more readable as a function.
>
> > +#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))
>
> Same here.
Personal tendency, because we are working in a dedicated team rather than
an individual person.
But I can fix as you like.
>
> > +/* available compression algorithm types */
> > +enum {
> > + Z_EROFS_COMPRESSION_LZ4,
> > + Z_EROFS_COMPRESSION_MAX
> > +};
>
> Seems like an on-disk value
Re: [PATCH v6 08/24] erofs: add namei functions
On Fri, Aug 02, 2019 at 08:53:31PM +0800, Gao Xiang wrote:
> +struct erofs_qstr {
> + const unsigned char *name;
> + const unsigned char *end;
> +};
Maybe erofs_name? The q in qstr stands for quick, because of the
existing hash and len, which this doesn't really provide.
Also I don't really see why you don't just pass the actual qstr and
just document that dirnamecmp does not look at the hash and thus
doesn't require it to be filled out.
Re: [PATCH v6 06/24] erofs: support special inode
On Fri, Aug 02, 2019 at 08:53:29PM +0800, Gao Xiang wrote: > This patch adds to support special inode, such as > block dev, char, socket, pipe inode. > > Signed-off-by: Gao Xiang > --- > fs/erofs/inode.c | 27 +-- > 1 file changed, 25 insertions(+), 2 deletions(-) > > diff --git a/fs/erofs/inode.c b/fs/erofs/inode.c > index b6ea997bc4ae..637bf6e4de44 100644 > --- a/fs/erofs/inode.c > +++ b/fs/erofs/inode.c > @@ -34,7 +34,16 @@ static int read_inode(struct inode *inode, void *data) > vi->xattr_isize = ondisk_xattr_ibody_size(v2->i_xattr_icount); > > inode->i_mode = le16_to_cpu(v2->i_mode); > - vi->raw_blkaddr = le32_to_cpu(v2->i_u.raw_blkaddr); > + if (S_ISREG(inode->i_mode) || S_ISDIR(inode->i_mode) || > + S_ISLNK(inode->i_mode)) > + vi->raw_blkaddr = le32_to_cpu(v2->i_u.raw_blkaddr); > + else if (S_ISCHR(inode->i_mode) || S_ISBLK(inode->i_mode)) > + inode->i_rdev = > + new_decode_dev(le32_to_cpu(v2->i_u.rdev)); > + else if (S_ISFIFO(inode->i_mode) || S_ISSOCK(inode->i_mode)) > + inode->i_rdev = 0; > + else > + return -EIO; Please use a switch statement when dealing with the file modes to make everything easier to read.
Re: [PATCH v6 05/24] erofs: add inode operations
On Fri, Aug 02, 2019 at 08:53:28PM +0800, Gao Xiang wrote:
> This adds core functions to get, read an inode.
> It adds statx support as well.
>
> Signed-off-by: Gao Xiang
> ---
> fs/erofs/inode.c | 291 +++
> 1 file changed, 291 insertions(+)
> create mode 100644 fs/erofs/inode.c
>
> diff --git a/fs/erofs/inode.c b/fs/erofs/inode.c
> new file mode 100644
> index ..b6ea997bc4ae
> --- /dev/null
> +++ b/fs/erofs/inode.c
> @@ -0,0 +1,291 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * linux/fs/erofs/inode.c
> + *
> + * Copyright (C) 2017-2018 HUAWEI, Inc.
> + * http://www.huawei.com/
> + * Created by Gao Xiang
> + */
> +#include "internal.h"
> +
> +#include
> +
> +/* no locking */
> +static int read_inode(struct inode *inode, void *data)
> +{
> + struct erofs_vnode *vi = EROFS_V(inode);
> + struct erofs_inode_v1 *v1 = data;
> + const unsigned int advise = le16_to_cpu(v1->i_advise);
> + erofs_blk_t nblks = 0;
> +
> + vi->datamode = __inode_data_mapping(advise);
What is the deal with these magic underscores here and various
other similar helpers?
> + /* fast symlink (following ext4) */
This actually originates in FFS. But it is so common that the comment
seems a little pointless.
> + if (S_ISLNK(inode->i_mode) && inode->i_size < PAGE_SIZE) {
> + char *lnk = erofs_kmalloc(sbi, inode->i_size + 1, GFP_KERNEL);
Please just use plain kmalloc everywhere and let the normal kernel
error injection code take care of injeting any errors.
> + /* inline symlink data shouldn't across page boundary as well */
... should not cross ..
> + if (unlikely(m_pofs + inode->i_size > PAGE_SIZE)) {
> + DBG_BUGON(1);
> + kfree(lnk);
> + return -EIO;
> + }
> +
> + /* get in-page inline data */
s/get/copy/, but the comment seems rather pointless.
> + memcpy(lnk, data + m_pofs, inode->i_size);
> + lnk[inode->i_size] = '\0';
> +
> + inode->i_link = lnk;
> + set_inode_fast_symlink(inode);
Please just set the ops directly instead of obsfucating that in a single
caller, single line inline function. And please set it instead of the
normal symlink iops in the same place where you also set those.:w
> + err = read_inode(inode, data + ofs);
> + if (!err) {
if (err)
goto out_unlock;
.. and save one level of indentation.
> + if (is_inode_layout_compression(inode)) {
The name of this helper is a little odd. But I think just
opencoding it seems generally cleaner anyway.
> + err = -ENOTSUPP;
> + goto out_unlock;
> + }
> +
> + inode->i_mapping->a_ops = _raw_access_aops;
> +
> + /* fill last page if inline data is available */
> + err = fill_inline_data(inode, data, ofs);
Well, I think you should move the is_inode_flat_inline and
(S_ISLNK(inode->i_mode) && inode->i_size < PAGE_SIZE) checks from that
helper here, as otherwise you make everyone wonder why you'd always
fill out the inline data.
> +static inline struct inode *erofs_iget_locked(struct super_block *sb,
> + erofs_nid_t nid)
> +{
> + const unsigned long hashval = erofs_inode_hash(nid);
> +
> +#if BITS_PER_LONG >= 64
> + /* it is safe to use iget_locked for >= 64-bit platform */
> + return iget_locked(sb, hashval);
> +#else
> + return iget5_locked(sb, hashval, erofs_ilookup_test_actor,
> + erofs_iget_set_actor, );
> +#endif
Just use the slightly more complicated 32-bit version everywhere so that
you have a single actually tested code path. And then remove this
helper.
Re: [PATCH v6 04/24] erofs: add raw address_space operations
The actual address_space operations seem to largely duplicate the iomap versions. Please use those instead. Also I don't think any new file system should write up ->bmap these days.
Re: [PATCH v6 03/24] erofs: add super block operations
On Fri, Aug 02, 2019 at 08:53:26PM +0800, Gao Xiang wrote:
> +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;
Please just use normal if/else. Also having this function seems
entirely pointless.
> +static void erofs_exit_inode_cache(void)
> +{
> + kmem_cache_destroy(erofs_inode_cachep);
> +}
Same for this one.
> +static void free_inode(struct inode *inode)
Please use an erofs_ prefix for all your functions.
> +{
> + struct erofs_vnode *vi = EROFS_V(inode);
Why is this called vnode instead of inode? That seems like a rather
odd naming for a Linux file system.
> +
> + /* be careful RCU symlink path (see ext4_inode_info->i_data)! */
> + if (is_inode_fast_symlink(inode))
> + kfree(inode->i_link);
is_inode_fast_symlink only shows up in a later patch. And really
obsfucates the check here in the only caller as you can just do an
unconditional kfree here - i_link will be NULL except for the case
where you explicitly set it.
Also this code is nothing like ext4, so the code seems a little confusing.
> +static bool check_layout_compatibility(struct super_block *sb,
> +struct erofs_super_block *layout)
> +{
> + const unsigned int requirements = le32_to_cpu(layout->requirements);
Why is the variable name for the on-disk subperblock layout? We usually
still calls this something with sb in the name, e.g. dsb. for disk
super block.
> + EROFS_SB(sb)->requirements = requirements;
> +
> + /* check if current kernel meets all mandatory requirements */
> + if (requirements & (~EROFS_ALL_REQUIREMENTS)) {
> + errln("unidentified requirements %x, please upgrade kernel
> version",
> + requirements & ~EROFS_ALL_REQUIREMENTS);
> + return false;
> + }
> + return true;
Note that normally we call this features, but that doesn't really
matter too much.
> +static int superblock_read(struct super_block *sb)
> +{
> + struct erofs_sb_info *sbi;
> + struct buffer_head *bh;
> + struct erofs_super_block *layout;
> + unsigned int blkszbits;
> + int ret;
> +
> + bh = sb_bread(sb, 0);
Is there any good reasons to use buffer heads like this in new code
vs directly using bios?
> +
> + sbi->blocks = le32_to_cpu(layout->blocks);
> + sbi->meta_blkaddr = le32_to_cpu(layout->meta_blkaddr);
> + sbi->islotbits = ffs(sizeof(struct erofs_inode_v1)) - 1;
> + sbi->root_nid = le16_to_cpu(layout->root_nid);
> + sbi->inos = le64_to_cpu(layout->inos);
> +
> + sbi->build_time = le64_to_cpu(layout->build_time);
> + sbi->build_time_nsec = le32_to_cpu(layout->build_time_nsec);
> +
> + memcpy(>s_uuid, layout->uuid, sizeof(layout->uuid));
> + memcpy(sbi->volume_name, layout->volume_name,
> +sizeof(layout->volume_name));
s_uuid should preferably be a uuid_t (assuming it is a real BE uuid,
if it is le it should be a guid_t).
> +/* set up default EROFS parameters */
> +static void default_options(struct erofs_sb_info *sbi)
> +{
> +}
No need to add an empty function.
> +static int erofs_fill_super(struct super_block *sb, void *data, int silent)
> +{
> + struct inode *inode;
> + struct erofs_sb_info *sbi;
> + int err;
> +
> + infoln("fill_super, device -> %s", sb->s_id);
> + infoln("options -> %s", (char *)data);
That is some very verbose debug info. We usually don't add that and
let people trace the function instead. Also you should probably
implement the new mount API.
new mount API.
> +static void erofs_kill_sb(struct super_block *sb)
> +{
> + struct erofs_sb_info *sbi;
> +
> + WARN_ON(sb->s_magic != EROFS_SUPER_MAGIC);
> + infoln("unmounting for %s", sb->s_id);
> +
> + kill_block_super(sb);
> +
> + sbi = EROFS_SB(sb);
> + if (!sbi)
> + return;
> + kfree(sbi);
> + sb->s_fs_info = NULL;
> +}
Why is this needed? You can just free your sb privatte information in
->put_super and wire up kill_block_super as the ->kill_sb method
directly.
Re: [PATCH v6 01/24] erofs: add on-disk layout
> --- /dev/null
> +++ b/fs/erofs/erofs_fs.h
> @@ -0,0 +1,316 @@
> +/* SPDX-License-Identifier: GPL-2.0-only OR Apache-2.0 */
> +/*
> + * linux/fs/erofs/erofs_fs.h
Please remove the pointless file names in the comment headers.
> +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 */
Please remove all the byte offset comments. That is something that can
easily be checked with gdb or pahole.
> +/* 64 */__u8 volume_name[16]; /* volume name */
> +/* 80 */__le32 requirements;/* (aka. feature_incompat) */
> +
> +/* 84 */__u8 reserved2[44];
> +} __packed; /* 128 bytes */
Please don't add __packed. In this case I think you don't need it
(but double check with pahole), but even if you would need it using
proper padding fields and making sure all fields are naturally aligned
will give you much better code generation on architectures that don't
support native unaligned access.
> +/*
> + * erofs inode data mapping:
> + * 0 - inode plain without inline data A:
> + * inode, [xattrs], ... | ... | no-holed data
> + * 1 - inode VLE compression B (legacy):
> + * inode, [xattrs], extents ... | ...
> + * 2 - inode plain with inline data C:
> + * inode, [xattrs], last_inline_data, ... | ... | no-holed data
> + * 3 - inode compression D:
> + * inode, [xattrs], map_header, extents ... | ...
> + * 4~7 - reserved
> + */
> +enum {
> + EROFS_INODE_FLAT_PLAIN,
This one doesn't actually seem to be used.
> + EROFS_INODE_FLAT_COMPRESSION_LEGACY,
why are we adding a legacy field to a brand new file system?
> + EROFS_INODE_FLAT_INLINE,
> + EROFS_INODE_FLAT_COMPRESSION,
> + EROFS_INODE_LAYOUT_MAX
It seems like these come from the on-disk format, in which case they
should have explicit values assigned to them.
Btw, I think it generally helps file system implementation quality
if you use a separate header for the on-disk structures vs in-memory
structures, as that keeps it clear in everyones mind what needs to
stay persistent and what can be chenged easily.
> +static bool erofs_inode_is_data_compressed(unsigned int datamode)
> +{
> + if (datamode == EROFS_INODE_FLAT_COMPRESSION)
> + return true;
> + return datamode == EROFS_INODE_FLAT_COMPRESSION_LEGACY;
> +}
This looks like a really obsfucated way to write:
return datamode == EROFS_INODE_FLAT_COMPRESSION ||
datamode == EROFS_INODE_FLAT_COMPRESSION_LEGACY;
> +/* 28 */__le32 i_reserved2;
> +} __packed;
Sane comment as above.
> +
> +/* 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;
Why do we have two inode version in a newly added file system?
> +#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); })
This would be much more readable as a function.
> +#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))
Same here.
> +/* available compression algorithm types */
> +enum {
> + Z_EROFS_COMPRESSION_LZ4,
> + Z_EROFS_COMPRESSION_MAX
> +};
Seems like an on-disk value again that should use explicitly assigned
numbers.
