[Virtio-fs] [PATCH] fuse: refcount FORGET requests

[Liu Bo]

On Fri, May 31, 2019 at 01:44:35PM -0400, Vivek Goyal wrote:
> On Fri, May 31, 2019 at 10:38:22AM -0700, Liu Bo wrote:
> > On Fri, May 31, 2019 at 09:22:28AM -0400, Vivek Goyal wrote:
> > > On Fri, May 31, 2019 at 05:24:03PM +0800, Peng Tao wrote:
> > > > Right now FORGET requests are not tracked and they might be sent
> > > > after DESTROY request.
> > > 
> > > How does that happen?
> > > 
> > > > Normally this is OK, since user space should
> > > > be able to reject them knowing that the file system is already umounted.
> > > > But if the same file system is mounted right again and the file is
> > > > opened again, user space can be confused by the refcount decrement
> > > > carried by the old FORGET requests.
> > > > 
> > > > E.g., it can trigger an assertion in virtiofsd when running xfstests
> > > > generic/129 and generic/130 together:
> > > > 
> > > > unique: 23, opcode: FORGET (2), nodeid: 4, insize: 64, pid: 0
> > > >   forget 4 1 -2
> > > > virtiofsd: contrib/virtiofsd/passthrough_ll.c:1044: unref_inode: Assertion `inode->refcount >= n' failed.
> > > > 
> > > > To avoid confusing user space in such case, refcount FORGET requests so
> > > > that fuse_sb_destroy() waits for all inflight requests before returning.
> > > 
> > > forgets are optional and destroy is supposed to cleanup any pending
> > > state.
> > 
> > Since we've ensured that DESTROY is the last fuse_request sent to userspace
> > daemon, we only need to deal with those FORGET requests sent before DESTROY.
> > 
> > Given that FORGET is sent via HIGHPRI vq, in order to solve the problem, would
> > it make sense to route DESTROY to HIGHPRI vq as well?
> > 
> > Or, we can let lo_init to clear reqs in HIGHPRI vq?
> 
> I am looking at another approach. Given sending forget is optional, I am
> writing a patch to not send forgets after virtio_kill_sb() has been
> called. And also flush any in flight forgets. Something like.
> 
> virtio_kill_sb() {
> 	disconnect fsvq. Do not send any further forgets.
> 	flush in flight and pending forgets;
> 	fuse_kill_sb_anon()
> }
> 
> This should make sure once daemon seems destroy, after that there are
> no more forget requests.

Does it refer to flushing &fsvq->done_work?

If so, flushing inflight only covers those returned FORGETs, doesn't it?

If there are inflight forgets not coming back from daemon, I think we have to
wait for them as well.

thanks,
-liubo