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

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

[Peng Tao]

Right now FORGET requests are not tracked and they might be sent
after DESTROY request. 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.

Signed-off-by: Peng Tao <tao.peng@linux.alibaba.com>
---
 fs/fuse/dev.c       |  8 +++++++-
 fs/fuse/fuse_i.h    |  8 ++++++++
 fs/fuse/virtio_fs.c | 10 ++++++++--
 3 files changed, 23 insertions(+), 3 deletions(-)

diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
index ee9dd38bc0f0..8f5617e07309 100644
--- a/fs/fuse/dev.c
+++ b/fs/fuse/dev.c
@@ -164,7 +164,7 @@ static bool fuse_block_alloc(struct fuse_conn *fc, bool for_background)
 	return !fc->initialized || (for_background && fc->blocked);
 }
 
-static void fuse_drop_waiting(struct fuse_conn *fc)
+void fuse_drop_waiting(struct fuse_conn *fc)
 {
 	/*
 	 * lockess check of fc->connected is okay, because atomic_dec_and_test()
@@ -177,6 +177,7 @@ static void fuse_drop_waiting(struct fuse_conn *fc)
 		wake_up_all(&fc->blocked_waitq);
 	}
 }
+EXPORT_SYMBOL_GPL(fuse_drop_waiting);
 
 static struct fuse_req *__fuse_get_req(struct fuse_conn *fc, unsigned npages,
 				       bool for_background)
@@ -414,6 +415,7 @@ void fuse_queue_forget(struct fuse_conn *fc, struct fuse_forget_link *forget,
 
 	spin_lock(&fiq->waitq.lock);
 	if (fiq->connected) {
+		atomic_inc(&fc->num_waiting);
 		fiq->forget_list_tail->next = forget;
 		fiq->forget_list_tail = forget;
 		fiq->ops->wake_forget_and_unlock(fiq);
@@ -1207,6 +1209,7 @@ static struct fuse_forget_link *dequeue_forget(struct fuse_iqueue *fiq,
 					       unsigned max,
 					       unsigned *countp)
 {
+	struct fuse_conn *fc = get_fuse_iqueue_conn(fiq);
 	struct fuse_forget_link *head = fiq->forget_list_head.next;
 	struct fuse_forget_link **newhead = &head;
 	unsigned count;
@@ -1222,6 +1225,9 @@ static struct fuse_forget_link *dequeue_forget(struct fuse_iqueue *fiq,
 	if (countp != NULL)
 		*countp = count;
 
+	while(count-- > 0)
+		fuse_drop_waiting(fc);
+
 	return head;
 }
 
diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
index 7ac7f9a0b81b..609eafd60c30 100644
--- a/fs/fuse/fuse_i.h
+++ b/fs/fuse/fuse_i.h
@@ -923,6 +923,11 @@ static inline struct fuse_conn *get_fuse_conn(struct inode *inode)
 	return get_fuse_conn_super(inode->i_sb);
 }
 
+static inline struct fuse_conn *get_fuse_iqueue_conn(struct fuse_iqueue *fiq)
+{
+	return container_of(fiq, struct fuse_conn, iq);
+}
+
 static inline struct fuse_inode *get_fuse_inode(struct inode *inode)
 {
 	return container_of(inode, struct fuse_inode, inode);
@@ -975,6 +980,9 @@ struct fuse_forget_link *fuse_alloc_forget(void);
 /* Used by READDIRPLUS */
 void fuse_force_forget(struct file *file, u64 nodeid);
 
+/* Used by FORGET callback */
+void fuse_drop_waiting(struct fuse_conn *fc);
+
 /**
  * Initialize READ or READDIR request
  */
diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c
index 18fc0dca0abc..466b88b944c0 100644
--- a/fs/fuse/virtio_fs.c
+++ b/fs/fuse/virtio_fs.c
@@ -172,6 +172,7 @@ static void virtio_fs_hiprio_done_work(struct work_struct *work)
 {
 	struct virtio_fs_vq *fsvq = container_of(work, struct virtio_fs_vq,
 						 done_work);
+	struct fuse_conn *fc = fsvq->fud->fc;
 	struct virtqueue *vq = fsvq->vq;
 
 	/* Free completed FUSE_FORGET requests */
@@ -182,8 +183,10 @@ static void virtio_fs_hiprio_done_work(struct work_struct *work)
 
 		virtqueue_disable_cb(vq);
 
-		while ((req = virtqueue_get_buf(vq, &len)) != NULL)
+		while ((req = virtqueue_get_buf(vq, &len)) != NULL) {
 			kfree(req);
+			fuse_drop_waiting(fc);
+		}
 	} while (!virtqueue_enable_cb(vq) && likely(!virtqueue_is_broken(vq)));
 	spin_unlock(&fsvq->lock);
 }
