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

[Tao Peng]

On Mon, Jun 3, 2019 at 9:10 PM Vivek Goyal <vgoyal at redhat.com> wrote:
>
> On Sat, Jun 01, 2019 at 09:40:27AM +0800, Tao Peng wrote:
> > On Sat, Jun 1, 2019 at 3:54 AM Liu Bo <bo.liu at linux.alibaba.com> wrote:
> > >
> > > On Fri, May 31, 2019 at 02:59:07PM -0400, Vivek Goyal wrote:
> > > > On Fri, May 31, 2019 at 11:44:55AM -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?
> > > > > >
> > > > >
> > > > > A bit more details, it is those FORGETs that remain in HIGHPRI vq,
> > > > > sent before DESTROY but not yet processed by daemon before DESTROY
> > > > > gets back to guest, then they get processed after the 2nd INIT.
> > > >
> > > > I just posted 3 patches to make sure forget request is not sent after
> > > > destroy. Can you give it a try and see if it solves the problem.
> > > >
> > >
> > > Sorry, somehow this can only be reproduced on Peng Tao's setup.
> > >
> > > > I also pushed my changes to this branch.
> > > >
> > > > https://github.com/rhvgoyal/linux/commits/flush-forget
> > Hi Vivek,
> >
> > I looked at your patchset and I think the main issue is that there is
> > already fc->num_waiting to track inflight requests. Yet you added
> > virtio_fs_vq->in_flight to track vq requests and it is only used by
> > the hiprio vq. You may think that it is only to track forget requests,
> > but why inventing a specific wheel when there is a general one? The
> > maintenance APIs for fc->num_waiting are already there and we don't
> > need to re-implement the busy-waiting logic as is done in
> > virtio_fs_flush_hiprio_queue(). The generic fuse code uses a waitqueue
> > for that in fuse_wait_aborted().
>
> I did the wait in virtio-fs for two reasons.
>
> - Waiting for FORGET is need of virtio-fs and fuse never waited for
>   completion of FORGET requests. So if generic fuse does not care
>   about FORGET completion, it makes sense that virtio-fs takes care
>   of it.
>
> - FUSE currently aborts existing requests if these are still on
>   internal list and have not been sent to user space. In case of virtio-fs
>   FORGET request is handed over to virtio-fs which might keep it on
>   internal list (because virtqueue is full) and there is no way to abort
>   it. Not that aborting forget is a must. So in that respect also it
>   did not gel well with generic fuse logic.
>
Yeah, this is actually a very good point and my patch is missing it
(the internal hiprio list). I'm fine with using you version as it is
virtio-fs self-contained and the less virtio-fs modifies generic fuse
logic, the less it will be questioned when it comes to go upstream.
I've tested your patchset for more than 10 times and it all passed.

Cheers,
Tao
-- 
Into something rich and strange.