linux-fsdevel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 00/18] virtiofs: Fix various races and cleanups round 1
@ 2019-09-05 19:48 Vivek Goyal
  2019-09-05 19:48 ` [PATCH 01/18] virtiofs: Remove request from processing list before calling end Vivek Goyal
                   ` (19 more replies)
  0 siblings, 20 replies; 62+ messages in thread
From: Vivek Goyal @ 2019-09-05 19:48 UTC (permalink / raw)
  To: linux-fsdevel, virtualization, miklos
  Cc: linux-kernel, virtio-fs, vgoyal, stefanha, dgilbert, mst

Hi,

Michael Tsirkin pointed out issues w.r.t various locking related TODO
items and races w.r.t device removal. 

In this first round of cleanups, I have taken care of most pressing
issues.

These patches apply on top of following.

git://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/fuse.git#virtiofs-v4

I have tested these patches with mount/umount and device removal using
qemu monitor. For example.

virsh qemu-monitor-command --hmp vm9-f28 device_del myvirtiofs

Now a mounted device can be removed and file system will return errors
-ENOTCONN and one can unmount it.

Miklos, if you are fine with the patches, I am fine if you fold these
all into existing patch. I kept them separate so that review is easier.

Any feedback or comments are welcome.

Thanks
Vivek


Vivek Goyal (18):
  virtiofs: Remove request from processing list before calling end
  virtiofs: Check whether hiprio queue is connected at submission time
  virtiofs: Pass fsvq instead of vq as parameter to
    virtio_fs_enqueue_req
  virtiofs: Check connected state for VQ_REQUEST queue as well
  Maintain count of in flight requests for VQ_REQUEST queue
  virtiofs: ->remove should not clean virtiofs fuse devices
  virtiofs: Stop virtiofs queues when device is being removed
  virtiofs: Drain all pending requests during ->remove time
  virtiofs: Add an helper to start all the queues
  virtiofs: Do not use device managed mem for virtio_fs and virtio_fs_vq
  virtiofs: stop and drain queues after sending DESTROY
  virtiofs: Use virtio_fs_free_devs() in error path
  virtiofs: Do not access virtqueue in request submission path
  virtiofs: Add a fuse_iqueue operation to put() reference
  virtiofs: Make virtio_fs object refcounted
  virtiofs: Use virtio_fs_mutex for races w.r.t ->remove and mount path
  virtiofs: Remove TODO to quiesce/end_requests
  virtiofs: Remove TODO item from virtio_fs_free_devs()

 fs/fuse/fuse_i.h    |   5 +
 fs/fuse/inode.c     |   6 +
 fs/fuse/virtio_fs.c | 259 ++++++++++++++++++++++++++++++++------------
 3 files changed, 198 insertions(+), 72 deletions(-)

-- 
2.20.1


^ permalink raw reply	[flat|nested] 62+ messages in thread

* [PATCH 01/18] virtiofs: Remove request from processing list before calling end
  2019-09-05 19:48 [PATCH 00/18] virtiofs: Fix various races and cleanups round 1 Vivek Goyal
@ 2019-09-05 19:48 ` Vivek Goyal
  2019-09-06 10:40   ` Stefan Hajnoczi
  2019-09-05 19:48 ` [PATCH 02/18] virtiofs: Check whether hiprio queue is connected at submission time Vivek Goyal
                   ` (18 subsequent siblings)
  19 siblings, 1 reply; 62+ messages in thread
From: Vivek Goyal @ 2019-09-05 19:48 UTC (permalink / raw)
  To: linux-fsdevel, virtualization, miklos
  Cc: linux-kernel, virtio-fs, vgoyal, stefanha, dgilbert, mst

In error path we are calling fuse_request_end() but we need to clear
FR_SENT bit as well as remove request from processing queue. Otherwise
fuse_request_end() triggers a warning as well as other issues show up.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 fs/fuse/virtio_fs.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c
index 197e79e536f9..a708ccb65662 100644
--- a/fs/fuse/virtio_fs.c
+++ b/fs/fuse/virtio_fs.c
@@ -826,6 +826,10 @@ __releases(fiq->waitq.lock)
 		}
 		req->out.h.error = ret;
 		pr_err("virtio-fs: virtio_fs_enqueue_req() failed %d\n", ret);
+		spin_lock(&fpq->lock);
+		clear_bit(FR_SENT, &req->flags);
+		list_del_init(&req->list);
+		spin_unlock(&fpq->lock);
 		fuse_request_end(fc, req);
 		return;
 	}
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 62+ messages in thread

* [PATCH 02/18] virtiofs: Check whether hiprio queue is connected at submission time
  2019-09-05 19:48 [PATCH 00/18] virtiofs: Fix various races and cleanups round 1 Vivek Goyal
  2019-09-05 19:48 ` [PATCH 01/18] virtiofs: Remove request from processing list before calling end Vivek Goyal
@ 2019-09-05 19:48 ` Vivek Goyal
  2019-09-06 10:43   ` Stefan Hajnoczi
  2019-09-05 19:48 ` [PATCH 03/18] virtiofs: Pass fsvq instead of vq as parameter to virtio_fs_enqueue_req Vivek Goyal
                   ` (17 subsequent siblings)
  19 siblings, 1 reply; 62+ messages in thread
From: Vivek Goyal @ 2019-09-05 19:48 UTC (permalink / raw)
  To: linux-fsdevel, virtualization, miklos
  Cc: linux-kernel, virtio-fs, vgoyal, stefanha, dgilbert, mst

For hiprio queue (forget requests), we are keeping a state in queue whether
queue is connected or not. If queue is not connected, do not try to submit
request and return error instead.

As of now, we are checking for this state only in path where worker tries
to submit forget after first attempt failed. Check this state even in
the path when request is being submitted first time.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 fs/fuse/virtio_fs.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c
index a708ccb65662..e9497b565dd8 100644
--- a/fs/fuse/virtio_fs.c
+++ b/fs/fuse/virtio_fs.c
@@ -577,9 +577,16 @@ __releases(fiq->waitq.lock)
 	sg_init_one(&sg, forget, sizeof(*forget));
 
 	/* Enqueue the request */
+	spin_lock(&fsvq->lock);
+
+	if (!fsvq->connected) {
+		kfree(forget);
+		spin_unlock(&fsvq->lock);
+		goto out;
+	}
+
 	vq = fsvq->vq;
 	dev_dbg(&vq->vdev->dev, "%s\n", __func__);
-	spin_lock(&fsvq->lock);
 
 	ret = virtqueue_add_sgs(vq, sgs, 1, 0, forget, GFP_ATOMIC);
 	if (ret < 0) {
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 62+ messages in thread

* [PATCH 03/18] virtiofs: Pass fsvq instead of vq as parameter to virtio_fs_enqueue_req
  2019-09-05 19:48 [PATCH 00/18] virtiofs: Fix various races and cleanups round 1 Vivek Goyal
  2019-09-05 19:48 ` [PATCH 01/18] virtiofs: Remove request from processing list before calling end Vivek Goyal
  2019-09-05 19:48 ` [PATCH 02/18] virtiofs: Check whether hiprio queue is connected at submission time Vivek Goyal
@ 2019-09-05 19:48 ` Vivek Goyal
  2019-09-06 10:44   ` Stefan Hajnoczi
  2019-09-05 19:48 ` [PATCH 04/18] virtiofs: Check connected state for VQ_REQUEST queue as well Vivek Goyal
                   ` (16 subsequent siblings)
  19 siblings, 1 reply; 62+ messages in thread
From: Vivek Goyal @ 2019-09-05 19:48 UTC (permalink / raw)
  To: linux-fsdevel, virtualization, miklos
  Cc: linux-kernel, virtio-fs, vgoyal, stefanha, dgilbert, mst

Pass fsvq instead of vq as parameter to virtio_fs_enqueue_req(). We will
retrieve vq from fsvq under spin lock.

Later in the patch series we will retrieve vq only if fsvq is still connected
other vq might have been cleaned up by device ->remove code and we will
return error.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 fs/fuse/virtio_fs.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c
index e9497b565dd8..9d30530e3ca9 100644
--- a/fs/fuse/virtio_fs.c
+++ b/fs/fuse/virtio_fs.c
@@ -698,14 +698,15 @@ static unsigned int sg_init_fuse_args(struct scatterlist *sg,
 }
 
 /* Add a request to a virtqueue and kick the device */
-static int virtio_fs_enqueue_req(struct virtqueue *vq, struct fuse_req *req)
+static int virtio_fs_enqueue_req(struct virtio_fs_vq *fsvq,
+				 struct fuse_req *req)
 {
 	/* requests need at least 4 elements */
 	struct scatterlist *stack_sgs[6];
 	struct scatterlist stack_sg[ARRAY_SIZE(stack_sgs)];
 	struct scatterlist **sgs = stack_sgs;
 	struct scatterlist *sg = stack_sg;
-	struct virtio_fs_vq *fsvq;
+	struct virtqueue *vq;
 	unsigned int argbuf_used = 0;
 	unsigned int out_sgs = 0;
 	unsigned int in_sgs = 0;
@@ -752,9 +753,9 @@ static int virtio_fs_enqueue_req(struct virtqueue *vq, struct fuse_req *req)
 	for (i = 0; i < total_sgs; i++)
 		sgs[i] = &sg[i];
 
-	fsvq = vq_to_fsvq(vq);
 	spin_lock(&fsvq->lock);
 
+	vq = fsvq->vq;
 	ret = virtqueue_add_sgs(vq, sgs, out_sgs, in_sgs, req, GFP_ATOMIC);
 	if (ret < 0) {
 		/* TODO handle full virtqueue */
@@ -824,7 +825,7 @@ __releases(fiq->waitq.lock)
 	/* TODO check for FR_INTERRUPTED? */
 
 retry:
-	ret = virtio_fs_enqueue_req(fs->vqs[queue_id].vq, req);
+	ret = virtio_fs_enqueue_req(&fs->vqs[queue_id], req);
 	if (ret < 0) {
 		if (ret == -ENOMEM || ret == -ENOSPC) {
 			/* Virtqueue full. Retry submission */
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 62+ messages in thread

* [PATCH 04/18] virtiofs: Check connected state for VQ_REQUEST queue as well
  2019-09-05 19:48 [PATCH 00/18] virtiofs: Fix various races and cleanups round 1 Vivek Goyal
                   ` (2 preceding siblings ...)
  2019-09-05 19:48 ` [PATCH 03/18] virtiofs: Pass fsvq instead of vq as parameter to virtio_fs_enqueue_req Vivek Goyal
@ 2019-09-05 19:48 ` Vivek Goyal
  2019-09-06 10:45   ` Stefan Hajnoczi
  2019-09-05 19:48 ` [PATCH 05/18] Maintain count of in flight requests for VQ_REQUEST queue Vivek Goyal
                   ` (15 subsequent siblings)
  19 siblings, 1 reply; 62+ messages in thread
From: Vivek Goyal @ 2019-09-05 19:48 UTC (permalink / raw)
  To: linux-fsdevel, virtualization, miklos
  Cc: linux-kernel, virtio-fs, vgoyal, stefanha, dgilbert, mst

Right now we are checking ->connected state only for VQ_HIPRIO. Now we want
to make use of this method for all queues. So check it for VQ_REQUEST as
well.

This will be helpful if device has been removed and virtqueue is gone. In
that case ->connected will be false and request can't be submitted anymore
and user space will see error -ENOTCONN.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 fs/fuse/virtio_fs.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c
index 9d30530e3ca9..c46dd4d284d6 100644
--- a/fs/fuse/virtio_fs.c
+++ b/fs/fuse/virtio_fs.c
@@ -755,6 +755,12 @@ static int virtio_fs_enqueue_req(struct virtio_fs_vq *fsvq,
 
 	spin_lock(&fsvq->lock);
 
+	if (!fsvq->connected) {
+		spin_unlock(&fsvq->lock);
+		ret = -ENOTCONN;
+		goto out;
+	}
+
 	vq = fsvq->vq;
 	ret = virtqueue_add_sgs(vq, sgs, out_sgs, in_sgs, req, GFP_ATOMIC);
 	if (ret < 0) {
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 62+ messages in thread

* [PATCH 05/18] Maintain count of in flight requests for VQ_REQUEST queue
  2019-09-05 19:48 [PATCH 00/18] virtiofs: Fix various races and cleanups round 1 Vivek Goyal
                   ` (3 preceding siblings ...)
  2019-09-05 19:48 ` [PATCH 04/18] virtiofs: Check connected state for VQ_REQUEST queue as well Vivek Goyal
@ 2019-09-05 19:48 ` Vivek Goyal
  2019-09-06 10:46   ` Stefan Hajnoczi
  2019-09-05 19:48 ` [PATCH 06/18] virtiofs: ->remove should not clean virtiofs fuse devices Vivek Goyal
                   ` (14 subsequent siblings)
  19 siblings, 1 reply; 62+ messages in thread
From: Vivek Goyal @ 2019-09-05 19:48 UTC (permalink / raw)
  To: linux-fsdevel, virtualization, miklos
  Cc: linux-kernel, virtio-fs, vgoyal, stefanha, dgilbert, mst

As of now we maintain this count only for VQ_HIPRIO. Maintain it for
VQ_REQUEST as well so that later it can be used to drain VQ_REQUEST
queue.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 fs/fuse/virtio_fs.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c
index c46dd4d284d6..5df97dfee37d 100644
--- a/fs/fuse/virtio_fs.c
+++ b/fs/fuse/virtio_fs.c
@@ -360,6 +360,9 @@ static void virtio_fs_requests_done_work(struct work_struct *work)
 		spin_unlock(&fpq->lock);
 
 		fuse_request_end(fc, req);
+		spin_lock(&fsvq->lock);
+		fsvq->in_flight--;
+		spin_unlock(&fsvq->lock);
 	}
 }
 
@@ -769,6 +772,7 @@ static int virtio_fs_enqueue_req(struct virtio_fs_vq *fsvq,
 		goto out;
 	}
 
+	fsvq->in_flight++;
 	notify = virtqueue_kick_prepare(vq);
 
 	spin_unlock(&fsvq->lock);
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 62+ messages in thread

* [PATCH 06/18] virtiofs: ->remove should not clean virtiofs fuse devices
  2019-09-05 19:48 [PATCH 00/18] virtiofs: Fix various races and cleanups round 1 Vivek Goyal
                   ` (4 preceding siblings ...)
  2019-09-05 19:48 ` [PATCH 05/18] Maintain count of in flight requests for VQ_REQUEST queue Vivek Goyal
@ 2019-09-05 19:48 ` Vivek Goyal
  2019-09-06 10:47   ` Stefan Hajnoczi
  2019-09-05 19:48 ` [PATCH 07/18] virtiofs: Stop virtiofs queues when device is being removed Vivek Goyal
                   ` (13 subsequent siblings)
  19 siblings, 1 reply; 62+ messages in thread
From: Vivek Goyal @ 2019-09-05 19:48 UTC (permalink / raw)
  To: linux-fsdevel, virtualization, miklos
  Cc: linux-kernel, virtio-fs, vgoyal, stefanha, dgilbert, mst

We maintain a fuse device per virt queue. This fuse devices are allocated
and installed during mount time and should be cleaned up when super block
is going away. Device removal should not clean it. Device removal should
stop queues and virtuques can go away.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 fs/fuse/virtio_fs.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c
index 5df97dfee37d..f68a25ca9e9d 100644
--- a/fs/fuse/virtio_fs.c
+++ b/fs/fuse/virtio_fs.c
@@ -497,8 +497,6 @@ static void virtio_fs_remove(struct virtio_device *vdev)
 {
 	struct virtio_fs *fs = vdev->priv;
 
-	virtio_fs_free_devs(fs);
-
 	vdev->config->reset(vdev);
 	virtio_fs_cleanup_vqs(vdev, fs);
 
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 62+ messages in thread

* [PATCH 07/18] virtiofs: Stop virtiofs queues when device is being removed
  2019-09-05 19:48 [PATCH 00/18] virtiofs: Fix various races and cleanups round 1 Vivek Goyal
                   ` (5 preceding siblings ...)
  2019-09-05 19:48 ` [PATCH 06/18] virtiofs: ->remove should not clean virtiofs fuse devices Vivek Goyal
@ 2019-09-05 19:48 ` Vivek Goyal
  2019-09-06 10:47   ` Stefan Hajnoczi
  2019-09-05 19:48 ` [PATCH 08/18] virtiofs: Drain all pending requests during ->remove time Vivek Goyal
                   ` (12 subsequent siblings)
  19 siblings, 1 reply; 62+ messages in thread
From: Vivek Goyal @ 2019-09-05 19:48 UTC (permalink / raw)
  To: linux-fsdevel, virtualization, miklos
  Cc: linux-kernel, virtio-fs, vgoyal, stefanha, dgilbert, mst

Stop all the virt queues when device is going away. This will ensure that
no new requests are submitted to virtqueue and and request will end with
error -ENOTCONN.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 fs/fuse/virtio_fs.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c
index f68a25ca9e9d..90e7b2f345e5 100644
--- a/fs/fuse/virtio_fs.c
+++ b/fs/fuse/virtio_fs.c
@@ -493,10 +493,24 @@ static int virtio_fs_probe(struct virtio_device *vdev)
 	return ret;
 }
 
+static void virtio_fs_stop_all_queues(struct virtio_fs *fs)
+{
+	struct virtio_fs_vq *fsvq;
+	int i;
+
+	for (i = 0; i < fs->nvqs; i++) {
+		fsvq = &fs->vqs[i];
+		spin_lock(&fsvq->lock);
+		fsvq->connected = false;
+		spin_unlock(&fsvq->lock);
+	}
+}
+
 static void virtio_fs_remove(struct virtio_device *vdev)
 {
 	struct virtio_fs *fs = vdev->priv;
 
+	virtio_fs_stop_all_queues(fs);
 	vdev->config->reset(vdev);
 	virtio_fs_cleanup_vqs(vdev, fs);
 
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 62+ messages in thread

* [PATCH 08/18] virtiofs: Drain all pending requests during ->remove time
  2019-09-05 19:48 [PATCH 00/18] virtiofs: Fix various races and cleanups round 1 Vivek Goyal
                   ` (6 preceding siblings ...)
  2019-09-05 19:48 ` [PATCH 07/18] virtiofs: Stop virtiofs queues when device is being removed Vivek Goyal
@ 2019-09-05 19:48 ` Vivek Goyal
  2019-09-06 10:52   ` Stefan Hajnoczi
  2019-09-08 10:11   ` [Virtio-fs] " piaojun
  2019-09-05 19:48 ` [PATCH 09/18] virtiofs: Add an helper to start all the queues Vivek Goyal
                   ` (11 subsequent siblings)
  19 siblings, 2 replies; 62+ messages in thread
From: Vivek Goyal @ 2019-09-05 19:48 UTC (permalink / raw)
  To: linux-fsdevel, virtualization, miklos
  Cc: linux-kernel, virtio-fs, vgoyal, stefanha, dgilbert, mst

When device is going away, drain all pending requests.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 fs/fuse/virtio_fs.c | 83 ++++++++++++++++++++++++++++-----------------
 1 file changed, 51 insertions(+), 32 deletions(-)

diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c
index 90e7b2f345e5..d5730a50b303 100644
--- a/fs/fuse/virtio_fs.c
+++ b/fs/fuse/virtio_fs.c
@@ -63,6 +63,55 @@ static inline struct fuse_pqueue *vq_to_fpq(struct virtqueue *vq)
 	return &vq_to_fsvq(vq)->fud->pq;
 }
 
+static void virtio_fs_drain_queue(struct virtio_fs_vq *fsvq)
+{
+	WARN_ON(fsvq->in_flight < 0);
+
+	/* Wait for in flight requests to finish.*/
+	while (1) {
+		spin_lock(&fsvq->lock);
+		if (!fsvq->in_flight) {
+			spin_unlock(&fsvq->lock);
+			break;
+		}
+		spin_unlock(&fsvq->lock);
+		usleep_range(1000, 2000);
+	}
+
+	flush_work(&fsvq->done_work);
+	flush_delayed_work(&fsvq->dispatch_work);
+}
+
+static inline void drain_hiprio_queued_reqs(struct virtio_fs_vq *fsvq)
+{
+	struct virtio_fs_forget *forget;
+
+	spin_lock(&fsvq->lock);
+	while (1) {
+		forget = list_first_entry_or_null(&fsvq->queued_reqs,
+						struct virtio_fs_forget, list);
+		if (!forget)
+			break;
+		list_del(&forget->list);
+		kfree(forget);
+	}
+	spin_unlock(&fsvq->lock);
+}
+
+static void virtio_fs_drain_all_queues(struct virtio_fs *fs)
+{
+	struct virtio_fs_vq *fsvq;
+	int i;
+
+	for (i = 0; i < fs->nvqs; i++) {
+		fsvq = &fs->vqs[i];
+		if (i == VQ_HIPRIO)
+			drain_hiprio_queued_reqs(fsvq);
+
+		virtio_fs_drain_queue(fsvq);
+	}
+}
+
 /* Add a new instance to the list or return -EEXIST if tag name exists*/
 static int virtio_fs_add_instance(struct virtio_fs *fs)
 {
@@ -511,6 +560,7 @@ static void virtio_fs_remove(struct virtio_device *vdev)
 	struct virtio_fs *fs = vdev->priv;
 
 	virtio_fs_stop_all_queues(fs);
+	virtio_fs_drain_all_queues(fs);
 	vdev->config->reset(vdev);
 	virtio_fs_cleanup_vqs(vdev, fs);
 
@@ -865,37 +915,6 @@ __releases(fiq->waitq.lock)
 	}
 }
 
