Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 8 Dec 2020 10:41:23 -0800
From:   "Darrick J. Wong" <darrick.wong@oracle.com>
To:     Gabriel Krisman Bertazi <krisman@collabora.com>
Cc:     David Howells <dhowells@redhat.com>, viro@zeniv.linux.org.uk,
        tytso@mit.edu, khazhy@google.com, adilger.kernel@dilger.ca,
        linux-ext4@vger.kernel.org, linux-fsdevel@vger.kernel.org,
        kernel@collabora.com
Subject: Re: [PATCH 5/8] vfs: Include origin of the SB error notification
Message-ID: <20201208184123.GC106255@magnolia>
References: <20201208003117.342047-6-krisman@collabora.com>
 <20201208003117.342047-1-krisman@collabora.com>
 <952750.1607431868@warthog.procyon.org.uk>
 <87r1o05ua6.fsf@collabora.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <87r1o05ua6.fsf@collabora.com>
Precedence: bulk

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.

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

> 
> 
> >
> > David
> >
> 
> -- 
> Gabriel Krisman Bertazi