Vivek Goyal
2022-Jun-08 12:44 UTC
[PATCH] fuse: allow skipping abort interface for virtiofs
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 at 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