lkml.org 
[lkml]   [2021]   [Oct]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [PATCH 1/1] ubifs: ubifs to export filesystem error counters
    Stefan,

    ----- Ursprüngliche Mail -----
    > Von: "schaecsn" <schaecsn@gmx.net>
    > An: "richard" <richard@nod.at>, "linux-mtd" <linux-mtd@lists.infradead.org>, "linux-kernel"
    > <linux-kernel@vger.kernel.org>
    > CC: "schaecsn" <schaecsn@gmx.net>, "Stefan Schaeckeler" <sschaeck@cisco.com>
    > Gesendet: Dienstag, 7. September 2021 23:40:34
    > Betreff: [PATCH 1/1] ubifs: ubifs to export filesystem error counters

    > Not all ubifs filesystem errors are propagated to userspace.
    >
    > Export bad magic, bad node and crc errors via sysfs. This allows userspace
    > to notice filesystem errors:
    >
    > /sys/fs/ubifs/ubiX_Y/errors_magic
    > /sys/fs/ubifs/ubiX_Y/errors_node
    > /sys/fs/ubifs/ubiX_Y/errors_crc
    >
    > The counters are reset to 0 with a remount. Writing anything into the
    > counters also clears them.

    I think this is a nice feature. Thanks for implementing it.
    Please see some minor nits below.

    Is there a specific reason why you didn't implement it via UBIFS's debugfs interface?
    sysfs is ABI, so we cannot change much anymore.

    > Signed-off-by: Stefan Schaeckeler <sschaeck@cisco.com>
    > ---
    > fs/ubifs/Makefile | 2 +-
    > fs/ubifs/io.c | 6 ++
    > fs/ubifs/super.c | 17 ++++-
    > fs/ubifs/sysfs.c | 187 ++++++++++++++++++++++++++++++++++++++++++++++
    > fs/ubifs/sysfs.h | 39 ++++++++++
    > fs/ubifs/ubifs.h | 11 +++
    > 6 files changed, 260 insertions(+), 2 deletions(-)
    > create mode 100644 fs/ubifs/sysfs.c
    > create mode 100644 fs/ubifs/sysfs.h
    >
    > diff --git a/fs/ubifs/Makefile b/fs/ubifs/Makefile
    > index 5c4b845754a7..314c80b24a76 100644
    > --- a/fs/ubifs/Makefile
    > +++ b/fs/ubifs/Makefile
    > @@ -5,7 +5,7 @@ ubifs-y += shrinker.o journal.o file.o dir.o super.o sb.o io.o
    > ubifs-y += tnc.o master.o scan.o replay.o log.o commit.o gc.o orphan.o
    > ubifs-y += budget.o find.o tnc_commit.o compress.o lpt.o lprops.o
    > ubifs-y += recovery.o ioctl.o lpt_commit.o tnc_misc.o debug.o
    > -ubifs-y += misc.o
    > +ubifs-y += misc.o sysfs.o
    > ubifs-$(CONFIG_FS_ENCRYPTION) += crypto.o
    > ubifs-$(CONFIG_UBIFS_FS_XATTR) += xattr.o
    > ubifs-$(CONFIG_UBIFS_FS_AUTHENTICATION) += auth.o
    > diff --git a/fs/ubifs/io.c b/fs/ubifs/io.c
    > index 00b61dba62b7..0b158e420cc1 100644
    > --- a/fs/ubifs/io.c
    > +++ b/fs/ubifs/io.c
    > @@ -238,6 +238,8 @@ int ubifs_check_node(const struct ubifs_info *c, const void
    > *buf, int len,
    > if (!quiet)
    > ubifs_err(c, "bad magic %#08x, expected %#08x",
    > magic, UBIFS_NODE_MAGIC);
    > + if (c->stats)
    > + c->stats->magic_errors++;

    Please wrap this into a helper function.

    > err = -EUCLEAN;
    > goto out;
    > }
    > @@ -246,6 +248,8 @@ int ubifs_check_node(const struct ubifs_info *c, const void
    > *buf, int len,
    > if (type < 0 || type >= UBIFS_NODE_TYPES_CNT) {
    > if (!quiet)
    > ubifs_err(c, "bad node type %d", type);
    > + if (c->stats)
    > + c->stats->node_errors++;

    Same.

    > goto out;
    > }
    >
    > @@ -270,6 +274,8 @@ int ubifs_check_node(const struct ubifs_info *c, const void
    > *buf, int len,
    > if (!quiet)
    > ubifs_err(c, "bad CRC: calculated %#08x, read %#08x",
    > crc, node_crc);
    > + if (c->stats)
    > + c->stats->crc_errors++;

    Same.

    > err = -EUCLEAN;
    > goto out;
    > }
    > diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c
    > index f0fb25727d96..50b934854a84 100644
    > --- a/fs/ubifs/super.c
    > +++ b/fs/ubifs/super.c
    > @@ -24,6 +24,7 @@
    > #include <linux/mount.h>
    > #include <linux/math64.h>
    > #include <linux/writeback.h>
    > +#include "sysfs.h"
    > #include "ubifs.h"
    >
    > static int ubifs_default_version_set(const char *val, const struct kernel_param
    > *kp)
    > @@ -1264,6 +1265,10 @@ static int mount_ubifs(struct ubifs_info *c)
    > if (err)
    > return err;
    >
    > + err = ubifs_sysfs_register(c);
    > + if (err)
    > + goto out_debugging;
    > +
    > err = check_volume_empty(c);
    > if (err)
    > goto out_free;
    > @@ -1641,6 +1646,8 @@ static int mount_ubifs(struct ubifs_info *c)
    > vfree(c->sbuf);
    > kfree(c->bottom_up_buf);
    > kfree(c->sup_node);
    > + ubifs_sysfs_unregister(c);
    > +out_debugging:
    > ubifs_debugging_exit(c);
    > return err;
    > }
    > @@ -1684,6 +1691,7 @@ static void ubifs_umount(struct ubifs_info *c)
    > kfree(c->bottom_up_buf);
    > kfree(c->sup_node);
    > ubifs_debugging_exit(c);
    > + ubifs_sysfs_unregister(c);
    > }
    >
    > /**
    > @@ -2436,14 +2444,20 @@ static int __init ubifs_init(void)
    >
    > dbg_debugfs_init();
    >
    > + err = ubifs_sysfs_init();
    > + if (err)
    > + goto out_dbg;
    > +
    > err = register_filesystem(&ubifs_fs_type);
    > if (err) {
    > pr_err("UBIFS error (pid %d): cannot register file system, error %d",
    > current->pid, err);
    > - goto out_dbg;
    > + goto out_sysfs;
    > }
    > return 0;
    >
    > +out_sysfs:
    > + ubifs_sysfs_exit();
    > out_dbg:
    > dbg_debugfs_exit();
    > ubifs_compressors_exit();
    > @@ -2462,6 +2476,7 @@ static void __exit ubifs_exit(void)
    > WARN_ON(atomic_long_read(&ubifs_clean_zn_cnt) != 0);
    >
    > dbg_debugfs_exit();
    > + ubifs_sysfs_exit();
    > ubifs_compressors_exit();
    > unregister_shrinker(&ubifs_shrinker_info);
    >
    > diff --git a/fs/ubifs/sysfs.c b/fs/ubifs/sysfs.c
    > new file mode 100644
    > index 000000000000..bac53a0f0451
    > --- /dev/null
    > +++ b/fs/ubifs/sysfs.c
    > @@ -0,0 +1,187 @@
    > +// SPDX-License-Identifier: GPL-2.0-only
    > +/*
    > + * This file is part of UBIFS.
    > + *
    > + * Copyright (C) 2021 Cisco Systems
    > + *
    > + * Author: Stefan Schaeckeler
    > + */
    > +
    > +
    > +#include <linux/fs.h>
    > +
    > +#include "sysfs.h"
    > +#include "ubifs.h"
    > +
    > +
    > +enum attr_id_t {
    > + attr_errors_magic,
    > + attr_errors_node,
    > + attr_errors_crc,
    > +};
    > +
    > +struct ubifs_attr {
    > + struct attribute attr;
    > + enum attr_id_t attr_id;
    > +};
    > +
    > +#define UBIFS_ATTR(_name, _mode, _id) \
    > +static struct ubifs_attr ubifs_attr_##_name = { \
    > + .attr = {.name = __stringify(_name), .mode = _mode }, \
    > + .attr_id = attr_##_id, \
    > +}
    > +
    > +#define UBIFS_ATTR_FUNC(_name, _mode) UBIFS_ATTR(_name, _mode, _name)
    > +
    > +UBIFS_ATTR_FUNC(errors_magic, 0644);
    > +UBIFS_ATTR_FUNC(errors_crc, 0644);
    > +UBIFS_ATTR_FUNC(errors_node, 0644);
    I'm not sure whether everyone should be allowed to read these stats.
    dmesg is also restricted these days. An unprivileged user should not see the
    errors he can indirectly trigger.

    > +#define ATTR_LIST(name) (&ubifs_attr_##name.attr)
    > +
    > +static struct attribute *ubifs_attrs[] = {
    > + ATTR_LIST(errors_magic),
    > + ATTR_LIST(errors_node),
    > + ATTR_LIST(errors_crc),
    > + NULL,
    > +};
    > +
    > +
    > +static ssize_t ubifs_attr_show(struct kobject *kobj,
    > + struct attribute *attr, char *buf)
    > +{
    > + struct ubifs_info *sbi = container_of(kobj, struct ubifs_info,
    > + kobj);
    > +
    > + struct ubifs_attr *a = container_of(attr, struct ubifs_attr, attr);
    > +
    > + switch (a->attr_id) {
    > + case attr_errors_magic:
    > + return snprintf(buf, PAGE_SIZE, "%u\n",
    > + sbi->stats->magic_errors);
    > + case attr_errors_node:
    > + return snprintf(buf, PAGE_SIZE, "%u\n",
    > + sbi->stats->node_errors);
    > + case attr_errors_crc:
    > + return snprintf(buf, PAGE_SIZE, "%u\n",
    > + sbi->stats->crc_errors);

    Please use sysfs_emit().

    > + }
    > + return 0;
    > +};
    > +
    > +
    > +static ssize_t ubifs_attr_store(struct kobject *kobj,
    > + struct attribute *attr,
    > + const char *buf, size_t len)
    > +{
    > + struct ubifs_info *sbi = container_of(kobj, struct ubifs_info,
    > + kobj);
    > +
    > + struct ubifs_attr *a = container_of(attr, struct ubifs_attr, attr);
    > +
    > + switch (a->attr_id) {
    > + case attr_errors_magic:
    > + sbi->stats->magic_errors = 0;
    > + break;
    > + case attr_errors_node:
    > + sbi->stats->node_errors = 0;
    > + break;
    > + case attr_errors_crc:
    > + sbi->stats->crc_errors = 0;
    > + break;
    > + }
    > + return len;
    > +}
    > +
    > +
    > +static void ubifs_sb_release(struct kobject *kobj)
    > +{
    > + struct ubifs_info *c = container_of(kobj, struct ubifs_info, kobj);
    > +
    > + complete(&c->kobj_unregister);
    > +}
    > +
    > +
    > +static const struct sysfs_ops ubifs_attr_ops = {
    > + .show = ubifs_attr_show,
    > + .store = ubifs_attr_store,
    > +};
    > +
    > +static struct kobj_type ubifs_sb_ktype = {
    > + .default_attrs = ubifs_attrs,
    > + .sysfs_ops = &ubifs_attr_ops,
    > + .release = ubifs_sb_release,
    > +};
    > +
    > +static struct kobj_type ubifs_ktype = {
    > + .sysfs_ops = &ubifs_attr_ops,
    > +};
    > +
    > +static struct kset ubifs_kset = {
    > + .kobj = {.ktype = &ubifs_ktype},
    > +};
    > +
    > +
    > +int ubifs_sysfs_register(struct ubifs_info *c)
    > +{
    > + int ret, n;
    > + char dfs_dir_name[UBIFS_DFS_DIR_LEN+1];
    > +
    > + c->stats = kzalloc(sizeof(struct ubifs_stats_info), GFP_KERNEL);
    > + if (!c->stats) {
    > + ret = -ENOMEM;
    > + goto out_last;
    > + }
    > + n = snprintf(dfs_dir_name, UBIFS_DFS_DIR_LEN + 1, UBIFS_DFS_DIR_NAME,
    > + c->vi.ubi_num, c->vi.vol_id);
    > +
    > + if (n == UBIFS_DFS_DIR_LEN) {
    > + /* The array size is too small */
    > + ret = -EINVAL;
    > + goto out_last;

    Where is c->stats released in case of an error?

    > + }
    > +
    > + c->kobj.kset = &ubifs_kset;
    > + init_completion(&c->kobj_unregister);
    > +
    > +
    > + ret = kobject_init_and_add(&c->kobj, &ubifs_sb_ktype, NULL,
    > + "%s", dfs_dir_name);
    > + if (ret)
    > + goto out_put;
    > +
    > + return 0;
    > +
    > +out_put:
    > + kobject_put(&c->kobj);
    > + wait_for_completion(&c->kobj_unregister);
    > +out_last:
    > + ubifs_err(c, "cannot create sysfs entry for ubifs%d_%d, error %d\n",
    > + c->vi.ubi_num, c->vi.vol_id, ret);
    > + return ret;
    > +}
    > +
    > +void ubifs_sysfs_unregister(struct ubifs_info *c)
    > +{
    > + kobject_del(&c->kobj);
    > + kobject_put(&c->kobj);
    > + wait_for_completion(&c->kobj_unregister);
    > +
    > + kfree(c->stats);
    > +}
    > +
    > +int __init ubifs_sysfs_init(void)
    > +{
    > + int ret;
    > +
    > + kobject_set_name(&ubifs_kset.kobj, "ubifs");
    > + ubifs_kset.kobj.parent = fs_kobj;
    > + ret = kset_register(&ubifs_kset);
    > +
    > + return ret;
    > +}
    > +
    > +void ubifs_sysfs_exit(void)
    > +{
    > + kset_unregister(&ubifs_kset);
    > +}
    > diff --git a/fs/ubifs/sysfs.h b/fs/ubifs/sysfs.h
    > new file mode 100644
    > index 000000000000..a10a82585af8
    > --- /dev/null
    > +++ b/fs/ubifs/sysfs.h

    Do we really need a new header file?
    Usually most run-time stuff of UBIFS is part of ubifs.h.

    > @@ -0,0 +1,39 @@
    > +/* SPDX-License-Identifier: GPL-2.0-only */
    > +/*
    > + * This file is part of UBIFS.
    > + *
    > + * Copyright (C) 2021 Cisco Systems
    > + *
    > + * Author: Stefan Schaeckeler
    > + */
    > +
    > +#ifndef __UBIFS_SYSFS_H__
    > +#define __UBIFS_SYSFS_H__
    > +
    > +struct ubifs_info;
    > +
    > +/*
    > + * The UBIFS sysfs directory name pattern and maximum name length (3 for "ubi"
    > + * + 1 for "_" and plus 2x2 for 2 UBI numbers and 1 for the trailing zero byte.
    > + */
    > +#define UBIFS_DFS_DIR_NAME "ubi%d_%d"
    > +#define UBIFS_DFS_DIR_LEN (3 + 1 + 2*2 + 1)
    > +
    > +/**
    > + * ubifs_stats_info - per-FS statistics information.
    > + * @magic_errors: number of bad magic numbers (will be reset with a new mount).
    > + * @node_errors: number of bad nodes (will be reset with a new mount).
    > + * @crc_errors: number of bad crcs (will be reset with a new mount).
    > + */
    > +struct ubifs_stats_info {
    > + unsigned int magic_errors;
    > + unsigned int node_errors;
    > + unsigned int crc_errors;
    > +};
    > +
    > +int ubifs_sysfs_init(void);
    > +void ubifs_sysfs_exit(void);
    > +int ubifs_sysfs_register(struct ubifs_info *c);
    > +void ubifs_sysfs_unregister(struct ubifs_info *c);
    > +
    > +#endif
    > diff --git a/fs/ubifs/ubifs.h b/fs/ubifs/ubifs.h
    > index c38066ce9ab0..bfc0f20b41a1 100644
    > --- a/fs/ubifs/ubifs.h
    > +++ b/fs/ubifs/ubifs.h
    > @@ -27,12 +27,15 @@
    > #include <linux/security.h>
    > #include <linux/xattr.h>
    > #include <linux/random.h>
    > +#include <linux/sysfs.h>
    > +#include <linux/completion.h>
    > #include <crypto/hash_info.h>
    > #include <crypto/hash.h>
    > #include <crypto/algapi.h>
    >
    > #include <linux/fscrypt.h>
    >
    > +#include "sysfs.h"
    > #include "ubifs-media.h"
    >
    > /* Version of this UBIFS implementation */
    > @@ -1251,6 +1254,10 @@ struct ubifs_debug_info;
    > * @mount_opts: UBIFS-specific mount options
    > *
    > * @dbg: debugging-related information
    > + * @stats: statistics exported over sysfs
    > + *
    > + * @kobj: kobject for /sys/fs/ubifs/
    > + * @kobj_unregister: completion to unregister sysfs kobject
    > */
    > struct ubifs_info {
    > struct super_block *vfs_sb;
    > @@ -1286,6 +1293,9 @@ struct ubifs_info {
    > spinlock_t cs_lock;
    > wait_queue_head_t cmt_wq;
    >
    > + struct kobject kobj;
    > + struct completion kobj_unregister;
    > +
    > unsigned int big_lpt:1;
    > unsigned int space_fixup:1;
    > unsigned int double_hash:1;
    > @@ -1493,6 +1503,7 @@ struct ubifs_info {
    > struct ubifs_mount_opts mount_opts;
    >
    > struct ubifs_debug_info *dbg;
    > + struct ubifs_stats_info *stats;
    > };
    >
    > extern struct list_head ubifs_infos;
    > --
    > 2.32.0

    \
     
     \ /
      Last update: 2021-10-03 21:59    [W:4.246 / U:1.092 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site