On Tue, Jun 07, 2022 at 07:05:04PM +0800, Xie Yongji wrote:
> The commit 15c8e72e88e0 ("fuse: allow skipping control
> interface and forced unmount") tries to remove the control
> interface for virtio-fs since it does not support aborting
> requests which are being processed. But it doesn't work now.
Aha.., so "no_control" basically has no effect? I was looking at
the code and did not find anybody using "no_control" and I was
wondering who is making use of "no_control" variable.
I mounted virtiofs and noticed a directory named "40" showed up
under /sys/fs/fuse/connections/. That must be belonging to
virtiofs instance, I am assuming.
BTW, if there are multiple fuse connections, how will one figure
out which directory belongs to which instance. Because without knowing
that, one will be shooting in dark while trying to read/write any
of the control files.
So I think a separate patch should be sent which just gets rid of
"no_control" saying nobody uses. it.
>
> This commit fixes the bug, but only remove the abort interface
> instead since other interfaces should be useful.
Hmm.., so writing to "abort" file is bad as it ultimately does.
fc->connected = 0;
So getting rid of this file till we support aborting the pending
requests properly, makes sense.
I think this probably should be a separate patch which explains
why adding "no_abort_control" is a good idea.
Thanks
Vivek
>
> Fixes: 15c8e72e88e0 ("fuse: allow skipping control interface and forced unmount")
> Signed-off-by: Xie Yongji <xieyongji@xxxxxxxxxxxxx>
> ---
> fs/fuse/control.c | 4 ++--
> fs/fuse/fuse_i.h | 6 +++---
> fs/fuse/inode.c | 2 +-
> fs/fuse/virtio_fs.c | 2 +-
> 4 files changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/fs/fuse/control.c b/fs/fuse/control.c
> index 7cede9a3bc96..d93d8ea3a090 100644
> --- a/fs/fuse/control.c
> +++ b/fs/fuse/control.c
> @@ -272,8 +272,8 @@ int fuse_ctl_add_conn(struct fuse_conn *fc)
>
> if (!fuse_ctl_add_dentry(parent, fc, "waiting", S_IFREG | 0400, 1,
> NULL, &fuse_ctl_waiting_ops) ||
> - !fuse_ctl_add_dentry(parent, fc, "abort", S_IFREG | 0200, 1,
> - NULL, &fuse_ctl_abort_ops) ||
> + (!fc->no_abort_control && !fuse_ctl_add_dentry(parent, fc, "abort",
> + S_IFREG | 0200, 1, NULL, &fuse_ctl_abort_ops)) ||
> !fuse_ctl_add_dentry(parent, fc, "max_background", S_IFREG | 0600,
> 1, NULL, &fuse_conn_max_background_ops) ||
> !fuse_ctl_add_dentry(parent, fc, "congestion_threshold",
> diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
> index 488b460e046f..e29a4e2f2b35 100644
> --- a/fs/fuse/fuse_i.h
> +++ b/fs/fuse/fuse_i.h
> @@ -507,7 +507,7 @@ struct fuse_fs_context {
> bool default_permissions:1;
> bool allow_other:1;
> bool destroy:1;
> - bool no_control:1;
> + bool no_abort_control:1;
> bool no_force_umount:1;
> bool legacy_opts_show:1;
> enum fuse_dax_mode dax_mode;
> @@ -766,8 +766,8 @@ struct fuse_conn {
> /* Delete dentries that have gone stale */
> unsigned int delete_stale:1;
>
> - /** Do not create entry in fusectl fs */
> - unsigned int no_control:1;
> + /** Do not create abort entry in fusectl fs */
> + unsigned int no_abort_control:1;
>
> /** Do not allow MNT_FORCE umount */
> unsigned int no_force_umount:1;
> diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
> index 8c0665c5dff8..02a16cd35f42 100644
> --- a/fs/fuse/inode.c
> +++ b/fs/fuse/inode.c
> @@ -1564,7 +1564,7 @@ int fuse_fill_super_common(struct super_block *sb, struct fuse_fs_context *ctx)
> fc->legacy_opts_show = ctx->legacy_opts_show;
> fc->max_read = max_t(unsigned int, 4096, ctx->max_read);
> fc->destroy = ctx->destroy;
> - fc->no_control = ctx->no_control;
> + fc->no_abort_control = ctx->no_abort_control;
> fc->no_force_umount = ctx->no_force_umount;
>
> err = -ENOMEM;
> diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c
> index 8db53fa67359..af369bea6dbb 100644
> --- a/fs/fuse/virtio_fs.c
> +++ b/fs/fuse/virtio_fs.c
> @@ -1287,7 +1287,7 @@ static inline void virtio_fs_ctx_set_defaults(struct fuse_fs_context *ctx)
> ctx->max_read = UINT_MAX;
> ctx->blksize = 512;
> ctx->destroy = true;
> - ctx->no_control = true;
> + ctx->no_abort_control = true;
> ctx->no_force_umount = true;
> }
>
> --
> 2.20.1
>
