2021-11-09 11:55:32

by Huang Jianan

[permalink] [raw]
Subject: [PATCH 1/2] erofs: add sysfs interface

Add sysfs interface to configure erofs related parameters in the
future.

Signed-off-by: Huang Jianan <[email protected]>
---
fs/erofs/Makefile | 2 +-
fs/erofs/internal.h | 10 ++
fs/erofs/super.c | 12 +++
fs/erofs/sysfs.c | 239 ++++++++++++++++++++++++++++++++++++++++++++
4 files changed, 262 insertions(+), 1 deletion(-)
create mode 100644 fs/erofs/sysfs.c

diff --git a/fs/erofs/Makefile b/fs/erofs/Makefile
index 756fe2d65272..8a3317e38e5a 100644
--- a/fs/erofs/Makefile
+++ b/fs/erofs/Makefile
@@ -1,7 +1,7 @@
# SPDX-License-Identifier: GPL-2.0-only

obj-$(CONFIG_EROFS_FS) += erofs.o
-erofs-objs := super.o inode.o data.o namei.o dir.o utils.o pcpubuf.o
+erofs-objs := super.o inode.o data.o namei.o dir.o utils.o pcpubuf.o sysfs.o
erofs-$(CONFIG_EROFS_FS_XATTR) += xattr.o
erofs-$(CONFIG_EROFS_FS_ZIP) += decompressor.o zmap.o zdata.o
erofs-$(CONFIG_EROFS_FS_ZIP_LZMA) += decompressor_lzma.o
diff --git a/fs/erofs/internal.h b/fs/erofs/internal.h
index 3265688af7f9..d0cd712dc222 100644
--- a/fs/erofs/internal.h
+++ b/fs/erofs/internal.h
@@ -134,6 +134,10 @@ struct erofs_sb_info {
u8 volume_name[16]; /* volume name */
u32 feature_compat;
u32 feature_incompat;
+
+ /* sysfs support */
+ struct kobject s_kobj; /* /sys/fs/erofs/<devname> */
+ struct completion s_kobj_unregister;
};

#define EROFS_SB(sb) ((struct erofs_sb_info *)(sb)->s_fs_info)
@@ -498,6 +502,12 @@ int erofs_pcpubuf_growsize(unsigned int nrpages);
void erofs_pcpubuf_init(void);
void erofs_pcpubuf_exit(void);

+/* sysfs.c */
+int erofs_register_sysfs(struct super_block *sb);
+void erofs_unregister_sysfs(struct super_block *sb);
+int __init erofs_init_sysfs(void);
+void erofs_exit_sysfs(void);
+
/* utils.c / zdata.c */
struct page *erofs_allocpage(struct page **pagepool, gfp_t gfp);
static inline void erofs_pagepool_add(struct page **pagepool,
diff --git a/fs/erofs/super.c b/fs/erofs/super.c
index 6a969b1e0ee6..abc1da5d1719 100644
--- a/fs/erofs/super.c
+++ b/fs/erofs/super.c
@@ -695,6 +695,10 @@ static int erofs_fc_fill_super(struct super_block *sb, struct fs_context *fc)
if (err)
return err;

+ err = erofs_register_sysfs(sb);
+ if (err)
+ return err;
+
erofs_info(sb, "mounted with root inode @ nid %llu.", ROOT_NID(sbi));
return 0;
}
@@ -808,6 +812,7 @@ static void erofs_put_super(struct super_block *sb)

DBG_BUGON(!sbi);

+ erofs_unregister_sysfs(sb);
erofs_shrinker_unregister(sb);
#ifdef CONFIG_EROFS_FS_ZIP
iput(sbi->managed_cache);
@@ -852,6 +857,10 @@ static int __init erofs_module_init(void)
if (err)
goto zip_err;

+ err = erofs_init_sysfs();
+ if (err)
+ goto sysfs_err;
+
err = register_filesystem(&erofs_fs_type);
if (err)
goto fs_err;
@@ -859,6 +868,8 @@ static int __init erofs_module_init(void)
return 0;

fs_err:
+ erofs_exit_sysfs();
+sysfs_err:
z_erofs_exit_zip_subsystem();
zip_err:
z_erofs_lzma_exit();
@@ -877,6 +888,7 @@ static void __exit erofs_module_exit(void)
/* Ensure all RCU free inodes / pclusters are safe to be destroyed. */
rcu_barrier();

