Patchwork [v2] ext4: add sysfs entry showing whether the fs contains errors

login
register
mail settings
Submitter Lukas Czerner
Date May 7, 2014, 12:04 p.m.
Message ID <1399464274-16310-1-git-send-email-lczerner@redhat.com>
Download mbox | patch
Permalink /patch/346571/
State New
Headers show

Comments

Lukas Czerner - May 7, 2014, 12:04 p.m.
Currently there is no easy way to tell that the mounted file system
contains errors other than checking for log messages, or reading the
information directly from superblock.

This patch adds new sysfs entry "errors" for each ext4 file
system so user can simply check

cat /sys/fs/ext4/sda/errors

If the file system is not marked as containing errors then the file
returns just 0. Otherwise it would print out the following information:

<error count> first <first_error_time> <first_error_func>:<first_error_line> \
last <last_error_time> <last_error_func>:<last_error_line>

For example:

2 first 1399305407 trigger_test_error:2630 last 1399463224 trigger_test_error:2601

Signed-off-by: Lukas Czerner <lczerner@redhat.com>
---
v2: Change the name of the file to errors and change the output format

 fs/ext4/super.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)
Theodore Ts'o - May 7, 2014, 2:36 p.m.
On Wed, May 07, 2014 at 02:04:34PM +0200, Lukas Czerner wrote:
> 
> cat /sys/fs/ext4/sda/errors
> 
> If the file system is not marked as containing errors then the file
> returns just 0. Otherwise it would print out the following information:
> 
> <error count> first <first_error_time> <first_error_func>:<first_error_line> \
> last <last_error_time> <last_error_func>:<last_error_line>

