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:   Mon, 7 Dec 2020 16:51:10 -0800
From:   "Darrick J. Wong" <darrick.wong@oracle.com>
To:     Gabriel Krisman Bertazi <krisman@collabora.com>
Cc:     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: <20201208005110.GA106255@magnolia>
References: <20201208003117.342047-1-krisman@collabora.com>
 <20201208003117.342047-6-krisman@collabora.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20201208003117.342047-6-krisman@collabora.com>
Precedence: bulk

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
>