+ erofs_exit_sysfs();
z_erofs_exit_zip_subsystem();
z_erofs_lzma_exit();
erofs_exit_shrinker();
diff --git a/fs/erofs/sysfs.c b/fs/erofs/sysfs.c
new file mode 100644
index 000000000000..c7685f1a8f34
--- /dev/null
+++ b/fs/erofs/sysfs.c
@@ -0,0 +1,239 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C), 2008-2021, OPPO Mobile Comm Corp., Ltd.
+ * https://www.oppo.com/
+ */
+#include <linux/sysfs.h>
+#include <linux/kobject.h>
+
+#include "internal.h"
+
+enum {
+ attr_feature,
+ attr_pointer_ui,
+ attr_pointer_bool,
+};
+
+enum {
+ struct_erofs_sb_info,
+};
+
+struct erofs_attr {
+ struct attribute attr;
+ short attr_id;
+ int struct_type;
+ int offset;
+};
+
+#define EROFS_ATTR(_name, _mode, _id) \
+static struct erofs_attr erofs_attr_##_name = { \
+ .attr = {.name = __stringify(_name), .mode = _mode }, \
+ .attr_id = attr_##_id, \
+}
+#define EROFS_ATTR_FUNC(_name, _mode) EROFS_ATTR(_name, _mode, _name)
+#define EROFS_FEATURE_ATTR(_name) EROFS_ATTR(_name, 0444, feature)
+
+#define EROFS_ATTR_OFFSET(_name, _mode, _id, _struct) \
+static struct erofs_attr erofs_attr_##_name = { \
+ .attr = {.name = __stringify(_name), .mode = _mode }, \
+ .attr_id = attr_##_id, \
+ .struct_type = struct_##_struct, \
+ .offset = offsetof(struct _struct, _name),\
+}
+
+#define EROFS_RW_ATTR(_name, _id, _struct) \
+ EROFS_ATTR_OFFSET(_name, 0644, _id, _struct)
+
+#define EROFS_RO_ATTR(_name, _id, _struct) \
+ EROFS_ATTR_OFFSET(_name, 0444, _id, _struct)
+
+#define EROFS_RW_ATTR_UI(_name, _struct) \
+ EROFS_RW_ATTR(_name, pointer_ui, _struct)
+
+#define EROFS_RW_ATTR_BOOL(_name, _struct) \
+ EROFS_RW_ATTR(_name, pointer_bool, _struct)
+
+#define ATTR_LIST(name) (&erofs_attr_##name.attr)
+
+static struct attribute *erofs_attrs[] = {
+ NULL,
+};
+ATTRIBUTE_GROUPS(erofs);
+
+/* Features this copy of erofs supports */
+EROFS_FEATURE_ATTR(lz4_0padding);
+EROFS_FEATURE_ATTR(compr_cfgs);
+EROFS_FEATURE_ATTR(big_pcluster);
+EROFS_FEATURE_ATTR(device_table);
+EROFS_FEATURE_ATTR(sb_chksum);
+
+static struct attribute *erofs_feat_attrs[] = {
+ ATTR_LIST(lz4_0padding),
+ ATTR_LIST(compr_cfgs),
+ ATTR_LIST(big_pcluster),
+ ATTR_LIST(device_table),
+ ATTR_LIST(sb_chksum),
+ NULL,
+};
+ATTRIBUTE_GROUPS(erofs_feat);
+
+static unsigned char *__struct_ptr(struct erofs_sb_info *sbi,
+ int struct_type, int offset)
+{
+ if (struct_type == struct_erofs_sb_info)
+ return (unsigned char *)sbi + offset;
+ return NULL;
+}
+
+static ssize_t erofs_attr_show(struct kobject *kobj,
+ struct attribute *attr, char *buf)
+{
+ struct erofs_sb_info *sbi = container_of(kobj, struct erofs_sb_info,
+ s_kobj);
+ struct erofs_attr *a = container_of(attr, struct erofs_attr, attr);
+ unsigned char *ptr = __struct_ptr(sbi, a->struct_type, a->offset);
+
+ switch (a->attr_id) {
+ case attr_feature:
+ return snprintf(buf, PAGE_SIZE, "supported\n");
+ case attr_pointer_ui:
+ if (!ptr)
+ return 0;
+ return snprintf(buf, PAGE_SIZE, "%u\n",
+ *((unsigned int *) ptr));
+ case attr_pointer_bool:
+ if (!ptr)
+ return 0;
+ return snprintf(buf, PAGE_SIZE,
+ *((bool *) ptr) ? "true\n" : "false\n");
+ }
+
+ return 0;
+}
+
+static ssize_t erofs_attr_store(struct kobject *kobj, struct attribute *attr,
+ const char *buf, size_t len)
+{
+ struct erofs_sb_info *sbi = container_of(kobj, struct erofs_sb_info,
+ s_kobj);
+ struct erofs_attr *a = container_of(attr, struct erofs_attr, attr);
+ unsigned char *ptr = __struct_ptr(sbi, a->struct_type, a->offset);
+ unsigned long t;
+ int ret;
+
+ switch (a->attr_id) {
+ case attr_pointer_ui:
+ if (!ptr)
+ return 0;
+ ret = kstrtoul(skip_spaces(buf), 0, &t);
+ if (ret)
+ return ret;
+ *((unsigned int *) ptr) = t;
+ return len;
+ case attr_pointer_bool:
+ if (!ptr)
+ return 0;
+ ret = kstrtoul(skip_spaces(buf), 0, &t);
+ if (ret)
+ return ret;
+ *((bool *) ptr) = !!t;
+ return len;
+ }
+
+ return 0;
+}
+
+static void erofs_sb_release(struct kobject *kobj)
+{
+ struct erofs_sb_info *sbi = container_of(kobj, struct erofs_sb_info,
+ s_kobj);
+ complete(&sbi->s_kobj_unregister);
+}
+
+static const struct sysfs_ops erofs_attr_ops = {
+ .show = erofs_attr_show,
+ .store = erofs_attr_store,
+};
+
+static struct kobj_type erofs_sb_ktype = {
+ .default_groups = erofs_groups,
+ .sysfs_ops = &erofs_attr_ops,
+ .release = erofs_sb_release,
+};
+
+static struct kobj_type erofs_ktype = {
+ .sysfs_ops = &erofs_attr_ops,
+};
+
+static struct kset erofs_root = {
+ .kobj = {.ktype = &erofs_ktype},
+};
+
+static struct kobj_type erofs_feat_ktype = {
+ .default_groups = erofs_feat_groups,
+ .sysfs_ops = &erofs_attr_ops,
+};
+
+static struct kobject erofs_feat = {
+ .kset = &erofs_root,
+};
+
+int erofs_register_sysfs(struct super_block *sb)
+{
+ struct erofs_sb_info *sbi = EROFS_SB(sb);
+ int err;
+
+ sbi->s_kobj.kset = &erofs_root;
+ init_completion(&sbi->s_kobj_unregister);
+ err = kobject_init_and_add(&sbi->s_kobj, &erofs_sb_ktype, NULL,
+ "%s", sb->s_id);
+ if (err)
+ goto put_sb_kobj;
+
+ return 0;
+
+put_sb_kobj:
+ kobject_put(&sbi->s_kobj);
+ wait_for_completion(&sbi->s_kobj_unregister);
+ return err;
+}
+
+void erofs_unregister_sysfs(struct super_block *sb)
+{
+ struct erofs_sb_info *sbi = EROFS_SB(sb);
+
+ kobject_del(&sbi->s_kobj);
+ kobject_put(&sbi->s_kobj);
+ wait_for_completion(&sbi->s_kobj_unregister);
+}
+
+int __init erofs_init_sysfs(void)
+{
+ int ret;
+
+ kobject_set_name(&erofs_root.kobj, "erofs");
+ erofs_root.kobj.parent = fs_kobj;
+ ret = kset_register(&erofs_root);
+ if (ret)
+ goto root_err;
+
+ ret = kobject_init_and_add(&erofs_feat, &erofs_feat_ktype,
+ NULL, "features");
+ if (ret)
+ goto feat_err;
+
+ return ret;
+
+feat_err:
+ kobject_put(&erofs_feat);
+ kset_unregister(&erofs_root);
+root_err:
+ return ret;
+}
+
+void erofs_exit_sysfs(void)
+{
+ kobject_put(&erofs_feat);
+ kset_unregister(&erofs_root);
+}
+
--
2.25.1


2021-11-09 11:56:31

by Huang Jianan

[permalink] [raw]
Subject: [PATCH 2/2] erofs: add sysfs node to control sync decompression strategy

Although readpage is a synchronous path, there will be no additional
kworker scheduling overhead in non-atomic contexts. So we can add a
sysfs node to allow disable sync decompression.