This goes against the typical way in which information is returned in
sysfs.  Personally, I've always preferred the scheme used by, for
example /proc/acpi/battery/BAT0/info, versus needing to read N
different files in /sys/class/power_supply/BAT0/*, but the argument is
that it's easier for programs to parse information if they are in
separate files.

It's one of the reasons why I've kept /proc/fs/ext4/sda3/mb_groups,
since trying to convert that file over to the Church of Sysfs's style
guidelines was far more work than it was worth.

I'm not actually sure it's that important to be able to expose the
error function and error line number via sysfs or procfs.  If a
process wants a complete record of all of the various errors, then
dmesg or maybe some netlink socket is really the best interface for
getting this information.

For sysfs, I suspect the primary use will be answering the questions:
"is this file system healthy or not", and "when did it first become
unhealthy".  And for questoins like this, the errors_count and
first_error_time and last_error_time is probably the most useful bits
of information to expose.

Cheers,

					- Ted
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lukas Czerner - May 7, 2014, 3:35 p.m.
On Wed, 7 May 2014, Theodore Ts'o wrote:

> Date: Wed, 7 May 2014 10:36:46 -0400
> From: Theodore Ts'o <tytso@mit.edu>
> To: Lukas Czerner <lczerner@redhat.com>
> Cc: linux-ext4@vger.kernel.org
> Subject: Re: [PATCH v2] ext4: add sysfs entry showing whether the fs contains
>     errors
> 
> On Wed, May 07, 2014 at 02:04:34PM +0200, Lukas Czerner wrote:
> > 
> > cat /sys/fs/ext4/sda/errors
> > 
> > If the file system is not marked as containing errors then the file
> > returns just 0. Otherwise it would print out the following information:
> > 
> > <error count> first <first_error_time> <first_error_func>:<first_error_line> \
> > last <last_error_time> <last_error_func>:<last_error_line>
> 
> This goes against the typical way in which information is returned in
> sysfs.  Personally, I've always preferred the scheme used by, for
> example /proc/acpi/battery/BAT0/info, versus needing to read N
> different files in /sys/class/power_supply/BAT0/*, but the argument is
> that it's easier for programs to parse information if they are in
> separate files.

What about /sys/class/power_supply/BAT0/uevent ? It it is easily
parsable and has all the information in
/sys/class/power_supply/BAT0/*

Also something like /sys/block/sda/stat seems to differ from the
rest.

> 
> It's one of the reasons why I've kept /proc/fs/ext4/sda3/mb_groups,
> since trying to convert that file over to the Church of Sysfs's style
> guidelines was far more work than it was worth.

I tried to find sysfs guidelines but I can not see any in
Documentation speaking about the contents of the files.

What are the guidelines then ?

> 
> I'm not actually sure it's that important to be able to expose the
> error function and error line number via sysfs or procfs.  If a
> process wants a complete record of all of the various errors, then
> dmesg or maybe some netlink socket is really the best interface for
> getting this information.

Maybe not important, but it seems useful enough. However we might
want to restrict read permissions to owner only, since it does not
seem like a good idea to expose this information to the world.

> 
> For sysfs, I suspect the primary use will be answering the questions:
> "is this file system healthy or not", and "when did it first become
> unhealthy".  And for questoins like this, the errors_count and
> first_error_time and last_error_time is probably the most useful bits
> of information to expose.

So you're suggesting to have three sysfs files ?

errors_count
first_error_time
last_error_time

-Lukas

> 
> Cheers,
> 
> 					- Ted
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Theodore Ts'o - May 7, 2014, 4:01 p.m.
On Wed, May 07, 2014 at 05:35:54PM +0200, Lukáš Czerner wrote:
> 
> I tried to find sysfs guidelines but I can not see any in
> Documentation speaking about the contents of the files.
> 
> What are the guidelines then ?

Documentation/filesystems/sysfs.txt:

   Attributes can be exported for kobjects in the form of regular
   files in the filesystem. Sysfs forwards file I/O operations to
   methods defined for the attributes, providing a means to read and
   write kernel attributes.

   Attributes should be ASCII text files, preferably with only one value
   per file. It is noted that it may not be efficient to contain only one
   value per file, so it is socially acceptable to express an array of
   values of the same type. 

I don't remember that last sentence; it was apparently added since the
last time I've looked at it.  Originally, the requirement that each
sysfs file (which was supposed to be an kobject attribute) was
required to be a single value.  Now there's an escape hatch for
"efficiency", which is nice....

> So you're suggesting to have three sysfs files ?
> 
> errors_count
> first_error_time
> last_error_time

Yes, that's what I was suggesting.

						- Ted
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lukas Czerner - May 7, 2014, 4:03 p.m.
On Wed, 7 May 2014, Theodore Ts'o wrote:

> Date: Wed, 7 May 2014 12:01:52 -0400
> From: Theodore Ts'o <tytso@mit.edu>
> To: Lukáš Czerner <lczerner@redhat.com>
> Cc: linux-ext4@vger.kernel.org
> Subject: Re: [PATCH v2] ext4: add sysfs entry showing whether the fs contains
>     errors
> 
> On Wed, May 07, 2014 at 05:35:54PM +0200, Lukáš Czerner wrote:
> > 
> > I tried to find sysfs guidelines but I can not see any in
> > Documentation speaking about the contents of the files.
> > 
> > What are the guidelines then ?
> 
> Documentation/filesystems/sysfs.txt:
> 
>    Attributes can be exported for kobjects in the form of regular
>    files in the filesystem. Sysfs forwards file I/O operations to
>    methods defined for the attributes, providing a means to read and
>    write kernel attributes.
> 
>    Attributes should be ASCII text files, preferably with only one value
>    per file. It is noted that it may not be efficient to contain only one
>    value per file, so it is socially acceptable to express an array of
>    values of the same type. 
> 
> I don't remember that last sentence; it was apparently added since the
> last time I've looked at it.  Originally, the requirement that each
> sysfs file (which was supposed to be an kobject attribute) was
> required to be a single value.  Now there's an escape hatch for
> "efficiency", which is nice....

Perfect, thanks!
-Lukas

> 
> > So you're suggesting to have three sysfs files ?
> > 
> > errors_count
> > first_error_time
> > last_error_time
> 
> Yes, that's what I was suggesting.
> 
> 						- Ted
>

Patch

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 6f9e6fa..9d49ec1 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -2502,6 +2502,27 @@  static ssize_t lifetime_write_kbytes_show(struct ext4_attr *a,
 			  EXT4_SB(sb)->s_sectors_written_start) >> 1)));
 }
 
+static ssize_t errors_show(struct ext4_attr *a,
+			   struct ext4_sb_info *sbi, char *buf)
+{
+	struct ext4_super_block *es = sbi->s_es;
+
+	if (es->s_error_count)
+		return snprintf(buf, PAGE_SIZE,
+				"%u first %u %.*s:%d last %u %.*s:%d\n",
+				le32_to_cpu(es->s_error_count),
+				le32_to_cpu(es->s_first_error_time),
+				(int) sizeof(es->s_first_error_func),
+				es->s_first_error_func,
+				le32_to_cpu(es->s_first_error_line),
+				le32_to_cpu(es->s_last_error_time),
+				(int) sizeof(es->s_last_error_func),
+				es->s_last_error_func,
+				le32_to_cpu(es->s_last_error_line));
+	else
+		return snprintf(buf, PAGE_SIZE, "0\n");
+}
+
 static ssize_t inode_readahead_blks_store(struct ext4_attr *a,
 					  struct ext4_sb_info *sbi,
 					  const char *buf, size_t count)
@@ -2617,6 +2638,7 @@  static struct ext4_attr ext4_attr_##_name = {			\
 EXT4_RO_ATTR(delayed_allocation_blocks);
 EXT4_RO_ATTR(session_write_kbytes);
 EXT4_RO_ATTR(lifetime_write_kbytes);
+EXT4_RO_ATTR(errors);
 EXT4_RW_ATTR(reserved_clusters);
 EXT4_ATTR_OFFSET(inode_readahead_blks, 0644, sbi_ui_show,
 		 inode_readahead_blks_store, s_inode_readahead_blks);
@@ -2641,6 +2663,7 @@  static struct attribute *ext4_attrs[] = {
 	ATTR_LIST(delayed_allocation_blocks),
 	ATTR_LIST(session_write_kbytes),
 	ATTR_LIST(lifetime_write_kbytes),
+	ATTR_LIST(errors),
 	ATTR_LIST(reserved_clusters),
 	ATTR_LIST(inode_readahead_blks),
 	ATTR_LIST(inode_goal),