-static void virtio_fs_flush_hiprio_queue(struct virtio_fs_vq *fsvq)
-{
-	struct virtio_fs_forget *forget;
-
-	WARN_ON(fsvq->in_flight < 0);
-
-	/* Go through pending forget requests and free them */
-	spin_lock(&fsvq->lock);
-	while (1) {
-		forget = list_first_entry_or_null(&fsvq->queued_reqs,
-					struct virtio_fs_forget, list);
-		if (!forget)
-			break;
-		list_del(&forget->list);
-		kfree(forget);
-	}
-
-	spin_unlock(&fsvq->lock);
-
-	/* Wait for in flight requests to finish.*/
-	while (1) {
-		spin_lock(&fsvq->lock);
-		if (!fsvq->in_flight) {
-			spin_unlock(&fsvq->lock);
-			break;
-		}
-		spin_unlock(&fsvq->lock);
-		usleep_range(1000, 2000);
-	}
-}
-
 const static struct fuse_iqueue_ops virtio_fs_fiq_ops = {
 	.wake_forget_and_unlock		= virtio_fs_wake_forget_and_unlock,
 	.wake_interrupt_and_unlock	= virtio_fs_wake_interrupt_and_unlock,
@@ -988,7 +1007,7 @@ static void virtio_kill_sb(struct super_block *sb)
 	spin_lock(&fsvq->lock);
 	fsvq->connected = false;
 	spin_unlock(&fsvq->lock);
-	virtio_fs_flush_hiprio_queue(fsvq);
+	virtio_fs_drain_all_queues(vfs);
 
 	fuse_kill_sb_anon(sb);
 	virtio_fs_free_devs(vfs);
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 62+ messages in thread

* [PATCH 09/18] virtiofs: Add an helper to start all the queues
  2019-09-05 19:48 [PATCH 00/18] virtiofs: Fix various races and cleanups round 1 Vivek Goyal
                   ` (7 preceding siblings ...)
  2019-09-05 19:48 ` [PATCH 08/18] virtiofs: Drain all pending requests during ->remove time Vivek Goyal
@ 2019-09-05 19:48 ` Vivek Goyal
  2019-09-06 10:54   ` Stefan Hajnoczi
  2019-09-05 19:48 ` [PATCH 10/18] virtiofs: Do not use device managed mem for virtio_fs and virtio_fs_vq Vivek Goyal
                   ` (10 subsequent siblings)
  19 siblings, 1 reply; 62+ messages in thread
From: Vivek Goyal @ 2019-09-05 19:48 UTC (permalink / raw)
  To: linux-fsdevel, virtualization, miklos
  Cc: linux-kernel, virtio-fs, vgoyal, stefanha, dgilbert, mst

This just marks are the queues are connected and ready to accept the
request.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 fs/fuse/virtio_fs.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c
index d5730a50b303..f2936daca39c 100644
--- a/fs/fuse/virtio_fs.c
+++ b/fs/fuse/virtio_fs.c
@@ -112,6 +112,19 @@ static void virtio_fs_drain_all_queues(struct virtio_fs *fs)
 	}
 }
 
+static void virtio_fs_start_all_queues(struct virtio_fs *fs)
+{
+	struct virtio_fs_vq *fsvq;
+	int i;
+
+	for (i = 0; i < fs->nvqs; i++) {
+		fsvq = &fs->vqs[i];
+		spin_lock(&fsvq->lock);
+		fsvq->connected = true;
+		spin_unlock(&fsvq->lock);
+	}
+}
+
 /* Add a new instance to the list or return -EEXIST if tag name exists*/
 static int virtio_fs_add_instance(struct virtio_fs *fs)
 {
@@ -483,10 +496,10 @@ static int virtio_fs_setup_vqs(struct virtio_device *vdev,
 	if (ret < 0)
 		goto out;
 
-	for (i = 0; i < fs->nvqs; i++) {
+	for (i = 0; i < fs->nvqs; i++)
 		fs->vqs[i].vq = vqs[i];
-		fs->vqs[i].connected = true;
-	}
+
+	virtio_fs_start_all_queues(fs);
 out:
 	kfree(names);
 	kfree(callbacks);
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 62+ messages in thread

* [PATCH 10/18] virtiofs: Do not use device managed mem for virtio_fs and virtio_fs_vq
  2019-09-05 19:48 [PATCH 00/18] virtiofs: Fix various races and cleanups round 1 Vivek Goyal
                   ` (8 preceding siblings ...)
  2019-09-05 19:48 ` [PATCH 09/18] virtiofs: Add an helper to start all the queues Vivek Goyal
@ 2019-09-05 19:48 ` Vivek Goyal
  2019-09-06 10:56   ` Stefan Hajnoczi
  2019-09-05 19:48 ` [PATCH 11/18] virtiofs: stop and drain queues after sending DESTROY Vivek Goyal
                   ` (9 subsequent siblings)
  19 siblings, 1 reply; 62+ messages in thread
From: Vivek Goyal @ 2019-09-05 19:48 UTC (permalink / raw)
  To: linux-fsdevel, virtualization, miklos
  Cc: linux-kernel, virtio-fs, vgoyal, stefanha, dgilbert, mst

These data structures should go away when virtio_fs object is going away.
When deivce is going away, we need to just make sure virtqueues can go
away and after that none of the code accesses vq and all the requests
get error.

So allocate memory for virtio_fs and virtio_fs_vq normally and free it
at right time.

This patch still frees up memory during device remove time. A later patch
will make virtio_fs object reference counted and this memory will be
freed when last reference to object is dropped.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 fs/fuse/virtio_fs.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c
index f2936daca39c..1ea0f889e804 100644
--- a/fs/fuse/virtio_fs.c
+++ b/fs/fuse/virtio_fs.c
@@ -446,7 +446,7 @@ static int virtio_fs_setup_vqs(struct virtio_device *vdev,
 	vq_callback_t **callbacks;
 	const char **names;
 	unsigned int i;
-	int ret;
+	int ret = 0;
 
 	virtio_cread(vdev, struct virtio_fs_config, num_queues,
 		     &fs->num_queues);
@@ -454,9 +454,7 @@ static int virtio_fs_setup_vqs(struct virtio_device *vdev,
 		return -EINVAL;
 
 	fs->nvqs = 1 + fs->num_queues;
-
-	fs->vqs = devm_kcalloc(&vdev->dev, fs->nvqs,
-				sizeof(fs->vqs[VQ_HIPRIO]), GFP_KERNEL);
+	fs->vqs = kcalloc(fs->nvqs, sizeof(fs->vqs[VQ_HIPRIO]), GFP_KERNEL);
 	if (!fs->vqs)
 		return -ENOMEM;
 
@@ -504,6 +502,8 @@ static int virtio_fs_setup_vqs(struct virtio_device *vdev,
 	kfree(names);
 	kfree(callbacks);
 	kfree(vqs);
+	if (ret)
+		kfree(fs->vqs);
 	return ret;
 }
 
@@ -519,7 +519,7 @@ static int virtio_fs_probe(struct virtio_device *vdev)
 	struct virtio_fs *fs;
 	int ret;
 
-	fs = devm_kzalloc(&vdev->dev, sizeof(*fs), GFP_KERNEL);
+	fs = kzalloc(sizeof(*fs), GFP_KERNEL);
 	if (!fs)
 		return -ENOMEM;
 	vdev->priv = fs;
@@ -552,6 +552,7 @@ static int virtio_fs_probe(struct virtio_device *vdev)
 
 out:
 	vdev->priv = NULL;
+	kfree(fs);
 	return ret;
 }
 
@@ -582,6 +583,8 @@ static void virtio_fs_remove(struct virtio_device *vdev)
 	mutex_unlock(&virtio_fs_mutex);
 
 	vdev->priv = NULL;
+	kfree(fs->vqs);
+	kfree(fs);
 }
 
 #ifdef CONFIG_PM_SLEEP
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 62+ messages in thread

* [PATCH 11/18] virtiofs: stop and drain queues after sending DESTROY
  2019-09-05 19:48 [PATCH 00/18] virtiofs: Fix various races and cleanups round 1 Vivek Goyal
                   ` (9 preceding siblings ...)
  2019-09-05 19:48 ` [PATCH 10/18] virtiofs: Do not use device managed mem for virtio_fs and virtio_fs_vq Vivek Goyal
@ 2019-09-05 19:48 ` Vivek Goyal
  2019-09-06 11:50   ` Stefan Hajnoczi
  2019-09-05 19:48 ` [PATCH 12/18] virtiofs: Use virtio_fs_free_devs() in error path Vivek Goyal
                   ` (8 subsequent siblings)
  19 siblings, 1 reply; 62+ messages in thread
From: Vivek Goyal @ 2019-09-05 19:48 UTC (permalink / raw)
  To: linux-fsdevel, virtualization, miklos
  Cc: linux-kernel, virtio-fs, vgoyal, stefanha, dgilbert, mst

During virtio_kill_sb() we first stop forget queue and drain it and then
call fuse_kill_sb_anon(). This will result in sending DESTROY request to
fuse server. Once finished, stop all the queues and drain one more time
just to be sure and then free up the devices.

Given drain queues will call flush_work() on various workers, remove this
logic from virtio_free_devs().

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 fs/fuse/virtio_fs.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c
index 1ea0f889e804..a76bd5a04521 100644
--- a/fs/fuse/virtio_fs.c
+++ b/fs/fuse/virtio_fs.c
@@ -180,9 +180,6 @@ static void virtio_fs_free_devs(struct virtio_fs *fs)
 		if (!fsvq->fud)
 			continue;
 
-		flush_work(&fsvq->done_work);
-		flush_delayed_work(&fsvq->dispatch_work);
-
 		/* TODO need to quiesce/end_requests/decrement dev_count */
 		fuse_dev_free(fsvq->fud);
 		fsvq->fud = NULL;
@@ -994,6 +991,8 @@ static int virtio_fs_fill_super(struct super_block *sb)
 		atomic_inc(&fc->dev_count);
 	}
 
+	/* Previous unmount will stop all queues. Start these again */
+	virtio_fs_start_all_queues(fs);
 	fuse_send_init(fc, init_req);
 	return 0;
 
@@ -1026,6 +1025,12 @@ static void virtio_kill_sb(struct super_block *sb)
 	virtio_fs_drain_all_queues(vfs);
 
 	fuse_kill_sb_anon(sb);
+
+	/* fuse_kill_sb_anon() must have sent destroy. Stop all queues
+	 * and drain one more time and free fuse devices.
+	 */
+	virtio_fs_stop_all_queues(vfs);
+	virtio_fs_drain_all_queues(vfs);
 	virtio_fs_free_devs(vfs);
 }
 
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 62+ messages in thread

* [PATCH 12/18] virtiofs: Use virtio_fs_free_devs() in error path
  2019-09-05 19:48 [PATCH 00/18] virtiofs: Fix various races and cleanups round 1 Vivek Goyal
                   ` (10 preceding siblings ...)
  2019-09-05 19:48 ` [PATCH 11/18] virtiofs: stop and drain queues after sending DESTROY Vivek Goyal
@ 2019-09-05 19:48 ` Vivek Goyal
  2019-09-06 11:51   ` Stefan Hajnoczi
  2019-09-05 19:48 ` [PATCH 13/18] virtiofs: Do not access virtqueue in request submission path Vivek Goyal
                   ` (7 subsequent siblings)
  19 siblings, 1 reply; 62+ messages in thread
From: Vivek Goyal @ 2019-09-05 19:48 UTC (permalink / raw)
  To: linux-fsdevel, virtualization, miklos
  Cc: linux-kernel, virtio-fs, vgoyal, stefanha, dgilbert, mst

We already have an helper to cleanup fuse devices. Use that instead of
duplicating the code.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 fs/fuse/virtio_fs.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c
index a76bd5a04521..40259368a6bd 100644
--- a/fs/fuse/virtio_fs.c
+++ b/fs/fuse/virtio_fs.c
@@ -999,8 +999,7 @@ static int virtio_fs_fill_super(struct super_block *sb)
 err_free_init_req:
 	fuse_request_free(init_req);
 err_free_fuse_devs:
-	for (i = 0; i < fs->nvqs; i++)
-		fuse_dev_free(fs->vqs[i].fud);
+	virtio_fs_free_devs(fs);
 err:
 	return err;
 }
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 62+ messages in thread

* [PATCH 13/18] virtiofs: Do not access virtqueue in request submission path
  2019-09-05 19:48 [PATCH 00/18] virtiofs: Fix various races and cleanups round 1 Vivek Goyal
                   ` (11 preceding siblings ...)
  2019-09-05 19:48 ` [PATCH 12/18] virtiofs: Use virtio_fs_free_devs() in error path Vivek Goyal
@ 2019-09-05 19:48 ` Vivek Goyal
  2019-09-06 11:52   ` Stefan Hajnoczi
  2019-09-05 19:48 ` [PATCH 14/18] virtiofs: Add a fuse_iqueue operation to put() reference Vivek Goyal
                   ` (6 subsequent siblings)
  19 siblings, 1 reply; 62+ messages in thread
From: Vivek Goyal @ 2019-09-05 19:48 UTC (permalink / raw)
  To: linux-fsdevel, virtualization, miklos
  Cc: linux-kernel, virtio-fs, vgoyal, stefanha, dgilbert, mst

In request submission path it is possible that virtqueue is already gone
due to driver->remove(). So do not access it in dev_dbg(). Use pr_debug()
instead.

If virtuqueue is gone, this will result in NULL pointer deference.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 fs/fuse/virtio_fs.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c
index 40259368a6bd..01bbf2c0e144 100644
--- a/fs/fuse/virtio_fs.c
+++ b/fs/fuse/virtio_fs.c
@@ -888,10 +888,10 @@ __releases(fiq->waitq.lock)
 	fs = fiq->priv;
 	fc = fs->vqs[queue_id].fud->fc;
 
-	dev_dbg(&fs->vqs[queue_id].vq->vdev->dev,
-		"%s: opcode %u unique %#llx nodeid %#llx in.len %u out.len %u\n",
-		__func__, req->in.h.opcode, req->in.h.unique, req->in.h.nodeid,
-		req->in.h.len, fuse_len_args(req->out.numargs, req->out.args));
+	pr_debug("%s: opcode %u unique %#llx nodeid %#llx in.len %u out.len %u"
+		 "\n", __func__, req->in.h.opcode, req->in.h.unique,
+		 req->in.h.nodeid, req->in.h.len,
+		 fuse_len_args(req->out.numargs, req->out.args));
 
 	fpq = &fs->vqs[queue_id].fud->pq;
 	spin_lock(&fpq->lock);
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 62+ messages in thread

* [PATCH 14/18] virtiofs: Add a fuse_iqueue operation to put() reference
  2019-09-05 19:48 [PATCH 00/18] virtiofs: Fix various races and cleanups round 1 Vivek Goyal
                   ` (12 preceding siblings ...)
  2019-09-05 19:48 ` [PATCH 13/18] virtiofs: Do not access virtqueue in request submission path Vivek Goyal
@ 2019-09-05 19:48 ` Vivek Goyal
  2019-09-06 12:00   ` Stefan Hajnoczi
  2019-09-05 19:48 ` [PATCH 15/18] virtiofs: Make virtio_fs object refcounted Vivek Goyal
                   ` (5 subsequent siblings)
  19 siblings, 1 reply; 62+ messages in thread
From: Vivek Goyal @ 2019-09-05 19:48 UTC (permalink / raw)
  To: linux-fsdevel, virtualization, miklos
  Cc: linux-kernel, virtio-fs, vgoyal, stefanha, dgilbert, mst

Soon I will make virtio_fs object reference counted, where reference will
be taken by device as well as by fuse_conn (fuse_conn->fuse_iqueue->fiq_priv).

When fuse_connection is going away, it should put its reference on virtio_fs
object.

So add a fuse_iqueue method which can be used to call into virtio_fs to
put the reference on the object (fiq_priv).

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 fs/fuse/fuse_i.h | 5 +++++
 fs/fuse/inode.c  | 6 ++++++
 2 files changed, 11 insertions(+)

diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
index 85e2dcad68c1..04e2c000d63f 100644
--- a/fs/fuse/fuse_i.h
+++ b/fs/fuse/fuse_i.h
@@ -479,6 +479,11 @@ struct fuse_iqueue_ops {
 	 */
 	void (*wake_pending_and_unlock)(struct fuse_iqueue *fiq)
 		__releases(fiq->waitq.lock);
+
+	/**
+	 * Put a reference on fiq_priv.
+	 */
+	void (*put)(struct fuse_iqueue *fiq);
 };
 
 /** /dev/fuse input queue operations */
diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
index 7fa0dcc6f565..70a433bdf01f 100644
--- a/fs/fuse/inode.c
+++ b/fs/fuse/inode.c
@@ -631,8 +631,14 @@ EXPORT_SYMBOL_GPL(fuse_conn_init);
 void fuse_conn_put(struct fuse_conn *fc)
 {
 	if (refcount_dec_and_test(&fc->count)) {
+		struct fuse_iqueue *fiq = &fc->iq;
+
 		if (fc->destroy_req)
 			fuse_request_free(fc->destroy_req);
+		if (fiq->priv && fiq->ops->put) {
+			fiq->ops->put(fiq);
+			fiq->priv = NULL;
+		}
 		put_pid_ns(fc->pid_ns);
 		put_user_ns(fc->user_ns);
 		fc->release(fc);
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 62+ messages in thread

* [PATCH 15/18] virtiofs: Make virtio_fs object refcounted
  2019-09-05 19:48 [PATCH 00/18] virtiofs: Fix various races and cleanups round 1 Vivek Goyal
                   ` (13 preceding siblings ...)
  2019-09-05 19:48 ` [PATCH 14/18] virtiofs: Add a fuse_iqueue operation to put() reference Vivek Goyal
@ 2019-09-05 19:48 ` Vivek Goyal
  2019-09-06 12:03   ` Stefan Hajnoczi
  2019-09-08 11:10   ` [Virtio-fs] " piaojun
  2019-09-05 19:48 ` [PATCH 16/18] virtiofs: Use virtio_fs_mutex for races w.r.t ->remove and mount path Vivek Goyal
                   ` (4 subsequent siblings)
  19 siblings, 2 replies; 62+ messages in thread
From: Vivek Goyal @ 2019-09-05 19:48 UTC (permalink / raw)
  To: linux-fsdevel, virtualization, miklos
  Cc: linux-kernel, virtio-fs, vgoyal, stefanha, dgilbert, mst

This object is used both by fuse_connection as well virt device. So make
this object reference counted and that makes it easy to define life cycle
of the object.

Now deivce can be removed while filesystem is still mounted. This will
cleanup all the virtqueues but virtio_fs object will still be around and
will be cleaned when filesystem is unmounted and sb/fc drops its reference.

Removing a device also stops all virt queues and any new reuqest gets
error -ENOTCONN. All existing in flight requests are drained before
->remove returns.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 fs/fuse/virtio_fs.c | 52 +++++++++++++++++++++++++++++++++++++--------
 1 file changed, 43 insertions(+), 9 deletions(-)

diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c
index 01bbf2c0e144..29ec2f5bbbe2 100644
--- a/fs/fuse/virtio_fs.c
+++ b/fs/fuse/virtio_fs.c
@@ -37,6 +37,7 @@ struct virtio_fs_vq {
 
 /* A virtio-fs device instance */
 struct virtio_fs {
+	struct kref refcount;
 	struct list_head list;    /* on virtio_fs_instances */
 	char *tag;
 	struct virtio_fs_vq *vqs;
@@ -63,6 +64,27 @@ static inline struct fuse_pqueue *vq_to_fpq(struct virtqueue *vq)
 	return &vq_to_fsvq(vq)->fud->pq;
 }
 
+static void release_virtiofs_obj(struct kref *ref)
+{
+	struct virtio_fs *vfs = container_of(ref, struct virtio_fs, refcount);
+
+	kfree(vfs->vqs);
+	kfree(vfs);
+}
+
+static void virtiofs_put(struct virtio_fs *fs)
+{
+	mutex_lock(&virtio_fs_mutex);
+	kref_put(&fs->refcount, release_virtiofs_obj);
+	mutex_unlock(&virtio_fs_mutex);
+}
+
+static void virtio_fs_put(struct fuse_iqueue *fiq)
+{
+	struct virtio_fs *vfs = fiq->priv;
+	virtiofs_put(vfs);
+}
+
 static void virtio_fs_drain_queue(struct virtio_fs_vq *fsvq)
 {
 	WARN_ON(fsvq->in_flight < 0);
@@ -156,8 +178,10 @@ static struct virtio_fs *virtio_fs_find_instance(const char *tag)
 	mutex_lock(&virtio_fs_mutex);
 
 	list_for_each_entry(fs, &virtio_fs_instances, list) {
-		if (strcmp(fs->tag, tag) == 0)
+		if (strcmp(fs->tag, tag) == 0) {
+			kref_get(&fs->refcount);
 			goto found;
+		}
 	}
 
 	fs = NULL; /* not found */
@@ -519,6 +543,7 @@ static int virtio_fs_probe(struct virtio_device *vdev)
 	fs = kzalloc(sizeof(*fs), GFP_KERNEL);
 	if (!fs)
 		return -ENOMEM;
+	kref_init(&fs->refcount);
 	vdev->priv = fs;
 
 	ret = virtio_fs_read_tag(vdev, fs);
@@ -570,18 +595,18 @@ static void virtio_fs_remove(struct virtio_device *vdev)
 {
 	struct virtio_fs *fs = vdev->priv;
 
+	mutex_lock(&virtio_fs_mutex);
+	list_del_init(&fs->list);
+	mutex_unlock(&virtio_fs_mutex);
+
 	virtio_fs_stop_all_queues(fs);
 	virtio_fs_drain_all_queues(fs);
 	vdev->config->reset(vdev);
 	virtio_fs_cleanup_vqs(vdev, fs);
 
-	mutex_lock(&virtio_fs_mutex);
-	list_del(&fs->list);
-	mutex_unlock(&virtio_fs_mutex);
-
 	vdev->priv = NULL;
-	kfree(fs->vqs);
-	kfree(fs);
+	/* Put device reference on virtio_fs object */
+	virtiofs_put(fs);
 }
 
 #ifdef CONFIG_PM_SLEEP
@@ -932,6 +957,7 @@ const static struct fuse_iqueue_ops virtio_fs_fiq_ops = {
 	.wake_forget_and_unlock		= virtio_fs_wake_forget_and_unlock,
 	.wake_interrupt_and_unlock	= virtio_fs_wake_interrupt_and_unlock,
 	.wake_pending_and_unlock	= virtio_fs_wake_pending_and_unlock,
+	.put				= virtio_fs_put,
 };
 
 static int virtio_fs_fill_super(struct super_block *sb)
@@ -1026,7 +1052,9 @@ static void virtio_kill_sb(struct super_block *sb)
 	fuse_kill_sb_anon(sb);
 
 	/* fuse_kill_sb_anon() must have sent destroy. Stop all queues
-	 * and drain one more time and free fuse devices.
+	 * and drain one more time and free fuse devices. Freeing fuse
+	 * devices will drop their reference on fuse_conn and that in
+	 * turn will drop its reference on virtio_fs object.
 	 */
 	virtio_fs_stop_all_queues(vfs);
 	virtio_fs_drain_all_queues(vfs);
@@ -1060,6 +1088,10 @@ static int virtio_fs_get_tree(struct fs_context *fsc)
 	struct fuse_conn *fc;
 	int err;
 
+	/* This gets a reference on virtio_fs object. This ptr gets installed
+	 * in fc->iq->priv. Once fuse_conn is going away, it calls ->put()
+	 * to drop the reference to this object.
+	 */
 	fs = virtio_fs_find_instance(fsc->source);
 	if (!fs) {
 		pr_info("virtio-fs: tag <%s> not found\n", fsc->source);
@@ -1067,8 +1099,10 @@ static int virtio_fs_get_tree(struct fs_context *fsc)
 	}
 
 	fc = kzalloc(sizeof(struct fuse_conn), GFP_KERNEL);
-	if (!fc)
+	if (!fc) {
+		virtiofs_put(fs);
 		return -ENOMEM;
+	}
 
 	fuse_conn_init(fc, get_user_ns(current_user_ns()), &virtio_fs_fiq_ops,
 		       fs);
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 62+ messages in thread

* [PATCH 16/18] virtiofs: Use virtio_fs_mutex for races w.r.t ->remove and mount path
  2019-09-05 19:48 [PATCH 00/18] virtiofs: Fix various races and cleanups round 1 Vivek Goyal
                   ` (14 preceding siblings ...)
  2019-09-05 19:48 ` [PATCH 15/18] virtiofs: Make virtio_fs object refcounted Vivek Goyal
@ 2019-09-05 19:48 ` Vivek Goyal
  2019-09-06 12:05   ` Stefan Hajnoczi
  2019-09-08 11:19   ` [Virtio-fs] " piaojun
  2019-09-05 19:48 ` [PATCH 17/18] virtiofs: Remove TODO to quiesce/end_requests Vivek Goyal
                   ` (3 subsequent siblings)
  19 siblings, 2 replies; 62+ messages in thread
From: Vivek Goyal @ 2019-09-05 19:48 UTC (permalink / raw)
  To: linux-fsdevel, virtualization, miklos
  Cc: linux-kernel, virtio-fs, vgoyal, stefanha, dgilbert, mst

It is possible that a mount is in progress and device is being removed at
the same time. Use virtio_fs_mutex to avoid races.

This also takes care of bunch of races and removes some TODO items.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 fs/fuse/virtio_fs.c | 32 ++++++++++++++++++++++----------
 1 file changed, 22 insertions(+), 10 deletions(-)

diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c
index 29ec2f5bbbe2..c483482185b6 100644
--- a/fs/fuse/virtio_fs.c
+++ b/fs/fuse/virtio_fs.c
@@ -13,7 +13,9 @@
 #include <linux/highmem.h>
 #include "fuse_i.h"
 
-/* List of virtio-fs device instances and a lock for the list */
+/* List of virtio-fs device instances and a lock for the list. Also provides
+ * mutual exclusion in device removal and mounting path
+ */
 static DEFINE_MUTEX(virtio_fs_mutex);
 static LIST_HEAD(virtio_fs_instances);
 
@@ -72,17 +74,19 @@ static void release_virtiofs_obj(struct kref *ref)
 	kfree(vfs);
 }
 
+/* Make sure virtiofs_mutex is held */
 static void virtiofs_put(struct virtio_fs *fs)
 {
-	mutex_lock(&virtio_fs_mutex);
 	kref_put(&fs->refcount, release_virtiofs_obj);
-	mutex_unlock(&virtio_fs_mutex);
 }
 
 static void virtio_fs_put(struct fuse_iqueue *fiq)
 {
 	struct virtio_fs *vfs = fiq->priv;
+
+	mutex_lock(&virtio_fs_mutex);
 	virtiofs_put(vfs);
+	mutex_unlock(&virtio_fs_mutex);
 }
 
 static void virtio_fs_drain_queue(struct virtio_fs_vq *fsvq)
@@ -596,9 +600,8 @@ static void virtio_fs_remove(struct virtio_device *vdev)
 	struct virtio_fs *fs = vdev->priv;
 
 	mutex_lock(&virtio_fs_mutex);
+	/* This device is going away. No one should get new reference */
 	list_del_init(&fs->list);
-	mutex_unlock(&virtio_fs_mutex);
-
 	virtio_fs_stop_all_queues(fs);
 	virtio_fs_drain_all_queues(fs);
 	vdev->config->reset(vdev);
@@ -607,6 +610,7 @@ static void virtio_fs_remove(struct virtio_device *vdev)
 	vdev->priv = NULL;
 	/* Put device reference on virtio_fs object */
 	virtiofs_put(fs);
+	mutex_unlock(&virtio_fs_mutex);
 }
 
 #ifdef CONFIG_PM_SLEEP
@@ -978,10 +982,15 @@ static int virtio_fs_fill_super(struct super_block *sb)
 		.no_force_umount = true,
 	};
 
-	/* TODO lock */
-	if (fs->vqs[VQ_REQUEST].fud) {
-		pr_err("virtio-fs: device already in use\n");
-		err = -EBUSY;
+	mutex_lock(&virtio_fs_mutex);
+
+	/* After holding mutex, make sure virtiofs device is still there.
+	 * Though we are holding a refernce to it, drive ->remove might
+	 * still have cleaned up virtual queues. In that case bail out.
+	 */
+	err = -EINVAL;
+	if (list_empty(&fs->list)) {
+		pr_info("virtio-fs: tag <%s> not found\n", fs->tag);
 		goto err;
 	}
 
@@ -1007,7 +1016,6 @@ static int virtio_fs_fill_super(struct super_block *sb)
 
 	fc = fs->vqs[VQ_REQUEST].fud->fc;
 
-	/* TODO take fuse_mutex around this loop? */
 	for (i = 0; i < fs->nvqs; i++) {
 		struct virtio_fs_vq *fsvq = &fs->vqs[i];
 
@@ -1020,6 +1028,7 @@ static int virtio_fs_fill_super(struct super_block *sb)
 	/* Previous unmount will stop all queues. Start these again */
 	virtio_fs_start_all_queues(fs);
 	fuse_send_init(fc, init_req);
+	mutex_unlock(&virtio_fs_mutex);
 	return 0;
 
 err_free_init_req:
@@ -1027,6 +1036,7 @@ static int virtio_fs_fill_super(struct super_block *sb)
 err_free_fuse_devs:
 	virtio_fs_free_devs(fs);
 err:
+	mutex_unlock(&virtio_fs_mutex);
 	return err;
 }
 
@@ -1100,7 +1110,9 @@ static int virtio_fs_get_tree(struct fs_context *fsc)
 
 	fc = kzalloc(sizeof(struct fuse_conn), GFP_KERNEL);
 	if (!fc) {
+		mutex_lock(&virtio_fs_mutex);
 		virtiofs_put(fs);
+		mutex_unlock(&virtio_fs_mutex);
 		return -ENOMEM;
 	}
 
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 62+ messages in thread

* [PATCH 17/18] virtiofs: Remove TODO to quiesce/end_requests
  2019-09-05 19:48 [PATCH 00/18] virtiofs: Fix various races and cleanups round 1 Vivek Goyal
                   ` (15 preceding siblings ...)
  2019-09-05 19:48 ` [PATCH 16/18] virtiofs: Use virtio_fs_mutex for races w.r.t ->remove and mount path Vivek Goyal
@ 2019-09-05 19:48 ` Vivek Goyal
  2019-09-06 12:06   ` Stefan Hajnoczi
  2019-09-05 19:48 ` [PATCH 18/18] virtiofs: Remove TODO item from virtio_fs_free_devs() Vivek Goyal
                   ` (2 subsequent siblings)
  19 siblings, 1 reply; 62+ messages in thread
From: Vivek Goyal @ 2019-09-05 19:48 UTC (permalink / raw)
  To: linux-fsdevel, virtualization, miklos
  Cc: linux-kernel, virtio-fs, vgoyal, stefanha, dgilbert, mst

We now stop queues and drain all the pending requests from all virtqueues.
So this is not a TODO anymore.

Got rid of incrementing fc->dev_count as well. It did not seem meaningful
for virtio_fs.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 fs/fuse/virtio_fs.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c
index c483482185b6..eadaea6eb8e2 100644
--- a/fs/fuse/virtio_fs.c
+++ b/fs/fuse/virtio_fs.c
@@ -208,7 +208,6 @@ static void virtio_fs_free_devs(struct virtio_fs *fs)
 		if (!fsvq->fud)
 			continue;
 
-		/* TODO need to quiesce/end_requests/decrement dev_count */
 		fuse_dev_free(fsvq->fud);
 		fsvq->fud = NULL;
 	}
@@ -1022,7 +1021,6 @@ static int virtio_fs_fill_super(struct super_block *sb)
 		if (i == VQ_REQUEST)
 			continue; /* already initialized */
 		fuse_dev_install(fsvq->fud, fc);
-		atomic_inc(&fc->dev_count);
 	}
 
 	/* Previous unmount will stop all queues. Start these again */
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 62+ messages in thread

* [PATCH 18/18] virtiofs: Remove TODO item from virtio_fs_free_devs()
  2019-09-05 19:48 [PATCH 00/18] virtiofs: Fix various races and cleanups round 1 Vivek Goyal
                   ` (16 preceding siblings ...)
  2019-09-05 19:48 ` [PATCH 17/18] virtiofs: Remove TODO to quiesce/end_requests Vivek Goyal
@ 2019-09-05 19:48 ` Vivek Goyal
  2019-09-06 12:06   ` Stefan Hajnoczi
  2019-09-06  8:15 ` [PATCH 00/18] virtiofs: Fix various races and cleanups round 1 Miklos Szeredi
  2019-09-06 13:37 ` Michael S. Tsirkin
  19 siblings, 1 reply; 62+ messages in thread
From: Vivek Goyal @ 2019-09-05 19:48 UTC (permalink / raw)
  To: linux-fsdevel, virtualization, miklos
  Cc: linux-kernel, virtio-fs, vgoyal, stefanha, dgilbert, mst

virtio_fs_free_devs() is now called from ->kill_sb(). By this time
all device queues have been quiesced. I am assuming that while
->kill_sb() is in progress, another mount instance will wait for
it to finish (sb->s_umount mutex provides mutual exclusion).

W.r.t ->remove path, we should be fine as we are not touching vdev
or virtqueues. And we have reference on virtio_fs object, so we know
rest of the data structures are valid.

