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

[Gabriel Krisman Bertazi <krisman@collabora.com>]

"Darrick J. Wong" <darrick.wong@oracle.com> writes:

> On Tue, Dec 08, 2020 at 09:58:25AM -0300, Gabriel Krisman Bertazi wrote:
>> David Howells <dhowells@redhat.com> writes:
>> 
>> > Gabriel Krisman Bertazi <krisman@collabora.com> wrote:
>> >
>> >> @@ -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.

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

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

>
>> The alternative would be generating something like unique IDs for each
>> error notification in the filesystem, no?
>> 
>> > 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.

-- 
Gabriel Krisman Bertazi