@@ -695,7 +698,7 @@ __releases(fiq->waitq.lock)
 	struct virtio_fs_vq *fsvq;
 	bool notify;
 	u64 unique;
-	int ret;
+	int ret = -ENOMEM;
 
 	BUG_ON(!fiq->forget_list_head.next);
 	link = fiq->forget_list_head.next;
@@ -757,6 +760,9 @@ __releases(fiq->waitq.lock)
 	if (notify)
 		virtqueue_notify(vq);
 out:
+	if (ret < 0)
+		fuse_drop_waiting(fsvq->fud->fc);
+
 	kfree(link);
 }
 
-- 
2.17.1

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

[Vivek Goyal]

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. 

If this is a real issue, we probably need some notion of generation 
number (either for the mount or for inode) and ignore forgets if
they don't belong to same generation. 

Given this is per mount, probably some sort of number per init request
should make sense.

Thanks
Vivek


> 
> Signed-off-by: Peng Tao <tao.peng@linux.alibaba.com>
> ---
>  fs/fuse/dev.c       |  8 +++++++-
>  fs/fuse/fuse_i.h    |  8 ++++++++
>  fs/fuse/virtio_fs.c | 10 ++++++++--
>  3 files changed, 23 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
> index ee9dd38bc0f0..8f5617e07309 100644
> --- a/fs/fuse/dev.c
> +++ b/fs/fuse/dev.c
> @@ -164,7 +164,7 @@ static bool fuse_block_alloc(struct fuse_conn *fc, bool for_background)
>  	return !fc->initialized || (for_background && fc->blocked);
>  }
>  
> -static void fuse_drop_waiting(struct fuse_conn *fc)
> +void fuse_drop_waiting(struct fuse_conn *fc)
>  {
>  	/*
>  	 * lockess check of fc->connected is okay, because atomic_dec_and_test()
> @@ -177,6 +177,7 @@ static void fuse_drop_waiting(struct fuse_conn *fc)
>  		wake_up_all(&fc->blocked_waitq);
>  	}
>  }
> +EXPORT_SYMBOL_GPL(fuse_drop_waiting);
>  
>  static struct fuse_req *__fuse_get_req(struct fuse_conn *fc, unsigned npages,
>  				       bool for_background)
> @@ -414,6 +415,7 @@ void fuse_queue_forget(struct fuse_conn *fc, struct fuse_forget_link *forget,
>  
>  	spin_lock(&fiq->waitq.lock);
>  	if (fiq->connected) {
> +		atomic_inc(&fc->num_waiting);
>  		fiq->forget_list_tail->next = forget;
>  		fiq->forget_list_tail = forget;
>  		fiq->ops->wake_forget_and_unlock(fiq);
> @@ -1207,6 +1209,7 @@ static struct fuse_forget_link *dequeue_forget(struct fuse_iqueue *fiq,
>  					       unsigned max,
>  					       unsigned *countp)
>  {
> +	struct fuse_conn *fc = get_fuse_iqueue_conn(fiq);
>  	struct fuse_forget_link *head = fiq->forget_list_head.next;
>  	struct fuse_forget_link **newhead = &head;
>  	unsigned count;
> @@ -1222,6 +1225,9 @@ static struct fuse_forget_link *dequeue_forget(struct fuse_iqueue *fiq,
>  	if (countp != NULL)
>  		*countp = count;
>  
> +	while(count-- > 0)
> +		fuse_drop_waiting(fc);
> +
>  	return head;
>  }
>  
> diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
> index 7ac7f9a0b81b..609eafd60c30 100644
> --- a/fs/fuse/fuse_i.h
> +++ b/fs/fuse/fuse_i.h
> @@ -923,6 +923,11 @@ static inline struct fuse_conn *get_fuse_conn(struct inode *inode)
>  	return get_fuse_conn_super(inode->i_sb);
>  }
>  
> +static inline struct fuse_conn *get_fuse_iqueue_conn(struct fuse_iqueue *fiq)
> +{
> +	return container_of(fiq, struct fuse_conn, iq);
> +}
> +
>  static inline struct fuse_inode *get_fuse_inode(struct inode *inode)
>  {
>  	return container_of(inode, struct fuse_inode, inode);
> @@ -975,6 +980,9 @@ struct fuse_forget_link *fuse_alloc_forget(void);
>  /* Used by READDIRPLUS */
>  void fuse_force_forget(struct file *file, u64 nodeid);
>  
> +/* Used by FORGET callback */
> +void fuse_drop_waiting(struct fuse_conn *fc);
> +
>  /**
>   * Initialize READ or READDIR request
>   */
> diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c
> index 18fc0dca0abc..466b88b944c0 100644
> --- a/fs/fuse/virtio_fs.c
> +++ b/fs/fuse/virtio_fs.c
> @@ -172,6 +172,7 @@ static void virtio_fs_hiprio_done_work(struct work_struct *work)
>  {
>  	struct virtio_fs_vq *fsvq = container_of(work, struct virtio_fs_vq,
>  						 done_work);
> +	struct fuse_conn *fc = fsvq->fud->fc;
>  	struct virtqueue *vq = fsvq->vq;
>  
>  	/* Free completed FUSE_FORGET requests */
> @@ -182,8 +183,10 @@ static void virtio_fs_hiprio_done_work(struct work_struct *work)
>  
>  		virtqueue_disable_cb(vq);
>  
> -		while ((req = virtqueue_get_buf(vq, &len)) != NULL)
> +		while ((req = virtqueue_get_buf(vq, &len)) != NULL) {
>  			kfree(req);
> +			fuse_drop_waiting(fc);
> +		}
>  	} while (!virtqueue_enable_cb(vq) && likely(!virtqueue_is_broken(vq)));
>  	spin_unlock(&fsvq->lock);
>  }
> @@ -695,7 +698,7 @@ __releases(fiq->waitq.lock)
>  	struct virtio_fs_vq *fsvq;
>  	bool notify;
>  	u64 unique;
> -	int ret;
> +	int ret = -ENOMEM;
>  
>  	BUG_ON(!fiq->forget_list_head.next);
>  	link = fiq->forget_list_head.next;
> @@ -757,6 +760,9 @@ __releases(fiq->waitq.lock)
>  	if (notify)
>  		virtqueue_notify(vq);
>  out:
> +	if (ret < 0)
> +		fuse_drop_waiting(fsvq->fud->fc);
> +
>  	kfree(link);
>  }
>  
> -- 
> 2.17.1
> 

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

