2023-02-03 03:53:12

by Jingbo Xu

[permalink] [raw]
Subject: [PATCH 0/3] erofs: cleanup for erofs_map_blocks()

Jingbo Xu (3):
erofs: add print symbols for various flags in trace
erofs: remove unused flags parameter of erofs_map_blocks()
erofs: call erofs_map_dev() inside erofs_map_blocks()

fs/erofs/data.c | 31 ++++++++++++++-----------------
fs/erofs/fscache.c | 20 ++------------------
fs/erofs/internal.h | 6 +++---
include/trace/events/erofs.h | 14 ++++++++++----
4 files changed, 29 insertions(+), 42 deletions(-)

--
2.19.1.6.gb485710b



2023-02-03 03:53:14

by Jingbo Xu

[permalink] [raw]
Subject: [PATCH 1/3] erofs: add print symbols for various flags in trace

As new flags introduced, the corresponding print symbols for trace are
not added accordingly. Add these missing print symbols for these flags.

Signed-off-by: Jingbo Xu <[email protected]>
---
include/trace/events/erofs.h | 14 ++++++++++----
1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/include/trace/events/erofs.h b/include/trace/events/erofs.h
index 4f4c44ea3a65..c0dbe9c4f656 100644
--- a/include/trace/events/erofs.h
+++ b/include/trace/events/erofs.h
@@ -19,12 +19,18 @@ struct erofs_map_blocks;
{ 1, "DIR" })

#define show_map_flags(flags) __print_flags(flags, "|", \
- { EROFS_GET_BLOCKS_RAW, "RAW" })
+ { EROFS_GET_BLOCKS_RAW, "RAW" }, \
+ { EROFS_GET_BLOCKS_FIEMAP, "FIEMAP" }, \
+ { EROFS_GET_BLOCKS_READMORE, "READMORE" }, \
+ { EROFS_GET_BLOCKS_FINDTAIL, "FINDTAIL" })

#define show_mflags(flags) __print_flags(flags, "", \
- { EROFS_MAP_MAPPED, "M" }, \
- { EROFS_MAP_META, "I" }, \
- { EROFS_MAP_ENCODED, "E" })
+ { EROFS_MAP_MAPPED, "M" }, \
+ { EROFS_MAP_META, "I" }, \
+ { EROFS_MAP_ENCODED, "E" }, \
+ { EROFS_MAP_FULL_MAPPED, "F" }, \
+ { EROFS_MAP_FRAGMENT, "R" }, \
+ { EROFS_MAP_PARTIAL_REF, "P" })

