On Wed, Oct 27, 2021 at 08:31:27AM -0400, Jeff Layton wrote:
> Writing a non-zero value to this file will trigger spurious shutdown of
> the client, simulating the effect of receiving a bad mdsmap or fsmap.
>
> Note that this effect cannot be reversed. The only remedy is to unmount.
>
> Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx>
> ---
> fs/ceph/debugfs.c | 28 ++++++++++++++++++++++++++++
> fs/ceph/super.h | 1 +
> 2 files changed, 29 insertions(+)
>
> I used this patch to do some fault injection testing before I proposed
> the patch recently to shut down the mount on receipt of a bad fsmap or
> mdsmap.
>
> Is this something we should consider for mainline kernels?
>
> We could put it behind a new Kconfig option if we're worried about
> footguns in production kernels. Maybe we could call the new file
> "fault_inject", and allow writing a mask value to it? We could roll
> tests for teuthology that use this too.
>
> There are a lot of possibilities.
My opinion is that this is a bit too dangerous to be enabled, even in for
debugfs. So, adding a new kconfig option for this sort of things would
probably make sense.
OTOH, I was wondering if it would be possible to use the in-kernel
fault-injection infrastructure to simulate this sort of things. For
example, for your invalid mdsmap use-case, would you get similar results
by forcing ceph_mdsmap_decode() to return an error? If so, then I guess
the fault-injection would be a better option (using the
ALLOW_ERROR_INJECTION macro).
Anyway, that was my two cents (and also a minor nit bellow).
>
> diff --git a/fs/ceph/debugfs.c b/fs/ceph/debugfs.c
> index 55426514491b..57a72f267f6e 100644
> --- a/fs/ceph/debugfs.c
> +++ b/fs/ceph/debugfs.c
> @@ -401,11 +401,32 @@ DEFINE_SIMPLE_ATTRIBUTE(congestion_kb_fops, congestion_kb_get,
> congestion_kb_set, "%llu\n");
>
>
> +static int client_shutdown_set(void *data, u64 val)
> +{
> + struct ceph_fs_client *fsc = (struct ceph_fs_client *)data;
> +
> + if (val)
> + ceph_umount_begin(fsc->sb);
Function ceph_umount_begin needs to be declared as 'extern'.
Cheers,
--
Luís
> + return 0;
> +}
> +
> +static int client_shutdown_get(void *data, u64 *val)
> +{
> + struct ceph_fs_client *fsc = (struct ceph_fs_client *)data;
> +
> + *val = (u64)(fsc->mount_state == CEPH_MOUNT_SHUTDOWN);
> + return 0;
> +}
> +
> +DEFINE_SIMPLE_ATTRIBUTE(client_shutdown_fops, client_shutdown_get,
> + client_shutdown_set, "%llu\n");
> +
> void ceph_fs_debugfs_cleanup(struct ceph_fs_client *fsc)
> {
> dout("ceph_fs_debugfs_cleanup\n");
> debugfs_remove(fsc->debugfs_bdi);
> debugfs_remove(fsc->debugfs_congestion_kb);
> + debugfs_remove(fsc->debugfs_client_shutdown);
> debugfs_remove(fsc->debugfs_mdsmap);
> debugfs_remove(fsc->debugfs_mds_sessions);
> debugfs_remove(fsc->debugfs_caps);
> @@ -426,6 +447,13 @@ void ceph_fs_debugfs_init(struct ceph_fs_client *fsc)
> fsc,
> &congestion_kb_fops);
>
> + fsc->debugfs_client_shutdown =
> + debugfs_create_file("client_shutdown",
> + 0600,
> + fsc->client->debugfs_dir,
> + fsc,
> + &client_shutdown_fops);
> +
> snprintf(name, sizeof(name), "../../bdi/%s",
> bdi_dev_name(fsc->sb->s_bdi));
> fsc->debugfs_bdi =
> diff --git a/fs/ceph/super.h b/fs/ceph/super.h
> index ed51e04739c4..e5d0ad5c6135 100644
> --- a/fs/ceph/super.h
> +++ b/fs/ceph/super.h
> @@ -135,6 +135,7 @@ struct ceph_fs_client {
> struct dentry *debugfs_status;
> struct dentry *debugfs_mds_sessions;
> struct dentry *debugfs_metrics_dir;
> + struct dentry *debugfs_client_shutdown;
> #endif
>
> #ifdef CONFIG_CEPH_FSCACHE
> --
> 2.31.1
>