So I can't see the need of any additional locking yet.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 fs/fuse/virtio_fs.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c
index eadaea6eb8e2..61aa3eba7b22 100644
--- a/fs/fuse/virtio_fs.c
+++ b/fs/fuse/virtio_fs.c
@@ -200,8 +200,6 @@ static void virtio_fs_free_devs(struct virtio_fs *fs)
 {
 	unsigned int i;
 
-	/* TODO lock */
-
 	for (i = 0; i < fs->nvqs; i++) {
 		struct virtio_fs_vq *fsvq = &fs->vqs[i];
 
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 62+ messages in thread

* Re: [PATCH 00/18] virtiofs: Fix various races and cleanups round 1
  2019-09-05 19:48 [PATCH 00/18] virtiofs: Fix various races and cleanups round 1 Vivek Goyal
                   ` (17 preceding siblings ...)
  2019-09-05 19:48 ` [PATCH 18/18] virtiofs: Remove TODO item from virtio_fs_free_devs() Vivek Goyal
@ 2019-09-06  8:15 ` Miklos Szeredi
  2019-09-06 10:36   ` Stefan Hajnoczi
  2019-09-06 13:37 ` Michael S. Tsirkin
  19 siblings, 1 reply; 62+ messages in thread
From: Miklos Szeredi @ 2019-09-06  8:15 UTC (permalink / raw)
  To: Vivek Goyal
  Cc: linux-fsdevel, virtualization, linux-kernel, virtio-fs,
	Stefan Hajnoczi, Dr. David Alan Gilbert, Michael S. Tsirkin

On Thu, Sep 5, 2019 at 9:49 PM Vivek Goyal <vgoyal@redhat.com> wrote:
>
> Hi,
>
> Michael Tsirkin pointed out issues w.r.t various locking related TODO
> items and races w.r.t device removal.
>
> In this first round of cleanups, I have taken care of most pressing
> issues.
>
> These patches apply on top of following.
>
> git://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/fuse.git#virtiofs-v4
>
> I have tested these patches with mount/umount and device removal using
> qemu monitor. For example.

Is device removal mandatory?  Can't this be made a non-removable
device?  Is there a good reason why removing the virtio-fs device
makes sense?

Thanks,
Miklos

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH 00/18] virtiofs: Fix various races and cleanups round 1
  2019-09-06  8:15 ` [PATCH 00/18] virtiofs: Fix various races and cleanups round 1 Miklos Szeredi
@ 2019-09-06 10:36   ` Stefan Hajnoczi
  2019-09-06 11:52     ` Miklos Szeredi
  0 siblings, 1 reply; 62+ messages in thread
From: Stefan Hajnoczi @ 2019-09-06 10:36 UTC (permalink / raw)
  To: Miklos Szeredi
  Cc: Vivek Goyal, linux-fsdevel, virtualization, linux-kernel,
	virtio-fs, Dr. David Alan Gilbert, Michael S. Tsirkin

[-- Attachment #1: Type: text/plain, Size: 902 bytes --]

On Fri, Sep 06, 2019 at 10:15:14AM +0200, Miklos Szeredi wrote:
> On Thu, Sep 5, 2019 at 9:49 PM Vivek Goyal <vgoyal@redhat.com> wrote:
> >
> > Hi,
> >
> > Michael Tsirkin pointed out issues w.r.t various locking related TODO
> > items and races w.r.t device removal.
> >
> > In this first round of cleanups, I have taken care of most pressing
> > issues.
> >
> > These patches apply on top of following.
> >
> > git://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/fuse.git#virtiofs-v4
> >
> > I have tested these patches with mount/umount and device removal using
> > qemu monitor. For example.
> 
> Is device removal mandatory?  Can't this be made a non-removable
> device?  Is there a good reason why removing the virtio-fs device
> makes sense?

Hot plugging and unplugging virtio PCI adapters is common.  I'd very
much like removal to work from the beginning.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH 01/18] virtiofs: Remove request from processing list before calling end
  2019-09-05 19:48 ` [PATCH 01/18] virtiofs: Remove request from processing list before calling end Vivek Goyal
@ 2019-09-06 10:40   ` Stefan Hajnoczi
  0 siblings, 0 replies; 62+ messages in thread
From: Stefan Hajnoczi @ 2019-09-06 10:40 UTC (permalink / raw)
  To: Vivek Goyal
  Cc: linux-fsdevel, virtualization, miklos, linux-kernel, virtio-fs,
	dgilbert, mst

[-- Attachment #1: Type: text/plain, Size: 466 bytes --]

On Thu, Sep 05, 2019 at 03:48:42PM -0400, Vivek Goyal wrote:
> In error path we are calling fuse_request_end() but we need to clear
> FR_SENT bit as well as remove request from processing queue. Otherwise
> fuse_request_end() triggers a warning as well as other issues show up.
> 
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
>  fs/fuse/virtio_fs.c | 4 ++++
>  1 file changed, 4 insertions(+)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH 02/18] virtiofs: Check whether hiprio queue is connected at submission time
  2019-09-05 19:48 ` [PATCH 02/18] virtiofs: Check whether hiprio queue is connected at submission time Vivek Goyal
@ 2019-09-06 10:43   ` Stefan Hajnoczi
  0 siblings, 0 replies; 62+ messages in thread
From: Stefan Hajnoczi @ 2019-09-06 10:43 UTC (permalink / raw)
  To: Vivek Goyal
  Cc: linux-fsdevel, virtualization, miklos, linux-kernel, virtio-fs,
	dgilbert, mst

[-- Attachment #1: Type: text/plain, Size: 670 bytes --]

On Thu, Sep 05, 2019 at 03:48:43PM -0400, Vivek Goyal wrote:
> For hiprio queue (forget requests), we are keeping a state in queue whether
> queue is connected or not. If queue is not connected, do not try to submit
> request and return error instead.
> 
> As of now, we are checking for this state only in path where worker tries
> to submit forget after first attempt failed. Check this state even in
> the path when request is being submitted first time.
> 
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
>  fs/fuse/virtio_fs.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH 03/18] virtiofs: Pass fsvq instead of vq as parameter to virtio_fs_enqueue_req
  2019-09-05 19:48 ` [PATCH 03/18] virtiofs: Pass fsvq instead of vq as parameter to virtio_fs_enqueue_req Vivek Goyal
@ 2019-09-06 10:44   ` Stefan Hajnoczi
  0 siblings, 0 replies; 62+ messages in thread
From: Stefan Hajnoczi @ 2019-09-06 10:44 UTC (permalink / raw)
  To: Vivek Goyal
  Cc: linux-fsdevel, virtualization, miklos, linux-kernel, virtio-fs,
	dgilbert, mst

[-- Attachment #1: Type: text/plain, Size: 562 bytes --]

On Thu, Sep 05, 2019 at 03:48:44PM -0400, Vivek Goyal wrote:
> Pass fsvq instead of vq as parameter to virtio_fs_enqueue_req(). We will
> retrieve vq from fsvq under spin lock.
> 
> Later in the patch series we will retrieve vq only if fsvq is still connected
> other vq might have been cleaned up by device ->remove code and we will
> return error.
> 
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
>  fs/fuse/virtio_fs.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH 04/18] virtiofs: Check connected state for VQ_REQUEST queue as well
  2019-09-05 19:48 ` [PATCH 04/18] virtiofs: Check connected state for VQ_REQUEST queue as well Vivek Goyal
@ 2019-09-06 10:45   ` Stefan Hajnoczi
  0 siblings, 0 replies; 62+ messages in thread
From: Stefan Hajnoczi @ 2019-09-06 10:45 UTC (permalink / raw)
  To: Vivek Goyal
  Cc: linux-fsdevel, virtualization, miklos, linux-kernel, virtio-fs,
	dgilbert, mst

[-- Attachment #1: Type: text/plain, Size: 615 bytes --]

On Thu, Sep 05, 2019 at 03:48:45PM -0400, Vivek Goyal wrote:
> Right now we are checking ->connected state only for VQ_HIPRIO. Now we want
> to make use of this method for all queues. So check it for VQ_REQUEST as
> well.
> 
> This will be helpful if device has been removed and virtqueue is gone. In
> that case ->connected will be false and request can't be submitted anymore
> and user space will see error -ENOTCONN.
> 
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
>  fs/fuse/virtio_fs.c | 6 ++++++
>  1 file changed, 6 insertions(+)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH 05/18] Maintain count of in flight requests for VQ_REQUEST queue
  2019-09-05 19:48 ` [PATCH 05/18] Maintain count of in flight requests for VQ_REQUEST queue Vivek Goyal
@ 2019-09-06 10:46   ` Stefan Hajnoczi
  0 siblings, 0 replies; 62+ messages in thread
From: Stefan Hajnoczi @ 2019-09-06 10:46 UTC (permalink / raw)
  To: Vivek Goyal
  Cc: linux-fsdevel, virtualization, miklos, linux-kernel, virtio-fs,
	dgilbert, mst

[-- Attachment #1: Type: text/plain, Size: 399 bytes --]

On Thu, Sep 05, 2019 at 03:48:46PM -0400, Vivek Goyal wrote:
> As of now we maintain this count only for VQ_HIPRIO. Maintain it for
> VQ_REQUEST as well so that later it can be used to drain VQ_REQUEST
> queue.
> 
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
>  fs/fuse/virtio_fs.c | 4 ++++
>  1 file changed, 4 insertions(+)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH 06/18] virtiofs: ->remove should not clean virtiofs fuse devices
  2019-09-05 19:48 ` [PATCH 06/18] virtiofs: ->remove should not clean virtiofs fuse devices Vivek Goyal
@ 2019-09-06 10:47   ` Stefan Hajnoczi
  0 siblings, 0 replies; 62+ messages in thread
From: Stefan Hajnoczi @ 2019-09-06 10:47 UTC (permalink / raw)
  To: Vivek Goyal
  Cc: linux-fsdevel, virtualization, miklos, linux-kernel, virtio-fs,
	dgilbert, mst

[-- Attachment #1: Type: text/plain, Size: 515 bytes --]

On Thu, Sep 05, 2019 at 03:48:47PM -0400, Vivek Goyal wrote:
> We maintain a fuse device per virt queue. This fuse devices are allocated
> and installed during mount time and should be cleaned up when super block
> is going away. Device removal should not clean it. Device removal should
> stop queues and virtuques can go away.
> 
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
>  fs/fuse/virtio_fs.c | 2 --
>  1 file changed, 2 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH 07/18] virtiofs: Stop virtiofs queues when device is being removed
  2019-09-05 19:48 ` [PATCH 07/18] virtiofs: Stop virtiofs queues when device is being removed Vivek Goyal
@ 2019-09-06 10:47   ` Stefan Hajnoczi
  0 siblings, 0 replies; 62+ messages in thread
From: Stefan Hajnoczi @ 2019-09-06 10:47 UTC (permalink / raw)
  To: Vivek Goyal
  Cc: linux-fsdevel, virtualization, miklos, linux-kernel, virtio-fs,
	dgilbert, mst

[-- Attachment #1: Type: text/plain, Size: 431 bytes --]

On Thu, Sep 05, 2019 at 03:48:48PM -0400, Vivek Goyal wrote:
> Stop all the virt queues when device is going away. This will ensure that
> no new requests are submitted to virtqueue and and request will end with
> error -ENOTCONN.
> 
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
>  fs/fuse/virtio_fs.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH 08/18] virtiofs: Drain all pending requests during ->remove time
  2019-09-05 19:48 ` [PATCH 08/18] virtiofs: Drain all pending requests during ->remove time Vivek Goyal
@ 2019-09-06 10:52   ` Stefan Hajnoczi
  2019-09-06 14:17     ` Vivek Goyal
  2019-09-08 10:11   ` [Virtio-fs] " piaojun
  1 sibling, 1 reply; 62+ messages in thread
From: Stefan Hajnoczi @ 2019-09-06 10:52 UTC (permalink / raw)
  To: Vivek Goyal
  Cc: linux-fsdevel, virtualization, miklos, linux-kernel, virtio-fs,
	dgilbert, mst

[-- Attachment #1: Type: text/plain, Size: 493 bytes --]

On Thu, Sep 05, 2019 at 03:48:49PM -0400, Vivek Goyal wrote:
> +static void virtio_fs_drain_queue(struct virtio_fs_vq *fsvq)
> +{
> +	WARN_ON(fsvq->in_flight < 0);
> +
> +	/* Wait for in flight requests to finish.*/
> +	while (1) {
> +		spin_lock(&fsvq->lock);
> +		if (!fsvq->in_flight) {
> +			spin_unlock(&fsvq->lock);
> +			break;
> +		}
> +		spin_unlock(&fsvq->lock);
> +		usleep_range(1000, 2000);
> +	}

I think all contexts that call this allow sleeping so we could avoid
usleep here.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH 09/18] virtiofs: Add an helper to start all the queues
  2019-09-05 19:48 ` [PATCH 09/18] virtiofs: Add an helper to start all the queues Vivek Goyal
@ 2019-09-06 10:54   ` Stefan Hajnoczi
  0 siblings, 0 replies; 62+ messages in thread
From: Stefan Hajnoczi @ 2019-09-06 10:54 UTC (permalink / raw)
  To: Vivek Goyal
  Cc: linux-fsdevel, virtualization, miklos, linux-kernel, virtio-fs,
	dgilbert, mst

[-- Attachment #1: Type: text/plain, Size: 363 bytes --]

On Thu, Sep 05, 2019 at 03:48:50PM -0400, Vivek Goyal wrote:
> This just marks are the queues are connected and ready to accept the
> request.
> 
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
>  fs/fuse/virtio_fs.c | 19 ++++++++++++++++---
>  1 file changed, 16 insertions(+), 3 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH 10/18] virtiofs: Do not use device managed mem for virtio_fs and virtio_fs_vq
  2019-09-05 19:48 ` [PATCH 10/18] virtiofs: Do not use device managed mem for virtio_fs and virtio_fs_vq Vivek Goyal
@ 2019-09-06 10:56   ` Stefan Hajnoczi
  0 siblings, 0 replies; 62+ messages in thread
From: Stefan Hajnoczi @ 2019-09-06 10:56 UTC (permalink / raw)
  To: Vivek Goyal
  Cc: linux-fsdevel, virtualization, miklos, linux-kernel, virtio-fs,
	dgilbert, mst

[-- Attachment #1: Type: text/plain, Size: 810 bytes --]

On Thu, Sep 05, 2019 at 03:48:51PM -0400, Vivek Goyal wrote:
> These data structures should go away when virtio_fs object is going away.
> When deivce is going away, we need to just make sure virtqueues can go
> away and after that none of the code accesses vq and all the requests
> get error.
> 
> So allocate memory for virtio_fs and virtio_fs_vq normally and free it
> at right time.
> 
> This patch still frees up memory during device remove time. A later patch
> will make virtio_fs object reference counted and this memory will be
> freed when last reference to object is dropped.
> 
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
>  fs/fuse/virtio_fs.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH 11/18] virtiofs: stop and drain queues after sending DESTROY
  2019-09-05 19:48 ` [PATCH 11/18] virtiofs: stop and drain queues after sending DESTROY Vivek Goyal
@ 2019-09-06 11:50   ` Stefan Hajnoczi
  0 siblings, 0 replies; 62+ messages in thread
From: Stefan Hajnoczi @ 2019-09-06 11:50 UTC (permalink / raw)
  To: Vivek Goyal
  Cc: linux-fsdevel, virtualization, miklos, linux-kernel, virtio-fs,
	dgilbert, mst

[-- Attachment #1: Type: text/plain, Size: 661 bytes --]

On Thu, Sep 05, 2019 at 03:48:52PM -0400, Vivek Goyal wrote:
> During virtio_kill_sb() we first stop forget queue and drain it and then
> call fuse_kill_sb_anon(). This will result in sending DESTROY request to
> fuse server. Once finished, stop all the queues and drain one more time
> just to be sure and then free up the devices.
> 
> Given drain queues will call flush_work() on various workers, remove this
> logic from virtio_free_devs().
> 
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
>  fs/fuse/virtio_fs.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH 12/18] virtiofs: Use virtio_fs_free_devs() in error path
  2019-09-05 19:48 ` [PATCH 12/18] virtiofs: Use virtio_fs_free_devs() in error path Vivek Goyal
@ 2019-09-06 11:51   ` Stefan Hajnoczi
  0 siblings, 0 replies; 62+ messages in thread
From: Stefan Hajnoczi @ 2019-09-06 11:51 UTC (permalink / raw)
  To: Vivek Goyal
  Cc: linux-fsdevel, virtualization, miklos, linux-kernel, virtio-fs,
	dgilbert, mst

[-- Attachment #1: Type: text/plain, Size: 359 bytes --]

On Thu, Sep 05, 2019 at 03:48:53PM -0400, Vivek Goyal wrote:
> We already have an helper to cleanup fuse devices. Use that instead of
> duplicating the code.
> 
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
>  fs/fuse/virtio_fs.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH 13/18] virtiofs: Do not access virtqueue in request submission path
  2019-09-05 19:48 ` [PATCH 13/18] virtiofs: Do not access virtqueue in request submission path Vivek Goyal
@ 2019-09-06 11:52   ` Stefan Hajnoczi
  0 siblings, 0 replies; 62+ messages in thread
From: Stefan Hajnoczi @ 2019-09-06 11:52 UTC (permalink / raw)
  To: Vivek Goyal
  Cc: linux-fsdevel, virtualization, miklos, linux-kernel, virtio-fs,
	dgilbert, mst

[-- Attachment #1: Type: text/plain, Size: 505 bytes --]

On Thu, Sep 05, 2019 at 03:48:54PM -0400, Vivek Goyal wrote:
> In request submission path it is possible that virtqueue is already gone
> due to driver->remove(). So do not access it in dev_dbg(). Use pr_debug()
> instead.
> 
> If virtuqueue is gone, this will result in NULL pointer deference.
> 
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
>  fs/fuse/virtio_fs.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH 00/18] virtiofs: Fix various races and cleanups round 1
  2019-09-06 10:36   ` Stefan Hajnoczi
@ 2019-09-06 11:52     ` Miklos Szeredi
  2019-09-06 12:08       ` Vivek Goyal
  2019-09-08 11:53       ` [Virtio-fs] " piaojun
  0 siblings, 2 replies; 62+ messages in thread
From: Miklos Szeredi @ 2019-09-06 11:52 UTC (permalink / raw)
  To: Stefan Hajnoczi
  Cc: Vivek Goyal, linux-fsdevel, virtualization, linux-kernel,
	virtio-fs, Dr. David Alan Gilbert, Michael S. Tsirkin

On Fri, Sep 6, 2019 at 12:36 PM Stefan Hajnoczi <stefanha@redhat.com> wrote:
>
> On Fri, Sep 06, 2019 at 10:15:14AM +0200, Miklos Szeredi wrote:
> > On Thu, Sep 5, 2019 at 9:49 PM Vivek Goyal <vgoyal@redhat.com> wrote:
> > >
> > > Hi,
> > >
> > > Michael Tsirkin pointed out issues w.r.t various locking related TODO
> > > items and races w.r.t device removal.
> > >
> > > In this first round of cleanups, I have taken care of most pressing
> > > issues.
> > >
> > > These patches apply on top of following.
> > >
> > > git://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/fuse.git#virtiofs-v4
> > >
> > > I have tested these patches with mount/umount and device removal using
> > > qemu monitor. For example.
> >
> > Is device removal mandatory?  Can't this be made a non-removable
> > device?  Is there a good reason why removing the virtio-fs device
> > makes sense?
>
> Hot plugging and unplugging virtio PCI adapters is common.  I'd very
> much like removal to work from the beginning.

Can you give an example use case?

Thanks,
Miklos

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH 14/18] virtiofs: Add a fuse_iqueue operation to put() reference
  2019-09-05 19:48 ` [PATCH 14/18] virtiofs: Add a fuse_iqueue operation to put() reference Vivek Goyal
@ 2019-09-06 12:00   ` Stefan Hajnoczi
  2019-09-06 13:35     ` Vivek Goyal
  0 siblings, 1 reply; 62+ messages in thread
From: Stefan Hajnoczi @ 2019-09-06 12:00 UTC (permalink / raw)
  To: Vivek Goyal
  Cc: linux-fsdevel, virtualization, miklos, linux-kernel, virtio-fs,
	dgilbert, mst

[-- Attachment #1: Type: text/plain, Size: 881 bytes --]

On Thu, Sep 05, 2019 at 03:48:55PM -0400, Vivek Goyal wrote:
> diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
> index 85e2dcad68c1..04e2c000d63f 100644
> --- a/fs/fuse/fuse_i.h
> +++ b/fs/fuse/fuse_i.h
> @@ -479,6 +479,11 @@ struct fuse_iqueue_ops {
>  	 */
>  	void (*wake_pending_and_unlock)(struct fuse_iqueue *fiq)
>  		__releases(fiq->waitq.lock);
> +
> +	/**
> +	 * Put a reference on fiq_priv.

I'm a bit confused about fiq->priv's role in this.  The callback takes
struct fuse_iqueue *fiq as the argument, not void *priv, so it could
theoretically do more than just release priv.

I think one of the following would be clearer:

 /**
  * Drop a reference to fiq->priv.
  */
 void (*put_priv)(void *priv);

Or:

 /**
  * Clean up when fuse_iqueue is destroyed.
  */
 void (*release)(struct fuse_iqueue *fiq);

In the second case fuse_conn_put() shouldn't check fiq->priv.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH 15/18] virtiofs: Make virtio_fs object refcounted
  2019-09-05 19:48 ` [PATCH 15/18] virtiofs: Make virtio_fs object refcounted Vivek Goyal
@ 2019-09-06 12:03   ` Stefan Hajnoczi
  2019-09-06 13:50     ` Vivek Goyal
  2019-09-08 11:10   ` [Virtio-fs] " piaojun
  1 sibling, 1 reply; 62+ messages in thread
From: Stefan Hajnoczi @ 2019-09-06 12:03 UTC (permalink / raw)
  To: Vivek Goyal
  Cc: linux-fsdevel, virtualization, miklos, linux-kernel, virtio-fs,
	dgilbert, mst

[-- Attachment #1: Type: text/plain, Size: 2101 bytes --]

On Thu, Sep 05, 2019 at 03:48:56PM -0400, Vivek Goyal wrote:
> This object is used both by fuse_connection as well virt device. So make
> this object reference counted and that makes it easy to define life cycle
> of the object.
> 
> Now deivce can be removed while filesystem is still mounted. This will
> cleanup all the virtqueues but virtio_fs object will still be around and
> will be cleaned when filesystem is unmounted and sb/fc drops its reference.
> 
> Removing a device also stops all virt queues and any new reuqest gets
> error -ENOTCONN. All existing in flight requests are drained before
> ->remove returns.
> 
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
>  fs/fuse/virtio_fs.c | 52 +++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 43 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c
> index 01bbf2c0e144..29ec2f5bbbe2 100644
> --- a/fs/fuse/virtio_fs.c
> +++ b/fs/fuse/virtio_fs.c
> @@ -37,6 +37,7 @@ struct virtio_fs_vq {
>  
>  /* A virtio-fs device instance */
>  struct virtio_fs {
> +	struct kref refcount;
>  	struct list_head list;    /* on virtio_fs_instances */
>  	char *tag;
>  	struct virtio_fs_vq *vqs;
> @@ -63,6 +64,27 @@ static inline struct fuse_pqueue *vq_to_fpq(struct virtqueue *vq)
>  	return &vq_to_fsvq(vq)->fud->pq;
>  }
>  
> +static void release_virtiofs_obj(struct kref *ref)
> +{
> +	struct virtio_fs *vfs = container_of(ref, struct virtio_fs, refcount);
> +
> +	kfree(vfs->vqs);
> +	kfree(vfs);
> +}
> +
> +static void virtiofs_put(struct virtio_fs *fs)

Why do the two function names above contain "virtiofs" instead
of "virtio_fs"?  I'm not sure if this is intentional and is supposed to
mean something, but it's confusing.

> +{
> +	mutex_lock(&virtio_fs_mutex);
> +	kref_put(&fs->refcount, release_virtiofs_obj);
> +	mutex_unlock(&virtio_fs_mutex);
> +}
> +
> +static void virtio_fs_put(struct fuse_iqueue *fiq)

Minor issue: this function name is confusingly similar to
virtiofs_put().  Please rename to virtio_fs_fiq_put().

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH 16/18] virtiofs: Use virtio_fs_mutex for races w.r.t ->remove and mount path
  2019-09-05 19:48 ` [PATCH 16/18] virtiofs: Use virtio_fs_mutex for races w.r.t ->remove and mount path Vivek Goyal
@ 2019-09-06 12:05   ` Stefan Hajnoczi
  2019-09-06 13:51     ` Vivek Goyal
  2019-09-08 11:19   ` [Virtio-fs] " piaojun
  1 sibling, 1 reply; 62+ messages in thread
From: Stefan Hajnoczi @ 2019-09-06 12:05 UTC (permalink / raw)
  To: Vivek Goyal
  Cc: linux-fsdevel, virtualization, miklos, linux-kernel, virtio-fs,
	dgilbert, mst

[-- Attachment #1: Type: text/plain, Size: 684 bytes --]

On Thu, Sep 05, 2019 at 03:48:57PM -0400, Vivek Goyal wrote:
> It is possible that a mount is in progress and device is being removed at
> the same time. Use virtio_fs_mutex to avoid races.
> 
> This also takes care of bunch of races and removes some TODO items.
> 
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
>  fs/fuse/virtio_fs.c | 32 ++++++++++++++++++++++----------
>  1 file changed, 22 insertions(+), 10 deletions(-)

Let's move to a per-device mutex in the future.  That way a single
device that fails to drain/complete requests will not hang all other
virtio-fs instances.  This is fine for now.

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH 17/18] virtiofs: Remove TODO to quiesce/end_requests
  2019-09-05 19:48 ` [PATCH 17/18] virtiofs: Remove TODO to quiesce/end_requests Vivek Goyal
@ 2019-09-06 12:06   ` Stefan Hajnoczi
  0 siblings, 0 replies; 62+ messages in thread
From: Stefan Hajnoczi @ 2019-09-06 12:06 UTC (permalink / raw)
  To: Vivek Goyal
  Cc: linux-fsdevel, virtualization, miklos, linux-kernel, virtio-fs,
	dgilbert, mst

[-- Attachment #1: Type: text/plain, Size: 454 bytes --]

On Thu, Sep 05, 2019 at 03:48:58PM -0400, Vivek Goyal wrote:
> We now stop queues and drain all the pending requests from all virtqueues.
> So this is not a TODO anymore.
> 
> Got rid of incrementing fc->dev_count as well. It did not seem meaningful
> for virtio_fs.
> 
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
>  fs/fuse/virtio_fs.c | 2 --
>  1 file changed, 2 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH 18/18] virtiofs: Remove TODO item from virtio_fs_free_devs()
  2019-09-05 19:48 ` [PATCH 18/18] virtiofs: Remove TODO item from virtio_fs_free_devs() Vivek Goyal
@ 2019-09-06 12:06   ` Stefan Hajnoczi
  0 siblings, 0 replies; 62+ messages in thread
From: Stefan Hajnoczi @ 2019-09-06 12:06 UTC (permalink / raw)
  To: Vivek Goyal
  Cc: linux-fsdevel, virtualization, miklos, linux-kernel, virtio-fs,
	dgilbert, mst

[-- Attachment #1: Type: text/plain, Size: 761 bytes --]

On Thu, Sep 05, 2019 at 03:48:59PM -0400, Vivek Goyal wrote:
> virtio_fs_free_devs() is now called from ->kill_sb(). By this time
> all device queues have been quiesced. I am assuming that while
> ->kill_sb() is in progress, another mount instance will wait for
> it to finish (sb->s_umount mutex provides mutual exclusion).
> 
> W.r.t ->remove path, we should be fine as we are not touching vdev
> or virtqueues. And we have reference on virtio_fs object, so we know
> rest of the data structures are valid.
> 
> So I can't see the need of any additional locking yet.
> 
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
>  fs/fuse/virtio_fs.c | 2 --
>  1 file changed, 2 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH 00/18] virtiofs: Fix various races and cleanups round 1
  2019-09-06 11:52     ` Miklos Szeredi
@ 2019-09-06 12:08       ` Vivek Goyal
  2019-09-06 13:55         ` Miklos Szeredi
  2019-09-06 13:57         ` Michael S. Tsirkin
  2019-09-08 11:53       ` [Virtio-fs] " piaojun
  1 sibling, 2 replies; 62+ messages in thread
From: Vivek Goyal @ 2019-09-06 12:08 UTC (permalink / raw)
  To: Miklos Szeredi
  Cc: Stefan Hajnoczi, linux-fsdevel, virtualization, linux-kernel,
	virtio-fs, Dr. David Alan Gilbert, Michael S. Tsirkin

On Fri, Sep 06, 2019 at 01:52:41PM +0200, Miklos Szeredi wrote:
> On Fri, Sep 6, 2019 at 12:36 PM Stefan Hajnoczi <stefanha@redhat.com> wrote:
> >
> > On Fri, Sep 06, 2019 at 10:15:14AM +0200, Miklos Szeredi wrote:
> > > On Thu, Sep 5, 2019 at 9:49 PM Vivek Goyal <vgoyal@redhat.com> wrote:
> > > >
> > > > Hi,
> > > >
> > > > Michael Tsirkin pointed out issues w.r.t various locking related TODO
> > > > items and races w.r.t device removal.
> > > >
> > > > In this first round of cleanups, I have taken care of most pressing
> > > > issues.
> > > >
> > > > These patches apply on top of following.
> > > >
> > > > git://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/fuse.git#virtiofs-v4
> > > >
> > > > I have tested these patches with mount/umount and device removal using
> > > > qemu monitor. For example.
> > >
> > > Is device removal mandatory?  Can't this be made a non-removable
> > > device?  Is there a good reason why removing the virtio-fs device
> > > makes sense?
> >
> > Hot plugging and unplugging virtio PCI adapters is common.  I'd very
> > much like removal to work from the beginning.
> 
> Can you give an example use case?

David Gilbert mentioned this could be useful if daemon stops responding
or dies. One could remove device. That will fail all future requests
and allow unmounting filesystem.

Havind said that, current implementation will help in above situation
only if there are no pending requests. If there are pending requests
and daemon stops responding, then removal will hang too, as we wait
for draining the queues.

So at some point of time, we also need some sort of timeout functionality
where we end requests with error after a timeout.

I feel we should support removing device at some point of time. But its
not necessarily a must have feature for first round.

Thanks
Vivek

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH 14/18] virtiofs: Add a fuse_iqueue operation to put() reference
  2019-09-06 12:00   ` Stefan Hajnoczi
@ 2019-09-06 13:35     ` Vivek Goyal
  0 siblings, 0 replies; 62+ messages in thread
From: Vivek Goyal @ 2019-09-06 13:35 UTC (permalink / raw)
  To: Stefan Hajnoczi
  Cc: linux-fsdevel, virtualization, miklos, linux-kernel, virtio-fs,
	dgilbert, mst

On Fri, Sep 06, 2019 at 01:00:09PM +0100, Stefan Hajnoczi wrote:
> On Thu, Sep 05, 2019 at 03:48:55PM -0400, Vivek Goyal wrote:
> > diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
> > index 85e2dcad68c1..04e2c000d63f 100644
> > --- a/fs/fuse/fuse_i.h
> > +++ b/fs/fuse/fuse_i.h
> > @@ -479,6 +479,11 @@ struct fuse_iqueue_ops {
> >  	 */
> >  	void (*wake_pending_and_unlock)(struct fuse_iqueue *fiq)
> >  		__releases(fiq->waitq.lock);
> > +
> > +	/**
> > +	 * Put a reference on fiq_priv.
> 
> I'm a bit confused about fiq->priv's role in this.  The callback takes
> struct fuse_iqueue *fiq as the argument, not void *priv, so it could
> theoretically do more than just release priv.
> 
> I think one of the following would be clearer:
> 
>  /**
>   * Drop a reference to fiq->priv.
>   */
>  void (*put_priv)(void *priv);
> 
> Or:
> 
>  /**
>   * Clean up when fuse_iqueue is destroyed.
>   */
>  void (*release)(struct fuse_iqueue *fiq);
> 
> In the second case fuse_conn_put() shouldn't check fiq->priv.

Hi Stefan,

I like using ->release() method sounds better. Here is the updated patch.
This also changes the next patch, so will post that as well.

Thanks
Vivek

Subject: virtiofs: Add a fuse_iqueue operation to put() reference

Soon I will make virtio_fs object reference counted, where reference will
be taken by device as well as by fuse_conn (fuse_conn->fuse_iqueue->fiq_priv).

When fuse_connection is going away, it should put its reference on virtio_fs
object.

So add a fuse_iqueue method which can be used to call into virtio_fs to
put the reference on the object (fiq_priv).

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 fs/fuse/fuse_i.h |    5 +++++
 fs/fuse/inode.c  |    4 ++++
 2 files changed, 9 insertions(+)

Index: rhvgoyal-linux-fuse/fs/fuse/fuse_i.h
===================================================================
--- rhvgoyal-linux-fuse.orig/fs/fuse/fuse_i.h	2019-09-06 08:46:41.846086544 -0400
+++ rhvgoyal-linux-fuse/fs/fuse/fuse_i.h	2019-09-06 09:24:32.752245246 -0400
@@ -479,6 +479,11 @@ struct fuse_iqueue_ops {
 	 */
 	void (*wake_pending_and_unlock)(struct fuse_iqueue *fiq)
 		__releases(fiq->waitq.lock);
+
+	/**
+	 * Cleanup up when fuse_iqueue is destroyed
+	 */
+	void (*release)(struct fuse_iqueue *fiq);
 };
 
 /** /dev/fuse input queue operations */
Index: rhvgoyal-linux-fuse/fs/fuse/inode.c
===================================================================
--- rhvgoyal-linux-fuse.orig/fs/fuse/inode.c	2019-09-06 08:46:41.847086544 -0400
+++ rhvgoyal-linux-fuse/fs/fuse/inode.c	2019-09-06 09:24:32.754245246 -0400
@@ -631,8 +631,12 @@ EXPORT_SYMBOL_GPL(fuse_conn_init);
 void fuse_conn_put(struct fuse_conn *fc)
 {
 	if (refcount_dec_and_test(&fc->count)) {
+		struct fuse_iqueue *fiq = &fc->iq;
+
 		if (fc->destroy_req)
 			fuse_request_free(fc->destroy_req);
+		if (fiq->ops->release)
+			fiq->ops->release(fiq);
 		put_pid_ns(fc->pid_ns);
 		put_user_ns(fc->user_ns);
 		fc->release(fc);



^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH 00/18] virtiofs: Fix various races and cleanups round 1
  2019-09-05 19:48 [PATCH 00/18] virtiofs: Fix various races and cleanups round 1 Vivek Goyal
                   ` (18 preceding siblings ...)
  2019-09-06  8:15 ` [PATCH 00/18] virtiofs: Fix various races and cleanups round 1 Miklos Szeredi
@ 2019-09-06 13:37 ` Michael S. Tsirkin
  19 siblings, 0 replies; 62+ messages in thread
From: Michael S. Tsirkin @ 2019-09-06 13:37 UTC (permalink / raw)
  To: Vivek Goyal
  Cc: linux-fsdevel, virtualization, miklos, linux-kernel, virtio-fs,
	stefanha, dgilbert

On Thu, Sep 05, 2019 at 03:48:41PM -0400, Vivek Goyal wrote:
> Hi,
> 
> Michael Tsirkin pointed out issues w.r.t various locking related TODO
> items and races w.r.t device removal. 
> 
> In this first round of cleanups, I have taken care of most pressing
> issues.
> 
> These patches apply on top of following.
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/fuse.git#virtiofs-v4
> 
> I have tested these patches with mount/umount and device removal using
> qemu monitor. For example.
> 
> virsh qemu-monitor-command --hmp vm9-f28 device_del myvirtiofs
> 
> Now a mounted device can be removed and file system will return errors
> -ENOTCONN and one can unmount it.
> 
> Miklos, if you are fine with the patches, I am fine if you fold these
> all into existing patch. I kept them separate so that review is easier.
> 
> Any feedback or comments are welcome.
> 
> Thanks
> Vivek
> 


Another version of a patch with fixes all rolled in would also
be nice.
> Vivek Goyal (18):
>   virtiofs: Remove request from processing list before calling end
>   virtiofs: Check whether hiprio queue is connected at submission time
>   virtiofs: Pass fsvq instead of vq as parameter to
>     virtio_fs_enqueue_req
>   virtiofs: Check connected state for VQ_REQUEST queue as well
>   Maintain count of in flight requests for VQ_REQUEST queue
>   virtiofs: ->remove should not clean virtiofs fuse devices
>   virtiofs: Stop virtiofs queues when device is being removed
>   virtiofs: Drain all pending requests during ->remove time
>   virtiofs: Add an helper to start all the queues
>   virtiofs: Do not use device managed mem for virtio_fs and virtio_fs_vq
>   virtiofs: stop and drain queues after sending DESTROY
>   virtiofs: Use virtio_fs_free_devs() in error path
>   virtiofs: Do not access virtqueue in request submission path
>   virtiofs: Add a fuse_iqueue operation to put() reference
>   virtiofs: Make virtio_fs object refcounted
>   virtiofs: Use virtio_fs_mutex for races w.r.t ->remove and mount path
>   virtiofs: Remove TODO to quiesce/end_requests
>   virtiofs: Remove TODO item from virtio_fs_free_devs()
> 
>  fs/fuse/fuse_i.h    |   5 +
>  fs/fuse/inode.c     |   6 +
>  fs/fuse/virtio_fs.c | 259 ++++++++++++++++++++++++++++++++------------
>  3 files changed, 198 insertions(+), 72 deletions(-)
> 
> -- 
> 2.20.1

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH 15/18] virtiofs: Make virtio_fs object refcounted
  2019-09-06 12:03   ` Stefan Hajnoczi
@ 2019-09-06 13:50     ` Vivek Goyal
  2019-09-09 16:12       ` Stefan Hajnoczi
  0 siblings, 1 reply; 62+ messages in thread
From: Vivek Goyal @ 2019-09-06 13:50 UTC (permalink / raw)
  To: Stefan Hajnoczi
  Cc: linux-fsdevel, virtualization, miklos, linux-kernel, virtio-fs,
	dgilbert, mst

On Fri, Sep 06, 2019 at 01:03:09PM +0100, Stefan Hajnoczi wrote:
> On Thu, Sep 05, 2019 at 03:48:56PM -0400, Vivek Goyal wrote:
> > This object is used both by fuse_connection as well virt device. So make
> > this object reference counted and that makes it easy to define life cycle
> > of the object.
> > 
> > Now deivce can be removed while filesystem is still mounted. This will
> > cleanup all the virtqueues but virtio_fs object will still be around and
> > will be cleaned when filesystem is unmounted and sb/fc drops its reference.
> > 
> > Removing a device also stops all virt queues and any new reuqest gets
> > error -ENOTCONN. All existing in flight requests are drained before
> > ->remove returns.
> > 
> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > ---
> >  fs/fuse/virtio_fs.c | 52 +++++++++++++++++++++++++++++++++++++--------
> >  1 file changed, 43 insertions(+), 9 deletions(-)
> > 
> > diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c
> > index 01bbf2c0e144..29ec2f5bbbe2 100644
> > --- a/fs/fuse/virtio_fs.c
> > +++ b/fs/fuse/virtio_fs.c
> > @@ -37,6 +37,7 @@ struct virtio_fs_vq {
> >  
> >  /* A virtio-fs device instance */
> >  struct virtio_fs {
> > +	struct kref refcount;
> >  	struct list_head list;    /* on virtio_fs_instances */
> >  	char *tag;
> >  	struct virtio_fs_vq *vqs;
> > @@ -63,6 +64,27 @@ static inline struct fuse_pqueue *vq_to_fpq(struct virtqueue *vq)
> >  	return &vq_to_fsvq(vq)->fud->pq;
> >  }
> >  
> > +static void release_virtiofs_obj(struct kref *ref)
> > +{
> > +	struct virtio_fs *vfs = container_of(ref, struct virtio_fs, refcount);
> > +
> > +	kfree(vfs->vqs);
> > +	kfree(vfs);
> > +}
> > +
> > +static void virtiofs_put(struct virtio_fs *fs)
> 
> Why do the two function names above contain "virtiofs" instead
> of "virtio_fs"?  I'm not sure if this is intentional and is supposed to
> mean something, but it's confusing.
> 
> > +{
> > +	mutex_lock(&virtio_fs_mutex);
> > +	kref_put(&fs->refcount, release_virtiofs_obj);
> > +	mutex_unlock(&virtio_fs_mutex);
> > +}
> > +
> > +static void virtio_fs_put(struct fuse_iqueue *fiq)
> 
> Minor issue: this function name is confusingly similar to
> virtiofs_put().  Please rename to virtio_fs_fiq_put().

Fixed with ->release semantics. Replaced "virtiofs" with "virtio_fs".


Subject: virtiofs: Make virtio_fs object refcounted

This object is used both by fuse_connection as well virt device. So make
this object reference counted and that makes it easy to define life cycle
of the object. 

Now deivce can be removed while filesystem is still mounted. This will
cleanup all the virtqueues but virtio_fs object will still be around and
will be cleaned when filesystem is unmounted and sb/fc drops its reference.

Removing a device also stops all virt queues and any new reuqest gets
error -ENOTCONN. All existing in flight requests are drained before
->remove returns.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 fs/fuse/virtio_fs.c |   52 +++++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 43 insertions(+), 9 deletions(-)

Index: rhvgoyal-linux-fuse/fs/fuse/virtio_fs.c
===================================================================
--- rhvgoyal-linux-fuse.orig/fs/fuse/virtio_fs.c	2019-09-06 09:24:21.177245246 -0400
+++ rhvgoyal-linux-fuse/fs/fuse/virtio_fs.c	2019-09-06 09:40:53.309245246 -0400
@@ -37,6 +37,7 @@ struct virtio_fs_vq {
 
 /* A virtio-fs device instance */
 struct virtio_fs {
+	struct kref refcount;
 	struct list_head list;    /* on virtio_fs_instances */
 	char *tag;
 	struct virtio_fs_vq *vqs;
@@ -63,6 +64,27 @@ static inline struct fuse_pqueue *vq_to_
 	return &vq_to_fsvq(vq)->fud->pq;
 }
 
+static void release_virtio_fs_obj(struct kref *ref)
+{
+	struct virtio_fs *vfs = container_of(ref, struct virtio_fs, refcount);
+
+	kfree(vfs->vqs);
+	kfree(vfs);
+}
+
+static void virtio_fs_put(struct virtio_fs *fs)
+{
+	mutex_lock(&virtio_fs_mutex);
+	kref_put(&fs->refcount, release_virtio_fs_obj);
+	mutex_unlock(&virtio_fs_mutex);
+}
+
+static void virtio_fs_fiq_release(struct fuse_iqueue *fiq)
+{
+	struct virtio_fs *vfs = fiq->priv;
+	virtio_fs_put(vfs);
+}
+
 static void virtio_fs_drain_queue(struct virtio_fs_vq *fsvq)
 {
 	WARN_ON(fsvq->in_flight < 0);
@@ -156,8 +178,10 @@ static struct virtio_fs *virtio_fs_find_
 	mutex_lock(&virtio_fs_mutex);
 
 	list_for_each_entry(fs, &virtio_fs_instances, list) {
-		if (strcmp(fs->tag, tag) == 0)
+		if (strcmp(fs->tag, tag) == 0) {
+			kref_get(&fs->refcount);
 			goto found;
+		}
 	}
 
 	fs = NULL; /* not found */
@@ -519,6 +543,7 @@ static int virtio_fs_probe(struct virtio
 	fs = kzalloc(sizeof(*fs), GFP_KERNEL);
 	if (!fs)
 		return -ENOMEM;
+	kref_init(&fs->refcount);
 	vdev->priv = fs;
 
 	ret = virtio_fs_read_tag(vdev, fs);
@@ -570,18 +595,18 @@ static void virtio_fs_remove(struct virt
 {
 	struct virtio_fs *fs = vdev->priv;
 
+	mutex_lock(&virtio_fs_mutex);
+	list_del_init(&fs->list);
+	mutex_unlock(&virtio_fs_mutex);
+
 	virtio_fs_stop_all_queues(fs);
 	virtio_fs_drain_all_queues(fs);
 	vdev->config->reset(vdev);
 	virtio_fs_cleanup_vqs(vdev, fs);
 
-	mutex_lock(&virtio_fs_mutex);
-	list_del(&fs->list);
-	mutex_unlock(&virtio_fs_mutex);
-
 	vdev->priv = NULL;
-	kfree(fs->vqs);
-	kfree(fs);
+	/* Put device reference on virtio_fs object */
+	virtio_fs_put(fs);
 }
 
 #ifdef CONFIG_PM_SLEEP
@@ -932,6 +957,7 @@ const static struct fuse_iqueue_ops virt
 	.wake_forget_and_unlock		= virtio_fs_wake_forget_and_unlock,
 	.wake_interrupt_and_unlock	= virtio_fs_wake_interrupt_and_unlock,
 	.wake_pending_and_unlock	= virtio_fs_wake_pending_and_unlock,
+	.release			= virtio_fs_fiq_release,
 };
 
 static int virtio_fs_fill_super(struct super_block *sb)
@@ -1026,7 +1052,9 @@ static void virtio_kill_sb(struct super_
 	fuse_kill_sb_anon(sb);
 
 	/* fuse_kill_sb_anon() must have sent destroy. Stop all queues
-	 * and drain one more time and free fuse devices.
+	 * and drain one more time and free fuse devices. Freeing fuse
+	 * devices will drop their reference on fuse_conn and that in
+	 * turn will drop its reference on virtio_fs object.
 	 */
 	virtio_fs_stop_all_queues(vfs);
 	virtio_fs_drain_all_queues(vfs);
@@ -1060,6 +1088,10 @@ static int virtio_fs_get_tree(struct fs_
 	struct fuse_conn *fc;
 	int err;
 
+	/* This gets a reference on virtio_fs object. This ptr gets installed
+	 * in fc->iq->priv. Once fuse_conn is going away, it calls ->put()
+	 * to drop the reference to this object.
+	 */
 	fs = virtio_fs_find_instance(fsc->source);
 	if (!fs) {
 		pr_info("virtio-fs: tag <%s> not found\n", fsc->source);
@@ -1067,8 +1099,10 @@ static int virtio_fs_get_tree(struct fs_
 	}
 
 	fc = kzalloc(sizeof(struct fuse_conn), GFP_KERNEL);
-	if (!fc)
+	if (!fc) {
+		virtio_fs_put(fs);
 		return -ENOMEM;
+	}
 
 	fuse_conn_init(fc, get_user_ns(current_user_ns()), &virtio_fs_fiq_ops,
 		       fs);

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH 16/18] virtiofs: Use virtio_fs_mutex for races w.r.t ->remove and mount path
  2019-09-06 12:05   ` Stefan Hajnoczi
@ 2019-09-06 13:51     ` Vivek Goyal
  2019-09-09 16:13       ` Stefan Hajnoczi
  0 siblings, 1 reply; 62+ messages in thread
From: Vivek Goyal @ 2019-09-06 13:51 UTC (permalink / raw)
  To: Stefan Hajnoczi
  Cc: linux-fsdevel, virtualization, miklos, linux-kernel, virtio-fs,
	dgilbert, mst

On Fri, Sep 06, 2019 at 01:05:34PM +0100, Stefan Hajnoczi wrote:
> On Thu, Sep 05, 2019 at 03:48:57PM -0400, Vivek Goyal wrote:
> > It is possible that a mount is in progress and device is being removed at
> > the same time. Use virtio_fs_mutex to avoid races.
> > 
> > This also takes care of bunch of races and removes some TODO items.
> > 
> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > ---
> >  fs/fuse/virtio_fs.c | 32 ++++++++++++++++++++++----------
> >  1 file changed, 22 insertions(+), 10 deletions(-)
> 
> Let's move to a per-device mutex in the future.  That way a single
> device that fails to drain/complete requests will not hang all other
> virtio-fs instances.  This is fine for now.

Good point. For now I updated the patch so that it applies cleanly
after previous two patches changed.

Subject: virtiofs: Use virtio_fs_mutex for races w.r.t ->remove and mount path

It is possible that a mount is in progress and device is being removed at
the same time. Use virtio_fs_mutex to avoid races. 

This also takes care of bunch of races and removes some TODO items.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 fs/fuse/virtio_fs.c |   32 ++++++++++++++++++++++----------
 1 file changed, 22 insertions(+), 10 deletions(-)

Index: rhvgoyal-linux-fuse/fs/fuse/virtio_fs.c
===================================================================
--- rhvgoyal-linux-fuse.orig/fs/fuse/virtio_fs.c	2019-09-06 09:40:53.309245246 -0400
+++ rhvgoyal-linux-fuse/fs/fuse/virtio_fs.c	2019-09-06 09:43:25.335245246 -0400
@@ -13,7 +13,9 @@
 #include <linux/highmem.h>
 #include "fuse_i.h"
 
-/* List of virtio-fs device instances and a lock for the list */
+/* List of virtio-fs device instances and a lock for the list. Also provides
+ * mutual exclusion in device removal and mounting path
+ */
 static DEFINE_MUTEX(virtio_fs_mutex);
 static LIST_HEAD(virtio_fs_instances);
 
@@ -72,17 +74,19 @@ static void release_virtio_fs_obj(struct
 	kfree(vfs);
 }
 
+/* Make sure virtiofs_mutex is held */
 static void virtio_fs_put(struct virtio_fs *fs)
 {
-	mutex_lock(&virtio_fs_mutex);
 	kref_put(&fs->refcount, release_virtio_fs_obj);
-	mutex_unlock(&virtio_fs_mutex);
 }
 
 static void virtio_fs_fiq_release(struct fuse_iqueue *fiq)
 {
 	struct virtio_fs *vfs = fiq->priv;
+
+	mutex_lock(&virtio_fs_mutex);
 	virtio_fs_put(vfs);
+	mutex_unlock(&virtio_fs_mutex);
 }
 
 static void virtio_fs_drain_queue(struct virtio_fs_vq *fsvq)
@@ -596,9 +600,8 @@ static void virtio_fs_remove(struct virt
 	struct virtio_fs *fs = vdev->priv;
 
 	mutex_lock(&virtio_fs_mutex);
+	/* This device is going away. No one should get new reference */
 	list_del_init(&fs->list);
-	mutex_unlock(&virtio_fs_mutex);
-
 	virtio_fs_stop_all_queues(fs);
 	virtio_fs_drain_all_queues(fs);
 	vdev->config->reset(vdev);
@@ -607,6 +610,7 @@ static void virtio_fs_remove(struct virt
 	vdev->priv = NULL;
 	/* Put device reference on virtio_fs object */
 	virtio_fs_put(fs);
+	mutex_unlock(&virtio_fs_mutex);
 }
 
 #ifdef CONFIG_PM_SLEEP
@@ -978,10 +982,15 @@ static int virtio_fs_fill_super(struct s
 		.no_force_umount = true,
 	};
 
-	/* TODO lock */
-	if (fs->vqs[VQ_REQUEST].fud) {
-		pr_err("virtio-fs: device already in use\n");
-		err = -EBUSY;
+	mutex_lock(&virtio_fs_mutex);
+
+	/* After holding mutex, make sure virtiofs device is still there.
+	 * Though we are holding a refernce to it, drive ->remove might
+	 * still have cleaned up virtual queues. In that case bail out.
+	 */
+	err = -EINVAL;
+	if (list_empty(&fs->list)) {
+		pr_info("virtio-fs: tag <%s> not found\n", fs->tag);
 		goto err;
 	}
 
@@ -1007,7 +1016,6 @@ static int virtio_fs_fill_super(struct s
 
 	fc = fs->vqs[VQ_REQUEST].fud->fc;
 
-	/* TODO take fuse_mutex around this loop? */
 	for (i = 0; i < fs->nvqs; i++) {
 		struct virtio_fs_vq *fsvq = &fs->vqs[i];
 
@@ -1020,6 +1028,7 @@ static int virtio_fs_fill_super(struct s
 	/* Previous unmount will stop all queues. Start these again */
 	virtio_fs_start_all_queues(fs);
 	fuse_send_init(fc, init_req);
+	mutex_unlock(&virtio_fs_mutex);
 	return 0;
 
 err_free_init_req:
@@ -1027,6 +1036,7 @@ err_free_init_req:
 err_free_fuse_devs:
 	virtio_fs_free_devs(fs);
 err:
+	mutex_unlock(&virtio_fs_mutex);
 	return err;
 }
 
@@ -1100,7 +1110,9 @@ static int virtio_fs_get_tree(struct fs_
 
 	fc = kzalloc(sizeof(struct fuse_conn), GFP_KERNEL);
 	if (!fc) {
+		mutex_lock(&virtio_fs_mutex);
 		virtio_fs_put(fs);
+		mutex_unlock(&virtio_fs_mutex);
 		return -ENOMEM;
 	}
 


^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH 00/18] virtiofs: Fix various races and cleanups round 1
  2019-09-06 12:08       ` Vivek Goyal
@ 2019-09-06 13:55         ` Miklos Szeredi
  2019-09-06 13:57         ` Michael S. Tsirkin
  1 sibling, 0 replies; 62+ messages in thread
From: Miklos Szeredi @ 2019-09-06 13:55 UTC (permalink / raw)
  To: Vivek Goyal
  Cc: Stefan Hajnoczi, linux-fsdevel, virtualization, linux-kernel,
	virtio-fs, Dr. David Alan Gilbert, Michael S. Tsirkin

On Fri, Sep 6, 2019 at 2:08 PM Vivek Goyal <vgoyal@redhat.com> wrote:
>
> On Fri, Sep 06, 2019 at 01:52:41PM +0200, Miklos Szeredi wrote:
> > On Fri, Sep 6, 2019 at 12:36 PM Stefan Hajnoczi <stefanha@redhat.com> wrote:
> > >
> > > On Fri, Sep 06, 2019 at 10:15:14AM +0200, Miklos Szeredi wrote:
> > > > On Thu, Sep 5, 2019 at 9:49 PM Vivek Goyal <vgoyal@redhat.com> wrote:
> > > > >
> > > > > Hi,
> > > > >
> > > > > Michael Tsirkin pointed out issues w.r.t various locking related TODO
> > > > > items and races w.r.t device removal.
> > > > >
> > > > > In this first round of cleanups, I have taken care of most pressing
> > > > > issues.
> > > > >
> > > > > These patches apply on top of following.
> > > > >
> > > > > git://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/fuse.git#virtiofs-v4
> > > > >
> > > > > I have tested these patches with mount/umount and device removal using
> > > > > qemu monitor. For example.
> > > >
> > > > Is device removal mandatory?  Can't this be made a non-removable
> > > > device?  Is there a good reason why removing the virtio-fs device
> > > > makes sense?
> > >
> > > Hot plugging and unplugging virtio PCI adapters is common.  I'd very
> > > much like removal to work from the beginning.
> >
> > Can you give an example use case?
>
> David Gilbert mentioned this could be useful if daemon stops responding
> or dies. One could remove device. That will fail all future requests
> and allow unmounting filesystem.
>
> Havind said that, current implementation will help in above situation
> only if there are no pending requests. If there are pending requests
> and daemon stops responding, then removal will hang too, as we wait
> for draining the queues.
>
> So at some point of time, we also need some sort of timeout functionality
> where we end requests with error after a timeout.
>
> I feel we should support removing device at some point of time. But its
> not necessarily a must have feature for first round.

If there's no compelling reason to do it in the first round, than I'd
prefer to not do it.   More complexity -> more bugs.

Thanks,
Miklos

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH 00/18] virtiofs: Fix various races and cleanups round 1
  2019-09-06 12:08       ` Vivek Goyal
  2019-09-06 13:55         ` Miklos Szeredi
@ 2019-09-06 13:57         ` Michael S. Tsirkin
  2019-09-06 14:11           ` Miklos Szeredi
  1 sibling, 1 reply; 62+ messages in thread
From: Michael S. Tsirkin @ 2019-09-06 13:57 UTC (permalink / raw)
  To: Vivek Goyal
  Cc: Miklos Szeredi, Stefan Hajnoczi, linux-fsdevel, virtualization,
	linux-kernel, virtio-fs, Dr. David Alan Gilbert

On Fri, Sep 06, 2019 at 08:08:17AM -0400, Vivek Goyal wrote:
> On Fri, Sep 06, 2019 at 01:52:41PM +0200, Miklos Szeredi wrote:
> > On Fri, Sep 6, 2019 at 12:36 PM Stefan Hajnoczi <stefanha@redhat.com> wrote:
> > >
> > > On Fri, Sep 06, 2019 at 10:15:14AM +0200, Miklos Szeredi wrote:
> > > > On Thu, Sep 5, 2019 at 9:49 PM Vivek Goyal <vgoyal@redhat.com> wrote:
> > > > >
> > > > > Hi,
> > > > >
> > > > > Michael Tsirkin pointed out issues w.r.t various locking related TODO
> > > > > items and races w.r.t device removal.
> > > > >
> > > > > In this first round of cleanups, I have taken care of most pressing
> > > > > issues.
> > > > >
> > > > > These patches apply on top of following.
> > > > >
> > > > > git://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/fuse.git#virtiofs-v4
> > > > >
> > > > > I have tested these patches with mount/umount and device removal using
> > > > > qemu monitor. For example.
> > > >
> > > > Is device removal mandatory?  Can't this be made a non-removable
> > > > device?  Is there a good reason why removing the virtio-fs device
> > > > makes sense?
> > >
> > > Hot plugging and unplugging virtio PCI adapters is common.  I'd very
> > > much like removal to work from the beginning.
> > 
> > Can you give an example use case?
> 
> David Gilbert mentioned this could be useful if daemon stops responding
> or dies. One could remove device. That will fail all future requests
> and allow unmounting filesystem.
> 
> Havind said that, current implementation will help in above situation
> only if there are no pending requests. If there are pending requests
> and daemon stops responding, then removal will hang too, as we wait
> for draining the queues.
> 
> So at some point of time, we also need some sort of timeout functionality
> where we end requests with error after a timeout.
> 
> I feel we should support removing device at some point of time. But its
> not necessarily a must have feature for first round.
> 
> Thanks
> Vivek

Without removal how do we stop guest poking at some files if we want to?

I guess we could invent a special event to block accesses,
but unplug will just do it.

blk and scsi support removal out of box, if this is supposed
to be a drop in replacement then I think yes, you want this
support.

-- 
MST

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH 00/18] virtiofs: Fix various races and cleanups round 1
  2019-09-06 13:57         ` Michael S. Tsirkin
@ 2019-09-06 14:11           ` Miklos Szeredi
  2019-09-06 14:17             ` Michael S. Tsirkin
  0 siblings, 1 reply; 62+ messages in thread
From: Miklos Szeredi @ 2019-09-06 14:11 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: Vivek Goyal, Stefan Hajnoczi, linux-fsdevel, virtualization,
	linux-kernel, virtio-fs, Dr. David Alan Gilbert

On Fri, Sep 6, 2019 at 3:57 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Fri, Sep 06, 2019 at 08:08:17AM -0400, Vivek Goyal wrote:
> > On Fri, Sep 06, 2019 at 01:52:41PM +0200, Miklos Szeredi wrote:
> > > On Fri, Sep 6, 2019 at 12:36 PM Stefan Hajnoczi <stefanha@redhat.com> wrote:
> > > >
> > > > On Fri, Sep 06, 2019 at 10:15:14AM +0200, Miklos Szeredi wrote:
> > > > > On Thu, Sep 5, 2019 at 9:49 PM Vivek Goyal <vgoyal@redhat.com> wrote:
> > > > > >
> > > > > > Hi,
> > > > > >
> > > > > > Michael Tsirkin pointed out issues w.r.t various locking related TODO
> > > > > > items and races w.r.t device removal.
> > > > > >
> > > > > > In this first round of cleanups, I have taken care of most pressing
> > > > > > issues.
> > > > > >
> > > > > > These patches apply on top of following.
> > > > > >
> > > > > > git://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/fuse.git#virtiofs-v4
> > > > > >
> > > > > > I have tested these patches with mount/umount and device removal using
> > > > > > qemu monitor. For example.
> > > > >
> > > > > Is device removal mandatory?  Can't this be made a non-removable
> > > > > device?  Is there a good reason why removing the virtio-fs device
> > > > > makes sense?
> > > >
> > > > Hot plugging and unplugging virtio PCI adapters is common.  I'd very
> > > > much like removal to work from the beginning.
> > >
> > > Can you give an example use case?
> >
> > David Gilbert mentioned this could be useful if daemon stops responding
> > or dies. One could remove device. That will fail all future requests
> > and allow unmounting filesystem.
> >
> > Havind said that, current implementation will help in above situation
> > only if there are no pending requests. If there are pending requests
> > and daemon stops responding, then removal will hang too, as we wait
> > for draining the queues.
> >
> > So at some point of time, we also need some sort of timeout functionality
> > where we end requests with error after a timeout.
> >
> > I feel we should support removing device at some point of time. But its
> > not necessarily a must have feature for first round.
> >
> > Thanks
> > Vivek
>
> Without removal how do we stop guest poking at some files if we want to?
>
> I guess we could invent a special event to block accesses,
> but unplug will just do it.
>
> blk and scsi support removal out of box, if this is supposed
> to be a drop in replacement then I think yes, you want this
> support.

This is not a drop in replacement for blk and scsi transports.  More
for virtio-9p.  Does that have anything similar?

If we get a request for this feature, then yes, what you are saying
makes sense.   But that hasn't happened yet, so I think this can wait.

Thanks,
Miklos

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH 08/18] virtiofs: Drain all pending requests during ->remove time
  2019-09-06 10:52   ` Stefan Hajnoczi
@ 2019-09-06 14:17     ` Vivek Goyal
  2019-09-06 14:18       ` Michael S. Tsirkin
  0 siblings, 1 reply; 62+ messages in thread
From: Vivek Goyal @ 2019-09-06 14:17 UTC (permalink / raw)
  To: Stefan Hajnoczi
  Cc: linux-fsdevel, virtualization, miklos, linux-kernel, virtio-fs,
	dgilbert, mst

On Fri, Sep 06, 2019 at 11:52:10AM +0100, Stefan Hajnoczi wrote:
> On Thu, Sep 05, 2019 at 03:48:49PM -0400, Vivek Goyal wrote:
> > +static void virtio_fs_drain_queue(struct virtio_fs_vq *fsvq)
> > +{
> > +	WARN_ON(fsvq->in_flight < 0);
> > +
> > +	/* Wait for in flight requests to finish.*/
> > +	while (1) {
> > +		spin_lock(&fsvq->lock);
> > +		if (!fsvq->in_flight) {
> > +			spin_unlock(&fsvq->lock);
> > +			break;
> > +		}
> > +		spin_unlock(&fsvq->lock);
> > +		usleep_range(1000, 2000);
> > +	}
> 
> I think all contexts that call this allow sleeping so we could avoid
> usleep here.

usleep_range() is supposed to be used from non-atomic context.

https://github.com/torvalds/linux/blob/master/Documentation/timers/timers-howto.rst

What construct you are thinking of?

Vivek

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH 00/18] virtiofs: Fix various races and cleanups round 1
  2019-09-06 14:11           ` Miklos Szeredi
@ 2019-09-06 14:17             ` Michael S. Tsirkin
  0 siblings, 0 replies; 62+ messages in thread
From: Michael S. Tsirkin @ 2019-09-06 14:17 UTC (permalink / raw)
  To: Miklos Szeredi
  Cc: Vivek Goyal, Stefan Hajnoczi, linux-fsdevel, virtualization,
	linux-kernel, virtio-fs, Dr. David Alan Gilbert

On Fri, Sep 06, 2019 at 04:11:45PM +0200, Miklos Szeredi wrote:
> This is not a drop in replacement for blk and scsi transports.  More
> for virtio-9p.  Does that have anything similar?

9p seems to supports unplug, yes. It's not great in that it
blocks until we close the channel, but it's there and
it does not crash or leak memory.

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH 08/18] virtiofs: Drain all pending requests during ->remove time
  2019-09-06 14:17     ` Vivek Goyal
@ 2019-09-06 14:18       ` Michael S. Tsirkin
  2019-09-09 16:10         ` Stefan Hajnoczi
  0 siblings, 1 reply; 62+ messages in thread
From: Michael S. Tsirkin @ 2019-09-06 14:18 UTC (permalink / raw)
  To: Vivek Goyal
  Cc: Stefan Hajnoczi, linux-fsdevel, virtualization, miklos,
	linux-kernel, virtio-fs, dgilbert

On Fri, Sep 06, 2019 at 10:17:05AM -0400, Vivek Goyal wrote:
> On Fri, Sep 06, 2019 at 11:52:10AM +0100, Stefan Hajnoczi wrote:
> > On Thu, Sep 05, 2019 at 03:48:49PM -0400, Vivek Goyal wrote:
> > > +static void virtio_fs_drain_queue(struct virtio_fs_vq *fsvq)
> > > +{
> > > +	WARN_ON(fsvq->in_flight < 0);
> > > +
> > > +	/* Wait for in flight requests to finish.*/
> > > +	while (1) {
> > > +		spin_lock(&fsvq->lock);
> > > +		if (!fsvq->in_flight) {
> > > +			spin_unlock(&fsvq->lock);
> > > +			break;
> > > +		}
> > > +		spin_unlock(&fsvq->lock);
> > > +		usleep_range(1000, 2000);
> > > +	}
> > 
> > I think all contexts that call this allow sleeping so we could avoid
> > usleep here.
> 
> usleep_range() is supposed to be used from non-atomic context.
> 
> https://github.com/torvalds/linux/blob/master/Documentation/timers/timers-howto.rst
> 
> What construct you are thinking of?
> 
> Vivek

completion + signal on vq callback?

-- 
MST

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [Virtio-fs] [PATCH 08/18] virtiofs: Drain all pending requests during ->remove time
  2019-09-05 19:48 ` [PATCH 08/18] virtiofs: Drain all pending requests during ->remove time Vivek Goyal
  2019-09-06 10:52   ` Stefan Hajnoczi
@ 2019-09-08 10:11   ` piaojun
  1 sibling, 0 replies; 62+ messages in thread
From: piaojun @ 2019-09-08 10:11 UTC (permalink / raw)
  To: Vivek Goyal, linux-fsdevel, virtualization, miklos
  Cc: mst, linux-kernel, virtio-fs



On 2019/9/6 3:48, Vivek Goyal wrote:
> When device is going away, drain all pending requests.
> 
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
>  fs/fuse/virtio_fs.c | 83 ++++++++++++++++++++++++++++-----------------
>  1 file changed, 51 insertions(+), 32 deletions(-)
> 
> diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c
> index 90e7b2f345e5..d5730a50b303 100644
> --- a/fs/fuse/virtio_fs.c
> +++ b/fs/fuse/virtio_fs.c
> @@ -63,6 +63,55 @@ static inline struct fuse_pqueue *vq_to_fpq(struct virtqueue *vq)
>  	return &vq_to_fsvq(vq)->fud->pq;
>  }
>  
> +static void virtio_fs_drain_queue(struct virtio_fs_vq *fsvq)
> +{
> +	WARN_ON(fsvq->in_flight < 0);
> +
> +	/* Wait for in flight requests to finish.*/

blank space missed after *finish.*.

> +	while (1) {
> +		spin_lock(&fsvq->lock);
> +		if (!fsvq->in_flight) {
> +			spin_unlock(&fsvq->lock);
> +			break;
> +		}
> +		spin_unlock(&fsvq->lock);
> +		usleep_range(1000, 2000);
> +	}
> +
> +	flush_work(&fsvq->done_work);
> +	flush_delayed_work(&fsvq->dispatch_work);
> +}
> +
> +static inline void drain_hiprio_queued_reqs(struct virtio_fs_vq *fsvq)

Should we add *virtio_fs* prefix for this function? And I wonder if
there are only forget reqs to drain? Maybe we should call it
*virtio_fs_drain_queued_forget_reqs* or someone containing *forget_reqs*.

Thanks,
Jun

> +{
> +	struct virtio_fs_forget *forget;
> +
> +	spin_lock(&fsvq->lock);
> +	while (1) {
> +		forget = list_first_entry_or_null(&fsvq->queued_reqs,
> +						struct virtio_fs_forget, list);
> +		if (!forget)
> +			break;
> +		list_del(&forget->list);
> +		kfree(forget);
> +	}
> +	spin_unlock(&fsvq->lock);
> +}
> +
> +static void virtio_fs_drain_all_queues(struct virtio_fs *fs)
> +{
> +	struct virtio_fs_vq *fsvq;
> +	int i;
> +
> +	for (i = 0; i < fs->nvqs; i++) {
> +		fsvq = &fs->vqs[i];
> +		if (i == VQ_HIPRIO)
> +			drain_hiprio_queued_reqs(fsvq);
> +
> +		virtio_fs_drain_queue(fsvq);
> +	}
> +}
> +
>  /* Add a new instance to the list or return -EEXIST if tag name exists*/
>  static int virtio_fs_add_instance(struct virtio_fs *fs)
>  {
> @@ -511,6 +560,7 @@ static void virtio_fs_remove(struct virtio_device *vdev)
>  	struct virtio_fs *fs = vdev->priv;
>  
>  	virtio_fs_stop_all_queues(fs);
> +	virtio_fs_drain_all_queues(fs);
>  	vdev->config->reset(vdev);
>  	virtio_fs_cleanup_vqs(vdev, fs);
>  
> @@ -865,37 +915,6 @@ __releases(fiq->waitq.lock)
>  	}
>  }
>  
> -static void virtio_fs_flush_hiprio_queue(struct virtio_fs_vq *fsvq)
> -{
> -	struct virtio_fs_forget *forget;
> -
> -	WARN_ON(fsvq->in_flight < 0);
> -
> -	/* Go through pending forget requests and free them */
> -	spin_lock(&fsvq->lock);
> -	while (1) {
> -		forget = list_first_entry_or_null(&fsvq->queued_reqs,
> -					struct virtio_fs_forget, list);
> -		if (!forget)
> -			break;
> -		list_del(&forget->list);
> -		kfree(forget);
> -	}
> -
> -	spin_unlock(&fsvq->lock);
> -
> -	/* Wait for in flight requests to finish.*/
> -	while (1) {
> -		spin_lock(&fsvq->lock);
> -		if (!fsvq->in_flight) {
> -			spin_unlock(&fsvq->lock);
> -			break;
> -		}
> -		spin_unlock(&fsvq->lock);
> -		usleep_range(1000, 2000);
> -	}
> -}
> -
>  const static struct fuse_iqueue_ops virtio_fs_fiq_ops = {
>  	.wake_forget_and_unlock		= virtio_fs_wake_forget_and_unlock,
>  	.wake_interrupt_and_unlock	= virtio_fs_wake_interrupt_and_unlock,
> @@ -988,7 +1007,7 @@ static void virtio_kill_sb(struct super_block *sb)
>  	spin_lock(&fsvq->lock);
>  	fsvq->connected = false;
>  	spin_unlock(&fsvq->lock);
> -	virtio_fs_flush_hiprio_queue(fsvq);
> +	virtio_fs_drain_all_queues(vfs);
>  
>  	fuse_kill_sb_anon(sb);
>  	virtio_fs_free_devs(vfs);
> 

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [Virtio-fs] [PATCH 15/18] virtiofs: Make virtio_fs object refcounted
  2019-09-05 19:48 ` [PATCH 15/18] virtiofs: Make virtio_fs object refcounted Vivek Goyal
  2019-09-06 12:03   ` Stefan Hajnoczi
@ 2019-09-08 11:10   ` piaojun
  2019-09-09 15:35     ` Vivek Goyal
  1 sibling, 1 reply; 62+ messages in thread
From: piaojun @ 2019-09-08 11:10 UTC (permalink / raw)
  To: Vivek Goyal, linux-fsdevel, virtualization, miklos
  Cc: mst, linux-kernel, virtio-fs



On 2019/9/6 3:48, Vivek Goyal wrote:
> This object is used both by fuse_connection as well virt device. So make
> this object reference counted and that makes it easy to define life cycle
> of the object.
> 
> Now deivce can be removed while filesystem is still mounted. This will
> cleanup all the virtqueues but virtio_fs object will still be around and
> will be cleaned when filesystem is unmounted and sb/fc drops its reference.
> 
> Removing a device also stops all virt queues and any new reuqest gets
> error -ENOTCONN. All existing in flight requests are drained before
> ->remove returns.
> 
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
>  fs/fuse/virtio_fs.c | 52 +++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 43 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c
> index 01bbf2c0e144..29ec2f5bbbe2 100644
> --- a/fs/fuse/virtio_fs.c
> +++ b/fs/fuse/virtio_fs.c
> @@ -37,6 +37,7 @@ struct virtio_fs_vq {
>  
>  /* A virtio-fs device instance */
>  struct virtio_fs {
> +	struct kref refcount;
>  	struct list_head list;    /* on virtio_fs_instances */
>  	char *tag;
>  	struct virtio_fs_vq *vqs;
> @@ -63,6 +64,27 @@ static inline struct fuse_pqueue *vq_to_fpq(struct virtqueue *vq)
>  	return &vq_to_fsvq(vq)->fud->pq;
>  }
>  
> +static void release_virtiofs_obj(struct kref *ref)
> +{
> +	struct virtio_fs *vfs = container_of(ref, struct virtio_fs, refcount);
> +
> +	kfree(vfs->vqs);
> +	kfree(vfs);
> +}
> +
> +static void virtio_fs_put(struct virtio_fs *fs)
> +{
> +	mutex_lock(&virtio_fs_mutex);
> +	kref_put(&fs->refcount, release_virtiofs_obj);
> +	mutex_unlock(&virtio_fs_mutex);
> +}
> +
> +static void virtio_fs_put(struct fuse_iqueue *fiq)
> +{
> +	struct virtio_fs *vfs = fiq->priv;
> +	virtiofs_put(vfs);
> +}

It's a little confusing that virtiofs_put() looks like virtiofs_put(),
and could we use __virtio_fs_put to replace virtio_fs_put?

Thanks,
Jun

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [Virtio-fs] [PATCH 16/18] virtiofs: Use virtio_fs_mutex for races w.r.t ->remove and mount path
  2019-09-05 19:48 ` [PATCH 16/18] virtiofs: Use virtio_fs_mutex for races w.r.t ->remove and mount path Vivek Goyal
  2019-09-06 12:05   ` Stefan Hajnoczi
@ 2019-09-08 11:19   ` piaojun
  1 sibling, 0 replies; 62+ messages in thread
From: piaojun @ 2019-09-08 11:19 UTC (permalink / raw)
  To: Vivek Goyal, linux-fsdevel, virtualization, miklos
  Cc: mst, linux-kernel, virtio-fs



On 2019/9/6 3:48, Vivek Goyal wrote:
> It is possible that a mount is in progress and device is being removed at
> the same time. Use virtio_fs_mutex to avoid races.
> 
> This also takes care of bunch of races and removes some TODO items.
> 
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
>  fs/fuse/virtio_fs.c | 32 ++++++++++++++++++++++----------
>  1 file changed, 22 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c
> index 29ec2f5bbbe2..c483482185b6 100644
> --- a/fs/fuse/virtio_fs.c
> +++ b/fs/fuse/virtio_fs.c
> @@ -13,7 +13,9 @@
>  #include <linux/highmem.h>
>  #include "fuse_i.h"
>  
> -/* List of virtio-fs device instances and a lock for the list */
> +/* List of virtio-fs device instances and a lock for the list. Also provides
> + * mutual exclusion in device removal and mounting path
> + */
>  static DEFINE_MUTEX(virtio_fs_mutex);
>  static LIST_HEAD(virtio_fs_instances);
>  
> @@ -72,17 +74,19 @@ static void release_virtiofs_obj(struct kref *ref)
>  	kfree(vfs);
>  }
>  
> +/* Make sure virtiofs_mutex is held */

Typo? virtiofs_mutex->virtio_fs_mutex

Jun

>  static void virtiofs_put(struct virtio_fs *fs)
>  {
> -	mutex_lock(&virtio_fs_mutex);
>  	kref_put(&fs->refcount, release_virtiofs_obj);
> -	mutex_unlock(&virtio_fs_mutex);
>  }
>  
>  static void virtio_fs_put(struct fuse_iqueue *fiq)
>  {
>  	struct virtio_fs *vfs = fiq->priv;
> +
> +	mutex_lock(&virtio_fs_mutex);
>  	virtiofs_put(vfs);
> +	mutex_unlock(&virtio_fs_mutex);
>  }
>  
>  static void virtio_fs_drain_queue(struct virtio_fs_vq *fsvq)
> @@ -596,9 +600,8 @@ static void virtio_fs_remove(struct virtio_device *vdev)
>  	struct virtio_fs *fs = vdev->priv;
>  
>  	mutex_lock(&virtio_fs_mutex);
> +	/* This device is going away. No one should get new reference */
>  	list_del_init(&fs->list);
> -	mutex_unlock(&virtio_fs_mutex);
> -
>  	virtio_fs_stop_all_queues(fs);
>  	virtio_fs_drain_all_queues(fs);
>  	vdev->config->reset(vdev);
> @@ -607,6 +610,7 @@ static void virtio_fs_remove(struct virtio_device *vdev)
>  	vdev->priv = NULL;
>  	/* Put device reference on virtio_fs object */
>  	virtiofs_put(fs);
> +	mutex_unlock(&virtio_fs_mutex);
>  }
>  
>  #ifdef CONFIG_PM_SLEEP
> @@ -978,10 +982,15 @@ static int virtio_fs_fill_super(struct super_block *sb)
>  		.no_force_umount = true,
>  	};
>  
> -	/* TODO lock */
> -	if (fs->vqs[VQ_REQUEST].fud) {
> -		pr_err("virtio-fs: device already in use\n");
> -		err = -EBUSY;
> +	mutex_lock(&virtio_fs_mutex);
> +
> +	/* After holding mutex, make sure virtiofs device is still there.
> +	 * Though we are holding a refernce to it, drive ->remove might
> +	 * still have cleaned up virtual queues. In that case bail out.
> +	 */
> +	err = -EINVAL;
> +	if (list_empty(&fs->list)) {
> +		pr_info("virtio-fs: tag <%s> not found\n", fs->tag);
>  		goto err;
>  	}
>  
> @@ -1007,7 +1016,6 @@ static int virtio_fs_fill_super(struct super_block *sb)
>  
>  	fc = fs->vqs[VQ_REQUEST].fud->fc;
>  
> -	/* TODO take fuse_mutex around this loop? */
>  	for (i = 0; i < fs->nvqs; i++) {
>  		struct virtio_fs_vq *fsvq = &fs->vqs[i];
>  
> @@ -1020,6 +1028,7 @@ static int virtio_fs_fill_super(struct super_block *sb)
>  	/* Previous unmount will stop all queues. Start these again */
>  	virtio_fs_start_all_queues(fs);
>  	fuse_send_init(fc, init_req);
> +	mutex_unlock(&virtio_fs_mutex);
>  	return 0;
>  
>  err_free_init_req:
> @@ -1027,6 +1036,7 @@ static int virtio_fs_fill_super(struct super_block *sb)
>  err_free_fuse_devs:
>  	virtio_fs_free_devs(fs);
>  err:
> +	mutex_unlock(&virtio_fs_mutex);
>  	return err;
>  }
>  
> @@ -1100,7 +1110,9 @@ static int virtio_fs_get_tree(struct fs_context *fsc)
>  
>  	fc = kzalloc(sizeof(struct fuse_conn), GFP_KERNEL);
>  	if (!fc) {
> +		mutex_lock(&virtio_fs_mutex);
>  		virtiofs_put(fs);
> +		mutex_unlock(&virtio_fs_mutex);
>  		return -ENOMEM;
>  	}
>  
> 

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [Virtio-fs] [PATCH 00/18] virtiofs: Fix various races and cleanups round 1
  2019-09-06 11:52     ` Miklos Szeredi
  2019-09-06 12:08       ` Vivek Goyal
@ 2019-09-08 11:53       ` piaojun
  2019-09-09 16:14         ` Stefan Hajnoczi
  1 sibling, 1 reply; 62+ messages in thread
From: piaojun @ 2019-09-08 11:53 UTC (permalink / raw)
  To: Miklos Szeredi, Stefan Hajnoczi
  Cc: Michael S. Tsirkin, linux-kernel, virtio-fs, linux-fsdevel,
	virtualization, Vivek Goyal



On 2019/9/6 19:52, Miklos Szeredi wrote:
> On Fri, Sep 6, 2019 at 12:36 PM Stefan Hajnoczi <stefanha@redhat.com> wrote:
>>
>> On Fri, Sep 06, 2019 at 10:15:14AM +0200, Miklos Szeredi wrote:
>>> On Thu, Sep 5, 2019 at 9:49 PM Vivek Goyal <vgoyal@redhat.com> wrote:
>>>>
>>>> Hi,
>>>>
>>>> Michael Tsirkin pointed out issues w.r.t various locking related TODO
>>>> items and races w.r.t device removal.
>>>>
>>>> In this first round of cleanups, I have taken care of most pressing
>>>> issues.
>>>>
>>>> These patches apply on top of following.
>>>>
>>>> git://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/fuse.git#virtiofs-v4
>>>>
>>>> I have tested these patches with mount/umount and device removal using
>>>> qemu monitor. For example.
>>>
>>> Is device removal mandatory?  Can't this be made a non-removable
>>> device?  Is there a good reason why removing the virtio-fs device
>>> makes sense?
>>
>> Hot plugging and unplugging virtio PCI adapters is common.  I'd very
>> much like removal to work from the beginning.
> 
> Can you give an example use case?

I think VirtFS migration need hot plugging, or it may cause QEMU crash
or some problems.

Thanks,
Jun

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [Virtio-fs] [PATCH 15/18] virtiofs: Make virtio_fs object refcounted
  2019-09-08 11:10   ` [Virtio-fs] " piaojun
@ 2019-09-09 15:35     ` Vivek Goyal
  0 siblings, 0 replies; 62+ messages in thread
From: Vivek Goyal @ 2019-09-09 15:35 UTC (permalink / raw)
  To: piaojun
  Cc: linux-fsdevel, virtualization, miklos, mst, linux-kernel, virtio-fs

On Sun, Sep 08, 2019 at 07:10:03PM +0800, piaojun wrote:
> 
> 
> On 2019/9/6 3:48, Vivek Goyal wrote:
> > This object is used both by fuse_connection as well virt device. So make
> > this object reference counted and that makes it easy to define life cycle
> > of the object.
> > 
> > Now deivce can be removed while filesystem is still mounted. This will
> > cleanup all the virtqueues but virtio_fs object will still be around and
> > will be cleaned when filesystem is unmounted and sb/fc drops its reference.
> > 
> > Removing a device also stops all virt queues and any new reuqest gets
> > error -ENOTCONN. All existing in flight requests are drained before
> > ->remove returns.
> > 
> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > ---
> >  fs/fuse/virtio_fs.c | 52 +++++++++++++++++++++++++++++++++++++--------
> >  1 file changed, 43 insertions(+), 9 deletions(-)
> > 
> > diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c
> > index 01bbf2c0e144..29ec2f5bbbe2 100644
> > --- a/fs/fuse/virtio_fs.c
> > +++ b/fs/fuse/virtio_fs.c
> > @@ -37,6 +37,7 @@ struct virtio_fs_vq {
> >  
> >  /* A virtio-fs device instance */
> >  struct virtio_fs {
> > +	struct kref refcount;
> >  	struct list_head list;    /* on virtio_fs_instances */
> >  	char *tag;
> >  	struct virtio_fs_vq *vqs;
> > @@ -63,6 +64,27 @@ static inline struct fuse_pqueue *vq_to_fpq(struct virtqueue *vq)
> >  	return &vq_to_fsvq(vq)->fud->pq;
> >  }
> >  
> > +static void release_virtiofs_obj(struct kref *ref)
> > +{
> > +	struct virtio_fs *vfs = container_of(ref, struct virtio_fs, refcount);
> > +
> > +	kfree(vfs->vqs);
> > +	kfree(vfs);
> > +}
> > +
> > +static void virtio_fs_put(struct virtio_fs *fs)
> > +{
> > +	mutex_lock(&virtio_fs_mutex);
> > +	kref_put(&fs->refcount, release_virtiofs_obj);
> > +	mutex_unlock(&virtio_fs_mutex);
> > +}
> > +
> > +static void virtio_fs_put(struct fuse_iqueue *fiq)
> > +{
> > +	struct virtio_fs *vfs = fiq->priv;
> > +	virtiofs_put(vfs);
> > +}
> 
> It's a little confusing that virtiofs_put() looks like virtiofs_put(),
> and could we use __virtio_fs_put to replace virtio_fs_put?

Fixed this in follow up patch I posted.

https://www.redhat.com/archives/virtio-fs/2019-September/msg00091.html

Vivek

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH 08/18] virtiofs: Drain all pending requests during ->remove time
  2019-09-06 14:18       ` Michael S. Tsirkin
@ 2019-09-09 16:10         ` Stefan Hajnoczi
  0 siblings, 0 replies; 62+ messages in thread
From: Stefan Hajnoczi @ 2019-09-09 16:10 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: Vivek Goyal, linux-fsdevel, virtualization, miklos, linux-kernel,
	virtio-fs, dgilbert

[-- Attachment #1: Type: text/plain, Size: 1350 bytes --]

On Fri, Sep 06, 2019 at 10:18:49AM -0400, Michael S. Tsirkin wrote:
> On Fri, Sep 06, 2019 at 10:17:05AM -0400, Vivek Goyal wrote:
> > On Fri, Sep 06, 2019 at 11:52:10AM +0100, Stefan Hajnoczi wrote:
> > > On Thu, Sep 05, 2019 at 03:48:49PM -0400, Vivek Goyal wrote:
> > > > +static void virtio_fs_drain_queue(struct virtio_fs_vq *fsvq)
> > > > +{
> > > > +	WARN_ON(fsvq->in_flight < 0);
> > > > +
> > > > +	/* Wait for in flight requests to finish.*/
> > > > +	while (1) {
> > > > +		spin_lock(&fsvq->lock);
> > > > +		if (!fsvq->in_flight) {
> > > > +			spin_unlock(&fsvq->lock);
> > > > +			break;
> > > > +		}
> > > > +		spin_unlock(&fsvq->lock);
> > > > +		usleep_range(1000, 2000);
> > > > +	}
> > > 
> > > I think all contexts that call this allow sleeping so we could avoid
> > > usleep here.
> > 
> > usleep_range() is supposed to be used from non-atomic context.
> > 
> > https://github.com/torvalds/linux/blob/master/Documentation/timers/timers-howto.rst
> > 
> > What construct you are thinking of?
> > 
> > Vivek
> 
> completion + signal on vq callback?

Yes.  Time-based sleep() is sub-optimal because we could wake up exactly
when in_flight is decremented from the vq callback.  This avoids
unnecessary sleep wakeups and the extra time spent sleeping after
in_flight has been decremented.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH 15/18] virtiofs: Make virtio_fs object refcounted
  2019-09-06 13:50     ` Vivek Goyal
@ 2019-09-09 16:12       ` Stefan Hajnoczi
  0 siblings, 0 replies; 62+ messages in thread
From: Stefan Hajnoczi @ 2019-09-09 16:12 UTC (permalink / raw)
  To: Vivek Goyal
  Cc: linux-fsdevel, virtualization, miklos, linux-kernel, virtio-fs,
	dgilbert, mst

[-- Attachment #1: Type: text/plain, Size: 7600 bytes --]

On Fri, Sep 06, 2019 at 09:50:32AM -0400, Vivek Goyal wrote:
> On Fri, Sep 06, 2019 at 01:03:09PM +0100, Stefan Hajnoczi wrote:
> > On Thu, Sep 05, 2019 at 03:48:56PM -0400, Vivek Goyal wrote:
> > > This object is used both by fuse_connection as well virt device. So make
> > > this object reference counted and that makes it easy to define life cycle
> > > of the object.
> > > 
> > > Now deivce can be removed while filesystem is still mounted. This will
> > > cleanup all the virtqueues but virtio_fs object will still be around and
> > > will be cleaned when filesystem is unmounted and sb/fc drops its reference.
> > > 
> > > Removing a device also stops all virt queues and any new reuqest gets
> > > error -ENOTCONN. All existing in flight requests are drained before
> > > ->remove returns.
> > > 
> > > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > > ---
> > >  fs/fuse/virtio_fs.c | 52 +++++++++++++++++++++++++++++++++++++--------
> > >  1 file changed, 43 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c
> > > index 01bbf2c0e144..29ec2f5bbbe2 100644
> > > --- a/fs/fuse/virtio_fs.c
> > > +++ b/fs/fuse/virtio_fs.c
> > > @@ -37,6 +37,7 @@ struct virtio_fs_vq {
> > >  
> > >  /* A virtio-fs device instance */
> > >  struct virtio_fs {
> > > +	struct kref refcount;
> > >  	struct list_head list;    /* on virtio_fs_instances */
> > >  	char *tag;
> > >  	struct virtio_fs_vq *vqs;
> > > @@ -63,6 +64,27 @@ static inline struct fuse_pqueue *vq_to_fpq(struct virtqueue *vq)
> > >  	return &vq_to_fsvq(vq)->fud->pq;
> > >  }
> > >  
> > > +static void release_virtiofs_obj(struct kref *ref)
> > > +{
> > > +	struct virtio_fs *vfs = container_of(ref, struct virtio_fs, refcount);
> > > +
> > > +	kfree(vfs->vqs);
> > > +	kfree(vfs);
> > > +}
> > > +
> > > +static void virtiofs_put(struct virtio_fs *fs)
> > 
> > Why do the two function names above contain "virtiofs" instead
> > of "virtio_fs"?  I'm not sure if this is intentional and is supposed to
> > mean something, but it's confusing.
> > 
> > > +{
> > > +	mutex_lock(&virtio_fs_mutex);
> > > +	kref_put(&fs->refcount, release_virtiofs_obj);
> > > +	mutex_unlock(&virtio_fs_mutex);
> > > +}
> > > +
> > > +static void virtio_fs_put(struct fuse_iqueue *fiq)
> > 
> > Minor issue: this function name is confusingly similar to
> > virtiofs_put().  Please rename to virtio_fs_fiq_put().
> 
> Fixed with ->release semantics. Replaced "virtiofs" with "virtio_fs".
> 
> 
> Subject: virtiofs: Make virtio_fs object refcounted
> 
> This object is used both by fuse_connection as well virt device. So make
> this object reference counted and that makes it easy to define life cycle
> of the object. 
> 
> Now deivce can be removed while filesystem is still mounted. This will
> cleanup all the virtqueues but virtio_fs object will still be around and
> will be cleaned when filesystem is unmounted and sb/fc drops its reference.
> 
> Removing a device also stops all virt queues and any new reuqest gets
> error -ENOTCONN. All existing in flight requests are drained before
> ->remove returns.
> 
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
>  fs/fuse/virtio_fs.c |   52 +++++++++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 43 insertions(+), 9 deletions(-)
> 
> Index: rhvgoyal-linux-fuse/fs/fuse/virtio_fs.c
> ===================================================================
> --- rhvgoyal-linux-fuse.orig/fs/fuse/virtio_fs.c	2019-09-06 09:24:21.177245246 -0400
> +++ rhvgoyal-linux-fuse/fs/fuse/virtio_fs.c	2019-09-06 09:40:53.309245246 -0400
> @@ -37,6 +37,7 @@ struct virtio_fs_vq {
>  
>  /* A virtio-fs device instance */
>  struct virtio_fs {
> +	struct kref refcount;
>  	struct list_head list;    /* on virtio_fs_instances */
>  	char *tag;
>  	struct virtio_fs_vq *vqs;
> @@ -63,6 +64,27 @@ static inline struct fuse_pqueue *vq_to_
>  	return &vq_to_fsvq(vq)->fud->pq;
>  }
>  
> +static void release_virtio_fs_obj(struct kref *ref)
> +{
> +	struct virtio_fs *vfs = container_of(ref, struct virtio_fs, refcount);
> +
> +	kfree(vfs->vqs);
> +	kfree(vfs);
> +}
> +
> +static void virtio_fs_put(struct virtio_fs *fs)
> +{
> +	mutex_lock(&virtio_fs_mutex);
> +	kref_put(&fs->refcount, release_virtio_fs_obj);
> +	mutex_unlock(&virtio_fs_mutex);
> +}
> +
> +static void virtio_fs_fiq_release(struct fuse_iqueue *fiq)
> +{
> +	struct virtio_fs *vfs = fiq->priv;
> +	virtio_fs_put(vfs);
> +}
> +
>  static void virtio_fs_drain_queue(struct virtio_fs_vq *fsvq)
>  {
>  	WARN_ON(fsvq->in_flight < 0);
> @@ -156,8 +178,10 @@ static struct virtio_fs *virtio_fs_find_
>  	mutex_lock(&virtio_fs_mutex);
>  
>  	list_for_each_entry(fs, &virtio_fs_instances, list) {
> -		if (strcmp(fs->tag, tag) == 0)
> +		if (strcmp(fs->tag, tag) == 0) {
> +			kref_get(&fs->refcount);
>  			goto found;
> +		}
>  	}
>  
>  	fs = NULL; /* not found */
> @@ -519,6 +543,7 @@ static int virtio_fs_probe(struct virtio
>  	fs = kzalloc(sizeof(*fs), GFP_KERNEL);
>  	if (!fs)
>  		return -ENOMEM;
> +	kref_init(&fs->refcount);
>  	vdev->priv = fs;
>  
>  	ret = virtio_fs_read_tag(vdev, fs);
> @@ -570,18 +595,18 @@ static void virtio_fs_remove(struct virt
>  {
>  	struct virtio_fs *fs = vdev->priv;
>  
> +	mutex_lock(&virtio_fs_mutex);
> +	list_del_init(&fs->list);
> +	mutex_unlock(&virtio_fs_mutex);
> +
>  	virtio_fs_stop_all_queues(fs);
>  	virtio_fs_drain_all_queues(fs);
>  	vdev->config->reset(vdev);
>  	virtio_fs_cleanup_vqs(vdev, fs);
>  
> -	mutex_lock(&virtio_fs_mutex);
> -	list_del(&fs->list);
> -	mutex_unlock(&virtio_fs_mutex);
> -
>  	vdev->priv = NULL;
> -	kfree(fs->vqs);
> -	kfree(fs);
> +	/* Put device reference on virtio_fs object */
> +	virtio_fs_put(fs);
>  }
>  
>  #ifdef CONFIG_PM_SLEEP
> @@ -932,6 +957,7 @@ const static struct fuse_iqueue_ops virt
>  	.wake_forget_and_unlock		= virtio_fs_wake_forget_and_unlock,
>  	.wake_interrupt_and_unlock	= virtio_fs_wake_interrupt_and_unlock,
>  	.wake_pending_and_unlock	= virtio_fs_wake_pending_and_unlock,
> +	.release			= virtio_fs_fiq_release,
>  };
>  
>  static int virtio_fs_fill_super(struct super_block *sb)
> @@ -1026,7 +1052,9 @@ static void virtio_kill_sb(struct super_
>  	fuse_kill_sb_anon(sb);
>  
>  	/* fuse_kill_sb_anon() must have sent destroy. Stop all queues
> -	 * and drain one more time and free fuse devices.
> +	 * and drain one more time and free fuse devices. Freeing fuse
> +	 * devices will drop their reference on fuse_conn and that in
> +	 * turn will drop its reference on virtio_fs object.
>  	 */
>  	virtio_fs_stop_all_queues(vfs);
>  	virtio_fs_drain_all_queues(vfs);
> @@ -1060,6 +1088,10 @@ static int virtio_fs_get_tree(struct fs_
>  	struct fuse_conn *fc;
>  	int err;
>  
> +	/* This gets a reference on virtio_fs object. This ptr gets installed
> +	 * in fc->iq->priv. Once fuse_conn is going away, it calls ->put()
> +	 * to drop the reference to this object.
> +	 */
>  	fs = virtio_fs_find_instance(fsc->source);
>  	if (!fs) {
>  		pr_info("virtio-fs: tag <%s> not found\n", fsc->source);
> @@ -1067,8 +1099,10 @@ static int virtio_fs_get_tree(struct fs_
>  	}
>  
>  	fc = kzalloc(sizeof(struct fuse_conn), GFP_KERNEL);
> -	if (!fc)
> +	if (!fc) {
> +		virtio_fs_put(fs);
>  		return -ENOMEM;
> +	}
>  
>  	fuse_conn_init(fc, get_user_ns(current_user_ns()), &virtio_fs_fiq_ops,
>  		       fs);

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 484 bytes --]

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH 16/18] virtiofs: Use virtio_fs_mutex for races w.r.t ->remove and mount path
  2019-09-06 13:51     ` Vivek Goyal
@ 2019-09-09 16:13       ` Stefan Hajnoczi
  0 siblings, 0 replies; 62+ messages in thread
From: Stefan Hajnoczi @ 2019-09-09 16:13 UTC (permalink / raw)
  To: Vivek Goyal
  Cc: linux-fsdevel, virtualization, miklos, linux-kernel, virtio-fs,
	dgilbert, mst

[-- Attachment #1: Type: text/plain, Size: 5023 bytes --]

On Fri, Sep 06, 2019 at 09:51:31AM -0400, Vivek Goyal wrote:
> On Fri, Sep 06, 2019 at 01:05:34PM +0100, Stefan Hajnoczi wrote:
> > On Thu, Sep 05, 2019 at 03:48:57PM -0400, Vivek Goyal wrote:
> > > It is possible that a mount is in progress and device is being removed at
> > > the same time. Use virtio_fs_mutex to avoid races.
> > > 
> > > This also takes care of bunch of races and removes some TODO items.
> > > 
> > > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > > ---
> > >  fs/fuse/virtio_fs.c | 32 ++++++++++++++++++++++----------
> > >  1 file changed, 22 insertions(+), 10 deletions(-)
> > 
> > Let's move to a per-device mutex in the future.  That way a single
> > device that fails to drain/complete requests will not hang all other
> > virtio-fs instances.  This is fine for now.
> 
> Good point. For now I updated the patch so that it applies cleanly
> after previous two patches changed.
> 
> Subject: virtiofs: Use virtio_fs_mutex for races w.r.t ->remove and mount path
> 
> It is possible that a mount is in progress and device is being removed at
> the same time. Use virtio_fs_mutex to avoid races. 
> 
> This also takes care of bunch of races and removes some TODO items.
> 
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
>  fs/fuse/virtio_fs.c |   32 ++++++++++++++++++++++----------
>  1 file changed, 22 insertions(+), 10 deletions(-)
> 
> Index: rhvgoyal-linux-fuse/fs/fuse/virtio_fs.c
> ===================================================================
> --- rhvgoyal-linux-fuse.orig/fs/fuse/virtio_fs.c	2019-09-06 09:40:53.309245246 -0400
> +++ rhvgoyal-linux-fuse/fs/fuse/virtio_fs.c	2019-09-06 09:43:25.335245246 -0400
> @@ -13,7 +13,9 @@
>  #include <linux/highmem.h>
>  #include "fuse_i.h"
>  
> -/* List of virtio-fs device instances and a lock for the list */
> +/* List of virtio-fs device instances and a lock for the list. Also provides
> + * mutual exclusion in device removal and mounting path
> + */
>  static DEFINE_MUTEX(virtio_fs_mutex);
>  static LIST_HEAD(virtio_fs_instances);
>  
> @@ -72,17 +74,19 @@ static void release_virtio_fs_obj(struct
>  	kfree(vfs);
>  }
>  
> +/* Make sure virtiofs_mutex is held */
>  static void virtio_fs_put(struct virtio_fs *fs)
>  {
> -	mutex_lock(&virtio_fs_mutex);
>  	kref_put(&fs->refcount, release_virtio_fs_obj);
> -	mutex_unlock(&virtio_fs_mutex);
>  }
>  
>  static void virtio_fs_fiq_release(struct fuse_iqueue *fiq)
>  {
>  	struct virtio_fs *vfs = fiq->priv;
> +
> +	mutex_lock(&virtio_fs_mutex);
>  	virtio_fs_put(vfs);
> +	mutex_unlock(&virtio_fs_mutex);
>  }
>  
>  static void virtio_fs_drain_queue(struct virtio_fs_vq *fsvq)
> @@ -596,9 +600,8 @@ static void virtio_fs_remove(struct virt
>  	struct virtio_fs *fs = vdev->priv;
>  
>  	mutex_lock(&virtio_fs_mutex);
> +	/* This device is going away. No one should get new reference */
>  	list_del_init(&fs->list);
> -	mutex_unlock(&virtio_fs_mutex);
> -
>  	virtio_fs_stop_all_queues(fs);
>  	virtio_fs_drain_all_queues(fs);
>  	vdev->config->reset(vdev);
> @@ -607,6 +610,7 @@ static void virtio_fs_remove(struct virt
>  	vdev->priv = NULL;
>  	/* Put device reference on virtio_fs object */
>  	virtio_fs_put(fs);
> +	mutex_unlock(&virtio_fs_mutex);
>  }
>  
>  #ifdef CONFIG_PM_SLEEP
> @@ -978,10 +982,15 @@ static int virtio_fs_fill_super(struct s
>  		.no_force_umount = true,
>  	};
>  
> -	/* TODO lock */
> -	if (fs->vqs[VQ_REQUEST].fud) {
> -		pr_err("virtio-fs: device already in use\n");
> -		err = -EBUSY;
> +	mutex_lock(&virtio_fs_mutex);
> +
> +	/* After holding mutex, make sure virtiofs device is still there.
> +	 * Though we are holding a refernce to it, drive ->remove might
> +	 * still have cleaned up virtual queues. In that case bail out.
> +	 */
> +	err = -EINVAL;
> +	if (list_empty(&fs->list)) {
> +		pr_info("virtio-fs: tag <%s> not found\n", fs->tag);
>  		goto err;
>  	}
>  
> @@ -1007,7 +1016,6 @@ static int virtio_fs_fill_super(struct s
>  
>  	fc = fs->vqs[VQ_REQUEST].fud->fc;
>  
> -	/* TODO take fuse_mutex around this loop? */
>  	for (i = 0; i < fs->nvqs; i++) {
>  		struct virtio_fs_vq *fsvq = &fs->vqs[i];
>  
> @@ -1020,6 +1028,7 @@ static int virtio_fs_fill_super(struct s
>  	/* Previous unmount will stop all queues. Start these again */
>  	virtio_fs_start_all_queues(fs);
>  	fuse_send_init(fc, init_req);
> +	mutex_unlock(&virtio_fs_mutex);
>  	return 0;
>  
>  err_free_init_req:
> @@ -1027,6 +1036,7 @@ err_free_init_req:
>  err_free_fuse_devs:
>  	virtio_fs_free_devs(fs);
>  err:
> +	mutex_unlock(&virtio_fs_mutex);
>  	return err;
>  }
>  
> @@ -1100,7 +1110,9 @@ static int virtio_fs_get_tree(struct fs_
>  
>  	fc = kzalloc(sizeof(struct fuse_conn), GFP_KERNEL);
>  	if (!fc) {
> +		mutex_lock(&virtio_fs_mutex);
>  		virtio_fs_put(fs);
> +		mutex_unlock(&virtio_fs_mutex);
>  		return -ENOMEM;
>  	}
>  
> 

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [Virtio-fs] [PATCH 00/18] virtiofs: Fix various races and cleanups round 1
  2019-09-08 11:53       ` [Virtio-fs] " piaojun
@ 2019-09-09 16:14         ` Stefan Hajnoczi
  2019-09-09 16:18           ` Michael S. Tsirkin
  2019-09-09 23:52           ` piaojun
  0 siblings, 2 replies; 62+ messages in thread
From: Stefan Hajnoczi @ 2019-09-09 16:14 UTC (permalink / raw)
  To: piaojun
  Cc: Miklos Szeredi, Michael S. Tsirkin, linux-kernel, virtio-fs,
	linux-fsdevel, virtualization, Vivek Goyal

[-- Attachment #1: Type: text/plain, Size: 1564 bytes --]

On Sun, Sep 08, 2019 at 07:53:55PM +0800, piaojun wrote:
> 
> 
> On 2019/9/6 19:52, Miklos Szeredi wrote:
> > On Fri, Sep 6, 2019 at 12:36 PM Stefan Hajnoczi <stefanha@redhat.com> wrote:
> >>
> >> On Fri, Sep 06, 2019 at 10:15:14AM +0200, Miklos Szeredi wrote:
> >>> On Thu, Sep 5, 2019 at 9:49 PM Vivek Goyal <vgoyal@redhat.com> wrote:
> >>>>
> >>>> Hi,
> >>>>
> >>>> Michael Tsirkin pointed out issues w.r.t various locking related TODO
> >>>> items and races w.r.t device removal.
> >>>>
> >>>> In this first round of cleanups, I have taken care of most pressing
> >>>> issues.
> >>>>
> >>>> These patches apply on top of following.
> >>>>
> >>>> git://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/fuse.git#virtiofs-v4
> >>>>
> >>>> I have tested these patches with mount/umount and device removal using
> >>>> qemu monitor. For example.
> >>>
> >>> Is device removal mandatory?  Can't this be made a non-removable
> >>> device?  Is there a good reason why removing the virtio-fs device
> >>> makes sense?
> >>
> >> Hot plugging and unplugging virtio PCI adapters is common.  I'd very
> >> much like removal to work from the beginning.
> > 
> > Can you give an example use case?
> 
> I think VirtFS migration need hot plugging, or it may cause QEMU crash
> or some problems.

Live migration is currently unsupported.  Hot unplugging the virtio-fs
device would allow the guest to live migrate successfully, so it's a
useful feature to work around the missing live migration support.

Is this what you mean?

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [Virtio-fs] [PATCH 00/18] virtiofs: Fix various races and cleanups round 1
  2019-09-09 16:14         ` Stefan Hajnoczi
@ 2019-09-09 16:18           ` Michael S. Tsirkin
  2019-09-09 23:52           ` piaojun
  1 sibling, 0 replies; 62+ messages in thread
From: Michael S. Tsirkin @ 2019-09-09 16:18 UTC (permalink / raw)
  To: Stefan Hajnoczi
  Cc: piaojun, Miklos Szeredi, linux-kernel, virtio-fs, linux-fsdevel,
	virtualization, Vivek Goyal

On Mon, Sep 09, 2019 at 06:14:55PM +0200, Stefan Hajnoczi wrote:
> On Sun, Sep 08, 2019 at 07:53:55PM +0800, piaojun wrote:
> > 
> > 
> > On 2019/9/6 19:52, Miklos Szeredi wrote:
> > > On Fri, Sep 6, 2019 at 12:36 PM Stefan Hajnoczi <stefanha@redhat.com> wrote:
> > >>
> > >> On Fri, Sep 06, 2019 at 10:15:14AM +0200, Miklos Szeredi wrote:
> > >>> On Thu, Sep 5, 2019 at 9:49 PM Vivek Goyal <vgoyal@redhat.com> wrote:
> > >>>>
> > >>>> Hi,
> > >>>>
> > >>>> Michael Tsirkin pointed out issues w.r.t various locking related TODO
> > >>>> items and races w.r.t device removal.
> > >>>>
> > >>>> In this first round of cleanups, I have taken care of most pressing
> > >>>> issues.
> > >>>>
> > >>>> These patches apply on top of following.
> > >>>>
> > >>>> git://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/fuse.git#virtiofs-v4
> > >>>>
> > >>>> I have tested these patches with mount/umount and device removal using
> > >>>> qemu monitor. For example.
> > >>>
> > >>> Is device removal mandatory?  Can't this be made a non-removable
> > >>> device?  Is there a good reason why removing the virtio-fs device
> > >>> makes sense?
> > >>
> > >> Hot plugging and unplugging virtio PCI adapters is common.  I'd very
> > >> much like removal to work from the beginning.
> > > 
> > > Can you give an example use case?
> > 
> > I think VirtFS migration need hot plugging, or it may cause QEMU crash
> > or some problems.
> 
> Live migration is currently unsupported.  Hot unplugging the virtio-fs
> device would allow the guest to live migrate successfully, so it's a
> useful feature to work around the missing live migration support.
> 
> Is this what you mean?
> 
> Stefan

Exactly. That's what I also said. To add to that, hotplug can not be
negotiated, so it's not a feature we can easily add down the road if old
guests crash on unplug. Thus a driver that does not support unplug
should not claim to support removal.

-- 
MST

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [Virtio-fs] [PATCH 00/18] virtiofs: Fix various races and cleanups round 1
  2019-09-09 16:14         ` Stefan Hajnoczi
  2019-09-09 16:18           ` Michael S. Tsirkin
@ 2019-09-09 23:52           ` piaojun
  1 sibling, 0 replies; 62+ messages in thread
From: piaojun @ 2019-09-09 23:52 UTC (permalink / raw)
  To: Stefan Hajnoczi
  Cc: Miklos Szeredi, Michael S. Tsirkin, linux-kernel, virtio-fs,
	linux-fsdevel, virtualization, Vivek Goyal



On 2019/9/10 0:14, Stefan Hajnoczi wrote:
> On Sun, Sep 08, 2019 at 07:53:55PM +0800, piaojun wrote:
>>
>>
>> On 2019/9/6 19:52, Miklos Szeredi wrote:
>>> On Fri, Sep 6, 2019 at 12:36 PM Stefan Hajnoczi <stefanha@redhat.com> wrote:
>>>>
>>>> On Fri, Sep 06, 2019 at 10:15:14AM +0200, Miklos Szeredi wrote:
>>>>> On Thu, Sep 5, 2019 at 9:49 PM Vivek Goyal <vgoyal@redhat.com> wrote:
>>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> Michael Tsirkin pointed out issues w.r.t various locking related TODO
>>>>>> items and races w.r.t device removal.
>>>>>>
>>>>>> In this first round of cleanups, I have taken care of most pressing
>>>>>> issues.
>>>>>>
>>>>>> These patches apply on top of following.
>>>>>>
>>>>>> git://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/fuse.git#virtiofs-v4
>>>>>>
>>>>>> I have tested these patches with mount/umount and device removal using
>>>>>> qemu monitor. For example.
>>>>>
>>>>> Is device removal mandatory?  Can't this be made a non-removable
>>>>> device?  Is there a good reason why removing the virtio-fs device
>>>>> makes sense?
>>>>
>>>> Hot plugging and unplugging virtio PCI adapters is common.  I'd very
>>>> much like removal to work from the beginning.
>>>
>>> Can you give an example use case?
>>
>> I think VirtFS migration need hot plugging, or it may cause QEMU crash
>> or some problems.
> 
> Live migration is currently unsupported.  Hot unplugging the virtio-fs
> device would allow the guest to live migrate successfully, so it's a
> useful feature to work around the missing live migration support.
> 
> Is this what you mean?

Agreed, migration support is necessary for user, and hot
plugging/unplugging is also common for virtio device.

Jun

^ permalink raw reply	[flat|nested] 62+ messages in thread

end of thread, other threads:[~2019-09-09 23:52 UTC | newest]

Thread overview: 62+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-05 19:48 [PATCH 00/18] virtiofs: Fix various races and cleanups round 1 Vivek Goyal
2019-09-05 19:48 ` [PATCH 01/18] virtiofs: Remove request from processing list before calling end Vivek Goyal
2019-09-06 10:40   ` Stefan Hajnoczi
2019-09-05 19:48 ` [PATCH 02/18] virtiofs: Check whether hiprio queue is connected at submission time Vivek Goyal
2019-09-06 10:43   ` Stefan Hajnoczi
2019-09-05 19:48 ` [PATCH 03/18] virtiofs: Pass fsvq instead of vq as parameter to virtio_fs_enqueue_req Vivek Goyal
2019-09-06 10:44   ` Stefan Hajnoczi
2019-09-05 19:48 ` [PATCH 04/18] virtiofs: Check connected state for VQ_REQUEST queue as well Vivek Goyal
2019-09-06 10:45   ` Stefan Hajnoczi
2019-09-05 19:48 ` [PATCH 05/18] Maintain count of in flight requests for VQ_REQUEST queue Vivek Goyal
2019-09-06 10:46   ` Stefan Hajnoczi
2019-09-05 19:48 ` [PATCH 06/18] virtiofs: ->remove should not clean virtiofs fuse devices Vivek Goyal
2019-09-06 10:47   ` Stefan Hajnoczi
2019-09-05 19:48 ` [PATCH 07/18] virtiofs: Stop virtiofs queues when device is being removed Vivek Goyal
2019-09-06 10:47   ` Stefan Hajnoczi
2019-09-05 19:48 ` [PATCH 08/18] virtiofs: Drain all pending requests during ->remove time Vivek Goyal
2019-09-06 10:52   ` Stefan Hajnoczi
2019-09-06 14:17     ` Vivek Goyal
2019-09-06 14:18       ` Michael S. Tsirkin
2019-09-09 16:10         ` Stefan Hajnoczi
2019-09-08 10:11   ` [Virtio-fs] " piaojun
2019-09-05 19:48 ` [PATCH 09/18] virtiofs: Add an helper to start all the queues Vivek Goyal
2019-09-06 10:54   ` Stefan Hajnoczi
2019-09-05 19:48 ` [PATCH 10/18] virtiofs: Do not use device managed mem for virtio_fs and virtio_fs_vq Vivek Goyal
2019-09-06 10:56   ` Stefan Hajnoczi
2019-09-05 19:48 ` [PATCH 11/18] virtiofs: stop and drain queues after sending DESTROY Vivek Goyal
2019-09-06 11:50   ` Stefan Hajnoczi
2019-09-05 19:48 ` [PATCH 12/18] virtiofs: Use virtio_fs_free_devs() in error path Vivek Goyal
2019-09-06 11:51   ` Stefan Hajnoczi
2019-09-05 19:48 ` [PATCH 13/18] virtiofs: Do not access virtqueue in request submission path Vivek Goyal
2019-09-06 11:52   ` Stefan Hajnoczi
2019-09-05 19:48 ` [PATCH 14/18] virtiofs: Add a fuse_iqueue operation to put() reference Vivek Goyal
2019-09-06 12:00   ` Stefan Hajnoczi
2019-09-06 13:35     ` Vivek Goyal
2019-09-05 19:48 ` [PATCH 15/18] virtiofs: Make virtio_fs object refcounted Vivek Goyal
2019-09-06 12:03   ` Stefan Hajnoczi
2019-09-06 13:50     ` Vivek Goyal
2019-09-09 16:12       ` Stefan Hajnoczi
2019-09-08 11:10   ` [Virtio-fs] " piaojun
2019-09-09 15:35     ` Vivek Goyal
2019-09-05 19:48 ` [PATCH 16/18] virtiofs: Use virtio_fs_mutex for races w.r.t ->remove and mount path Vivek Goyal
2019-09-06 12:05   ` Stefan Hajnoczi
2019-09-06 13:51     ` Vivek Goyal
2019-09-09 16:13       ` Stefan Hajnoczi
2019-09-08 11:19   ` [Virtio-fs] " piaojun
2019-09-05 19:48 ` [PATCH 17/18] virtiofs: Remove TODO to quiesce/end_requests Vivek Goyal
2019-09-06 12:06   ` Stefan Hajnoczi
2019-09-05 19:48 ` [PATCH 18/18] virtiofs: Remove TODO item from virtio_fs_free_devs() Vivek Goyal
2019-09-06 12:06   ` Stefan Hajnoczi
2019-09-06  8:15 ` [PATCH 00/18] virtiofs: Fix various races and cleanups round 1 Miklos Szeredi
2019-09-06 10:36   ` Stefan Hajnoczi
2019-09-06 11:52     ` Miklos Szeredi
2019-09-06 12:08       ` Vivek Goyal
2019-09-06 13:55         ` Miklos Szeredi
2019-09-06 13:57         ` Michael S. Tsirkin
2019-09-06 14:11           ` Miklos Szeredi
2019-09-06 14:17             ` Michael S. Tsirkin
2019-09-08 11:53       ` [Virtio-fs] " piaojun
2019-09-09 16:14         ` Stefan Hajnoczi
2019-09-09 16:18           ` Michael S. Tsirkin
2019-09-09 23:52           ` piaojun
2019-09-06 13:37 ` Michael S. Tsirkin

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).