| Message ID | 20201208003117.342047-6-krisman@collabora.com |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | Superblock Notifications | expand |
On Mon, Dec 07, 2020 at 09:31:14PM -0300, Gabriel Krisman Bertazi wrote: > When reporting a filesystem error, we really need to know where the > error came from, therefore, include "function:line" information in the > notification sent to userspace. There is no current users of notify_sb > in the kernel, so there are no callers to update. > > Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.com> > --- > include/linux/fs.h | 11 +++++++++-- > include/uapi/linux/watch_queue.h | 3 +++ > 2 files changed, 12 insertions(+), 2 deletions(-) > > diff --git a/include/linux/fs.h b/include/linux/fs.h > index df588edc0a34..864d86fcc68c 100644 > --- a/include/linux/fs.h > +++ b/include/linux/fs.h > @@ -3514,14 +3514,17 @@ static inline void notify_sb(struct super_block *s, > /** > * notify_sb_error: Post superblock error notification. > * @s: The superblock the notification is about. > + * @function: function name reported as source of the warning. > + * @line: source code line reported as source of the warning. > * @error: The error number to be recorded. > * @inode: The inode the error refers to (if available, 0 otherwise) > * @block: The block the error refers to (if available, 0 otherwise) > * @fmt: Formating string for extra information appended to the notification > * @args: arguments for extra information string appended to the notification > */ > -static inline int notify_sb_error(struct super_block *s, int error, u64 inode, > - u64 block, const char *fmt, va_list *args) > +static inline int notify_sb_error(struct super_block *s, const char *function, int line, > + int error, u64 inode, u64 block, > + const char *fmt, va_list *args) > { > #ifdef CONFIG_SB_NOTIFICATIONS > if (unlikely(s->s_watchers)) { > @@ -3534,8 +3537,12 @@ static inline int notify_sb_error(struct super_block *s, int error, u64 inode, > .error_cookie = 0, > .inode = inode, > .block = block, > + .line = line, > }; > > + memcpy(&n.function, function, SB_NOTIFICATION_FNAME_LEN); > + n.function[SB_NOTIFICATION_FNAME_LEN-1] = '\0'; > + > post_sb_notification(s, &n.s, fmt, args); > } > #endif > diff --git a/include/uapi/linux/watch_queue.h b/include/uapi/linux/watch_queue.h > index 937363d9f7b3..5fa5286c5cc7 100644 > --- a/include/uapi/linux/watch_queue.h > +++ b/include/uapi/linux/watch_queue.h > @@ -114,6 +114,7 @@ enum superblock_notification_type { > > #define NOTIFY_SUPERBLOCK_IS_NOW_RO WATCH_INFO_FLAG_0 /* Superblock changed to R/O */ > > +#define SB_NOTIFICATION_FNAME_LEN 30 > /* > * Superblock notification record. > * - watch.type = WATCH_TYPE_MOUNT_NOTIFY > @@ -130,6 +131,8 @@ struct superblock_error_notification { > __u32 error_cookie; > __u64 inode; > __u64 block; > + char function[SB_NOTIFICATION_FNAME_LEN]; > + __u16 line; Er... this is enlarging a structure in the userspace ABI, right? Which will break userspace that latched on to the structure definition in the previous patch, and therefore isn't expecting a function name here. If you're gonna put a character string(?) at the end then I guess you have to pre-pad the notification structure so that we can add things later, or.... bump the type code every time you add a field? (Maybe I misread that? But this is include/uapi/...) --D > char desc[0]; > }; > > -- > 2.29.2 >
"Darrick J. Wong" <darrick.wong@oracle.com> writes: > On Mon, Dec 07, 2020 at 09:31:14PM -0300, Gabriel Krisman Bertazi wrote: >> When reporting a filesystem error, we really need to know where the >> error came from, therefore, include "function:line" information in the >> notification sent to userspace. There is no current users of notify_sb >> in the kernel, so there are no callers to update. >> >> Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.com> >> --- >> include/linux/fs.h | 11 +++++++++-- >> include/uapi/linux/watch_queue.h | 3 +++ >> 2 files changed, 12 insertions(+), 2 deletions(-) >> >> diff --git a/include/linux/fs.h b/include/linux/fs.h >> index df588edc0a34..864d86fcc68c 100644 >> --- a/include/linux/fs.h >> +++ b/include/linux/fs.h >> @@ -3514,14 +3514,17 @@ static inline void notify_sb(struct super_block *s, >> /** >> * notify_sb_error: Post superblock error notification. >> * @s: The superblock the notification is about. >> + * @function: function name reported as source of the warning. >> + * @line: source code line reported as source of the warning. >> * @error: The error number to be recorded. >> * @inode: The inode the error refers to (if available, 0 otherwise) >> * @block: The block the error refers to (if available, 0 otherwise) >> * @fmt: Formating string for extra information appended to the notification >> * @args: arguments for extra information string appended to the notification >> */ >> -static inline int notify_sb_error(struct super_block *s, int error, u64 inode, >> - u64 block, const char *fmt, va_list *args) >> +static inline int notify_sb_error(struct super_block *s, const char *function, int line, >> + int error, u64 inode, u64 block, >> + const char *fmt, va_list *args) >> { >> #ifdef CONFIG_SB_NOTIFICATIONS >> if (unlikely(s->s_watchers)) { >> @@ -3534,8 +3537,12 @@ static inline int notify_sb_error(struct super_block *s, int error, u64 inode, >> .error_cookie = 0, >> .inode = inode, >> .block = block, >> + .line = line, >> }; >> >> + memcpy(&n.function, function, SB_NOTIFICATION_FNAME_LEN); >> + n.function[SB_NOTIFICATION_FNAME_LEN-1] = '\0'; >> + >> post_sb_notification(s, &n.s, fmt, args); >> } >> #endif >> diff --git a/include/uapi/linux/watch_queue.h b/include/uapi/linux/watch_queue.h >> index 937363d9f7b3..5fa5286c5cc7 100644 >> --- a/include/uapi/linux/watch_queue.h >> +++ b/include/uapi/linux/watch_queue.h >> @@ -114,6 +114,7 @@ enum superblock_notification_type { >> >> #define NOTIFY_SUPERBLOCK_IS_NOW_RO WATCH_INFO_FLAG_0 /* Superblock changed to R/O */ >> >> +#define SB_NOTIFICATION_FNAME_LEN 30 >> /* >> * Superblock notification record. >> * - watch.type = WATCH_TYPE_MOUNT_NOTIFY >> @@ -130,6 +131,8 @@ struct superblock_error_notification { >> __u32 error_cookie; >> __u64 inode; >> __u64 block; >> + char function[SB_NOTIFICATION_FNAME_LEN]; >> + __u16 line; > > Er... this is enlarging a structure in the userspace ABI, right? Which > will break userspace that latched on to the structure definition in the > previous patch, and therefore isn't expecting a function name here. Hi Darrick, Since the structure is defined in the patch immediately before, I thought it would be ok to split the patch to preserve authorship of the different parts. I will fold this into patch 4 in the next iteration. > > If you're gonna put a character string(?) at the end then I guess you > have to pre-pad the notification structure so that we can add things > later, or.... bump the type code every time you add a field? > > (Maybe I misread that? But this is include/uapi/...) > > --D > >> char desc[0]; >> }; >> >> -- >> 2.29.2 >>
Gabriel Krisman Bertazi <krisman@collabora.com> wrote: > Since the structure is defined in the patch immediately before, I > thought it would be ok to split the patch to preserve authorship of the > different parts. Think git bisect. David
Gabriel Krisman Bertazi <krisman@collabora.com> wrote: > @@ -130,6 +131,8 @@ struct superblock_error_notification { > __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. 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? David
David Howells <dhowells@redhat.com> writes: > Gabriel Krisman Bertazi <krisman@collabora.com> wrote: > >> @@ -130,6 +131,8 @@ struct superblock_error_notification { >> __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 think it is a problem for them to change between kernel versions, as the monitoring userspace can easily associate it with the running kernel. 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); > > David >
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
"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.
On Tue, Dec 08, 2020 at 04:29:32PM -0300, Gabriel Krisman Bertazi wrote: > "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. 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). (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.) --D > > -- > Gabriel Krisman Bertazi
"Darrick J. Wong" <darrick.wong@oracle.com> writes: > On Tue, Dec 08, 2020 at 04:29:32PM -0300, Gabriel Krisman Bertazi wrote: >> "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. > > 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?
On Wed, Dec 09, 2020 at 10:06:07AM -0300, Gabriel Krisman Bertazi wrote: > "Darrick J. Wong" <darrick.wong@oracle.com> writes: > > > On Tue, Dec 08, 2020 at 04:29:32PM -0300, Gabriel Krisman Bertazi wrote: > >> "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. > > > > 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. Er... I don't think we (XFS) really are going to maintain a userspace library to decode kernel messages. > > (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? /me shrugs and thinks that chaining fs notifications via cookie might be a better way to do this, since then you could push out a stream of notices about a filesystem event: (0) generic vfs error telling you something happened at a inode/offset (1) fs-specific notice giving more details about what that fs thinks is wrong (2) formatted message string so that you can send it to the sysadmin or feed it to google translate or whatever :) --D > -- > Gabriel Krisman Bertazi
diff --git a/include/linux/fs.h b/include/linux/fs.h index df588edc0a34..864d86fcc68c 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -3514,14 +3514,17 @@ static inline void notify_sb(struct super_block *s, /** * notify_sb_error: Post superblock error notification. * @s: The superblock the notification is about. + * @function: function name reported as source of the warning. + * @line: source code line reported as source of the warning. * @error: The error number to be recorded. * @inode: The inode the error refers to (if available, 0 otherwise) * @block: The block the error refers to (if available, 0 otherwise) * @fmt: Formating string for extra information appended to the notification * @args: arguments for extra information string appended to the notification */ -static inline int notify_sb_error(struct super_block *s, int error, u64 inode, - u64 block, const char *fmt, va_list *args) +static inline int notify_sb_error(struct super_block *s, const char *function, int line, + int error, u64 inode, u64 block, + const char *fmt, va_list *args) { #ifdef CONFIG_SB_NOTIFICATIONS if (unlikely(s->s_watchers)) { @@ -3534,8 +3537,12 @@ static inline int notify_sb_error(struct super_block *s, int error, u64 inode, .error_cookie = 0, .inode = inode, .block = block, + .line = line, }; + memcpy(&n.function, function, SB_NOTIFICATION_FNAME_LEN); + n.function[SB_NOTIFICATION_FNAME_LEN-1] = '\0'; + post_sb_notification(s, &n.s, fmt, args); } #endif diff --git a/include/uapi/linux/watch_queue.h b/include/uapi/linux/watch_queue.h index 937363d9f7b3..5fa5286c5cc7 100644 --- a/include/uapi/linux/watch_queue.h +++ b/include/uapi/linux/watch_queue.h @@ -114,6 +114,7 @@ enum superblock_notification_type { #define NOTIFY_SUPERBLOCK_IS_NOW_RO WATCH_INFO_FLAG_0 /* Superblock changed to R/O */ +#define SB_NOTIFICATION_FNAME_LEN 30 /* * Superblock notification record. * - watch.type = WATCH_TYPE_MOUNT_NOTIFY @@ -130,6 +131,8 @@ struct superblock_error_notification { __u32 error_cookie; __u64 inode; __u64 block; + char function[SB_NOTIFICATION_FNAME_LEN]; + __u16 line; char desc[0]; };
When reporting a filesystem error, we really need to know where the error came from, therefore, include "function:line" information in the notification sent to userspace. There is no current users of notify_sb in the kernel, so there are no callers to update. Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.com> --- include/linux/fs.h | 11 +++++++++-- include/uapi/linux/watch_queue.h | 3 +++ 2 files changed, 12 insertions(+), 2 deletions(-)