Message-Id: <20211004055758.C52AD899CC4@corona.crabdance.com>
Date:   Sun,  3 Oct 2021 22:57:58 -0700 (PDT)
From:   Stefan Schaeckeler <schaecsn@....net>
To:     richard@....at
CC:     linux-mtd@...ts.infradead.org, linux-kernel@...r.kernel.org,
        sschaeck@...co.com
Subject: 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


> 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?

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.


> > +	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.


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?

- 644 (world-readable) counters to be consistent with ext4?

- keep sysfs.h to be consistent with debugfs?

 Stefan

Powered by blists - more mailing lists