Signed-off-by: Huang Jianan <[email protected]>
---
fs/erofs/internal.h | 2 +-
fs/erofs/super.c | 2 +-
fs/erofs/sysfs.c | 6 ++++++
fs/erofs/zdata.c | 9 ++++-----
4 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/fs/erofs/internal.h b/fs/erofs/internal.h
index d0cd712dc222..1ab96878009d 100644
--- a/fs/erofs/internal.h
+++ b/fs/erofs/internal.h
@@ -60,7 +60,7 @@ struct erofs_mount_opts {
#ifdef CONFIG_EROFS_FS_ZIP
/* current strategy of how to use managed cache */
unsigned char cache_strategy;
- /* strategy of sync decompression (false - auto, true - force on) */
+ /* strategy of sync decompression (false - disable, true - force on) */
bool readahead_sync_decompress;

/* threshold for decompression synchronously */
diff --git a/fs/erofs/super.c b/fs/erofs/super.c
index abc1da5d1719..26585d865503 100644
--- a/fs/erofs/super.c
+++ b/fs/erofs/super.c
@@ -423,7 +423,7 @@ static void erofs_default_options(struct erofs_fs_context *ctx)
#ifdef CONFIG_EROFS_FS_ZIP
ctx->opt.cache_strategy = EROFS_ZIP_CACHE_READAROUND;
ctx->opt.max_sync_decompress_pages = 3;
- ctx->opt.readahead_sync_decompress = false;
+ ctx->opt.readahead_sync_decompress = true;
#endif
#ifdef CONFIG_EROFS_FS_XATTR
set_opt(&ctx->opt, XATTR_USER);
diff --git a/fs/erofs/sysfs.c b/fs/erofs/sysfs.c
index c7685f1a8f34..fe67ed490735 100644
--- a/fs/erofs/sysfs.c
+++ b/fs/erofs/sysfs.c
@@ -16,6 +16,7 @@ enum {

enum {
struct_erofs_sb_info,
+ struct_erofs_mount_opts,
};

struct erofs_attr {
@@ -55,7 +56,10 @@ static struct erofs_attr erofs_attr_##_name = { \

#define ATTR_LIST(name) (&erofs_attr_##name.attr)

+EROFS_RW_ATTR_BOOL(readahead_sync_decompress, erofs_mount_opts);
+
static struct attribute *erofs_attrs[] = {
+ ATTR_LIST(readahead_sync_decompress),
NULL,
};
ATTRIBUTE_GROUPS(erofs);
@@ -82,6 +86,8 @@ static unsigned char *__struct_ptr(struct erofs_sb_info *sbi,
{
if (struct_type == struct_erofs_sb_info)
return (unsigned char *)sbi + offset;
+ if (struct_type == struct_erofs_mount_opts)
+ return (unsigned char *)&sbi->opt + offset;
return NULL;
}

diff --git a/fs/erofs/zdata.c b/fs/erofs/zdata.c
index bcb1b91b234f..ad11333b367a 100644
--- a/fs/erofs/zdata.c
+++ b/fs/erofs/zdata.c
@@ -776,8 +776,6 @@ static void z_erofs_decompressqueue_work(struct work_struct *work);
static void z_erofs_decompress_kickoff(struct z_erofs_decompressqueue *io,
bool sync, int bios)
{
- struct erofs_sb_info *const sbi = EROFS_SB(io->sb);
-
/* wake up the caller thread for sync decompression */
if (sync) {
unsigned long flags;
@@ -791,10 +789,9 @@ static void z_erofs_decompress_kickoff(struct z_erofs_decompressqueue *io,

if (atomic_add_return(bios, &io->pending_bios))
return;
- /* Use workqueue and sync decompression for atomic contexts only */
+ /* Use workqueue decompression for atomic contexts only */
if (in_atomic() || irqs_disabled()) {
queue_work(z_erofs_workqueue, &io->u.work);
- sbi->opt.readahead_sync_decompress = true;
return;
}
z_erofs_decompressqueue_work(&io->u.work);
@@ -1454,6 +1451,7 @@ static void z_erofs_pcluster_readmore(struct z_erofs_decompress_frontend *f,
static int z_erofs_readpage(struct file *file, struct page *page)
{
struct inode *const inode = page->mapping->host;
+ struct erofs_sb_info *const sbi = EROFS_I_SB(inode);
struct z_erofs_decompress_frontend f = DECOMPRESS_FRONTEND_INIT(inode);
struct page *pagepool = NULL;
int err;
@@ -1469,7 +1467,8 @@ static int z_erofs_readpage(struct file *file, struct page *page)
(void)z_erofs_collector_end(&f.clt);

/* if some compressed cluster ready, need submit them anyway */
- z_erofs_runqueue(inode->i_sb, &f, &pagepool, true);
+ z_erofs_runqueue(inode->i_sb, &f, &pagepool,
+ sbi->opt.readahead_sync_decompress);

if (err)
erofs_err(inode->i_sb, "failed to read, err [%d]", err);
--
2.25.1

2021-11-09 12:09:38

by Gao Xiang

[permalink] [raw]
Subject: Re: [PATCH 2/2] erofs: add sysfs node to control sync decompression strategy

On Tue, Nov 09, 2021 at 10:54:45AM +0800, Huang Jianan via Linux-erofs wrote:
> Although readpage is a synchronous path, there will be no additional
> kworker scheduling overhead in non-atomic contexts. So we can add a
> sysfs node to allow disable sync decompression.
>
> Signed-off-by: Huang Jianan <[email protected]>
> ---
> fs/erofs/internal.h | 2 +-
> fs/erofs/super.c | 2 +-
> fs/erofs/sysfs.c | 6 ++++++
> fs/erofs/zdata.c | 9 ++++-----
> 4 files changed, 12 insertions(+), 7 deletions(-)
>
> diff --git a/fs/erofs/internal.h b/fs/erofs/internal.h
> index d0cd712dc222..1ab96878009d 100644
> --- a/fs/erofs/internal.h
> +++ b/fs/erofs/internal.h
> @@ -60,7 +60,7 @@ struct erofs_mount_opts {
> #ifdef CONFIG_EROFS_FS_ZIP
> /* current strategy of how to use managed cache */
> unsigned char cache_strategy;
> - /* strategy of sync decompression (false - auto, true - force on) */
> + /* strategy of sync decompression (false - disable, true - force on) */

Please leave false - auto.

> bool readahead_sync_decompress;
>
> /* threshold for decompression synchronously */
> diff --git a/fs/erofs/super.c b/fs/erofs/super.c
> index abc1da5d1719..26585d865503 100644
> --- a/fs/erofs/super.c
> +++ b/fs/erofs/super.c
> @@ -423,7 +423,7 @@ static void erofs_default_options(struct erofs_fs_context *ctx)
> #ifdef CONFIG_EROFS_FS_ZIP
> ctx->opt.cache_strategy = EROFS_ZIP_CACHE_READAROUND;
> ctx->opt.max_sync_decompress_pages = 3;
> - ctx->opt.readahead_sync_decompress = false;
> + ctx->opt.readahead_sync_decompress = true;

Please leave readahead_sync_decompress = false 'auto' by default.

I don't like .readpage() applies async decompression since it's
actually a sync way, otherwise, it will cause more scheduling
overhead, see:
https://lore.kernel.org/r/[email protected]
https://lore.kernel.org/r/[email protected]

Thanks,
Gao Xiang

2021-11-09 12:33:51

by Gao Xiang

[permalink] [raw]
Subject: Re: [PATCH 1/2] erofs: add sysfs interface

Hi Jianan,

On Tue, Nov 09, 2021 at 10:54:44AM +0800, Huang Jianan via Linux-erofs wrote:

You might need to add a "From:" tag here, otherwise, the author will
show "Huang Jianan via Linux-erofs" due to your mailing server...

> Add sysfs interface to configure erofs related parameters in the
> future.

s/in the future/Later/

>
> Signed-off-by: Huang Jianan <[email protected]>
> ---
> fs/erofs/Makefile | 2 +-
> fs/erofs/internal.h | 10 ++
> fs/erofs/super.c | 12 +++
> fs/erofs/sysfs.c | 239 ++++++++++++++++++++++++++++++++++++++++++++

At a quick glance, we might need to add sysfs API documentation
as well:
Documentation/ABI/testing/sysfs-fs-erofs

Thanks,
Gao Xiang

2021-11-09 12:52:51

by Joe Perches

[permalink] [raw]
Subject: Re: [PATCH 1/2] erofs: add sysfs interface

On Tue, 2021-11-09 at 10:54 +0800, Huang Jianan wrote:
> Add sysfs interface to configure erofs related parameters in the
> future.
[]
> diff --git a/fs/erofs/sysfs.c b/fs/erofs/sysfs.c
[]
> +static ssize_t erofs_attr_show(struct kobject *kobj,
> + struct attribute *attr, char *buf)
> +{
> + struct erofs_sb_info *sbi = container_of(kobj, struct erofs_sb_info,
> + s_kobj);
> + struct erofs_attr *a = container_of(attr, struct erofs_attr, attr);
> + unsigned char *ptr = __struct_ptr(sbi, a->struct_type, a->offset);
> +
> + switch (a->attr_id) {
> + case attr_feature:
> + return snprintf(buf, PAGE_SIZE, "supported\n");
> + case attr_pointer_ui:
> + if (!ptr)
> + return 0;
> + return snprintf(buf, PAGE_SIZE, "%u\n",
> + *((unsigned int *) ptr));

Prefer sysfs_emit over snprintf

case attr_feature:
return sysfs_emit(buf, "supported\n");
case attr_pointer_ui:
...
return sysfs_emit(buf, "%u\n", *(unsigned int *)ptr);

etc...


2021-11-09 13:18:13

by Huang Jianan

[permalink] [raw]
Subject: Re: [PATCH 1/2] erofs: add sysfs interface

?? 2021/11/9 11:14, Joe Perches д??:
> On Tue, 2021-11-09 at 10:54 +0800, Huang Jianan wrote:
>> Add sysfs interface to configure erofs related parameters in the
>> future.
> []
>> diff --git a/fs/erofs/sysfs.c b/fs/erofs/sysfs.c
> []
>> +static ssize_t erofs_attr_show(struct kobject *kobj,
>> + struct attribute *attr, char *buf)
>> +{
>> + struct erofs_sb_info *sbi = container_of(kobj, struct erofs_sb_info,
>> + s_kobj);
>> + struct erofs_attr *a = container_of(attr, struct erofs_attr, attr);
>> + unsigned char *ptr = __struct_ptr(sbi, a->struct_type, a->offset);
>> +
>> + switch (a->attr_id) {
>> + case attr_feature:
>> + return snprintf(buf, PAGE_SIZE, "supported\n");
>> + case attr_pointer_ui:
>> + if (!ptr)
>> + return 0;
>> + return snprintf(buf, PAGE_SIZE, "%u\n",
>> + *((unsigned int *) ptr));
> Prefer sysfs_emit over snprintf
>
> case attr_feature:
> return sysfs_emit(buf, "supported\n");
> case attr_pointer_ui:
> ...
> return sysfs_emit(buf, "%u\n", *(unsigned int *)ptr);
>
> etc...
>
Thanks for reminding this, I will fix it in the next version.

Thanks,
Jianan

2021-11-09 14:38:44

by Huang Jianan

[permalink] [raw]
Subject: Re: [PATCH 1/2] erofs: add sysfs interface

Hi Xiang,

在 2021/11/9 11:10, Gao Xiang 写道:
> Hi Jianan,
>
> On Tue, Nov 09, 2021 at 10:54:44AM +0800, Huang Jianan via Linux-erofs wrote:
>
> You might need to add a "From:" tag here, otherwise, the author will
> show "Huang Jianan via Linux-erofs" due to your mailing server...
I have used --from, maybe I need to add on the patch manually ... ????
>> Add sysfs interface to configure erofs related parameters in the
>> future.
> s/in the future/Later/
Will be fixed.
>> Signed-off-by: Huang Jianan <[email protected]>
>> ---
>> fs/erofs/Makefile | 2 +-
>> fs/erofs/internal.h | 10 ++
>> fs/erofs/super.c | 12 +++
>> fs/erofs/sysfs.c | 239 ++++++++++++++++++++++++++++++++++++++++++++
> At a quick glance, we might need to add sysfs API documentation
> as well:
> Documentation/ABI/testing/sysfs-fs-erofs
I will attach it in the next version.

Thanks,
Jianan
> Thanks,
> Gao Xiang
>

2021-11-09 15:01:39

by Huang Jianan

[permalink] [raw]
Subject: Re: [PATCH 2/2] erofs: add sysfs node to control sync decompression strategy

Hi Xiang,

在 2021/11/9 11:16, Gao Xiang 写道:
> On Tue, Nov 09, 2021 at 10:54:45AM +0800, Huang Jianan via Linux-erofs wrote:
>> Although readpage is a synchronous path, there will be no additional
>> kworker scheduling overhead in non-atomic contexts. So we can add a
>> sysfs node to allow disable sync decompression.
>>
>> Signed-off-by: Huang Jianan <[email protected]>
>> ---
>> fs/erofs/internal.h | 2 +-
>> fs/erofs/super.c | 2 +-
>> fs/erofs/sysfs.c | 6 ++++++
>> fs/erofs/zdata.c | 9 ++++-----
>> 4 files changed, 12 insertions(+), 7 deletions(-)
>>
>> diff --git a/fs/erofs/internal.h b/fs/erofs/internal.h
>> index d0cd712dc222..1ab96878009d 100644
>> --- a/fs/erofs/internal.h
>> +++ b/fs/erofs/internal.h
>> @@ -60,7 +60,7 @@ struct erofs_mount_opts {
>> #ifdef CONFIG_EROFS_FS_ZIP
>> /* current strategy of how to use managed cache */
>> unsigned char cache_strategy;
>> - /* strategy of sync decompression (false - auto, true - force on) */
>> + /* strategy of sync decompression (false - disable, true - force on) */
> Please leave false - auto.
Agree.
>> bool readahead_sync_decompress;
>>
>> /* threshold for decompression synchronously */
>> diff --git a/fs/erofs/super.c b/fs/erofs/super.c
>> index abc1da5d1719..26585d865503 100644
>> --- a/fs/erofs/super.c
>> +++ b/fs/erofs/super.c
>> @@ -423,7 +423,7 @@ static void erofs_default_options(struct erofs_fs_context *ctx)
>> #ifdef CONFIG_EROFS_FS_ZIP
>> ctx->opt.cache_strategy = EROFS_ZIP_CACHE_READAROUND;
>> ctx->opt.max_sync_decompress_pages = 3;
>> - ctx->opt.readahead_sync_decompress = false;
>> + ctx->opt.readahead_sync_decompress = true;
> Please leave readahead_sync_decompress = false 'auto' by default.
>
> I don't like .readpage() applies async decompression since it's
> actually a sync way, otherwise, it will cause more scheduling
> overhead, see:
> https://lore.kernel.org/r/[email protected]
> https://lore.kernel.org/r/[email protected]

Maybe consider adding a disbale strategy? And then we will have:
0 - auto, 1 - force on, 2 - disable
I will try this in the next version.

for .readpage(), If the decompression can be done in a non-atomic context,
there will be no additional scheduling overhead? In this case, we can
turn off
the syncdecompression.

Thanks,
Jianan

> Thanks,
> Gao Xiang

2021-11-09 15:31:29

by Huang Jianan

[permalink] [raw]
Subject: [PATCH v2 1/2] erofs: add sysfs interface

Add sysfs interface to configure erofs related parameters later.

Signed-off-by: Huang Jianan <[email protected]>
---
changes since v1:
- Add sysfs API documentation.
- Use sysfs_emit over snprintf.

Documentation/ABI/testing/sysfs-fs-erofs | 7 +
fs/erofs/Makefile | 2 +-
fs/erofs/internal.h | 10 +
fs/erofs/super.c | 12 ++
fs/erofs/sysfs.c | 237 +++++++++++++++++++++++
5 files changed, 267 insertions(+), 1 deletion(-)
create mode 100644 Documentation/ABI/testing/sysfs-fs-erofs
create mode 100644 fs/erofs/sysfs.c

diff --git a/Documentation/ABI/testing/sysfs-fs-erofs b/Documentation/ABI/testing/sysfs-fs-erofs
new file mode 100644
index 000000000000..86d0d0234473
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-fs-erofs
@@ -0,0 +1,7 @@
+What: /sys/fs/erofs/features/
+Date: November 2021
+Contact: "Huang Jianan" <[email protected]>
+Description: Shows all enabled kernel features.
+ Supported features:
+ lz4_0padding, compr_cfgs, big_pcluster, device_table,
+ sb_chksum.
diff --git a/fs/erofs/Makefile b/fs/erofs/Makefile
index 756fe2d65272..8a3317e38e5a 100644
--- a/fs/erofs/Makefile
+++ b/fs/erofs/Makefile
@@ -1,7 +1,7 @@
# SPDX-License-Identifier: GPL-2.0-only

obj-$(CONFIG_EROFS_FS) += erofs.o
-erofs-objs := super.o inode.o data.o namei.o dir.o utils.o pcpubuf.o
+erofs-objs := super.o inode.o data.o namei.o dir.o utils.o pcpubuf.o sysfs.o
erofs-$(CONFIG_EROFS_FS_XATTR) += xattr.o
erofs-$(CONFIG_EROFS_FS_ZIP) += decompressor.o zmap.o zdata.o
erofs-$(CONFIG_EROFS_FS_ZIP_LZMA) += decompressor_lzma.o
diff --git a/fs/erofs/internal.h b/fs/erofs/internal.h
index 3265688af7f9..d0cd712dc222 100644
--- a/fs/erofs/internal.h
+++ b/fs/erofs/internal.h
@@ -134,6 +134,10 @@ struct erofs_sb_info {
u8 volume_name[16]; /* volume name */
u32 feature_compat;
u32 feature_incompat;
+
+ /* sysfs support */
+ struct kobject s_kobj; /* /sys/fs/erofs/<devname> */
+ struct completion s_kobj_unregister;
};

#define EROFS_SB(sb) ((struct erofs_sb_info *)(sb)->s_fs_info)
@@ -498,6 +502,12 @@ int erofs_pcpubuf_growsize(unsigned int nrpages);
void erofs_pcpubuf_init(void);
void erofs_pcpubuf_exit(void);

+/* sysfs.c */
+int erofs_register_sysfs(struct super_block *sb);
+void erofs_unregister_sysfs(struct super_block *sb);
+int __init erofs_init_sysfs(void);
+void erofs_exit_sysfs(void);
+
/* utils.c / zdata.c */
struct page *erofs_allocpage(struct page **pagepool, gfp_t gfp);
static inline void erofs_pagepool_add(struct page **pagepool,
diff --git a/fs/erofs/super.c b/fs/erofs/super.c
index 6a969b1e0ee6..abc1da5d1719 100644
--- a/fs/erofs/super.c
+++ b/fs/erofs/super.c
@@ -695,6 +695,10 @@ static int erofs_fc_fill_super(struct super_block *sb, struct fs_context *fc)
if (err)
return err;

+ err = erofs_register_sysfs(sb);
+ if (err)
+ return err;
+
erofs_info(sb, "mounted with root inode @ nid %llu.", ROOT_NID(sbi));
return 0;
}
@@ -808,6 +812,7 @@ static void erofs_put_super(struct super_block *sb)

DBG_BUGON(!sbi);

+ erofs_unregister_sysfs(sb);
erofs_shrinker_unregister(sb);
#ifdef CONFIG_EROFS_FS_ZIP
iput(sbi->managed_cache);
@@ -852,6 +857,10 @@ static int __init erofs_module_init(void)
if (err)
goto zip_err;

+ err = erofs_init_sysfs();
+ if (err)
+ goto sysfs_err;
+
err = register_filesystem(&erofs_fs_type);
if (err)
goto fs_err;
@@ -859,6 +868,8 @@ static int __init erofs_module_init(void)
return 0;

fs_err:
+ erofs_exit_sysfs();
+sysfs_err:
z_erofs_exit_zip_subsystem();
zip_err:
z_erofs_lzma_exit();
@@ -877,6 +888,7 @@ static void __exit erofs_module_exit(void)
/* Ensure all RCU free inodes / pclusters are safe to be destroyed. */
rcu_barrier();

+ erofs_exit_sysfs();
z_erofs_exit_zip_subsystem();
z_erofs_lzma_exit();
erofs_exit_shrinker();
diff --git a/fs/erofs/sysfs.c b/fs/erofs/sysfs.c
new file mode 100644
index 000000000000..dd328d20c451
--- /dev/null
+++ b/fs/erofs/sysfs.c
@@ -0,0 +1,237 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C), 2008-2021, OPPO Mobile Comm Corp., Ltd.
+ * https://www.oppo.com/
+ */
+#include <linux/sysfs.h>
+#include <linux/kobject.h>
+
+#include "internal.h"
+
+enum {
+ attr_feature,
+ attr_pointer_ui,
+ attr_pointer_bool,
+};
+
+enum {
+ struct_erofs_sb_info,
+};
+
+struct erofs_attr {
+ struct attribute attr;
+ short attr_id;
+ int struct_type;
+ int offset;
+};
+
+#define EROFS_ATTR(_name, _mode, _id) \
+static struct erofs_attr erofs_attr_##_name = { \
+ .attr = {.name = __stringify(_name), .mode = _mode }, \
+ .attr_id = attr_##_id, \
+}
+#define EROFS_ATTR_FUNC(_name, _mode) EROFS_ATTR(_name, _mode, _name)
+#define EROFS_ATTR_FEATURE(_name) EROFS_ATTR(_name, 0444, feature)
+
+#define EROFS_ATTR_OFFSET(_name, _mode, _id, _struct) \
+static struct erofs_attr erofs_attr_##_name = { \
+ .attr = {.name = __stringify(_name), .mode = _mode }, \
+ .attr_id = attr_##_id, \
+ .struct_type = struct_##_struct, \
+ .offset = offsetof(struct _struct, _name),\
+}
+
+#define EROFS_ATTR_RW(_name, _id, _struct) \
+ EROFS_ATTR_OFFSET(_name, 0644, _id, _struct)
+
+#define EROFS_RO_ATTR(_name, _id, _struct) \
+ EROFS_ATTR_OFFSET(_name, 0444, _id, _struct)
+
+#define EROFS_ATTR_RW_UI(_name, _struct) \
+ EROFS_ATTR_RW(_name, pointer_ui, _struct)
+
+#define EROFS_ATTR_RW_BOOL(_name, _struct) \
+ EROFS_ATTR_RW(_name, pointer_bool, _struct)
+
+#define ATTR_LIST(name) (&erofs_attr_##name.attr)
+
+static struct attribute *erofs_attrs[] = {
+ NULL,
+};
+ATTRIBUTE_GROUPS(erofs);
+
+/* Features this copy of erofs supports */
+EROFS_ATTR_FEATURE(lz4_0padding);
+EROFS_ATTR_FEATURE(compr_cfgs);
+EROFS_ATTR_FEATURE(big_pcluster);
+EROFS_ATTR_FEATURE(device_table);
+EROFS_ATTR_FEATURE(sb_chksum);
+
+static struct attribute *erofs_feat_attrs[] = {
+ ATTR_LIST(lz4_0padding),
+ ATTR_LIST(compr_cfgs),
+ ATTR_LIST(big_pcluster),
+ ATTR_LIST(device_table),
+ ATTR_LIST(sb_chksum),
+ NULL,
+};
+ATTRIBUTE_GROUPS(erofs_feat);
+
+static unsigned char *__struct_ptr(struct erofs_sb_info *sbi,
+ int struct_type, int offset)
+{
+ if (struct_type == struct_erofs_sb_info)
+ return (unsigned char *)sbi + offset;
+ return NULL;
+}
+
+static ssize_t erofs_attr_show(struct kobject *kobj,
+ struct attribute *attr, char *buf)
+{
+ struct erofs_sb_info *sbi = container_of(kobj, struct erofs_sb_info,
+ s_kobj);
+ struct erofs_attr *a = container_of(attr, struct erofs_attr, attr);
+ unsigned char *ptr = __struct_ptr(sbi, a->struct_type, a->offset);
+
+ switch (a->attr_id) {
+ case attr_feature:
+ return sysfs_emit(buf, "supported\n");
+ case attr_pointer_ui:
+ if (!ptr)
+ return 0;
+ return sysfs_emit(buf, "%u\n", *(unsigned int *)ptr);
+ case attr_pointer_bool:
+ if (!ptr)
+ return 0;
+ return sysfs_emit(buf, *(bool *)ptr ? "true\n" : "false\n");
+ }
+
+ return 0;
+}
+
+static ssize_t erofs_attr_store(struct kobject *kobj, struct attribute *attr,
+ const char *buf, size_t len)
+{
+ struct erofs_sb_info *sbi = container_of(kobj, struct erofs_sb_info,
+ s_kobj);
+ struct erofs_attr *a = container_of(attr, struct erofs_attr, attr);
+ unsigned char *ptr = __struct_ptr(sbi, a->struct_type, a->offset);
+ unsigned long t;
+ int ret;
+
+ switch (a->attr_id) {
+ case attr_pointer_ui:
+ if (!ptr)
+ return 0;
+ ret = kstrtoul(skip_spaces(buf), 0, &t);
+ if (ret)
+ return ret;
+ *((unsigned int *) ptr) = t;
+ return len;
+ case attr_pointer_bool:
+ if (!ptr)
+ return 0;
+ ret = kstrtoul(skip_spaces(buf), 0, &t);
+ if (ret)
+ return ret;
+ *((bool *) ptr) = !!t;
+ return len;
+ }
+
+ return 0;
+}
+
+static void erofs_sb_release(struct kobject *kobj)
+{
+ struct erofs_sb_info *sbi = container_of(kobj, struct erofs_sb_info,
+ s_kobj);
+ complete(&sbi->s_kobj_unregister);
+}
+
+static const struct sysfs_ops erofs_attr_ops = {
+ .show = erofs_attr_show,
+ .store = erofs_attr_store,
+};
+
+static struct kobj_type erofs_sb_ktype = {
+ .default_groups = erofs_groups,
+ .sysfs_ops = &erofs_attr_ops,
+ .release = erofs_sb_release,
+};
+
+static struct kobj_type erofs_ktype = {
+ .sysfs_ops = &erofs_attr_ops,
+};
+
+static struct kset erofs_root = {
+ .kobj = {.ktype = &erofs_ktype},
+};
+
+static struct kobj_type erofs_feat_ktype = {
+ .default_groups = erofs_feat_groups,
+ .sysfs_ops = &erofs_attr_ops,
+};
+
+static struct kobject erofs_feat = {
+ .kset = &erofs_root,
+};
+
+int erofs_register_sysfs(struct super_block *sb)
+{
+ struct erofs_sb_info *sbi = EROFS_SB(sb);
+ int err;
+
+ sbi->s_kobj.kset = &erofs_root;
+ init_completion(&sbi->s_kobj_unregister);
+ err = kobject_init_and_add(&sbi->s_kobj, &erofs_sb_ktype, NULL,
+ "%s", sb->s_id);
+ if (err)
+ goto put_sb_kobj;
+
+ return 0;
+
+put_sb_kobj:
+ kobject_put(&sbi->s_kobj);
+ wait_for_completion(&sbi->s_kobj_unregister);
+ return err;
+}
+
+void erofs_unregister_sysfs(struct super_block *sb)
+{
+ struct erofs_sb_info *sbi = EROFS_SB(sb);
+
+ kobject_del(&sbi->s_kobj);
+ kobject_put(&sbi->s_kobj);
+ wait_for_completion(&sbi->s_kobj_unregister);
+}
+
+int __init erofs_init_sysfs(void)
+{
+ int ret;
+
+ kobject_set_name(&erofs_root.kobj, "erofs");
+ erofs_root.kobj.parent = fs_kobj;
+ ret = kset_register(&erofs_root);
+ if (ret)
+ goto root_err;
+
+ ret = kobject_init_and_add(&erofs_feat, &erofs_feat_ktype,
+ NULL, "features");
+ if (ret)
+ goto feat_err;
+
+ return ret;
+
+feat_err:
+ kobject_put(&erofs_feat);
+ kset_unregister(&erofs_root);
+root_err:
+ return ret;
+}
+
+void erofs_exit_sysfs(void)
+{
+ kobject_put(&erofs_feat);
+ kset_unregister(&erofs_root);
+}
+
--
2.25.1

2021-11-09 23:50:09

by Chao Yu

[permalink] [raw]
Subject: Re: [PATCH v2 1/2] erofs: add sysfs interface

On 2021/11/9 15:45, Huang Jianan via Linux-erofs wrote:
> Add sysfs interface to configure erofs related parameters later.
>
> Signed-off-by: Huang Jianan <[email protected]>
> ---
> changes since v1:
> - Add sysfs API documentation.
> - Use sysfs_emit over snprintf.
>
> Documentation/ABI/testing/sysfs-fs-erofs | 7 +
> fs/erofs/Makefile | 2 +-
> fs/erofs/internal.h | 10 +
> fs/erofs/super.c | 12 ++
> fs/erofs/sysfs.c | 237 +++++++++++++++++++++++
> 5 files changed, 267 insertions(+), 1 deletion(-)
> create mode 100644 Documentation/ABI/testing/sysfs-fs-erofs
> create mode 100644 fs/erofs/sysfs.c
>
> diff --git a/Documentation/ABI/testing/sysfs-fs-erofs b/Documentation/ABI/testing/sysfs-fs-erofs
> new file mode 100644
> index 000000000000..86d0d0234473
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-fs-erofs
> @@ -0,0 +1,7 @@
> +What: /sys/fs/erofs/features/
> +Date: November 2021
> +Contact: "Huang Jianan" <[email protected]>
> +Description: Shows all enabled kernel features.
> + Supported features:
> + lz4_0padding, compr_cfgs, big_pcluster, device_table,
> + sb_chksum.
> diff --git a/fs/erofs/Makefile b/fs/erofs/Makefile
> index 756fe2d65272..8a3317e38e5a 100644
> --- a/fs/erofs/Makefile
> +++ b/fs/erofs/Makefile
> @@ -1,7 +1,7 @@
> # SPDX-License-Identifier: GPL-2.0-only
>
> obj-$(CONFIG_EROFS_FS) += erofs.o
> -erofs-objs := super.o inode.o data.o namei.o dir.o utils.o pcpubuf.o
> +erofs-objs := super.o inode.o data.o namei.o dir.o utils.o pcpubuf.o sysfs.o
> erofs-$(CONFIG_EROFS_FS_XATTR) += xattr.o
> erofs-$(CONFIG_EROFS_FS_ZIP) += decompressor.o zmap.o zdata.o
> erofs-$(CONFIG_EROFS_FS_ZIP_LZMA) += decompressor_lzma.o
> diff --git a/fs/erofs/internal.h b/fs/erofs/internal.h
> index 3265688af7f9..d0cd712dc222 100644
> --- a/fs/erofs/internal.h
> +++ b/fs/erofs/internal.h
> @@ -134,6 +134,10 @@ struct erofs_sb_info {
> u8 volume_name[16]; /* volume name */
> u32 feature_compat;
> u32 feature_incompat;
> +
> + /* sysfs support */
> + struct kobject s_kobj; /* /sys/fs/erofs/<devname> */
> + struct completion s_kobj_unregister;
> };
>
> #define EROFS_SB(sb) ((struct erofs_sb_info *)(sb)->s_fs_info)
> @@ -498,6 +502,12 @@ int erofs_pcpubuf_growsize(unsigned int nrpages);
> void erofs_pcpubuf_init(void);
> void erofs_pcpubuf_exit(void);
>
> +/* sysfs.c */
> +int erofs_register_sysfs(struct super_block *sb);
> +void erofs_unregister_sysfs(struct super_block *sb);
> +int __init erofs_init_sysfs(void);
> +void erofs_exit_sysfs(void);
> +
> /* utils.c / zdata.c */
> struct page *erofs_allocpage(struct page **pagepool, gfp_t gfp);
> static inline void erofs_pagepool_add(struct page **pagepool,
> diff --git a/fs/erofs/super.c b/fs/erofs/super.c
> index 6a969b1e0ee6..abc1da5d1719 100644
> --- a/fs/erofs/super.c
> +++ b/fs/erofs/super.c
> @@ -695,6 +695,10 @@ static int erofs_fc_fill_super(struct super_block *sb, struct fs_context *fc)
> if (err)
> return err;
>
> + err = erofs_register_sysfs(sb);
> + if (err)
> + return err;
> +
> erofs_info(sb, "mounted with root inode @ nid %llu.", ROOT_NID(sbi));
> return 0;
> }
> @@ -808,6 +812,7 @@ static void erofs_put_super(struct super_block *sb)
>
> DBG_BUGON(!sbi);
>
> + erofs_unregister_sysfs(sb);
> erofs_shrinker_unregister(sb);
> #ifdef CONFIG_EROFS_FS_ZIP
> iput(sbi->managed_cache);
> @@ -852,6 +857,10 @@ static int __init erofs_module_init(void)
> if (err)
> goto zip_err;
>
> + err = erofs_init_sysfs();
> + if (err)
> + goto sysfs_err;
> +
> err = register_filesystem(&erofs_fs_type);
> if (err)
> goto fs_err;
> @@ -859,6 +868,8 @@ static int __init erofs_module_init(void)
> return 0;
>
> fs_err:
> + erofs_exit_sysfs();
> +sysfs_err:
> z_erofs_exit_zip_subsystem();
> zip_err:
> z_erofs_lzma_exit();
> @@ -877,6 +888,7 @@ static void __exit erofs_module_exit(void)
> /* Ensure all RCU free inodes / pclusters are safe to be destroyed. */
> rcu_barrier();
>
> + erofs_exit_sysfs();
> z_erofs_exit_zip_subsystem();
> z_erofs_lzma_exit();
> erofs_exit_shrinker();
> diff --git a/fs/erofs/sysfs.c b/fs/erofs/sysfs.c
> new file mode 100644
> index 000000000000..dd328d20c451
> --- /dev/null
> +++ b/fs/erofs/sysfs.c
> @@ -0,0 +1,237 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C), 2008-2021, OPPO Mobile Comm Corp., Ltd.
> + * https://www.oppo.com/
> + */
> +#include <linux/sysfs.h>
> +#include <linux/kobject.h>
> +
> +#include "internal.h"
> +
> +enum {
> + attr_feature,
> + attr_pointer_ui,
> + attr_pointer_bool,
> +};
> +
> +enum {
> + struct_erofs_sb_info,
> +};
> +
> +struct erofs_attr {
> + struct attribute attr;
> + short attr_id;
> + int struct_type;
> + int offset;
> +};
> +
> +#define EROFS_ATTR(_name, _mode, _id) \
> +static struct erofs_attr erofs_attr_##_name = { \
> + .attr = {.name = __stringify(_name), .mode = _mode }, \
> + .attr_id = attr_##_id, \
> +}
> +#define EROFS_ATTR_FUNC(_name, _mode) EROFS_ATTR(_name, _mode, _name)
> +#define EROFS_ATTR_FEATURE(_name) EROFS_ATTR(_name, 0444, feature)
> +
> +#define EROFS_ATTR_OFFSET(_name, _mode, _id, _struct) \
> +static struct erofs_attr erofs_attr_##_name = { \
> + .attr = {.name = __stringify(_name), .mode = _mode }, \
> + .attr_id = attr_##_id, \
> + .struct_type = struct_##_struct, \
> + .offset = offsetof(struct _struct, _name),\
> +}
> +
> +#define EROFS_ATTR_RW(_name, _id, _struct) \
> + EROFS_ATTR_OFFSET(_name, 0644, _id, _struct)
> +
> +#define EROFS_RO_ATTR(_name, _id, _struct) \
> + EROFS_ATTR_OFFSET(_name, 0444, _id, _struct)
> +
> +#define EROFS_ATTR_RW_UI(_name, _struct) \
> + EROFS_ATTR_RW(_name, pointer_ui, _struct)
> +
> +#define EROFS_ATTR_RW_BOOL(_name, _struct) \
> + EROFS_ATTR_RW(_name, pointer_bool, _struct)
> +
> +#define ATTR_LIST(name) (&erofs_attr_##name.attr)
> +
> +static struct attribute *erofs_attrs[] = {
> + NULL,
> +};
> +ATTRIBUTE_GROUPS(erofs);
> +
> +/* Features this copy of erofs supports */
> +EROFS_ATTR_FEATURE(lz4_0padding);
> +EROFS_ATTR_FEATURE(compr_cfgs);
> +EROFS_ATTR_FEATURE(big_pcluster);
> +EROFS_ATTR_FEATURE(device_table);
> +EROFS_ATTR_FEATURE(sb_chksum);
> +
> +static struct attribute *erofs_feat_attrs[] = {
> + ATTR_LIST(lz4_0padding),
> + ATTR_LIST(compr_cfgs),
> + ATTR_LIST(big_pcluster),
> + ATTR_LIST(device_table),
> + ATTR_LIST(sb_chksum),
> + NULL,
> +};
> +ATTRIBUTE_GROUPS(erofs_feat);
> +
> +static unsigned char *__struct_ptr(struct erofs_sb_info *sbi,
> + int struct_type, int offset)
> +{
> + if (struct_type == struct_erofs_sb_info)
> + return (unsigned char *)sbi + offset;
> + return NULL;
> +}
> +
> +static ssize_t erofs_attr_show(struct kobject *kobj,
> + struct attribute *attr, char *buf)
> +{
> + struct erofs_sb_info *sbi = container_of(kobj, struct erofs_sb_info,
> + s_kobj);
> + struct erofs_attr *a = container_of(attr, struct erofs_attr, attr);
> + unsigned char *ptr = __struct_ptr(sbi, a->struct_type, a->offset);
> +
> + switch (a->attr_id) {
> + case attr_feature:
> + return sysfs_emit(buf, "supported\n");
> + case attr_pointer_ui:
> + if (!ptr)
> + return 0;
> + return sysfs_emit(buf, "%u\n", *(unsigned int *)ptr);
> + case attr_pointer_bool:
> + if (!ptr)
> + return 0;
> + return sysfs_emit(buf, *(bool *)ptr ? "true\n" : "false\n");

Is it fine to just print 1 or 0 like the value user set via the sysfs entry
to indicate true or false.

> + }
> +
> + return 0;
> +}
> +
> +static ssize_t erofs_attr_store(struct kobject *kobj, struct attribute *attr,
> + const char *buf, size_t len)
> +{
> + struct erofs_sb_info *sbi = container_of(kobj, struct erofs_sb_info,
> + s_kobj);
> + struct erofs_attr *a = container_of(attr, struct erofs_attr, attr);
> + unsigned char *ptr = __struct_ptr(sbi, a->struct_type, a->offset);
> + unsigned long t;
> + int ret;
> +
> + switch (a->attr_id) {
> + case attr_pointer_ui:
> + if (!ptr)
> + return 0;
> + ret = kstrtoul(skip_spaces(buf), 0, &t);
> + if (ret)
> + return ret;

if (t > UINT_MAX)
return -EINVAL;

> + *((unsigned int *) ptr) = t;
> + return len;
> + case attr_pointer_bool:
> + if (!ptr)
> + return 0;
> + ret = kstrtoul(skip_spaces(buf), 0, &t);
> + if (ret)
> + return ret;

@t should be 0 or 1?

if (t != 0 && t != 1)
return -EINVAL;

Thanks,

> + *((bool *) ptr) = !!t;
> + return len;
> + }
> +
> + return 0;
> +}
> +
> +static void erofs_sb_release(struct kobject *kobj)
> +{
> + struct erofs_sb_info *sbi = container_of(kobj, struct erofs_sb_info,
> + s_kobj);
> + complete(&sbi->s_kobj_unregister);
> +}
> +
> +static const struct sysfs_ops erofs_attr_ops = {
> + .show = erofs_attr_show,
> + .store = erofs_attr_store,
> +};
> +
> +static struct kobj_type erofs_sb_ktype = {
> + .default_groups = erofs_groups,
> + .sysfs_ops = &erofs_attr_ops,
> + .release = erofs_sb_release,
> +};
> +
> +static struct kobj_type erofs_ktype = {
> + .sysfs_ops = &erofs_attr_ops,
> +};
> +
> +static struct kset erofs_root = {
> + .kobj = {.ktype = &erofs_ktype},
> +};
> +
> +static struct kobj_type erofs_feat_ktype = {
> + .default_groups = erofs_feat_groups,
> + .sysfs_ops = &erofs_attr_ops,
> +};
> +
> +static struct kobject erofs_feat = {
> + .kset = &erofs_root,
> +};
> +
> +int erofs_register_sysfs(struct super_block *sb)
> +{
> + struct erofs_sb_info *sbi = EROFS_SB(sb);
> + int err;
> +
> + sbi->s_kobj.kset = &erofs_root;
> + init_completion(&sbi->s_kobj_unregister);
> + err = kobject_init_and_add(&sbi->s_kobj, &erofs_sb_ktype, NULL,
> + "%s", sb->s_id);
> + if (err)
> + goto put_sb_kobj;
> +
> + return 0;
> +
> +put_sb_kobj:
> + kobject_put(&sbi->s_kobj);
> + wait_for_completion(&sbi->s_kobj_unregister);
> + return err;
> +}
> +
> +void erofs_unregister_sysfs(struct super_block *sb)
> +{
> + struct erofs_sb_info *sbi = EROFS_SB(sb);
> +
> + kobject_del(&sbi->s_kobj);
> + kobject_put(&sbi->s_kobj);
> + wait_for_completion(&sbi->s_kobj_unregister);
> +}
> +
> +int __init erofs_init_sysfs(void)
> +{
> + int ret;
> +
> + kobject_set_name(&erofs_root.kobj, "erofs");
> + erofs_root.kobj.parent = fs_kobj;
> + ret = kset_register(&erofs_root);
> + if (ret)
> + goto root_err;
> +
> + ret = kobject_init_and_add(&erofs_feat, &erofs_feat_ktype,
> + NULL, "features");
> + if (ret)
> + goto feat_err;
> +
> + return ret;
> +
> +feat_err:
> + kobject_put(&erofs_feat);
> + kset_unregister(&erofs_root);
> +root_err:
> + return ret;
> +}
> +
> +void erofs_exit_sysfs(void)
> +{
> + kobject_put(&erofs_feat);
> + kset_unregister(&erofs_root);
> +}
> +
>

2021-11-11 20:17:58

by kernel test robot

[permalink] [raw]
Subject: Re: [PATCH 2/2] erofs: add sysfs node to control sync decompression strategy

Hi Huang,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on xiang-erofs/dev-test]
[also build test ERROR on next-20211111]
[cannot apply to v5.15]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url: https://github.com/0day-ci/linux/commits/Huang-Jianan/erofs-add-sysfs-interface/20211109-105542
base: https://git.kernel.org/pub/scm/linux/kernel/git/xiang/erofs.git dev-test
config: xtensa-buildonly-randconfig-r004-20211111 (attached as .config)
compiler: xtensa-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/d6bf9edf69e87ee0a9795421ff2e1a9b69a29ce8
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Huang-Jianan/erofs-add-sysfs-interface/20211109-105542
git checkout d6bf9edf69e87ee0a9795421ff2e1a9b69a29ce8
# save the attached .config to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=xtensa SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>

All errors (new ones prefixed by >>):

In file included from <command-line>:
>> include/linux/compiler_types.h:140:41: error: 'struct erofs_mount_opts' has no member named 'readahead_sync_decompress'
140 | #define __compiler_offsetof(a, b) __builtin_offsetof(a, b)
| ^~~~~~~~~~~~~~~~~~
include/linux/stddef.h:17:33: note: in expansion of macro '__compiler_offsetof'
17 | #define offsetof(TYPE, MEMBER) __compiler_offsetof(TYPE, MEMBER)
| ^~~~~~~~~~~~~~~~~~~
fs/erofs/sysfs.c:42:19: note: in expansion of macro 'offsetof'
42 | .offset = offsetof(struct _struct, _name),\
| ^~~~~~~~
fs/erofs/sysfs.c:46:9: note: in expansion of macro 'EROFS_ATTR_OFFSET'
46 | EROFS_ATTR_OFFSET(_name, 0644, _id, _struct)
| ^~~~~~~~~~~~~~~~~
fs/erofs/sysfs.c:55:9: note: in expansion of macro 'EROFS_RW_ATTR'
55 | EROFS_RW_ATTR(_name, pointer_bool, _struct)
| ^~~~~~~~~~~~~
fs/erofs/sysfs.c:59:1: note: in expansion of macro 'EROFS_RW_ATTR_BOOL'
59 | EROFS_RW_ATTR_BOOL(readahead_sync_decompress, erofs_mount_opts);
| ^~~~~~~~~~~~~~~~~~


vim +140 include/linux/compiler_types.h

71391bdd2e9aab Xiaozhou Liu 2018-12-14 139
71391bdd2e9aab Xiaozhou Liu 2018-12-14 @140 #define __compiler_offsetof(a, b) __builtin_offsetof(a, b)
71391bdd2e9aab Xiaozhou Liu 2018-12-14 141

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]


Attachments:
(No filename) (3.00 kB)
.config.gz (37.11 kB)
Download all attachments

2021-11-12 02:08:49

by Huang Jianan

[permalink] [raw]
Subject: Re: [PATCH 2/2] erofs: add sysfs node to control sync decompression strategy

在 2021/11/12 4:17, kernel test robot 写道:
> Hi Huang,
>
> Thank you for the patch! Yet something to improve:
>
> [auto build test ERROR on xiang-erofs/dev-test]
> [also build test ERROR on next-20211111]
> [cannot apply to v5.15]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://apc01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit-scm.com%2Fdocs%2Fgit-format-patch&amp;data=04%7C01%7Chuangjianan%40oppo.com%7C0d3d672ac3e94a90cf9108d9a5505aae%7Cf1905eb1c35341c5951662b4a54b5ee6%7C0%7C0%7C637722589678393057%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=OVZ%2BPtlKYw1zTA3lxF2mFAqcHDs6FjweHQpTpS%2B6uog%3D&amp;reserved=0]
>
> url: https://apc01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2F0day-ci%2Flinux%2Fcommits%2FHuang-Jianan%2Ferofs-add-sysfs-interface%2F20211109-105542&amp;data=04%7C01%7Chuangjianan%40oppo.com%7C0d3d672ac3e94a90cf9108d9a5505aae%7Cf1905eb1c35341c5951662b4a54b5ee6%7C0%7C0%7C637722589678403052%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=vvNfYf7RfWT813%2FG%2FOUvTPLOyYqlw1FXQZXjPuqoqsk%3D&amp;reserved=0
> base: https://git.kernel.org/pub/scm/linux/kernel/git/xiang/erofs.git dev-test
> config: xtensa-buildonly-randconfig-r004-20211111 (attached as .config)
> compiler: xtensa-linux-gcc (GCC) 11.2.0
> reproduce (this is a W=1 build):
> wget https://apc01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fraw.githubusercontent.com%2Fintel%2Flkp-tests%2Fmaster%2Fsbin%2Fmake.cross&amp;data=04%7C01%7Chuangjianan%40oppo.com%7C0d3d672ac3e94a90cf9108d9a5505aae%7Cf1905eb1c35341c5951662b4a54b5ee6%7C0%7C0%7C637722589678403052%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=SIkgxAB9wqCI7O0OjvYinNEpFtSD7vUO2YX%2FwQFC480%3D&amp;reserved=0 -O ~/bin/make.cross
> chmod +x ~/bin/make.cross
> # https://apc01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2F0day-ci%2Flinux%2Fcommit%2Fd6bf9edf69e87ee0a9795421ff2e1a9b69a29ce8&amp;data=04%7C01%7Chuangjianan%40oppo.com%7C0d3d672ac3e94a90cf9108d9a5505aae%7Cf1905eb1c35341c5951662b4a54b5ee6%7C0%7C0%7C637722589678403052%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=%2BHaZRNBm3bV0VbSMU0vlz6pokoI3tA8kNpVxCGF1wtc%3D&amp;reserved=0
> git remote add linux-review https://apc01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2F0day-ci%2Flinux&amp;data=04%7C01%7Chuangjianan%40oppo.com%7C0d3d672ac3e94a90cf9108d9a5505aae%7Cf1905eb1c35341c5951662b4a54b5ee6%7C0%7C0%7C637722589678403052%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=ICXqRmM2WczvCZQYR9hBUVQddexNDPdhSmr80YLCueY%3D&amp;reserved=0
> git fetch --no-tags linux-review Huang-Jianan/erofs-add-sysfs-interface/20211109-105542
> git checkout d6bf9edf69e87ee0a9795421ff2e1a9b69a29ce8
> # save the attached .config to linux build tree
> mkdir build_dir
> COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=xtensa SHELL=/bin/bash
>
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <[email protected]>
>
> All errors (new ones prefixed by >>):
>
> In file included from <command-line>:
>>> include/linux/compiler_types.h:140:41: error: 'struct erofs_mount_opts' has no member named 'readahead_sync_decompress'
This problem has been fixed by adding macros in v2.
I will resend a final version of the patch in a clean chain later. ????

Thanks,
Jianan
> 140 | #define __compiler_offsetof(a, b) __builtin_offsetof(a, b)
> | ^~~~~~~~~~~~~~~~~~
> include/linux/stddef.h:17:33: note: in expansion of macro '__compiler_offsetof'
> 17 | #define offsetof(TYPE, MEMBER) __compiler_offsetof(TYPE, MEMBER)
> | ^~~~~~~~~~~~~~~~~~~
> fs/erofs/sysfs.c:42:19: note: in expansion of macro 'offsetof'
> 42 | .offset = offsetof(struct _struct, _name),\
> | ^~~~~~~~
> fs/erofs/sysfs.c:46:9: note: in expansion of macro 'EROFS_ATTR_OFFSET'
> 46 | EROFS_ATTR_OFFSET(_name, 0644, _id, _struct)
> | ^~~~~~~~~~~~~~~~~
> fs/erofs/sysfs.c:55:9: note: in expansion of macro 'EROFS_RW_ATTR'
> 55 | EROFS_RW_ATTR(_name, pointer_bool, _struct)
> | ^~~~~~~~~~~~~
> fs/erofs/sysfs.c:59:1: note: in expansion of macro 'EROFS_RW_ATTR_BOOL'
> 59 | EROFS_RW_ATTR_BOOL(readahead_sync_decompress, erofs_mount_opts);
> | ^~~~~~~~~~~~~~~~~~
>
>
> vim +140 include/linux/compiler_types.h
>
> 71391bdd2e9aab Xiaozhou Liu 2018-12-14 139
> 71391bdd2e9aab Xiaozhou Liu 2018-12-14 @140 #define __compiler_offsetof(a, b) __builtin_offsetof(a, b)
> 71391bdd2e9aab Xiaozhou Liu 2018-12-14 141
>
> ---
> 0-DAY CI Kernel Test Service, Intel Corporation
> https://apc01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.01.org%2Fhyperkitty%2Flist%2Fkbuild-all%40lists.01.org&amp;data=04%7C01%7Chuangjianan%40oppo.com%7C0d3d672ac3e94a90cf9108d9a5505aae%7Cf1905eb1c35341c5951662b4a54b5ee6%7C0%7C0%7C637722589678403052%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=I8XIGfml9qKANF9lYjvZ1eRcBGmbE%2BPDSsaajTOgfXo%3D&amp;reserved=0