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

[Vivek Goyal <vgoyal@redhat.com>]

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@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.

Anyway, I do not have strong opinions on this. Given waiting for FORGET
was virtio-fs requirement and not fuse reuiqrement, I thought it was 
more appropriate that virtio-fs takes care of it.

Vivek