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

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

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.

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?

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?

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

Even better if you could find a way to send the string and the arguments
separately so that whatever's on the receiving end could run it through
a localization catalogue.  Though I remember my roommates trying to
localize 2.0.35 into Latin 25 years ago and never getting very far. :)

--D

quoted

David

-- 
Gabriel Krisman Bertazi