[Liu Bo]

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?

thanks,
-liubo

> 
> If this is a real issue, we probably need some notion of generation 
> number (either for the mount or for inode) and ignore forgets if
> they don't belong to same generation. 
> 
> Given this is per mount, probably some sort of number per init request
> should make sense.
> 
> Thanks
> Vivek
> 
> 
> > 
> > Signed-off-by: Peng Tao <tao.peng@linux.alibaba.com>
> > ---
> >  fs/fuse/dev.c       |  8 +++++++-
> >  fs/fuse/fuse_i.h    |  8 ++++++++
> >  fs/fuse/virtio_fs.c | 10 ++++++++--
> >  3 files changed, 23 insertions(+), 3 deletions(-)
> > 
> > diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
> > index ee9dd38bc0f0..8f5617e07309 100644
> > --- a/fs/fuse/dev.c
> > +++ b/fs/fuse/dev.c
> > @@ -164,7 +164,7 @@ static bool fuse_block_alloc(struct fuse_conn *fc, bool for_background)
> >  	return !fc->initialized || (for_background && fc->blocked);
> >  }
> >  
> > -static void fuse_drop_waiting(struct fuse_conn *fc)
> > +void fuse_drop_waiting(struct fuse_conn *fc)
> >  {
> >  	/*
> >  	 * lockess check of fc->connected is okay, because atomic_dec_and_test()
> > @@ -177,6 +177,7 @@ static void fuse_drop_waiting(struct fuse_conn *fc)
> >  		wake_up_all(&fc->blocked_waitq);
> >  	}
> >  }
> > +EXPORT_SYMBOL_GPL(fuse_drop_waiting);
> >  
> >  static struct fuse_req *__fuse_get_req(struct fuse_conn *fc, unsigned npages,
> >  				       bool for_background)
> > @@ -414,6 +415,7 @@ void fuse_queue_forget(struct fuse_conn *fc, struct fuse_forget_link *forget,
> >  
> >  	spin_lock(&fiq->waitq.lock);
> >  	if (fiq->connected) {
> > +		atomic_inc(&fc->num_waiting);
> >  		fiq->forget_list_tail->next = forget;
> >  		fiq->forget_list_tail = forget;
> >  		fiq->ops->wake_forget_and_unlock(fiq);
> > @@ -1207,6 +1209,7 @@ static struct fuse_forget_link *dequeue_forget(struct fuse_iqueue *fiq,
> >  					       unsigned max,
> >  					       unsigned *countp)
> >  {
> > +	struct fuse_conn *fc = get_fuse_iqueue_conn(fiq);
> >  	struct fuse_forget_link *head = fiq->forget_list_head.next;
> >  	struct fuse_forget_link **newhead = &head;
> >  	unsigned count;
> > @@ -1222,6 +1225,9 @@ static struct fuse_forget_link *dequeue_forget(struct fuse_iqueue *fiq,
> >  	if (countp != NULL)
> >  		*countp = count;
> >  
> > +	while(count-- > 0)
> > +		fuse_drop_waiting(fc);
> > +
> >  	return head;
> >  }
> >  
> > diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
> > index 7ac7f9a0b81b..609eafd60c30 100644
> > --- a/fs/fuse/fuse_i.h
> > +++ b/fs/fuse/fuse_i.h
> > @@ -923,6 +923,11 @@ static inline struct fuse_conn *get_fuse_conn(struct inode *inode)
> >  	return get_fuse_conn_super(inode->i_sb);
> >  }
> >  
> > +static inline struct fuse_conn *get_fuse_iqueue_conn(struct fuse_iqueue *fiq)
> > +{
> > +	return container_of(fiq, struct fuse_conn, iq);
> > +}
> > +
> >  static inline struct fuse_inode *get_fuse_inode(struct inode *inode)
> >  {
> >  	return container_of(inode, struct fuse_inode, inode);
> > @@ -975,6 +980,9 @@ struct fuse_forget_link *fuse_alloc_forget(void);
> >  /* Used by READDIRPLUS */
> >  void fuse_force_forget(struct file *file, u64 nodeid);
> >  
> > +/* Used by FORGET callback */
> > +void fuse_drop_waiting(struct fuse_conn *fc);
> > +
> >  /**
> >   * Initialize READ or READDIR request
> >   */
> > diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c
> > index 18fc0dca0abc..466b88b944c0 100644
> > --- a/fs/fuse/virtio_fs.c
> > +++ b/fs/fuse/virtio_fs.c
> > @@ -172,6 +172,7 @@ static void virtio_fs_hiprio_done_work(struct work_struct *work)
> >  {
> >  	struct virtio_fs_vq *fsvq = container_of(work, struct virtio_fs_vq,
> >  						 done_work);
> > +	struct fuse_conn *fc = fsvq->fud->fc;
> >  	struct virtqueue *vq = fsvq->vq;
> >  
> >  	/* Free completed FUSE_FORGET requests */
> > @@ -182,8 +183,10 @@ static void virtio_fs_hiprio_done_work(struct work_struct *work)
> >  
> >  		virtqueue_disable_cb(vq);
> >  
> > -		while ((req = virtqueue_get_buf(vq, &len)) != NULL)
> > +		while ((req = virtqueue_get_buf(vq, &len)) != NULL) {
> >  			kfree(req);
> > +			fuse_drop_waiting(fc);
> > +		}
> >  	} while (!virtqueue_enable_cb(vq) && likely(!virtqueue_is_broken(vq)));
> >  	spin_unlock(&fsvq->lock);
> >  }
> > @@ -695,7 +698,7 @@ __releases(fiq->waitq.lock)
> >  	struct virtio_fs_vq *fsvq;
> >  	bool notify;
> >  	u64 unique;
> > -	int ret;
> > +	int ret = -ENOMEM;
> >  
> >  	BUG_ON(!fiq->forget_list_head.next);
> >  	link = fiq->forget_list_head.next;
> > @@ -757,6 +760,9 @@ __releases(fiq->waitq.lock)
> >  	if (notify)
> >  		virtqueue_notify(vq);
> >  out:
> > +	if (ret < 0)
> > +		fuse_drop_waiting(fsvq->fud->fc);
> > +
> >  	kfree(link);
> >  }
> >  
> > -- 
> > 2.17.1
> > 

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

