From: Richard Weinberger <richard@nod.at> To: schaecsn <schaecsn@gmx.net> Cc: linux-mtd <linux-mtd@lists.infradead.org>, linux-kernel <linux-kernel@vger.kernel.org>, Stefan Schaeckeler <sschaeck@cisco.com> Subject: Re: [PATCH 1/1] ubifs: ubifs to export filesystem error counters Date: Sat, 9 Oct 2021 22:48:53 +0200 (CEST) [thread overview] Message-ID: <67641638.55077.1633812533457.JavaMail.zimbra@nod.at> (raw) In-Reply-To: <20211004055758.C52AD899CC4@corona.crabdance.com> Stefan, ----- Ursprüngliche Mail ----- > Von: "schaecsn" <schaecsn@gmx.net> > An: "richard" <richard@nod.at> > CC: "linux-mtd" <linux-mtd@lists.infradead.org>, "linux-kernel" <linux-kernel@vger.kernel.org>, "Stefan Schaeckeler" > <sschaeck@cisco.com> > Gesendet: Montag, 4. Oktober 2021 07:57:58 > Betreff: Re: [PATCH 1/1] ubifs: ubifs to export filesystem error counters > Hello Richard, > >> > 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? > > These error counters are not meant for aiding debugging but for userspace to > monitor the sanity of the filesystem. Also ext4 exports error counters via > sysfs - it's probably a good idea to be consistent with them. > > $ dir /sys/fs/ext4/sdb2/*error* > -r--r--r-- 1 root root 4096 Oct 3 13:46 /sys/fs/ext4/sdb2/errors_count > -r--r--r-- 1 root root 4096 Oct 3 13:46 /sys/fs/ext4/sdb2/first_error_block > -r--r--r-- 1 root root 4096 Oct 3 13:46 /sys/fs/ext4/sdb2/first_error_errcode > -r--r--r-- 1 root root 4096 Oct 3 13:46 /sys/fs/ext4/sdb2/first_error_func > -r--r--r-- 1 root root 4096 Oct 3 13:46 /sys/fs/ext4/sdb2/first_error_ino > -r--r--r-- 1 root root 4096 Oct 3 13:46 /sys/fs/ext4/sdb2/first_error_line > -r--r--r-- 1 root root 4096 Oct 3 13:46 /sys/fs/ext4/sdb2/first_error_time > -r--r--r-- 1 root root 4096 Oct 3 13:46 /sys/fs/ext4/sdb2/last_error_block > -r--r--r-- 1 root root 4096 Oct 3 13:46 /sys/fs/ext4/sdb2/last_error_errcode > -r--r--r-- 1 root root 4096 Oct 3 13:46 /sys/fs/ext4/sdb2/last_error_func > -r--r--r-- 1 root root 4096 Oct 3 13:47 /sys/fs/ext4/sdb2/last_error_ino > -r--r--r-- 1 root root 4096 Oct 3 13:46 /sys/fs/ext4/sdb2/last_error_line > -r--r--r-- 1 root root 4096 Oct 3 13:46 /sys/fs/ext4/sdb2/last_error_time > --w------- 1 root root 4096 Oct 3 13:46 /sys/fs/ext4/sdb2/trigger_fs_error ext4 is not the reference design for filesystems in Linux. e.g. btrfs has an ioctl for such stats. > >> sysfs is ABI, so we cannot change much anymore. > > Judging by the filesystem permissions above, ext4 does not seem to allow > resetting error counters. If you worry about unchangable ABIs then we could > play it safe and don't support write callbacks for resetting the error counters > in an initial version of the ubifs sysfs. What do you think? Ack. > When we are at it, in the current code, writing anything into a sysfs node > resets the corresponding counter. One could fine-tune that to set the counter > to whatever non-negative integer is passed. > > >> > + if (c->stats) >> > + c->stats->magic_errors++; >> >> Please wrap this into a helper function. > > Ack. > > >> > + if (c->stats) >> > + c->stats->node_errors++; >> >> Same. > > Ack. > > >> > + if (c->stats) >> > + c->stats->crc_errors++; >> >> Same. > > Ack. > > >> > +#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. > > I don't mind 600, but having error counters world-readable is consistent with > ext4 (see dir above) and so I suggest to keep 644. > Ok. >> > + case attr_errors_crc: >> > + return snprintf(buf, PAGE_SIZE, "%u\n", >> > + sbi->stats->crc_errors); >> >> Please use sysfs_emit(). > > Ack. > > >> > + 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? > > My fault. Will be fixed. > > >> > 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. > > I wanted to be consistent with debugfs where fs/ubifs/debug.c comes with its > own header fs/ubifs/debug.h. debug.h is not just about debugfs. It is about debugging/developing UBIFS. That's why it is kind of special. > > I'll send out a new patch once we agree on all changes. To recap: > > - write callbacks: do we remove them? If not, do we keep them as is or do we > fine-tine them by letting a non-negative integer set the counter? I'd go for read-only first. > - 644 (world-readable) counters to be consistent with ext4? Ack. > - keep sysfs.h to be consistent with debugfs? Please remove sysfs.h. Thanks, //richard
WARNING: multiple messages have this Message-ID (diff)
From: Richard Weinberger <richard@nod.at> To: schaecsn <schaecsn@gmx.net> Cc: linux-mtd <linux-mtd@lists.infradead.org>, linux-kernel <linux-kernel@vger.kernel.org>, Stefan Schaeckeler <sschaeck@cisco.com> Subject: Re: [PATCH 1/1] ubifs: ubifs to export filesystem error counters Date: Sat, 9 Oct 2021 22:48:53 +0200 (CEST) [thread overview] Message-ID: <67641638.55077.1633812533457.JavaMail.zimbra@nod.at> (raw) In-Reply-To: <20211004055758.C52AD899CC4@corona.crabdance.com> Stefan, ----- Ursprüngliche Mail ----- > Von: "schaecsn" <schaecsn@gmx.net> > An: "richard" <richard@nod.at> > CC: "linux-mtd" <linux-mtd@lists.infradead.org>, "linux-kernel" <linux-kernel@vger.kernel.org>, "Stefan Schaeckeler" > <sschaeck@cisco.com> > Gesendet: Montag, 4. Oktober 2021 07:57:58 > Betreff: Re: [PATCH 1/1] ubifs: ubifs to export filesystem error counters > Hello Richard, > >> > 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? > > These error counters are not meant for aiding debugging but for userspace to > monitor the sanity of the filesystem. Also ext4 exports error counters via > sysfs - it's probably a good idea to be consistent with them. > > $ dir /sys/fs/ext4/sdb2/*error* > -r--r--r-- 1 root root 4096 Oct 3 13:46 /sys/fs/ext4/sdb2/errors_count > -r--r--r-- 1 root root 4096 Oct 3 13:46 /sys/fs/ext4/sdb2/first_error_block > -r--r--r-- 1 root root 4096 Oct 3 13:46 /sys/fs/ext4/sdb2/first_error_errcode > -r--r--r-- 1 root root 4096 Oct 3 13:46 /sys/fs/ext4/sdb2/first_error_func > -r--r--r-- 1 root root 4096 Oct 3 13:46 /sys/fs/ext4/sdb2/first_error_ino > -r--r--r-- 1 root root 4096 Oct 3 13:46 /sys/fs/ext4/sdb2/first_error_line > -r--r--r-- 1 root root 4096 Oct 3 13:46 /sys/fs/ext4/sdb2/first_error_time > -r--r--r-- 1 root root 4096 Oct 3 13:46 /sys/fs/ext4/sdb2/last_error_block > -r--r--r-- 1 root root 4096 Oct 3 13:46 /sys/fs/ext4/sdb2/last_error_errcode > -r--r--r-- 1 root root 4096 Oct 3 13:46 /sys/fs/ext4/sdb2/last_error_func > -r--r--r-- 1 root root 4096 Oct 3 13:47 /sys/fs/ext4/sdb2/last_error_ino > -r--r--r-- 1 root root 4096 Oct 3 13:46 /sys/fs/ext4/sdb2/last_error_line > -r--r--r-- 1 root root 4096 Oct 3 13:46 /sys/fs/ext4/sdb2/last_error_time > --w------- 1 root root 4096 Oct 3 13:46 /sys/fs/ext4/sdb2/trigger_fs_error ext4 is not the reference design for filesystems in Linux. e.g. btrfs has an ioctl for such stats. > >> sysfs is ABI, so we cannot change much anymore. > > Judging by the filesystem permissions above, ext4 does not seem to allow > resetting error counters. If you worry about unchangable ABIs then we could > play it safe and don't support write callbacks for resetting the error counters > in an initial version of the ubifs sysfs. What do you think? Ack. > When we are at it, in the current code, writing anything into a sysfs node > resets the corresponding counter. One could fine-tune that to set the counter > to whatever non-negative integer is passed. > > >> > + if (c->stats) >> > + c->stats->magic_errors++; >> >> Please wrap this into a helper function. > > Ack. > > >> > + if (c->stats) >> > + c->stats->node_errors++; >> >> Same. > > Ack. > > >> > + if (c->stats) >> > + c->stats->crc_errors++; >> >> Same. > > Ack. > > >> > +#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. > > I don't mind 600, but having error counters world-readable is consistent with > ext4 (see dir above) and so I suggest to keep 644. > Ok. >> > + case attr_errors_crc: >> > + return snprintf(buf, PAGE_SIZE, "%u\n", >> > + sbi->stats->crc_errors); >> >> Please use sysfs_emit(). > > Ack. > > >> > + 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? > > My fault. Will be fixed. > > >> > 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. > > I wanted to be consistent with debugfs where fs/ubifs/debug.c comes with its > own header fs/ubifs/debug.h. debug.h is not just about debugfs. It is about debugging/developing UBIFS. That's why it is kind of special. > > I'll send out a new patch once we agree on all changes. To recap: > > - write callbacks: do we remove them? If not, do we keep them as is or do we > fine-tine them by letting a non-negative integer set the counter? I'd go for read-only first. > - 644 (world-readable) counters to be consistent with ext4? Ack. > - keep sysfs.h to be consistent with debugfs? Please remove sysfs.h. Thanks, //richard ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/
next prev parent reply other threads:[~2021-10-09 20:49 UTC|newest] Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-09-07 21:40 [PATCH 0/1] ubifs: ubifs to export filesystem error counters Stefan Schaeckeler 2021-09-07 21:40 ` Stefan Schaeckeler 2021-09-07 21:40 ` [PATCH 1/1] " Stefan Schaeckeler 2021-09-07 21:40 ` Stefan Schaeckeler 2021-10-03 19:58 ` Richard Weinberger 2021-10-03 19:58 ` Richard Weinberger 2021-10-04 5:57 ` Stefan Schaeckeler 2021-10-04 5:57 ` Stefan Schaeckeler 2021-10-09 20:48 ` Richard Weinberger [this message] 2021-10-09 20:48 ` Richard Weinberger 2021-09-20 2:48 ` [PATCH 0/1] " Stefan Schaeckeler 2021-09-20 2:48 ` Stefan Schaeckeler 2021-09-20 21:57 ` Richard Weinberger 2021-09-20 21:57 ` Richard Weinberger 2021-10-02 21:12 ` Stefan Schaeckeler 2021-10-02 21:12 ` Stefan Schaeckeler
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=67641638.55077.1633812533457.JavaMail.zimbra@nod.at \ --to=richard@nod.at \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-mtd@lists.infradead.org \ --cc=schaecsn@gmx.net \ --cc=sschaeck@cisco.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.