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:56:40 -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 4/8] vfs: Add superblock notifications
Message-ID: <20201208005640.GB106255@magnolia>
References: <20201208003117.342047-1-krisman@collabora.com>
 <20201208003117.342047-5-krisman@collabora.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20201208003117.342047-5-krisman@collabora.com>
Precedence: bulk

On Mon, Dec 07, 2020 at 09:31:13PM -0300, Gabriel Krisman Bertazi wrote:
> From: David Howells <dhowells@redhat.com>
> 
> Add a superblock event notification facility whereby notifications about
> superblock events, such as I/O errors (EIO), quota limits being hit
> (EDQUOT) and running out of space (ENOSPC) can be reported to a monitoring
> process asynchronously.  Note that this does not cover vfsmount topology
> changes.  watch_mount() is used for that.

<being a lazy reviewer and skipping straight to the data format>

> diff --git a/include/uapi/linux/watch_queue.h b/include/uapi/linux/watch_queue.h
> index c3d8320b5d3a..937363d9f7b3 100644
> --- a/include/uapi/linux/watch_queue.h
> +++ b/include/uapi/linux/watch_queue.h
> @@ -14,7 +14,8 @@
>  enum watch_notification_type {
>  	WATCH_TYPE_META		= 0,	/* Special record */
>  	WATCH_TYPE_KEY_NOTIFY	= 1,	/* Key change event notification */
> -	WATCH_TYPE__NR		= 2
> +	WATCH_TYPE_SB_NOTIFY	= 2,
> +	WATCH_TYPE__NR		= 3
>  };
>  
>  enum watch_meta_notification_subtype {
> @@ -101,4 +102,35 @@ struct key_notification {
>  	__u32	aux;		/* Per-type auxiliary data */
>  };
>  
> +/*
> + * Type of superblock notification.
> + */
> +enum superblock_notification_type {
> +	NOTIFY_SUPERBLOCK_READONLY	= 0, /* Filesystem toggled between R/O and R/W */
> +	NOTIFY_SUPERBLOCK_ERROR		= 1, /* Error in filesystem or blockdev */
> +	NOTIFY_SUPERBLOCK_EDQUOT	= 2, /* EDQUOT notification */
> +	NOTIFY_SUPERBLOCK_NETWORK	= 3, /* Network status change */
> +};
> +
> +#define NOTIFY_SUPERBLOCK_IS_NOW_RO	WATCH_INFO_FLAG_0 /* Superblock changed to R/O */
> +
> +/*
> + * Superblock notification record.
> + * - watch.type = WATCH_TYPE_MOUNT_NOTIFY
> + * - watch.subtype = enum superblock_notification_subtype
> + */
> +struct superblock_notification {
> +	struct watch_notification watch; /* WATCH_TYPE_SB_NOTIFY */
> +	__u64	sb_id;			/* 64-bit superblock ID [fsinfo_ids::f_sb_id] */
> +};
> +
> +struct superblock_error_notification {
> +	struct superblock_notification s; /* subtype = notify_superblock_error */
> +	__u32	error_number;
> +	__u32	error_cookie;
> +	__u64	inode;
> +	__u64	block;

Is this a file offset?  In ... i_blocksize() units?  What about
filesystems that have multiple file offset mapping structures, like xfs?

IOWs can we make a structure that covers enough of the "common"ly
desired features that we don't just end up with the per-fs but then
duplicated everywhere mess that is GETFLAGS/FSGETXATTR?

> +	char	desc[0];

If the end of this is a VLA then I guess we can't add new fields by
bumping the size and hoping userspace notices.  I guess that implies the
need for some padding and a flags field that we can set bits in when we
start using that padding...

--D

> +};
> +
>  #endif /* _UAPI_LINUX_WATCH_QUEUE_H */
> diff --git a/kernel/sys_ni.c b/kernel/sys_ni.c
> index f27ac94d5fa7..3e97984bc4c8 100644
> --- a/kernel/sys_ni.c
> +++ b/kernel/sys_ni.c
> @@ -51,6 +51,9 @@ COND_SYSCALL_COMPAT(io_pgetevents);
>  COND_SYSCALL(io_uring_setup);
>  COND_SYSCALL(io_uring_enter);
>  COND_SYSCALL(io_uring_register);
> +COND_SYSCALL(fsinfo);
> +COND_SYSCALL(watch_mount);
> +COND_SYSCALL(watch_sb);
>  
>  /* fs/xattr.c */
>  
> -- 
> 2.29.2
>