[Vivek Goyal]

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.

Thanks
Vivek

Re: [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

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

[Liu Bo]

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.

thanks,
-liubo
> > 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. 
> 
> If this is a real issue, we probably need some notion of generation 
> number (either for the mount or for inode) and ignore forgets if
> they don't belong to same generation. 
> 
> Given this is per mount, probably some sort of number per init request
> should make sense.
> 
> Thanks
> Vivek
> 
> 
> > 
> > Signed-off-by: Peng Tao <tao.peng@linux.alibaba.com>
> > ---
> >  fs/fuse/dev.c       |  8 +++++++-
> >  fs/fuse/fuse_i.h    |  8 ++++++++
> >  fs/fuse/virtio_fs.c | 10 ++++++++--
> >  3 files changed, 23 insertions(+), 3 deletions(-)
> > 
> > diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
> > index ee9dd38bc0f0..8f5617e07309 100644
> > --- a/fs/fuse/dev.c
> > +++ b/fs/fuse/dev.c
> > @@ -164,7 +164,7 @@ static bool fuse_block_alloc(struct fuse_conn *fc, bool for_background)
> >  	return !fc->initialized || (for_background && fc->blocked);
> >  }
> >  
> > -static void fuse_drop_waiting(struct fuse_conn *fc)
> > +void fuse_drop_waiting(struct fuse_conn *fc)
> >  {
> >  	/*
> >  	 * lockess check of fc->connected is okay, because atomic_dec_and_test()
> > @@ -177,6 +177,7 @@ static void fuse_drop_waiting(struct fuse_conn *fc)
> >  		wake_up_all(&fc->blocked_waitq);
> >  	}
> >  }
> > +EXPORT_SYMBOL_GPL(fuse_drop_waiting);
> >  
> >  static struct fuse_req *__fuse_get_req(struct fuse_conn *fc, unsigned npages,
> >  				       bool for_background)
> > @@ -414,6 +415,7 @@ void fuse_queue_forget(struct fuse_conn *fc, struct fuse_forget_link *forget,
> >  
> >  	spin_lock(&fiq->waitq.lock);
> >  	if (fiq->connected) {
> > +		atomic_inc(&fc->num_waiting);
> >  		fiq->forget_list_tail->next = forget;
> >  		fiq->forget_list_tail = forget;
> >  		fiq->ops->wake_forget_and_unlock(fiq);
> > @@ -1207,6 +1209,7 @@ static struct fuse_forget_link *dequeue_forget(struct fuse_iqueue *fiq,
> >  					       unsigned max,
> >  					       unsigned *countp)
> >  {
> > +	struct fuse_conn *fc = get_fuse_iqueue_conn(fiq);
> >  	struct fuse_forget_link *head = fiq->forget_list_head.next;
> >  	struct fuse_forget_link **newhead = &head;
> >  	unsigned count;
> > @@ -1222,6 +1225,9 @@ static struct fuse_forget_link *dequeue_forget(struct fuse_iqueue *fiq,
> >  	if (countp != NULL)
> >  		*countp = count;
> >  
> > +	while(count-- > 0)
> > +		fuse_drop_waiting(fc);
> > +
> >  	return head;
> >  }
> >  
> > diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
> > index 7ac7f9a0b81b..609eafd60c30 100644
> > --- a/fs/fuse/fuse_i.h
> > +++ b/fs/fuse/fuse_i.h
> > @@ -923,6 +923,11 @@ static inline struct fuse_conn *get_fuse_conn(struct inode *inode)
> >  	return get_fuse_conn_super(inode->i_sb);
> >  }
> >  
> > +static inline struct fuse_conn *get_fuse_iqueue_conn(struct fuse_iqueue *fiq)
> > +{
> > +	return container_of(fiq, struct fuse_conn, iq);
> > +}
> > +
> >  static inline struct fuse_inode *get_fuse_inode(struct inode *inode)
> >  {
> >  	return container_of(inode, struct fuse_inode, inode);
> > @@ -975,6 +980,9 @@ struct fuse_forget_link *fuse_alloc_forget(void);
> >  /* Used by READDIRPLUS */
> >  void fuse_force_forget(struct file *file, u64 nodeid);
> >  
> > +/* Used by FORGET callback */
> > +void fuse_drop_waiting(struct fuse_conn *fc);
> > +
> >  /**
> >   * Initialize READ or READDIR request
> >   */
> > diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c
> > index 18fc0dca0abc..466b88b944c0 100644
> > --- a/fs/fuse/virtio_fs.c
> > +++ b/fs/fuse/virtio_fs.c
> > @@ -172,6 +172,7 @@ static void virtio_fs_hiprio_done_work(struct work_struct *work)
> >  {
> >  	struct virtio_fs_vq *fsvq = container_of(work, struct virtio_fs_vq,
> >  						 done_work);
> > +	struct fuse_conn *fc = fsvq->fud->fc;
> >  	struct virtqueue *vq = fsvq->vq;
> >  
> >  	/* Free completed FUSE_FORGET requests */
> > @@ -182,8 +183,10 @@ static void virtio_fs_hiprio_done_work(struct work_struct *work)
> >  
> >  		virtqueue_disable_cb(vq);
> >  
> > -		while ((req = virtqueue_get_buf(vq, &len)) != NULL)
> > +		while ((req = virtqueue_get_buf(vq, &len)) != NULL) {
> >  			kfree(req);
> > +			fuse_drop_waiting(fc);
> > +		}
> >  	} while (!virtqueue_enable_cb(vq) && likely(!virtqueue_is_broken(vq)));
> >  	spin_unlock(&fsvq->lock);
> >  }
> > @@ -695,7 +698,7 @@ __releases(fiq->waitq.lock)
> >  	struct virtio_fs_vq *fsvq;
> >  	bool notify;
> >  	u64 unique;
> > -	int ret;
> > +	int ret = -ENOMEM;
> >  
> >  	BUG_ON(!fiq->forget_list_head.next);
> >  	link = fiq->forget_list_head.next;
> > @@ -757,6 +760,9 @@ __releases(fiq->waitq.lock)
> >  	if (notify)
> >  		virtqueue_notify(vq);
> >  out:
> > +	if (ret < 0)
> > +		fuse_drop_waiting(fsvq->fud->fc);
> > +
> >  	kfree(link);
> >  }
> >  
> > -- 
> > 2.17.1
> > 

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

