| Message ID | 20220607110504.198-1-xieyongji@bytedance.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fuse: allow skipping abort interface for virtiofs | expand |
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 >
On Wed, Jun 8, 2022 at 3:34 AM Vivek Goyal <vgoyal@redhat.com> wrote: > > 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. > I think so. > 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. > We can use "stat $mountpoint" to get the device minor ID which is the name of the corresponding control directory. > So I think a separate patch should be sent which just gets rid of > "no_control" saying nobody uses. it. > OK. > > > > 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; > Another problem is that it might trigger UAF since virtio_fs_request_complete() doesn't know the requests are aborted. > 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. > OK. Thanks, Yongji
On Wed, Jun 08, 2022 at 04:42:46PM +0800, Yongji Xie wrote: > On Wed, Jun 8, 2022 at 3:34 AM Vivek Goyal <vgoyal@redhat.com> wrote: > > > > 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. > > > > I think so. > > > 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. > > > > We can use "stat $mountpoint" to get the device minor ID which is the > name of the corresponding control directory. > > > So I think a separate patch should be sent which just gets rid of > > "no_control" saying nobody uses. it. > > > > OK. > > > > > > > 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; > > > > Another problem is that it might trigger UAF since > virtio_fs_request_complete() doesn't know the requests are aborted. > > > 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. > > > > OK. BTW, which particular knob you are finding useful in control filesystem for virtiofs. As you mentioned "abort" we will not implement. "waiting" might not have much significance as well because requests are handed over to virtiofs immidiately and if they can be sent to server (because virtqueue is full) these are queued internally and fuse layer will not have an idea. That leaves us with "congestion_threshold" and "max_background". max_background seems to control how many background requests can be submitted at a time. That probably can be useful if server is overwhelemed and we want to slow down the client a bit. Not sure about congestion threshold. So have you found some knob useful for your use case? Thanks Vivek
On Wed, Jun 8, 2022 at 8:44 PM Vivek Goyal <vgoyal@redhat.com> wrote: > > On Wed, Jun 08, 2022 at 04:42:46PM +0800, Yongji Xie wrote: > > On Wed, Jun 8, 2022 at 3:34 AM Vivek Goyal <vgoyal@redhat.com> wrote: > > > > > > 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. > > > > > > > I think so. > > > > > 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. > > > > > > > We can use "stat $mountpoint" to get the device minor ID which is the > > name of the corresponding control directory. > > > > > So I think a separate patch should be sent which just gets rid of > > > "no_control" saying nobody uses. it. > > > > > > > OK. > > > > > > > > > > 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; > > > > > > > Another problem is that it might trigger UAF since > > virtio_fs_request_complete() doesn't know the requests are aborted. > > > > > 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. > > > > > > > OK. > > BTW, which particular knob you are finding useful in control filesystem > for virtiofs. As you mentioned "abort" we will not implement. "waiting" > might not have much significance as well because requests are handed > over to virtiofs immidiately and if they can be sent to server (because > virtqueue is full) these are queued internally and fuse layer will not > have an idea. > Couldn't it be used to check the inflight I/O for virtiofs? > That leaves us with "congestion_threshold" and "max_background". > max_background seems to control how many background requests can be > submitted at a time. That probably can be useful if server is overwhelemed > and we want to slow down the client a bit. > > Not sure about congestion threshold. > > So have you found some knob useful for your use case? > Since it doesn't do harm to the system, I think it would be better to just keep it as it is. Maybe some fuse users can make use of it. Thanks, Yongji
On Wed, Jun 08, 2022 at 09:57:51PM +0800, Yongji Xie wrote: > On Wed, Jun 8, 2022 at 8:44 PM Vivek Goyal <vgoyal@redhat.com> wrote: > > > > On Wed, Jun 08, 2022 at 04:42:46PM +0800, Yongji Xie wrote: > > > On Wed, Jun 8, 2022 at 3:34 AM Vivek Goyal <vgoyal@redhat.com> wrote: > > > > > > > > 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. > > > > > > > > > > I think so. > > > > > > > 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. > > > > > > > > > > We can use "stat $mountpoint" to get the device minor ID which is the > > > name of the corresponding control directory. > > > > > > > So I think a separate patch should be sent which just gets rid of > > > > "no_control" saying nobody uses. it. > > > > > > > > > > OK. > > > > > > > > > > > > > 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; > > > > > > > > > > Another problem is that it might trigger UAF since > > > virtio_fs_request_complete() doesn't know the requests are aborted. > > > > > > > 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. > > > > > > > > > > OK. > > > > BTW, which particular knob you are finding useful in control filesystem > > for virtiofs. As you mentioned "abort" we will not implement. "waiting" > > might not have much significance as well because requests are handed > > over to virtiofs immidiately and if they can be sent to server (because > > virtqueue is full) these are queued internally and fuse layer will not > > have an idea. > > > > Couldn't it be used to check the inflight I/O for virtiofs? Actually I might be wrong. It probably should work. Looking at implementation. fuse_conn_waiting_read() looks at fc->num_waiting to figure out how many requests are in flight. And either fuse_get_req()/fuse_simple_request() will bump up the fc->num_request count and fuse_put_request() will drop that count once request completes. And this seems to be independent of virtiofs. So looks like it should work even with virtiofs. Please give it a try. > > > That leaves us with "congestion_threshold" and "max_background". > > max_background seems to control how many background requests can be > > submitted at a time. That probably can be useful if server is overwhelemed > > and we want to slow down the client a bit. > > > > Not sure about congestion threshold. > > > > So have you found some knob useful for your use case? > > > > Since it doesn't do harm to the system, I think it would be better to > just keep it as it is. Maybe some fuse users can make use of it. I guess fair enough. I don't mind creating "control" file system for virtiofs. Either we don't create "abort" file or may be somehow writing to file returns error. I guess both the solutions should work. Thanks Vivek
On Thu, Jun 9, 2022 at 9:31 PM Vivek Goyal <vgoyal@redhat.com> wrote: > > On Wed, Jun 08, 2022 at 09:57:51PM +0800, Yongji Xie wrote: > > On Wed, Jun 8, 2022 at 8:44 PM Vivek Goyal <vgoyal@redhat.com> wrote: > > > > > > On Wed, Jun 08, 2022 at 04:42:46PM +0800, Yongji Xie wrote: > > > > On Wed, Jun 8, 2022 at 3:34 AM Vivek Goyal <vgoyal@redhat.com> wrote: > > > > > > > > > > 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. > > > > > > > > > > > > > I think so. > > > > > > > > > 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. > > > > > > > > > > > > > We can use "stat $mountpoint" to get the device minor ID which is the > > > > name of the corresponding control directory. > > > > > > > > > So I think a separate patch should be sent which just gets rid of > > > > > "no_control" saying nobody uses. it. > > > > > > > > > > > > > OK. > > > > > > > > > > > > > > > > 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; > > > > > > > > > > > > > Another problem is that it might trigger UAF since > > > > virtio_fs_request_complete() doesn't know the requests are aborted. > > > > > > > > > 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. > > > > > > > > > > > > > OK. > > > > > > BTW, which particular knob you are finding useful in control filesystem > > > for virtiofs. As you mentioned "abort" we will not implement. "waiting" > > > might not have much significance as well because requests are handed > > > over to virtiofs immidiately and if they can be sent to server (because > > > virtqueue is full) these are queued internally and fuse layer will not > > > have an idea. > > > > > > > Couldn't it be used to check the inflight I/O for virtiofs? > > Actually I might be wrong. It probably should work. Looking at > implementation. > > fuse_conn_waiting_read() looks at fc->num_waiting to figure out > how many requests are in flight. > > And either fuse_get_req()/fuse_simple_request() will bump up the > fc->num_request count and fuse_put_request() will drop that count > once request completes. And this seems to be independent of > virtiofs. > > So looks like it should work even with virtiofs. Please give it a try. > OK. > > > > > That leaves us with "congestion_threshold" and "max_background". > > > max_background seems to control how many background requests can be > > > submitted at a time. That probably can be useful if server is overwhelemed > > > and we want to slow down the client a bit. > > > > > > Not sure about congestion threshold. > > > > > > So have you found some knob useful for your use case? > > > > > > > Since it doesn't do harm to the system, I think it would be better to > > just keep it as it is. Maybe some fuse users can make use of it. > > I guess fair enough. I don't mind creating "control" file system for > virtiofs. Either we don't create "abort" file or may be somehow > writing to file returns error. I guess both the solutions should > work. > I think so. Thanks, Yongji
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; }
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. This commit fixes the bug, but only remove the abort interface instead since other interfaces should be useful. 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(-)