From: Vivek Goyal <vgoyal@redhat.com> To: Xie Yongji <xieyongji@bytedance.com> Cc: linux-fsdevel@vger.kernel.org, zhangjiachen.jaycee@bytedance.com, virtualization@lists.linux-foundation.org, stefanha@redhat.com, miklos@szeredi.hu Subject: Re: [PATCH] fuse: allow skipping abort interface for virtiofs Date: Tue, 7 Jun 2022 15:33:36 -0400 [thread overview] Message-ID: <Yp+oEPGnisNx+Nzo@redhat.com> (raw) In-Reply-To: <20220607110504.198-1-xieyongji@bytedance.com> 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@bytedance.com> > --- > 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 > _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
WARNING: multiple messages have this Message-ID (diff)
From: Vivek Goyal <vgoyal@redhat.com> To: Xie Yongji <xieyongji@bytedance.com> Cc: miklos@szeredi.hu, stefanha@redhat.com, zhangjiachen.jaycee@bytedance.com, linux-fsdevel@vger.kernel.org, virtualization@lists.linux-foundation.org Subject: Re: [PATCH] fuse: allow skipping abort interface for virtiofs Date: Tue, 7 Jun 2022 15:33:36 -0400 [thread overview] Message-ID: <Yp+oEPGnisNx+Nzo@redhat.com> (raw) In-Reply-To: <20220607110504.198-1-xieyongji@bytedance.com> 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@bytedance.com> > --- > 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 >
next prev parent reply other threads:[~2022-06-07 19:34 UTC|newest] Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-06-07 11:05 [PATCH] fuse: allow skipping abort interface for virtiofs Xie Yongji 2022-06-07 19:33 ` Vivek Goyal [this message] 2022-06-07 19:33 ` Vivek Goyal 2022-06-08 8:42 ` Yongji Xie 2022-06-08 12:44 ` Vivek Goyal 2022-06-08 12:44 ` Vivek Goyal 2022-06-08 13:57 ` Yongji Xie 2022-06-09 13:31 ` Vivek Goyal 2022-06-09 13:31 ` Vivek Goyal 2022-06-09 14:19 ` Yongji Xie
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=Yp+oEPGnisNx+Nzo@redhat.com \ --to=vgoyal@redhat.com \ --cc=linux-fsdevel@vger.kernel.org \ --cc=miklos@szeredi.hu \ --cc=stefanha@redhat.com \ --cc=virtualization@lists.linux-foundation.org \ --cc=xieyongji@bytedance.com \ --cc=zhangjiachen.jaycee@bytedance.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.