[Vivek Goyal]

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.

I also pushed my changes to this branch.

https://github.com/rhvgoyal/linux/commits/flush-forget

Vivek

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

[Liu Bo]

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
>

The 2nd mount & INIT can only work after vq->fud is released, and

virtio_kill_sb
  fuse_kill_sb_anon
  virtio_fs_free_devs
    #release vq->fud

So it's not necessary to have DESTROY to be the very last one, it only
matters whether we wait for inflight FORGETS before setting vq->fud to
NULL.

Peng's patch seems more fit.

thanks,
-liubo

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

[Tao Peng]

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().

And another issue is that now virtiofs tracks FORGET requests while
generic fuse does not. It is better to unify the behavior.

> >
>
> The 2nd mount & INIT can only work after vq->fud is released, and
>
> virtio_kill_sb
>   fuse_kill_sb_anon
>   virtio_fs_free_devs
>     #release vq->fud
>
> So it's not necessary to have DESTROY to be the very last one, it only
> matters whether we wait for inflight FORGETS before setting vq->fud to
> NULL.
Yes, we don't necessarily need to make sure DESTROY is the last
request. We just need to make sure when kill_sb returns, all inflight
requests are cleaned up. And it is exactly what my patch is doing.

Cheers,
Tao
-- 
Into something rich and strange.

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

[Vivek Goyal]

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

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

[Vivek Goyal]

On Fri, May 31, 2019 at 12:53:57PM -0700, Liu Bo 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
> >
> 
> The 2nd mount & INIT can only work after vq->fud is released, and
> 
> virtio_kill_sb
>   fuse_kill_sb_anon
>   virtio_fs_free_devs
>     #release vq->fud
> 
> So it's not necessary to have DESTROY to be the very last one, it only
> matters whether we wait for inflight FORGETS before setting vq->fud to
> NULL.
> 
> Peng's patch seems more fit.

Peng's patch will wait for FORGET requests to complete as well? It might
release vq->fud earlier but that does not matter anyway.

I think key differentiator here is that should generic fuse wait for
FORGET request completion or it should be left to virtio-fs. Given FORGET
command does not expect a reply and genric fuse does not wait for its
completion in general, I chose to limit this behavior in virtio-fs.

Having said that, I am fine with implementing it fuse as well. Miklos's
call.

Vivek

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

[Tao Peng]

On Mon, Jun 3, 2019 at 9:10 PM Vivek Goyal <vgoyal@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@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.