TRACE_EVENT(erofs_lookup,

--
2.19.1.6.gb485710b


2023-02-03 03:53:18

by Jingbo Xu

[permalink] [raw]
Subject: [PATCH 2/3] erofs: remove unused flags parameter of erofs_map_blocks()

For erofs_map_blocks() and erofs_map_blocks_flatmode(), the flags
argument is always EROFS_GET_BLOCKS_RAW, as the compression routine
implements its own map_blocks() variant.

Signed-off-by: Jingbo Xu <[email protected]>
---
fs/erofs/data.c | 14 ++++++--------
fs/erofs/fscache.c | 4 ++--
fs/erofs/internal.h | 5 ++---
3 files changed, 10 insertions(+), 13 deletions(-)

diff --git a/fs/erofs/data.c b/fs/erofs/data.c
index f57f921683d7..32e66d29968f 100644
--- a/fs/erofs/data.c
+++ b/fs/erofs/data.c
@@ -74,8 +74,7 @@ void *erofs_read_metabuf(struct erofs_buf *buf, struct super_block *sb,
}

static int erofs_map_blocks_flatmode(struct inode *inode,
- struct erofs_map_blocks *map,
- int flags)
+ struct erofs_map_blocks *map)
{
erofs_blk_t nblocks, lastblk;
u64 offset = map->m_la;
@@ -117,8 +116,7 @@ static int erofs_map_blocks_flatmode(struct inode *inode,
return 0;
}

-int erofs_map_blocks(struct inode *inode,
- struct erofs_map_blocks *map, int flags)
+int erofs_map_blocks(struct inode *inode, struct erofs_map_blocks *map)
{
struct super_block *sb = inode->i_sb;
struct erofs_inode *vi = EROFS_I(inode);
@@ -130,7 +128,7 @@ int erofs_map_blocks(struct inode *inode,
void *kaddr;
int err = 0;

- trace_erofs_map_blocks_enter(inode, map, flags);
+ trace_erofs_map_blocks_enter(inode, map, EROFS_GET_BLOCKS_RAW);
map->m_deviceid = 0;
if (map->m_la >= inode->i_size) {
/* leave out-of-bound access unmapped */
@@ -140,7 +138,7 @@ int erofs_map_blocks(struct inode *inode,
}

if (vi->datalayout != EROFS_INODE_CHUNK_BASED) {
- err = erofs_map_blocks_flatmode(inode, map, flags);
+ err = erofs_map_blocks_flatmode(inode, map);
goto out;
}

@@ -192,7 +190,7 @@ int erofs_map_blocks(struct inode *inode,
out:
if (!err)
map->m_llen = map->m_plen;
- trace_erofs_map_blocks_exit(inode, map, flags, 0);
+ trace_erofs_map_blocks_exit(inode, map, EROFS_GET_BLOCKS_RAW, err);
return err;
}

@@ -255,7 +253,7 @@ static int erofs_iomap_begin(struct inode *inode, loff_t offset, loff_t length,
map.m_la = offset;
map.m_llen = length;

- ret = erofs_map_blocks(inode, &map, EROFS_GET_BLOCKS_RAW);
+ ret = erofs_map_blocks(inode, &map);
if (ret < 0)
return ret;

diff --git a/fs/erofs/fscache.c b/fs/erofs/fscache.c
index d47b04dfdc48..7f1ef2ffc4db 100644
--- a/fs/erofs/fscache.c
+++ b/fs/erofs/fscache.c
@@ -229,7 +229,7 @@ static int erofs_fscache_data_read_slice(struct erofs_fscache_request *primary)
int ret;

map.m_la = pos;
- ret = erofs_map_blocks(inode, &map, EROFS_GET_BLOCKS_RAW);
+ ret = erofs_map_blocks(inode, &map);
if (ret)
return ret;

@@ -377,7 +377,7 @@ static int erofs_fscache_share_file_open(struct inode *inode, struct file *filp)
struct file *realfile;
int ret;

- ret = erofs_map_blocks(inode, &map, EROFS_GET_BLOCKS_RAW);
+ ret = erofs_map_blocks(inode, &map);
if (ret)
return ret;

diff --git a/fs/erofs/internal.h b/fs/erofs/internal.h
index a63a9e951fe0..323c2c775023 100644
--- a/fs/erofs/internal.h
+++ b/fs/erofs/internal.h
@@ -445,7 +445,7 @@ struct erofs_map_blocks {
unsigned int m_flags;
};

-/* Flags used by erofs_map_blocks_flatmode() */
+/* Used to map raw data */
#define EROFS_GET_BLOCKS_RAW 0x0001
/*
* Used to get the exact decompressed length, e.g. fiemap (consider lookback
@@ -502,8 +502,7 @@ void *erofs_read_metabuf(struct erofs_buf *buf, struct super_block *sb,
int erofs_map_dev(struct super_block *sb, struct erofs_map_dev *dev);
int erofs_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
u64 start, u64 len);
-int erofs_map_blocks(struct inode *inode,
- struct erofs_map_blocks *map, int flags);
+int erofs_map_blocks(struct inode *inode, struct erofs_map_blocks *map);

/* inode.c */
static inline unsigned long erofs_inode_hash(erofs_nid_t nid)
--
2.19.1.6.gb485710b


2023-02-03 03:53:24

by Jingbo Xu

[permalink] [raw]
Subject: [PATCH 3/3] erofs: call erofs_map_dev() inside erofs_map_blocks()

For now erofs_map_blocks() is always followed by erofs_map_dev().
Make erofs_map_dev() called inside erofs_map_blocks() to reduce code
duplication.

Signed-off-by: Jingbo Xu <[email protected]>
---
fs/erofs/data.c | 21 ++++++++++-----------
fs/erofs/fscache.c | 20 ++------------------
fs/erofs/internal.h | 3 ++-
3 files changed, 14 insertions(+), 30 deletions(-)

diff --git a/fs/erofs/data.c b/fs/erofs/data.c
index 32e66d29968f..cbe7a6d6846d 100644
--- a/fs/erofs/data.c
+++ b/fs/erofs/data.c
@@ -116,7 +116,8 @@ static int erofs_map_blocks_flatmode(struct inode *inode,
return 0;
}

-int erofs_map_blocks(struct inode *inode, struct erofs_map_blocks *map)
+int erofs_map_blocks(struct inode *inode, struct erofs_map_blocks *map,
+ struct erofs_map_dev *mdev)
{
struct super_block *sb = inode->i_sb;
struct erofs_inode *vi = EROFS_I(inode);
@@ -188,8 +189,14 @@ int erofs_map_blocks(struct inode *inode, struct erofs_map_blocks *map)
out_unlock:
erofs_put_metabuf(&buf);
out:
- if (!err)
+ if (!err) {
map->m_llen = map->m_plen;
+ *mdev = (struct erofs_map_dev) {
+ .m_deviceid = map->m_deviceid,
+ .m_pa = map->m_pa,
+ };
+ err = erofs_map_dev(sb, mdev);
+ }
trace_erofs_map_blocks_exit(inode, map, EROFS_GET_BLOCKS_RAW, err);
return err;
}
@@ -253,15 +260,7 @@ static int erofs_iomap_begin(struct inode *inode, loff_t offset, loff_t length,
map.m_la = offset;
map.m_llen = length;

- ret = erofs_map_blocks(inode, &map);
- if (ret < 0)
- return ret;
-
- mdev = (struct erofs_map_dev) {
- .m_deviceid = map.m_deviceid,
- .m_pa = map.m_pa,
- };
- ret = erofs_map_dev(inode->i_sb, &mdev);
+ ret = erofs_map_blocks(inode, &map, &mdev);
if (ret)
return ret;

diff --git a/fs/erofs/fscache.c b/fs/erofs/fscache.c
index 7f1ef2ffc4db..140ccacc1043 100644
--- a/fs/erofs/fscache.c
+++ b/fs/erofs/fscache.c
@@ -229,7 +229,7 @@ static int erofs_fscache_data_read_slice(struct erofs_fscache_request *primary)
int ret;

map.m_la = pos;
- ret = erofs_map_blocks(inode, &map);
+ ret = erofs_map_blocks(inode, &map, &mdev);
if (ret)
return ret;

@@ -270,14 +270,6 @@ static int erofs_fscache_data_read_slice(struct erofs_fscache_request *primary)
count = min_t(size_t, map.m_llen - (pos - map.m_la), count);
DBG_BUGON(!count || count % PAGE_SIZE);

- mdev = (struct erofs_map_dev) {
- .m_deviceid = map.m_deviceid,
- .m_pa = map.m_pa,
- };
- ret = erofs_map_dev(sb, &mdev);
- if (ret)
- return ret;
-
req = erofs_fscache_req_chain(primary, count);
if (IS_ERR(req))
return PTR_ERR(req);
@@ -377,15 +369,7 @@ static int erofs_fscache_share_file_open(struct inode *inode, struct file *filp)
struct file *realfile;
int ret;

- ret = erofs_map_blocks(inode, &map);
- if (ret)
- return ret;
-
- mdev = (struct erofs_map_dev) {
- .m_deviceid = map.m_deviceid,
- .m_pa = map.m_pa,
- };
- ret = erofs_map_dev(inode->i_sb, &mdev);
+ ret = erofs_map_blocks(inode, &map, &mdev);
if (ret)
return ret;

diff --git a/fs/erofs/internal.h b/fs/erofs/internal.h
index 323c2c775023..c54dec32a868 100644
--- a/fs/erofs/internal.h
+++ b/fs/erofs/internal.h
@@ -502,7 +502,8 @@ void *erofs_read_metabuf(struct erofs_buf *buf, struct super_block *sb,
int erofs_map_dev(struct super_block *sb, struct erofs_map_dev *dev);
int erofs_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
u64 start, u64 len);
-int erofs_map_blocks(struct inode *inode, struct erofs_map_blocks *map);
+int erofs_map_blocks(struct inode *inode, struct erofs_map_blocks *map,
+ struct erofs_map_dev *mdev);

/* inode.c */
static inline unsigned long erofs_inode_hash(erofs_nid_t nid)
--
2.19.1.6.gb485710b


2023-02-03 05:48:25

by Gao Xiang

[permalink] [raw]
Subject: Re: [PATCH 1/3] erofs: add print symbols for various flags in trace

Hi Jingbo,

On 2023/2/3 11:53, Jingbo Xu wrote:
> As new flags introduced, the corresponding print symbols for trace are
> not added accordingly. Add these missing print symbols for these flags.
>
> Signed-off-by: Jingbo Xu <[email protected]>
> ---
> include/trace/events/erofs.h | 14 ++++++++++----
> 1 file changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/include/trace/events/erofs.h b/include/trace/events/erofs.h
> index 4f4c44ea3a65..c0dbe9c4f656 100644
> --- a/include/trace/events/erofs.h
> +++ b/include/trace/events/erofs.h
> @@ -19,12 +19,18 @@ struct erofs_map_blocks;
> { 1, "DIR" })
>
> #define show_map_flags(flags) __print_flags(flags, "|", \
> - { EROFS_GET_BLOCKS_RAW, "RAW" })
> + { EROFS_GET_BLOCKS_RAW, "RAW" }, \

Initially, it was just used for reading compressed (raw) data
for compressed files. However, it's never used actually.

Could we just gid of this flag (EROFS_GET_BLOCKS_RAW)?

Thanks,
Gao Xiang

> + { EROFS_GET_BLOCKS_FIEMAP, "FIEMAP" }, \
> + { EROFS_GET_BLOCKS_READMORE, "READMORE" }, \
> + { EROFS_GET_BLOCKS_FINDTAIL, "FINDTAIL" })
>
> #define show_mflags(flags) __print_flags(flags, "", \
> - { EROFS_MAP_MAPPED, "M" }, \
> - { EROFS_MAP_META, "I" }, \
> - { EROFS_MAP_ENCODED, "E" })
> + { EROFS_MAP_MAPPED, "M" }, \
> + { EROFS_MAP_META, "I" }, \
> + { EROFS_MAP_ENCODED, "E" }, \
> + { EROFS_MAP_FULL_MAPPED, "F" }, \
> + { EROFS_MAP_FRAGMENT, "R" }, \
> + { EROFS_MAP_PARTIAL_REF, "P" })
>
> TRACE_EVENT(erofs_lookup,
>

2023-02-03 05:52:38

by Gao Xiang

[permalink] [raw]
Subject: Re: [PATCH 3/3] erofs: call erofs_map_dev() inside erofs_map_blocks()



On 2023/2/3 11:53, Jingbo Xu wrote:
> For now erofs_map_blocks() is always followed by erofs_map_dev().
> Make erofs_map_dev() called inside erofs_map_blocks() to reduce code
> duplication.

Could we just integrate mdev into struct erofs_map_blocks?

BTW, I still think erofs_map_dev() is useful since it can handle
non-inode IO requests, so let's keep this rather than mergeing all
code into erofs_map_blocks()

Thanks,
Gao Xiang

>
> Signed-off-by: Jingbo Xu <[email protected]>
> ---
> fs/erofs/data.c | 21 ++++++++++-----------
> fs/erofs/fscache.c | 20 ++------------------
> fs/erofs/internal.h | 3 ++-
> 3 files changed, 14 insertions(+), 30 deletions(-)
>
> diff --git a/fs/erofs/data.c b/fs/erofs/data.c
> index 32e66d29968f..cbe7a6d6846d 100644
> --- a/fs/erofs/data.c
> +++ b/fs/erofs/data.c
> @@ -116,7 +116,8 @@ static int erofs_map_blocks_flatmode(struct inode *inode,
> return 0;
> }
>
> -int erofs_map_blocks(struct inode *inode, struct erofs_map_blocks *map)
> +int erofs_map_blocks(struct inode *inode, struct erofs_map_blocks *map,
> + struct erofs_map_dev *mdev)
> {
> struct super_block *sb = inode->i_sb;
> struct erofs_inode *vi = EROFS_I(inode);
> @@ -188,8 +189,14 @@ int erofs_map_blocks(struct inode *inode, struct erofs_map_blocks *map)
> out_unlock:
> erofs_put_metabuf(&buf);
> out:
> - if (!err)
> + if (!err) {
> map->m_llen = map->m_plen;
> + *mdev = (struct erofs_map_dev) {
> + .m_deviceid = map->m_deviceid,
> + .m_pa = map->m_pa,
> + };
> + err = erofs_map_dev(sb, mdev);
> + }
> trace_erofs_map_blocks_exit(inode, map, EROFS_GET_BLOCKS_RAW, err);
> return err;
> }
> @@ -253,15 +260,7 @@ static int erofs_iomap_begin(struct inode *inode, loff_t offset, loff_t length,
> map.m_la = offset;
> map.m_llen = length;
>
> - ret = erofs_map_blocks(inode, &map);
> - if (ret < 0)
> - return ret;
> -
> - mdev = (struct erofs_map_dev) {
> - .m_deviceid = map.m_deviceid,
> - .m_pa = map.m_pa,
> - };
> - ret = erofs_map_dev(inode->i_sb, &mdev);
> + ret = erofs_map_blocks(inode, &map, &mdev);
> if (ret)
> return ret;
>
> diff --git a/fs/erofs/fscache.c b/fs/erofs/fscache.c
> index 7f1ef2ffc4db..140ccacc1043 100644
> --- a/fs/erofs/fscache.c
> +++ b/fs/erofs/fscache.c
> @@ -229,7 +229,7 @@ static int erofs_fscache_data_read_slice(struct erofs_fscache_request *primary)
> int ret;
>
> map.m_la = pos;
> - ret = erofs_map_blocks(inode, &map);
> + ret = erofs_map_blocks(inode, &map, &mdev);
> if (ret)
> return ret;
>
> @@ -270,14 +270,6 @@ static int erofs_fscache_data_read_slice(struct erofs_fscache_request *primary)
> count = min_t(size_t, map.m_llen - (pos - map.m_la), count);
> DBG_BUGON(!count || count % PAGE_SIZE);
>
> - mdev = (struct erofs_map_dev) {
> - .m_deviceid = map.m_deviceid,
> - .m_pa = map.m_pa,
> - };
> - ret = erofs_map_dev(sb, &mdev);
> - if (ret)
> - return ret;
> -
> req = erofs_fscache_req_chain(primary, count);
> if (IS_ERR(req))
> return PTR_ERR(req);
> @@ -377,15 +369,7 @@ static int erofs_fscache_share_file_open(struct inode *inode, struct file *filp)
> struct file *realfile;
> int ret;
>
> - ret = erofs_map_blocks(inode, &map);
> - if (ret)
> - return ret;
> -
> - mdev = (struct erofs_map_dev) {
> - .m_deviceid = map.m_deviceid,
> - .m_pa = map.m_pa,
> - };
> - ret = erofs_map_dev(inode->i_sb, &mdev);
> + ret = erofs_map_blocks(inode, &map, &mdev);
> if (ret)
> return ret;
>
> diff --git a/fs/erofs/internal.h b/fs/erofs/internal.h
> index 323c2c775023..c54dec32a868 100644
> --- a/fs/erofs/internal.h
> +++ b/fs/erofs/internal.h
> @@ -502,7 +502,8 @@ void *erofs_read_metabuf(struct erofs_buf *buf, struct super_block *sb,
> int erofs_map_dev(struct super_block *sb, struct erofs_map_dev *dev);
> int erofs_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
> u64 start, u64 len);
> -int erofs_map_blocks(struct inode *inode, struct erofs_map_blocks *map);
> +int erofs_map_blocks(struct inode *inode, struct erofs_map_blocks *map,
> + struct erofs_map_dev *mdev);
>
> /* inode.c */
> static inline unsigned long erofs_inode_hash(erofs_nid_t nid)

2023-02-03 05:55:56

by Jingbo Xu

[permalink] [raw]
Subject: Re: [PATCH 3/3] erofs: call erofs_map_dev() inside erofs_map_blocks()



On 2/3/23 1:52 PM, Gao Xiang wrote:
>
>
> On 2023/2/3 11:53, Jingbo Xu wrote:
>> For now erofs_map_blocks() is always followed by erofs_map_dev().
>> Make erofs_map_dev() called inside erofs_map_blocks() to reduce code
>> duplication.
>
> Could we just integrate mdev into struct erofs_map_blocks?

Okay I will give it a try.


>
> BTW, I still think erofs_map_dev() is useful since it can handle
> non-inode IO requests, so let's keep this rather than mergeing all
> code into erofs_map_blocks()

Yeah erofs_map_dev() is kept as a separated function API.


--
Thanks,
Jingbo