Re: [PATCH 5/8] vfs: Include origin of the SB error notification

"Darrick J. Wong" [off-list ref] writes:

On Tue, Dec 08, 2020 at 04:29:32PM -0300, Gabriel Krisman Bertazi wrote:

quoted

"Darrick J. Wong" [off-list ref] writes:

quoted

On Tue, Dec 08, 2020 at 09:58:25AM -0300, Gabriel Krisman Bertazi wrote:

quoted

David Howells [off-list ref] writes:

quoted

Gabriel Krisman Bertazi [off-list ref] wrote:

quoted

@@ -130,6 +131,8 @@ struct superblock_error_notification {

FWIW I wonder if this really should be inode_error_notification?

If (for example) ext4 discovered an error in the blockgroup descriptor
and wanted to report it, the inode and block numbers would be
irrelevant, but the blockgroup number would be nice to have.

A previous RFC had superblock_error_notification and
superblock_inode_error_notification split, I think we can recover that.

quoted

quoted

quoted

quoted

 	__u32	error_cookie;
 	__u64	inode;
 	__u64	block;
+	char	function[SB_NOTIFICATION_FNAME_LEN];
+	__u16	line;
 	char	desc[0];
 };

As Darrick said, this is a UAPI breaker, so you shouldn't do this (you can,
however, merge this ahead a patch).  Also, I would put the __u16 before the
char[].

That said, I'm not sure whether it's useful to include the function name and
line.  Both fields are liable to change over kernel commits, so it's not
something userspace can actually interpret.  I think you're better off dumping
those into dmesg.

Further, this reduces the capacity of desc[] significantly - I don't know if
that's a problem.

Yes, that is a big problem as desc is already quite limited.  I don't

How limited?

The largest notification is 128 bytes, the one with the biggest header
is superblock_error_notification which leaves 56 bytes for description.

quoted

quoted

think it is a problem for them to change between kernel versions, as the
monitoring userspace can easily associate it with the running kernel.

How do you make that association?  $majordistro's 4.18 kernel is not the
same as the upstream 4.18.  Wouldn't you rather the notification message
be entirely self-describing rather than depending on some external
information about the sender?

True.  I was thinking on my use case where the customer controls their
infrastructure and would specialize their userspace tools, but that is
poor design on my part.  A self describing mechanism would be better.

quoted

quoted

The alternative would be generating something like unique IDs for each
error notification in the filesystem, no?

quoted

And yet further, there's no room for addition of new fields with the desc[]
buffer on the end.  Now maybe you're planning on making use of desc[] for
text-encoding?

Yes.  I would like to be able to provide more details on the error,
without having a unique id.  For instance, desc would have the formatted
string below, describing the warning:

ext4_warning(inode->i_sb, "couldn't mark inode dirty (err %d)", err);

Depending on the upper limit on the length of messages, I wonder if you
could split the superblock notification and the description string into
separate messages (with maybe the error cookie to tie them together) so
that the struct isn't limited by having a VLA on the end, and the
description can be more or less an arbitrary string?

(That said I'm not familiar with the watch queue system so I have no
idea if chained messages even make sense here, or are already
implemented in some other way, or...)

I don't see any support for chaining messages in the current watch_queue
implementation, I'd need to extend the interface to support it.  I
considered this idea before, given the small description size, but I
thought it would be over-complicated, even though much more future
proof.  I will look into that.

What about the kernel exporting a per-filesystem table, as a build
target or in /sys/fs/<fs>/errors, that has descriptions strings for each
error?  Then the notification can have only the FS type, index to the
table and params.  This won't exactly be self-describing as you wanted
but, differently from function:line, it removes the need for the source
code, and allows localization.  The per-filesystem table would be
stable ABI, of course.

Yikes.  I don't think people are going to be ok with a message table
where we can never remove the strings.  I bet GregKH won't like that
either (one value per sysfs file).

Indeed, sysfs seems out of question.  In fact the string format doesn't
even need to be in the kernel, and we don't need the strings to be sent
as part of the notifications.  What if we can have a bunch of
notification types, specific for each error message, and a library in
userspace that parses the notifications and understands the parameters
passed?  The library then displays the data as they wish.

(Maybe I misread that and all you meant by stable ABI is the fact that
the table exists at a given path and the notification message gives you
a index into ... wherever we put it.)

The kernel could even export the table as a build-time target, that
get's installed into X. But even that is not necessary if a library can
make sense of a notification that uniquely identifies each error and
only includes the useful debug parameters without any string formatting?

-- 
Gabriel Krisman Bertazi