Re: [PATCH] fuse: allow skipping abort interface for virtiofs

On Wed, Jun 08, 2022 at 09:57:51PM +0800, Yongji Xie wrote:

On Wed, Jun 8, 2022 at 8:44 PM Vivek Goyal [off-list ref] wrote:

quoted

On Wed, Jun 08, 2022 at 04:42:46PM +0800, Yongji Xie wrote:

quoted

On Wed, Jun 8, 2022 at 3:34 AM Vivek Goyal [off-list ref] wrote:

quoted

On Tue, Jun 07, 2022 at 07:05:04PM +0800, Xie Yongji wrote:

quoted

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.

quoted

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.

quoted

So I think a separate patch should be sent which just gets rid of
"no_control" saying nobody uses. it.

OK.

quoted

quoted

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.

quoted

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.

quoted

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

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization